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

[Bugfix] Make filter boosts respect local vs. remote scope #38

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

sneakers-the-rat
Copy link
Collaborator

Closes: #37
Continues: #36

The first version of filter duplicate boosts forgot to take the max of only local or remote scopes, so if a post had been boosted more recently by a remote account, the most recent local boost wouldn't be seen and vice versa for the federated feed. This fixes that by including those scopes in the correlated subquery.

Caveats:

  • I was able to figure out how to express correlated subqueries using Arel, but try as I might, I wasn't able to figure out how to change the table alias of the contexts from Status when used in the subquery (so rather than WHERE s2.local = true it would always be WHERE statuses.local = true, which didn't work). This means that if the definition of the contexts change, then our subquery will get out of sync, but if that happens the tests should catch us.

Other changes

  • Add a multicolumn index on reblog_of_id and id to make subquery faster!
  • Move the scope.merge! call out of the block of other scopes so that it's easier to maintain - fewer merge conflicts
  • split scope into two scopes to match rest of structure which only has one scope per method
  • remove crappy sleep hack to ensure monotonic increase of IDs, just manually increment them.
  • explicit rspec contexts for local and remote feeds

I am not sure why the ruby linting is failing now, but i didn't touch any of those files - we'll sort of linter errors when we merge upstream tomorrow

… scopes as far as i can tell, so this is not the most desirable implementation (because we will fall out of sync if the Status.local or Status.remote definition changes) but in that case we should catch the test failing.
… local boosts from remote feed for completeness sake
@sneakers-the-rat sneakers-the-rat merged commit 8dc82ed into dev Feb 1, 2024
54 of 56 checks passed
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.

1 participant