Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flush requests queue on opened push event #183

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

ajaysubra
Copy link
Contributor

Description

In order to keep engagement metric as up to date as possible we want to try and flush the queue whenever an opened push event request is enqueued.

Check List

  • Are you changing anything with the public API?
  • Have you tested this change on real device?
  • Are your changes backwards compatible with previous SDK Versions?
  • Have you added unit test coverage for your changes?
  • Have you verified that your changes are compatible with all the operating system version this SDK currently supports?

Manual Test Plan

Unit tests but also using a proxy -

  1. Increased the flush interval to something very high (100 secs)
  2. Enqueued various events and confirmed they didn't flush
  3. Enqueued a opened push event and noticed in the proxy that they all flushed.

@ajaysubra ajaysubra requested a review from a team as a code owner June 4, 2024 16:55
@ajaysubra ajaysubra requested a review from jordan-griffin June 4, 2024 16:55
we don't miss any user engagement events. In all other cases we will flush the queue
using the flush intervals defined above in `StateManagementConstants`
*/
return event.metric.name == .OpenedPush ? .task { .flushQueue } : .none
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main change.

@@ -501,7 +509,7 @@ class StateManagementTests: XCTestCase {
let initialState = INITILIZING_TEST_STATE()
let store = TestStore(initialState: initialState, reducer: KlaviyoReducer())

let event = Event(name: .OpenedPush)
let event = Event(name: .OpenedAppMetric)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to non opened push event since it doesn't matter for this test which event we use.

@@ -485,14 +485,22 @@ class StateManagementTests: XCTestCase {
// MARK: - Test enqueue event

@MainActor
func testEnqueueEvent() async throws {
func testEnqueueEvents() async throws {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this test to test all events and added an exception for opened push at the bottom.

@ajaysubra ajaysubra requested review from ndurell and evan-masseau June 4, 2024 16:57
Copy link
Contributor

@evan-masseau evan-masseau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks right to me

@ajaysubra ajaysubra merged commit 176e395 into master Jun 4, 2024
9 checks passed
@ajaysubra ajaysubra deleted the as/chnl-9274-flush-on-opened-event branch June 4, 2024 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants