refactor: KitManagerImpl user-attribute forwarding and UserAttributeListener cleanup#693
Conversation
Introduce activeKits() to return non-disabled kit integrations from the providers map. Add KitManagerImplTest.testActiveKitsExcludesDisabled to verify disabled kits are omitted. Made-with: Cursor
Use activeKits() for user attribute and identity paths; inline former setUserAttribute helpers to avoid duplicate isDisabled checks. Flatten syncUserIdentities with early returns and drop redundant null check on identities map. Made-with: Cursor
…ttributesReceived Declare getConfiguration() and getName() on UserAttributeListener so callers can access kit metadata consistently. Route onUserAttributesReceived through activeKits() and align variable naming with other active-kit paths. Made-with: Cursor
Drop the user parameter from KitIntegration.UserAttributeListener methods; update KitManagerImpl forwarding, instrumented/unit tests, and kit implementations (Adobe, AppsFlyer, Apptentive, Braze, CleverTap, etc.). Made-with: Cursor
Add getConfiguration() and getName() to UserAttributeListener so callers can use the listener type without casting. Introduce userAttributeListeners() and iterate with variable name listener; keep activeKits() package-visible for tests. Made-with: Cursor
PR SummaryMedium Risk Overview Refactors Reviewed by Cursor Bugbot for commit 2d2e931. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if (provider instanceof KitIntegration.UserAttributeListener && !provider.isDisabled()) { | ||
| try { | ||
| ((KitIntegration.UserAttributeListener) provider).onConsentStateUpdated(oldState, newState, FilteredMParticleUser.getInstance(mpid, provider)); | ||
| ((KitIntegration.UserAttributeListener) provider).onConsentStateUpdated(oldState, newState); |
There was a problem hiding this comment.
Inconsistent refactoring of onConsentStateUpdated forwarding loop
Low Severity
The onConsentStateUpdated method still iterates providers.values() directly and manually checks !provider.isDisabled(), while every other UserAttributeListener forwarding method (setUserAttribute, removeUserAttribute, incrementUserAttribute, setUserTag, setUserAttributeList, onUserAttributesReceived) was refactored to use the new userAttributeListeners() helper. This inconsistency means future changes to the active-kit filtering logic (e.g., in activeKits() or userAttributeListeners()) would not apply to consent forwarding, risking divergent behavior.
denischilik
requested review from
BrandonStalnaker,
jamesnrokt,
nickolas-dimitrakas and
thomson-t
…Listener API Remove third argument and unused FilteredMParticleUser mock setup from unit tests. Made-with: Cursor
UserAttributeListenerTestKit exposes (String?) -> Unit; a two-parameter lambda failed Kotlin type inference in compileDebugAndroidTestKotlin. Made-with: Cursor
KitIntegration.UserAttributeListener now passes only old and new ConsentState. Drop the removed FilteredMParticleUser argument and unused mock from AppboyKitTests across braze-38 through braze-41. Made-with: Cursor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7524884. Configure here.
|
|
||
| Map<MParticle.IdentityType, String> identities = user.getUserIdentities(); | ||
|
|
||
| for (Map.Entry<MParticle.IdentityType, String> entry : identities.entrySet()) { |
There was a problem hiding this comment.
Removed null guard on identities map in syncUserIdentities
Low Severity
The refactored syncUserIdentities removed the null check on the identities map returned by getUserIdentities(). The old code guarded against a null return with if (identities != null) before iterating. While getUserIdentities() is declared @NonNull in the MParticleUser interface, the defensive check protected against contract violations. A null return now causes a NullPointerException on identities.entrySet().
Reviewed by Cursor Bugbot for commit 7524884. Configure here.
Align GoogleAnalyticsFirebaseKitTest and GoogleAnalyticsFirebaseGA4KitTest with KitIntegration: drop the removed FilteredMParticleUser argument and unused mock from consent tests. Made-with: Cursor
|
BrandonStalnaker
left a comment
BrandonStalnaker
left a comment
There was a problem hiding this comment.
In general this seems good to me. I agree with the removal of FilteredUser for these events and Dennis has separately confirmed with me that no existing kits were utilizing that parameter. I'm less confident about the Kotlin code itself so I'd suggest another reviewer whose more confident before merging.
3 participants
Background
Please review:
FilteredMParticleUserwas removed fromUserAttributeListenercallback parameters because, in the current kit implementations, it is not used in a meaningful way—callbacks do not rely on kit-scoped filtering via that type. If you know a reason we should keep it (e.g. a kit or integration pattern we missed), please comment; follow-up work may depend on that.Refactor focus: duplicated dispatch logic in
KitManagerImplwas removed by consolidating user-attribute and identity forwarding into shared listener iteration instead of repeating the same patterns in multiple places.What Has Changed
UserAttributeListenerAPI:FilteredMParticleUserremoved from callback signatures (those callbacks no longer take a user parameter).KitManagerImpl: centralized forwarding for user-attribute and identity listeners (activeKits(), shared loops) to avoid duplication across the class.Screenshots/Video
N/A (SDK internals)
Checklist