acts_as_list_no_update

[randoum]

ping @swanandp @fabn

[randoum]

ping @brendon

[randoum]

@swanandp @fabn @brendon guys, question: are we going to work on this PR, or just ignore it for some time, like the previous one? Kindly let me know so I wait or I don't. Tell me to close it or let's work on it but don't just ignore it.

171 was open in 2015! Guys ...

[brendon]

Hi @randoum, here in New Zealand it's summer and this is our holiday period so I've not been in work mode for some weeks. I'm not ignoring you, just not working on anything right now. I had marked your message to follow up later.

The reason why I haven't moved to accept this PR on my own is that I don't own this project and this seems like a reasonably big feature. I'd rather @swanandp sign it off first.

@swanandp, looking at the latest code, it seems like a much cleaner solution. Can you give some input and then we can put this one to bed.

[swanandp]

Alright, let's go ahead with this one. We will need to push a new release.

[swanandp]

@brendon Please don't feel like you don't really own this repo, I am perfectly fine if you take decisions. Though, I do agree that on major additions like these, it's best if two people signed off.

[randoum]

Great guys, thanks for stepping in. Though I will need a bit of help to fix issues around postgres, cause I really don't know how to set-up a postgres environment and I'm kind of too busy to take time and try to figure it out.

[brendon]

Thanks @swanandp, that's very helpful :) I agree, a minor release would be appropriate.

@randoum, I didn't notice that the postgres tests had failed. The travis.yml file gives some clues on how we automate the test database creation on travis. All you need to do is install postgresql then create the database:

psql -c 'create database acts_as_list;' -U postgres

By default the postgres user shouldn't have a password. test/database.yml shows you the credentials required (none).

You might need to tweak this stuff locally to get the tests running. To execute the tests against postgres you just set the environment DB=postgresql rake.

I hope that helps.

Also have a look at the travis test runs. There are a lot of ruby warnings in there that need to be eliminated etc.. These won't show for you locally because ruby hides these by default.

[randoum]

Concerning the warnings, I see them alright. They are coming from spec lines assert_equal nil, value, and the warning tells us to use assert_nil value instead. I am always using assert_nil in my projects. But in this PR I just used the same coding style that was already there. See for example test/shared_no_addition.rb#test_insert which is using assert_equal nil, value

Bottom line, I propose to leave it like this for now, and then in a next PR work on refactoring the tests. I propose this cause 1) I don't want to touch other unrelated code, for clarity of this PR and 2) there are other improvement that can be done. Examples :

• I suggest to raise raise ActiveModel dependency to 3.2.1, then instead of using Model.all.map(&:id) we could use Model.pluck :id
• Instead of using Model.where(id: 2).first you should use Model.find 2

And so on, so perhaps all refactoring could be done in one single PR. What do you think ?

[randoum]

Also, I'll add : using shared_examples so failure messages can show clearly the context of failing tests in the logs

[randoum]

Tests fixed, the problem was just coming from the way PostgreSQL was sorting records

[brendon]

Thanks @randoum, just added a small note to one line.

I was perusing the tests which I hadn't done closely before and noticed how tests for this new feature are interspersed among existing test cases. My initial instinct is that it'd be easier to reason about the tests overall if this feature is tested in its own space/file. Can you explain why you've done it this way? :)

[brendon]

I agree with your idea to fix those warnings in a different PR. Let's focus on getting this one merged :) Also, is there anything additional that RequestStore does that we should be doing with regards to Thread?

[randoum]

My initial instinct is that it'd be easier to reason about the tests overall if this feature is tested in its own space/file. Can you explain why you've done it this way? :)

Because the existing tests was more integration than unit test, i.e you are running a small scenarios where you test records interactions between each others. Example test_insert_at where you do multiple create then few insert_at and check the order of all records at the end.

So I follow the same pattern, and integrated acts_as_list_no_update in the middle, which has 2 advantages : 1) make sure acts_as_list_no_update works, and 2) make sure that acts_as_list knows how to deal with a mix of in-list / not-in-list records (there where no such scenario before).

And yes, it would be good to have unit tests AND integration tests, which is not the case now because we only have integration tests (I means kind-of-integration test)

Is there anything additional that RequestStore does that we should be doing with regards to Thread?

Not in our case. RequestStore is useful if you call Thread variables at different places of your code. Here we call a Thread variable in one single location and clean it in a ensure block, so we are safe with Thread variable, no need for an additional dependency.

[randoum]

Thanks @randoum, just added a small note to one line.

Done

[brendon]

@randoum, looks like Rails has no support for NULLS FIRST etc... so back out that last commit. Sorry for the rigmarole. Also, if master works for you with the broken loading order, merge that in and I'll sign this one off :)

[brendon]

RE your comments on testing. That makes sense, and thank you for documenting that here for future use :)

[randoum]

Nah it was my fault, though all tests was green on my machine. This last commit should pass on Travis too

[brendon]

Thanks @randoum, all merged now :) Thank you for your commitment!!

[randoum]

Awesome