Use unscoped blocks instead of chaining

[brendon]

Chaining unscoped with further scopes negates the unscoped call as per
the documentation: http://apidock.com/rails/ActiveRecord/Base/unscoped/class

Issue: #120

[brendon]

My testing of this in my app confirms it's now working properly.

[swanandp]

There are loads of Rails 4 errors in the test suite. Can you please look into them ?

[brendon]

It's failing because of another bug: #119. Do you want to merge that pull request in or do you want that fixed as part of this pull request?

[brendon]

Ok, this is messy, I'm getting random test failures. Sometimes I get a full passing suite, other times lots of failures. I think something fishy is going on with the database cleaning here? With regards to #119 you can probably do: self.class.connection.quote() instead if you don't want to worry about passing in the column. One of the rails tests on the pull request that made this change was changed to use that method (obviously when a column isn't available).

Here's my gems list if it helps:

*** LOCAL GEMS ***

activemodel (4.1.1)
activerecord (4.1.1)
activesupport (4.1.1)
arel (5.0.1.20140414130214)
bigdecimal (1.1.0)
builder (3.2.2)
bundler (1.6.2)
bundler-unload (1.0.2)
daemon_controller (1.2.0)
executable-hooks (1.3.1)
gem-wrappers (1.2.4)
i18n (0.6.9)
io-console (0.3)
json (1.8.1, 1.5.5)
minitest (5.3.4, 2.5.1)
passenger (4.0.42)
rack (1.5.2)
rake (10.3.2, 0.9.2.2)
rdoc (3.9.5)
rubygems-bundler (1.4.3)
rvm (1.11.3.9)
sqlite3 (1.3.9)
thread_safe (0.3.3)
tzinfo (1.1.0)

Running 1.9.3p545.

[brendon]

Ok, I've fixed a couple of bugs in the tests, but there is still something funky going on with Rails 4. I suspect some kind of caching in the connection? It randomly returns different results (sometimes everything passes, sometimes things are out of order). I think the acts_as_list code itself is fine, it's down at the connection layer that things are going awry. I've spent a morning trying things (connection pool to 1, forcing only one connection, only creating the tables once at the start of the test and then truncating them using database_cleaner. Nothing works. Maybe there is a bug with the sqlite memory interface?

[brendon]

Ok, I've spent waaaaay too long on this! I hope you don't mind me bundling these clarifications in, but changing scopes wasn't being handled properly when it came to keeping the position values clean between scopes. If you moved an item from a scope that had many items to a scope that had fewer, the position wouldn't be updated to be the next integer at the bottom of the list, it'd remain at its large value.

I've updated the code to handle this situation better. You also had a broken test that only passed because you moved an item that would have had the same position in both scopes (co-incidence). I think there are few more tests like this in the suite - fortunately too because one failing test taught me that one can't rely on the dirty functionality of active record if the position is explicitly set to what it already was.

Anyway, all the tests pass in Rails 3. I did't write any new tests as there doesn't seem to be anything new to test.

With regards to the failures in Rails 4, It looks like the tests aren't being executed in the same order each time. Perhaps some assumption in the test suits is causing them to fail in this case. Maybe the included shared tests aren't being included in the same order each time?

[brendon]

I've done some extra testing. Rails 4 seems to cause these tests to fail on occasion due to the testing of the default column value. Re-creating the table with or without the default value doesn't seem to be enough to clear whatever cache exists, and so sometimes records that should be created without a default value on the position column, get a default value of 1. That's not even what the default value is set to in the table definition, but that's what's happening. I'd appreciate your help on this if you wouldn't mind. :)

[swanandp]

Sorry for not responding yet, I've been out of action for over a week now.
Will certainly look into this in the next few days.

On Thu, May 29, 2014 at 12:31 AM, Brendon Muir notifications@github.comwrote:

I've done some extra testing. Rails 4 seems to cause these tests to fail
on occasion due to the testing of the default column value. Re-creating the
table with or without the default value doesn't seem to be enough to clear
whatever cache exists, and so sometimes records that should be created
without a default value on the position column, get a default value of 1.
That's not even what the default value is set to in the table definition,
but that's what's happening. I'd appreciate your help on this if you
wouldn't mind. :)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/121#issuecomment-44450406
.

[brendon]

Thanks for that. I've found the main problem is that after changing the table (by removing and recreating it with the modified default value, we're not calling reset_column_information on the model, thus it still has the old default value. This is find when the tests are executed where the non-default table is the first one created as then all the tests pass, if the order is reversed, it exposes flaws in the tests that were probably always broken but because earlier versions of MiniTest don't seem to randomise the tests, the problems were never discovered. The failing tests seem to have to do with the before_validation that sets the position to be the acts_as_list_top value if it's less than that value (this is the case when it's defaulting to 0. Then there is further broken logic changing the position again.

[brendon]

Ok, the tests pass now. Not sure what the rbx stuff is all about, but Rails 3 and 4 with MRI pass fine. As you can see from the latest commit, there was a bug in the code relating to the highest position ceiling and default column values. I've also forced Minitest 5 which was what the Rails 4 tests were using (which is why the Rails 3 tests passed). I think they changed the way test ordering was decided between 4 and 5. Calling reset_column_information on all of the models now means the tests operate as intended. I'm not sure how long it has all been broken, so it might pay to check the tests to make sure there haven't been any assumptions made.

[brendon]

Hi Swanand, are you able to look at merging this in? It passes all the tests and even fixes some bugs that currently existing master. Let me know if you have any questions.

[swanandp]

ok, trying this now.

[brendon]

Thanks Swanand :)

[swanandp]

@brendon : This seems a bit suspicious. Why would we need to change the order ?

[brendon]

Hi Swanand, this is because moving a node into a new scope should add the node to the end of the list unless the position is explicitly specified in the update. Seems like the most reasonable expectation :)

[swanandp]

This is great, thanks @brendon for the hard work

[swanandp]

I added you as a collaborator on the repo.

[brendon]

Thanks Swanand :) Glad to be able to help!

[brendon]

Would it be possible to have a new gem release now that #119 is able to be closed?