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

Can't delete system messages #1049

Closed
pushcx opened this issue Jan 4, 2022 · 5 comments
Closed

Can't delete system messages #1049

pushcx opened this issue Jan 4, 2022 · 5 comments

Comments

@pushcx
Copy link
Member

pushcx commented Jan 4, 2022

When a Story is edited by user suggestions, the submitter gets a Message. That Message gets an author_user_id of 0 and appears in the inbox as being from System rather than a named user.

Since 12644b2 (and triggered by a user for the first time last month after >3y!) a bug prevents deleting these Messages, individually or batch-delete. When a recipient deletes a message it toggles the deleted_by_recipient boolean and saves, whereupon a callback (arrrgggggh) checks to see if the record should be deleted from the database. But instead an exception is thrown by .save! because the object is invalid without a real author_user_id.

This needs a fix so users can delete system messages. Maybe the right fix is to make author_user_id nullable and migrate to replace the 0s with NULLs? Or maybe MessagesController#destroy and #batch_delete should just directly delete records when possible and delete the check_for_both_deleted magic entirely?

@mrtorks
Copy link

mrtorks commented Jan 23, 2022

Hi @pushcx
Interesting bug.

I do agree with using setting records with NULL instead of 0 and then check for a null condition with an override to delete the record.
Directly deleting the record through
MessagesController#destroy and #batch_delete is also a great idea however in my opinion, implementing that might introduce complexity.

If possible more details about the bug could help me develop a more concrete solution.

@pushcx
Copy link
Member Author

pushcx commented Jan 23, 2022

What details would be useful to you? After I figured out what caused the exception I wrote this up to give as much context as possible, so I don't know what more would help or I would've already included it. :)

@mrtorks
Copy link

mrtorks commented Jan 23, 2022

Pardon my dust @pushcx my thought process ran wild and it was late when I was responding. Using the null option definitely works. You could give that a try to see where it goes.

@vincentrolfs
Copy link
Contributor

I'm on it :)

vincentrolfs added a commit to vincentrolfs/lobsters that referenced this issue Apr 1, 2022
One of these tests is failing right now, this is subject of issue lobsters#1049 and will be fixed in the next commits.
@vincentrolfs
Copy link
Contributor

I created PR #1078

@pushcx pushcx closed this as completed in b90a2cb Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants