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

[Bug Report] Deleting a user breaks all pages where their edits appear #623

Closed
aghoulcoder opened this issue Mar 11, 2023 · 3 comments · Fixed by #674
Closed

[Bug Report] Deleting a user breaks all pages where their edits appear #623

aghoulcoder opened this issue Mar 11, 2023 · 3 comments · Fixed by #674
Labels
help wanted Extra attention is needed

Comments

@aghoulcoder
Copy link
Contributor

aghoulcoder commented Mar 11, 2023

Describe the bug

Deleting a user breaks all pages where their edits appear. If an edit by a deleted user appears in the /edits page or in individual "edits" sections of performers, studios, and scenes, the page fails to fully render and displays: Error: Failed to load edits.

To Reproduce

  1. Create a user.
  2. Make an edit with that user and have it approved.
  3. Delete the user.
  4. Visit the edit directly or any page where the edit should appear. Enjoy your Error: Failed to load edits.

Expected behavior

Pages should render correctly.
There are multiple potential approaches to solving this but ultimately, the expected behavior is that deleting a user should not break the site.

Workaround/Recovery

To resolve the database corruption, I created a dummy user named deleted user.

I then set user_id to be the deleted user id in all affected records of the edits and edit_comments tables.

stash-box=# select id from users where name = 'deleted user';
                  id                  
--------------------------------------
 <ID>
(1 row)

stash-box=# update edits set user_id = '<ID>' where user_id is null;
UPDATE 4
stash-box=# update edit_comments set user_id = '<ID>' where user_id is null;
UPDATE 4

Now all the edits are rendered correctly and the edits of the deleted user are attributed to a user named 'deleted user'.

Solution proposals

  • Don't allow user deletion. That's what mediawiki does.
    They explain their rationale for not deleting users in their FAQ.
    They also address GDPR considerations relating to the inability to delete users

  • Allow user deletion but instead of setting the id to null, set it to a special "deleted user" uid that the system will create just for this purpose. This is similar to what is possible in mediawiki through the UserMerge extension. Conceptually, you would merge all edits from a deleted user into the special "deleted user" user.

  • Patch it in the frontend somehow? Make the frontend handle null values for user_id. I'm no expert in database design but this sounds like the most messy approach.

Desktop

any

Smartphone

any

Additional context

This bug was encountered in https://pmvstash.org

@aghoulcoder aghoulcoder added the help wanted Extra attention is needed label Mar 11, 2023
@InfiniteStash
Copy link
Collaborator

Nulling the user_id on delete is a bit of an oversight since the system does not expect null values. Optional values are annoying to deal with, so I think the solution is to just leave the original ID in place. We could obfuscate the edit history by setting to a fixed "anon id" as you suggest, but that makes restoring accounts impossible.

@aghoulcoder
Copy link
Contributor Author

Thanks for checking this @InfiniteStash !
I think what you propose makes sense, especially in combination with a clear "block" function which could be used as an alternative that doesn't affect the edit history. Currently removing all roles functions somewhat like a block, but the user can still login/authenticate against the system.

@InfiniteStash
Copy link
Collaborator

Looking at it again now the frontend already supports optional values, so it was just a matter of making the db value optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants