diff --git a/app/controllers/admin/enterprises_controller.rb b/app/controllers/admin/enterprises_controller.rb index ff9dcfaea8..db67df4406 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,18 @@ module Admin @enterprise.update_contact(user_id) end + def update_vouchers + 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) + + 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/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/app/models/voucher.rb b/app/models/voucher.rb index 8b678efd33..12ec455edf 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/app/views/admin/enterprises/form/_vouchers.html.haml b/app/views/admin/enterprises/form/_vouchers.html.haml index 8df72f951c..2e3912475a 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.with_deleted.present? %table %thead %tr %th= t('.voucher_code') %th= t('.rate') + %th= t('.active') /%th= t('.label') /%th= t('.purpose') /%th= t('.expiry') @@ -16,17 +17,16 @@ /%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 + %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 d966cd21c1..56aac72d3c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1239,6 +1239,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 b7054e694a..25bf52056f 100644 --- a/spec/controllers/admin/enterprises_controller_spec.rb +++ b/spec/controllers/admin/enterprises_controller_spec.rb @@ -281,12 +281,72 @@ 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 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 deactivating the only existing voucher" do + it "deactivates the voucher" do + # 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_b + 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 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 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 diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index f805eb81bc..b97489c7eb 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -1139,16 +1139,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