Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete participations of long inactive users #2329

Open
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

Redflashx12
Copy link
Collaborator

Fixes #2176.

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..

Copy link
Member

@richardebeling richardebeling left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
evap/staff/tools.py Outdated Show resolved Hide resolved
evap/staff/tools.py Outdated Show resolved Hide resolved
evap/staff/tools.py Outdated Show resolved Hide resolved
evap/staff/tools.py Outdated Show resolved Hide resolved
@Redflashx12
Copy link
Collaborator Author

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 👍.

evap/staff/tools.py Outdated Show resolved Hide resolved
evap/staff/tools.py Outdated Show resolved Hide resolved
@niklasmohrin
Copy link
Member

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

@Redflashx12
Copy link
Collaborator Author

Redflashx12 commented Dec 16, 2024 via email

@niklasmohrin
Copy link
Member

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

evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
evap/staff/tests/test_tools.py Show resolved Hide resolved
evap/staff/tests/test_tools.py Show resolved Hide resolved
evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
evap/staff/tools.py Outdated Show resolved Hide resolved
Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, I only have minor comments:

evap/staff/tools.py Outdated Show resolved Hide resolved
@@ -195,6 +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 in users_to_mark_inactive:
for message in remove_inactive_participations(user, test_run):
messages.warning(request, message)
Copy link
Member

@niklasmohrin niklasmohrin Jan 27, 2025

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

In the future, it would probably be nice to have the accounts listed in one message like those to be marked inactive, but I think it's fine for now.

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

Successfully merging this pull request may close these issues.

Delete participations of long inactive users
4 participants