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 account search not returning followed accounts first #22956

Merged
merged 4 commits into from Jan 6, 2023

Conversation

dariusk
Copy link
Contributor

@dariusk dariusk commented Jan 5, 2023

This makes it so that (when elasticsearch is disabled) when a user types @foo in the compose box, they are first going to get accounts they follow ordered by the usual ranking algorithm, and then second they will get accounts they do not follow, also ordered by the ranking algorithm.

This makes behavior more consistent with user expectation and also with results when elasticsearch is enabled.

This makes it so that (when elasticsearch is disabled) when a user types '@foo' in the compose box, they are first going to get accounts they follow ordered by the ranking algorithm, and then second they will get accounts they do not follow, also ordered by the ranking algorithm.

This makes behavior more consistent with user expectation and also with results when elasticsearch is enabled.
@a-dows
Copy link

a-dows commented Jan 5, 2023

Please! Users are extremely unlikely to compose a new post to a random person with lots of followers rather than someone they follow.

@dariusk
Copy link
Contributor Author

dariusk commented Jan 5, 2023

Also, I am not sure why there are linter errors happening here. They apply to lines of code I did not touch and that are the same as what I see in the currently-passing main branch.

@Gargron
Copy link
Member

Gargron commented Jan 5, 2023

What about the join two lines up?

@dariusk
Copy link
Contributor Author

dariusk commented Jan 5, 2023

@Gargron What about it?

@lucasgonze
Copy link

lucasgonze commented Jan 5, 2023

This is a good idea as a feature, and the code seems credible.

I wonder about the efficiency of the subselect. A subselect is not optimizable. Given a relatively trivial look at the query, I can imagine an approach that modifies the LEFT OUTER JOIN follows. (This is a naive perspective - I am not a Mastodon contributor).

@a-dows
Copy link

a-dows commented Jan 5, 2023

This is a good idea as a feature, and the code seems credible.

to be clear, before mastodon 4.0, followed accounts were returned first. The change to algorithm-only for search-disabled instances is new in 4.

@dariusk
Copy link
Contributor Author

dariusk commented Jan 5, 2023

I updated the SQL and it is much improved. No nasty extra JOIN. Thanks @Gargron for the pointer

@Gargron Gargron changed the title Make autosuggest for mentions return followed accounts first Fix account search not returning followed accounts first Jan 5, 2023
Copy link
Member

@Gargron Gargron left a comment

Choose a reason for hiding this comment

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

For the record this is a regression in 4.0. The described behaviour has already existed in Mastodon.

app/models/account.rb Outdated Show resolved Hide resolved
@Gargron Gargron merged commit 264655c into mastodon:main Jan 6, 2023
nametoolong pushed a commit to nametoolong/nuage that referenced this pull request Jan 12, 2023
)

* Make autosuggest for mentions return followed accounts first

This makes it so that (when elasticsearch is disabled) when a user types '@foo' in the compose box, they are first going to get accounts they follow ordered by the ranking algorithm, and then second they will get accounts they do not follow, also ordered by the ranking algorithm.

This makes behavior more consistent with user expectation and also with results when elasticsearch is enabled.

* Fix ranking order to correct direction

* One more fixup per @Gargron suggestion

* Tweak to ranking to no longer include following modifier
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

4 participants