-
Notifications
You must be signed in to change notification settings - Fork 81
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
[MM51547] Fix telemetry for gitlab and add account connected event #365
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #365 +/- ##
==========================================
- Coverage 32.33% 32.17% -0.16%
==========================================
Files 21 21
Lines 3451 3468 +17
==========================================
Hits 1116 1116
- Misses 2225 2242 +17
Partials 110 110
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@@ -400,6 +400,8 @@ func (p *Plugin) completeConnectUserToGitlab(c *Context, w http.ResponseWriter, | |||
} | |||
} | |||
|
|||
p.TrackUserEvent("account_connected", userID, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister is this the most proper/accurate place or should it be within the else
block with the welcome message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct. If it's in the else
block, it doesn't get tracked if the wizard is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @m1lt0n
The code is fine and the logs are written correctly. My only doubt is the pluginapi dep version in go.mod. I wonder if this is being reused from the version downloaded by GitHub plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall lgtm, let's fix the lint
@spirosoik done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Summary
Ticket Link
https://mattermost.atlassian.net/browse/MM-51547