Fix for #317

[Merovex]

I've never done a PR before, but I think I've fixed the problem.

• Extended test coverage to cover what I understand the issue to be: an insert_at value greater than the maximum position.
• Added bounds check in insert_at_position.

[brendon]

Hi @Merovex, well done on your first PR :)

Can you give me a rundown of the problem with an example? I'm struggling to understand your test case.

insert_at hasn't ever clamped to the highest value in the list so this could be a breaking change if people are using it to set arbitrary position values.

[Merovex]

OP's #317 problem appears to be caused when there's a gap, e.g [1, 2, 3, 4, 6]. I encountered the same problem with default HTML drag-and-drop (DnD). I explain in my workaround that this causes problems in DnD where the last item becomes pinned. I used insert_at, which is an alias for insert_at_position. This happens because DnD assigns the position after the last item. I explained my problem originally in #317. My solution was to bound-check the list and ensure contiguous values.

Given a list: [1, 2, 3, 4, 5]
when I DnD 2 to end of list: [1, 3, 4, 5, 6]
and insert_at_position reindexes
then I have [1, 2, 3, 4, 6]

The test I created with this PR asserts contiguousness is the preferred behavior. It's a copy from the test above that assumes when user moves 2->5, which asserts [1, 2, 3, 4, 5] after move (would have been [1, 3, 4, 5, 5], which is a bit of a shock).

If non-contiguousness is the designed intent, then #317 is a cautionary tale vice an incident. Through the thread, the feedback appears to be "should be guaranteed," and you seem to concur, and invited a PR (that I might have misinterpreted as I say below).

If this is not the PR you're looking for, I'm happy to adjust. If the goal is to find/remove gaps, then my reindex method could be adapted. Perhaps with an option for explicit_sequencing to avoid breaking those who rely on the gap. I see there's a second case when a deletion occurs, which makes the reindex more relevant than bounds-checking. It's a bad label since the model's index isn't affected, resequence is probably better.

(For context, I'm a developer who shifted to PM work right before Rails took off, or I would have stayed a developer. My PM work doesn't get within miles of code and my "next career" may be novel writing. Or, I'll code something that sells.)

[brendon]

Thanks for the details @Merovex. Can you let me know why you use insert_at instead of just setting the position column value to what you want it to be and saving the record?

item.update position: 6

This should trigger a set of callbacks that keep the list contiguous. Can you try that approach, or let me know why you can't use this method?

[Merovex]

Well, that certainly works. If someone had provided that feedback on 1 April 2022 when I presented my quandry and solution, I would have implemented it and moved on.

[brendon]

If you scroll up that issue you'll see a previous comment by me saying exactly that advice:

#317 (comment)

and then again further down:

#317 (comment)

To be fair, the actual docs for this gem don't mention this convenience even though it's basically the most useful part. I think this functionality came about later on in this gem's history.

If you'd like to have another crack at your first PR, one that improves the README would be greatly appreciated. :D