-
Notifications
You must be signed in to change notification settings - Fork 332
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
Fall back to using Segment integration when we cannot get settings #636
Conversation
@@ -1,6 +1,6 @@ | |||
Pod::Spec.new do |s| | |||
s.name = "Analytics" | |||
s.version = "3.5.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should stay as is.
@@ -562,8 +562,16 @@ - (void)refreshSettings | |||
self.settingsRequest = [self.httpClient settingsForWriteKey:self.configuration.writeKey completionHandler:^(BOOL success, NSDictionary *settings) { | |||
if (success) { | |||
[self setCachedSettings:settings]; | |||
} else { | |||
// Hotfix: If settings request fail, fall back to using just Segment integration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hotfix seems like a weird comment to leave in. I'd rather put in something like "todo: we need to fix this behaviour"
@@ -583,7 +591,7 @@ + (void)debug:(BOOL)showDebugLogs | |||
|
|||
+ (NSString *)version | |||
{ | |||
return @"3.5.3"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should stay as (same as above) and updated as part of the release process.
@f2prateek updated |
@tonyxiao nice, Let's do a quick manual test to verify the offline behaviour and get this out (adding one more comment but shouldn't block the change) |
// Won't catch situation where this callback never gets called - that will get addressed separately in regular dev | ||
[self setCachedSettings:@{ | ||
@"integrations": @{ | ||
@"Segment.io": @{ @"apiKey": self.configuration.writeKey }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually instead of doing setCachedSettings we should do updateIntegrationWithSettings (can't do this yet because the code references self.cachedSettings directly in a few places)>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Wanted to change as few lines as possible since this is a hotfix. It'll become a lot more clear in dev.
@f2prateek Yea I did a manual test and verified that it behaves as expected. Merging. |
cc @f2prateek @ladanazita
To repro the original buggy behavior
Application Launched 1
Application Launched 2
Application Launched 2
will hit tracking API, whileApplication Launched 1
will be missing.Basically what happens is that during step 2, if the settings request were to fail, no integrations would be initialized (segment included). This hotfix falls back to segment integration in those cases and we'll have a more comprehensive set of fixes to address this from a design level in development branch.