Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use create_if_necessary instead of a simple find_or_initialize #3494

Merged
merged 7 commits into from
Mar 18, 2020
Merged

Use create_if_necessary instead of a simple find_or_initialize #3494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
df95ee0
Move a loose OrdersController spec to its spec file
elia Feb 3, 2020
4bfe9a7
Only pass the `lock` option to #find_order_by_token_or_user
elia Feb 3, 2020
aa5eaab
Avoid mutating the options hash passed order helpers
elia Feb 3, 2020
7c79a1c
Don't define top-level classes within specs
elia Feb 3, 2020
f40f627
Let Order#record_ip_address work with new records
elia Feb 3, 2020
d90be78
Let current_order accept a build_order_if_necessary option
elia Feb 3, 2020
030bf6c
Use current_order with build_if_necessary in cart page
elia Jan 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion core/app/models/spree/order.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,9 @@ def add_default_payment_from_wallet
deprecate assign_default_credit_card: :add_default_payment_from_wallet, deprecator: Spree::Deprecation

def record_ip_address(ip_address)
if last_ip_address != ip_address
if new_record?
self.last_ip_address = ip_address
elsif last_ip_address != ip_address
update_column(:last_ip_address, ip_address)
end
end
Expand Down
15 changes: 8 additions & 7 deletions core/lib/spree/core/controller_helpers/order.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,19 @@ def simple_current_order

# The current incomplete order from the guest_token for use in cart and during checkout
def current_order(options = {})
options[:create_order_if_necessary] ||= false
should_create = options[:create_order_if_necessary] || false
should_build = options[:build_order_if_necessary] || should_create

return @current_order if @current_order

@current_order = find_order_by_token_or_user(options)
@current_order = find_order_by_token_or_user(lock: options[:lock])

if options[:create_order_if_necessary] && (@current_order.nil? || @current_order.completed?)
if should_build && (@current_order.nil? || @current_order.completed?)
@current_order = Spree::Order.new(new_order_params)
@current_order.user ||= try_spree_current_user
# See issue https://github.com/spree/spree/issues/3346 for reasons why this line is here
@current_order.created_by ||= try_spree_current_user
@current_order.save!
@current_order.save! if should_create
end

if @current_order
Expand Down Expand Up @@ -84,14 +85,14 @@ def new_order_params
end

def find_order_by_token_or_user(options = {}, with_adjustments = false)
options[:lock] ||= false
should_lock = options[:lock] || false

# Find any incomplete orders for the guest_token
if with_adjustments
Spree::Deprecation.warn "The second argument to find_order_by_token_or_user is deprecated, and will be removed in a future version."
order = Spree::Order.incomplete.includes(:adjustments).lock(options[:lock]).find_by(current_order_params)
order = Spree::Order.incomplete.includes(:adjustments).lock(should_lock).find_by(current_order_params)
else
order = Spree::Order.incomplete.lock(options[:lock]).find_by(current_order_params)
order = Spree::Order.incomplete.lock(should_lock).find_by(current_order_params)
end

# Find any incomplete orders for the current user
Expand Down
22 changes: 12 additions & 10 deletions core/spec/lib/spree/core/controller_helpers/auth_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@

require 'rails_helper'

class FakesController < ApplicationController
include Spree::Core::ControllerHelpers::Auth
def index; render plain: 'index'; end
end

RSpec.describe Spree::Core::ControllerHelpers::Auth, type: :controller do
controller(FakesController) {}
controller(ApplicationController) {
include Spree::Core::ControllerHelpers::Auth
def index; render plain: 'index'; end
}

describe '#current_ability' do
it 'returns Spree::Ability instance' do
Expand All @@ -17,9 +15,12 @@ def index; render plain: 'index'; end
end

describe '#redirect_back_or_default' do
controller(FakesController) do
def index; redirect_back_or_default('/'); end
before do
def controller.index
redirect_back_or_default('/')
end
end

it 'redirects to session url' do
session[:spree_user_return_to] = '/redirect'
get :index
Expand All @@ -32,12 +33,13 @@ def index; redirect_back_or_default('/'); end
end

describe '#set_guest_token' do
controller(FakesController) do
def index
before do
def controller.index
set_guest_token
render plain: 'index'
end
end

it 'sends cookie header' do
get :index
expect(response.headers["Set-Cookie"]).to match(/guest_token.*HttpOnly/)
Expand Down
28 changes: 23 additions & 5 deletions core/spec/lib/spree/core/controller_helpers/order_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@

require 'rails_helper'

class FakesController < ApplicationController
include Spree::Core::ControllerHelpers::Order
end

