-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: management command to clean up roles for all users #103
base: main
Are you sure you want to change the base?
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.
Thanks @shadinaif! Looks great!
I've added few suggestions.
if not user.is_active: | ||
print('**** User is not active, skipping..') | ||
continue |
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.
Inactive role should lose all of its roles to avoid data accidential data breaches.
if not user.is_active: | ||
print('**** User is not active, skipping..') | ||
continue | ||
if is_system_staff_user(user): |
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 staff should lose all of its roles to avoid needless permissions appearing on the admin pages.
invalid_orgs = CourseAccessRole.objects.filter( | ||
user_id=user_id, | ||
).exclude( | ||
org__in=all_orgs, |
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 think this is case-sensitive, which shouldn't be.
type=str, | ||
) | ||
|
||
def handle(self, *args: list, **options: Dict[str, str]) -> None: |
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.
Please move the def handle
into a clean_roles
util function, this would help testing and actual usage in the future by /admin
panel actions.
f8f8273
to
de295c9
Compare
11d7780
to
4036c95
Compare
4036c95
to
31d5e59
Compare
I'll get back to this later. Tehreem is working on adding |
feat: management command to clean up roles for all users
Dry run:
Commit:
The command will simply run on all users with roles and perform
update_course_access_roles
with the same roles returned byUserRolesSerializer
. It'll also give details about records that cannot be automatically cleaned for any reason