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.

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shadinaif
Copy link
Collaborator

@shadinaif shadinaif commented Oct 18, 2024

feat: management command to clean up roles for all users

Dry run:

python manage.py lms course_access_roles_clean_up

Commit:

python manage.py lms course_access_roles_clean_up --commit=yes

The command will simply run on all users with roles and perform update_course_access_roles with the same roles returned by UserRolesSerializer. It'll also give details about records that cannot be automatically cleaned for any reason

Copy link
Collaborator

@OmarIthawi OmarIthawi left a 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.

Comment on lines 77 to 105
if not user.is_active:
print('**** User is not active, skipping..')
continue
Copy link
Collaborator

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):
Copy link
Collaborator

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,
Copy link
Collaborator

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:
Copy link
Collaborator

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.

@shadinaif shadinaif force-pushed the shadinaif/roles-cleanup-command branch 5 times, most recently from f8f8273 to de295c9 Compare October 21, 2024 14:56
@shadinaif shadinaif force-pushed the shadinaif/roles-cleanup-command branch 2 times, most recently from 11d7780 to 4036c95 Compare October 22, 2024 06:58
@shadinaif shadinaif force-pushed the shadinaif/roles-cleanup-command branch from 4036c95 to 31d5e59 Compare October 22, 2024 07:12
@shadinaif shadinaif marked this pull request as draft October 31, 2024 04:45
@shadinaif
Copy link
Collaborator Author

I'll get back to this later. Tehreem is working on adding library support. I'll update my code after that to make it more robust

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants