search

brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.05k stars 356 forks source link

Add composite primary key support #430

Closed divagueame closed 5 months ago

divagueame commented 6 months ago

This PR adds support for Composite primary key which is supported from Rails 7.1 This was requested in this issue https://github.com/brendon/acts_as_list/issues/417

Locally I had to run bundle exec appraisal install to generate _rails_7_1.gemfile_ to run the tests. I didn't include it in the PR since I am not sure if it' s needed/left out for any particular reason/be dependent on my local environment. Any feedback is welcome.

divagueame commented 5 months ago

This is all looking really good. Are you able to give me a quick overview of the tests? I've tried to quickly understand them but there's quite a lot of change there. Here's what I think you've done:

  • You've modified the SharedList tests to be able to cope with an iteration where the keys are composite. It looks like you just set both keys to be the same integer each time for convenience?
  • You've kept the second dedicated suite of tests from the original PR.

Let me know if I got it wrong :)

I think you got quite right. I adjusted shared_list, so that it can be reused with a model different than ListMixin,

test/shared_list.rb

def setup(mixin = ListMixin, mixinError = ListMixinError, id = :id)
...

It can be now used overriding the default Model (ListMixin) used for those test runs, like this:

test/test_list.rb

    def setup
      setup_db

      super(CompositePrimaryKeyListScoped, CompositePrimaryKeyListScopedError, :first_id)
    end

On those iterations, :first_id and :second_id will be the same for the sake of simplicity.

I left the tests of the original PR in which the model does not use scoped lists. I guess it could be possible to refactor Shared::List to account for those cases, however it would need to be further refactored adding more abstraction. I tried to test the main documented APIs from the Readme, so now I'm thinking that maybe it could be improved. Not sure. Let me know what you think about it.

brendon commented 5 months ago

I think as long as you're running the shared tests with composite keys and have covered off anything unique to composite key tests in the other suite then we're good to go. Let me know if you're happy with that decision and I'll merge :)

Do you need an immediate new version released?

divagueame commented 5 months ago

I think it's good like it is. I would say let's merge it like that. I don't personally need a version released immediately. Just do it as usual. Not really sure how that works : )

brendon commented 5 months ago

All good. The project only seldom received updates so I'll release a new version. Are you able to add a changelog entry for me in the Unreleased section?

divagueame commented 5 months ago

Sure, just added it to the changelog.

brendon commented 5 months ago

Thanks @divagueame, this is released as 1.2.0. Looking forward to working with you on this project again in the future.

brendon commented 3 months ago

Hi @divagueame, just a heads up, in the process of adding this feature to my positioning gem I realised that Rails handles a lot of the overhead of composite primary keys for us so the implementation here can be simplified. I've committed some changes to master if you're interested :)

divagueame commented 3 months ago

Sweet! That looks neat. Thanks for the update. : )