From 79f4caaee29c8c1c386ed43b35ecdf235d9e96ad Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 5 Sep 2023 15:48:30 +0200 Subject: [PATCH 1/9] Clean up spec --- spec/controllers/admin/enterprises_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/admin/enterprises_controller_spec.rb b/spec/controllers/admin/enterprises_controller_spec.rb index f965bb6357..10bd9d3200 100644 --- a/spec/controllers/admin/enterprises_controller_spec.rb +++ b/spec/controllers/admin/enterprises_controller_spec.rb @@ -281,7 +281,7 @@ describe Admin::EnterprisesController, type: :controller do } } } - expect(tag_rule.reload).to be + new_tag_rule = TagRule::FilterOrderCycles.last expect(new_tag_rule.preferred_exchange_tags).to eq "tags,are,awesome" end From d1b5dcab885f267a5ae4f0de0111cc8febfbddf3 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 5 Sep 2023 15:48:59 +0200 Subject: [PATCH 2/9] Add ability to activate deactivate a voucher Plus controller specs --- .../admin/enterprises_controller.rb | 15 ++++++ .../enterprises/form/_vouchers.html.haml | 15 +++++- config/locales/en.yml | 1 + .../admin/enterprises_controller_spec.rb | 48 +++++++++++++++++++ 4 files changed, 77 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/enterprises_controller.rb b/app/controllers/admin/enterprises_controller.rb index dbdd701fb8..775d00840b 100644 --- a/app/controllers/admin/enterprises_controller.rb +++ b/app/controllers/admin/enterprises_controller.rb @@ -63,6 +63,7 @@ module Admin tag_rules_attributes = params[object_name].delete :tag_rules_attributes update_tag_rules(tag_rules_attributes) if tag_rules_attributes.present? update_enterprise_notifications + update_vouchers delete_custom_tab if params[:custom_tab] == 'false' @@ -268,6 +269,20 @@ module Admin @enterprise.update_contact(user_id) end + def update_vouchers + return if params[:enterprise][:voucher_ids].blank? + + params_voucher_ids = params[:enterprise][:voucher_ids].map(&:to_i) + voucher_ids = @enterprise.vouchers.map(&:id) + deleted_voucher_ids = @enterprise.vouchers.only_deleted.map(&:id) + + vouchers_to_destroy = voucher_ids - params_voucher_ids + Voucher.where(id: vouchers_to_destroy).destroy_all if vouchers_to_destroy.present? + + vouchers_to_restore = deleted_voucher_ids.intersection(params_voucher_ids) + Voucher.restore(vouchers_to_restore) if vouchers_to_restore.present? + end + def create_calculator_for(rule, attrs) return unless attrs[:calculator_type].present? && attrs[:calculator_attributes].present? diff --git a/app/views/admin/enterprises/form/_vouchers.html.haml b/app/views/admin/enterprises/form/_vouchers.html.haml index 8df72f951c..5aa9ba9dd4 100644 --- a/app/views/admin/enterprises/form/_vouchers.html.haml +++ b/app/views/admin/enterprises/form/_vouchers.html.haml @@ -3,12 +3,13 @@ = t('.add_new') %br -- if @enterprise.vouchers.present? +- if @enterprise.vouchers.present? || @enterprise.vouchers.only_deleted.present? %table %thead %tr %th= t('.voucher_code') %th= t('.rate') + %th= t('.active') /%th= t('.label') /%th= t('.purpose') /%th= t('.expiry') @@ -20,13 +21,23 @@ %tr %td= voucher.code %td= voucher.display_value + %td= f.check_box :voucher_ids, { :multiple => true }, voucher.id, nil + /%td + /%td + /%td + /%td + /%td + - @enterprise.vouchers.only_deleted.each do |voucher| + %tr + %td= voucher.code + %td= voucher.display_value + %td= f.check_box :voucher_ids, { :multiple => true }, voucher.id, nil /%td /%td /%td /%td /%td /%td - - else %p.text-center = t('.no_voucher_yet') diff --git a/config/locales/en.yml b/config/locales/en.yml index 9020961d32..b4d04d7221 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1236,6 +1236,7 @@ en: use_limit: Use/Limit customers: Customer net_value: Net Value + active: Active? add_new: Add New no_voucher_yet: No Vouchers yet white_label: diff --git a/spec/controllers/admin/enterprises_controller_spec.rb b/spec/controllers/admin/enterprises_controller_spec.rb index 10bd9d3200..c68c04b82d 100644 --- a/spec/controllers/admin/enterprises_controller_spec.rb +++ b/spec/controllers/admin/enterprises_controller_spec.rb @@ -287,6 +287,54 @@ describe Admin::EnterprisesController, type: :controller do end end end + + describe "vouchers" do + let(:enterprise) { create(:distributor_enterprise) } + let!(:voucher_a) { create(:voucher, enterprise: enterprise, code: "voucher 1") } + let!(:voucher_b) { create(:voucher, enterprise: enterprise, code: "voucher 2") } + + before do + controller_login_as_enterprise_user [enterprise] + end + + it "activates checked vouchers" do + voucher_b.destroy + + spree_put :update, + id: enterprise, + enterprise: { + voucher_ids: [voucher_a.id.to_s, voucher_b.id.to_s] + } + + expect(voucher_b.reload.deleted?).to be(false) + end + + it "deactivates unchecked vouchers" do + spree_put :update, + id: enterprise, + enterprise: { + voucher_ids: [voucher_b.id.to_s] + } + + expect(voucher_a.reload.deleted?).to be(true) + end + + context "when activating and deactivating voucher at the same time" do + it "deactivates and activates accordingly" do + voucher_c = create(:voucher, enterprise: enterprise, code: "voucher 3") + voucher_c.destroy + + spree_put :update, + id: enterprise, + enterprise: { + voucher_ids: [voucher_a.id.to_s, voucher_c.id.to_s] + } + + expect(enterprise.vouchers.reload).to include(voucher_c) + expect(enterprise.vouchers.reload.only_deleted).to include(voucher_b) + end + end + end end context "as owner" do From b955e0b25dfc21ba02172f92ec75959d3cab61dd Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Thu, 7 Sep 2023 13:58:07 +0200 Subject: [PATCH 3/9] Use depedend: nil on adjustment association We want to keep the voucher adjustment associated with the Voucher event when the voucher as been soft deleted, as we use this functionality as activate/deactivate feature --- app/models/voucher.rb | 4 +++- spec/models/voucher_spec.rb | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 9ba6a95c3b..5e5983fa13 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -7,10 +7,12 @@ class Voucher < ApplicationRecord belongs_to :enterprise, optional: false + # We want to keep the association with the adjustment when a voucher is "destroyed" as we use + # the soft delete functionality to activate/deactivate vouchers has_many :adjustments, as: :originator, class_name: 'Spree::Adjustment', - dependent: :nullify + dependent: nil validates :code, presence: true, uniqueness: { scope: :enterprise_id } diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index 4e3c294af9..a895db0da2 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -12,7 +12,7 @@ describe Voucher do describe 'associations' do it { is_expected.to belong_to(:enterprise).required } - it { is_expected.to have_many(:adjustments) } + it { is_expected.to have_many(:adjustments).dependent(nil) } end describe '#code=' do From 63cd8ccf286ae7d19aa4bda3d67eea5dad0458d1 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Thu, 7 Sep 2023 14:00:26 +0200 Subject: [PATCH 4/9] Test scenario where a voucher is deactivated before end of checkout A customer should be able to complete the checkout even if the voucher has been deactivated after being added to the order. --- spec/system/consumer/split_checkout_spec.rb | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 1ee0350fae..c7c9b93f53 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -1134,16 +1134,29 @@ describe "As a consumer, I want to checkout my order" do before do add_voucher_to_order(voucher, order) - - visit checkout_step_path(:summary) end it "shows the applied voucher" do + visit checkout_step_path(:summary) + within ".summary-right" do expect(page).to have_content "some_code" expect(page).to have_content "-6" end end + + context "with voucher deactivated after being added to an order" do + it "completes the order" do + visit checkout_step_path(:summary) + + # Deactivate voucher + voucher.destroy + + place_order + + expect(order.reload.state).to eq "complete" + end + end end end From f225cd78dfb9864b50622d5055ef577d322ae025 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Thu, 7 Sep 2023 15:13:47 +0200 Subject: [PATCH 5/9] Handle unique voucher code exception Rails validation doesn't handle unique validation for soft deleted object. So we rescue the exception raise by the database and display a nice error message. We don't want an enterprise to be able to reuse a code in case the voucher get reactivated. --- app/controllers/admin/vouchers_controller.rb | 5 +++++ spec/requests/admin/vouchers_spec.rb | 14 ++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/app/controllers/admin/vouchers_controller.rb b/app/controllers/admin/vouchers_controller.rb index 33093bb07d..72bb759e10 100644 --- a/app/controllers/admin/vouchers_controller.rb +++ b/app/controllers/admin/vouchers_controller.rb @@ -22,6 +22,11 @@ module Admin rescue ActiveRecord::SubclassNotFound @voucher.errors.add(:type) render_error + rescue ActiveRecord::RecordNotUnique + # Rails unique validation doesn't work with soft deleted object, so we rescue the database + # exception to display a nice message to the user + @voucher.errors.add(:code, :taken) + render_error end private diff --git a/spec/requests/admin/vouchers_spec.rb b/spec/requests/admin/vouchers_spec.rb index 86242e0ef1..f3ae69d573 100644 --- a/spec/requests/admin/vouchers_spec.rb +++ b/spec/requests/admin/vouchers_spec.rb @@ -81,6 +81,20 @@ describe "/admin/enterprises/:enterprise_id/vouchers", type: :request do end end + context "with a code used by a deactivated voucher" do + before do + voucher = create(:voucher, code:, enterprise:) + voucher.destroy + end + + it "render the new page with a code taken error" do + create_voucher + + expect(response).to render_template("admin/vouchers/new") + expect(flash[:error]).to eq("Code has already been taken") + end + end + it "redirects to admin enterprise setting page, voucher panel" do create_voucher From 48956b9bd1003ac379f9dec6076223392a190ca6 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 11 Sep 2023 11:59:34 +0200 Subject: [PATCH 6/9] As per review comment, simplify voucher view a little --- .../admin/enterprises/form/_vouchers.html.haml | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/app/views/admin/enterprises/form/_vouchers.html.haml b/app/views/admin/enterprises/form/_vouchers.html.haml index 5aa9ba9dd4..2e3912475a 100644 --- a/app/views/admin/enterprises/form/_vouchers.html.haml +++ b/app/views/admin/enterprises/form/_vouchers.html.haml @@ -3,7 +3,7 @@ = t('.add_new') %br -- if @enterprise.vouchers.present? || @enterprise.vouchers.only_deleted.present? +- if @enterprise.vouchers.with_deleted.present? %table %thead %tr @@ -17,7 +17,7 @@ /%th= t('.customers') /%th= t('.net_value') %tbody - - @enterprise.vouchers.each do |voucher| + - @enterprise.vouchers.with_deleted.order(deleted_at: :desc, code: :asc).each do |voucher| %tr %td= voucher.code %td= voucher.display_value @@ -27,17 +27,6 @@ /%td /%td /%td - - @enterprise.vouchers.only_deleted.each do |voucher| - %tr - %td= voucher.code - %td= voucher.display_value - %td= f.check_box :voucher_ids, { :multiple => true }, voucher.id, nil - /%td - /%td - /%td - /%td - /%td - /%td - else %p.text-center = t('.no_voucher_yet') From e44a0092eaba2eb304df0989058bedbf586f6457 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 11 Sep 2023 12:00:48 +0200 Subject: [PATCH 7/9] Fix edge case when trying to deactivate the only availabe voucher Plus spec --- app/controllers/admin/enterprises_controller.rb | 2 +- .../admin/enterprises_controller_spec.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/controllers/admin/enterprises_controller.rb b/app/controllers/admin/enterprises_controller.rb index 775d00840b..d7ad6d48c1 100644 --- a/app/controllers/admin/enterprises_controller.rb +++ b/app/controllers/admin/enterprises_controller.rb @@ -270,7 +270,7 @@ module Admin end def update_vouchers - return if params[:enterprise][:voucher_ids].blank? + params[:enterprise][:voucher_ids] = [] if params[:enterprise][:voucher_ids].blank? params_voucher_ids = params[:enterprise][:voucher_ids].map(&:to_i) voucher_ids = @enterprise.vouchers.map(&:id) diff --git a/spec/controllers/admin/enterprises_controller_spec.rb b/spec/controllers/admin/enterprises_controller_spec.rb index c68c04b82d..484901f83c 100644 --- a/spec/controllers/admin/enterprises_controller_spec.rb +++ b/spec/controllers/admin/enterprises_controller_spec.rb @@ -319,6 +319,19 @@ describe Admin::EnterprisesController, type: :controller do expect(voucher_a.reload.deleted?).to be(true) end + context "when desactivating the only existing voucher" do + it "deactivates the voucher" do + voucher_b.really_destroy! + + # Rails won't populate vouchers_id params if no voucher selected + spree_put :update, + id: enterprise, + enterprise: {} + + expect(voucher_a.reload.deleted?).to be(true) + end + end + context "when activating and deactivating voucher at the same time" do it "deactivates and activates accordingly" do voucher_c = create(:voucher, enterprise: enterprise, code: "voucher 3") From e7a52e4733f0c7a79ebc2f24b74f58d54930cd95 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou <40413322+rioug@users.noreply.github.com> Date: Thu, 14 Sep 2023 17:57:31 +1000 Subject: [PATCH 8/9] Update app/controllers/admin/enterprises_controller.rb Simplify code Co-authored-by: Maikel --- app/controllers/admin/enterprises_controller.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/controllers/admin/enterprises_controller.rb b/app/controllers/admin/enterprises_controller.rb index d7ad6d48c1..6efac571b0 100644 --- a/app/controllers/admin/enterprises_controller.rb +++ b/app/controllers/admin/enterprises_controller.rb @@ -270,9 +270,7 @@ module Admin end def update_vouchers - params[:enterprise][:voucher_ids] = [] if params[:enterprise][:voucher_ids].blank? - - params_voucher_ids = params[:enterprise][:voucher_ids].map(&:to_i) + params_voucher_ids = params[:enterprise][:voucher_ids].to_a.map(&:to_i) voucher_ids = @enterprise.vouchers.map(&:id) deleted_voucher_ids = @enterprise.vouchers.only_deleted.map(&:id) From fb93aeb1956850efe4fae7f5f94e3298f3cb1218 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 19 Sep 2023 13:39:36 +1000 Subject: [PATCH 9/9] Optimise spec --- spec/controllers/admin/enterprises_controller_spec.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/spec/controllers/admin/enterprises_controller_spec.rb b/spec/controllers/admin/enterprises_controller_spec.rb index 484901f83c..12e5915db4 100644 --- a/spec/controllers/admin/enterprises_controller_spec.rb +++ b/spec/controllers/admin/enterprises_controller_spec.rb @@ -291,7 +291,7 @@ describe Admin::EnterprisesController, type: :controller do describe "vouchers" do let(:enterprise) { create(:distributor_enterprise) } let!(:voucher_a) { create(:voucher, enterprise: enterprise, code: "voucher 1") } - let!(:voucher_b) { create(:voucher, enterprise: enterprise, code: "voucher 2") } + let(:voucher_b) { create(:voucher, enterprise: enterprise, code: "voucher 2") } before do controller_login_as_enterprise_user [enterprise] @@ -319,10 +319,8 @@ describe Admin::EnterprisesController, type: :controller do expect(voucher_a.reload.deleted?).to be(true) end - context "when desactivating the only existing voucher" do + context "when deactivating the only existing voucher" do it "deactivates the voucher" do - voucher_b.really_destroy! - # Rails won't populate vouchers_id params if no voucher selected spree_put :update, id: enterprise, @@ -334,6 +332,7 @@ describe Admin::EnterprisesController, type: :controller do context "when activating and deactivating voucher at the same time" do it "deactivates and activates accordingly" do + voucher_b voucher_c = create(:voucher, enterprise: enterprise, code: "voucher 3") voucher_c.destroy