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

Fix some bugs when viewing a user's posts: include their self-replies (threads) even when excludeReplies is set, and use in_reply_to_uri instead of in_reply_to_id to filter out replies #769

Merged

Conversation

blackle
Copy link
Contributor

@blackle blackle commented Aug 27, 2022

This makes the behaviour more like mastodon: excludeReplies=true will still show replies that the user made with themselves (i.e. a long thread)

also fixes a bug where a post that is a response to another post can accidentally appear when excludeReplies=true, since the in_reply_to_id is unset because the post it was replying to isn't in the instance's DB.

… (threads) even when excludeReplies is set, and use in_reply_to_uri instead of in_reply_to_id to filter out replies
@blackle
Copy link
Contributor Author

blackle commented Aug 27, 2022

I will note there's a caveat here: if there is a thread of posts like:

  • user1: I like cheese
    • user2: @ user1 omg so do I
      • user2: @ user1 my favourite is gouda :D

the last post will appear in the user's account page because they're technically replying to themselves, though there is an @ in it. a better solution might be to check the body of the post for @ s, especially if the whole thread can't be fetched for whatever reason. I may take a look at how mastodon does it internally.

Copy link
Contributor

@tsmethurst tsmethurst left a comment

Choose a reason for hiding this comment

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

I realised when reviewing this code that we don't use in_reply_to_uri consistently: rn it's only set for statuses that come in via the federating API, not statuses created via the client API. So for this change to be effective and not cause bugs elsewhere, we should also set the reply_to_uri here as well:

status.InReplyToID = repliedStatus.ID

@tsmethurst
Copy link
Contributor

tsmethurst commented Aug 27, 2022

a better solution might be to check the body of the post for @ s

I don't think this is a good heuristic: users can create replies to other users and just remove the @ at the beginning if they like. Then it will still be a reply, it just won't contain a mention anymore. Perhaps instead of doing all of this via db selects and WHEREs, it might be worthwhile to generalize/repurpose some of the logic from the StatusHometimelineable function here: https://github.com/superseriousbusiness/gotosocial/blob/main/internal/visibility/statushometimelineable.go

[edit] I'm not saying this is definitely the way to go or anything, just throwing some ideas around :P

@blackle
Copy link
Contributor Author

blackle commented Aug 27, 2022

"we should also set the reply_to_uri here as well:"

seems like we'd want a migration to fix existing posts as well. something like

UPDATE statuses
SET in_reply_to_uri = (SELECT uri FROM statuses AS secondary WHERE id = statuses.in_reply_to_id)
WHERE in_reply_to_id IS NOT null AND in_reply_to_uri IS null;

"I don't think this is a good heuristic..."

yes, I agree, though I want to avoid a big bunch of refactoring for now (still getting my feet wet with incremental improvements.) I do think that refactoring this in the future to be more consistent is a good idea.

@tsmethurst
Copy link
Contributor

Right, makes sense! Agree that it's definitely due a good refactor, but indeed out of scope for this PR.

Re: the migration to retroactively update old post reply_to_uri's, I think it's not too important, as long as we just fix the logic for new posts going forward: at worst, people see more posts than they expected from people they share an instance with, which is not really a big deal. We're still in alpha so (imo) it's OK to have some weirdness as we go along fixing things :)

Speaking of migrations though, we should create a migration to add an index on in_reply_to_uri, since we're selecting using that now.

@blackle blackle force-pushed the better-account-timeline-filter branch from 16ed6fb to 0331669 Compare August 27, 2022 09:17
@blackle
Copy link
Contributor Author

blackle commented Aug 27, 2022

Added the migration. I figured out how to do the statuses update using bun so I just added it to the migration that adds the index.

@tsmethurst
Copy link
Contributor

Alright cool! Don't forget to add the reply_to_uri to new statuses created through the client api: #769 (review)!

@tsmethurst tsmethurst merged commit 54f6cae into superseriousbusiness:main Aug 27, 2022
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.

None yet

2 participants