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

Remove private messages after specific cache duration #1333

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

chrysle
Copy link
Contributor

@chrysle chrysle commented Aug 18, 2024

This will not affect messages in the 'archive' folder. Closes #372. Probably also a configuration option should be created for this, in case users want to disable it? At least there'll have to be an announcement for the changed behaviour.

This will not affect messages in the 'archive' folder.
@sebix
Copy link
Member

sebix commented Aug 19, 2024

Probably also a configuration option should be created for this, in case users want to disable it? At least there'll have to be an announcement for the changed behaviour.

Please also coordinate this with the uu.de team members, as this change will have an impact on the documentation of communication between team members with users

@chrysle
Copy link
Contributor Author

chrysle commented Aug 19, 2024

Please also coordinate this with the uu.de team members

I will, definitely before the next release. Could an option also be to disable this by default for team members?

@sebix
Copy link
Member

sebix commented Aug 19, 2024

Please also coordinate this with the uu.de team members

I will, definitely before the next release. Could an option also be to disable this by default for team members?

Could be a suitable mitigation, yes.

But I wonder why deleting the messages from Inbox and Sent is necessary in the first place.

@chrysle
Copy link
Contributor Author

chrysle commented Aug 19, 2024

But I wonder why deleting the messages from Inbox and Sent is necessary in the first place.

The contents of most messages should be fairly trivial, in most cases; probably they are never again looked at, and all take space. More important material can still be archived.

Copy link
Member

@chris34 chris34 left a comment

Choose a reason for hiding this comment

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

Additionally, to the inline comments: is_team_member exists, if messages should not be deleted for them.

has been reached.
"""
privmsgs_trash = PrivateMessageEntry.objects.filter(
folder="2",
Copy link
Member

Choose a reason for hiding this comment

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

i would suggest to remove magic numbers (and instead use a constant in the model f.e.)

after end of cache duration according to settings
has been reached.
"""
privmsgs_trash = PrivateMessageEntry.objects.filter(
Copy link
Member

Choose a reason for hiding this comment

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

I's better to move logic out of @shared_task, to for example two methods in the model manger. So the shared task would only call these two (plus the logger)

This will make it possible to add some basic tests (for example: are only old messages are deleted? are messages in archive are kept? ...)

@chrysle
Copy link
Contributor Author

chrysle commented Sep 1, 2024

I'll come back to this soon, sorry, I was quite busy with other things lately.

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

Successfully merging this pull request may close these issues.

Celery Tasks to purge Trash for PNs
3 participants