Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-2998 Add telemetry for product notices #6934

Merged
merged 5 commits into from
Oct 28, 2020
Merged

MM-2998 Add telemetry for product notices #6934

merged 5 commits into from
Oct 28, 2020

Conversation

sudheerDev
Copy link
Contributor

Summary

  • Add track event when user clicks on action button of notice

Ticket Link

https://mattermost.atlassian.net/browse/MM-29998

Related Pull Requests

mattermost/mattermost-redux#1277

@sudheerDev
MM-2998 Add telemetry for product notices
545dc1b 
   * Add  track event when user clicks on action button of notice
@sudheerDev sudheerDev added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Oct 27, 2020
@sudheerDev sudheerDev added this to the v5.29.0 milestone Oct 27, 2020
nevyangelova
nevyangelova approved these changes Oct 27, 2020
View reviewed changes
Copy link
Contributor

@nevyangelova nevyangelova left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM but there seems to be a changed import? isn't it import {trackEvent} from 'actions/telemetry_actions.jsx' ?

Willyfrog
Willyfrog approved these changes Oct 28, 2020
View reviewed changes
Copy link
Contributor

@Willyfrog Willyfrog left a comment

Choose a reason for hiding this comment

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

left a comment, but I don't think it is in the scope of the change, something to consider for the future. LGTM

components/product_notices_modal/product_notices_modal.tsx
@@ -186,6 +188,11 @@ export default class ProductNoticesModal extends React.PureComponent<Props, Stat
return null;
}

private trackClickEvent = () => {
const presentNoticeInfo = this.state.noticesData[this.state.presentNoticeIndex];
trackEvent('ui', `notice_click_${presentNoticeInfo.id}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

not to change it right now, but feels like 'ui' should really be a constant so we use the same string throughout the code prevent typos

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, i agree. All events strings need to be changed to constants. I think we can run a campaign on this

@amyblais amyblais removed the 2: Dev Review Requires review by a core commiter label Oct 28, 2020
@jgilliam17 jgilliam17 self-requested a review October 28, 2020 14:43
jgilliam17
jgilliam17 approved these changes Oct 28, 2020
View reviewed changes
Copy link
Contributor

@jgilliam17 jgilliam17 left a comment

Choose a reason for hiding this comment

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

Thank you @sudheerDev
Per our discussion QA will test telemetry for product notices after merge.

@jgilliam17 jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Oct 28, 2020
@sudheerDev sudheerDev merged commit 6333dca into mattermost:master Oct 28, 2020
@mattermod
Copy link
Contributor

Cherry pick is scheduled.

@mattermod
Copy link
Contributor

Error trying doing the automated Cherry picking. Please do this manually

+++ Updating remotes...
Fetching upstream
Failed to add the RSA host key for IP address '140.82.112.4' to the list of known hosts (/app/.ssh/known_hosts).
From github.com:mattermost/mattermost-webapp
   dc6d2d127..3fa214d6c  MM-28843-feature-flag-system-console -> upstream/MM-28843-feature-flag-system-console
   844bbb964..6333dcaa8  master       -> upstream/master
   382d108b5..99e70ba4c  release-5.29 -> upstream/release-5.29
Fetching origin
Failed to add the RSA host key for IP address '140.82.112.4' to the list of known hosts (/app/.ssh/known_hosts).
+++ Updating remotes done...
+++ Creating local branch automated-cherry-pick-of-mattermost-webapp-#6934-upstream-release-5.29-1603899593
Switched to a new branch 'automated-cherry-pick-of-mattermost-webapp-#6934-upstream-release-5.29-1603899593'
Branch 'automated-cherry-pick-of-mattermost-webapp-#6934-upstream-release-5.29-1603899593' set up to track remote branch 'release-5.29' from 'upstream'.

+++ About to attempt cherry pick of PR #6934 with merge commit 6333dcaa8eddd92d9fee0942f17dad359841761c.

error: could not apply 6333dcaa8... MM-2998 Add telemetry for product notices (#6934)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

+++ Conflicts detected:

UU package-lock.json
UU package.json
Aborting.

+++ Aborting in-progress git cherry-pick.

+++ Returning you to the master branch and cleaning up.

@sudheerDev sudheerDev deleted the MM-29998 branch October 28, 2020 15:39
sudheerDev added a commit that referenced this pull request Oct 28, 2020
@sudheerDev
MM-2998 Add telemetry for product notices (#6934)
f142b95 
* MM-2998 Add telemetry for product notices

   * Add  track event when user clicks on action button of notice

* Update redux hash

* Update trackEvent path

* Fix lint

* Redux hash update
@sudheerDev sudheerDev added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Oct 28, 2020
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Needed Requires documentation labels Oct 28, 2020
@amyblais
Copy link
Member

amyblais commented Nov 3, 2020

@esethna @sudheerDev I added the "Docs/Needed" label as I wasn't sure if this needs docs for https://docs.mattermost.com/administration/telemetry.html. Please take a look.

@esethna
Copy link
Contributor

esethna commented Nov 3, 2020

@sudheerDev can you help submit a PR as needed?

@sudheerDev
Copy link
Contributor Author

@sudheerDev can you help submit a PR as needed?

@amyblais @esethna Will do

@amyblais amyblais added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nevyangelova nevyangelova nevyangelova approved these changes

@jgilliam17 jgilliam17 jgilliam17 approved these changes

@Willyfrog Willyfrog Willyfrog approved these changes

Assignees

@esethna esethna

Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Done Required documentation has been written
Projects
None yet
Milestone
v5.29.0
Development

Successfully merging this pull request may close these issues.
7 participants
@sudheerDev @mattermod @amyblais @esethna @Willyfrog @nevyangelova @jgilliam17