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

[wip] backend funnel fix #235

Merged
merged 10 commits into from
Apr 9, 2018
Merged

[wip] backend funnel fix #235

merged 10 commits into from
Apr 9, 2018

Conversation

eoji
Copy link
Contributor

@eoji eoji commented Apr 2, 2018

what

We should be setting the visitor cookie on web and api. The value is InstanceId.getInstance().getId(). Read more about it here -> https://developer.android.com/training/articles/user-data-ids.html.

I also added our first instrumentation test ✨ but I'm doing some gross stuff with optionals so I'm open to feedback. (Cookies are returned as a list and not a map because 2 cookies can have the same name, party time.)

Shout out to @ryanpeterson for pairing on this with me for many moons.

@eoji eoji requested a review from Scollaco April 2, 2018 18:25
@eoji
Copy link
Contributor Author

eoji commented Apr 3, 2018

Copy link
Contributor Author

@eoji eoji left a comment

Choose a reason for hiding this comment

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

some small changes, ty


private void setVisitorCookie() {
final String deviceId = InstanceID.getInstance(this).getId();
final String uuid = TextUtils.isEmpty(deviceId) ? UUID.randomUUID().toString() : deviceId;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rename to uniqueIdentifier

}

private Optional<HttpCookie> getOptionalVisitorCookieForURI(final @NonNull CookieStore cookieStore, final @NonNull URI webUri) {
// it's problematic, i know
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's fine

final HttpCookie webCookie = webVisCookie.get();
final HttpCookie apiCookie = apiVisCookie.get();

Assert.assertNotNull(webCookie);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instead of asserting they aren't null, assert that cookie.isPresent()

Copy link
Contributor

@Scollaco Scollaco left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

@eoji eoji merged commit 49d7218 into master Apr 9, 2018
@eoji eoji deleted the backend-funnel-fix branch April 9, 2018 20:17
Rcureton pushed a commit that referenced this pull request Jul 31, 2018
* attempting to have unique tracking ids, switching to `InstanceId.getInstance().getId()` and our first instrumentation test
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.

None yet

2 participants