-
Notifications
You must be signed in to change notification settings - Fork 24
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
jimfuqian/BB2-3641 Added 2 Django Commands to repair and set expiration date of grants #1284
jimfuqian/BB2-3641 Added 2 Django Commands to repair and set expiration date of grants #1284
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.
Have some comments, maybe I'm being too nitpicky given that this is code that we will probably only use for the next week or so and then eventually remove, but I think there's a lot of ways we can make this code cleaner, so let me know what you think! I'm available to pair on this tomorrow if that helps too.
# validate turn on date in date range | ||
if not (created_after_date <= turn_on_date and turn_on_date <= created_before_date): | ||
print("Error: turnondate {} must be in [createdafter {}: createdbefore{}]".format(turn_on_date_str, created_after_date_str, created_before_date_str)) | ||
return False |
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 don't understand the reason for this restriction, the created before/after seems to just be used to determine which grants to update, and the turnondate seems to only be used to determine what the new expiration date should be, so it would still be valid to use those parameters, even if the turnondate is outside of that range.
if app is None: | ||
print("Error: App '{}' not found".format(app_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.
Is this case possible given the check above?
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.
likely not, just checking (paranoid)
print("Error: This command only applies to app with THIRTEEN_MONTH data access type, '{}' data access type: {}.".format(app.name, app.data_access_type)) | ||
return False | ||
|
||
grants = DataAccessGrant.objects.filter(application=app.id).filter(created_at__range=(created_after_date, created_before_date)) |
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 where the function actually starts doing something interesting, over 100 lines into the function. Can we use helper methods for all of the validation above to make this easier to read? Something like validate_inputs
that does all of that in a separate function, and/or something like validate_and_retrieve_date_arg
that can be used to reduce some of the duplicated error checking code.
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.
yea, lots a validation and core code is a few lines
before_turn_on = before_turn_on + 1 | ||
grant.expiration_date = turn_on_date + relativedelta(months=+13) | ||
else: | ||
print("Warning: NULL expiration date encountered, command terminates.") |
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.
Is there a reason to terminate when encountering a null expiration date? Seems like we would just want to handle that by setting it to 13 months past the creation date.
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.
in the use case dealing with, a null is unexpected, so prefer to stop and check....
@@ -0,0 +1,141 @@ | |||
import pytz |
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.
A lot of the comments from the other file apply here as well. One thought that comes to mind, since these functions are so similar, is that we could simplify testing/review/etc to have one function that handles both of these cases. Basically it would set the expiration to 13 months after creation if its expiration is not already set; if the expiration is already set, then it would not change anything if the turnondate isn't set, or it would change it to 13 months after creation date or turnondate based on the current logic in repair_grants_expiration date. I won't insist on it, just think it simplifies a lot of this.
Thx for the feedback, changes are on the way |
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.
New code looks good! However I am having trouble testing it out:
$ python manage.py repair_grants_expiration_date --dryrun --appname 'app0_DevUserFN0.DevUserLN0' --appnameagain 'app0_DevUserFN0.DevUserLN0' --range '01/16/25 19:58:26-01/18/25 23:59:59'
Error: bad date time value: 01/16/25 19:58:26
time data '01/16/25 19:58:26' does not match format '%m/%d/%Y %H:%M:%S'
Am I doing something wrong, or is there an issue with the date format string?
apps/authorization/management/commands/repair_grants_expiration_date.py
Outdated
Show resolved
Hide resolved
Oh use 2025 |
apps/authorization/management/commands/repair_grants_expiration_date.py
Outdated
Show resolved
Hide resolved
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.
Looks good, I like the adjustments to the summary print statements as well.
JIRA Ticket:
BB2-3641
What Does This PR Do?
See 'Context" in jira ticket for details.
Some grants are impacted - their expiration_date now have incorrect values:
case1: the impacted grants have NULL in their expiration_date
case2: the impacted grants have (mostly) their expiration_date previous date extended
Added Django command ( repair_grants_expiration_date ) to repair the grants:
What Should Reviewers Watch For?
The commands can be tested locally, the repair details are described in ticket
(commands for internal use)
If you're reviewing this PR, please check for these things in particular:
Validation
Steps to test locally:
Django command: repair_grants_expiration_date:
8. Take notes of the grants associated with the app you want to test the new commands with
9. Run help of the command to see the syntax
10. FIX case1: do DRYRUN to preview result:
python manage.py repair_grants_expiration_date --dryrun --appname 'app0_DevUserFN4.DevUserLN4' --appnameagain 'app0_DevUserFN4.DevUserLN4' --range '01/21/25 03:34:12-01/21/25 03:34:16' Info: dryrun = True, this is a dry run, no change to database. process restriction: feature turned on date: None and created_at in range: [2025-01-21 03:34:12+00:00, 2025-01-21 03:34:16+00:00]. Number of grants to be checked for repair = 26 +#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+# Processed grants: 26, created after feature turned on date: 26, created before turned on date: 0
11. Check DRYRUN output, do a real run if preview looks good:
python manage.py repair_grants_expiration_date --appname 'app0_DevUserFN4.DevUserLN4' --appnameagain 'app0_DevUserFN4.DevUserLN4' --range '01/21/25 03:34:12-01/21/25 03:34:16' process restriction: feature turned on date: None and created_at in range: [2025-01-21 03:34:12+00:00, 2025-01-21 03:34:16+00:00]. Number of grants to be checked for repair = 26 +$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$ Processed grants: 26, created after feature turned on date: 26, created before turned on date: 0
12. Query the table authorization_dataaccessgrant for the grants (for app: app0_DevUserFN4.DevUserLN4), verify that the grants in the date time range have their expiration_date set to their created_at + 13month.
13. FIX case2 (--turnondate present): do DRYRUN
python manage.py repair_grants_expiration_date --dryrun --appname 'app0_DevUserFN4.DevUserLN4' --appnameagain 'app0_DevUserFN4.DevUserLN4' --range '01/21/25 03:34:11-01/21/25 03:34:17' --turnondate '01/21/25 03:34:15' Info: dryrun = True, this is a dry run, no change to database. process restriction: feature turned on date: 2025-01-21 03:34:15+00:00 and created_at in range: [2025-01-21 03:34:11+00:00, 2025-01-21 03:34:17+00:00]. Number of grants to be checked for repair = 50 -#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#-#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#+#-#+# Processed grants: 50, created after feature turned on date: 20, created before turned on date: 30
14. Check DRYRUN output, do a real run if preview looks good:
python manage.py repair_grants_expiration_date --appname 'app0_DevUserFN4.DevUserLN4' --appnameagain 'app0_DevUserFN4.DevUserLN4' --range '01/21/25 03:34:11-01/21/25 03:34:17' --turnondate '01/21/25 03:34:15' process restriction: feature turned on date: 2025-01-21 03:34:15+00:00 and created_at in range: [2025-01-21 03:34:11+00:00, 2025-01-21 03:34:17+00:00]. Number of grants to be checked for repair = 50 -$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$-$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$+$-$+$ Processed grants: 50, created after feature turned on date: 20, created before turned on date: 30
15. Query the table authorization_dataaccessgrant for the grants (for app: app0_DevUserFN4.DevUserLN4), verify that the grants in the date time range (50) have their expiration_date set to their created_at + 13month if the created_at is after the turn_on_date (20), otherwise the expiration_date is set to turn_on_date + 13month (30).
What Security Implications Does This PR Have?
Please indicate if this PR does any of the following:
security engineer's approval.
Any Migrations?
etc)