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

Better handling of deleted messages (Fixes #40) #45

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Better handling of deleted messages (Fixes #40) #45

wants to merge 5 commits into from

Conversation

danjjl
Copy link

@danjjl danjjl commented Jan 13, 2015

  • Messages deleted more than MESSAGES_DELETED_MAX_AGE days ago do not appear in the trash.
  • Users can mark a message as permanently deleted.

If both users permanently deleted a message or if message was deleted more than MESSAGES_DELETED_MAX_AGE days ago, then the message is removed from the database (on permanently_delete() call, on empty_trash() call or using delete_deleted_messages.py)

New dependency to handle MESSAGES_DELETED_MAX_AGE setting: django-appconf

  • I have not added extra tests.
  • They could be mistakes in the use of the notification package.
  • I have not updated the translations

* default is 30 days
* New project dependency: `django-appconf`
 * Job can now be called with no arguments: setting `MESSAGES_DELETED_MAX_AGE` is used
 * Or using previous behaviour (by passing a minimum age as an argument)
Internally we change the deleted_at date of a permanently deleted message to more than MESSAGES_DELETED_MAX_AGE days ago.
If both users permanently deleted a message or if deleted_at is more than MESSAGES_DELETED_MAX_AGE days ago, the message is deleted from the database.
Even if the message could not be deleted from the database (one of the users did not delete the message), the message will not appear in the trash anymore.
@arneb
Copy link
Owner

arneb commented Jan 13, 2015

Thank you for the contribution. Looks good, but I'm against adding django-appconf as a dependency (I can remove this myself when merging).

@danjjl
Copy link
Author

danjjl commented Feb 10, 2015

Just came back from holiday.

Is there anything you want me to change before you can merge?

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.

2 participants