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

Adds anonymous Id to segment #29

Merged
merged 5 commits into from
Nov 30, 2021

Conversation

schultzp2020
Copy link
Contributor

@schultzp2020 schultzp2020 commented Nov 19, 2021

What does this PR do / why we need it:
Adds anonymous Id to segment

Fixes devfile/api#678
Fixes devfile/api#651

pages/_app.tsx Outdated
userId: publicRuntimeConfig.segmentUserId,
const event: SegmentEvent = {
type: 'track',
anonymousId: analytics.user().anonymousId(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any situation where this can return nil or empty? Just wondering what happens if the user has an ad blocker or disables cookie tracking, can we still generate an anonymous ID? If not, we should default to the generic client name if that's the case

kim-tsao
kim-tsao previously approved these changes Nov 19, 2021
pages/_app.tsx Outdated
const region = getUserRegion(router.locale);
const { publicRuntimeConfig } = getConfig();

// analytics.identify(
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@openshift-ci openshift-ci bot added the lgtm label Nov 19, 2021
userId: publicRuntimeConfig.segmentUserId,
event: 'Starter Project Downloaded',
properties: {
analytics.track(
Copy link
Member

Choose a reason for hiding this comment

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

we can do this later, but probably a good idea to track after the call to await download(project); has finished. If there is an exception in the download function and a user spam clicks download, this will just spam analytics to thinking a user really clicked on this project download many times 🤔

Copy link
Member

@maysunfaisal maysunfaisal left a comment

Choose a reason for hiding this comment

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

just a small minor suggestion which can be thought about for later

Copy link
Member

@johnmcollier johnmcollier left a comment

Choose a reason for hiding this comment

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

I can believe I keep forgetting to review this 🤦‍♂️

LGTM

@openshift-ci
Copy link

openshift-ci bot commented Nov 25, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnmcollier, kim-tsao, maysunfaisal, schultzp2020

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [johnmcollier,maysunfaisal,schultzp2020]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@schultzp2020 schultzp2020 merged commit d3bd38b into devfile:main Nov 30, 2021
@schultzp2020 schultzp2020 deleted the add-anonymous-id branch November 30, 2021 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants