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

[NT-1851] Enable/Disable Segment By Feature Flag #1440

Merged
merged 4 commits into from
Apr 21, 2021

Conversation

justinswart
Copy link
Contributor

📲 What

  • Moves our configuration of Segment to be after the AppEnvironment is set up.
  • Enables and disables all Segment tracking by feature flag.
  • Tidies up our protocols that we were using before to mock and feature flag tracking.

🤔 Why

  • We had a race condition in that we were instantiating Segment's Analytics during the instantiation of the AppEnvironment and therefore could not inspect the environment at the same time. Moving this makes more tests possible.
  • Previously we were feature-flagging the track, identify, etc methods but Segment would still track app lifecycle events. It turns out Segment provides disable() and enable() functions to completely disable/enable tracking which we are now using instead.
    • Note: Segment is disabled by default and enabled by the feature flag configuration.
  • We no longer need some of the protocol functions as we don't need to test the feature flagging at those particular call-sites.

🛠 How

  • Moved Segment initialization to be driven by an AppDelegateViewModel output.
  • Observing configuration changes and enabling/disabling Segment.
  • Tidied up these protocols.

✅ Acceptance criteria

Open Segment's debugger and run the app.

  • Toggle the Segment feature flag in the app's debug menu, observe that events are or are not sent to Segment based on the state of the flag.
  • Trigger a reload of the config and intercept it using Charles Proxy. Edit the response to enable or disable Segment using the feature flag. Observe that events are or are not sent to Segment based on the state of the flag.

}
}

NotificationCenter.default
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ordinarily we would hang on to the token for these observers and release them but the AppDelegate is never released so I guess it's ok here 😄


extension Analytics: IdentifyingTrackingClient {
public func identify(userId: String?, traits: [String: Any]?) {
guard AppEnvironment.current.environmentVariables.isTrackingEnabled,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weirdly no tests failed removing this so it looks like we weren't testing it anyway 🤦‍♂️

@codecov
Copy link

codecov bot commented Apr 20, 2021

Codecov Report

Merging #1440 (72402b7) into master (37094c6) will decrease coverage by 0.00%.
The diff coverage is 88.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1440      +/-   ##
==========================================
- Coverage   86.09%   86.09%   -0.01%     
==========================================
  Files        1108     1107       -1     
  Lines       98602    98646      +44     
==========================================
+ Hits        84895    84932      +37     
- Misses      13707    13714       +7     
Impacted Files Coverage Δ
Library/Tracking/Segment.swift 0.00% <0.00%> (-76.00%) ⬇️
Library/Tracking/TrackingClient.swift 40.47% <0.00%> (-0.33%) ⬇️
Library/Tracking/KSRAnalytics.swift 85.34% <70.00%> (-0.29%) ⬇️
...kstarter-iOS/ViewModels/AppDelegateViewModel.swift 96.64% <100.00%> (+0.07%) ⬆️
...ter-iOS/ViewModels/AppDelegateViewModelTests.swift 99.67% <100.00%> (+<0.01%) ⬆️
Library/Tracking/MockTrackingClient.swift 90.47% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37094c6...72402b7. Read the comment docs.

public static let staging = "write-key"
public static let production = "write-key"
public static let staging = "write-key-production"
public static let production = "write-key-staging"
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like you mixed these up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh lol yeah, they mostly just need to not be the same but best have them the right way around 👍


self.viewModel.outputs.segmentIsEnabled
.observeValues { enabled in
if enabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

would a ternary operator work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't see why not! 👍

Copy link
Contributor

@singhhari singhhari left a comment

Choose a reason for hiding this comment

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

lgtm

@justinswart justinswart merged commit 7c02930 into master Apr 21, 2021
@justinswart justinswart deleted the NT-1851-segment-init-feature-flag branch April 21, 2021 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants