Adds configuration flag for the deprecation of associate_user #3677
Conversation
| @@ -52,6 +52,8 @@ def current_order(options = {}) | |||
| end | |||
|
|
|||
| def associate_user | |||
| return if Spree::Config.manually_associate_user | |||
| warn "Order#associate_user is deprecated and will be removed in 2.11" | |||
There was a problem hiding this comment.
Great! All deprecation in Solidus are managed via Spree::Deprecation.warn (see examples around the code) and would also be better to have a spec ensuring the deprecation is displayed (there are numerous examples of that in spec/ as well).
core/lib/spree/app_configuration.rb
Outdated
| @@ -173,6 +173,10 @@ class AppConfiguration < Preferences::Configuration | |||
| # @return [String] Email address used as +From:+ field in transactional emails. | |||
| preference :mails_from, :string, default: '[email protected]' | |||
|
|
|||
| # @!attribute [rw] manually_associate_user | |||
| # @return [Boolean] manually call associate_user instead of relying on current_order | |||
| preference :manually_associate_user, :boolean, default: true | |||
There was a problem hiding this comment.
Can you add something to the pref name that makes it clear that the association is responsibility of the authentication system? Like manually_associate_user_in_authentication_extension, or even without the initial manually_.
There was a problem hiding this comment.
Also the default (contrary to what I initially said 😅) should be false, to maintain the current behavior.
|
@carlosep can you please review the commit message? It does not conform to the other ones on this project as it's very long and it does not read very nicely on github:
I would keep the title at around 60 chars and add a description body with some details. Thanks 🙏 |
carlosep
force-pushed
the
add_configuration_flag_deprecated_associated_user
branch
from
1712b3b to
7a3e461
Compare
|
@elia and @spaghetticode, all of the mentioned points were addressed, if something else is needed please reach out |
|
@carlosep great job! I see the CI is failing because it treats deprecations as errors, I'm not sure how to deal with such a pervasive one, maybe @kennyadsl or @spaghetticode have handled similar ones in the past |
|
@carlosep you can remove the deprecations (and consequently their errors) by overriding the default configuration value for the new preference in dummy_app.rb: Spree.config do |config|
# ...
config.use_combined_first_and_last_name_in_address = true
config.use_legacy_order_state_machine = false
# add your preference here:
config.associate_user_in_authentication_extension = true
endThis way, when the test suite runs the preference value will be set to |
A configuration flag was needed to start a cycle of deprecation of the Spree::Core::ControllerHelpers::Order#associate_user method. This method is deprecated because now this association is done immediately by the authentication system when needed.
carlosep
force-pushed
the
add_configuration_flag_deprecated_associated_user
branch
from
7a3e461 to
428617d
Compare
|
@spaghetticode, I added this configuration and still the CI broke. Is there some other place that requires this flag besides dummy_app.rb? |
|
Hi @elia, @carlosep @kennyadsl Is still actual to check CI errors, fix and merge this PR? |
|
@dborovsky yes, this needs to be done, and IIRC this PR just lack the proper boilerplate for a smooth deprecation, interested in picking this up? |
|
@elia yea, I want to help with this PR. |
|
That's great, if you have any question about the comments we left above just ask and I'll try to help 👍 |
|
@elia @spaghetticode please review my PR #4201 |
|
Closing in favor of #4201 |
Description
This Pull Request solves the issue #3559, and it is done after #3667, which was closed due to a better solution on this one.
Based on this discussion the
associate_usermethod is deprecated due to changes during the creation or building of an order viacurrent_orderwhere before the creation and association of an user were made in different places now it is done immediately. After discussing on the issue we came to the conclusion that a good solution would be to add a configuration flag and enter a deprecation cycle starting at 2.11. For that I added the flag:associate_user_in_authentication_extensionwhich will allow clients to safely move their customization onassociate_userChecklist: