Ensure passwords are not subject to another order when storing#289
Ensure passwords are not subject to another order when storing#289dillonwelch merged 3 commits intodevise-security:masterfrom
Conversation
|
https://github.com/devise-security/devise-security/pull/289/checks?check_run_id=2135460923 looks like your test fails on this version combo. Fix that up and this is good to go. |
|
Is that a genuine test failure? Looks like only MongoID + Rails 4.2 + ruby 2.5, given it works on ruby 2.6 I'm inclined to believe it's a random failure not the new tests. |
|
So it does look genuine, as the test failure is on your new test. 1) Error:
TestPasswordArchivable#test_default_sort_orders_do_not_affect_archiving:
ArgumentError: wrong number of arguments (given 0, expected 1)
/home/runner/work/devise-security/devise-security/gemfiles/vendor/bundle/gems/mongoid-4.0.2/lib/mongoid/scopable.rb:90:in `default_scope'
/home/runner/work/devise-security/devise-security/test/test_password_archivable.rb:79:in `<class:OldPassword>'
/home/runner/work/devise-security/devise-security/test/test_password_archivable.rb:78:in `block in <class:TestPasswordArchivable>'The line of code that's failing: default_scope { order(created_at: :asc) }And here is the relevant method in mongoid 4.0.2: https://github.com/mongodb/mongoid/blob/v4.0.2/lib/mongoid/scopable.rb#L66 Based on the doc comment, my guess is that they want one of the following: default_scope where(active: true)
default_scope ->{ where(active: true) }It looks like this behavior was changed in this commit , which is included in versions 5+, which explains why this only failed on this one version. |
|
Oh wow I'm sorry to have caused you to go out of your way for that. You're totally right. |
|
Not a problem. Always happy to assist anyone who submits issues or PRs :) |
|
Now that we no longer support Rails 4.2, that failing test is not an issue and everything is 🟢 ! |
Replaced #286
For anyone using UUIDs as default in system, OR for anyone using a default sort order, use reorder rather than order to avoid chained orders from taking effect.
It's unclear if Mongodb uses reorder vs order but I assume it to be ActiveRecord compatible since none of the other tests exploded.