Add ability delete received private messages#6378
Conversation
48848bf to
ba3af7c
Compare
crates/api/api_crud/src/private_message/delete_for_recipient.rs
Outdated
Show resolved
Hide resolved
| .and(not(private_message::deleted)) | ||
| .and(not(private_message::removed)), | ||
| .and(not(private_message::removed)) | ||
| .and(not(private_message::deleted_by_recipient)), |
There was a problem hiding this comment.
Needs to be something like this, so that these messages are only excluded if current user is not the creator (best write a unit test to ensure it works correctly).
| .and(not(private_message::deleted_by_recipient)), | |
| .and(private_message::creator_id.eq(person_id).or(not(private_message::deleted_by_recipient))), |
There was a problem hiding this comment.
I tried that code and it appears to crash the server process with a stack overflow error. I can get the exact error message (& stack trace), but I remember running into this before too. Will poke around to see why that's happening, but any insight would be appreciated!
There was a problem hiding this comment.
this is the error I receive, tried with RUST_BACKTRACE=1 which didn't seem to give any useful information either:
thread 'actix-server worker 3' (19510) has overflowed its stack
fatal runtime error: stack overflow, aborting
There was a problem hiding this comment.
Here isn't the right place to try to extract the creator. We have two options:
- Do it like you've done here, and don't show messages you've sent that the recipient deleted
- Add the
deleted_by_recipientlogic somewhere inside this function. That does have access to the creator and recipient.
I spose it might be important for message creators to still view their past messages, so the 2nd option is maybe better.
This also needs a unit test in crates/api/api_utils/src/notify.rs
There was a problem hiding this comment.
Ok, I managed to get it working, but I had to in-memory filtering, which is probably not ideal?
I couldn't add on to the existing filter you mentioned because as far as I can tell, I have no way of getting to the deleted_by_recipient field of the private message, from the notification?
I also added a unit test, hopefully it's sufficient.
d3d60b0 to
08fdba7
Compare
| .and(not(private_message::deleted)) | ||
| .and(not(private_message::removed)), | ||
| .and(not(private_message::removed)) | ||
| .and(not(private_message::deleted_by_recipient)), |
There was a problem hiding this comment.
Here isn't the right place to try to extract the creator. We have two options:
- Do it like you've done here, and don't show messages you've sent that the recipient deleted
- Add the
deleted_by_recipientlogic somewhere inside this function. That does have access to the creator and recipient.
I spose it might be important for message creators to still view their past messages, so the 2nd option is maybe better.
This also needs a unit test in crates/api/api_utils/src/notify.rs
| .filter(|r| { | ||
| r.private_message | ||
| .as_ref() | ||
| .is_none_or(|pm| !(pm.deleted_by_recipient && pm.recipient_id == my_person.id)) | ||
| }) |
There was a problem hiding this comment.
This works but we should really be doing this on the query side somewhere. I'll take a look and see if I can figure out where.
There was a problem hiding this comment.
Agreed. I poked around a bit to try and do it in the query part you referenced before, but couldn't figure it out. I'll take another look later today too.
There was a problem hiding this comment.
It can probably be done with query = query.filter(...) just before the let paginated_query. You check if private message is null (none) by checking if one column is null. Probably notification::private_message_id.is_null() if such a column exists.
There was a problem hiding this comment.
I figured it out I think: smorks#1
Unfortunately I had to remove one join (post image details) because with notifications, we're hitting the limits of diesel's joins. That's for us to deal with tho.
There was a problem hiding this comment.
Ahhhh ok, so that explains the stack overflow I was getting when doing it that way too.
Thanks for looking into that!
There was a problem hiding this comment.
No probs, ya sry about that, its a diesel related bug because we have too many joins there.
| .left_join(instance_join) | ||
| // This causes a stack overflow currently, due to diesel join limitations. | ||
| // image_details aren't critical for for post notifications anyway. | ||
| // .left_join(image_details_join()) |
There was a problem hiding this comment.
There are only 20 joins here. I tried to uncomment these lines and it works just fine with cargo 1.93
There was a problem hiding this comment.
It failed on my local machine for 1.92, but I'll try it for that one.
There was a problem hiding this comment.
I'm still getting the same overflowed stack error, even on 1.94
There was a problem hiding this comment.
Nevermind, had it added at the wrong place. I'll try again.
There was a problem hiding this comment.
I also got overflows on both 1.93 and 1.94, with that line uncommented.
There was a problem hiding this comment.
I got it, I had to change the position of the join to make it earlier.
Normally diesel isn't this finicky, its just with the large amount of joins there.
Once I get CI passing, I'll push directly to this branch. Won't be any logical changes, just clippy fixes.
|
I'll handle the clippy errors and such, and push directly to your branch to handle them. |
|
Added back in the |
| @@ -173,12 +173,17 @@ impl NotificationQuery { | |||
| } | |||
There was a problem hiding this comment.
Change the definition of this function to:
pub fn list(...) -> impl Future<Output = LemmyResult<PagedResponse<NotificationView>>> {
Box::pin(async move {
...
})Then its not necessary to use Box::pin for every single function call.
|
Thanks @smorks! |
No problem! I'll look at getting those UI changes done soon as well so this can be completely done. thanks to both of you for helping me out with this one! |
This should fix #4120.
It also required PR's in lemmy-ui and lemmy-js-client, that I will open shortly too.
There are some issues with this but I figure I'd get this into a PR before I forget about it again, haha.
The only remaining issue that I know about is that this also causes the message to stop showing for the sender too. Pretty sure it just needs the view modified so it only filters the message for when the recipient of the message matches the current user, but I haven't looked to hard into doing that yet.
Edit: Oops, I just noticed that I should update the timestamp of the migration file too.