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

Create "DeletedUser" model for tracking number of deleted users and deletion reason #1118

Closed
wants to merge 29 commits into from

Conversation

metalerk
Copy link

Create "DeletedUser" model for tracking number of deleted users and deletion reason #1086

DeletedUser model should contain following fields:

  • date of deletion
  • deletion reason (by user request, by staff, by agreement decline, inactive account)

In future we could use this data to display both positive and negative trends on "forum growth" graph in the admin panel.

NOTE

The DeletedUser model was created based on UsernameChange model.

@rafalp
Copy link
Owner

rafalp commented Nov 1, 2018

Thank you for opening PR to Misago!

I've did quick skim of your code, and there's little more that needs to be done before this will be mergeable.

First, this model will have to be moved to its own file. I know there are other models packed in user.py, but this is something we want to eventually fix.

Next, model will have to be reduced to two fields: deleted_on, reason. (Prefixing all fields with deleted is unnecessary).

user relation is meaningless because model we are pointing at won't exist.

reason should be positive int field accepting one of three states: "by user", "by staff", "by system". Perhaps this field should be named "deleted_by"?

The idea for this model is to hold historical data on user deletions somewhere, so in future we'll be able to display both positive and negative trends on user registrations over period of time.

Also we don't want to keep any usernames on this model for GDPR compliance reasons.

Lastly, we will need to have migration creating database table for this model, utility module for recording deletions, update codebase to register those events, and add test assertions.

@rafalp rafalp self-requested a review November 1, 2018 18:01
@rafalp rafalp added area: backend This issue involves Python, Django or dependency (eg. database) new feature New feature labels Nov 1, 2018
@metalerk
Copy link
Author

metalerk commented Nov 5, 2018

About migrations update, where can I find it in the documentation?

@rafalp
Copy link
Owner

rafalp commented Nov 11, 2018

Hi,

Sorry I've didn't answer this sooner, I was out of the country (continent in fact) so my time for Misago has been severly limited.

Misago is using Django framework, and its documentation for database migrations is available here.

Long story short: ./dev manage.py makemigrations is what you want.

@metalerk
Copy link
Author

Oh, you're referring to Django migrations, I thought you were talking about something else.
Cool, I'll update it 👍

Copy link
Owner

@rafalp rafalp left a comment

Choose a reason for hiding this comment

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

So far this is looking very nice, thank you. 👍

misago/users/migrations/0016_deleteduser.py Outdated Show resolved Hide resolved
misago/users/models/deleteduser.py Outdated Show resolved Hide resolved
misago/users/models/deleteduser.py Outdated Show resolved Hide resolved
misago/users/utils.py Show resolved Hide resolved
misago/users/utils.py Outdated Show resolved Hide resolved
)

class Meta:
get_latest_by = "deleted_on"
Copy link
Owner

Choose a reason for hiding this comment

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

Please use ordering = ['deleted_on'] instead, and set db_index=True on deleted_on field.

misago/users/utils.py Outdated Show resolved Hide resolved
def record_user_deleted_by_self():
return DeletedUser.objects.create(
deleted_by=1,
deleted_on=timezone.now()
Copy link
Owner

Choose a reason for hiding this comment

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

It's unnecessary to do deleted_on=timezone.now() in utility functions, because model itself already sets those on creation.


def record_user_deleted_by_self():
return DeletedUser.objects.create(
deleted_by=1,
Copy link
Owner

Choose a reason for hiding this comment

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

Please replace magical numbers with human-understandable names.

misago/users/models/deleteduser.py Show resolved Hide resolved
@metalerk
Copy link
Author

This last year I was full of work. Is this still relevant or should be closed? :)

@rafalp
Copy link
Owner

rafalp commented Jul 30, 2019

Hey!

In Misago admin panel we are now displaying the graph for new users registrations. I would like to merge this PR eventually so we could also display "User deletions" graph somewhere in the, or on dedicated page in admin panel.

Thanks for memory btw. If you have time to spare, could you rebase the PR on latest master to fix conflicts?

@metalerk
Copy link
Author

As i can see, all the requested fixes are done :)

@metalerk
Copy link
Author

The migration conflict is now fixed :)

@rafalp
Copy link
Owner

rafalp commented Aug 25, 2019

@metalerk doesn't seem like its fixed. Could please you remove your migration from this PR, rebase against master, and then add new migration?

@rafalp
Copy link
Owner

rafalp commented Aug 26, 2019

@metalerk please make sure you are working with latest Misago code. Migration you've generated is not reflecting that theres already an migration with ID 0021.

@metalerk
Copy link
Author

@rafalp you're right, I noticed that my master branch is not being synced with upstream.
Today later I'm going to fix it and rebase properly :)

@rafalp
Copy link
Owner

rafalp commented Sep 1, 2019

This is not a rebase. This PR now includes changes that are not part of it like SSO changes. ;)

@rafalp
Copy link
Owner

rafalp commented Sep 28, 2019

I've pulled this branch locally and rebased it against master. I will ship this in Misago 0.23. Thanks!

@rafalp rafalp closed this Sep 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: backend This issue involves Python, Django or dependency (eg. database) new feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants