-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat: replace user identity strings with user IDs #38775
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
base: master
Are you sure you want to change the base?
Changes from all commits
c9d20bf
a06f530
88218fa
020a5f2
35f8781
b426fd3
b08bbd7
5e2a0f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| """ | ||
| import logging | ||
|
|
||
| from django.conf import settings | ||
| from django.contrib.auth import get_user_model | ||
| from django.db import transaction | ||
| from rest_framework import permissions, status | ||
|
|
@@ -56,11 +57,19 @@ def post(self, request, **kwargs): # pylint: disable=unused-argument | |
| user_to_retire = User.objects.get(username=username) | ||
| with transaction.atomic(): | ||
| create_retirement_request_and_deactivate_account(user_to_retire) | ||
| log.info(f'The user "{username}" has been added to the retirement pipeline \ | ||
| by "{request.user}"') | ||
| if settings.FEATURES['SQUELCH_PII_IN_LOGS']: | ||
| log.info('A user has been added to the retirement pipeline') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @robrap Should we keep
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably just use the request user's id instead.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||
| else: | ||
| log.info('The user "%s" has been added to the retirement pipeline by "%s"', | ||
| username, | ||
| request.user, | ||
| ) | ||
|
|
||
| except User.DoesNotExist: | ||
| log.exception(f'The user "{username}" does not exist.') | ||
| if settings.FEATURES['SQUELCH_PII_IN_LOGS']: | ||
| log.exception('A user does not exist for bulk retirement.') | ||
| else: | ||
| log.exception(f'The user "{username}" does not exist.') | ||
| failed_user_retirements.append(username) | ||
|
|
||
| except Exception as exc: # pylint: disable=broad-except | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -462,7 +462,10 @@ def post(self, request, course_id): # pylint: disable=too-many-statements | |
| warnings.append({ | ||
| 'username': username, 'email': email, 'response': warning_message | ||
| }) | ||
| log.warning('email %s already exist', email) | ||
| if settings.FEATURES['SQUELCH_PII_IN_LOGS']: | ||
| log.warning('email [REDACTED] already exist') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the logger warning is a necessity to know which user's email is being referred here , then as a suggestion we can use it as: Just a suggestion upto you to decide, can we use this or the current change is enough? Other places also may need some attention to see if user id can be used in the logger, so take a look at those also.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||
| else: | ||
| log.warning('email %s already exist', email) | ||
| else: | ||
| log.info( | ||
| "user already exists with username '%s' and email '%s'", | ||
|
|
||
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.
studio_request_emailwas and how we'd spell that out. It turns out it is just a system email coming from a setting, so there is no reason to redact that email in the first place. Something this brings up, if we choose to not redact an email (something that looks like PII), what's a good way to annotate that? For now, you could just add a comment like: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.
updated