Use create_if_necessary instead of a simple find_or_initialize

[elia]

Description

Using current_order with create_if_necessary creates order that are
much more complete adding info about the ip, creator and other meta
data that is better to have rather than not. Not to mention that it's
also more readable.

Fixes #3467

I did a historical check and no specific reason for doing it this way came about.

• 3516aa3
• 4efa789

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)

[kennyadsl]

Please also read #3467.

[kennyadsl]

Anyway, I was trying to make something similar and the only reason why I don't like this change is that we are writing something into the database on a GET request, which is not ideal (and could be an easy endpoint to target for DoS attacks).

I think another valid way to handle this would be redirecting the user to the root_path if they are trying to see the cart without a current_order in place. Or maybe the empty cart is something that customers want to see anyway?

[elia]

I think it's legitimate to have an empty cart page, and I actually saw it on many stores, usually with links to a product-list or with inline suggestions.

If we don't want to create an order upon visiting /cart it it's missing we I would suggest to let current_order learn how to initialize one. For example by accepting a build_if_necessary argument.

[spaghetticode]

@elia interesting fix, I left a few notes, thank you.

[spaghetticode]

since the order is not persisted, IMO this should read it builds a new order instead.

[spaghetticode]

It's not evident to me why we need to include all these modules now, can you explain the reason?

[elia]

Previously there was the FakesController class that was shared among all controller helpers spec files. During the file loading (but before the specs were ran) each helper was including itself into that class. So, at the time the specs were ready to run, the FakesController class had all the helpers included.

Now, instead, should be more clear on which helpers each helper depends, e.g. the Order helper needs methods from Store, Pricing, and Auth.

[spaghetticode]

@elia thank you 👍

[bitberry-dev]

Great work, thank you!
Coincidentally, I also researched current_order behaviour and found some interesting points in legacy code.

1. Remove reduntant find_or_initialize_by and just build order (you fixed it, great!)
2. Remove associate_user call in OrdersController#edit

That code was added in this commit spree/spree@a3d1f9a

In that version when you call current_order(true) you create new order without any attributes (without current user, without store_id, without currency, without guest_token and so on, check it https://github.com/spree/spree/blob/a3d1f9acdbd5f0ec6c5eceb07116e30cd953948b/core/lib/spree/core/current_order.rb#L27). The logic of order creation was divided - in one place you create empty order, at another place you set necessary attributes such as user_id via associate_user call. This mistake is still here for 8 years despite the fact that in current version necessary attributes are set at current_order.

associate_user call can be safely removed here.

P.S. associate_user defined at this concern Spree::Core::ControllerHelpers::Order and called only at two places - this one (reduntant) and before any action at Spree::CheckoutController. Perhaps it is also reduntant because guest order associates with current user at Warden's after_set_user hook.

[elia]

@bitberryru thanks for the analysis and the hint!

So basically, if we inline associate_user inside spree/orders#edit we get this:

    # Shows the current incomplete order from the session
    def edit
      @order = current_order(build_order_if_necessary: true)
      authorize! :read, @order, cookies.signed[:guest_token]

      # vvv Inlined version of #associate_user 
      if try_spree_current_user && (@order.user.blank? || @order.email.blank?)
        @order.associate_user!(try_spree_current_user)
      end

      if params[:id] && @order.number != params[:id]
        flash[:error] = t('spree.cannot_edit_orders')
        redirect_to cart_path
      end
    end

The idea is that it's the responsibility of the authentication system to do the association in case the order was created before the login, and we can safely assume that we'll never encounter a persisted order that is authorized (via guest_token) and not already associated to the current user.

Here's the solidus_auth_devise warden callback:

# Merges users orders to their account after sign in and sign up.
Warden::Manager.after_set_user except: :fetch do |user, auth, _opts|
  if auth.cookies.signed[:guest_token].present?
    if user.is_a?(Spree::User)
      Spree::Order.incomplete.where(guest_token: auth.cookies.signed[:guest_token], user_id: nil).each do |order|
        order.associate_user!(user)
      end
    end
  end
end

---

About the checkout controller seems like the code is executed in a different sequence:

  class CheckoutController < Spree::StoreController
    before_action :load_order                   # <<<< calls #current_order
    around_action :lock_order

    before_action :ensure_order_is_not_skipping_states
    before_action :ensure_order_not_completed
    before_action :ensure_checkout_allowed
    before_action :ensure_sufficient_stock_lines
    before_action :ensure_valid_state

    before_action :associate_user               # <<<< calls #associate_user
    before_action :check_authorization          # <<<< checks against the guest_token
    before_action :apply_coupon_code

I'd like to more research to be sure not to break something for some store…

@kennyadsl @bitberryru what about dealing with removing #associate_user in another PR?

[kennyadsl]

Sure, another PR is perfect for this change.

[kennyadsl]

Moved the discussion in a separate issue: #3559

[kennyadsl]

Thanks @elia, this seems to fix #3467 without breaking existing functionalities. 👏