RSpec.describe Spree::Core::ControllerHelpers::Order, type: :controller do
controller(FakesController) {}
controller(ApplicationController) {
include Spree::Core::ControllerHelpers::Store
include Spree::Core::ControllerHelpers::Pricing
include Spree::Core::ControllerHelpers::Auth
include Spree::Core::ControllerHelpers::Order
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

let(:user) { create(:user) }
let(:order) { create(:order, user: user, store: store) }
Expand Down Expand Up @@ -74,6 +75,23 @@ class FakesController < ApplicationController
}.from(nil).to("0.0.0.0")
end
end

context 'build_order_if_necessary option is true' do
subject { controller.current_order(build_order_if_necessary: true) }

it 'builds a new order' do
expect { subject }.not_to change(Spree::Order, :count).from(0)
expect(subject).not_to be_persisted
end

it 'assigns the current_store id' do
expect(subject.store_id).to eq store.id
end

it 'records last_ip_address' do
expect(subject.last_ip_address).to eq("0.0.0.0")
end
end
end

describe '#associate_user' do
Expand Down
9 changes: 4 additions & 5 deletions core/spec/lib/spree/core/controller_helpers/pricing_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@

require 'rails_helper'

class FakesController < ApplicationController
include Spree::Core::ControllerHelpers::Pricing
end

RSpec.describe Spree::Core::ControllerHelpers::Pricing, type: :controller do
controller(FakesController) {}
controller(ApplicationController) {
include Spree::Core::ControllerHelpers::Store
include Spree::Core::ControllerHelpers::Pricing
}

before do
allow(controller).to receive(:current_store).and_return(store)
Expand Down
10 changes: 5 additions & 5 deletions core/spec/lib/spree/core/controller_helpers/search_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@

require 'rails_helper'

class FakesController < ApplicationController
include Spree::Core::ControllerHelpers::Search
end

RSpec.describe Spree::Core::ControllerHelpers::Search, type: :controller do
controller(FakesController) {}
controller(ApplicationController) {
include Spree::Core::ControllerHelpers::Auth
include Spree::Core::ControllerHelpers::Pricing
include Spree::Core::ControllerHelpers::Search
}

describe '#build_searcher' do
it 'returns Spree::Core::Search::Base instance' do
Expand Down
8 changes: 3 additions & 5 deletions core/spec/lib/spree/core/controller_helpers/store_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@

require 'rails_helper'

class FakesController < ApplicationController
include Spree::Core::ControllerHelpers::Store
end

RSpec.describe Spree::Core::ControllerHelpers::Store, type: :controller do
controller(FakesController) {}
controller(ApplicationController) {
include Spree::Core::ControllerHelpers::Store
}

describe '#current_store' do
let!(:store) { create :store, default: true }
Expand Down
8 changes: 3 additions & 5 deletions core/spec/lib/spree/core/controller_helpers/strong_parameters_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@

require 'rails_helper'

class FakesController < ApplicationController
include Spree::Core::ControllerHelpers::StrongParameters
end

RSpec.describe Spree::Core::ControllerHelpers::StrongParameters, type: :controller do
controller(FakesController) {}
controller(ApplicationController) {
include Spree::Core::ControllerHelpers::StrongParameters
}

describe '#permitted_attributes' do
it 'returns Spree::PermittedAttributes module' do
Expand Down
8 changes: 8 additions & 0 deletions core/spec/models/spree/order_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1526,6 +1526,14 @@ def generate
expect(subject).to change(order, :last_ip_address).to(ip_address)
end
end

context "with a new order" do
let(:order) { build(:order) }

it "updates the IP address" do
expect(subject).to change(order, :last_ip_address).to(ip_address)
end
end
end

describe "#display_order_total_after_store_credit" do
Expand Down
2 changes: 1 addition & 1 deletion frontend/app/controllers/spree/orders_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def update

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

@bitberry-dev bitberry-dev Mar 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, another PR is perfect for this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the discussion in a separate issue: #3559

if params[:id] && @order.number != params[:id]
Expand Down
14 changes: 0 additions & 14 deletions frontend/spec/controllers/spree/current_order_tracking_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,3 @@ def index
end
end
end

describe Spree::OrdersController, type: :controller do
let(:user) { create(:user) }

before { allow(controller).to receive_messages(try_spree_current_user: user) }

describe Spree::OrdersController do
it "doesn't create a new order out of the blue" do
expect {
get :edit
}.not_to change { Spree::Order.count }
end
end
end
21 changes: 21 additions & 0 deletions frontend/spec/controllers/spree/orders_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,4 +251,25 @@
expect(order.reload.line_items.count).to eq 0
end
end

describe '#edit' do
subject { get :edit }
let(:user) { build :user }

it "builds a new valid order with complete meta-data" do
allow(controller).to receive_messages(try_spree_current_user: user)

subject

order = controller.instance_variable_get(:@order)

aggregate_failures do
expect(order).to be_valid
expect(order).not_to be_persisted
expect(order.store).to be_present
expect(order.user).to eq(user)
expect(order.created_by).to eq(user)
end
end
end
end