Freeze preferences for Backend, Frontend and Api specs as well

[kennyadsl]

Description

This PR completes the work started with #3220.

It freezes preferences for sub gems as well, also allowing to use stub_spree_preferences into stores and extensions that define their own configuration classes, like Spree::Auth::Config in solidus_auth_devise for example.

I plan to backport this into 2.9.x because I think this change is mandatory to be able to correctly stub preferences (instead of resetting them) into extensions.

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
  - [ ] I have updated Guides and README accordingly to this change (if needed)
  - [ ] I have added tests to cover this change (if needed)

[kennyadsl]

I'd like not to define a module-level method here, but I still can't find a better way to do this since I need to use this method into the conf.before block. Any idea?

[ericsaupe]

I'm OK with this. It makes the idea of name spacing configs per extension easy to use and understand.

[spaghetticode]

@kennyadsl I feel the double hash syntax is quite cumbersome:

stub_spree_preferences({ requires_authentication: false }, preference_store_class: Spree::Api::Config)

What about something simpler? Something like:

stub_spree_preferences(Spree::Api::Config, requires_authentication: false)

still with the first parameter optional?

I know the method definition becomes quite ugly and does not feel good, but maybe we should favor the simpler interface over the implementation code? 🤔

def stub_spree_preferences(prefs_or_conf_class, prefs=nil)
  if Hash === prefs_or_conf_class
    preference_store_class = Spree::Config
    preferences = prefs_or_conf_class
  else
    preference_store_class = prefs_or_conf_class
    preferences = prefs
  end
  # remaining code here
end

[kennyadsl]

@spaghetticode thanks for the feedback. I think having a cleaner interface is something is a great concern. I think your solution is better but I'd love to find a way to have a consistent interface.

Another option could be always passing the preferences store class to the method. We'll have to specify the class for Spree::Config as well:

stub_spree_preferences(Spree::Config, requires_authentication: false)

More code but less implicit. To solve writing more useless code (we use Spree::Config most of the times), we could be adding some helpers like:

stub_core_preferences(hash)
stub_api_preferences(hash)
stub_backend_preferences(hash)
stub_fronted_preferences(hash)

If we meta-program them, this will also allow the helpers to work into extensions and stores that define their own config classes like:

# if Spree::Auth::Config exists
stub_auth_preferences(hash)

What're your thoughts on this?

[spaghetticode]

Regarding always passing the config class all the times:

stub_spree_preferences(Spree::Config, currency: 'EUR')

I kinda prefer the previous implementation, when one could avoid it and consider Spree::Api::Config a sensible default. It's shorter, and the existing code calls to stub_spree_preferences don't need to be changed.

Regarding the second solution:

stub_core_preferences(hash)
stub_api_preferences(hash)
...

I like it, but I think it has some disadvantage, as the new interface increases the cognitive effort for the developer... there's more cognitive distance between the spree config classes and their stub method names.

With stub_spree_preferences you need to remember one single method name, and know that you need to pass the preferences class name when it's different than Spree::Api::Config.

This means that, if I see in the application codebase:

Spree::Api::Config[:requires_authentication] = false 

then in order to stub it, quite mechanically I can derive this code:

stub_spree_preferences Spree::Api::Config, requires_authentication: false

With the new methods, you need to remember there's a different one for each config class, and you may have trouble in guessing the right method name.

If one knows stub_api_preferences works for Spree::Api::Config, then he may think that the method for Spree::Config is simply stub_preferences, but that's wrong, as these method names derive from the Solidus section that uses that preference, not from the preference class. So in this case the right name is stub_core_preferences.

Specularly, if you change the method names to derive from the config class, then one may wrongly assume they come from the Solidus section, and then be wrong again.

On the other hand, I really like the idea of being able to stub extension preferences without writing any code, for example having stub_auth_preferences(hash) working out of the box.

[kennyadsl]

@spaghetticode I've made the proposed changes.

I also changed the TestingSupport::Preferences module a little bit to automatically freeze all the Solidus configuration classes when included (and those classes are defined). This way we'll have the core/backend/frontend/api preferences frozen when including this module into extensions and stores specs.

[spaghetticode]

@kennyadsl 💯 👍 thank you for moving this forward!

[ericsaupe]

LGTM!

[jacobherrington]

I'm a big fan of this change!