-
Notifications
You must be signed in to change notification settings - Fork 481
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
[Amplitude] Send Record on Teacher Login #50081
Conversation
) { | ||
trySetLocalStorage(LOGGED_TEACHER_SESSION, 'true'); | ||
|
||
analyticsReporter.sendEvent(EVENTS.TEACHER_LOGIN_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.
Forgive my ignorance on local storage -- would this trigger correctly if a teacher logged in, logged out, and logged in again?
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 catch! I meant for this to be trySetSESSIONStorage
- fixing!
@@ -57,6 +60,13 @@ describe('StudentHomepage', () => { | |||
}); | |||
}); | |||
|
|||
it('does not log an Amplitude event for teacher signing-in', () => { |
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.
nit: should this test name be "does not log an Amplitude event for student signing-in"?
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.
Thanks for adding tests!
Part of our Amplitude project.
Triggers when a teacher renders the homepage, but only once per session (to emulate the 'on login' experience).
Includes testing.