Separation of API, State, and Side Effects #160
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 4 commits
April 22, 2024 10:19
Largely left tests alone except for adding a couple and adjusting setup to accommodate changes.
…pty string and null with identifiers. Added proper deserializing of the profile attributes store property
evan-masseau
commented
Apr 24, 2024
sdk/analytics/src/main/java/com/klaviyo/analytics/state/UserState.kt
Outdated
Show resolved
Hide resolved
evan-masseau
commented
Apr 25, 2024
| DevicePropertiesTest.mockDeviceProperties() | ||
| UserInfo.reset() | ||
| Klaviyo.initialize( |
There was a problem hiding this comment.
Had to adjust the setup for these tests because of how I changed UserInfo from an Object to a registered service that needs to be instantiated. The tests themselves are still effectively the same.
evan-masseau
commented
Apr 25, 2024
| @@ -43,7 +42,6 @@ internal class KlaviyoApiClientTest : BaseTest() { | |||
| private val flushIntervalOffline = 30_000L | |||
| private val queueDepth = 10 | |||
| private var postedJob: KlaviyoApiClient.NetworkRunnable? = null | |||
| private val staticClock = StaticClock(TIME, ISO_TIME) | |||
There was a problem hiding this comment.
needed this for other tests so i just lifted it into BaseTest
Don't need immutable profile to expose attributes, but would be good to expose single attribute getter
sdk/analytics/src/main/java/com/klaviyo/analytics/state/StateSideEffects.kt
Outdated
Show resolved
Hide resolved
sdk/analytics/src/main/java/com/klaviyo/analytics/state/StateSideEffects.kt
Outdated
Show resolved
Hide resolved
sdk/analytics/src/main/java/com/klaviyo/analytics/state/PersistentObservableProperty.kt
Outdated
Show resolved
Hide resolved
sdk/analytics/src/main/java/com/klaviyo/analytics/state/State.kt
Outdated
Show resolved
Hide resolved
sdk/analytics/src/main/java/com/klaviyo/analytics/state/StateSideEffects.kt
Outdated
Show resolved
Hide resolved
kennyklaviyo
approved these changes
Apr 30, 2024
evan-masseau
requested review from
a team and
ajaysubra
and removed request for
a team
dan-peluso
added a commit
that referenced
this pull request
Oct 23, 2024
* Separation of API, State, and Side Effects (#160) * Separation of API, State and Side Effects. Largely left tests alone except for adding a couple and adjusting setup to accommodate changes. * Use consistent nullability pattern so we aren't converting between empty string and null with identifiers. Added proper deserializing of the profile attributes store property * There is indeed a better way to do that * Renamed some of the new classes and added new tests for them * Doc blocks, minor cleanup, naming conventions * Fix nullability test Don't need immutable profile to expose attributes, but would be good to expose single attribute getter * First round of PR comments * missed another arg label --------- Co-authored-by: Evan Masseau <> * Set an alpha version number on this branch in case it gets pulled by a snapshot jitpack build * Remove deprecated lifecycle property (#156) Co-authored-by: Evan Masseau <> * Transition profile identifier keys to internal visibility (#155) Co-authored-by: Evan Masseau <> * Add Unregister Push Token Request (#162) * Publish old value of a state property (#164) Co-authored-by: Evan Masseau <> * Rename these properties so it is clear what true and false mean. Fix polarity of how we check background data (#166) Co-authored-by: Evan Masseau <> * Added tests for publishing of the old value (#165) * Added test coverage for publishing old value with change callback * Broadcast on reset, and tests * Reset as a cleanup step, not a setup step. --------- Co-authored-by: Evan Masseau <> * Trigger Unregister on API Key Change (#163) * Using token endpoint for profile requests when a push token is present in SDK state (#168) * making token request if token is present * minor refactor * fixed tests * added some tests * removed push state when resetting * updated tests * fixed side effects tests * removed unused comment * updated readme * finally fixed tests * Fix some setup/teardown in tests to take care of isolation issues. (#169) Co-authored-by: Evan Masseau <> --------- Co-authored-by: Evan C Masseau <[email protected]> * API Header Fix: Move attempt count increment to before we write request headers into the url connection. (#170) Co-authored-by: Evan Masseau <> * setProfileAttribute should accept any Serializable value not just String (#179) * changing profile attribute to accept serializable * updating for 3.0 release * logging incorrectly typed profile attribute * Concurrent network observer fix (#180) * using synchronized list for observers * adding concurreny safe structure to all observer instances * Refresh versions syntax for dependencies * removing concurrent suffix --------- Co-authored-by: Evan Masseau <> * Retry on 503 (#181) * Sending 503 through retry logic * removing version.properties unecessary changes * fixing http import * CHNL-6996 Proguard docs and consumer rules (#184) * Adding consumer rules and updating docs * progaurd changes * Introduce a de-dupe mechanism for push notifications (#177) * Introduce tag support so we have the option to de-dupe push notifications similar to how stock FCM sdk can. * adding constant id and null-checked notification tag --------- Co-authored-by: Evan Masseau <> Co-authored-by: Daniel Peluso <[email protected]> * CHNL-3990 Remove identifiers from state if format issues (#186) * Remove identifiers from state if API reports format errors * pr comments * removing extraneous decoder test * Exposing klaviyo SDK name and version privately (#185) * added klaviyo sdk name and version * Update sdk/analytics/src/main/java/com/klaviyo/analytics/DeviceProperties.kt Co-authored-by: dan-peluso <[email protected]> * fixed some formatting * updated the SDK name to be non lazy --------- Co-authored-by: dan-peluso <[email protected]> * Refactor singleton service initializers (#172) * Convert SystemClock to class * These shouldn't have been mocked in this test class, its masking a bug. * Remove unnecessary, slightly risky, object initializers in core. * Restore system clock to object -- I take it back, we don't have to get rid of objects, just have to be more particular about using object initializers. * KLog can be a singleton object, no risk there. * This simple fix would take care of the issue, but i still don't love KlaviyoApiClient being a singleton object that can hit an unrecoverable exception in its initializer... * Found a balanced solution for KlaviyoApiClient -- converting this to a class was complicated by the inner class, and besides it works well as a singleton. The initializer was the real problem, so I extract the listeners from the initializer and created a startService method. This is safe to run multiple times if the SDK is re-initialized. * Prefer this way: we don't need to wrap any of the code in this method body except the methods that are themselves protected anyway * Forgot to add back starting the service in these tests' setup * Naming convention things * First shot at a pre-init buffer for failed operations. Needs tests * Added test of in-memory slate * I don't like import * * Added comments, logging, and wrapped the retry operations in safeCall * Trivial: move error logging out of exception class. Now that we wrap all SDK functions in safeCall, it is more straightforward to log from there. * Allow ONLY handlePush to be buffered * Remove unused test * keep 3.0 version increment * don't add this back * never trust github * Update versions.properties --------- Co-authored-by: Evan Masseau <> * Automatically check push permission status on app resume (#188) * Protect against double subscriptions * readme and migration guide updates --------- Co-authored-by: Evan Masseau <> * Move API revision to core config property pulled from build property (#190) Co-authored-by: Evan Masseau <> * [CHNL-12521] Looking for react-native strings to determine sdk name and version for config (#191) * resource reading maven attmept * working resource share attempt * update log level * fixing unit tests and working resource sharing * pr comments * fixing config * replace bump version task with xml-based task * fixing unit test * fixing file pathing for composite builds * fixing bump version task * removing build config field from build.gradle * using readXml for default config * removing unused versionFor import * removing capability change broadcast (#192) * renaming removals to 'breaking changes' (#193) * bumping version --------- Co-authored-by: Evan Masseau <> Co-authored-by: Kenny Tsui <[email protected]> Co-authored-by: Ajay Subramanya <[email protected]> Co-authored-by: dan-peluso <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Separation of concerns:
Klaviyoexposes the public API, including initialize logic.UserInfohandles internal user state including persistenceSideEffectCoordinatorobserves and responds to state changesCheck List
Changelog / Code Overview
Test Plan
Related Issues/Tickets
CHNL-7012