Delete participations of long inactive users #2329

Redflashx12 · 2024-11-04T19:24:22Z

Additionally added two test in class RemoveUserDueToInactivity. During test run of "Bulk update users" in the web with evap/staff/fixtures/test_user_bulk_update_file.txt, Lyndsey Lattimore should get removed from 1 participation(s) due to inactivity..

…default of 18 months

…ing months to seconds

…tive

…or vote start/end date instead of flat value

…ed as "can_be_marked_inactive_by_manager"

…ed to add +1 to get above threshold

richardebeling

Good work so far! Sorry, quite a few comments added up, this could be a bit overwhelming. We want to address everything so we don't get stuck in endless review loops. We're always happy to help out and answer any questions, so make sure to ask if anything is unclear.

evap/settings.py

evap/staff/tests/test_tools.py

evap/staff/tools.py

Co-authored-by: Richard Ebeling <[email protected]>

Redflashx12 · 2024-11-11T20:03:50Z

Good work so far! Sorry, quite a few comments added up, this could be a bit overwhelming. We want to address everything so we don't get stuck in endless review loops. We're always happy to help out and answer any questions, so make sure to ask if anything is unclear.

Thanks for your time in reviewing this PR, especially for all the fixes and explanations. Much appreciated 👍.

…eat behaviour

…r as participant and add beginning check for user.evaluations_participating_in.exists()

janno42 · 2024-12-02T16:54:35Z

evap/staff/tools.py

+    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)


Suggested change

_("{} 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)

janno42 · 2024-12-02T17:32:27Z

evap/staff/tools.py

+            _("{} will be 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)]


Suggested change

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)

]

niklasmohrin · 2024-12-16T16:33:03Z

@Redflashx12 Do you want to keep working on this PR (this or next year), or should we take over?

Redflashx12 · 2024-12-16T22:22:37Z

I'll be active in EvaP in January again, still busy with moving in rn. Sorry 😐

…

On Mon, 16 Dec 2024, 17:33 Niklas Mohrin, ***@***.***> wrote: @Redflashx12 <https://github.com/Redflashx12> Do you want to keep working on this PR (this or next year), or should we take over? — Reply to this email directly, view it on GitHub <#2329 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKLX2ZLRWBUPBCKUJTKOG6T2F36FLAVCNFSM6AAAAABRE6L5G2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBWGA4TSNBTGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

niklasmohrin · 2024-12-16T22:39:55Z

Great, good luck with that! No need to worry or hurry, we just want to track the status of the PR :)

richardebeling · 2024-12-20T13:59:46Z

evap/staff/tests/test_tools.py

+        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(),


The usage of PARTICIPATION_DELETION_AFTER_INACTIVE_TIME is misleading here, we just need some earlier start date. I think we can just use hardcoded 180/200 for the test (or any other value that we consider memorable enough, I'd also be fine with vote_end_date being 10 or 100 days ago)

Suggested change

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(),

cls.evaluation = baker.make(

Evaluation,

state=Evaluation.State.PUBLISHED,

vote_start_datetime=datetime.today() - timedelta(days=200),

vote_end_date=date.today() - timedelta(days=180),

richardebeling · 2024-12-20T14:02:39Z

evap/staff/tests/test_tools.py

+
+    @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):


This relies on the default setting value, but it would be nice if the test passed even if some developers locally changed the setting. Thus, we should also

Suggested change

def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation(self):

@override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(360))

def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation(self):

richardebeling · 2024-12-20T14:03:37Z

evap/staff/tests/test_tools.py

+
+    @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):


Needs an override on PARTICIPATION_DELETION_AFTER_INACTIVE_TIME to ensure we actually don't do anything because of can_be_marked_inactive_by_manager, and not because the time simply hasn't passed.

richardebeling · 2024-12-20T14:04:39Z

evap/staff/tests/test_tools.py

+        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."]
+        )
+


This is just the same assertion again, duplicated?

We should instead assert that the participations of the user haven't changed.

richardebeling · 2024-12-20T14:15:37Z

evap/staff/tools.py

    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:


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?

Redflashx12 added 18 commits October 21, 2024 18:40

Add PARTICIPATION_DELETION_AFTER_INACTIVE_MONTHS to settings.py with …

517af89

…default of 18 months

Add bulk delete of inactive users from their evaluations

b36e41e

Update to use timedelta and just mentioning months instead of convert…

