Skip to content
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.

Sign up for GitHub

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

Jump to bottom

BB2-3243: Add --all option to update_access_grants Django command #1204

Merged
merged 3 commits into from
Jun 6, 2024
Merged

BB2-3243: Add --all option to update_access_grants Django command #1204

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d3693a
Added --all option
loganbertram Jun 5, 2024
2e1d95b
Revert dockerfile change
loganbertram Jun 5, 2024
648aaf6
Removed extra lines
loganbertram Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Added --all option
  • Loading branch information
@loganbertram
loganbertram committed Jun 5, 2024
commit 0d3693aa16bf62131491958570aa27852365a16e
2 changes: 1 addition & 1 deletion Dockerfile.selenium
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM selenium/standalone-chrome
FROM selenium/standalone-chromium

ENV PYTHONUNBUFFERED 1
USER root
Expand Down
14 changes: 11 additions & 3 deletions apps/authorization/management/commands/update_access_grants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,35 @@
from django.core.management.base import BaseCommand
from apps.authorization.models import DataAccessGrant
from apps.dot_ext.models import Application


Comment on lines +4 to +5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an accidental extra new line, or is this an automatic formatter thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purposeful to match PEP 8 standard on spacing around class declarations. We should get the team using PEP 8 tools in their IDE. I fix a lot of stuff like this reflexively when I see it because PyCharm yells at me if its not to the standard.

class Command(BaseCommand):
help = (
'Update Access Grants Per Application.'
'Pass in a list of application ids.'
)

def add_arguments(self, parser):
parser.add_argument('--applications', help="list of applications to update "
parser.add_argument('--applications', help="List of applications to update "
"id, comma separated: "
"eg. 1,2,3 ")
"eg. 1,2,3. Supersedes --all")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of superseding --all, it might be clearer to have the two options be mutually exclusive. Optional, just an idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we were introducing outside our team I probably would, lol. I may still! I thought that for us and future internal teams, the help string would make it make sense and defaulting to the less destructive option would at least limit accidents.

parser.add_argument('--all', action='store_true', help="Update access grants for all applications")
parser.set_defaults(all=False)


def handle(self, *args, **options):
if options['applications']:
application_ids = options['applications'].split(',')
elif options['all']:
application_ids = Application.objects.values_list('id', flat=True)
Comment on lines +22 to +23
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure the django orm compiles very similar sql with this pattern to if i iterated over the queryset directly below. I think this way is more readable, but may shuffle the whole thing to make the options mutually exclusive.

else:
print("You must pass at least one application to update.")
print("You must pass at least one application to update or use the --all option.")
return False
for app_id in application_ids:
application = Application.objects.get(id=app_id)
grants = DataAccessGrant.objects.filter(application=app_id)
if not grants:
continue
if "ONE_TIME" in application.data_access_type:
grants.delete()
elif "THIRTEEN_MONTH" in application.data_access_type:
Expand Down
Loading