Support inserting new records at a specified position

[DanielHeath]

Resolves #287 .

[brendon]

Hi @DanielHeath, thanks for this one. There are a lot of test failures in the CI suite. Would you be able to look into those when you get some time. Looks like it's mostly around unique constraints on specific DB's?

[DanielHeath]

Ahh - I only ran the default target locally. Will do.

[DanielHeath]

@brendon looks like the only remaining issues are caused by unrelated new gem releases that need locking down.

[DanielHeath]

@brendon I've fixed the gem dependency issues as well, which should get the build passing on mainline again.

[DanielHeath]

Anything else still to do on this?

[brendon]

Hi @DanielHeath, thank you so much for your work on this. I'm returning to work next week and endeavour to go through all my waiting PR's etc... I'll get back to you on it then. Have a great weekend! :D

[DanielHeath]

Hi @brendon no worries. Hope you had a great break :)

[DanielHeath]

RE 'there's clearly code intended to insert new records at the right position':

Prior to my changes,insert_at_position checks new_record? and calls set_list_position (which saves the record) if it's true.

The issue is that this does not work if you have a uniqueness constraint on postgres.

[DanielHeath]

I think we also need more tests for this one just to be sure it's working in all cases.

Were there particular cases / classes of cases you were concerned about? It looked to me like there's pretty good coverage of this area, but if there's anything missing I'm happy to add it.

[brendon]

I think we also need more tests for this one just to be sure it's working in all cases.

Were there particular cases / classes of cases you were concerned about? It looked to me like there's pretty good coverage of this area, but if there's anything missing I'm happy to add it.

Probably at least testing the shuffle_to method, especially around how it returns out for empty lists etc... There's a lot happening in there :)

[DanielHeath]

What do you think of these tests?

[brendon]

Hi @DanielHeath, sorry about the delay, those tests look good. Certainly good to exercise the code a bit more :) I'm bringing in @swanandp on this for a look too if you don't mind. @swanandp, let me know your thoughts :)

[DanielHeath]

@swanandp sorry for the radio silence, I've been unwell.

Will take a look at the failing tests today.

[DanielHeath]

OK, sorted out the tests and also incidentally fixed the issue where you'd get 'gaps' and duplicate values under concurrency (for postgres & mysql, at least)

[brendon]

Interesting approach @DanielHeath :) @swanandp, would you be able to take a look at this? :)

[DanielHeath]

@swanandp @brendon anything still needed on this one?

[brendon]

I think if @swanandp could take another look that'd be cool :)

[swanandp]

Cool, I'll check it out.

On Fri, 9 Aug 2019 at 4:18 AM, Brendon Muir notifications@github.com wrote:

I think if @swanandp https://github.com/swanandp could take another look that'd be cool :)

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/swanandp/acts_as_list/pull/334?email_source=notifications&email_token=AAAWGGFGMEXC4VFTSARP4ELQDSPFTA5CNFSM4GLVEWKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD35DQTI#issuecomment-519714893, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAWGGCVCV63KXLOAAX4YDLQDSPFTANCNFSM4GLVEWKA .

[DanielHeath]

Thanks @swanandp :)

[DanielHeath]

@brendon @swanandp is there anything still blocking this?

[brendon]

Sorry @DanielHeath, I'm struggling with the size and complexity of the change and I'm worried that it will have unintended side-effects for others of I don't go through it thoroughly and make sure I'm happy with it. I've been quite stretched for time lately in that regard. I was hoping @swanandp would have a bit of time to take a look and comment on whether the PR fits in with the general direction and conventions of the project but I think he's also really busy and not involved in the Rails world anymore. He's actually handing the project off to me. I'll set aside a couple of hours on Monday to take a look at this though.

[DanielHeath]

Hi @brendon

The requested changes are a fair bit of work after already working through 50+ comments.

I'll see when I can get to them, but as running this fork in production has fixed the performance issues I was having, it'll be hard to prioritize this, especially without some assurance that it'll ever get merged.

• Daniel

[brendon]

Thanks @DanielHeath, I totally understand. Just when you get the time. It might be easier for me to assemble a change based on this one that just focuses on the swapping and not any of the ancillary changes in style etc... If I get the time I'll let you know.

[brendon]

Hi @DanielHeath, I spent a few hours trying to work through this code last night and I'm afraid to say I'm still finding it very difficult to understand. I think for this to progress, new PR's will need to be done that isolate each change with a good explanation of what's going on. The shuffle_to method I find very hard to grok and I'm also not quite sure what it's solving (looking back through the previous messages).

I know it's not the outcome you were hoping for, but I have to weight all this up with the expectations of the other users of this gem.

[brendon]

Hi @DanielHeath, just cleaning up old PR's. Sad to see hard work wasted but I think things may have moved on a bit here and it might be better to start again on this one if you still need to. I've since released an alternative project that might suit your needs better: https://github.com/brendon/positioning