Fix call context when a preference default is a proc #4721

Roddoric · 2022-11-16T16:45:42Z

Summary

Before v3.1, the proc given as the default value in a preference had its context bound to the initialized instance. That allowed, for example, referencing other preferences.

From v3.1., the context is bound to the class.

We are coming back to the behavior by calling the proc with instance_exec

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 · 2022-11-16T17:14:36Z

Codecov Report

Merging #4721 (bf9748c) into master (0cae150) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head bf9748c differs from pull request most recent head 193e86d. Consider uploading reports for the commit 193e86d to get more accurate results

@@           Coverage Diff           @@
##           master    #4721   +/-   ##
=======================================
  Coverage   86.13%   86.13%           
=======================================
  Files         578      578           
  Lines       14651    14651           
=======================================
+ Hits        12619    12620    +1     
+ Misses       2032     2031    -1

Impacted Files	Coverage Δ
core/lib/spree/preferences/preferable.rb	`94.44% <ø> (ø)`
.../lib/spree/preferences/preferable_class_methods.rb	`100.00% <100.00%> (+2.04%)`	⬆️

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

waiting-for-dev

Thanks!! ❤️

kennyadsl

Thanks, this is great! I left a question about the testing approach, let me know! ❤️

core/spec/models/spree/preferences/preferable_spec.rb

Roddoric · 2022-11-17T15:44:44Z

I changed the test #preferences from

      def preferences
         @preferences ||= default_preferences
       end

to

      def preferences
         @preferences ||= {}
       end

       def initialize_preference_defaults
         @preferences = default_preferences.merge(preferences)
       end

Because #default_preferences will enter an infinite loop trying to evaluate the value of preferred_context
( default_preferences calls preferences which calls default_preferences...)

Any thoughts about it ?
Are the comments clear and should they stay ? (The commit message got updated to)

waiting-for-dev · 2022-11-18T04:46:55Z

Thanks, @Roddoric. We definitely need to understand better what's going on here 🤔

We need better understanding

Roddoric · 2022-11-18T08:32:17Z

I think I understood what's going on
Tell me if I can help you in anyway

Roddoric · 2022-11-22T16:21:32Z

Hello @waiting-for-dev @kennyadsl
Did you have some time to check the last changes and the comment ?

core/lib/spree/preferences/preferable.rb

waiting-for-dev · 2022-11-23T15:21:30Z

Hello @waiting-for-dev @kennyadsl Did you have some time to check the last changes and the comment ?

Thanks, @Roddoric. I answered in the comment.

core/spec/models/spree/preferences/preferable_spec.rb

Roddoric · 2022-11-30T14:55:47Z

Sorry for the delay...

@waiting-for-dev it should be fine now, all tests pass in local but I don't see the CI triggering

waiting-for-dev · 2022-11-30T15:17:49Z

Thanks, @Roddoric. Last ask, could you squash all the commits? 🙏

Yeah, we have some issues with CI. Once it's fixed we can go ahead.

waiting-for-dev · 2022-12-02T05:20:40Z

@Roddoric, I know I said last ask, but as any good movie there's still a VERY last scene 😅 Could you please rebase from master to trigger CI?

Before v3.1, the proc given as the default value in a preference had its context bound to the initialized instance. That allowed, for example, referencing other preferences. From v3.1., the context is bound to the class. We are coming back to the behavior by calling the proc with `instance_exec` The test changed because initializing `preferences` with `default_preferences` cause an infinite loop. This is not a regression as it's the expected behavior on anterior versions but added some information on the comments. Change Preference Context: - `class A` is declared once in a `before(:all)` so the class WILL NOT be destroyed/recreate for each test - `@a = A.new` is in a `before(:each)` therefore the variable is initialized before the test start - The 4 examples from l.285 define the same preference `:secret` but the last one is the only using a default If the test run first: - `@a` is instantiated without `preference :secret` - `@a.preferences` looks like `{:color=>"green"}` - We define the `preference :secret` with a default - Calling `@a.get_preference(:secret)` will work because it will look for the default If the test doesn't run first: - `@a` is instantiated with a `preference :secret` without `:default` - `@a.preferences` looks like `{:color=>"green", :secret=>"rJ0s--jPH2d2iyyB9KseLU--HbYfXFtRB36+yj9L3fSDFA=="} ` (`:secret` value is `nil` but encrypted) - We override `preference :secret` with a default - `@a.get_preference(:secret)` returns `nil` as it's initialized and does not look for the default Setting `@a = A.new` after the override works because it'll be initialized with the new default To fix it we declared the class as anonymous and create them only when needed.

Roddoric · 2022-12-02T14:04:51Z

I think it did not work 😅

waiting-for-dev

Thanks! ❤️

