From e59077e63e6daff67fd5c6a235a4cbb76e59dfcf Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Wed, 18 Dec 2019 09:54:23 +0100 Subject: [PATCH 1/2] Select by default single Hub/Shop option on creation of payment/shipping method - added a helper - added mode(new/edit) in payment/shipping views - updated checkbox creation - added tests --- app/helpers/spree/admin/base_helper_decorator.rb | 8 ++++++++ .../spree/admin/payment_methods/edit.html.haml | 2 +- .../spree/admin/payment_methods/new.html.haml | 2 +- .../spree/admin/shared/_hubs_sidebar.html.haml | 2 +- .../spree/admin/shipping_methods/edit.html.haml | 2 +- .../spree/admin/shipping_methods/new.html.haml | 2 +- spec/features/admin/payment_method_spec.rb | 15 +++++++++++++++ spec/features/admin/shipping_methods_spec.rb | 14 ++++++++++++++ 8 files changed, 42 insertions(+), 5 deletions(-) diff --git a/app/helpers/spree/admin/base_helper_decorator.rb b/app/helpers/spree/admin/base_helper_decorator.rb index 95016b3c88..88a835bfa0 100644 --- a/app/helpers/spree/admin/base_helper_decorator.rb +++ b/app/helpers/spree/admin/base_helper_decorator.rb @@ -21,6 +21,14 @@ module Spree link_to_with_icon('icon-trash', name, '#', html_options) + f.hidden_field(:_destroy) end + + def add_check_if_single(mode, count) + if mode == :new && count == 1 + { checked: true } + else + {} + end + end end end end diff --git a/app/views/spree/admin/payment_methods/edit.html.haml b/app/views/spree/admin/payment_methods/edit.html.haml index 9d8f6b147d..7ddac37d93 100644 --- a/app/views/spree/admin/payment_methods/edit.html.haml +++ b/app/views/spree/admin/payment_methods/edit.html.haml @@ -12,7 +12,7 @@ %fieldset.no-border-top = render partial: 'form', locals: { f: f } .one.column   - = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :payment_method } + = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :payment_method, mode: :edit } .clear .filter-actions.actions = button t(:update), 'icon-refresh' diff --git a/app/views/spree/admin/payment_methods/new.html.haml b/app/views/spree/admin/payment_methods/new.html.haml index c3039cc169..a9752371af 100644 --- a/app/views/spree/admin/payment_methods/new.html.haml +++ b/app/views/spree/admin/payment_methods/new.html.haml @@ -8,7 +8,7 @@ %fieldset.no-border-top = render partial: 'form', locals: { f: f } .one.column   - = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :payment_method } + = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :payment_method, mode: :new } .clear .filter-actions.actions = button t(:create), 'icon-ok' diff --git a/app/views/spree/admin/shared/_hubs_sidebar.html.haml b/app/views/spree/admin/shared/_hubs_sidebar.html.haml index 75ef9bb32f..306827ce6a 100644 --- a/app/views/spree/admin/shared/_hubs_sidebar.html.haml +++ b/app/views/spree/admin/shared/_hubs_sidebar.html.haml @@ -12,7 +12,7 @@ %span.four.columns %span.three.columns.alpha %label - = check_box klass, :distributor_ids, { multiple: true }, hub.id, nil + = check_box klass, :distributor_ids, { multiple: true }.merge(add_check_if_single(mode, @hubs.count)), hub.id, nil = hub.name %a.one.column.omega{ href: "#{main_app.edit_admin_enterprise_path(hub)}" } %span.icon-arrow-right diff --git a/app/views/spree/admin/shipping_methods/edit.html.haml b/app/views/spree/admin/shipping_methods/edit.html.haml index dc6d879e45..26eadb3afd 100644 --- a/app/views/spree/admin/shipping_methods/edit.html.haml +++ b/app/views/spree/admin/shipping_methods/edit.html.haml @@ -12,7 +12,7 @@ %fieldset.no-border-top = render partial: 'form', locals: { f: f } .one.column   - = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :shipping_method } + = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :shipping_method, mode: :edit } .clear %div = render partial: 'spree/admin/shared/edit_resource_links' diff --git a/app/views/spree/admin/shipping_methods/new.html.haml b/app/views/spree/admin/shipping_methods/new.html.haml index ab7c444fda..935ee463fb 100644 --- a/app/views/spree/admin/shipping_methods/new.html.haml +++ b/app/views/spree/admin/shipping_methods/new.html.haml @@ -10,7 +10,7 @@ %fieldset.no-border-top = render partial: 'form', locals: { f: f } .one.column   - = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :shipping_method } + = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :shipping_method, mode: :new } .clear %div = render partial: 'spree/admin/shared/new_resource_links' diff --git a/spec/features/admin/payment_method_spec.rb b/spec/features/admin/payment_method_spec.rb index 439d9ea0db..0d765f7b59 100644 --- a/spec/features/admin/payment_method_spec.rb +++ b/spec/features/admin/payment_method_spec.rb @@ -67,6 +67,21 @@ feature ' expect(page).to have_selector "#stripe-account-status .charges_enabled", text: "Charges Enabled: Yes" end end + + scenario "checking a single distributor is checked by default" do + 2.times.each { Enterprise.last.destroy } + quick_login_as_admin + visit spree.new_admin_payment_method_path + expect(page).to have_field "payment_method_distributor_ids_#{@distributors[0].id}", checked: true + end + + scenario "checking more than a distributor displays no default choice" do + quick_login_as_admin + visit spree.new_admin_payment_method_path + expect(page).to have_field "payment_method_distributor_ids_#{@distributors[0].id}", checked: false + expect(page).to have_field "payment_method_distributor_ids_#{@distributors[1].id}", checked: false + expect(page).to have_field "payment_method_distributor_ids_#{@distributors[2].id}", checked: false + end end scenario "updating a payment method", js: true do diff --git a/spec/features/admin/shipping_methods_spec.rb b/spec/features/admin/shipping_methods_spec.rb index ca00af3457..9f413a0935 100644 --- a/spec/features/admin/shipping_methods_spec.rb +++ b/spec/features/admin/shipping_methods_spec.rb @@ -65,6 +65,20 @@ feature 'shipping methods' do expect(page).to have_content "That shipping method cannot be deleted as it is referenced by an order: #{o.number}." expect(Spree::ShippingMethod.find(@sm.id)).not_to be_nil end + + scenario "checking a single distributor is checked by default" do + d1 = Enterprise.first + visit spree.new_admin_shipping_method_path + expect(page).to have_field "shipping_method_distributor_ids_#{d1.id}", checked: true + end + + scenario "checking more than a distributor displays no default choice" do + d1 = create(:distributor_enterprise, name: 'Aeronautical Adventures') + d2 = create(:distributor_enterprise, name: 'Nautical Travels') + visit spree.new_admin_shipping_method_path + expect(page).to have_field "shipping_method_distributor_ids_#{d1.id}", checked: false + expect(page).to have_field "shipping_method_distributor_ids_#{d2.id}", checked: false + end end context "as an enterprise user", js: true do From e6d9ec7bd7674b2c800e3e7764dc3f9ed636727c Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Thu, 19 Dec 2019 21:08:34 +0100 Subject: [PATCH 2/2] Small fixes for default single Hub/Shop options issue - removed mode variable - reverted html template accordingly - added a more specific helper - fixed some short variable names --- app/helpers/admin/enterprises_helper.rb | 11 +++ .../spree/admin/base_helper_decorator.rb | 8 --- .../admin/payment_methods/edit.html.haml | 2 +- .../spree/admin/payment_methods/new.html.haml | 2 +- .../admin/shared/_hubs_sidebar.html.haml | 2 +- .../admin/shipping_methods/edit.html.haml | 2 +- .../admin/shipping_methods/new.html.haml | 2 +- spec/features/admin/payment_method_spec.rb | 38 +++++----- spec/features/admin/shipping_methods_spec.rb | 72 +++++++++---------- 9 files changed, 71 insertions(+), 68 deletions(-) create mode 100644 app/helpers/admin/enterprises_helper.rb diff --git a/app/helpers/admin/enterprises_helper.rb b/app/helpers/admin/enterprises_helper.rb new file mode 100644 index 0000000000..4f4bcde0f6 --- /dev/null +++ b/app/helpers/admin/enterprises_helper.rb @@ -0,0 +1,11 @@ +module Admin + module EnterprisesHelper + def add_check_if_single(count) + if count == 1 + { checked: true } + else + {} + end + end + end +end diff --git a/app/helpers/spree/admin/base_helper_decorator.rb b/app/helpers/spree/admin/base_helper_decorator.rb index 88a835bfa0..95016b3c88 100644 --- a/app/helpers/spree/admin/base_helper_decorator.rb +++ b/app/helpers/spree/admin/base_helper_decorator.rb @@ -21,14 +21,6 @@ module Spree link_to_with_icon('icon-trash', name, '#', html_options) + f.hidden_field(:_destroy) end - - def add_check_if_single(mode, count) - if mode == :new && count == 1 - { checked: true } - else - {} - end - end end end end diff --git a/app/views/spree/admin/payment_methods/edit.html.haml b/app/views/spree/admin/payment_methods/edit.html.haml index 7ddac37d93..9d8f6b147d 100644 --- a/app/views/spree/admin/payment_methods/edit.html.haml +++ b/app/views/spree/admin/payment_methods/edit.html.haml @@ -12,7 +12,7 @@ %fieldset.no-border-top = render partial: 'form', locals: { f: f } .one.column   - = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :payment_method, mode: :edit } + = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :payment_method } .clear .filter-actions.actions = button t(:update), 'icon-refresh' diff --git a/app/views/spree/admin/payment_methods/new.html.haml b/app/views/spree/admin/payment_methods/new.html.haml index a9752371af..c3039cc169 100644 --- a/app/views/spree/admin/payment_methods/new.html.haml +++ b/app/views/spree/admin/payment_methods/new.html.haml @@ -8,7 +8,7 @@ %fieldset.no-border-top = render partial: 'form', locals: { f: f } .one.column   - = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :payment_method, mode: :new } + = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :payment_method } .clear .filter-actions.actions = button t(:create), 'icon-ok' diff --git a/app/views/spree/admin/shared/_hubs_sidebar.html.haml b/app/views/spree/admin/shared/_hubs_sidebar.html.haml index 306827ce6a..e1c1c3577d 100644 --- a/app/views/spree/admin/shared/_hubs_sidebar.html.haml +++ b/app/views/spree/admin/shared/_hubs_sidebar.html.haml @@ -12,7 +12,7 @@ %span.four.columns %span.three.columns.alpha %label - = check_box klass, :distributor_ids, { multiple: true }.merge(add_check_if_single(mode, @hubs.count)), hub.id, nil + = check_box klass, :distributor_ids, { multiple: true }.merge(add_check_if_single(@hubs.count)), hub.id, nil = hub.name %a.one.column.omega{ href: "#{main_app.edit_admin_enterprise_path(hub)}" } %span.icon-arrow-right diff --git a/app/views/spree/admin/shipping_methods/edit.html.haml b/app/views/spree/admin/shipping_methods/edit.html.haml index 26eadb3afd..dc6d879e45 100644 --- a/app/views/spree/admin/shipping_methods/edit.html.haml +++ b/app/views/spree/admin/shipping_methods/edit.html.haml @@ -12,7 +12,7 @@ %fieldset.no-border-top = render partial: 'form', locals: { f: f } .one.column   - = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :shipping_method, mode: :edit } + = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :shipping_method } .clear %div = render partial: 'spree/admin/shared/edit_resource_links' diff --git a/app/views/spree/admin/shipping_methods/new.html.haml b/app/views/spree/admin/shipping_methods/new.html.haml index 935ee463fb..ab7c444fda 100644 --- a/app/views/spree/admin/shipping_methods/new.html.haml +++ b/app/views/spree/admin/shipping_methods/new.html.haml @@ -10,7 +10,7 @@ %fieldset.no-border-top = render partial: 'form', locals: { f: f } .one.column   - = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :shipping_method, mode: :new } + = render partial: 'spree/admin/shared/hubs_sidebar', locals: { f: f, klass: :shipping_method } .clear %div = render partial: 'spree/admin/shared/new_resource_links' diff --git a/spec/features/admin/payment_method_spec.rb b/spec/features/admin/payment_method_spec.rb index 0d765f7b59..ea741fe004 100644 --- a/spec/features/admin/payment_method_spec.rb +++ b/spec/features/admin/payment_method_spec.rb @@ -85,10 +85,10 @@ feature ' end scenario "updating a payment method", js: true do - pm = create(:payment_method, distributors: [@distributors[0]]) + payment_method = create(:payment_method, distributors: [@distributors[0]]) quick_login_as_admin - visit spree.edit_admin_payment_method_path pm + visit spree.edit_admin_payment_method_path payment_method fill_in 'payment_method_name', with: 'New PM Name' find(:css, "tags-input .tags input").set "member\n" @@ -135,9 +135,9 @@ feature ' let(:distributor1) { create(:distributor_enterprise, name: 'First Distributor') } let(:distributor2) { create(:distributor_enterprise, name: 'Second Distributor') } let(:distributor3) { create(:distributor_enterprise, name: 'Third Distributor') } - let(:pm1) { create(:payment_method, name: 'One', distributors: [distributor1]) } - let(:pm2) { create(:payment_method, name: 'Two', distributors: [distributor1, distributor2]) } - let(:pm3) { create(:payment_method, name: 'Three', distributors: [distributor3]) } + let(:payment_method1) { create(:payment_method, name: 'One', distributors: [distributor1]) } + let(:payment_method2) { create(:payment_method, name: 'Two', distributors: [distributor1, distributor2]) } + let(:payment_method3) { create(:payment_method, name: 'Three', distributors: [distributor3]) } before(:each) do enterprise_user.enterprise_roles.build(enterprise: distributor1).save @@ -172,28 +172,28 @@ feature ' end it "shows me only payment methods I have access to" do - pm1 - pm2 - pm3 + payment_method1 + payment_method2 + payment_method3 visit spree.admin_payment_methods_path - expect(page).to have_content pm1.name - expect(page).to have_content pm2.name - expect(page).not_to have_content pm3.name + expect(page).to have_content payment_method1.name + expect(page).to have_content payment_method2.name + expect(page).not_to have_content payment_method3.name end it "does not show duplicates of payment methods" do - pm1 - pm2 + payment_method1 + payment_method2 visit spree.admin_payment_methods_path expect(page).to have_selector 'td', text: 'Two', count: 1 end pending "shows me only payment methods for the enterprise I select" do - pm1 - pm2 + payment_method1 + payment_method2 visit admin_enterprises_path within("#e_#{distributor1.id}") { click_link 'Settings' } @@ -201,8 +201,8 @@ feature ' click_link "Payment Methods" end - expect(page).to have_content pm1.name - expect(page).to have_content pm2.name + expect(page).to have_content payment_method1.name + expect(page).to have_content payment_method2.name click_link 'Enterprises' within("#e_#{distributor2.id}") { click_link 'Settings' } @@ -210,8 +210,8 @@ feature ' click_link "Payment Methods" end - expect(page).not_to have_content pm1.name - expect(page).to have_content pm2.name + expect(page).not_to have_content payment_method1.name + expect(page).to have_content payment_method2.name end end end diff --git a/spec/features/admin/shipping_methods_spec.rb b/spec/features/admin/shipping_methods_spec.rb index 9f413a0935..1fe1658988 100644 --- a/spec/features/admin/shipping_methods_spec.rb +++ b/spec/features/admin/shipping_methods_spec.rb @@ -5,7 +5,7 @@ feature 'shipping methods' do include WebHelper before :each do - @sm = create(:shipping_method) + @shipping_method = create(:shipping_method) end context "as a site admin" do @@ -15,8 +15,8 @@ feature 'shipping methods' do scenario "creating a shipping method owned by some distributors" do # Given some distributors - d1 = create(:distributor_enterprise, name: 'Aeronautical Adventures') - d2 = create(:distributor_enterprise, name: 'Nautical Travels') + distributor1 = create(:distributor_enterprise, name: 'Alice Farm Hub') + distributor2 = create(:distributor_enterprise, name: 'Bob Farm Shop') # Shows appropriate fields when logged in as admin visit spree.new_admin_shipping_method_path @@ -28,8 +28,8 @@ feature 'shipping methods' do # When I create a shipping method and set the distributors fill_in 'shipping_method_name', with: 'Carrier Pidgeon' - check "shipping_method_distributor_ids_#{d1.id}" - check "shipping_method_distributor_ids_#{d2.id}" + check "shipping_method_distributor_ids_#{distributor1.id}" + check "shipping_method_distributor_ids_#{distributor2.id}" check "shipping_method_shipping_categories_" click_button I18n.t("actions.create") @@ -41,43 +41,43 @@ feature 'shipping methods' do sm = Spree::ShippingMethod.last expect(sm.name).to eq('Carrier Pidgeon') - expect(sm.distributors).to match_array [d1, d2] + expect(sm.distributors).to match_array [distributor1, distributor2] end it "at checkout, user can only see shipping methods for their current distributor (checkout spec)" scenario "deleting a shipping method" do - visit_delete spree.admin_shipping_method_path(@sm) + visit_delete spree.admin_shipping_method_path(@shipping_method) - expect(page).to have_content "Shipping method \"#{@sm.name}\" has been successfully removed!" - expect(Spree::ShippingMethod.where(id: @sm.id)).to be_empty + expect(page).to have_content "Shipping method \"#{@shipping_method.name}\" has been successfully removed!" + expect(Spree::ShippingMethod.where(id: @shipping_method.id)).to be_empty end scenario "deleting a shipping method referenced by an order" do - o = create(:order) + order = create(:order) shipment = create(:shipment) - shipment.add_shipping_method(@sm, true) - o.shipments << shipment - o.save! + shipment.add_shipping_method(@shipping_method, true) + order.shipments << shipment + order.save! - visit_delete spree.admin_shipping_method_path(@sm) + visit_delete spree.admin_shipping_method_path(@shipping_method) - expect(page).to have_content "That shipping method cannot be deleted as it is referenced by an order: #{o.number}." - expect(Spree::ShippingMethod.find(@sm.id)).not_to be_nil + expect(page).to have_content "That shipping method cannot be deleted as it is referenced by an order: #{order.number}." + expect(Spree::ShippingMethod.find(@shipping_method.id)).not_to be_nil end scenario "checking a single distributor is checked by default" do - d1 = Enterprise.first + first_distributor = Enterprise.first visit spree.new_admin_shipping_method_path - expect(page).to have_field "shipping_method_distributor_ids_#{d1.id}", checked: true + expect(page).to have_field "shipping_method_distributor_ids_#{first_distributor.id}", checked: true end scenario "checking more than a distributor displays no default choice" do - d1 = create(:distributor_enterprise, name: 'Aeronautical Adventures') - d2 = create(:distributor_enterprise, name: 'Nautical Travels') + distributor1 = create(:distributor_enterprise, name: 'Alice Farm Shop') + distributor2 = create(:distributor_enterprise, name: 'Bob Farm Hub') visit spree.new_admin_shipping_method_path - expect(page).to have_field "shipping_method_distributor_ids_#{d1.id}", checked: false - expect(page).to have_field "shipping_method_distributor_ids_#{d2.id}", checked: false + expect(page).to have_field "shipping_method_distributor_ids_#{distributor1.id}", checked: false + expect(page).to have_field "shipping_method_distributor_ids_#{distributor2.id}", checked: false end end @@ -86,8 +86,8 @@ feature 'shipping methods' do let(:distributor1) { create(:distributor_enterprise, name: 'First Distributor') } let(:distributor2) { create(:distributor_enterprise, name: 'Second Distributor') } let(:distributor3) { create(:distributor_enterprise, name: 'Third Distributor') } - let(:sm1) { create(:shipping_method, name: 'One', distributors: [distributor1]) } - let(:sm2) { create(:shipping_method, name: 'Two', distributors: [distributor1, distributor2]) } + let(:shipping_method1) { create(:shipping_method, name: 'One', distributors: [distributor1]) } + let(:shipping_method2) { create(:shipping_method, name: 'Two', distributors: [distributor1, distributor2]) } let(:sm3) { create(:shipping_method, name: 'Three', distributors: [distributor3]) } let(:shipping_category) { create(:shipping_category) } @@ -136,20 +136,20 @@ feature 'shipping methods' do end it "shows me only shipping methods I have access to" do - sm1 - sm2 + shipping_method1 + shipping_method2 sm3 visit spree.admin_shipping_methods_path - expect(page).to have_content sm1.name - expect(page).to have_content sm2.name + expect(page).to have_content shipping_method1.name + expect(page).to have_content shipping_method2.name expect(page).not_to have_content sm3.name end it "does not show duplicates of shipping methods" do - sm1 - sm2 + shipping_method1 + shipping_method2 visit spree.admin_shipping_methods_path @@ -157,16 +157,16 @@ feature 'shipping methods' do end pending "shows me only shipping methods for the enterprise I select" do - sm1 - sm2 + shipping_method1 + shipping_method2 visit admin_enterprises_path within("#e_#{distributor1.id}") { click_link 'Settings' } within(".side_menu") do click_link "Shipping Methods" end - expect(page).to have_content sm1.name - expect(page).to have_content sm2.name + expect(page).to have_content shipping_method1.name + expect(page).to have_content shipping_method2.name click_link 'Enterprises' within("#e_#{distributor2.id}") { click_link 'Settings' } @@ -174,8 +174,8 @@ feature 'shipping methods' do click_link "Shipping Methods" end - expect(page).not_to have_content sm1.name - expect(page).to have_content sm2.name + expect(page).not_to have_content shipping_method1.name + expect(page).to have_content shipping_method2.name end end end