-
-
Notifications
You must be signed in to change notification settings - Fork 134
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
Skip-reranking when current_first/current_last is nil #148
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is otherwise nil. It's need to be truthy for
rearrange_ranks
to be calledThere 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.
return value never seems to need to be anything but truthy since it was introduced 8280511
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.
That's true. Perhaps it was intended for future use. For it to return a boolean you'd probably want to implement a
current_at_rank?
method.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.
@brendon So, the reason I'm looking at
current_at_rank
at all is because I wanted to reproduce the problem I experienced: creating a record, specifying a position, rearrange ranks failing due to current_first being nilIn order to do that, I discovered that in order for rearrange_ranks to be called in my situation,
current_at_rank
had to be truthy. I looked at the method and determined, for mocking purposes, I didn't need to care about what it was doing or returningI was a little curious about
current_at_rank
since I didn't see it used anywhere else or directly tested. So, I looked through the version history and found as noted above that it was introduced to replace the condition(current_order.find do |rankable| rankable.rank.nil? || rankable.rank == rank end)
which confirmed that its usage inassure_unique_position
is only to return something truthy.Interestingly, it's truthiness was replaced by
finder.except( :order ).where( ranker.column => rank ).first
where 'finder' is more or less equivalent to 'current_order'now, why would
current_at_rank(rank)
be truthy whencurrent_first
is nil, when current_first is defined asfinder.first
. I'm not sure, but it could be a race condition or a query cache issue or related to theexcept
call (though I doubt it)For the purpose of reproducing the failure and testing fixes, I think my test and and prs are sufficient
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'm not sure exactly what the intended meaning of current_at_rank is, or if it changed during the refactor. I think it is that another record exists?
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.
Let's continue the discussion here: #149