Add configuration flag deprecated associated user #4201

dborovsky · 2021-11-07T12:49:56Z

Description
task - #3559

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)
I have attached screenshots to this PR for visual changes (if needed)

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.

…ed_user

…ttps:https://github.com/carlosep/solidus into add_configuration_flag_deprecated_associated_user � Conflicts: � core/lib/spree/testing_support/dummy_app.rb

core/lib/spree/app_configuration.rb

core/lib/spree/core/controller_helpers/order.rb

core/lib/spree/testing_support/dummy_app.rb

core/spec/spec_helper.rb

frontend/spec/spec_helper.rb

core/lib/spree/core/controller_helpers/order.rb

dborovsky · 2021-11-13T11:09:33Z

@waiting-for-dev @kennyadsl. check please my last commit. I removed also Spree::Deprecation.behavior = :stderr

In my local environment by default behavior is :stedrr and tests are true. But circle/ci return error. I see deprecation return ActiveSupport::DeprecationException. How I understood correct it is mean behavior assigned as :raise.

waiting-for-dev · 2021-11-15T08:39:30Z

core/lib/spree/app_configuration.rb

@@ -169,6 +169,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] associate_user_in_authentication_extension
+ # @return [Boolean] manually call associate_user instead of relying on current_order
+ preference :associate_user_in_authentication_extension, :boolean, default: by_version(false, "4.0.0" => true)


Suggested change

preference :associate_user_in_authentication_extension, :boolean, default: by_version(false, "4.0.0" => true)

preference :associate_user_in_authentication_extension, :boolean, default: by_version(false, "3.2.0.alpha" => true)

We're going to remove the setting in v4.0 completely. We want it to be false on 3.1, but true on master (v3.2.0.alpha) and onward (v3.2.0). When users update v3.1 to v3.2 with bin/rails g solidus:update, they will keep having it to false as it will be overridden by the generated initializer new_solidus_defaults.rb. Then, they'll adapt their app (if needed) and remove the override. Once done, they'll be using true (default in v3.2), and their app will be ready for the future update to v4.0, where we'll completely remove all references to :associate_user_in_authentication_extension.

Once done this change, no error should be raised on the CI.

waiting-for-dev · 2021-11-15T08:43:40Z

core/lib/generators/solidus/install/templates/config/initializers/spree.rb.tt

@@ -40,6 +40,8 @@ Spree.config do |config|
 # Template to use when rendering layout
 # config.layout = "spree/layouts/spree_application"

+ # manually call associate_user instead of relying on current_order
+ # config.associate_user_in_authentication_extension = false


As we're going to remove the setting in v4.0 altogether, I think it's better not to add it in the generated spree.rb for fresh installations. They should be using the new default, and there's no need for them to know anything about the old behavior that will be removed. Thoughts @kennyadsl ?

I still would like some thought about it. @kennyadsl, what are we usually doing in these cases?

I agree with @waiting-for-dev, I'd remove this as well.

waiting-for-dev · 2021-11-15T08:45:38Z

Thanks for your continued work on this, @dborovsky! I tried to explain better the deprecation path. I hope it's clearer now. I'm also pinging @kennyadsl for a minor question.

In my local environment by default behavior is :stedrr and tests are true. But circle/ci return error. I see deprecation return ActiveSupport::DeprecationException. How I understood correct it is mean behavior assigned as :raise.

Once my comment is addressed, the CI should be 💚

forkata · 2021-11-23T00:29:17Z

core/spec/lib/spree/core/controller_helpers/order_spec.rb

+ context "configuration associate_user_in_authentication_extension is false" do
+ before do
+ stub_spree_preferences(associate_user_in_authentication_extension: false)
+ end


I think we should move the above specs into this context since they will start failing once we update the version and also they are only relevant in the case where the value of this setting is false.

We could also add a test for the new behaviour which should not associate the user nor show a deprecation warning.

We could also add a test for the new behaviour which should not associate the user nor show a deprecation warning.

Agree

I think we should move the above specs into this context since they will start failing once we update the version and also they are only relevant in the case where the value of this setting is false.

What I've been doing is still to test that the deprecation warning is shown. As it's useful for users, I like to have it covered. We'd remove it alongside the setting on v4.0. But, in this case, mine is not a strong opinion, so I'd be good with any outcome.

dborovsky · 2021-11-27T23:27:53Z

@forkata @waiting-for-dev check please my solution, could you check the tests which are failed in CI/circleci.

waiting-for-dev · 2021-11-29T05:03:25Z

frontend/spec/controllers/spree/checkout_controller_spec.rb

@@ -54,6 +54,7 @@
 # The first step for checkout controller is address
 # Transitioning into this state first is required
 order.update_column(:state, "address")
+ stub_spree_preferences(associate_user_in_authentication_extension: false)


How is it that we need to override the new setting value here? Wasn't the method useless?

waiting-for-dev · 2021-11-29T05:04:18Z

@forkata @waiting-for-dev check please my solution, could you check the tests which are failed in CI/circlec

You need to rebase from master because of #4209

…ed_user

…oller tests

dborovsky · 2021-11-30T17:02:51Z

@forkata @waiting-for-dev If the method associate_user won't be used in checkout _controller than method authorize!(:edit, current_order, cookies.signed[:guest_token]) return
CanCan::AccessDenied: You are not authorized to access this page.

solidus/frontend/app/controllers/spree/checkout_controller.rb

Line 223 in 9dbcad1

def check_authorization

kennyadsl

Thanks for taking care of this. I left a comment on the approach, which I think it's not the right one. Let me know!

kennyadsl · 2021-12-01T16:23:48Z

core/lib/spree/core/controller_helpers/order.rb

@@ -37,6 +37,11 @@ def current_order(options = {})
 end

 def associate_user
+ return if Spree::Config.associate_user_in_authentication_extension


I think we should just stop calling this method from inside our core codebase instead of returning if the preference is true.

I can see at least a couple of occurrences:

solidus/frontend/app/controllers/spree/orders_controller.rb

Line 43 in e878076

associate_user

solidus/frontend/app/controllers/spree/checkout_controller.rb

Line 18 in e878076

before_action :associate_user

If it's useless, things should keep working as before.

I think adding a conditional on the value of the preference in multiple places may be slightly worse than just checking it in the method itself.

@forkata I believe @kennyadsl is suggesting not having an option at all. We can simply deprecate it and stop using it in core, then later remove it entirely.

That makes, sense, I misread the comment here 😅

@waiting-for-dev @forkata @kennyadsl Please read my comment above #4201 (comment)

forkata · 2021-12-01T23:42:49Z

core/lib/spree/core/controller_helpers/order.rb

@@ -37,6 +37,11 @@ def current_order(options = {})
 end

 def associate_user
+ return if Spree::Config.associate_user_in_authentication_extension


I think adding a conditional on the value of the preference in multiple places may be slightly worse than just checking it in the method itself.

forkata · 2021-12-01T23:43:33Z

frontend/spec/controllers/spree/checkout_controller_spec.rb

+ stub_spree_preferences(associate_user_in_authentication_extension: true)
+ end
+
+ it "should associate the order with a user" do


This should probably say "should not associate the order with a user".

forkata · 2021-12-01T23:46:47Z

frontend/spec/controllers/spree/checkout_controller_spec.rb

- order.update_column :user_id, nil
- expect(order).to receive(:associate_user!).with(user)
- get :edit
+ context "current behavior" do


It might be better if we say "with associate_user_in_authentication_extension enabled" as this behaviour will change with the version bump.

forkata · 2021-12-01T23:47:19Z

frontend/spec/controllers/spree/checkout_controller_spec.rb

+ end
+ end
+
+ context "new behavior" do


Similar to the above comment, this may be better as "with associate_user_in_authentication_extension disabled".

forkata · 2021-12-01T23:48:19Z

core/spec/lib/spree/core/controller_helpers/order_spec.rb

+ end
+
+ context "user isn't blank" do
+ it 'does not calls Spree::Order#associate_user! method' do


This should be "does not call Spree::Order#associate_user! method".

kennyadsl · 2021-12-03T10:42:27Z

@forkata @waiting-for-dev If the method associate_user won't be used in checkout _controller than method authorize!(:edit, current_order, cookies.signed[:guest_token]) return CanCan::AccessDenied: You are not authorized to access this page.

Did you get why this is happening? Can you please explain where exactly and in which circumstances?

dborovsky · 2021-12-03T16:32:11Z

@forkata @waiting-for-dev If the method associate_user won't be used in checkout _controller than method authorize!(:edit, current_order, cookies.signed[:guest_token]) return CanCan::AccessDenied: You are not authorized to access this page.

Did you get why this is happening? Can you please explain where exactly and in which circumstances?

@kennyadsl

It happens here:

solidus/frontend/app/controllers/spree/checkout_controller.rb

Line 223 in 9dbcad1

def check_authorization

Because now current_order.user_id is nil.

kennyadsl · 2021-12-03T17:26:42Z

I guess we need to keep associate_user then. We can't deprecate a method that we are using and we need to make things work. Or am I missing something?

dborovsky · 2021-12-05T13:08:21Z

Yes. The method associate_user called the method @order.associate_user!(try_spree_current_user) . This method set user_id to current order.

@forkata @waiting-for-dev What do you think?

dborovsky · 2021-12-08T15:00:41Z

@forkata @waiting-for-dev @kennyadsl any news?

waiting-for-dev · 2022-06-09T09:46:38Z

I've been reviewing it, and I think we can close it. Thanks for your work, @dborovsky, and for discovering that the method is still helpful for the checkout controller. We could still remove the call from the orders controller. However, given that we're going to deprecate the current frontend in favor of solidus_starter_frontend, probably it's better to fix it there. Feel free to get back to us if you think it's the wrong decision or in case you still want to work on the removal from the orders controller.

dborovsky · 2022-06-10T10:52:11Z

Hi Marc. (@waiting-for-dev ) I agree. I would like to help contributing solidus. Tell me which tasks are actual and in priority now.

waiting-for-dev · 2022-06-13T04:04:09Z

Hey, @dborovsky, that would be amazing! Would contact me through DM on Slack work for you? My nickname is @waiting_for_dev. Let me know if you prefer any other channel.

carlosep and others added 6 commits August 7, 2020 10:06

Configuration flag for associate_user

428617d

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.

Merge branch 'master' into add_configuration_flag_deprecated_associat…

737621c

…ed_user

Merge branch 'add_configuration_flag_deprecated_associated_user' of h…

3a99ce1

…ttps:https://github.com/carlosep/solidus into add_configuration_flag_deprecated_associated_user � Conflicts: � core/lib/spree/testing_support/dummy_app.rb

solidusio#3559 configure Spree::Deprecation's behavior, test refactoring

56ea6f3

solidusio#3559 remove engine rspec test

58eeb34

solidusio#3559 remove configuration capybara default browser

e8f2733

dborovsky mentioned this pull request Nov 7, 2021

Adds configuration flag for the deprecation of associate_user #3677

Closed

5 tasks