Use Spree::Base as models base class

[filippoliverani]

Description
Ref #3450

This PR introduces Spree::ApplicationRecord class and makes every ActiveRecord model previously inheriting from ActiveRecord::Base inherit from it.
To leverage the new hierarchy, an existing Spree::Core::Permalinks monkey patch on ActiveRecord::Base has been moved to Spree::ApplicationRecord.

This way Solidus is more compliant with Rails 5+ recommendations and the risk of unwanted side effects on ActiveRecord::Base is mitigated.

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message

[tvdeyen]

This is a good change, thanks 👏

But, I recommend to name not name this class ApplicationRecord even though it is namespaced it potentially conflicts with the top level ApplicationRecord new Rails apps have.

What about Spree::BaseRecord? Also, we already have Spree::Base, right? What do we do about it?

[filippoliverani]

@tvdeyen Thank you for the quick review 🙇

This is a good change, thanks 👏

But, I recommend to name not name this class ApplicationRecord even though it is namespaced it potentially conflicts with the top level ApplicationRecord new Rails apps have.

What about Spree::BaseRecord? Also, we already have Spree::Base, right?
What do we do about it?

I think that is important to keep ApplicationRecord name because its has become a Rails convention, app and engine generators since Rails 5 create this class and almost everybody expects this name.
If we are not sure that namespacing is not enough we could use the require_dependency trick suggested here

[kennyadsl]

Thanks, @filippoliverani

[kennyadsl]

@filippoliverani we discussed this PR during the last core team meeting. We think that probably it's enough to just use Spree::Base as our ApplicationRecord, which is doing the same thing and it's an abstract class as well.

It has a different naming convention than what Rails users expect though but maybe changing the name of that class to Spree::ApplicationRecord is not worth it.

We propose to keep the rest of the changes (using it for models that were inheriting from ActiveRecord::Base directly + move the monkey patch into Spree::Base) and get rid of the ApplicationRecord file at all.

What do you think?

Re-considered my approval as is after a core team discussion

[filippoliverani]

@kennyadsl thank you for the update, the main point was to stop using ActiveRecord::Base directly, I will rearrange the code to reflect the preferred naming convention 👍

[kennyadsl]

Thanks @filippoliverani!

[tvdeyen]

Hmm. I am not sure if Spree::Base is the correct base class for all our database models. They now all have Ransack and Preferences included.

Is this what we want? Does this lead to potential issues?

🤔

[kennyadsl]

@tvdeyen Spree::Base is already the parent class for all our models. There were just a few that are inheriting from ActiveRecord::Base but doesn't seem to exist a specific reason for that.

[filippoliverani]

@tvdeyen There's a lot of stuff coming along with Spree::Base, not sure if that could be a problem. At this point is a trade off between having a new lean Spree::ApplicationRecord class or staying with an existing fat Spree::Base class.
Otherwise we could include Ransack and Preferences support as concerns only where needed.

[tvdeyen]

@kennyadsl @filippoliverani

Sorry, I wasn't so sure. Then it makes sense to use it for now. Next step could be to use a lean base class for models that do not need ransack and preferences.

[tvdeyen]

👏