From cb5dd6eed9f3f31f850fa155580b433c8f775be5 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 28 Dec 2021 23:02:03 +0000 Subject: [PATCH 1/8] Move T&C checks to service --- app/helpers/terms_and_conditions_helper.rb | 4 ++-- app/services/terms_of_service.rb | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/app/helpers/terms_and_conditions_helper.rb b/app/helpers/terms_and_conditions_helper.rb index ce53ac3ed9..35232da157 100644 --- a/app/helpers/terms_and_conditions_helper.rb +++ b/app/helpers/terms_and_conditions_helper.rb @@ -17,11 +17,11 @@ module TermsAndConditionsHelper end def platform_terms_required? - Spree::Config.shoppers_require_tos + TermsOfService.platform_terms_required? end def terms_and_conditions_activated? - current_order.distributor.terms_and_conditions.file? + TermsOfService.terms_and_conditions_activated?(current_order.distributor) end def all_terms_and_conditions_already_accepted? diff --git a/app/services/terms_of_service.rb b/app/services/terms_of_service.rb index dc84b14988..32ca768d0c 100644 --- a/app/services/terms_of_service.rb +++ b/app/services/terms_of_service.rb @@ -10,4 +10,12 @@ class TermsOfService TermsOfServiceFile.updated_at end end + + def self.platform_terms_required? + Spree::Config.shoppers_require_tos + end + + def self.terms_and_conditions_activated?(distributor) + distributor.terms_and_conditions.file? + end end From 58d3ad412a5db78ac92002a5491acdec758f48de Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 12 Jan 2022 16:01:32 +0000 Subject: [PATCH 2/8] Rename method for clarity --- app/helpers/terms_and_conditions_helper.rb | 8 ++++---- app/services/terms_of_service.rb | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/helpers/terms_and_conditions_helper.rb b/app/helpers/terms_and_conditions_helper.rb index 35232da157..2eb4773ecf 100644 --- a/app/helpers/terms_and_conditions_helper.rb +++ b/app/helpers/terms_and_conditions_helper.rb @@ -7,11 +7,11 @@ module TermsAndConditionsHelper end def render_terms_and_conditions - if platform_terms_required? && terms_and_conditions_activated? + if platform_terms_required? && distributor_terms_required? render("checkout/all_terms_and_conditions") elsif platform_terms_required? render "checkout/platform_terms_of_service" - elsif terms_and_conditions_activated? + elsif distributor_terms_required? render "checkout/terms_and_conditions" end end @@ -20,8 +20,8 @@ module TermsAndConditionsHelper TermsOfService.platform_terms_required? end - def terms_and_conditions_activated? - TermsOfService.terms_and_conditions_activated?(current_order.distributor) + def distributor_terms_required? + TermsOfService.distributor_terms_required?(current_order.distributor) end def all_terms_and_conditions_already_accepted? diff --git a/app/services/terms_of_service.rb b/app/services/terms_of_service.rb index 32ca768d0c..62e497f7ff 100644 --- a/app/services/terms_of_service.rb +++ b/app/services/terms_of_service.rb @@ -15,7 +15,7 @@ class TermsOfService Spree::Config.shoppers_require_tos end - def self.terms_and_conditions_activated?(distributor) + def self.distributor_terms_required?(distributor) distributor.terms_and_conditions.file? end end From f2c506a292944afe9a12066537a345dc2ef7e04a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 12 Jan 2022 21:02:53 +0000 Subject: [PATCH 3/8] Update and extract summary_step? check --- app/controllers/split_checkout_controller.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 5e36409b1c..d6d6450651 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -41,7 +41,7 @@ class SplitCheckoutController < ::BaseController end def confirm_order - return unless @order.confirmation? && params[:confirm_order] + return unless summary_step? && @order.confirmation? return unless validate_summary! && @order.errors.empty? @order.confirm! @@ -57,6 +57,10 @@ class SplitCheckoutController < ::BaseController @order.errors.empty? end + def summary_step? + params[:step] == "summary" + end + def advance_order_state return if @order.complete? From 18cdf98aa116ce887c3c1df697d9fdf02c2c350b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 28 Dec 2021 23:14:34 +0000 Subject: [PATCH 4/8] Set terms and conditions accepted when completing checkout --- app/controllers/split_checkout_controller.rb | 2 ++ app/services/terms_of_service.rb | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index d6d6450651..6ff7c24188 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -44,6 +44,7 @@ class SplitCheckoutController < ::BaseController return unless summary_step? && @order.confirmation? return unless validate_summary! && @order.errors.empty? + @order.customer.touch :terms_and_conditions_accepted_at @order.confirm! end @@ -81,6 +82,7 @@ class SplitCheckoutController < ::BaseController def validate_summary! return true if params[:accept_terms] + return true unless TermsOfService.required?(@order.distributor) @order.errors.add(:terms_and_conditions, t("split_checkout.errors.terms_not_accepted")) end diff --git a/app/services/terms_of_service.rb b/app/services/terms_of_service.rb index 62e497f7ff..95cbf661d6 100644 --- a/app/services/terms_of_service.rb +++ b/app/services/terms_of_service.rb @@ -11,6 +11,10 @@ class TermsOfService end end + def self.required?(distributor) + platform_terms_required? || distributor_terms_required?(distributor) + end + def self.platform_terms_required? Spree::Config.shoppers_require_tos end From f8efff9a4e7f1345daaac1a7c97233f99651a78a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 12 Jan 2022 22:11:31 +0000 Subject: [PATCH 5/8] Don't show terms of service unless required --- app/helpers/terms_and_conditions_helper.rb | 4 ++++ app/views/split_checkout/_summary.html.haml | 15 ++++++++------- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/app/helpers/terms_and_conditions_helper.rb b/app/helpers/terms_and_conditions_helper.rb index 2eb4773ecf..c30b57164b 100644 --- a/app/helpers/terms_and_conditions_helper.rb +++ b/app/helpers/terms_and_conditions_helper.rb @@ -16,6 +16,10 @@ module TermsAndConditionsHelper end end + def any_terms_required?(distributor) + TermsOfService.required?(distributor) + end + def platform_terms_required? TermsOfService.platform_terms_required? end diff --git a/app/views/split_checkout/_summary.html.haml b/app/views/split_checkout/_summary.html.haml index 57c9c18ab5..131f6f3e0e 100644 --- a/app/views/split_checkout/_summary.html.haml +++ b/app/views/split_checkout/_summary.html.haml @@ -71,15 +71,16 @@ = render 'spree/orders/summary', order: @order -%div.checkout-substep.medium-6 - %div.checkout-input - = f.check_box :accept_terms, { id: "accept_terms", name: "accept_terms", "checked": "#{all_terms_and_conditions_already_accepted?}" }, 1, nil - = f.label :accept_terms, t('split_checkout.step3.terms_and_conditions.message_html', terms_and_conditions_link: link_to( t("split_checkout.step3.terms_and_conditions.link_text"), @order.distributor.terms_and_conditions.url, target: '_blank'), tos_link: link_to_platform_terms), { for: "accept_terms" } +- if any_terms_required?(@order.distributor) + %div.checkout-substep.medium-6 + %div.checkout-input + = f.check_box :accept_terms, { id: "accept_terms", name: "accept_terms", "checked": "#{all_terms_and_conditions_already_accepted?}" }, 1, nil + = f.label :accept_terms, t('split_checkout.step3.terms_and_conditions.message_html', terms_and_conditions_link: link_to( t("split_checkout.step3.terms_and_conditions.link_text"), @order.distributor.terms_and_conditions.url, target: '_blank'), tos_link: link_to_platform_terms), { for: "accept_terms" } - = f.error_message_on :terms_and_conditions, standalone: true + = f.error_message_on :terms_and_conditions, standalone: true - %div.checkout-input - = t("split_checkout.step3.agree") + %div.checkout-input + = t("split_checkout.step3.agree") %div.checkout-submit.medium-6 = f.submit t("split_checkout.step3.submit"), name: "confirm_order", class: "button primary", disabled: @terms_and_conditions_accepted == false || @platform_tos_accepted == false From b850fd6fda635542d8f2bf128fdb04856d2684a2 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 12 Jan 2022 22:14:06 +0000 Subject: [PATCH 6/8] Add controller spec for split checkout --- .../split_checkout_controller_spec.rb | 182 ++++++++++++++++++ 1 file changed, 182 insertions(+) create mode 100644 spec/controllers/split_checkout_controller_spec.rb diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb new file mode 100644 index 0000000000..92f8cba389 --- /dev/null +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -0,0 +1,182 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SplitCheckoutController, type: :controller do + let(:user) { order.user } + let(:address) { create(:address) } + let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) } + let(:order_cycle) { create(:order_cycle, distributors: [distributor]) } + let(:exchange) { order_cycle.exchanges.outgoing.first } + let(:order) { + create(:order_with_line_items, line_items_count: 1, distributor: distributor, + order_cycle: order_cycle) + } + let(:payment_method) { distributor.payment_methods.first } + let(:shipping_method) { distributor.shipping_methods.first } + + before do + allow(Flipper).to receive(:enabled?).with(:split_checkout) { true } + allow(Flipper).to receive(:enabled?).with(:split_checkout, anything) { true } + + exchange.variants << order.line_items.first.variant + allow(controller).to receive(:current_order) { order } + allow(controller).to receive(:spree_current_user) { user } + end + + describe "#edit" do + it "renders the checkout" do + get :edit, params: { step: "details" } + expect(response.status).to eq 200 + end + + it "redirects to current step if no step is given" do + get :edit + expect(response).to redirect_to checkout_step_path(:details) + end + + context "when line items in the cart are not valid" do + before { allow(controller).to receive(:valid_order_line_items?) { false } } + + it "redirects to cart" do + get :edit + expect(response).to redirect_to cart_path + end + end + end + + describe "#update" do + let(:checkout_params) { {} } + let(:params) { { step: step }.merge(checkout_params) } + + context "details step" do + let(:step) { "details" } + + context "with incomplete data" do + let(:checkout_params) { { order: { email: user.email } } } + + it "returns 422 and some feedback" do + put :update, params: params + + expect(response.status).to eq 422 + expect(flash[:error]).to eq "Saving failed, please update the highlighted fields." + expect(order.reload.state).to eq "cart" + end + end + + context "with complete data" do + let(:checkout_params) do + { + order: { + email: user.email, + bill_address_attributes: address.to_param, + ship_address_attributes: address.to_param + }, + shipping_method_id: shipping_method.id + } + end + + it "updates and redirects to payment step" do + put :update, params: params + + expect(response).to redirect_to checkout_step_path(:payment) + expect(order.reload.state).to eq "payment" + end + end + end + + context "payment step" do + let(:step) { "payment" } + + before do + order.bill_address = address + order.ship_address = address + order.select_shipping_method shipping_method.id + OrderWorkflow.new(order).advance_to_payment + end + + context "with incomplete data" do + let(:checkout_params) { { order: { email: user.email } } } + + it "returns 422 and some feedback" do + put :update, params: params + + expect(response.status).to eq 422 + expect(flash[:error]).to eq "Saving failed, please update the highlighted fields." + expect(order.reload.state).to eq "payment" + end + end + + context "with complete data" do + let(:checkout_params) do + { + order: { + payments_attributes: [ + { payment_method_id: payment_method.id } + ] + } + } + end + + it "updates and redirects to payment step" do + put :update, params: params + + expect(response).to redirect_to checkout_step_path(:summary) + expect(order.reload.state).to eq "confirmation" + end + end + end + + context "summary step" do + let(:step) { "summary" } + + before do + order.bill_address = address + order.ship_address = address + order.select_shipping_method shipping_method.id + OrderWorkflow.new(order).advance_to_payment + + order.payments << build(:payment, amount: order.total, payment_method: payment_method) + order.next + end + + describe "confirming the order" do + it "completes the order and redirects to order confirmation" do + put :update, params: params + + expect(response).to redirect_to order_path(order) + expect(order.reload.state).to eq "complete" + end + end + + context "when accepting T&Cs is required" do + before do + allow(TermsOfService).to receive(:platform_terms_required?) { true } + end + + describe "submitting without accepting the T&Cs" do + let(:checkout_params) { {} } + + it "returns 422 and some feedback" do + put :update, params: params + + expect(response.status).to eq 422 + expect(flash[:error]).to eq "Saving failed, please update the highlighted fields." + expect(order.reload.state).to eq "confirmation" + end + end + + describe "submitting and accepting the T&Cs" do + let(:checkout_params) { { accept_terms: true } } + + it "completes the order and redirects to order confirmation" do + put :update, params: params + + expect(response).to redirect_to order_path(order) + expect(order.reload.state).to eq "complete" + end + end + end + end + end +end From 1d4803c31abbef53c6104d81f4aa95db38308299 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 12 Jan 2022 22:40:04 +0000 Subject: [PATCH 7/8] Whitelist valid step param values --- app/controllers/split_checkout_controller.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 6ff7c24188..25775745f0 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -53,7 +53,8 @@ class SplitCheckoutController < ::BaseController @order.select_shipping_method(params[:shipping_method_id]) @order.update(order_params) - send("validate_#{params[:step]}!") + + validate_current_step! @order.errors.empty? end @@ -68,6 +69,11 @@ class SplitCheckoutController < ::BaseController OrderWorkflow.new(@order).advance_checkout(raw_params.slice(:shipping_method_id)) end + def validate_current_step! + step = params[:step].tap{ |step| ["details", "payment", "summary"].include? step } + send("validate_#{step}!") + end + def validate_details! return true if params[:shipping_method_id].present? From 7c2b3cdf5111c65a9e7f45522c6ecf923296a356 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 18 Jan 2022 09:14:55 +0000 Subject: [PATCH 8/8] Fix param whitelisting Co-authored-by: Maikel --- app/controllers/split_checkout_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index 25775745f0..a69d1dc5e0 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -70,7 +70,7 @@ class SplitCheckoutController < ::BaseController end def validate_current_step! - step = params[:step].tap{ |step| ["details", "payment", "summary"].include? step } + step = ([params[:step]] & ["details", "payment", "summary"]).first send("validate_#{step}!") end