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

Unscope `select` to avoid PG::UndefinedFunction #283

Closed donv closed 6 years ago

donv commented 7 years ago

Having a default_scope with a select breaks since acts_as_list 0.9.5. The error type is discussed here:

https://github.com/rails/rails/issues/19146

Unscoping the select scope fixes the problem.

swanandp commented 7 years ago

@donv Thanks for contributing. Can you please add a failing test case to demonstrate the problem?

brendon commented 7 years ago

Hi @donv, should we be using count(:all) instead?

donv commented 7 years ago

Yes, using count(:all) is probably a better solution, but it was a bigger change, so I tried for the smallest change I could see 😄 .

brendon commented 7 years ago

Thanks @donv, I think count(:all) is probably the best way forward. What do you think @swanandp?

If you could switch to that method and provide a failing test that would be great :D

brendon commented 7 years ago

Hi @donv, would you mind doing that fix with the test? :)

donv commented 7 years ago

Hi @brendon !

No, I don't mind 😄 . Will put it on my plan for next week or the week after. Lots of things to do 😅 .

brendon commented 7 years ago

Thanks @donv, we really appreciate it :)

brendon commented 7 years ago

@donv: ping! :D

donv commented 7 years ago

Thanks for the ping. My plan has slipped :) Will get to it.

donv commented 6 years ago

Starting on this now.

donv commented 6 years ago

I have added a failing test that becomes green with the current change.

donv commented 6 years ago

I am trying to implement the count(:all) idea we had, but it proves complex since we expect to have the position_column available in many places. Having a default scope with a select results in the position column not being present in the resulting records.

I have tried a few ways to add the position_column to the query, but then we mess up the default select of all columns.

I now think my original idea is the best, to just unscope :select.

What do you think?

brendon commented 6 years ago

Unscoping a default :select seems like an ok solution. As you say, it's unworkable not to have the position column selected. No damage can be done by unscoping the :select? There were side effects to unscoping a default :order which is why we now only unscope the :where.

@swanandp, did you have any feedback? :)

brendon commented 6 years ago

@donv, someone has proposed the unscope :select solution in a seperate pull request so I think we'll merge this one. Can you update from master just so we can get a green suite then we should be good to go :)

donv commented 6 years ago

@brendon Rebased and green tests.

brendon commented 6 years ago

Thanks @donv, I'm happy with this. I'll release a new gem for you.