Add ApplicationRecord for customisations of models #7425
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7425 +/- ##
==========================================
+ Coverage 93.07% 93.08% +0.01%
==========================================
Files 633 634 +1
Lines 18144 18142 -2
==========================================
Hits 16888 16888
+ Misses 1256 1254 -2
Continue to review full report at Codecov.
|
Rails 5 introduced this new class to confine application-specific monkey patches to our models only, and not leak into other libraries using ActiveRecord::Base. https://bigbinary.com/blog/application-record-in-rails-5
We were also patching ActiveRecord::Relation for the `#find_by_param` methods but we are not using those any more. They were deprecated a while ago. We now use `find_by(permalink: ...)`.
| @@ -70,6 +62,3 @@ def save_permalink(permalink_value = to_param) | |||
| end | |||
| end | |||
| end | |||
|
|
|||
| ActiveRecord::Base.include(Spree::Core::Permalinks) | |||
| ActiveRecord::Relation.include(Spree::Core::Permalinks) | |||
There was a problem hiding this comment.
I wonder if not including it in ActiveRecord::Relation will have any consequences..?
There was a problem hiding this comment.
I wondered that, too. But I think that the reason for this were the find_by_param methods. Spree could do something like Product.in_stock.find_by_param. But we don't use those any more and I removed them. I couldn't find any usage of the other methods on scopes.
filipefurtad0
added
pr-staged-au
|
Hey @mkllnk , Thank you for the notes on what to test. I followed your suggestions, and verified:
I noticed this PR touches many files! So I thought I just run some quick additional tests, like:
Looks good and ready to go 👍 |
What? Why?
Rails 5 introduced this new class to confine application-specific monkey patches to our models only, and not leak into other libraries using ActiveRecord::Base.
For context: I was adding a new model for #6328 and noticed that Rails generated the
ApplicationRecordclass. I looked up the story behind it and found it quite useful.https://bigbinary.com/blog/application-record-in-rails-5
Bonus: 🔥 +78 −162 lines of code
What should we test?
Release notes
Changelog Category: Technical changes
We now use the Rails convention to inherit all models from ApplicationRecord. App-specific patches now apply only to ApplicationRecord instead of ActiveRecord::Base. This should lead to fewer unintended side effects.