From 517af89238a8b5e66a23fccca080548797ada1f6 Mon Sep 17 00:00:00 2001 From: anton Date: Mon, 21 Oct 2024 18:40:21 +0200 Subject: [PATCH 01/49] Add PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS to settings.py with default of 18 months --- evap/settings.py | 1 + 1 file changed, 1 insertion(+) diff --git a/evap/settings.py b/evap/settings.py index da524e4446..79d3c379be 100644 --- a/evap/settings.py +++ b/evap/settings.py @@ -36,6 +36,7 @@ VOTER_COUNT_NEEDED_FOR_PUBLISHING_RATING_RESULTS = 2 VOTER_PERCENTAGE_NEEDED_FOR_PUBLISHING_AVERAGE_GRADE = 0.2 SMALL_COURSE_SIZE = 5 # up to which number of participants the evaluation gets additional warnings about anonymity +PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS = 18 * 60 * 60 * 24 * 30 # a warning is shown next to results where less than RESULTS_WARNING_COUNT answers were given # or the number of answers is less than RESULTS_WARNING_PERCENTAGE times the median number of answers (for this question in this evaluation) From b36e41eda53da62231cfaa2ec998b274d4201b31 Mon Sep 17 00:00:00 2001 From: anton Date: Mon, 21 Oct 2024 18:45:31 +0200 Subject: [PATCH 02/49] Add bulk delete of inactive users from their evaluations --- evap/staff/tools.py | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 23b10fe42c..0d6e130e16 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -132,12 +132,16 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 users_to_be_updated.append((matching_user, imported_email)) emails_of_non_obsolete_users = set(imported_emails) | {user.email for user, _ in users_to_be_updated} - deletable_users, users_to_mark_inactive = [], [] + deletable_users, users_to_mark_inactive, inactive_users_participation = [], [], [] for user in UserProfile.objects.exclude(email__in=emails_of_non_obsolete_users): if user.can_be_deleted_by_manager: deletable_users.append(user) elif user.is_active and user.can_be_marked_inactive_by_manager: users_to_mark_inactive.append(user) + evaluations = Evaluation.objects.filter(participants__in=user, + state=Evaluation.State.PUBLISHED, + vote_end_date__lt=datetime.now() - settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS) + inactive_users_participation.append((user, evaluations)) messages.info( request, @@ -195,6 +199,9 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 user, deletable_users + users_to_mark_inactive, test_run ): messages.warning(request, message) + for user, evaluations in inactive_users_participation: + for message in remove_inactivate_participations(user, evaluations, test_run): + messages.warning(request, message) if test_run: messages.info(request, _("No data was changed in this test run.")) else: @@ -203,6 +210,7 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 for user in users_to_mark_inactive: user.is_active = False user.save() + for user, email in users_to_be_updated: user.email = email user.save() @@ -375,6 +383,22 @@ def remove_user_from_represented_and_ccing_users(user, ignored_users=None, test_ return remove_messages +def remove_inactivate_participations(user, evaluations, test_run): + remove_messages = [] + if test_run: + remove_messages.append( + _("{} will be removed from {} participation(s) due to inactivity.").format(user.full_name, len(evaluations)) + ) + else: + for evaluation in evaluations: + evaluation.participants.remove(user) + remove_messages.append( + _("Removed {} from {} participation(s) due to inactivity.").format(user.full_name, len(evaluations)) + ) + return remove_messages + + + def user_edit_link(user_id): return format_html( ' {}', From 6c9f948e4bd1843e8dc3ba3223b9d60ca3bf6f0d Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 21 Oct 2024 20:47:30 +0200 Subject: [PATCH 03/49] Update to use timedelta and just mentioning months instead of converting months to seconds --- evap/settings.py | 2 +- evap/staff/tools.py | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/evap/settings.py b/evap/settings.py index 79d3c379be..8d694ff1c6 100644 --- a/evap/settings.py +++ b/evap/settings.py @@ -36,7 +36,7 @@ VOTER_COUNT_NEEDED_FOR_PUBLISHING_RATING_RESULTS = 2 VOTER_PERCENTAGE_NEEDED_FOR_PUBLISHING_AVERAGE_GRADE = 0.2 SMALL_COURSE_SIZE = 5 # up to which number of participants the evaluation gets additional warnings about anonymity -PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS = 18 * 60 * 60 * 24 * 30 +PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS = 18 # a warning is shown next to results where less than RESULTS_WARNING_COUNT answers were given # or the number of answers is less than RESULTS_WARNING_PERCENTAGE times the median number of answers (for this question in this evaluation) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 0d6e130e16..e497545f8b 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -138,9 +138,8 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 deletable_users.append(user) elif user.is_active and user.can_be_marked_inactive_by_manager: users_to_mark_inactive.append(user) - evaluations = Evaluation.objects.filter(participants__in=user, - state=Evaluation.State.PUBLISHED, - vote_end_date__lt=datetime.now() - settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS) + evaluations = user.evaluations_participating_in.filter(state=Evaluation.State.PUBLISHED, + vote_end_date__lt=datetime.today() - timedelta(-settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS)) inactive_users_participation.append((user, evaluations)) messages.info( From bdfb350e30d4bb3a25da6cf61425922636210d31 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 21 Oct 2024 20:48:20 +0200 Subject: [PATCH 04/49] Filter users which do not have any evaluations in which they are inactive --- evap/staff/tools.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index e497545f8b..cbb91370b7 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -199,8 +199,9 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 ): messages.warning(request, message) for user, evaluations in inactive_users_participation: - for message in remove_inactivate_participations(user, evaluations, test_run): - messages.warning(request, message) + if len(evaluations) > 0: + for message in remove_inactivate_participations(user, evaluations, test_run): + messages.warning(request, message) if test_run: messages.info(request, _("No data was changed in this test run.")) else: From 582aba5288c4792204ab21bd79e70627bdf1b096 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 21 Oct 2024 20:58:53 +0200 Subject: [PATCH 05/49] change unit to days since months is not supported and add small comment --- evap/settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evap/settings.py b/evap/settings.py index 8d694ff1c6..735211b720 100644 --- a/evap/settings.py +++ b/evap/settings.py @@ -36,7 +36,7 @@ VOTER_COUNT_NEEDED_FOR_PUBLISHING_RATING_RESULTS = 2 VOTER_PERCENTAGE_NEEDED_FOR_PUBLISHING_AVERAGE_GRADE = 0.2 SMALL_COURSE_SIZE = 5 # up to which number of participants the evaluation gets additional warnings about anonymity -PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS = 18 +PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS = 18 * 30 # unit in days, i.e. 18 months = 18 * 30 # a warning is shown next to results where less than RESULTS_WARNING_COUNT answers were given # or the number of answers is less than RESULTS_WARNING_PERCENTAGE times the median number of answers (for this question in this evaluation) From cc3e1b233ac7b361424fef858bf61abf8601e5e7 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 21 Oct 2024 21:03:00 +0200 Subject: [PATCH 06/49] fix typo --- evap/staff/tools.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index cbb91370b7..4589da0fed 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -200,7 +200,7 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 messages.warning(request, message) for user, evaluations in inactive_users_participation: if len(evaluations) > 0: - for message in remove_inactivate_participations(user, evaluations, test_run): + for message in remove_inactive_participations(user, evaluations, test_run): messages.warning(request, message) if test_run: messages.info(request, _("No data was changed in this test run.")) @@ -383,7 +383,7 @@ def remove_user_from_represented_and_ccing_users(user, ignored_users=None, test_ return remove_messages -def remove_inactivate_participations(user, evaluations, test_run): +def remove_inactive_participations(user, evaluations, test_run): remove_messages = [] if test_run: remove_messages.append( From b4cf95cf49005b1a11da162cf057d9f55b495bc7 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 21 Oct 2024 21:04:35 +0200 Subject: [PATCH 07/49] fix correct date subtraction --- evap/staff/tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 4589da0fed..4ae194ea62 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -139,7 +139,7 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 elif user.is_active and user.can_be_marked_inactive_by_manager: users_to_mark_inactive.append(user) evaluations = user.evaluations_participating_in.filter(state=Evaluation.State.PUBLISHED, - vote_end_date__lt=datetime.today() - timedelta(-settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS)) + vote_end_date__lt=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS)) inactive_users_participation.append((user, evaluations)) messages.info( From 8c1a7559457a5e65131b80465fd4ca0e95468d24 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 21 Oct 2024 21:10:46 +0200 Subject: [PATCH 08/49] Add default value for test_run --- evap/staff/tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 4ae194ea62..1c86745775 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -383,7 +383,7 @@ def remove_user_from_represented_and_ccing_users(user, ignored_users=None, test_ return remove_messages -def remove_inactive_participations(user, evaluations, test_run): +def remove_inactive_participations(user, evaluations, test_run=False): remove_messages = [] if test_run: remove_messages.append( From 4e1fcf509ae66766c725a24bb498c282b7ca12d8 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 21 Oct 2024 21:11:13 +0200 Subject: [PATCH 09/49] WIP test for inactivity --- evap/staff/tests/test_tools.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index cab664fd59..3f5266932e 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -1,3 +1,4 @@ +from datetime import timedelta, datetime from io import BytesIO from itertools import cycle, repeat from unittest.mock import MagicMock, patch @@ -22,7 +23,7 @@ conditional_escape, merge_users, remove_user_from_represented_and_ccing_users, - user_edit_link, + user_edit_link, remove_inactive_participations, ) from evap.tools import assert_not_none from tools.enrollment_preprocessor import run_preprocessor @@ -218,6 +219,21 @@ def test_do_nothing_if_test_run(self): self.assertEqual([set(user2.delegates.all()), set(user2.cc_users.all())], [{delete_user}, {delete_user}]) self.assertEqual(len(messages), 4) +class RemoveUserDueToInactivity(TestCase): + def test_remove_user_due_to_inactivity(self): + inactive_user = baker.make(UserProfile) + inactive_user2 = baker.make(UserProfile) + + evaluation1 = baker.make(Evaluation, state=Evaluation.State.PUBLISHED, vote_end_date=datetime.today() + timedelta(days=(18+1)*30), participants=[inactive_user, inactive_user2]) + evaluation2 = baker.make(Evaluation, state=Evaluation.State.PUBLISHED, vote_end_date=datetime.today() + timedelta(days=(18-1)*30), participants=[inactive_user, inactive_user2]) + + + messages = remove_inactive_participations(inactive_user, evaluation1) + self.assertEqual(len(messages), 1) + + messages2 = remove_inactive_participations(inactive_user, evaluation2) + self.assertEqual(len(messages2), 0) + class UserEditLinkTest(TestCase): def test_user_edit_link(self): From 7dcfa6232e8577e5e6436337dacc1df5ce2c2b24 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 4 Nov 2024 19:00:58 +0100 Subject: [PATCH 10/49] Refactor functionality to dedicated function --- evap/staff/tools.py | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 1c86745775..4b246e6b1c 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -132,15 +132,13 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 users_to_be_updated.append((matching_user, imported_email)) emails_of_non_obsolete_users = set(imported_emails) | {user.email for user, _ in users_to_be_updated} - deletable_users, users_to_mark_inactive, inactive_users_participation = [], [], [] + deletable_users, users_to_mark_inactive = [], [] for user in UserProfile.objects.exclude(email__in=emails_of_non_obsolete_users): if user.can_be_deleted_by_manager: deletable_users.append(user) elif user.is_active and user.can_be_marked_inactive_by_manager: users_to_mark_inactive.append(user) - evaluations = user.evaluations_participating_in.filter(state=Evaluation.State.PUBLISHED, - vote_end_date__lt=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS)) - inactive_users_participation.append((user, evaluations)) + messages.info( request, @@ -198,10 +196,9 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 user, deletable_users + users_to_mark_inactive, test_run ): messages.warning(request, message) - for user, evaluations in inactive_users_participation: - if len(evaluations) > 0: - for message in remove_inactive_participations(user, evaluations, test_run): - messages.warning(request, message) + for user in users_to_mark_inactive: + for message in remove_inactive_participations(user, test_run): + messages.warning(request, message) if test_run: messages.info(request, _("No data was changed in this test run.")) else: @@ -383,18 +380,23 @@ def remove_user_from_represented_and_ccing_users(user, ignored_users=None, test_ return remove_messages -def remove_inactive_participations(user, evaluations, test_run=False): +def remove_inactive_participations(user, test_run=False): remove_messages = [] - if test_run: - remove_messages.append( - _("{} will be removed from {} participation(s) due to inactivity.").format(user.full_name, len(evaluations)) - ) - else: - for evaluation in evaluations: - evaluation.participants.remove(user) - remove_messages.append( - _("Removed {} from {} participation(s) due to inactivity.").format(user.full_name, len(evaluations)) - ) + evaluations = [] + if user.is_active and user.can_be_marked_inactive_by_manager: + evaluations = user.evaluations_participating_in.filter(state=Evaluation.State.PUBLISHED, + vote_end_date__lt=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS)) + if len(evaluations) > 0: + if test_run: + remove_messages.append( + _("{} will be removed from {} participation(s) due to inactivity.").format(user.full_name, len(evaluations)) + ) + else: + for evaluation in evaluations: + evaluation.participants.remove(user) + remove_messages.append( + _("Removed {} from {} participation(s) due to inactivity.").format(user.full_name, len(evaluations)) + ) return remove_messages From a2db34ed5b262f6ac40f38312ddbab45cd99cda6 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 4 Nov 2024 19:28:21 +0100 Subject: [PATCH 11/49] Fix test by archiving the semester and using settings value in test for vote start/end date instead of flat value --- evap/staff/tests/test_tools.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 3f5266932e..710d870bb4 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -10,6 +10,7 @@ from model_bakery import baker from openpyxl import load_workbook +from evap import settings from evap.evaluation.models import Contribution, Course, Evaluation, UserProfile from evap.evaluation.tests.tools import assert_no_database_modifications from evap.rewards.models import RewardPointGranting, RewardPointRedemption @@ -221,17 +222,23 @@ def test_do_nothing_if_test_run(self): class RemoveUserDueToInactivity(TestCase): def test_remove_user_due_to_inactivity(self): - inactive_user = baker.make(UserProfile) - inactive_user2 = baker.make(UserProfile) + inactive_user = baker.make(UserProfile, last_name="Inactive User") + active_user = baker.make(UserProfile, last_name="Active User") - evaluation1 = baker.make(Evaluation, state=Evaluation.State.PUBLISHED, vote_end_date=datetime.today() + timedelta(days=(18+1)*30), participants=[inactive_user, inactive_user2]) - evaluation2 = baker.make(Evaluation, state=Evaluation.State.PUBLISHED, vote_end_date=datetime.today() + timedelta(days=(18-1)*30), participants=[inactive_user, inactive_user2]) + inactive_evaluation = baker.make(Evaluation, state=Evaluation.State.PUBLISHED, + vote_start_datetime=datetime.today() - timedelta(days=(settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS+1)*30), + vote_end_date=datetime.today() - timedelta(days=(settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS+1)*30), + participants=[inactive_user]) + semester = inactive_evaluation.course.semester + semester.archive() - messages = remove_inactive_participations(inactive_user, evaluation1) + messages = remove_inactive_participations(inactive_user) + self.assertTrue(inactive_user.can_be_marked_inactive_by_manager) self.assertEqual(len(messages), 1) - messages2 = remove_inactive_participations(inactive_user, evaluation2) + messages2 = remove_inactive_participations(active_user) + self.assertFalse(active_user.can_be_marked_inactive_by_manager) self.assertEqual(len(messages2), 0) From 0b810cf042ca744db61cecb03eff281be3a62a44 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 4 Nov 2024 20:01:56 +0100 Subject: [PATCH 12/49] Remove unneeded +1 and brackets, add evaluation needed to not be marked as "can_be_marked_inactive_by_manager" --- evap/staff/tests/test_tools.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 710d870bb4..3af8b81fc7 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -226,9 +226,10 @@ def test_remove_user_due_to_inactivity(self): active_user = baker.make(UserProfile, last_name="Active User") inactive_evaluation = baker.make(Evaluation, state=Evaluation.State.PUBLISHED, - vote_start_datetime=datetime.today() - timedelta(days=(settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS+1)*30), - vote_end_date=datetime.today() - timedelta(days=(settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS+1)*30), + vote_start_datetime=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS * 30), + vote_end_date=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS * 30), participants=[inactive_user]) + active_evaluation = baker.make(Evaluation, state=Evaluation.State.PUBLISHED, participants=[active_user]) semester = inactive_evaluation.course.semester semester.archive() From 937acad58c091b92fcf15bf6886343c8f0a0ce5d Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 4 Nov 2024 20:05:57 +0100 Subject: [PATCH 13/49] refactor and split into two tests --- evap/staff/tests/test_tools.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 3af8b81fc7..64b636e63c 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -223,13 +223,10 @@ def test_do_nothing_if_test_run(self): class RemoveUserDueToInactivity(TestCase): def test_remove_user_due_to_inactivity(self): inactive_user = baker.make(UserProfile, last_name="Inactive User") - active_user = baker.make(UserProfile, last_name="Active User") - inactive_evaluation = baker.make(Evaluation, state=Evaluation.State.PUBLISHED, vote_start_datetime=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS * 30), vote_end_date=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS * 30), participants=[inactive_user]) - active_evaluation = baker.make(Evaluation, state=Evaluation.State.PUBLISHED, participants=[active_user]) semester = inactive_evaluation.course.semester semester.archive() @@ -238,9 +235,14 @@ def test_remove_user_due_to_inactivity(self): self.assertTrue(inactive_user.can_be_marked_inactive_by_manager) self.assertEqual(len(messages), 1) - messages2 = remove_inactive_participations(active_user) + def test_do_not_remove_user_due_to_inactivity(self): + active_user = baker.make(UserProfile, last_name="Active User") + + baker.make(Evaluation, state=Evaluation.State.PUBLISHED, participants=[active_user]) + + messages = remove_inactive_participations(active_user) self.assertFalse(active_user.can_be_marked_inactive_by_manager) - self.assertEqual(len(messages2), 0) + self.assertEqual(len(messages), 0) class UserEditLinkTest(TestCase): From 7852f2e4e9a28061c20b2bddeb1858950f80653c Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 4 Nov 2024 20:06:59 +0100 Subject: [PATCH 14/49] Fix indentation --- evap/staff/tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 4b246e6b1c..a949b57f6d 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -393,7 +393,7 @@ def remove_inactive_participations(user, test_run=False): ) else: for evaluation in evaluations: - evaluation.participants.remove(user) + evaluation.participants.remove(user) remove_messages.append( _("Removed {} from {} participation(s) due to inactivity.").format(user.full_name, len(evaluations)) ) From 60301ad8cd148235878e8c4e7d892c37a903a84d Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 4 Nov 2024 20:21:29 +0100 Subject: [PATCH 15/49] Forgot to remove factor 30 for months since it's already included, need to add +1 to get above threshold --- evap/staff/tests/test_tools.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 64b636e63c..6d6c950efd 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -224,8 +224,8 @@ class RemoveUserDueToInactivity(TestCase): def test_remove_user_due_to_inactivity(self): inactive_user = baker.make(UserProfile, last_name="Inactive User") inactive_evaluation = baker.make(Evaluation, state=Evaluation.State.PUBLISHED, - vote_start_datetime=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS * 30), - vote_end_date=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS * 30), + vote_start_datetime=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS+1), + vote_end_date=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS+1), participants=[inactive_user]) semester = inactive_evaluation.course.semester From a4475e03a4a4a578e86d5fdb8c2d8af7ee5dcd94 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 4 Nov 2024 20:34:27 +0100 Subject: [PATCH 16/49] black formatting --- evap/settings.py | 2 +- evap/staff/tests/test_tools.py | 16 +++++++++++----- evap/staff/tools.py | 12 +++++++----- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/evap/settings.py b/evap/settings.py index 735211b720..fe8da12bf7 100644 --- a/evap/settings.py +++ b/evap/settings.py @@ -36,7 +36,7 @@ VOTER_COUNT_NEEDED_FOR_PUBLISHING_RATING_RESULTS = 2 VOTER_PERCENTAGE_NEEDED_FOR_PUBLISHING_AVERAGE_GRADE = 0.2 SMALL_COURSE_SIZE = 5 # up to which number of participants the evaluation gets additional warnings about anonymity -PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS = 18 * 30 # unit in days, i.e. 18 months = 18 * 30 +PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS = 18 * 30 # unit in days, i.e. 18 months = 18 * 30 # a warning is shown next to results where less than RESULTS_WARNING_COUNT answers were given # or the number of answers is less than RESULTS_WARNING_PERCENTAGE times the median number of answers (for this question in this evaluation) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 6d6c950efd..05d24ea571 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -24,7 +24,8 @@ conditional_escape, merge_users, remove_user_from_represented_and_ccing_users, - user_edit_link, remove_inactive_participations, + user_edit_link, + remove_inactive_participations, ) from evap.tools import assert_not_none from tools.enrollment_preprocessor import run_preprocessor @@ -220,13 +221,18 @@ def test_do_nothing_if_test_run(self): self.assertEqual([set(user2.delegates.all()), set(user2.cc_users.all())], [{delete_user}, {delete_user}]) self.assertEqual(len(messages), 4) + class RemoveUserDueToInactivity(TestCase): def test_remove_user_due_to_inactivity(self): inactive_user = baker.make(UserProfile, last_name="Inactive User") - inactive_evaluation = baker.make(Evaluation, state=Evaluation.State.PUBLISHED, - vote_start_datetime=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS+1), - vote_end_date=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS+1), - participants=[inactive_user]) + inactive_evaluation = baker.make( + Evaluation, + state=Evaluation.State.PUBLISHED, + vote_start_datetime=datetime.today() + - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS + 1), + vote_end_date=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS + 1), + participants=[inactive_user], + ) semester = inactive_evaluation.course.semester semester.archive() diff --git a/evap/staff/tools.py b/evap/staff/tools.py index a949b57f6d..596a4c0fc7 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -139,7 +139,6 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 elif user.is_active and user.can_be_marked_inactive_by_manager: users_to_mark_inactive.append(user) - messages.info( request, _( @@ -384,12 +383,16 @@ def remove_inactive_participations(user, test_run=False): remove_messages = [] evaluations = [] if user.is_active and user.can_be_marked_inactive_by_manager: - evaluations = user.evaluations_participating_in.filter(state=Evaluation.State.PUBLISHED, - vote_end_date__lt=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS)) + evaluations = user.evaluations_participating_in.filter( + state=Evaluation.State.PUBLISHED, + vote_end_date__lt=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS), + ) if len(evaluations) > 0: if test_run: remove_messages.append( - _("{} will be removed from {} participation(s) due to inactivity.").format(user.full_name, len(evaluations)) + _("{} will be removed from {} participation(s) due to inactivity.").format( + user.full_name, len(evaluations) + ) ) else: for evaluation in evaluations: @@ -400,7 +403,6 @@ def remove_inactive_participations(user, test_run=False): return remove_messages - def user_edit_link(user_id): return format_html( ' {}', From 0a33410f130b877460909fd7e10a0a193b2e48dd Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 4 Nov 2024 21:00:42 +0100 Subject: [PATCH 17/49] isort formatting --- evap/staff/tests/test_tools.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 05d24ea571..88fe3941d0 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -1,4 +1,4 @@ -from datetime import timedelta, datetime +from datetime import datetime, timedelta from io import BytesIO from itertools import cycle, repeat from unittest.mock import MagicMock, patch @@ -23,9 +23,9 @@ from evap.staff.tools import ( conditional_escape, merge_users, + remove_inactive_participations, remove_user_from_represented_and_ccing_users, user_edit_link, - remove_inactive_participations, ) from evap.tools import assert_not_none from tools.enrollment_preprocessor import run_preprocessor From 5f101095a72603831cc725e34eb2f711b17f606b Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 4 Nov 2024 21:25:05 +0100 Subject: [PATCH 18/49] increase test coverage by adding test case for test run --- evap/staff/tests/test_tools.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 88fe3941d0..e966ab7247 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -250,6 +250,23 @@ def test_do_not_remove_user_due_to_inactivity(self): self.assertFalse(active_user.can_be_marked_inactive_by_manager) self.assertEqual(len(messages), 0) + def test_do_nothing_if_test_run(self): + inactive_user = baker.make(UserProfile, last_name="Test Run User") + inactive_evaluation = baker.make( + Evaluation, + state=Evaluation.State.PUBLISHED, + vote_start_datetime=datetime.today() + - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS + 1), + vote_end_date=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS + 1), + participants=[inactive_user], + ) + + inactive_evaluation.course.semester.archive() + + messages = remove_inactive_participations(inactive_user, test_run=True) + self.assertTrue(inactive_user.can_be_marked_inactive_by_manager) + self.assertEqual(len(messages), 1) + class UserEditLinkTest(TestCase): def test_user_edit_link(self): From 6cc1cf457cb2339a2d3826ffe7caf6de6920a17a Mon Sep 17 00:00:00 2001 From: Redflashx12 <43482469+Redflashx12@users.noreply.github.com> Date: Mon, 11 Nov 2024 17:43:04 +0100 Subject: [PATCH 19/49] Fix django import of settings Co-authored-by: Richard Ebeling --- evap/staff/tests/test_tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index e966ab7247..76e2feca0b 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -10,7 +10,7 @@ from model_bakery import baker from openpyxl import load_workbook -from evap import settings +from django.conf import settings from evap.evaluation.models import Contribution, Course, Evaluation, UserProfile from evap.evaluation.tests.tools import assert_no_database_modifications from evap.rewards.models import RewardPointGranting, RewardPointRedemption From 87bbf6d41bcb4c3f5fcb1ef9450af4c3f080d359 Mon Sep 17 00:00:00 2001 From: Redflashx12 <43482469+Redflashx12@users.noreply.github.com> Date: Mon, 11 Nov 2024 18:31:53 +0100 Subject: [PATCH 20/49] fix to use pythonic way of checking len Co-authored-by: Richard Ebeling --- evap/staff/tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 596a4c0fc7..ff940dabb2 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -387,7 +387,7 @@ def remove_inactive_participations(user, test_run=False): state=Evaluation.State.PUBLISHED, vote_end_date__lt=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS), ) - if len(evaluations) > 0: + if evaluations: if test_run: remove_messages.append( _("{} will be removed from {} participation(s) due to inactivity.").format( From b198732739d8c9d2c96d14d1a177d10868731992 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 11 Nov 2024 18:11:48 +0100 Subject: [PATCH 21/49] Use timedelta instead of numeric value for days --- evap/settings.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/evap/settings.py b/evap/settings.py index fe8da12bf7..272cd8b110 100644 --- a/evap/settings.py +++ b/evap/settings.py @@ -11,6 +11,7 @@ import logging import os import sys +from datetime import timedelta from fractions import Fraction from typing import Any @@ -36,7 +37,7 @@ VOTER_COUNT_NEEDED_FOR_PUBLISHING_RATING_RESULTS = 2 VOTER_PERCENTAGE_NEEDED_FOR_PUBLISHING_AVERAGE_GRADE = 0.2 SMALL_COURSE_SIZE = 5 # up to which number of participants the evaluation gets additional warnings about anonymity -PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS = 18 * 30 # unit in days, i.e. 18 months = 18 * 30 +PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS = timedelta(days=18*30) # a warning is shown next to results where less than RESULTS_WARNING_COUNT answers were given # or the number of answers is less than RESULTS_WARNING_PERCENTAGE times the median number of answers (for this question in this evaluation) From d5fa9c6b2ab6635159b2d822e5a06b810de85956 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 11 Nov 2024 19:16:21 +0100 Subject: [PATCH 22/49] Fix logic --- evap/staff/tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index ff940dabb2..e94f778e83 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -382,7 +382,7 @@ def remove_user_from_represented_and_ccing_users(user, ignored_users=None, test_ def remove_inactive_participations(user, test_run=False): remove_messages = [] evaluations = [] - if user.is_active and user.can_be_marked_inactive_by_manager: + if not user.is_active or user.can_be_marked_inactive_by_manager: evaluations = user.evaluations_participating_in.filter( state=Evaluation.State.PUBLISHED, vote_end_date__lt=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS), From c9f3a0af3ce8a27093c52bb88ce13e6be98e9d72 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 11 Nov 2024 20:44:57 +0100 Subject: [PATCH 23/49] Add inactive users to be removed --- evap/staff/tools.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index e94f778e83..329e18930b 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -132,12 +132,14 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 users_to_be_updated.append((matching_user, imported_email)) emails_of_non_obsolete_users = set(imported_emails) | {user.email for user, _ in users_to_be_updated} - deletable_users, users_to_mark_inactive = [], [] + deletable_users, users_to_mark_inactive, inactive_users = [], [], [] for user in UserProfile.objects.exclude(email__in=emails_of_non_obsolete_users): if user.can_be_deleted_by_manager: deletable_users.append(user) elif user.is_active and user.can_be_marked_inactive_by_manager: users_to_mark_inactive.append(user) + elif not user.is_active: + inactive_users.append(user) messages.info( request, @@ -195,7 +197,7 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 user, deletable_users + users_to_mark_inactive, test_run ): messages.warning(request, message) - for user in users_to_mark_inactive: + for user in users_to_mark_inactive + inactive_users: for message in remove_inactive_participations(user, test_run): messages.warning(request, message) if test_run: From 51b5f99dc782828256d6a77237e459be308f21d6 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 11 Nov 2024 20:46:05 +0100 Subject: [PATCH 24/49] Change django magic query to look and check latest vote_end_date --- evap/staff/tools.py | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 329e18930b..dd1656fb5e 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -2,13 +2,14 @@ from collections.abc import Iterable from datetime import date, datetime, timedelta from enum import Enum +from typing import Optional from django.conf import settings from django.contrib import messages from django.contrib.auth.models import Group from django.core.exceptions import SuspiciousOperation from django.db import transaction -from django.db.models import Count +from django.db.models import Count, Max from django.urls import reverse from django.utils.html import escape, format_html, format_html_join from django.utils.safestring import SafeString @@ -383,24 +384,20 @@ def remove_user_from_represented_and_ccing_users(user, ignored_users=None, test_ def remove_inactive_participations(user, test_run=False): remove_messages = [] - evaluations = [] + last_eval: Optional[date] = None if not user.is_active or user.can_be_marked_inactive_by_manager: - evaluations = user.evaluations_participating_in.filter( - state=Evaluation.State.PUBLISHED, - vote_end_date__lt=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS), - ) - if evaluations: + last_eval = user.evaluations_participating_in.aggregate(Max("vote_end_date"))["vote_end_date__max"] + + if last_eval and last_eval <= (datetime.today() - settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS).date(): + number_evals = len(user.evaluations_participating_in.all()) if test_run: remove_messages.append( - _("{} will be removed from {} participation(s) due to inactivity.").format( - user.full_name, len(evaluations) - ) + _("{} will be removed from {} participation(s) due to inactivity.").format(user.full_name, number_evals) ) else: - for evaluation in evaluations: - evaluation.participants.remove(user) + user.evaluations_participating_in.clear() remove_messages.append( - _("Removed {} from {} participation(s) due to inactivity.").format(user.full_name, len(evaluations)) + _("Removed {} from {} participation(s) due to inactivity.").format(user.full_name, number_evals) ) return remove_messages From 2f5711c2ce473ef0618a1a9f46ba83e57385a503 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 11 Nov 2024 20:48:36 +0100 Subject: [PATCH 25/49] Refactor test to use django's setUpTestData --- evap/staff/tests/test_tools.py | 56 +++++++++++++++------------------- 1 file changed, 25 insertions(+), 31 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 76e2feca0b..81788c339b 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -4,7 +4,7 @@ from unittest.mock import MagicMock, patch from django.contrib.auth.models import Group -from django.test import TestCase +from django.test import TestCase, override_settings from django.utils.html import escape from django_webtest import WebTest from model_bakery import baker @@ -223,48 +223,42 @@ def test_do_nothing_if_test_run(self): class RemoveUserDueToInactivity(TestCase): - def test_remove_user_due_to_inactivity(self): - inactive_user = baker.make(UserProfile, last_name="Inactive User") - inactive_evaluation = baker.make( + @classmethod + def setUpTestData(cls): + cls.user = baker.make(UserProfile) + six_months_ago = datetime.today() - timedelta(days=6 * 30) + cls.evaluation = baker.make( Evaluation, state=Evaluation.State.PUBLISHED, - vote_start_datetime=datetime.today() - - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS + 1), - vote_end_date=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS + 1), - participants=[inactive_user], + vote_start_datetime=six_months_ago - settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS, + vote_end_date=six_months_ago.date(), + participants=[cls.user], ) - semester = inactive_evaluation.course.semester - semester.archive() + @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(6 * 30 - 1)) + def test_remove_user_due_to_inactivity(self): + self.evaluation.course.semester.archive() - messages = remove_inactive_participations(inactive_user) - self.assertTrue(inactive_user.can_be_marked_inactive_by_manager) + messages = remove_inactive_participations(self.user) + self.assertTrue(self.user.can_be_marked_inactive_by_manager) + self.assertEqual(messages, [f"Removed {self.user.full_name} from {1} participation(s) due to inactivity."]) self.assertEqual(len(messages), 1) + @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(6 * 30 + 1)) def test_do_not_remove_user_due_to_inactivity(self): - active_user = baker.make(UserProfile, last_name="Active User") - - baker.make(Evaluation, state=Evaluation.State.PUBLISHED, participants=[active_user]) - - messages = remove_inactive_participations(active_user) - self.assertFalse(active_user.can_be_marked_inactive_by_manager) + messages = remove_inactive_participations(self.user) + self.assertFalse(self.user.can_be_marked_inactive_by_manager or not self.user.is_active) self.assertEqual(len(messages), 0) + @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(6 * 30 - 1)) def test_do_nothing_if_test_run(self): - inactive_user = baker.make(UserProfile, last_name="Test Run User") - inactive_evaluation = baker.make( - Evaluation, - state=Evaluation.State.PUBLISHED, - vote_start_datetime=datetime.today() - - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS + 1), - vote_end_date=datetime.today() - timedelta(days=settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS + 1), - participants=[inactive_user], - ) + self.evaluation.course.semester.archive() - inactive_evaluation.course.semester.archive() - - messages = remove_inactive_participations(inactive_user, test_run=True) - self.assertTrue(inactive_user.can_be_marked_inactive_by_manager) + messages = remove_inactive_participations(self.user, test_run=True) + self.assertTrue(self.user.can_be_marked_inactive_by_manager) + self.assertEqual( + messages, [f"{self.user.full_name} will be removed from {1} participation(s) due to inactivity."] + ) self.assertEqual(len(messages), 1) From 74e9076c9721b1a3ae08e1815a4f34acbbed4a3f Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 11 Nov 2024 20:56:13 +0100 Subject: [PATCH 26/49] "Fix" type hint to use | None instead of Optional --- evap/staff/tools.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index dd1656fb5e..63ffa9ccf0 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -2,7 +2,6 @@ from collections.abc import Iterable from datetime import date, datetime, timedelta from enum import Enum -from typing import Optional from django.conf import settings from django.contrib import messages @@ -384,7 +383,7 @@ def remove_user_from_represented_and_ccing_users(user, ignored_users=None, test_ def remove_inactive_participations(user, test_run=False): remove_messages = [] - last_eval: Optional[date] = None + last_eval: date | None = None if not user.is_active or user.can_be_marked_inactive_by_manager: last_eval = user.evaluations_participating_in.aggregate(Max("vote_end_date"))["vote_end_date__max"] From 1dc1fb7be065791f659eb899b41c0303033bb6cc Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 11 Nov 2024 20:56:35 +0100 Subject: [PATCH 27/49] reorder imports --- evap/staff/tests/test_tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 81788c339b..4104aeaf1e 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -3,6 +3,7 @@ from itertools import cycle, repeat from unittest.mock import MagicMock, patch +from django.conf import settings from django.contrib.auth.models import Group from django.test import TestCase, override_settings from django.utils.html import escape @@ -10,7 +11,6 @@ from model_bakery import baker from openpyxl import load_workbook -from django.conf import settings from evap.evaluation.models import Contribution, Course, Evaluation, UserProfile from evap.evaluation.tests.tools import assert_no_database_modifications from evap.rewards.models import RewardPointGranting, RewardPointRedemption From 36a5a004676a6d98bfc0824f71fdcd25a9753c1a Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 11 Nov 2024 20:56:46 +0100 Subject: [PATCH 28/49] linting --- evap/settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evap/settings.py b/evap/settings.py index 272cd8b110..22304ba49e 100644 --- a/evap/settings.py +++ b/evap/settings.py @@ -37,7 +37,7 @@ VOTER_COUNT_NEEDED_FOR_PUBLISHING_RATING_RESULTS = 2 VOTER_PERCENTAGE_NEEDED_FOR_PUBLISHING_AVERAGE_GRADE = 0.2 SMALL_COURSE_SIZE = 5 # up to which number of participants the evaluation gets additional warnings about anonymity -PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS = timedelta(days=18*30) +PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS = timedelta(days=18 * 30) # a warning is shown next to results where less than RESULTS_WARNING_COUNT answers were given # or the number of answers is less than RESULTS_WARNING_PERCENTAGE times the median number of answers (for this question in this evaluation) From 01d60a2f2265d75703c95e7e1f10cca54b53312f Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 11 Nov 2024 22:16:37 +0100 Subject: [PATCH 29/49] Add more edge cases to tests --- evap/staff/tests/test_tools.py | 36 ++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 4104aeaf1e..ba48fa6665 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -234,26 +234,46 @@ def setUpTestData(cls): vote_end_date=six_months_ago.date(), participants=[cls.user], ) + cls.evaluation.course.semester.archive() - @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(6 * 30 - 1)) + @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(6 * 30)) def test_remove_user_due_to_inactivity(self): - self.evaluation.course.semester.archive() - messages = remove_inactive_participations(self.user) self.assertTrue(self.user.can_be_marked_inactive_by_manager) self.assertEqual(messages, [f"Removed {self.user.full_name} from {1} participation(s) due to inactivity."]) self.assertEqual(len(messages), 1) - @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(6 * 30 + 1)) - def test_do_not_remove_user_due_to_inactivity(self): + @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(30)) + def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation(self): + recently_archived_evaluation = baker.make( + Evaluation, + state=Evaluation.State.PUBLISHED, + vote_start_datetime=datetime.today() - timedelta(days=3), + vote_end_date=datetime.today() - timedelta(days=1), + participants=[self.user], + ) + + recently_archived_evaluation.course.semester.archive() + + messages = remove_inactive_participations(self.user) + self.assertTrue( + self.user.can_be_marked_inactive_by_manager + ) # user can be marked inactive since all evaluations are archived but is not inactive for too long + self.assertTrue(self.user.is_active) + self.assertEqual(len(messages), 0) + + def test_do_not_remove_user_due_to_inactivity_with_active_evaluation(self): + not_archived_evaluation = baker.make( + Evaluation, + participants=[self.user], + ) + messages = remove_inactive_participations(self.user) self.assertFalse(self.user.can_be_marked_inactive_by_manager or not self.user.is_active) self.assertEqual(len(messages), 0) - @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(6 * 30 - 1)) + @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(6 * 30)) def test_do_nothing_if_test_run(self): - self.evaluation.course.semester.archive() - messages = remove_inactive_participations(self.user, test_run=True) self.assertTrue(self.user.can_be_marked_inactive_by_manager) self.assertEqual( From 03622d6ba16138ce821a4edba9f485d26f9ef8b1 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 11 Nov 2024 22:18:51 +0100 Subject: [PATCH 30/49] Remove assignment of never used variable --- evap/staff/tests/test_tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index ba48fa6665..0cddda5844 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -263,7 +263,7 @@ def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation( self.assertEqual(len(messages), 0) def test_do_not_remove_user_due_to_inactivity_with_active_evaluation(self): - not_archived_evaluation = baker.make( + baker.make( Evaluation, participants=[self.user], ) From 810a957f78831e5329ebe2e0f038f9d8fa2b79b1 Mon Sep 17 00:00:00 2001 From: Niklas Mohrin Date: Mon, 18 Nov 2024 18:03:43 +0100 Subject: [PATCH 31/49] refactor `remove_inactive_participations` --- evap/staff/tools.py | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 63ffa9ccf0..12a3dbbabf 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -16,7 +16,7 @@ from evap.evaluation.models import Contribution, Course, Evaluation, TextAnswer, UserProfile from evap.evaluation.models_logging import LogEntry -from evap.evaluation.tools import clean_email, is_external_email +from evap.evaluation.tools import StrOrPromise, clean_email, is_external_email from evap.grades.models import GradeDocument from evap.results.tools import STATES_WITH_RESULTS_CACHING, cache_results @@ -381,24 +381,24 @@ def remove_user_from_represented_and_ccing_users(user, ignored_users=None, test_ return remove_messages -def remove_inactive_participations(user, test_run=False): - remove_messages = [] - last_eval: date | None = None - if not user.is_active or user.can_be_marked_inactive_by_manager: - last_eval = user.evaluations_participating_in.aggregate(Max("vote_end_date"))["vote_end_date__max"] - - if last_eval and last_eval <= (datetime.today() - settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS).date(): - number_evals = len(user.evaluations_participating_in.all()) - if test_run: - remove_messages.append( - _("{} will be removed from {} participation(s) due to inactivity.").format(user.full_name, number_evals) - ) - else: - user.evaluations_participating_in.clear() - remove_messages.append( - _("Removed {} from {} participation(s) due to inactivity.").format(user.full_name, number_evals) - ) - return remove_messages +def remove_inactive_participations(user: UserProfile, test_run=False) -> list[StrOrPromise]: + if user.is_active or not user.can_be_marked_inactive_by_manager: + return [] + last_participation = user.evaluations_participating_in.aggregate(Max("vote_end_date"))["vote_end_date__max"] + if ( + last_participation is None + or (datetime.today() - last_participation) < settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS + ): + return [] + + evaluation_count = user.evaluations_participating_in.count() + if test_run: + return [ + _("{} will be removed from {} participation(s) due to inactivity.").format(user.full_name, evaluation_count) + ] + else: + user.evaluations_participating_in.clear() + return [_("Removed {} from {} participation(s) due to inactivity.").format(user.full_name, evaluation_count)] def user_edit_link(user_id): From 2ab9cffcd28310a1a46492788a47db4e24476111 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 18 Nov 2024 17:24:42 +0100 Subject: [PATCH 32/49] Rename variable --- evap/settings.py | 2 +- evap/staff/tests/test_tools.py | 2 +- evap/staff/tools.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/evap/settings.py b/evap/settings.py index aeafaa850c..06722d5388 100644 --- a/evap/settings.py +++ b/evap/settings.py @@ -37,7 +37,7 @@ VOTER_COUNT_NEEDED_FOR_PUBLISHING_RATING_RESULTS = 2 VOTER_PERCENTAGE_NEEDED_FOR_PUBLISHING_AVERAGE_GRADE = 0.2 SMALL_COURSE_SIZE = 5 # up to which number of participants the evaluation gets additional warnings about anonymity -PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS = timedelta(days=18 * 30) +PARTICIPATION_DELETION_AFTER_INACTIVE_TIME = timedelta(days=18 * 30) # a warning is shown next to results where less than RESULTS_WARNING_COUNT answers were given # or the number of answers is less than RESULTS_WARNING_PERCENTAGE times the median number of answers (for this question in this evaluation) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index df4c872acc..152c4b6799 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -229,7 +229,7 @@ def setUpTestData(cls): cls.evaluation = baker.make( Evaluation, state=Evaluation.State.PUBLISHED, - vote_start_datetime=six_months_ago - settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS, + vote_start_datetime=six_months_ago - settings.PARTICIPATION_DELETION_AFTER_INACTIVE_TIME, vote_end_date=six_months_ago.date(), participants=[cls.user], ) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 12a3dbbabf..2ea71bb6cc 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -387,7 +387,7 @@ def remove_inactive_participations(user: UserProfile, test_run=False) -> list[St last_participation = user.evaluations_participating_in.aggregate(Max("vote_end_date"))["vote_end_date__max"] if ( last_participation is None - or (datetime.today() - last_participation) < settings.PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS + or (datetime.today() - last_participation) < settings.PARTICIPATION_DELETION_AFTER_INACTIVE_TIME ): return [] From 6715f7778af1e66d4c3a323d005e493277e8058a Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 18 Nov 2024 18:11:38 +0100 Subject: [PATCH 33/49] Implement feedback --- evap/staff/tests/test_tools.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 152c4b6799..463d359777 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -221,7 +221,7 @@ def test_do_nothing_if_test_run(self): self.assertEqual(len(messages), 4) -class RemoveUserDueToInactivity(TestCase): +class RemoveParticipationDueToInactivityTest(TestCase): @classmethod def setUpTestData(cls): cls.user = baker.make(UserProfile) @@ -239,7 +239,7 @@ def setUpTestData(cls): def test_remove_user_due_to_inactivity(self): messages = remove_inactive_participations(self.user) self.assertTrue(self.user.can_be_marked_inactive_by_manager) - self.assertEqual(messages, [f"Removed {self.user.full_name} from {1} participation(s) due to inactivity."]) + self.assertEqual(messages, [f"Removed {self.user.full_name} from 1 participation(s) due to inactivity."]) self.assertEqual(len(messages), 1) @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(30)) @@ -259,7 +259,7 @@ def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation( self.user.can_be_marked_inactive_by_manager ) # user can be marked inactive since all evaluations are archived but is not inactive for too long self.assertTrue(self.user.is_active) - self.assertEqual(len(messages), 0) + self.assertEqual(messages, []) def test_do_not_remove_user_due_to_inactivity_with_active_evaluation(self): baker.make( @@ -268,17 +268,16 @@ def test_do_not_remove_user_due_to_inactivity_with_active_evaluation(self): ) messages = remove_inactive_participations(self.user) - self.assertFalse(self.user.can_be_marked_inactive_by_manager or not self.user.is_active) - self.assertEqual(len(messages), 0) + self.assertFalse(self.user.can_be_marked_inactive_by_manager) + self.assertEqual(messages, []) @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(6 * 30)) def test_do_nothing_if_test_run(self): messages = remove_inactive_participations(self.user, test_run=True) self.assertTrue(self.user.can_be_marked_inactive_by_manager) self.assertEqual( - messages, [f"{self.user.full_name} will be removed from {1} participation(s) due to inactivity."] + messages, [f"{self.user.full_name} will be removed from 1 participation(s) due to inactivity."] ) - self.assertEqual(len(messages), 1) class UserEditLinkTest(TestCase): From 351fbaa7bb78d33f2d8f8e8663fdb36e89639255 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 18 Nov 2024 18:58:21 +0100 Subject: [PATCH 34/49] Fix logic error and wrong type --- evap/staff/tools.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 2ea71bb6cc..273cd263a5 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -382,12 +382,12 @@ def remove_user_from_represented_and_ccing_users(user, ignored_users=None, test_ def remove_inactive_participations(user: UserProfile, test_run=False) -> list[StrOrPromise]: - if user.is_active or not user.can_be_marked_inactive_by_manager: + if user.is_active and not user.can_be_marked_inactive_by_manager: return [] last_participation = user.evaluations_participating_in.aggregate(Max("vote_end_date"))["vote_end_date__max"] if ( last_participation is None - or (datetime.today() - last_participation) < settings.PARTICIPATION_DELETION_AFTER_INACTIVE_TIME + or (date.today() - last_participation) < settings.PARTICIPATION_DELETION_AFTER_INACTIVE_TIME ): return [] From 08983c32aa2b01fecc356afd0e37eda692a73fd3 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 18 Nov 2024 19:00:38 +0100 Subject: [PATCH 35/49] Fix missing rename of variable in @override_settings --- evap/staff/tests/test_tools.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 463d359777..646e6aff2e 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -235,7 +235,7 @@ def setUpTestData(cls): ) cls.evaluation.course.semester.archive() - @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(6 * 30)) + @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(6 * 30)) def test_remove_user_due_to_inactivity(self): messages = remove_inactive_participations(self.user) self.assertTrue(self.user.can_be_marked_inactive_by_manager) @@ -271,7 +271,7 @@ def test_do_not_remove_user_due_to_inactivity_with_active_evaluation(self): self.assertFalse(self.user.can_be_marked_inactive_by_manager) self.assertEqual(messages, []) - @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(6 * 30)) + @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(6 * 30)) def test_do_nothing_if_test_run(self): messages = remove_inactive_participations(self.user, test_run=True) self.assertTrue(self.user.can_be_marked_inactive_by_manager) From eebef2baf9e5747555ac24158d5edae80e191a98 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 18 Nov 2024 19:04:15 +0100 Subject: [PATCH 36/49] Extend to test for user.evaluations_participating_in.exists() and repeat behaviour --- evap/staff/tests/test_tools.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 646e6aff2e..5282bef11b 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -238,11 +238,14 @@ def setUpTestData(cls): @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(6 * 30)) def test_remove_user_due_to_inactivity(self): messages = remove_inactive_participations(self.user) + self.assertFalse(self.user.evaluations_participating_in.exists()) self.assertTrue(self.user.can_be_marked_inactive_by_manager) self.assertEqual(messages, [f"Removed {self.user.full_name} from 1 participation(s) due to inactivity."]) self.assertEqual(len(messages), 1) - @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS=timedelta(30)) + messages = remove_inactive_participations(self.user) + self.assertEqual(messages, []) + def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation(self): recently_archived_evaluation = baker.make( Evaluation, @@ -258,6 +261,7 @@ def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation( self.assertTrue( self.user.can_be_marked_inactive_by_manager ) # user can be marked inactive since all evaluations are archived but is not inactive for too long + self.assertTrue(self.user.evaluations_participating_in.exists()) self.assertTrue(self.user.is_active) self.assertEqual(messages, []) @@ -268,17 +272,26 @@ def test_do_not_remove_user_due_to_inactivity_with_active_evaluation(self): ) messages = remove_inactive_participations(self.user) + self.assertTrue(self.user.evaluations_participating_in.exists()) self.assertFalse(self.user.can_be_marked_inactive_by_manager) self.assertEqual(messages, []) @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(6 * 30)) def test_do_nothing_if_test_run(self): + self.assertTrue(self.user.evaluations_participating_in.exists()) + messages = remove_inactive_participations(self.user, test_run=True) + self.assertTrue(self.user.evaluations_participating_in.exists()) self.assertTrue(self.user.can_be_marked_inactive_by_manager) self.assertEqual( messages, [f"{self.user.full_name} will be removed from 1 participation(s) due to inactivity."] ) + messages = remove_inactive_participations(self.user, test_run=True) + self.assertEqual( + messages, [f"{self.user.full_name} will be removed from 1 participation(s) due to inactivity."] + ) + class UserEditLinkTest(TestCase): def test_user_edit_link(self): From cd63d80dbe4bc8bb7d36a770464c27b6e14182d2 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 18 Nov 2024 20:34:37 +0100 Subject: [PATCH 37/49] Use mock.patch decorator instead of creating new Evaluations with user as participant and add beginning check for user.evaluations_participating_in.exists() --- evap/staff/tests/test_tools.py | 35 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 5282bef11b..566dc80f3e 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -1,7 +1,7 @@ from datetime import datetime, timedelta from io import BytesIO from itertools import cycle, repeat -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, patch, Mock from django.conf import settings from django.contrib.auth.models import Group @@ -237,43 +237,36 @@ def setUpTestData(cls): @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(6 * 30)) def test_remove_user_due_to_inactivity(self): + self.assertTrue(self.user.evaluations_participating_in.exists()) + messages = remove_inactive_participations(self.user) + self.assertFalse(self.user.evaluations_participating_in.exists()) self.assertTrue(self.user.can_be_marked_inactive_by_manager) self.assertEqual(messages, [f"Removed {self.user.full_name} from 1 participation(s) due to inactivity."]) - self.assertEqual(len(messages), 1) messages = remove_inactive_participations(self.user) + self.assertEqual(messages, []) + @patch("evap.evaluation.models.UserProfile.is_active", True) + @patch("evap.evaluation.models.UserProfile.can_be_marked_inactive_by_manager", True) def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation(self): - recently_archived_evaluation = baker.make( - Evaluation, - state=Evaluation.State.PUBLISHED, - vote_start_datetime=datetime.today() - timedelta(days=3), - vote_end_date=datetime.today() - timedelta(days=1), - participants=[self.user], - ) - - recently_archived_evaluation.course.semester.archive() + self.assertTrue(self.user.evaluations_participating_in.exists()) messages = remove_inactive_participations(self.user) - self.assertTrue( - self.user.can_be_marked_inactive_by_manager - ) # user can be marked inactive since all evaluations are archived but is not inactive for too long + self.assertTrue(self.user.evaluations_participating_in.exists()) - self.assertTrue(self.user.is_active) self.assertEqual(messages, []) + @patch("evap.evaluation.models.UserProfile.is_active", True) + @patch("evap.evaluation.models.UserProfile.can_be_marked_inactive_by_manager", False) def test_do_not_remove_user_due_to_inactivity_with_active_evaluation(self): - baker.make( - Evaluation, - participants=[self.user], - ) + self.assertTrue(self.user.evaluations_participating_in.exists()) messages = remove_inactive_participations(self.user) + self.assertTrue(self.user.evaluations_participating_in.exists()) - self.assertFalse(self.user.can_be_marked_inactive_by_manager) self.assertEqual(messages, []) @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(6 * 30)) @@ -281,6 +274,7 @@ def test_do_nothing_if_test_run(self): self.assertTrue(self.user.evaluations_participating_in.exists()) messages = remove_inactive_participations(self.user, test_run=True) + self.assertTrue(self.user.evaluations_participating_in.exists()) self.assertTrue(self.user.can_be_marked_inactive_by_manager) self.assertEqual( @@ -288,6 +282,7 @@ def test_do_nothing_if_test_run(self): ) messages = remove_inactive_participations(self.user, test_run=True) + self.assertEqual( messages, [f"{self.user.full_name} will be removed from 1 participation(s) due to inactivity."] ) From 3db7709d85e679c85c034a28bcef9e5c9084f453 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 18 Nov 2024 20:35:31 +0100 Subject: [PATCH 38/49] Remove unused import --- evap/staff/tests/test_tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 566dc80f3e..759123a90f 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -1,7 +1,7 @@ from datetime import datetime, timedelta from io import BytesIO from itertools import cycle, repeat -from unittest.mock import MagicMock, patch, Mock +from unittest.mock import MagicMock, patch from django.conf import settings from django.contrib.auth.models import Group From 32a4c79e22a922a20525b0a6b4adc2e9b61c0894 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 18 Nov 2024 20:40:36 +0100 Subject: [PATCH 39/49] ruff fix --- evap/staff/tools.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 273cd263a5..326c74bfa7 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -396,9 +396,8 @@ def remove_inactive_participations(user: UserProfile, test_run=False) -> list[St return [ _("{} will be removed from {} participation(s) due to inactivity.").format(user.full_name, evaluation_count) ] - 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)] def user_edit_link(user_id): From 3364e5c61ec64a0f31bd20f7db47230072ac3497 Mon Sep 17 00:00:00 2001 From: Redflashx12 <43482469+Redflashx12@users.noreply.github.com> Date: Mon, 6 Jan 2025 18:24:38 +0100 Subject: [PATCH 40/49] Use proper plural formatting Co-authored-by: Johannes Wolf --- evap/staff/tools.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 326c74bfa7..adf98f0d56 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -394,7 +394,11 @@ def remove_inactive_participations(user: UserProfile, test_run=False) -> list[St evaluation_count = user.evaluations_participating_in.count() if test_run: return [ - _("{} will be removed from {} participation(s) due to inactivity.").format(user.full_name, evaluation_count) + ngettext( + "{} participation of {} would be removed due to inactivity.", + "{} participations of {} would be removed due to inactivity.", + evaluation_count, + ).format(evaluation_count, user.full_name) ] user.evaluations_participating_in.clear() return [_("Removed {} from {} participation(s) due to inactivity.").format(user.full_name, evaluation_count)] From 0b863c68fdd8607efd6874044a681d17919dc922 Mon Sep 17 00:00:00 2001 From: Redflashx12 <43482469+Redflashx12@users.noreply.github.com> Date: Mon, 6 Jan 2025 18:25:24 +0100 Subject: [PATCH 41/49] Use proper plural formatting Co-authored-by: Johannes Wolf --- evap/staff/tools.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index adf98f0d56..35e652ee1b 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -401,7 +401,13 @@ def remove_inactive_participations(user: UserProfile, test_run=False) -> list[St ).format(evaluation_count, user.full_name) ] user.evaluations_participating_in.clear() - return [_("Removed {} from {} participation(s) due to inactivity.").format(user.full_name, evaluation_count)] + return [ + ngettext( + "{} participation of {} was removed due to inactivity.", + "{} participations of {} were removed due to inactivity.", + evaluation_count, + ).format(evaluation_count, user.full_name) + ] def user_edit_link(user_id): From 58d239d09c22a8c77393a0444155b4c5abf150a2 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 6 Jan 2025 18:41:44 +0100 Subject: [PATCH 42/49] Fix missing import of ngettext --- evap/staff/tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 35e652ee1b..13801bf053 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -12,7 +12,7 @@ from django.urls import reverse from django.utils.html import escape, format_html, format_html_join from django.utils.safestring import SafeString -from django.utils.translation import gettext_lazy as _ +from django.utils.translation import gettext_lazy as _, ngettext from evap.evaluation.models import Contribution, Course, Evaluation, TextAnswer, UserProfile from evap.evaluation.models_logging import LogEntry From fc9a393769a2c0ba37610de70a9961335a985d24 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 20 Jan 2025 17:41:41 +0100 Subject: [PATCH 43/49] Update assert in tests to use the new phrasing --- evap/staff/tests/test_tools.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 759123a90f..6d1413eede 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -243,7 +243,7 @@ def test_remove_user_due_to_inactivity(self): self.assertFalse(self.user.evaluations_participating_in.exists()) self.assertTrue(self.user.can_be_marked_inactive_by_manager) - self.assertEqual(messages, [f"Removed {self.user.full_name} from 1 participation(s) due to inactivity."]) + self.assertEqual(messages, [f"1 participation of {self.user.full_name} was removed due to inactivity."]) messages = remove_inactive_participations(self.user) @@ -278,13 +278,13 @@ def test_do_nothing_if_test_run(self): self.assertTrue(self.user.evaluations_participating_in.exists()) self.assertTrue(self.user.can_be_marked_inactive_by_manager) self.assertEqual( - messages, [f"{self.user.full_name} will be removed from 1 participation(s) due to inactivity."] + messages, [f"1 participation of {self.user.full_name} would be removed due to inactivity."] ) messages = remove_inactive_participations(self.user, test_run=True) self.assertEqual( - messages, [f"{self.user.full_name} will be removed from 1 participation(s) due to inactivity."] + messages, [f"1 participation of {self.user.full_name} would be removed due to inactivity."] ) From 83754ea2426c16b798e983b170c353d35726aa42 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 20 Jan 2025 18:00:51 +0100 Subject: [PATCH 44/49] Implement feedback from Richard --- evap/staff/tests/test_tools.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 6d1413eede..334928e609 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -225,12 +225,11 @@ class RemoveParticipationDueToInactivityTest(TestCase): @classmethod def setUpTestData(cls): cls.user = baker.make(UserProfile) - six_months_ago = datetime.today() - timedelta(days=6 * 30) cls.evaluation = baker.make( Evaluation, state=Evaluation.State.PUBLISHED, - vote_start_datetime=six_months_ago - settings.PARTICIPATION_DELETION_AFTER_INACTIVE_TIME, - vote_end_date=six_months_ago.date(), + vote_start_datetime=datetime.today() - timedelta(days=200), + vote_end_date=(datetime.today() - timedelta(days=180)).date(), participants=[cls.user], ) cls.evaluation.course.semester.archive() @@ -251,6 +250,7 @@ def test_remove_user_due_to_inactivity(self): @patch("evap.evaluation.models.UserProfile.is_active", True) @patch("evap.evaluation.models.UserProfile.can_be_marked_inactive_by_manager", True) + @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(360)) def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation(self): self.assertTrue(self.user.evaluations_participating_in.exists()) @@ -261,6 +261,7 @@ def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation( @patch("evap.evaluation.models.UserProfile.is_active", True) @patch("evap.evaluation.models.UserProfile.can_be_marked_inactive_by_manager", False) + @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(360)) def test_do_not_remove_user_due_to_inactivity_with_active_evaluation(self): self.assertTrue(self.user.evaluations_participating_in.exists()) @@ -274,6 +275,7 @@ def test_do_nothing_if_test_run(self): self.assertTrue(self.user.evaluations_participating_in.exists()) messages = remove_inactive_participations(self.user, test_run=True) + pre_count = self.user.evaluations_participating_in.count(); self.assertTrue(self.user.evaluations_participating_in.exists()) self.assertTrue(self.user.can_be_marked_inactive_by_manager) @@ -281,11 +283,8 @@ def test_do_nothing_if_test_run(self): messages, [f"1 participation of {self.user.full_name} would be removed due to inactivity."] ) - messages = remove_inactive_participations(self.user, test_run=True) - - self.assertEqual( - messages, [f"1 participation of {self.user.full_name} would be removed due to inactivity."] - ) + post_count = self.user.evaluations_participating_in.count(); + self.assertEqual(pre_count, post_count) class UserEditLinkTest(TestCase): From 6a7f0de4587edee4a862744e86944549098e19e1 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 20 Jan 2025 18:49:08 +0100 Subject: [PATCH 45/49] Remove last elif check which is unnecessary --- evap/staff/tools.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 13801bf053..7b7ffa228a 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -132,14 +132,13 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 users_to_be_updated.append((matching_user, imported_email)) emails_of_non_obsolete_users = set(imported_emails) | {user.email for user, _ in users_to_be_updated} - deletable_users, users_to_mark_inactive, inactive_users = [], [], [] + deletable_users, users_to_mark_inactive = [], [] for user in UserProfile.objects.exclude(email__in=emails_of_non_obsolete_users): if user.can_be_deleted_by_manager: deletable_users.append(user) elif user.is_active and user.can_be_marked_inactive_by_manager: users_to_mark_inactive.append(user) - elif not user.is_active: - inactive_users.append(user) + messages.info( request, @@ -197,7 +196,7 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 user, deletable_users + users_to_mark_inactive, test_run ): messages.warning(request, message) - for user in users_to_mark_inactive + inactive_users: + for user in users_to_mark_inactive: for message in remove_inactive_participations(user, test_run): messages.warning(request, message) if test_run: From 194e3cdb93f4715203b09893b475f7d646db4fab Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 20 Jan 2025 19:01:13 +0100 Subject: [PATCH 46/49] lint --- evap/staff/tests/test_tools.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 334928e609..6d98c25f6b 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -3,7 +3,6 @@ from itertools import cycle, repeat from unittest.mock import MagicMock, patch -from django.conf import settings from django.contrib.auth.models import Group from django.test import override_settings from django.utils.html import escape @@ -275,7 +274,7 @@ def test_do_nothing_if_test_run(self): self.assertTrue(self.user.evaluations_participating_in.exists()) messages = remove_inactive_participations(self.user, test_run=True) - pre_count = self.user.evaluations_participating_in.count(); + pre_count = self.user.evaluations_participating_in.count() self.assertTrue(self.user.evaluations_participating_in.exists()) self.assertTrue(self.user.can_be_marked_inactive_by_manager) @@ -283,7 +282,7 @@ def test_do_nothing_if_test_run(self): messages, [f"1 participation of {self.user.full_name} would be removed due to inactivity."] ) - post_count = self.user.evaluations_participating_in.count(); + post_count = self.user.evaluations_participating_in.count() self.assertEqual(pre_count, post_count) From 2a4553ea2233ba9dfde0a1f4c6c2e1b74c04f59c Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 20 Jan 2025 19:07:04 +0100 Subject: [PATCH 47/49] format --- evap/staff/tests/test_tools.py | 4 +--- evap/staff/tools.py | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 6d98c25f6b..3fff754cf2 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -278,9 +278,7 @@ def test_do_nothing_if_test_run(self): self.assertTrue(self.user.evaluations_participating_in.exists()) self.assertTrue(self.user.can_be_marked_inactive_by_manager) - self.assertEqual( - messages, [f"1 participation of {self.user.full_name} would be removed due to inactivity."] - ) + self.assertEqual(messages, [f"1 participation of {self.user.full_name} would be removed due to inactivity."]) post_count = self.user.evaluations_participating_in.count() self.assertEqual(pre_count, post_count) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 7b7ffa228a..a5a270732e 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -12,7 +12,8 @@ from django.urls import reverse from django.utils.html import escape, format_html, format_html_join from django.utils.safestring import SafeString -from django.utils.translation import gettext_lazy as _, ngettext +from django.utils.translation import gettext_lazy as _ +from django.utils.translation import ngettext from evap.evaluation.models import Contribution, Course, Evaluation, TextAnswer, UserProfile from evap.evaluation.models_logging import LogEntry @@ -139,7 +140,6 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 elif user.is_active and user.can_be_marked_inactive_by_manager: users_to_mark_inactive.append(user) - messages.info( request, _( From 0efe3407bcc27259ab29bdbe06c3b8418fee19c7 Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 27 Jan 2025 18:37:59 +0100 Subject: [PATCH 48/49] Implement feedback: more explicit test and renamed function --- evap/staff/tests/test_tools.py | 12 ++++++------ evap/staff/tools.py | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 3fff754cf2..263a98ff7a 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -235,7 +235,7 @@ def setUpTestData(cls): @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(6 * 30)) def test_remove_user_due_to_inactivity(self): - self.assertTrue(self.user.evaluations_participating_in.exists()) + self.assertEqual(list(self.user.evaluations_participating_in.all()), [self.evaluation]) messages = remove_inactive_participations(self.user) @@ -251,22 +251,22 @@ def test_remove_user_due_to_inactivity(self): @patch("evap.evaluation.models.UserProfile.can_be_marked_inactive_by_manager", True) @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(360)) def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation(self): - self.assertTrue(self.user.evaluations_participating_in.exists()) + self.assertEqual(list(self.user.evaluations_participating_in.all()), [self.evaluation]) messages = remove_inactive_participations(self.user) - self.assertTrue(self.user.evaluations_participating_in.exists()) + self.assertEqual(list(self.user.evaluations_participating_in.all()), [self.evaluation]) self.assertEqual(messages, []) @patch("evap.evaluation.models.UserProfile.is_active", True) @patch("evap.evaluation.models.UserProfile.can_be_marked_inactive_by_manager", False) @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(360)) def test_do_not_remove_user_due_to_inactivity_with_active_evaluation(self): - self.assertTrue(self.user.evaluations_participating_in.exists()) + self.assertEqual(list(self.user.evaluations_participating_in.all()), [self.evaluation]) messages = remove_inactive_participations(self.user) - self.assertTrue(self.user.evaluations_participating_in.exists()) + self.assertEqual(list(self.user.evaluations_participating_in.all()), [self.evaluation]) self.assertEqual(messages, []) @override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(6 * 30)) @@ -276,7 +276,7 @@ def test_do_nothing_if_test_run(self): messages = remove_inactive_participations(self.user, test_run=True) pre_count = self.user.evaluations_participating_in.count() - self.assertTrue(self.user.evaluations_participating_in.exists()) + self.assertEqual(list(self.user.evaluations_participating_in.all()), [self.evaluation]) self.assertTrue(self.user.can_be_marked_inactive_by_manager) self.assertEqual(messages, [f"1 participation of {self.user.full_name} would be removed due to inactivity."]) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index a5a270732e..05ec0478c8 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -380,7 +380,7 @@ def remove_user_from_represented_and_ccing_users(user, ignored_users=None, test_ return remove_messages -def remove_inactive_participations(user: UserProfile, test_run=False) -> list[StrOrPromise]: +def remove_participations_if_inactive(user: UserProfile, test_run=False) -> list[StrOrPromise]: if user.is_active and not user.can_be_marked_inactive_by_manager: return [] last_participation = user.evaluations_participating_in.aggregate(Max("vote_end_date"))["vote_end_date__max"] From 876f6193a9d050ffd98981740399725f238cc5be Mon Sep 17 00:00:00 2001 From: Anton Kaiser Date: Mon, 27 Jan 2025 18:43:02 +0100 Subject: [PATCH 49/49] Fix wrong rename --- evap/staff/tests/test_tools.py | 12 ++++++------ evap/staff/tools.py | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py index 263a98ff7a..ba33e4d97a 100644 --- a/evap/staff/tests/test_tools.py +++ b/evap/staff/tests/test_tools.py @@ -21,7 +21,7 @@ from evap.staff.tools import ( conditional_escape, merge_users, - remove_inactive_participations, + remove_participations_if_inactive, remove_user_from_represented_and_ccing_users, user_edit_link, ) @@ -237,13 +237,13 @@ def setUpTestData(cls): def test_remove_user_due_to_inactivity(self): self.assertEqual(list(self.user.evaluations_participating_in.all()), [self.evaluation]) - messages = remove_inactive_participations(self.user) + messages = remove_participations_if_inactive(self.user) self.assertFalse(self.user.evaluations_participating_in.exists()) self.assertTrue(self.user.can_be_marked_inactive_by_manager) self.assertEqual(messages, [f"1 participation of {self.user.full_name} was removed due to inactivity."]) - messages = remove_inactive_participations(self.user) + messages = remove_participations_if_inactive(self.user) self.assertEqual(messages, []) @@ -253,7 +253,7 @@ def test_remove_user_due_to_inactivity(self): def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation(self): self.assertEqual(list(self.user.evaluations_participating_in.all()), [self.evaluation]) - messages = remove_inactive_participations(self.user) + messages = remove_participations_if_inactive(self.user) self.assertEqual(list(self.user.evaluations_participating_in.all()), [self.evaluation]) self.assertEqual(messages, []) @@ -264,7 +264,7 @@ def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation( def test_do_not_remove_user_due_to_inactivity_with_active_evaluation(self): self.assertEqual(list(self.user.evaluations_participating_in.all()), [self.evaluation]) - messages = remove_inactive_participations(self.user) + messages = remove_participations_if_inactive(self.user) self.assertEqual(list(self.user.evaluations_participating_in.all()), [self.evaluation]) self.assertEqual(messages, []) @@ -273,7 +273,7 @@ def test_do_not_remove_user_due_to_inactivity_with_active_evaluation(self): def test_do_nothing_if_test_run(self): self.assertTrue(self.user.evaluations_participating_in.exists()) - messages = remove_inactive_participations(self.user, test_run=True) + messages = remove_participations_if_inactive(self.user, test_run=True) pre_count = self.user.evaluations_participating_in.count() self.assertEqual(list(self.user.evaluations_participating_in.all()), [self.evaluation]) diff --git a/evap/staff/tools.py b/evap/staff/tools.py index 05ec0478c8..c5eafdfb3d 100644 --- a/evap/staff/tools.py +++ b/evap/staff/tools.py @@ -197,7 +197,7 @@ def bulk_update_users(request, user_file_content, test_run): # noqa: PLR0912 ): messages.warning(request, message) for user in users_to_mark_inactive: - for message in remove_inactive_participations(user, test_run): + for message in remove_participations_if_inactive(user, test_run): messages.warning(request, message) if test_run: messages.info(request, _("No data was changed in this test run."))