Hello!

I have read this discussion and as far as I could understand, The idea here is that instead of removing associate_user completely we would check again every time and associate the user if it is needed via the @order.associate_user!, as pointed by @elia's inline. Am I correct? That seems like a safe solution if we cannot pinpoint all of the possible cases.

@bitberryru mentioned that every attribute that is set via associate_user is already being set at current_order, but I didn't understand how. Maybe if it is too implicit, could it be better to leave the redundancy for the sake of understandability and to avoid awkward behavior in case something would come to change in Spree?

Also at the checkout controller it is mentioned that the order of execution is different and therefore it could break something for some store. I'd like to know more about how different stores could have different behaviors in this specific point. Maybe I'm just not acquainted enough with this system or how it is used in production but this point left me somewhat confused.

My mail server continues to flag github mails as spam :( I have not seen the discussions, I would have answered earlier.

I’ll try to explain again, in the old versions building order and user association with it was in different places, now when you build/create an order (by calling current_order) it is immediately associated with the current user, now there is no need to separately associate the user with the order. This call (https://github.com/solidusio/solidus/blob/master/frontend/app/controllers/spree/orders_controller.rb#L44) has survived from the old days and has no sense now.

FYI: There is only one case when it is required to associate an anonymous order with a user - after user authentication. This task is solved by solidus_auth_devise in warden hook. And this is the responsibility of the authentication system.

Thanks, I understand now.
Based on this I opened #3667 removing the associate_user line from the OrdersController#edit. It also seems to make sense to remove it from the CheckoutController but I'm worried about the mentioned order of execution and would like some comments from @elia on the PR about that and maybe about some way we could test it before I could remove that line as well if it is the case.

@carlosep I agree with @bitberryru about being time to get rid of that method.

About testing it I would suggest going through a deprecation cycle (which I will make it into 2.11) using a configuration flag to get the old behavior. In this scenario any store having an OrderController decorator that defines a method like this:

    def associate_user
      super

      # Update the order to reset the adjustments on the order once a user has
      # been associated with it.
      @order.recalculate
    end

should see a deprecation and be able to move its customization elsewhere before upgrading. If done properly that should also cover the CheckoutController before-action and we should be able to cleanup things in a short cycle.

Hi @elia, This seems like a good solution. Would that mean just a warn in the associate_user method or is there a standard way we should go through regarding the deprecation cycle?

Hey @carlosep, yeah, that would surely mean a deprecation in the method, but that should also be accompanied by a Solidus configuration that allows disabling the new behavior, you can find many examples of similar deprecations in the Solidus codebase, e.g. you can see here how Spree::Config.use_combined_first_and_last_name_in_address is used to control the behavior and enables supporting some legacy attributes if set to false.

This is done to let store-owners update their stores individually, one deprecation at a time. Feel free to reach to me in the Solidus slack channel if you need more examples or have questions. 👍

Hi, @elia! I closed the previous PR and opened a new one with the configuration flag for this deprecation.