Fix check_scope to work on lists with array scopes #81
Conversation
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
|
Can you please add 1.8.7 compatibility, a lot of users are still on 1.8.7? |
|
@swanandp No problem, I'll try and get that in as soon as possible. |
|
@swanandp Sorry I haven't had a chance to work back on this, 1.8.7 is EOLed in 2 days, do you still require support for that? |
Squashed commits follow: Fix issue with old order not being set When moving an item between lists, previously the old order was not being set on the record before calling `decrement_positions_on_lower_items`. This had the side effect of leaving the previous list with duplicate or incorrect order numbers if you moved the item to a new list with a different position. TODO: Add support for multiple scope conditions when moving an item between lists. Refactor `check_scope` for clarity and DRYness. Conflicts: lib/acts_as_list/active_record/acts/list.rb Add failing tests Add failing tests for moving items between lists with multiple scopes Implement moving between lists with array scope When check_scope is called it checks if there have been any changes to scope properties. This is handled by a new private method, scope_changed? which looks at a new method 'scope' which returns the raw scope data. If the scope has changed, swap_changed attributes is called, this sets all the changes back to the objects previous state so we can update it's previous list. The changes are then swapped back using the same method and we add it to the new list. TODO: - Consider changing check_scope to a better name like update_previous_list? - How should this handle scopes that use string interpolation? Should the scope always be considered changed? Remove scope_name function Add comments to private methods Add tests for changing only one scope property Previous tests only covered changing all scope properties at once. Add tests for moving to empty list Add scope_name function back in Pass move_within_scope tests
|
@swanandp Should be passing 1.8.7 now, rebased. |
swanandp
added a commit
that referenced
this pull request
Jul 2, 2013
Fix check_scope to work on lists with array scopes
|
@conzett Thanks! 👍 |
Merged
|
This has not made it into latest gem on Rubygems (there the latest one is 0.2.0 from Feb 28.) Can you make sure this update is available there as well. Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Adds tests for several scenarios. Squashed commits an rebased. Build now passing on 1.8.7