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

Supports outOfSession via integration option & logEvent #52

Merged
merged 2 commits into from
Oct 30, 2017

Conversation

ladanazita
Copy link
Contributor

@ladanazita ladanazita commented Oct 27, 2017

Out-of-session events are not considered part of the current session, meaning they do not extend the current session. This might be useful if you are logging events triggered by push notifications, for example.

This supports calling logEvent with outOfSession passed in as true if the integration specific option is passed in: integrations.amplitude.outOfSession

There are four ways to log an event in Amplitude:

  • Log event with props, groups and out of session flag
  • Log event with props and groups
  • Log event with props and out of session flag
  • Log event with props

@ladanazita ladanazita changed the title Supports outOfSession via integration option and additional logEvent … Supports outOfSession via integration option & logEvent Oct 27, 2017
@@ -95,9 +95,17 @@ - (void)realTrack:(NSString *)event properties:(NSDictionary *)properties integr
{
NSDictionary *options = integrations[@"Amplitude"];
NSDictionary *groups = [options isKindOfClass:[NSDictionary class]] ? options[@"groups"] : nil;
if (groups && [groups isKindOfClass:[NSDictionary class]]) {
bool outOfSession = options[@"outOfSession"];
Copy link
Contributor

Choose a reason for hiding this comment

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

See the check for groups above and how it is retrieved. We need the same here.

@ladanazita
Copy link
Contributor Author

@f2prateek made those changes!

Copy link
Contributor

@f2prateek f2prateek left a comment

Choose a reason for hiding this comment

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

Code is ok and we can merge it.

However, we might be able simplify the code branches a fair bit. We should do this in a follow up PR.

Internally it's likely that Amplitude's logEvent methods call each other with default arguments.

in analytics-ios track:@"foo" internally just calls track:@"foo" properties:nil which itself just calls track:@"foo" properties:nil options:nil.

So instead of having multiple branches like:

if properties != nil && options != nil {
  [analytics track:@"foo" properties:properties options:options];
} else if options == nil && properties != nil {
  [analytics track:@"foo" properties:properties];
} else {
  [analytics track:@"foo"];
}

You can just do:

[analytics track:@"foo" properties:properties options:options];

and internally the library handles all those cases.

The Android one does something similar to reduce the number of branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants