-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
feat(codecov): Add FE analytics for Codecov link #43517
Conversation
codecovStatusCode?: CodecovStatusCode; | ||
codecovUrl?: string; | ||
} | ||
|
||
function CodecovLink({codecovUrl, codecovStatusCode}: CodecovLinkProps) { | ||
const onOpenCodecovLink = (organization: Organization, event: Event) => { | ||
trackAdvancedAnalyticsEvent('issue_details.codecov_link_clicked', { |
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.
can we keep this with the others like integrations.stacktrace_link_clicked
?
codecovStatusCode?: CodecovStatusCode; | ||
codecovUrl?: string; | ||
} | ||
|
||
function CodecovLink({codecovUrl, codecovStatusCode}: CodecovLinkProps) { | ||
const onOpenCodecovLink = (organization: Organization, event: Event) => { |
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.
lets declare this function inside of CodecovLink
and not pass the stuff, kinda like onOpenLink
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.
Good idea, thank you
userEvent.click(await screen.findByText('View Coverage Tests on Codecov')); | ||
|
||
expect(await screen.findByText('View Coverage Tests on Codecov')).toHaveAttribute( | ||
'href', | ||
'https://app.codecov.io/gh/path/to/file.py' | ||
); | ||
expect(trackAdvancedAnalyticsEvent).toHaveBeenCalled(); |
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.
checking that analytics is called is related to the click, so its easier to read if they're kept together. You might also want to check that it was called with something since there could be other analytics being used.
userEvent.click(await screen.findByText('View Coverage Tests on Codecov')); | |
expect(await screen.findByText('View Coverage Tests on Codecov')).toHaveAttribute( | |
'href', | |
'https://app.codecov.io/gh/path/to/file.py' | |
); | |
expect(trackAdvancedAnalyticsEvent).toHaveBeenCalled(); | |
const link = await screen.findByText('View Coverage Tests on Codecov'); | |
expect(link).toHaveAttribute( | |
'href', | |
'https://app.codecov.io/gh/path/to/file.py' | |
); | |
userEvent.click(link); | |
await waitFor(() => { | |
expect(trackAdvancedAnalyticsEvent).toHaveBeenCalledWith( | |
something | |
); | |
}); |
@@ -142,8 +149,17 @@ function CodecovLink({codecovUrl, codecovStatusCode}: CodecovLinkProps) { | |||
if (!codecovUrl) { | |||
return null; | |||
} | |||
|
|||
const onOpenCodecovLink = () => { | |||
trackIntegrationAnalytics('integrations.stacktrace_codecov_link_clicked', { |
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.
trackIntegrationAnalytics('integrations.stacktrace_codecov_link_clicked', { | |
trackIntegrationAnalytics(StacktraceLinkEvents.CODECOV_LINK_CLICKED, { |
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.
The rest of the trackIntegrationAnalytics
function calls use trackIntegrationAnalytics('integrations.<name>
. Should I change these too to make it more consistent?
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.
that would be great!
Fixes WOR-2560