Skip to content

Adds AB Testing support (ExPlat)#155

Merged
leandroalonso merged 8 commits intotrunkfrom
issue/ab_testing
Jan 12, 2021
Merged

Adds AB Testing support (ExPlat)#155
leandroalonso merged 8 commits intotrunkfrom
issue/ab_testing

Conversation

@leandroalonso
Copy link
Copy Markdown
Member

@leandroalonso leandroalonso commented Nov 6, 2020

This PR adds AB Testing support.

Configuration

(WPiOS PR here: wordpress-mobile/WordPress-iOS#15236)

Before having fun with AB testing, the app needs to configure the ExPlat instance. To do so, the switchToAuthenticatedUserWithUsername method was changed and now it requires a token parameter.

An example from WPiOS:

[self.tracksService switchToAuthenticatedUserWithUsername:username userID:@"" token:self.token skipAliasEventCreation:NO];

Then, you need to call the method refreshIfNeeded() in didFinishLaunchingWithOptions.

ExPlat.shared.refreshIfNeeded()

And that's all. :)

Usage

Once it's configured you can use like this:

switch ExPlat.shared.experiment("experiment-name") {
    case .treatment:
        // Show treatment option
    case .control:
        // Show control option
    case .unknown:
        // Experiment doesn't exist or wasn't retrieved
    case .other(let value) where value == "my-value":
        // any other value 
}

Or, more simpler:

switch ExPlat.shared.experiment("experiment-name") {
    case .treatment:
        // Show treatment option
    default:
        // Show control option
}

@leandroalonso
Copy link
Copy Markdown
Member Author

@mokagio I removed your private specs because the build was failing on the CI machine. Just want to double-check if this will not cause any unintended issue.

Copy link
Copy Markdown
Contributor

@astralbodies astralbodies left a comment

Choose a reason for hiding this comment

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

It probably makes sense to tie the ExPlat service into the Tracks service somehow so it can respond to auth/de-auth scenarios already set up in the host app.

Methods used by the host app to track auth: https://github.com/Automattic/Automattic-Tracks-iOS/blob/trunk/Automattic-Tracks-iOS/Services/TracksService.h#L29

Those two methods are already hooked in somewhere in the host app based on the auth lifecycle. The only thing this doesn't give you is the OAuth bearer token, of course. Just figured since we already deal with going from anon -> logged in -> anon with Tracks.

@astralbodies
Copy link
Copy Markdown
Contributor

Also, consider adding the OAuth Bearer token to the methods tracking authentication state. Since that token changes at the same time (usually), the TracksService could be the owner of that data, and then later inject that into the ExPlat service for use.

Does ExPlat work without a Bearer token? Wonder how that affects anonymous users / users not yet logged in.

@mokagio
Copy link
Copy Markdown
Contributor

mokagio commented Nov 19, 2020

Hey @leandroalonso, sorry for the long time it took me to get to this 😞 🙏

You did well. I'm pretty sure I added them in there only because the CDN didn't seem to find the .podspec for my Sodium-Fork for some reason (update propagation maybe?). The error we got in CI clearly shows that the spec is now there, so good to clean it 🧹

@leandroalonso
Copy link
Copy Markdown
Member Author

@astralbodies ready for another review!

ExPlat is now coupled with Tracks so it doesn't need to be configured, but it needs to be started in didFinishLaunchingWithOptions.

I have one question: I kept both the old method and the new one with a token param. My intention is to not break the apps that use this library. So, if the app wants to use the AB testing feature it needs to switch to the switchToAuthenticatedUserWithUsername with the token param. Thoughts about that?

@astralbodies
Copy link
Copy Markdown
Contributor

@leandroalonso I'm terribly sorry for missing this code review. I will be reviewing it today!

@@ -27,6 +27,7 @@ extern NSString *const TrackServiceDidSendQueuedEventsNotification;
- (NSDictionary *)dictionaryForTracksEvent:(TracksEvent *)tracksEvent withParentCommonProperties:(NSDictionary *)parentCommonProperties;

- (void)switchToAuthenticatedUserWithUsername:(NSString *)username userID:(NSString *)userID skipAliasEventCreation:(BOOL)skipEvent;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was thinking about maybe having you add a deprecation warning here... But then I think Simplenote uses this client as well, and they don't use WordPress.com logins. Now that has me wondering if we should name the token in the new method to be a wpcom bearer token for clarity.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you mean something like that?

- (void)switchToAuthenticatedUserWithUsername:(NSString *)username userID:(NSString *)userID wpComToken:(NSString *)token skipAliasEventCreation:(BOOL)skipEvent;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup, like that!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in 7f8c484

Copy link
Copy Markdown
Contributor

@astralbodies astralbodies left a comment

Choose a reason for hiding this comment

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

This looks good! I didn't test out the functionality with the backend system. I did suggest maybe changing the parameter name a bit for clarity's sake.

@leandroalonso leandroalonso merged commit aaddd82 into trunk Jan 12, 2021
@leandroalonso leandroalonso deleted the issue/ab_testing branch January 12, 2021 17:08
@leandroalonso leandroalonso restored the issue/ab_testing branch January 12, 2021 17:14
@astralbodies
Copy link
Copy Markdown
Contributor

@leandroalonso Ah crap, I didn't notice this PR was targeting trunk.

@jkmassel can we fix that?

@leandroalonso
Copy link
Copy Markdown
Member Author

@astralbodies I already fixed. 😅

public func experiment(_ name: String) -> Variation {
guard let assignments = UserDefaults.standard.object(forKey: assignmentsKey) as? [String: String?],
case let variation?? = assignments[name] else {
return .unknown
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It may be better to consider returning .control instead of .unknown so that an old client can properly fall back to the control experience if the experiment is no longer available.

@@ -0,0 +1,6 @@
{
"variations": {
"experiment": "control"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Explat will never return the name of the control variation, For control, it always returns null and/or it won't exist in the response. It only returns the name of treatment variations. Right now, we only support a singular treatment, but multi-variate experiments are in the works.

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.

4 participants