6c9f948

…ing months to seconds

Filter users which do not have any evaluations in which they are inac…

bdfb350

…tive

change unit to days since months is not supported and add small comment

582aba5

fix typo

cc3e1b2

fix correct date subtraction

b4cf95c

Add default value for test_run

8c1a755

WIP test for inactivity

4e1fcf5

Refactor functionality to dedicated function

7dcfa62

Fix test by archiving the semester and using settings value in test f…

a2db34e

…or vote start/end date instead of flat value

Remove unneeded +1 and brackets, add evaluation needed to not be mark…

0b810cf

…ed as "can_be_marked_inactive_by_manager"

refactor and split into two tests

937acad

Fix indentation

7852f2e

Forgot to remove factor 30 for months since it's already included, ne…

60301ad

…ed to add +1 to get above threshold

black formatting

a4475e0

isort formatting

0a33410

increase test coverage by adding test case for test run

5f10109

richardebeling requested changes Nov 4, 2024

View reviewed changes

Redflashx12 and others added 10 commits November 11, 2024 17:43

Fix django import of settings

6cc1cf4

Co-authored-by: Richard Ebeling <[email protected]>

fix to use pythonic way of checking len

87bbf6d

Co-authored-by: Richard Ebeling <[email protected]>

Use timedelta instead of numeric value for days

b198732

Fix logic

d5fa9c6

Add inactive users to be removed

c9f3a0a

Change django magic query to look and check latest vote_end_date

51b5f99

Refactor test to use django's setUpTestData

2f5711c

"Fix" type hint to use | None instead of Optional

74e9076

reorder imports

1dc1fb7

linting

36a5a00

Redflashx12 added 2 commits November 11, 2024 22:16

Add more edge cases to tests

01d60a2

Remove assignment of never used variable

03622d6

Redflashx12 requested review from richardebeling and niklasmohrin November 11, 2024 21:21

Redflashx12 and others added 10 commits November 11, 2024 22:38

Merge branch 'main' into main

c526f84

refactor remove_inactive_participations

810a957

Rename variable

2ab9cff

Implement feedback

6715f77

Fix logic error and wrong type

351fbaa

Fix missing rename of variable in @override_settings

08983c3

Extend to test for user.evaluations_participating_in.exists() and rep…

eebef2b

…eat behaviour

Use mock.patch decorator instead of creating new Evaluations with use…

cd63d80

…r as participant and add beginning check for user.evaluations_participating_in.exists()

Remove unused import

3db7709

ruff fix

32a4c79

janno42 requested changes Dec 2, 2024

View reviewed changes

richardebeling requested changes Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete participations of long inactive users #2329

Delete participations of long inactive users #2329

Redflashx12 commented Nov 4, 2024

richardebeling left a comment

Redflashx12 commented Nov 11, 2024

janno42 Dec 2, 2024

janno42 Dec 2, 2024

niklasmohrin commented Dec 16, 2024

Redflashx12 commented Dec 16, 2024 via email

niklasmohrin commented Dec 16, 2024

richardebeling Dec 20, 2024

richardebeling Dec 20, 2024

richardebeling Dec 20, 2024

richardebeling Dec 20, 2024

richardebeling Dec 20, 2024

-            _("{} 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)

	def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation(self):
	@override_settings(PARTICIPATION_DELETION_AFTER_INACTIVE_TIME=timedelta(360))
	def test_do_not_remove_user_due_to_inactivity_with_recently_archived_evaluation(self):

Delete participations of long inactive users #2329

Are you sure you want to change the base?

Delete participations of long inactive users #2329

Conversation

Redflashx12 commented Nov 4, 2024

richardebeling left a comment

Choose a reason for hiding this comment

Redflashx12 commented Nov 11, 2024

janno42 Dec 2, 2024

Choose a reason for hiding this comment

janno42 Dec 2, 2024

Choose a reason for hiding this comment

niklasmohrin commented Dec 16, 2024

Redflashx12 commented Dec 16, 2024 via email

niklasmohrin commented Dec 16, 2024

richardebeling Dec 20, 2024

Choose a reason for hiding this comment

richardebeling Dec 20, 2024

Choose a reason for hiding this comment

richardebeling Dec 20, 2024

Choose a reason for hiding this comment

richardebeling Dec 20, 2024

Choose a reason for hiding this comment

richardebeling Dec 20, 2024

Choose a reason for hiding this comment