-
-
Notifications
You must be signed in to change notification settings - Fork 357
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
Show items with same position in higher and lower items #231
Show items with same position in higher and lower items #231
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jpalumickas :) Thanks for this. It's a good idea. I've often been bit by this problem.
@@ -262,11 +262,12 @@ def higher_item | |||
|
|||
# Return the next n higher items in the list | |||
# selects all higher items by default | |||
def higher_items(limit=nil) | |||
def higher_items(limit = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is causing your test failures. I'm not sure spaces are ok between the argument and the default value. So do limit=nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -279,11 +280,12 @@ def lower_item | |||
|
|||
# Return the next n lower items in the list | |||
# selects all lower items by default | |||
def lower_items(limit=nil) | |||
def lower_items(limit = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And again here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use update_column
instead of update_columns
in tests to support older Rails versions.
Thanks @jpalumickas. A great fix! I thought of another potential problem that probably needs its own PR. What if we have 1, 2, [2], 2, 3, 4 and we pick out the one in brackets and move the rest lower. We'll end up with 1, 2, 3, 3, 4, 5. Ideally we'd want 1, 2, 3, 4, 5, 6. I suppose that's a harder problem to fix though. Unless we renumber all of the lower items explicitly instead of relying on their current position with SQL. Maybe something like this: http://stackoverflow.com/questions/15930514/mysql-auto-increment-temporary-column-in-select-statement |
Currently, when we're trying to move up and down items with same positions, they are not found in scope, because they have the same id. We need to show all items with the same id but exclude current one.
@brendon can you take a look ?