-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix: replace username with full name in attemp emails #250
Conversation
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.
LGTM
edx_exams/apps/core/email.py
Outdated
@@ -44,12 +44,13 @@ def send_attempt_status_email(attempt, escalation_email=None): | |||
contact_url = f'{settings.LMS_ROOT_URL}/support/contact_us' | |||
contact_url_text = contact_url | |||
|
|||
email_subject = f'Proctored exam {exam.exam_name} for user {attempt.user.username}' | |||
email_subject = f'Proctored exam {exam.exam_name} for user {attempt.user.full_name}' |
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.
AFAIK this IDA isn't populating full_name when it creates new User objects since up until this point there's been no use for it. I think you'll need to ensure that happens and possibly migrate that value into existing users.
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 you share how data is populated in this IDA?
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.
I'm not exactly sure, it's part of the cookie cutter installation for all IDAs. I think it's baked into JWT authentication within https://github.com/openedx/edx-drf-extensions somehow. Although, I imagine this could be an issue for all other IDAs as well depending on if the particular bit of code is looking at JWT details vs the User object from the database. Names should be on the JWT but doesn't seem to always exist in the User model.
e46f853
to
aafadce
Compare
aafadce
to
f15af7e
Compare
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 looks good. Could you please update the commit message and pull request title to accurately reflect the changes, since we are not longer replacing the username with the full name?
f15af7e
to
b2deb6b
Compare
I made the change now |
b2deb6b
to
ccfbbe3
Compare
Description: Remove username and change the subject of emails VAN-1833
ccfbbe3
to
d4122fc
Compare
@@ -44,7 +47,6 @@ def send_attempt_status_email(attempt, escalation_email=None): | |||
contact_url = f'{settings.LMS_ROOT_URL}/support/contact_us' | |||
contact_url_text = contact_url | |||
|
|||
email_subject = f'Proctored exam {exam.exam_name} for user {attempt.user.username}' |
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.
Why can we not leave the exam name 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.
Actually we have discussed subject line with UX team and they suggested us that we can remove the exam name from subject but if you still think we should get it back we can then discuss with UX again. Thanks
We already got 3 approval so no need to get any another
JIRA:
VAN-1833
Description:
Remove username and change subject in attempts emails
Testing instructions:
It has been tested locally