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

Delete participations of long inactive users #2329

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
517af89
Add PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS to settings.py with …
Redflashx12 Oct 21, 2024
b36e41e
Add bulk delete of inactive users from their evaluations
Redflashx12 Oct 21, 2024
6c9f948
Update to use timedelta and just mentioning months instead of convert…
Redflashx12 Oct 21, 2024
bdfb350
Filter users which do not have any evaluations in which they are inac…
Redflashx12 Oct 21, 2024
582aba5
change unit to days since months is not supported and add small comment
Redflashx12 Oct 21, 2024
cc3e1b2
fix typo
Redflashx12 Oct 21, 2024
b4cf95c
fix correct date subtraction
Redflashx12 Oct 21, 2024
8c1a755
Add default value for test_run
Redflashx12 Oct 21, 2024
4e1fcf5
WIP test for inactivity
Redflashx12 Oct 21, 2024
7dcfa62
Refactor functionality to dedicated function
Redflashx12 Nov 4, 2024
a2db34e
Fix test by archiving the semester and using settings value in test f…
Redflashx12 Nov 4, 2024
0b810cf
Remove unneeded +1 and brackets, add evaluation needed to not be mark…
Redflashx12 Nov 4, 2024
937acad
refactor and split into two tests
Redflashx12 Nov 4, 2024
7852f2e
Fix indentation
Redflashx12 Nov 4, 2024
60301ad
Forgot to remove factor 30 for months since it's already included, ne…
Redflashx12 Nov 4, 2024
a4475e0
black formatting
Redflashx12 Nov 4, 2024
0a33410
isort formatting
Redflashx12 Nov 4, 2024
5f10109
increase test coverage by adding test case for test run
Redflashx12 Nov 4, 2024
6cc1cf4
Fix django import of settings
Redflashx12 Nov 11, 2024
87bbf6d
fix to use pythonic way of checking len
Redflashx12 Nov 11, 2024
b198732
Use timedelta instead of numeric value for days
Redflashx12 Nov 11, 2024
d5fa9c6
Fix logic
Redflashx12 Nov 11, 2024
c9f3a0a
Add inactive users to be removed
Redflashx12 Nov 11, 2024
51b5f99
Change django magic query to look and check latest vote_end_date
Redflashx12 Nov 11, 2024
2f5711c
Refactor test to use django's setUpTestData
Redflashx12 Nov 11, 2024
74e9076
"Fix" type hint to use | None instead of Optional
Redflashx12 Nov 11, 2024
1dc1fb7
reorder imports
Redflashx12 Nov 11, 2024
36a5a00
linting
Redflashx12 Nov 11, 2024
01d60a2
Add more edge cases to tests
Redflashx12 Nov 11, 2024
03622d6
Remove assignment of never used variable
Redflashx12 Nov 11, 2024
c526f84
Merge branch 'main' into main
Redflashx12 Nov 11, 2024
810a957
refactor `remove_inactive_participations`
niklasmohrin Nov 18, 2024
2ab9cff
Rename variable
Redflashx12 Nov 18, 2024
6715f77
Implement feedback
Redflashx12 Nov 18, 2024
351fbaa
Fix logic error and wrong type
Redflashx12 Nov 18, 2024
08983c3
Fix missing rename of variable in @override_settings
Redflashx12 Nov 18, 2024
eebef2b
Extend to test for user.evaluations_participating_in.exists() and rep…
Redflashx12 Nov 18, 2024
cd63d80
Use mock.patch decorator instead of creating new Evaluations with use…
Redflashx12 Nov 18, 2024
3db7709
Remove unused import
Redflashx12 Nov 18, 2024
32a4c79
ruff fix
Redflashx12 Nov 18, 2024
3364e5c
Use proper plural formatting
Redflashx12 Jan 6, 2025
0b863c6
Use proper plural formatting
Redflashx12 Jan 6, 2025
58d239d
Fix missing import of ngettext
Redflashx12 Jan 6, 2025
fc9a393
Update assert in tests to use the new phrasing
Redflashx12 Jan 20, 2025
83754ea
Implement feedback from Richard
Redflashx12 Jan 20, 2025
6a7f0de
Remove last elif check which is unnecessary
Redflashx12 Jan 20, 2025
194e3cd
lint
Redflashx12 Jan 20, 2025
2a4553e
format
Redflashx12 Jan 20, 2025
0efe340
Implement feedback: more explicit test and renamed function
Redflashx12 Jan 27, 2025
876f619
Fix wrong rename
Redflashx12 Jan 27, 2025
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
Prev Previous commit
Next Next commit
ruff fix
Redflashx12 committed Nov 18, 2024
commit 32a4c79e22a922a20525b0a6b4adc2e9b61c0894
5 changes: 2 additions & 3 deletions evap/staff/tools.py
Original file line number Diff line number Diff line change
@@ -139,7 +139,7 @@
elif user.is_active and user.can_be_marked_inactive_by_manager:
users_to_mark_inactive.append(user)
elif not user.is_active:
Copy link
Member

Choose a reason for hiding this comment

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

The elif here seems suspicious to me. An inactive user that has can_be_marked_inactive_by_manager == False wouldn't end up in inactive_users, and thus we wouldn't call remove_inactive_participations. I don't know whether such a user can exist, but I'd prefer to not have to think about this.

I think the loop here was meaningful and concise before, and just handled user deletion (or deactivation, if deletion isn't possible). I'd view removal of participations as orthogonal, and try to keep these as separate as possible, i.e., first just handle all deletion/marking inactive, and then handle participation archival on the remaining users. What do you think about that?

Copy link
Collaborator Author

@Redflashx12 Redflashx12 Jan 20, 2025

Choose a reason for hiding this comment

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

I added that line because the requirement in the original issue #2176 mentioned that "long inactive users are users who are inactive (or can_be_marked_inactive_by_manager)", but I just checked the if condition and set a breakpoint and during the bulk update test nothing was ever added to that list, so it's probably unnecessary. Logically that would mean that the evaluations.count() > 0 and this and-statement of can_be_deleted_by_manageris is not true, otherwise they would be added to deletable_users :
self.participations_are_archived and self.grade_documents_are_deleted and self.results_are_archived

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also asked Johannes and he mentioned it's also not necessary. So I just deleted those lines.

inactive_users.append(user)

Check warning on line 142 in evap/staff/tools.py

Codecov / codecov/patch

evap/staff/tools.py#L142

Added line #L142 was not covered by tests

messages.info(
request,
@@ -199,7 +199,7 @@
messages.warning(request, message)
for user in users_to_mark_inactive + inactive_users:
for message in remove_inactive_participations(user, test_run):
messages.warning(request, message)

Check warning on line 202 in evap/staff/tools.py

Codecov / codecov/patch

evap/staff/tools.py#L202

Added line #L202 was not covered by tests
Copy link
Member

@niklasmohrin niklasmohrin Jan 27, 2025

Choose a reason for hiding this comment

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

This line is not covered by tests, because we only test the implementation of remove_inactive_participations - should we modify TestUserBulkUpdateView to include this? @richardebeling @Kakadus

It would boil down to setting up a situation where a user is marked as inactive and checking that the message appears.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure whether you want me to implement this and if so, where can I find the TestUserBuildUpdateView? I didn't find it when searching through the project files.

Copy link
Member

Choose a reason for hiding this comment

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

Typo, it's supposed to be bulk update (TestUserBulkUpdateView). Since @richardebeling seems to agree, I think we would want a short test in that class so that some of the warnings you added are shown

if test_run:
messages.info(request, _("No data was changed in this test run."))
else:
@@ -396,9 +396,8 @@
return [
_("{} will be removed from {} participation(s) due to inactivity.").format(user.full_name, evaluation_count)
Redflashx12 marked this conversation as resolved.
Show resolved Hide resolved
]
else:
user.evaluations_participating_in.clear()
return [_("Removed {} from {} participation(s) due to inactivity.").format(user.full_name, evaluation_count)]
user.evaluations_participating_in.clear()
return [_("Removed {} from {} participation(s) due to inactivity.").format(user.full_name, evaluation_count)]
Redflashx12 marked this conversation as resolved.
Show resolved Hide resolved


def user_edit_link(user_id):