From c2aaf88e98c4e41697993c963a8962ecdcc2a6e0 Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Wed, 31 May 2023 19:37:36 +0100 Subject: [PATCH] remove checkout controller --- app/controllers/checkout_controller.rb | 187 ---------- app/controllers/concerns/order_stock_check.rb | 8 - app/models/spree/order.rb | 7 +- app/services/checkout/form_data_adapter.rb | 85 ----- app/services/permitted_attributes/checkout.rb | 25 -- config/routes.rb | 21 +- spec/controllers/checkout_controller_spec.rb | 346 ------------------ 7 files changed, 8 insertions(+), 671 deletions(-) delete mode 100644 app/controllers/checkout_controller.rb delete mode 100644 app/services/checkout/form_data_adapter.rb delete mode 100644 app/services/permitted_attributes/checkout.rb delete mode 100644 spec/controllers/checkout_controller_spec.rb diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb deleted file mode 100644 index ea4ad42ac7..0000000000 --- a/app/controllers/checkout_controller.rb +++ /dev/null @@ -1,187 +0,0 @@ -# frozen_string_literal: true - -require 'open_food_network/address_finder' - -class CheckoutController < ::BaseController - include OrderStockCheck - include OrderCompletion - include WhiteLabel - - layout 'darkswarm' - - helper 'terms_and_conditions' - helper 'checkout' - - # We need pessimistic locking to avoid race conditions. - # Otherwise we fail on duplicate indexes or end up with negative stock. - prepend_around_action CurrentOrderLocker, only: [:edit, :update] - - prepend_before_action :check_hub_ready_for_checkout - prepend_before_action :check_order_cycle_expiry - prepend_before_action :require_order_cycle - prepend_before_action :require_distributor_chosen - - before_action :load_order - - before_action :handle_insufficient_stock - - before_action :associate_user - before_action :check_authorization - - before_action :hide_ofn_navigation, only: :edit - - helper 'spree/orders' - - def edit; end - - def update - params_adapter = Checkout::FormDataAdapter.new(permitted_params, @order, spree_current_user) - return action_failed unless @order.update(params_adapter.params[:order] || {}) - - checkout_workflow(params_adapter.shipping_method_id) - rescue Spree::Core::GatewayError => e - gateway_error(e) - action_failed(e) - rescue StandardError => e - flash[:error] = I18n.t("checkout.failed") - action_failed(e) - ensure - @order.update_order! - end - - private - - def check_authorization - authorize!(:edit, current_order, session[:access_token]) - end - - def load_order - load_checkout_order - - return handle_invalid_stock unless valid_order_line_items? - - before_address - setup_for_current_state - end - - def handle_invalid_stock - reset_order_to_cart - - respond_to do |format| - format.html do - redirect_to main_app.cart_path - end - - format.json do - render json: { path: main_app.cart_path }, status: :bad_request - end - end - end - - def setup_for_current_state - method_name = :"before_#{@order.state}" - __send__(method_name) if respond_to?(method_name, true) - end - - def before_address - associate_user - - finder = OpenFoodNetwork::AddressFinder.new(@order.email, @order.customer, spree_current_user) - - @order.bill_address = finder.bill_address - @order.ship_address = finder.ship_address - end - - def before_payment - current_order.payments.destroy_all if request.put? - end - - def checkout_workflow(shipping_method_id) - while @order.state != "complete" - if @order.state == "payment" - update_payment_total - return if redirect_to_payment_gateway - - return action_failed if @order.errors.any? - return action_failed unless @order.process_payments! - end - - next if OrderWorkflow.new(@order).next({ "shipping_method_id" => shipping_method_id }) - - return action_failed - end - - update_response - end - - def update_payment_total - @order.update_totals - @order.updater.update_pending_payment - end - - def redirect_to_payment_gateway - return unless selected_payment_method.external_gateway? - return unless (redirect_url = selected_payment_method.external_payment_url(order: @order)) - - render json: { path: redirect_url }, status: :ok - true - end - - def selected_payment_method - @selected_payment_method ||= Spree::PaymentMethod.find( - params.dig(:order, :payments_attributes, 0, :payment_method_id) - ) - end - - def update_response - if order_complete? - processing_succeeded - update_succeeded_response - else - action_failed(RuntimeError.new("Order not complete after the checkout workflow")) - end - end - - def order_complete? - @order.state == "complete" || @order.completed? - end - - def update_succeeded_response - respond_to do |format| - format.html do - respond_with(@order, location: order_completion_route) - end - format.json do - render json: { path: order_completion_route }, status: :ok - end - end - end - - def action_failed(error = RuntimeError.new(order_processing_error)) - processing_failed(error) - action_failed_response - end - - def action_failed_response - respond_to do |format| - format.html do - render :edit - end - format.json do - discard_flash_errors - render json: { errors: @order.errors, flash: flash.to_hash }.to_json, status: :bad_request - end - end - end - - def permitted_params - PermittedAttributes::Checkout.new(params).call - end - - def discard_flash_errors - # Marks flash errors for deletion after the current action has completed. - # This ensures flash errors generated during XHR requests are not persisted in the - # session for longer than expected. - flash.discard(:error) - end -end diff --git a/app/controllers/concerns/order_stock_check.rb b/app/controllers/concerns/order_stock_check.rb index 7ada12ab73..c979948686 100644 --- a/app/controllers/concerns/order_stock_check.rb +++ b/app/controllers/concerns/order_stock_check.rb @@ -13,8 +13,6 @@ module OrderStockCheck def handle_insufficient_stock return if sufficient_stock? - reset_order_to_cart - flash[:error] = Spree.t(:inventory_error_flash_for_insufficient_quantity) redirect_to main_app.cart_path end @@ -43,10 +41,4 @@ module OrderStockCheck def sufficient_stock? @sufficient_stock ||= @order.insufficient_stock_lines.blank? end - - def reset_order_to_cart - return if OpenFoodNetwork::FeatureToggle.enabled? :split_checkout, spree_current_user - - OrderCheckoutRestart.new(@order).call - end end diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 47dc49be26..d180a4b833 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -25,9 +25,7 @@ module Spree order.update_totals order.payment_required? } - go_to_state :confirmation, if: ->(order) { - OpenFoodNetwork::FeatureToggle.enabled? :split_checkout, order.created_by - } + go_to_state :confirmation go_to_state :complete end @@ -319,8 +317,7 @@ module Spree # Creates new tax charges if there are any applicable rates. If prices already # include taxes then price adjustments are created instead. def create_tax_charge! - return if state.in?(["cart", "address", "delivery"]) && - OpenFoodNetwork::FeatureToggle.enabled?(:split_checkout) + return if state.in?(["cart", "address", "delivery"]) clear_legacy_taxes! diff --git a/app/services/checkout/form_data_adapter.rb b/app/services/checkout/form_data_adapter.rb deleted file mode 100644 index f5550126d1..0000000000 --- a/app/services/checkout/form_data_adapter.rb +++ /dev/null @@ -1,85 +0,0 @@ -# frozen_string_literal: true - -# Adapts checkout form data (params) so that the order can be directly saved to the database -module Checkout - class FormDataAdapter - attr_reader :params, :shipping_method_id - - def initialize(params, order, current_user) - @params = params.deep_dup.to_h.with_indifferent_access - @order = order - @current_user = current_user - - move_payment_source_to_payment_attributes! - - fill_in_card_type - - set_amount_in_payments_attributes - - construct_saved_card_attributes if @params.dig(:order, :existing_card_id) - - @shipping_method_id = @params[:order]&.delete(:shipping_method_id) - end - - private - - # For payment step, filter order parameters to produce the expected - # nested attributes for a single payment and its source, - # discarding attributes for payment methods other than the one selected - def move_payment_source_to_payment_attributes! - return unless @params[:payment_source].present? && - payment_source_params = delete_payment_source_params! - - @params.dig(:order, :payments_attributes).first[:source_attributes] = payment_source_params - end - - # Ensures cc_type is always passed to the model by inferring the type when - # the frontend didn't provide it. - def fill_in_card_type - return unless payment_source_attributes - - return if payment_source_attributes.dig(:number).blank? - - payment_source_attributes[:cc_type] ||= card_brand(payment_source_attributes[:number]) - end - - def payment_source_attributes - @payment_source_attributes ||= - @params.dig(:order, :payments_attributes)&.first&.dig(:source_attributes) - end - - def card_brand(number) - ActiveMerchant::Billing::CreditCard.brand?(number) - end - - def delete_payment_source_params! - @params.delete(:payment_source)[ - @params.dig(:order, :payments_attributes).first[:payment_method_id].underscore - ] - end - - def set_amount_in_payments_attributes - return unless @params.dig(:order, :payments_attributes) - - @params.dig(:order, :payments_attributes).first[:amount] = @order.total - end - - def construct_saved_card_attributes - existing_card_id = @params[:order].delete(:existing_card_id) - return if existing_card_id.blank? - - add_to_payment_attributes(existing_card_id) - - @params.dig(:order, :payments_attributes).first.delete :source_attributes - end - - def add_to_payment_attributes(existing_card_id) - credit_card = Spree::CreditCard.find(existing_card_id) - if credit_card.try(:user_id).blank? || credit_card.user_id != @current_user.try(:id) - raise Spree::Core::GatewayError, I18n.t(:invalid_credit_card) - end - - @params.dig(:order, :payments_attributes).first[:source] = credit_card - end - end -end diff --git a/app/services/permitted_attributes/checkout.rb b/app/services/permitted_attributes/checkout.rb deleted file mode 100644 index e9d486d969..0000000000 --- a/app/services/permitted_attributes/checkout.rb +++ /dev/null @@ -1,25 +0,0 @@ -# frozen_string_literal: true - -module PermittedAttributes - class Checkout - def initialize(params) - @params = params - end - - def call - @params.permit( - order: [ - :email, :special_instructions, - :existing_card_id, :shipping_method_id, - { payments_attributes: [ - :payment_method_id, - { source_attributes: PermittedAttributes::PaymentSource.attributes } - ], - ship_address_attributes: PermittedAttributes::Address.attributes, - bill_address_attributes: PermittedAttributes::Address.attributes } - ], - payment_source: PermittedAttributes::PaymentSource.attributes - ) - end - end -end diff --git a/config/routes.rb b/config/routes.rb index be14086b9c..1d933363ac 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -84,24 +84,15 @@ Openfoodnetwork::Application.routes.draw do get "/stripe/authorize/:order_number", to: "stripe#authorize", as: :authorize_stripe end - constraints FeatureToggleConstraint.new(:split_checkout) do - get '/checkout', to: 'split_checkout#edit' + get '/checkout', to: 'split_checkout#edit' - constraints step: /(details|payment|summary)/ do - get '/checkout/:step', to: 'split_checkout#edit', as: :checkout_step - put '/checkout/:step', to: 'split_checkout#update', as: :checkout_update - end - - # Redirects to the new checkout for any other 'step' (ie. /checkout/cart from the legacy checkout) - get '/checkout/:other', to: redirect('/checkout') + constraints step: /(details|payment|summary)/ do + get '/checkout/:step', to: 'split_checkout#edit', as: :checkout_step + put '/checkout/:step', to: 'split_checkout#update', as: :checkout_update end - # When the split_checkout feature is disabled for the current user, use the legacy checkout - constraints FeatureToggleConstraint.new(:split_checkout, negate: true) do - get '/checkout', to: 'checkout#edit' - put '/checkout', to: 'checkout#update', as: :update_checkout - get '/checkout/:state', to: 'checkout#edit', as: :checkout_state - end + # Redirects to the new checkout for any other 'step' (ie. /checkout/cart from the legacy checkout) + get '/checkout/:other', to: redirect('/checkout') get 'embedded_shopfront/shopfront_session', to: 'application#shopfront_session' post 'embedded_shopfront/enable', to: 'application#enable_embedded_styles' diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb deleted file mode 100644 index e0661caaf7..0000000000 --- a/spec/controllers/checkout_controller_spec.rb +++ /dev/null @@ -1,346 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe CheckoutController, type: :controller do - include StripeStubs - - let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) } - let(:order_cycle) { create(:simple_order_cycle) } - let(:order) { create(:order) } - - before do - allow(order).to receive(:checkout_allowed?).and_return true - allow(controller).to receive(:check_authorization).and_return true - end - - it "redirects home when no distributor is selected" do - get :edit - expect(response).to redirect_to root_path - end - - it "redirects to the shop when no order cycle is selected" do - allow(controller).to receive(:current_distributor).and_return(distributor) - get :edit - expect(response).to redirect_to shop_path - end - - it "redirects to shopfront with message if order cycle is expired" do - allow(controller).to receive(:current_distributor).and_return(distributor) - expect(controller).to receive(:current_order_cycle).and_return(order_cycle).at_least(:once) - expect(controller).to receive(:current_order).and_return(order).at_least(:once) - expect(order_cycle).to receive(:closed?).and_return(true) - expect(order).to receive(:empty!) - expect(order).to receive(:set_order_cycle!).with(nil) - - get :edit - - expect(response).to redirect_to shop_url - expect(flash[:info]).to eq 'The order cycle you\'ve selected has just closed. Please try again!' - end - - it "redirects home with message if hub is not ready for checkout" do - allow(distributor).to receive(:ready_for_checkout?) { false } - allow(order).to receive_messages(distributor: distributor, order_cycle: order_cycle) - allow(controller).to receive(:current_order).and_return(order) - - expect(order).to receive(:empty!) - expect(order).to receive(:set_distribution!).with(nil, nil) - - get :edit - - expect(response).to redirect_to root_url - expect(flash[:info]).to eq('The hub you have selected is temporarily closed for orders. Please try again later.') - end - - describe "#update" do - let(:user) { order.user } - let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) } - let(:order_cycle) { create(:order_cycle, distributors: [distributor]) } - let(:order) { create(:order, distributor: distributor, order_cycle: order_cycle) } - let(:payment_method) { distributor.payment_methods.first } - let(:shipping_method) { distributor.shipping_methods.first } - - before do - order.contents.add(order_cycle.variants_distributed_by(distributor).first) - - allow(controller).to receive(:current_distributor).and_return(distributor) - allow(controller).to receive(:current_order_cycle).and_return(order_cycle) - allow(controller).to receive(:current_order).and_return(order) - allow(controller).to receive(:spree_current_user).and_return(user) - - user.bill_address = create(:address) - user.ship_address = create(:address) - user.save! - end - - it "completes the order and redirects to the order confirmation page" do - params = { - "order" => { - "bill_address_attributes" => order.bill_address.attributes, - "default_bill_address" => false, - "default_ship_address" => false, - "email" => user.email, - "payments_attributes" => [{ "payment_method_id" => payment_method.id }], - "ship_address_attributes" => order.bill_address.attributes, - "shipping_method_id" => shipping_method.id - } - } - expect { post :update, params: params }. - to change { Customer.count }.by(1) - expect(order.completed?).to be true - expect(response).to redirect_to order_path(order, order_token: order.token) - end - end - - describe "running out of stock" do - let(:order_cycle_distributed_variants) { double(:order_cycle_distributed_variants) } - - before do - allow(controller).to receive(:current_order).and_return(order) - allow(order).to receive(:distributor).and_return(distributor) - order.update(order_cycle: order_cycle) - - allow(OrderCycleDistributedVariants).to receive(:new).and_return(order_cycle_distributed_variants) - end - - context "handling stock issues" do - it "redirects when some items are out of stock" do - allow(order).to receive_message_chain(:insufficient_stock_lines, :empty?).and_return false - - get :edit - expect(response).to redirect_to cart_path - end - - it "redirects when some items are not available" do - allow(order).to receive_message_chain(:insufficient_stock_lines, :empty?).and_return true - expect(order_cycle_distributed_variants).to receive(:distributes_order_variants?).with(order).and_return(false) - - get :edit - expect(response).to redirect_to cart_path - end - end - end - - describe "building the order" do - before do - allow(controller).to receive(:current_distributor).and_return(distributor) - allow(controller).to receive(:current_order_cycle).and_return(order_cycle) - allow(controller).to receive(:current_order).and_return(order) - end - - it "set shipping_address_from_distributor when re-rendering edit" do - expect(order.updater).to receive(:shipping_address_from_distributor) - allow(order).to receive(:update).and_return false - spree_post :update, format: :json, order: {} - end - - it "set shipping_address_from_distributor when the order state cannot be advanced" do - expect(order.updater).to receive(:shipping_address_from_distributor) - allow(order).to receive(:update).and_return true - allow(order).to receive(:next).and_return false - spree_post :update, format: :json, order: {} - end - - context "#update with shipping_method_id" do - let(:test_shipping_method_id) { "111" } - - before do - allow(controller).to receive(:order_completion_reset) - allow(order).to receive(:update).and_return true - allow(controller).to receive(:current_order).and_return order - - # make order workflow pass through delivery - allow(order).to receive(:next).twice do - if order.state == 'cart' - order.update_column :state, 'delivery' - else - order.update_column :state, 'complete' - end - end - end - - it "does not fail to update" do - expect(controller).to_not receive(:clear_ship_address) - spree_post :update, order: { shipping_method_id: test_shipping_method_id } - end - - it "does not send shipping_method_id to the order model as an attribute" do - expect(order).to receive(:update).with({}) - spree_post :update, order: { shipping_method_id: test_shipping_method_id } - end - - it "selects the shipping_method in the order" do - expect(order).to receive(:select_shipping_method).with(test_shipping_method_id) - spree_post :update, order: { shipping_method_id: test_shipping_method_id } - end - end - - context 'when completing the order' do - before do - order.state = 'complete' - order.save! - allow(order).to receive(:update).and_return(true) - allow(order).to receive(:next).and_return(true) - allow(order).to receive(:set_distributor!).and_return(true) - end - - it "sets the new order's token to the same as the old order" do - order = controller.current_order(true) - spree_post :update, order: {} - expect(controller.current_order.token).to eq order.token - end - - it 'expires the current order' do - allow(controller).to receive(:expire_current_order) - put :update, params: { order: {} } - expect(controller).to have_received(:expire_current_order) - end - - it 'sets the access_token of the session' do - put :update, params: { order: {} } - expect(session[:access_token]).to eq(controller.current_order.token) - end - end - end - - describe '#expire_current_order' do - it 'empties the order_id of the session' do - expect(session).to receive(:[]=).with(:order_id, nil) - controller.send(:expire_current_order) - end - - it 'resets the @current_order ivar' do - controller.send(:expire_current_order) - expect(controller.instance_variable_get(:@current_order)).to be_nil - end - end - - context "via xhr" do - before do - allow(controller).to receive(:current_distributor).and_return(distributor) - - allow(controller).to receive(:current_order_cycle).and_return(order_cycle) - allow(controller).to receive(:current_order).and_return(order) - end - - it "returns errors and flash if order.update fails" do - spree_post :update, format: :json, order: {} - expect(response.status).to eq(400) - expect(response.body).to eq({ errors: assigns[:order].errors, - flash: { error: order.errors.full_messages.to_sentence } }.to_json) - end - - it "returns errors and flash if order.next fails" do - allow(order).to receive(:update).and_return true - allow(order).to receive(:next).and_return false - spree_post :update, format: :json, order: {} - expect(response.body).to eq({ errors: assigns[:order].errors, - flash: { error: "Payment could not be processed, please check the details you entered" } }.to_json) - end - - it "returns order confirmation url on success" do - expect(controller).to receive(:expire_current_order) - expect(controller).to receive(:build_new_order).with(order.distributor, order.token) - - allow(order).to receive(:update).and_return true - allow(order).to receive(:state).and_return "complete" - - spree_post :update, format: :json, order: {} - expect(response.status).to eq(200) - expect(response.body).to eq({ path: order_path(order, order_token: order.token) }.to_json) - end - - it "returns an error on unexpected failure" do - allow(order).to receive(:update).and_raise - - spree_post :update, format: :json, order: {} - expect(response.status).to eq(400) - expect(response.body).to eq({ errors: {}, - flash: { error: 'The checkout failed. Please let us know so that we can process your order.' } }.to_json) - end - - it "returns a specific error on Spree::Core::GatewayError" do - allow(order).to receive(:update).and_raise(Spree::Core::GatewayError.new("Gateway blow up")) - spree_post :update, format: :json, order: {} - - expect(response.status).to eq(400) - flash_message = "There was a problem with your payment information: %s" % 'Gateway blow up' - expect(json_response["flash"]["error"]).to eq flash_message - end - - describe "stale object handling" do - it "retries when a stale object error is encountered" do - expect(controller).to receive(:expire_current_order) - expect(controller).to receive(:build_new_order).with(order.distributor, order.token) - - allow(order).to receive(:update).and_return true - allow(controller).to receive(:state_callback) - - # The first time, raise a StaleObjectError. The second time, succeed. - allow(order).to receive(:next).once. - and_raise(ActiveRecord::StaleObjectError.new(Spree::Variant.new, 'update')) - allow(order).to receive(:next).once do - order.update_column :state, 'complete' - true - end - - spree_post :update, format: :json, order: {} - expect(response.status).to eq(200) - end - - it "tries a maximum of 3 times before giving up and returning an error" do - allow(order).to receive(:update).and_return true - allow(order).to receive(:next) { - raise ActiveRecord::StaleObjectError.new(Spree::Variant.new, 'update') - } - - spree_post :update, format: :json, order: {} - expect(response.status).to eq(400) - end - end - end - - describe "Payment redirects" do - before do - allow(controller).to receive(:current_distributor) { distributor } - allow(controller).to receive(:current_order_cycle) { order_cycle } - allow(controller).to receive(:current_order) { order } - allow(order).to receive(:update) { true } - allow(order).to receive(:state) { "payment" } - end - - describe "redirecting to an external payment gateway" do - let(:payment_method) { create(:payment_method) } - - it "should call Stripe redirect and redirect if a path is provided" do - expect(Spree::PaymentMethod).to receive(:find).and_return(payment_method) - expect(payment_method).to receive(:external_gateway?).and_return(true) - expect(payment_method).to receive(:external_payment_url).and_return("test_path") - - spree_post :update, - order: { payments_attributes: [{ payment_method_id: payment_method.id }] } - - expect(response.body).to eq({ path: "test_path" }.to_json) - end - end - end - - describe "#action_failed" do - let(:restart_checkout) { instance_double(OrderCheckoutRestart, call: true) } - - before do - controller.instance_variable_set(:@order, order) - allow(OrderCheckoutRestart).to receive(:new) { restart_checkout } - allow(controller).to receive(:current_order) { order } - end - - it "set shipping_address_from_distributor and restarts the checkout" do - expect(order.updater).to receive(:shipping_address_from_distributor) - expect(restart_checkout).to receive(:call) - expect(controller).to receive(:respond_to) - - controller.send(:action_failed) - end - end -end