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 moving an item up #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

dmarkow
Copy link

@dmarkow dmarkow commented Mar 29, 2017

Because the query to find the two previous items wasn’t sorted in descending order, it would always return the first two items in the list (positions 1 and 2), not the two items immediately before the item we want to move up. For nearby_query on an item in the 10th position would return the 1st/2nd items, not the 8th/9th.

The existing test didn't catch this because it was only using a 3-item list. I updated the test with 4 items, moving the 4th item up to make sure it goes between the 2nd/3rd items (instead of the 1st/2nd). Solution was to update the nearby_query function to accept an order_by direction.

options
|> rank_query
|> scope_query(options, cs)
|> select_rank(options.rank_field)
|> limit(2)
|> order_by(^options.rank_field)
|> Ecto.Query.exclude(:order_by)
Copy link
Author

@dmarkow dmarkow Mar 29, 2017

Choose a reason for hiding this comment

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

This exclude call is needed because rank_query already sets the order to be ascending on the rank field, so setting a descending order on the next line would be ignored.

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