From 06f986ff52ce359e398e4eeba18cd838a1cc1ffb Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 8 Aug 2023 16:13:04 +1000 Subject: [PATCH 1/7] Allow for creating a voucher with either flat or percentage rate In the scenario when you get an error when trying to create a percentage voucher, on the subsequent try we would be dealing with a "percentage rate voucher". The code now handle any type of voucher --- app/controllers/admin/vouchers_controller.rb | 12 ++++++++++-- spec/requests/admin/vouchers_controller_spec.rb | 9 +++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/vouchers_controller.rb b/app/controllers/admin/vouchers_controller.rb index 01ca689b91..cad08ab833 100644 --- a/app/controllers/admin/vouchers_controller.rb +++ b/app/controllers/admin/vouchers_controller.rb @@ -9,9 +9,13 @@ module Admin end def create + # In the scenario where you get an error when trying to create a percentage voucher, we'll + # now have percentage rate voucher instanciated. Hence why we check for both params type + voucher_type = params.dig(:vouchers_flat_rate, :voucher_type) || + params[:vouchers_percentage_rate][:voucher_type] + # The use of "safe_constantize" here will trigger a Brakeman error, it can safely be ignored # as it's a false positive : https://github.com/openfoodfoundation/openfoodnetwork/pull/10821 - voucher_type = params[:vouchers_flat_rate][:voucher_type] if Voucher::TYPES.include?(voucher_type) @voucher = voucher_type.safe_constantize.create( permitted_resource_params.merge(enterprise: @enterprise) @@ -44,7 +48,11 @@ module Admin end def permitted_resource_params - params.require(:vouchers_flat_rate).permit(:code, :amount) + if params[:vouchers_flat_rate].present? + return params.require(:vouchers_flat_rate).permit(:code, :amount) + end + + params.require(:vouchers_percentage_rate).permit(:code, :amount) end end end diff --git a/spec/requests/admin/vouchers_controller_spec.rb b/spec/requests/admin/vouchers_controller_spec.rb index 60b0f34e17..935255fff7 100644 --- a/spec/requests/admin/vouchers_controller_spec.rb +++ b/spec/requests/admin/vouchers_controller_spec.rb @@ -50,6 +50,15 @@ describe Admin::VouchersController, type: :request do end context "with a percentage rate voucher" do + let(:params) do + { + vouchers_percentage_rate: { + code: code, + amount: amount, + voucher_type: type + } + } + end let(:type) { "Vouchers::PercentageRate" } it "creates a new voucher" do From 4d5e1ffb3be52f23d366389c14928c6491cc9172 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 14 Aug 2023 11:39:59 +1000 Subject: [PATCH 2/7] Simplify VouchersController with single param name The Voucher form now deals with a generic Voucher instead of a subclass which makes the naming easier and is less confusing when changing types. --- app/controllers/admin/vouchers_controller.rb | 9 ++------- app/views/admin/vouchers/new.html.haml | 2 +- spec/requests/admin/vouchers_controller_spec.rb | 4 ++-- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/app/controllers/admin/vouchers_controller.rb b/app/controllers/admin/vouchers_controller.rb index cad08ab833..6a73bd7787 100644 --- a/app/controllers/admin/vouchers_controller.rb +++ b/app/controllers/admin/vouchers_controller.rb @@ -11,8 +11,7 @@ module Admin def create # In the scenario where you get an error when trying to create a percentage voucher, we'll # now have percentage rate voucher instanciated. Hence why we check for both params type - voucher_type = params.dig(:vouchers_flat_rate, :voucher_type) || - params[:vouchers_percentage_rate][:voucher_type] + voucher_type = params.dig(:voucher, :voucher_type) # The use of "safe_constantize" here will trigger a Brakeman error, it can safely be ignored # as it's a false positive : https://github.com/openfoodfoundation/openfoodnetwork/pull/10821 @@ -48,11 +47,7 @@ module Admin end def permitted_resource_params - if params[:vouchers_flat_rate].present? - return params.require(:vouchers_flat_rate).permit(:code, :amount) - end - - params.require(:vouchers_percentage_rate).permit(:code, :amount) + params.require(:voucher).permit(:code, :amount) end end end diff --git a/app/views/admin/vouchers/new.html.haml b/app/views/admin/vouchers/new.html.haml index a27ea65d7a..77eff6d903 100644 --- a/app/views/admin/vouchers/new.html.haml +++ b/app/views/admin/vouchers/new.html.haml @@ -1,4 +1,4 @@ -= form_with model: @voucher, url: admin_enterprise_vouchers_path(@enterprise), html: { name: "voucher_form" } do |f| += form_with model: @voucher.becomes(Voucher), url: admin_enterprise_vouchers_path(@enterprise), html: { name: "voucher_form" } do |f| .row .sixteen.columns.alpha .four.columns.alpha.text-right diff --git a/spec/requests/admin/vouchers_controller_spec.rb b/spec/requests/admin/vouchers_controller_spec.rb index 935255fff7..04e4c001c5 100644 --- a/spec/requests/admin/vouchers_controller_spec.rb +++ b/spec/requests/admin/vouchers_controller_spec.rb @@ -26,7 +26,7 @@ describe Admin::VouchersController, type: :request do let(:params) do { - vouchers_flat_rate: { + voucher: { code: code, amount: amount, voucher_type: type @@ -52,7 +52,7 @@ describe Admin::VouchersController, type: :request do context "with a percentage rate voucher" do let(:params) do { - vouchers_percentage_rate: { + voucher: { code: code, amount: amount, voucher_type: type From efe2dfff8e5b92eef8346221dc2c619ee04110c6 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 14 Aug 2023 11:51:34 +1000 Subject: [PATCH 3/7] Let Rails handle Voucher type building --- app/controllers/admin/vouchers_controller.rb | 22 ++++++------------- app/views/admin/vouchers/new.html.haml | 4 ++-- .../admin/vouchers_controller_spec.rb | 4 ++-- 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/app/controllers/admin/vouchers_controller.rb b/app/controllers/admin/vouchers_controller.rb index 6a73bd7787..33093bb07d 100644 --- a/app/controllers/admin/vouchers_controller.rb +++ b/app/controllers/admin/vouchers_controller.rb @@ -9,20 +9,9 @@ module Admin end def create - # In the scenario where you get an error when trying to create a percentage voucher, we'll - # now have percentage rate voucher instanciated. Hence why we check for both params type - voucher_type = params.dig(:voucher, :voucher_type) - - # The use of "safe_constantize" here will trigger a Brakeman error, it can safely be ignored - # as it's a false positive : https://github.com/openfoodfoundation/openfoodnetwork/pull/10821 - if Voucher::TYPES.include?(voucher_type) - @voucher = voucher_type.safe_constantize.create( - permitted_resource_params.merge(enterprise: @enterprise) - ) - else - @voucher.errors.add(:type) - return render_error - end + @voucher = Voucher.new( + permitted_resource_params.merge(enterprise: @enterprise) + ) if @voucher.save flash[:success] = I18n.t(:successfully_created, resource: "Voucher") @@ -30,6 +19,9 @@ module Admin else render_error end + rescue ActiveRecord::SubclassNotFound + @voucher.errors.add(:type) + render_error end private @@ -47,7 +39,7 @@ module Admin end def permitted_resource_params - params.require(:voucher).permit(:code, :amount) + params.require(:voucher).permit(:code, :amount, :type) end end end diff --git a/app/views/admin/vouchers/new.html.haml b/app/views/admin/vouchers/new.html.haml index 77eff6d903..de23acf867 100644 --- a/app/views/admin/vouchers/new.html.haml +++ b/app/views/admin/vouchers/new.html.haml @@ -17,9 +17,9 @@ = f.text_field :code, class: 'fullwidth' .row .alpha.four.columns - = f.label :voucher_type, t('.voucher_type') + = f.label :type, t('.voucher_type') .omega.eight.columns - = f.select :voucher_type, options_for_select(Voucher::TYPES.map { |type| [t(".#{type.demodulize.underscore}"), type] }, @voucher.class.to_s) + = f.select :type, options_for_select(Voucher::TYPES.map { |type| [t(".#{type.demodulize.underscore}"), type] }, @voucher.class.to_s) .row .alpha.four.columns = f.label :amount, t('.voucher_amount') diff --git a/spec/requests/admin/vouchers_controller_spec.rb b/spec/requests/admin/vouchers_controller_spec.rb index 04e4c001c5..80a6ca78fc 100644 --- a/spec/requests/admin/vouchers_controller_spec.rb +++ b/spec/requests/admin/vouchers_controller_spec.rb @@ -29,7 +29,7 @@ describe Admin::VouchersController, type: :request do voucher: { code: code, amount: amount, - voucher_type: type + type: type } } end @@ -55,7 +55,7 @@ describe Admin::VouchersController, type: :request do voucher: { code: code, amount: amount, - voucher_type: type + type: type } } end From 4b0e910dff8c3745e954ee15949341ed48342bc7 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 14 Aug 2023 12:23:00 +1000 Subject: [PATCH 4/7] Simplify default values in Voucher form --- app/views/admin/vouchers/new.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/admin/vouchers/new.html.haml b/app/views/admin/vouchers/new.html.haml index de23acf867..6956f809bc 100644 --- a/app/views/admin/vouchers/new.html.haml +++ b/app/views/admin/vouchers/new.html.haml @@ -19,9 +19,9 @@ .alpha.four.columns = f.label :type, t('.voucher_type') .omega.eight.columns - = f.select :type, options_for_select(Voucher::TYPES.map { |type| [t(".#{type.demodulize.underscore}"), type] }, @voucher.class.to_s) + = f.select :type, options_for_select(Voucher::TYPES.map { |type| [t(".#{type.demodulize.underscore}"), type] }, @voucher.type) .row .alpha.four.columns = f.label :amount, t('.voucher_amount') .omega.eight.columns - = f.text_field :amount, value: @voucher.amount + = f.text_field :amount From 160cdc7f8e1ce465e8c7a1ef6bd329da804c7593 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 14 Aug 2023 12:25:56 +1000 Subject: [PATCH 5/7] Clarify Voucher request spec not a controller spec --- .../admin/{vouchers_controller_spec.rb => vouchers_spec.rb} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename spec/requests/admin/{vouchers_controller_spec.rb => vouchers_spec.rb} (96%) diff --git a/spec/requests/admin/vouchers_controller_spec.rb b/spec/requests/admin/vouchers_spec.rb similarity index 96% rename from spec/requests/admin/vouchers_controller_spec.rb rename to spec/requests/admin/vouchers_spec.rb index 80a6ca78fc..a7665bc7d3 100644 --- a/spec/requests/admin/vouchers_controller_spec.rb +++ b/spec/requests/admin/vouchers_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -describe Admin::VouchersController, type: :request do +describe "/admin/enterprises/:enterprise_id/vouchers", type: :request do let(:enterprise) { create(:supplier_enterprise, name: "Feedme") } let(:enterprise_user) { create(:user, enterprise_limit: 1) } From ba53f3130994fb3197ed993b2c3af62ec34d78b5 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 14 Aug 2023 12:40:54 +1000 Subject: [PATCH 6/7] Use new hash syntax in Voucher spec --- .rubocop_todo.yml | 1 - spec/requests/admin/vouchers_spec.rb | 16 ++++++++-------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 92f25209a2..ab252f121b 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1322,7 +1322,6 @@ Style/HashSyntax: - 'spec/queries/customers_with_balance_spec.rb' - 'spec/queries/outstanding_balance_spec.rb' - 'spec/queries/payments_requiring_action_spec.rb' - - 'spec/requests/admin/vouchers_controller_spec.rb' - 'spec/requests/api/v1/customers_spec.rb' - 'spec/requests/checkout/failed_checkout_spec.rb' - 'spec/requests/checkout/paypal_spec.rb' diff --git a/spec/requests/admin/vouchers_spec.rb b/spec/requests/admin/vouchers_spec.rb index a7665bc7d3..86242e0ef1 100644 --- a/spec/requests/admin/vouchers_spec.rb +++ b/spec/requests/admin/vouchers_spec.rb @@ -9,7 +9,7 @@ describe "/admin/enterprises/:enterprise_id/vouchers", type: :request do before do Flipper.enable(:vouchers) - enterprise_user.enterprise_roles.build(enterprise: enterprise).save + enterprise_user.enterprise_roles.build(enterprise:).save sign_in enterprise_user end @@ -22,14 +22,14 @@ describe "/admin/enterprises/:enterprise_id/vouchers", type: :request do end describe "POST /admin/enterprises/:enterprise_id/vouchers" do - subject(:create_voucher) { post admin_enterprise_vouchers_path(enterprise), params: params } + subject(:create_voucher) { post admin_enterprise_vouchers_path(enterprise), params: } let(:params) do { voucher: { - code: code, - amount: amount, - type: type + code:, + amount:, + type: } } end @@ -53,9 +53,9 @@ describe "/admin/enterprises/:enterprise_id/vouchers", type: :request do let(:params) do { voucher: { - code: code, - amount: amount, - type: type + code:, + amount:, + type: } } end From 91e4fb18ed662d67714d2539e09eefb861474f38 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 14 Aug 2023 12:46:13 +1000 Subject: [PATCH 7/7] Use input labels in Voucher system spec It's more robust, and closer to what the user sees and does. It's also verifying that the labels are correctly connected to the inputs. --- spec/system/admin/vouchers_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/system/admin/vouchers_spec.rb b/spec/system/admin/vouchers_spec.rb index 68b7fe83f3..6c113ec693 100644 --- a/spec/system/admin/vouchers_spec.rb +++ b/spec/system/admin/vouchers_spec.rb @@ -50,9 +50,9 @@ describe ' context "with a flat rate voucher" do it 'creates a voucher' do # And I fill in the fields for a new voucher click save - fill_in 'vouchers_flat_rate_code', with: voucher_code - select "Flat", from: "vouchers_flat_rate_voucher_type" - fill_in 'vouchers_flat_rate_amount', with: amount + fill_in "Voucher Code", with: voucher_code + select "Flat", from: "Voucher Type" + fill_in "Amount", with: amount click_button 'Save' # Then I should get redirect to the entreprise voucher tab and see the created voucher @@ -64,9 +64,9 @@ describe ' context "with a percentage rate voucher" do it 'creates a voucher' do # And I fill in the fields for a new voucher click save - fill_in 'vouchers_flat_rate_code', with: voucher_code - select "Percentage (%)", from: "vouchers_flat_rate_voucher_type" - fill_in 'vouchers_flat_rate_amount', with: amount + fill_in "Voucher Code", with: voucher_code + select "Percentage (%)", from: "Voucher Type" + fill_in "Amount", with: amount click_button 'Save' # Then I should get redirect to the entreprise voucher tab and see the created voucher