Allow storing static preferences using string class names

[elia]

Summary

This will make unnecessary to wrap these definitions under .to_prepare blocks.

Additionally the error message for unexpected keys has improved and now lists both all the unexpected keys along with the expected keys, mentioning the name associated to the definition.

The only thing that changed is that it doesn't do so when the configuration is added but rather when it's fetched.

Completes #4070 and addresses the original request of #4040.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

• I have written a thorough PR description.
• I have kept my commits small and atomic.
• I have used clear, explanatory commit messages.

The following are not always needed (cross them out if they are not):

• I have added automated tests to cover my changes.
• I have attached screenshots to demo visual changes.
• I have opened a PR to update the guides.
• I have updated the README to account for my changes.

[codecov]

Codecov Report

Merging #4858 (8281f98) into master (c6050d7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4858   +/-   ##
=======================================
  Coverage   86.23%   86.24%           
=======================================
  Files         578      578           
  Lines       14674    14683    +9     
=======================================
+ Hits        12654    12663    +9     
  Misses       2020     2020           

Impacted Files	Coverage Δ
core/lib/spree/core/engine.rb	88.67% <100.00%> (+0.21%)	⬆️
.../lib/spree/preferences/static_model_preferences.rb	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[waiting-for-dev]

Thanks for working on it, @elia. That's something that needs to be addressed 🙌

However, did you happen to check if we will still render the error message on initialization, or otherwise, will it happen only for affected runtimes? Keeping the first behavior is optimal, as it saves from more headaches on production. If that changes, we could work around that by having one #to_prepare block on our engine doing the sanitization check for all configured preferences.

What do you think?

[waiting-for-dev]

I do think it's cleaner to create the Hash with a fresh hash as the default value, for a few reasons:

• Otherwise, we need to sanitize it every time we access a key, as we're doing now twice in #add & #fetch.
• Could break something in edge cases where code relies on returning a Hash.
• In my view, it communicates better as it nails down the data structure we need.

Thoughts?

[elia]

The reason I touched this is that while storing prefs for solidus_stripe I noticed that trying a non existing key would add the key to the hash, e.g.:

>> sp = Spree::Preferences::StaticModelPreferences.new
=> #<Spree::Preferences::StaticModelPreferences:0x00000001124dac70 @store={}>
>> sp.for_class("Foo")
=> {}
>> sp
=> 
#<Spree::Preferences::StaticModelPreferences:0x00000001124dac70
 @store={"Foo"=>{}}>          
>> 

I'll change it to a simpler Hash.new { {} } so it behaves this way:

>> h = Hash.new { {} }
=> {}
>> h[:asdf]
=> {}
>> h
=> {}
>> h[:asdf] = 1234
=> 1234
>> h
=> {:asdf=>1234}
>> 

[elia]

I had to walk back on this, my second example wasn't correct, and after all we only need to set the empty hash in #add and fallback to a default in #for_class which is not much.

The whole preferences system could probably be simplified but didn't want to launch into a campaign, and this will at least provide a consistent the experience within initializers.

[waiting-for-dev]

Hmm, I see. I never realized that Ruby had this behavior. What about the change in the API (i.e., returning nil instead of Hash? Do you think it's plausible it could break stores? cc @kennyadsl

[elia]

I'd say that the whole API a bit sparse and could be streamlined, it also only serves one model inside solidus (although it's probably more across the ecosystem). But I wanted to surgically solve only the problem of not being able to pass strings for class names and finally make it consistent with other parts of the configuration.

There's also probably a case for making some of these APIs to be private and so not susceptible to backward compatibility support.

[waiting-for-dev]

Hey, @elia, it looks nice, but what do you think about:

However, did you happen to check if we will still render the error message on initialization, or otherwise, will it happen only for affected runtimes? Keeping the first behavior is optimal, as it saves from more headaches on production. If that changes, we could work around that by having one #to_prepare block on our engine doing the sanitization check for all configured preferences.

[elia]

However, did you happen to check if we will still render the error message on initialization, or otherwise, will it happen only for affected runtimes? Keeping the first behavior is optimal, as it saves from more headaches on production. If that changes, we could work around that by having one #to_prepare block on our engine doing the sanitization check for all configured preferences.

What do you think?

@waiting-for-dev sorry I though I addressed that! I updated the PR moving the check to a validate! method that is being run once inside an after_initializer. Since the static preferences are not reloaded I thought it's not strictly necessary to check at every code change.

[kennyadsl]

What if you set the preference value after the app is initialized? Do you need to call validate! manually as well?

[elia]

After the app is initialized you could change the model configuration (e.g. adding or removing a preference) not the static preference value, but I regarded that as infrequent.

If we want to do it at every request in development we could move it to a to_prepare block, would you prefer that?

[kennyadsl]

You are right, they live in memory and we can't change their value at runtime. At least, not easily and it's not an expected usage.

[waiting-for-dev]

❤️

[github-actions]

💔 Some backports could not be created

Status	Branch	Result
✅	v3.2
❌	v3.1	An unhandled error occurred. Please see the logs for details
❌	v3.3	Cherrypick failed because the selected commit (8281f98) is empty. Did you already backport this commit?

Manual backport

To create the backport manually run:

backport --pr 4858

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

[github-actions]

💔 Some backports could not be created

Status	Branch	Result
✅	v3.2
❌	v3.1	An unhandled error occurred. Please see the logs for details
❌	v3.3	Cherrypick failed because the selected commit (8281f98) is empty. Did you already backport this commit?

Manual backport

To create the backport manually run:

backport --pr 4858

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

[github-actions]

💔 Some backports could not be created

Status	Branch	Result
✅	v3.2
❌	v3.1	An unhandled error occurred. Please see the logs for details
❌	v3.3	Cherrypick failed because the selected commit (8281f98) is empty. Did you already backport this commit?

Manual backport

To create the backport manually run:

backport --pr 4858

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details