feat: Add disabledKits option to MParticleOptions#347
Conversation
BrandonStalnaker
requested review from
Mansi-mParticle,
alexs-mparticle and
rmi22186
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (5)
- UnitTests/MPKitContainerTests.m: Language not supported
- mParticle-Apple-SDK/Include/MPKitContainer.h: Language not supported
- mParticle-Apple-SDK/Include/mParticle.h: Language not supported
- mParticle-Apple-SDK/Kits/MPKitContainer.mm: Language not supported
- mParticle-Apple-SDK/mParticle.m: Language not supported
| if (active && !disabledByRamping && !disabledByConsent && !disabledByExcludingAnonymousUsers) { | ||
| BOOL disabledKit = [_disabledKits containsObject:kitRegister.code]; | ||
|
|
||
| if (active && !disabledByRamping && !disabledByConsent && !disabledByExcludingAnonymousUsers && !disabledKit) { |
There was a problem hiding this comment.
Nitpick: I would extract all of these individual AND conditionals into a convenience method (hasKitBeenDisabled or something concise) to keep this code readable.
There was a problem hiding this comment.
I don't necessarily agree with this since this logic isn't reused anywhere else but I did the change anyway. To me this is less readable and more obfuscated.
|
|
||
| self.kitsInitialized = YES; | ||
| BOOL shouldStartKit = !(_disabledKits != nil && [_disabledKits containsObject:kitConfiguration.integrationId]); | ||
| if (shouldStartKit) { |
There was a problem hiding this comment.
Nit: The indenting looks way off here.
| if (kitInstance) { | ||
|
|
||
| BOOL disabled = [self isDisabledByConsentKitFilter:kitConfiguration.consentKitFilter]; | ||
| BOOL disabled = [self isDisabledByConsentKitFilter:kitConfiguration.consentKitFilter] || [_disabledKits containsObject:kitRegister.code]; |
There was a problem hiding this comment.
Nitpick/Readability: Same here. I would consider creating an isDisabled convenience method here.
| self.kitConfigurations[kitConfiguration.integrationId] = kitConfiguration; | ||
| [self startKit:kitConfiguration.integrationId configuration:kitConfiguration]; | ||
|
|
||
| self.kitsInitialized = YES; |
There was a problem hiding this comment.
Nit. kitsInitialized will be set to YES for every iteration of the loop. It might be better to just check the list of configured kits once the loop is done, then set it to YES, kind of like how self.sideloadedKits.count is being checked below.
|
Can we add enums so that a customer doesn't have to look through code to determine the module id when passing through? |
rmi22186
left a comment
rmi22186
left a comment
There was a problem hiding this comment.
Seeing from cursor that a couple additional tests shoudl be added...can you leverage cursor to try to add tests such as:
- (void)testDisableMultipleKits
- (void)testDisableNonExistentKit
- (void)testDisableKitAfterStart // not really sure what this means
- (void)testDisableKitAfterConfiguration // not really sure what this means
- (void)testEmptyDisabledKitsArray
- (void)testNilDisabledKits
|
besides the additon of stated tests and smll comment about confirming the reason |
BrandonStalnaker
deleted the
feat/SQDSDKS-7214-Add-DisabledKits-Option-To-MParticleOptions
branch
# [8.30.0](v8.29.0...v8.30.0) (2025-04-24) ### Features * Add disabledKits option to MParticleOptions ([#347](#347)) ([f708431](f708431)) * Add MPRoktEmbeddedView Class ([#345](#345)) ([f4fb946](f4fb946)) * Add user attribute mapping ([#343](#343)) ([50ce934](50ce934)) * Automatically include sandbox in MPRokt Attributes ([#344](#344)) ([bdf4280](bdf4280))
|
🎉 This PR is included in version 8.30.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)