github-actions · 2022-12-07T05:42:15Z

💚 All backports created successfully

Status	Branch	Result
✅	v3.2
✅	v3.1

Questions ?

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

github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Nov 16, 2022

Roddoric force-pushed the fix-context-for-proc-in-preferences branch from 867b550 to fc02e88 Compare November 16, 2022 16:46

waiting-for-dev previously approved these changes Nov 17, 2022

View reviewed changes

waiting-for-dev added Needs Backport type:bug Error, flaw or fault labels Nov 17, 2022

kennyadsl reviewed Nov 17, 2022

View reviewed changes

core/spec/models/spree/preferences/preferable_spec.rb Outdated Show resolved Hide resolved

Roddoric force-pushed the fix-context-for-proc-in-preferences branch from fc02e88 to ea53bc9 Compare November 17, 2022 15:36

Roddoric commented Nov 17, 2022

View reviewed changes

core/spec/models/spree/preferences/preferable_spec.rb Outdated Show resolved Hide resolved

Roddoric requested review from waiting-for-dev and kennyadsl and removed request for waiting-for-dev and kennyadsl November 17, 2022 15:45

Roddoric force-pushed the fix-context-for-proc-in-preferences branch from ea53bc9 to 6c17a48 Compare November 17, 2022 16:06

tvdeyen reviewed Nov 23, 2022

View reviewed changes

core/lib/spree/preferences/preferable.rb Outdated Show resolved Hide resolved

waiting-for-dev suggested changes Nov 25, 2022

View reviewed changes

core/spec/models/spree/preferences/preferable_spec.rb Outdated Show resolved Hide resolved

Roddoric force-pushed the fix-context-for-proc-in-preferences branch 2 times, most recently from 71132e5 to 686049f Compare November 30, 2022 14:45

Roddoric requested a review from waiting-for-dev November 30, 2022 14:49

Roddoric force-pushed the fix-context-for-proc-in-preferences branch from 686049f to f402c08 Compare December 1, 2022 09:13

Roddoric requested review from kennyadsl and waiting-for-dev and removed request for waiting-for-dev and kennyadsl December 1, 2022 09:14

Roddoric force-pushed the fix-context-for-proc-in-preferences branch from f402c08 to 67bd03d Compare December 2, 2022 14:02

Roddoric requested a review from a team as a code owner December 2, 2022 14:02

jarednorman approved these changes Dec 5, 2022

View reviewed changes

waiting-for-dev removed the Needs Backport label Dec 7, 2022

waiting-for-dev approved these changes Dec 7, 2022

View reviewed changes

waiting-for-dev merged commit aa3bbed into solidusio:master Dec 7, 2022

This was referenced Dec 7, 2022

[v3.2] Fix call context when a preference default is a proc #4768

Merged

[v3.1] Fix call context when a preference default is a proc #4769

Merged

nvandoorn mentioned this pull request Dec 12, 2024

Context for a proc given as a preference default changed from v3.0 to v3.1 #4720

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix call context when a preference default is a proc #4721

Fix call context when a preference default is a proc #4721

Roddoric commented Nov 16, 2022 •

edited

Loading

codecov bot commented Nov 16, 2022 •

edited

Loading

waiting-for-dev left a comment

kennyadsl left a comment

Roddoric commented Nov 17, 2022 •

edited

Loading

waiting-for-dev commented Nov 18, 2022

Roddoric commented Nov 18, 2022

Roddoric commented Nov 22, 2022

waiting-for-dev commented Nov 23, 2022

Roddoric commented Nov 30, 2022

waiting-for-dev commented Nov 30, 2022

waiting-for-dev commented Dec 2, 2022

Roddoric commented Dec 2, 2022

waiting-for-dev left a comment

github-actions bot commented Dec 7, 2022

Fix call context when a preference default is a proc #4721

Fix call context when a preference default is a proc #4721

Conversation

Roddoric commented Nov 16, 2022 • edited Loading

Summary

Checklist

codecov bot commented Nov 16, 2022 • edited Loading

Codecov Report

waiting-for-dev left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

Roddoric commented Nov 17, 2022 • edited Loading

waiting-for-dev commented Nov 18, 2022

Roddoric commented Nov 18, 2022

Roddoric commented Nov 22, 2022

waiting-for-dev commented Nov 23, 2022

Roddoric commented Nov 30, 2022

waiting-for-dev commented Nov 30, 2022

waiting-for-dev commented Dec 2, 2022

Roddoric commented Dec 2, 2022

waiting-for-dev left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 7, 2022

💚 All backports created successfully

Questions ?

Roddoric commented Nov 16, 2022 •

edited

Loading

codecov bot commented Nov 16, 2022 •

edited

Loading

Roddoric commented Nov 17, 2022 •

edited

Loading