Added Functionality for Repairing Duplicate Positions in List #335
Conversation
|
@brendon I am still working on this PR but I put it here so that you can have a look and let me know if this is what you had in mind. I still need to write the functions and checks for gaps in the list, improve tests and write documentation. I might also add a protection to impede the function from getting stuck in the while look... Just not sure if it makes sense... |
There was a problem hiding this comment.
A good start @pacarvalho :) Let me know once you added some more.
| # Checks if a list has no repeating indices | ||
| # Returns true if at least one value position_column has a duplicate value | ||
| def does_list_have_duplicates? | ||
| acts_as_list_class.group(position_column).having("count(?) > 1", position_column).any? |
There was a problem hiding this comment.
This could be .exists? instead of .any? I think (off the top of my head).
| # While there are duplicates we will increment the position of the second repetition for every number | ||
| while does_list_have_duplicates? do | ||
| # Get the position with repetitions | ||
| repeated_pos = acts_as_list_class.group(position_column).having("count(?) > 1", position_column).first.send(position_column) |
There was a problem hiding this comment.
Since you're using acts_as_list_class.group(position_column).having("count(?) > 1", position_column) more than one, you should probably make it a scope on the mode and give it a friendly name :)
| # Get the position with repetitions | ||
| repeated_pos = acts_as_list_class.group(position_column).having("count(?) > 1", position_column).first.send(position_column) | ||
| # Get one of the entries in the given position. This will be the entry that will keep the current position | ||
| first_repeated_entry_id = acts_as_list_class.where("#{position_column} >= ?", repeated_pos).first.id |
There was a problem hiding this comment.
Shouldn't this be = not >=? And if so, you can simplify the where call to remove the string interpolation :)
| end | ||
| end | ||
|
|
||
| # Check that there are no gaps in the list |
There was a problem hiding this comment.
Looking forward to what you come up with here :)
| # Get one of the entries in the given position. This will be the entry that will keep the current position | ||
| first_repeated_entry_id = acts_as_list_class.where("#{position_column} >= ?", repeated_pos).first.id | ||
| # Increment the position of all entries with position equal or greater than the current repeted position that is not the first entry | ||
| acts_as_list_class.where("#{position_column} >= ?", repeated_pos).where.not(id: first_repeated_entry_id).increment_all |
There was a problem hiding this comment.
When interpolating the position column use quoted_position_column_with_table_name as per list.rb.
| assert_equal true, DefaultScopedMixin.first.does_list_have_duplicates? | ||
|
|
||
| # Fix the list | ||
| DefaultScopedMixin.first.repair_duplicate_positions_in_list |
There was a problem hiding this comment.
I think the repair code would be better attached to the class rather than the instance. We can still call it from the instance.
There was a problem hiding this comment.
It would be DefaultScopedMixin.repair_duplicate_positions_in_list(DefaultScopedMixin.first) instead?
…osition_column_with_table_name, removed mention of detecting gaps
pacarvalho
changed the title
pacarvalho
changed the title
|
I'm still interested in adding this. Also see https://github.com/swanandp/acts_as_list/pull/284/files which I've closed but had a test for detecting corrupted lists. Perhaps we could add an auto-correct option (off by default)? |
|
@pacarvalho Is this still on your radar? |
|
@oyeanuj I am interested in seeing this completed. Just need to know what needs to be changed/added. It has been a while since I started this so I don't quite remember where we left off :/ Are you interested in collaborating? |
|
I'm happy to review what you guys come up with. I think the overarching goal of any auto-correction would be for it to be completely transparent to the user (and with no side effects). Since it'll likely happen at the time a change is being made to the list, this could get messy as if positions of items change from what the user was expecting, their intentions could become invalid. I might have the wrong end of the stick however, it's been a long time since I looked at this :D |
|
@brendon @pacarvalho Thanks for the quick responses! I honestly don't understand the code enough (or for that matter Rails), to be able to come up with a sufficiently competent PR. Mostly, I was curious if I should be expecting this PR to this library or not :) |
|
Closing this as it's been 4 years. Check out https://github.com/brendon/positioning as an alternative that does its best to avoid list corruption. It works with database uniqueness constraints and advisory locks to prevent these kinds of issues. |
closes #333