feat(users): add user archiving feature and update tests#1901
Conversation
|
coverage: 78.964% (-0.3%) from 79.252% — Pwngo:feature/archive-users into tjcsl:dev |
aarushtools
left a comment
aarushtools
left a comment
There was a problem hiding this comment.
Hi Ethan,
Thanks for the PR, you're off to a good start. In addition to my comments, does this PR do other things like removing archived users from search results? Remember it should function basically like a delete but without actually deleting the user.
|
|
||
| def handle_archive(self, *, senior_grad_year: int): | ||
| users = get_user_model().objects.filter(graduation_year__lt=senior_grad_year).exclude(user_type="alum") | ||
| archived_count, already_archived_count = get_user_model().archive_users(users) |
There was a problem hiding this comment.
I think this should be a queryset method instead of a manager method, because the input is always a filtered collection
| archived_count, already_archived_count = user_model.archive_users(users) | ||
| self.stdout.write(self.style.SUCCESS(f"Archived users: {archived_count}")) | ||
| if already_archived_count: | ||
| self.stdout.write(self.style.WARNING(f"Already archived users: {already_archived_count}")) |
There was a problem hiding this comment.
I like what you're doing with the identifiers for filtering being derived from the CSV columns, but I think it's better if you hardcode it in to prevent anything bad from accidentally happening. Use username as the only column in the spreadsheet
|
|
||
| @admin.register(User) | ||
| class UserAdmin(admin.ModelAdmin): | ||
| @admin.action(description="Archive selected users") |
|
|
||
| return not self.username.startswith("INVALID_USER") and not self.user_locked | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
docstrings (although it will be in a different place since I don't want this on the manager)
Pwngo
force-pushed
the
feature/archive-users
branch
from
1e3e2e8 to
d784cf4
Compare
|
Archiving sets user_locked=True on users, and search results are filtered to user.is_active only. is_active returns False when user_locked=True, so archived users are excluded from search results. |
3 participants
Proposed changes
Brief description of rationale
Closes #1815