From 5760aed6a6671c47d4532b32aff2f7001343fcc1 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 29 Mar 2019 18:16:55 +1100 Subject: [PATCH 01/12] Render only eligible adjustments on the order page https://github.com/openfoodfoundation/openfoodnetwork/issues/3477 The admin orders edit form was displaying adjustments even if they were "not eligible". For example, an additional fee with amount `0` is not eligible. They were already hidden in the adjustments view and we are hiding them in reports. --- app/views/spree/admin/orders/_form.html.haml | 2 +- spec/features/admin/orders_spec.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/views/spree/admin/orders/_form.html.haml b/app/views/spree/admin/orders/_form.html.haml index 1b3c17a72f..bebe2dea1d 100644 --- a/app/views/spree/admin/orders/_form.html.haml +++ b/app/views/spree/admin/orders/_form.html.haml @@ -5,7 +5,7 @@ = render :partial => "spree/admin/orders/shipment", :collection => @order.shipments, :locals => { :order => order } = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.price_adjustments, :order => order, :title => Spree.t(:line_item_adjustments)} - = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.adjustments, :order => order, :title => Spree.t(:order_adjustments)} + = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.adjustments.eligible, :order => order, :title => Spree.t(:order_adjustments)} - if order.line_items.exists? %fieldset#order-total.no-border-bottom{"data-hook" => "order_details_total"} diff --git a/spec/features/admin/orders_spec.rb b/spec/features/admin/orders_spec.rb index 48072ddad8..dc8b654be8 100644 --- a/spec/features/admin/orders_spec.rb +++ b/spec/features/admin/orders_spec.rb @@ -283,6 +283,19 @@ feature %q{ end end + scenario "shows only eligible adjustments" do + adjustment = create( + :adjustment, + adjustable: @order, + label: "invalid adjustment", + amount: 0 + ) + + visit spree.edit_admin_order_path(@order) + + expect(page).to have_no_content adjustment.label + end + scenario "cannot split the order in different stock locations" do # There's only 1 stock location in OFN, so the split functionality that comes with spree should be hidden expect(page).to_not have_selector '.split-item' From f630738b2c83d620841b744c1840edc0bb7bee91 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 29 Mar 2019 18:42:06 +1100 Subject: [PATCH 02/12] Fix HTML syntax in admin view --- app/views/spree/admin/orders/_form/_adjustments.html.haml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/views/spree/admin/orders/_form/_adjustments.html.haml b/app/views/spree/admin/orders/_form/_adjustments.html.haml index 27cb0af79e..e88819b1ac 100644 --- a/app/views/spree/admin/orders/_form/_adjustments.html.haml +++ b/app/views/spree/admin/orders/_form/_adjustments.html.haml @@ -3,8 +3,9 @@ %legend= title %table> %thead - %th= Spree.t('name') - %th= Spree.t('amount') + %tr + %th= Spree.t('name') + %th= Spree.t('amount') %tbody#order-charges.with-border - adjustments.each do |adjustment| - if (adjustment.originator_type != 'Spree::ShippingMethod') && !(adjustment.originator_type == 'Spree::TaxRate' && adjustment.amount == 0) From 500cf925ad57f790fe661c60da4e8db7d43e5828 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 2 Apr 2019 16:50:44 +1100 Subject: [PATCH 03/12] Fix HTML validity and element finding in spec The fixed partial was used twice on the same page. So the used element id appeared twice on the page which is invalid HTML. It also confused the spec. Now we look at the actually displayed text to select the correct element. --- app/views/spree/admin/orders/_form/_adjustments.html.haml | 2 +- spec/features/admin/orders_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/spree/admin/orders/_form/_adjustments.html.haml b/app/views/spree/admin/orders/_form/_adjustments.html.haml index e88819b1ac..8dfb0c37c5 100644 --- a/app/views/spree/admin/orders/_form/_adjustments.html.haml +++ b/app/views/spree/admin/orders/_form/_adjustments.html.haml @@ -6,7 +6,7 @@ %tr %th= Spree.t('name') %th= Spree.t('amount') - %tbody#order-charges.with-border + %tbody.with-border - adjustments.each do |adjustment| - if (adjustment.originator_type != 'Spree::ShippingMethod') && !(adjustment.originator_type == 'Spree::TaxRate' && adjustment.amount == 0) %tr.total diff --git a/spec/features/admin/orders_spec.rb b/spec/features/admin/orders_spec.rb index dc8b654be8..8d387b2678 100644 --- a/spec/features/admin/orders_spec.rb +++ b/spec/features/admin/orders_spec.rb @@ -267,7 +267,7 @@ feature %q{ end scenario "shows the order tax adjustments" do - within('tbody#order-charges') do + within('fieldset', text: 'LINE ITEM ADJUSTMENTS') do expect(page).to have_selector "td", match: :first, text: "Tax 1" expect(page).to have_selector "td.total", text: Spree::Money.new(10) end From d78c709ad216ee26943c6a89f5e4ffc7278a1f78 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 2 Apr 2019 16:58:08 +1100 Subject: [PATCH 04/12] Remove unused variables from view --- app/views/spree/admin/orders/_form.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/spree/admin/orders/_form.html.haml b/app/views/spree/admin/orders/_form.html.haml index bebe2dea1d..6fac842902 100644 --- a/app/views/spree/admin/orders/_form.html.haml +++ b/app/views/spree/admin/orders/_form.html.haml @@ -4,8 +4,8 @@ = render :partial => "spree/admin/orders/shipment", :collection => @order.shipments, :locals => { :order => order } - = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.price_adjustments, :order => order, :title => Spree.t(:line_item_adjustments)} - = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.adjustments.eligible, :order => order, :title => Spree.t(:order_adjustments)} + = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.price_adjustments, :title => Spree.t(:line_item_adjustments)} + = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.adjustments.eligible, :title => Spree.t(:order_adjustments)} - if order.line_items.exists? %fieldset#order-total.no-border-bottom{"data-hook" => "order_details_total"} From 11c12787f631166a88bf5ec9059f1e2369276bc8 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 2 Apr 2019 17:02:05 +1100 Subject: [PATCH 05/12] Move adjustment selection to helper --- app/helpers/admin/orders_helper.rb | 7 +++++++ app/views/spree/admin/orders/_form.html.haml | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 app/helpers/admin/orders_helper.rb diff --git a/app/helpers/admin/orders_helper.rb b/app/helpers/admin/orders_helper.rb new file mode 100644 index 0000000000..743b86ab6f --- /dev/null +++ b/app/helpers/admin/orders_helper.rb @@ -0,0 +1,7 @@ +module Admin + module OrdersHelper + def order_adjustments(order) + order.adjustments.eligible + end + end +end diff --git a/app/views/spree/admin/orders/_form.html.haml b/app/views/spree/admin/orders/_form.html.haml index 6fac842902..72d43900fd 100644 --- a/app/views/spree/admin/orders/_form.html.haml +++ b/app/views/spree/admin/orders/_form.html.haml @@ -5,7 +5,7 @@ = render :partial => "spree/admin/orders/shipment", :collection => @order.shipments, :locals => { :order => order } = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.price_adjustments, :title => Spree.t(:line_item_adjustments)} - = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.adjustments.eligible, :title => Spree.t(:order_adjustments)} + = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => order_adjustments(@order), :title => Spree.t(:order_adjustments)} - if order.line_items.exists? %fieldset#order-total.no-border-bottom{"data-hook" => "order_details_total"} From 2a6236823f263dba65c359147290918be3a2440b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 2 Apr 2019 17:25:33 +1100 Subject: [PATCH 06/12] Move adjustment filtering from view to helper --- app/helpers/admin/orders_helper.rb | 9 ++++++++- .../spree/admin/orders/_form/_adjustments.html.haml | 9 ++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/app/helpers/admin/orders_helper.rb b/app/helpers/admin/orders_helper.rb index 743b86ab6f..bd64bbe1c4 100644 --- a/app/helpers/admin/orders_helper.rb +++ b/app/helpers/admin/orders_helper.rb @@ -1,7 +1,14 @@ module Admin module OrdersHelper def order_adjustments(order) - order.adjustments.eligible + order.adjustments.eligible.select do |adjustment| + type = adjustment.originator_type + + is_shipping_method_fee = (type == 'Spree::ShippingMethod') + is_zero_tax_rate = (type == 'Spree::TaxRate' && adjustment.amount.zero?) + + !is_shipping_method_fee && !is_zero_tax_rate + end end end end diff --git a/app/views/spree/admin/orders/_form/_adjustments.html.haml b/app/views/spree/admin/orders/_form/_adjustments.html.haml index 8dfb0c37c5..ad9559d63d 100644 --- a/app/views/spree/admin/orders/_form/_adjustments.html.haml +++ b/app/views/spree/admin/orders/_form/_adjustments.html.haml @@ -8,8 +8,7 @@ %th= Spree.t('amount') %tbody.with-border - adjustments.each do |adjustment| - - if (adjustment.originator_type != 'Spree::ShippingMethod') && !(adjustment.originator_type == 'Spree::TaxRate' && adjustment.amount == 0) - %tr.total - %td.strong= adjustment.label + ":" - %td.total.align-center - %span= Spree::Money.new(adjustment.amount) + %tr.total + %td.strong= adjustment.label + ":" + %td.total.align-center + %span= Spree::Money.new(adjustment.amount) From 0c846758dd3d7b2591c1b366dd67043496bd639a Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 2 Apr 2019 17:47:36 +1100 Subject: [PATCH 07/12] Add missing translation to order form I saw the following error: translation missing: en.spree.line_item_adjustments In this commit I change all three translations in the view file to use lazy lookup. --- app/views/spree/admin/orders/_form.html.haml | 6 +++--- config/locales/en.yml | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/views/spree/admin/orders/_form.html.haml b/app/views/spree/admin/orders/_form.html.haml index 72d43900fd..96ba6ea89b 100644 --- a/app/views/spree/admin/orders/_form.html.haml +++ b/app/views/spree/admin/orders/_form.html.haml @@ -4,12 +4,12 @@ = render :partial => "spree/admin/orders/shipment", :collection => @order.shipments, :locals => { :order => order } - = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.price_adjustments, :title => Spree.t(:line_item_adjustments)} - = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => order_adjustments(@order), :title => Spree.t(:order_adjustments)} + = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.price_adjustments, :title => t(".line_item_adjustments")} + = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => order_adjustments(@order), :title => t(".order_adjustments")} - if order.line_items.exists? %fieldset#order-total.no-border-bottom{"data-hook" => "order_details_total"} - %legend= Spree.t(:order_total) + %legend= t(".order_total") %span.order-total= order.display_total = form_for @order, url: admin_order_url(@order), method: :put do |f| diff --git a/config/locales/en.yml b/config/locales/en.yml index 37e4eac949..3b5b1bd82b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2932,6 +2932,9 @@ See the %{link} to find out more about %{sitename}'s features and to start using title: "Distribution" distributor: "Distributor:" order_cycle: "Order cycle:" + line_item_adjustments: "Line Item Adjustments" + order_adjustments: "Order Adjustments" + order_total: "Order Total" overview: products: active_products: From 7035d572678ccd2efe6ae74e2db2cfcf56f6ee1c Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 3 Apr 2019 11:07:22 +1100 Subject: [PATCH 08/12] Better helper naming and spec cover --- app/helpers/admin/orders_helper.rb | 14 +++++++---- app/views/spree/admin/orders/_form.html.haml | 2 +- spec/helpers/admin/orders_helper_spec.rb | 25 ++++++++++++++++++++ 3 files changed, 36 insertions(+), 5 deletions(-) create mode 100644 spec/helpers/admin/orders_helper_spec.rb diff --git a/app/helpers/admin/orders_helper.rb b/app/helpers/admin/orders_helper.rb index bd64bbe1c4..9549f538c2 100644 --- a/app/helpers/admin/orders_helper.rb +++ b/app/helpers/admin/orders_helper.rb @@ -1,13 +1,19 @@ module Admin module OrdersHelper - def order_adjustments(order) + # Adjustments to display under "Order adjustments". + # + # We exclude shipping method adjustments because they are displayed in a + # separate table together with the order line items. + # + # We also exclude tax rate adjustment with zero value. + def order_adjustments_for_display(order) order.adjustments.eligible.select do |adjustment| type = adjustment.originator_type - is_shipping_method_fee = (type == 'Spree::ShippingMethod') - is_zero_tax_rate = (type == 'Spree::TaxRate' && adjustment.amount.zero?) + is_shipping_method_adjustment = (type == 'Spree::ShippingMethod') + is_zero_tax_rate_adjustment = (type == 'Spree::TaxRate' && adjustment.amount.zero?) - !is_shipping_method_fee && !is_zero_tax_rate + !is_shipping_method_adjustment && !is_zero_tax_rate_adjustment end end end diff --git a/app/views/spree/admin/orders/_form.html.haml b/app/views/spree/admin/orders/_form.html.haml index 96ba6ea89b..a14b308ba5 100644 --- a/app/views/spree/admin/orders/_form.html.haml +++ b/app/views/spree/admin/orders/_form.html.haml @@ -5,7 +5,7 @@ = render :partial => "spree/admin/orders/shipment", :collection => @order.shipments, :locals => { :order => order } = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => @order.price_adjustments, :title => t(".line_item_adjustments")} - = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => order_adjustments(@order), :title => t(".order_adjustments")} + = render :partial => "spree/admin/orders/_form/adjustments", :locals => { :adjustments => order_adjustments_for_display(@order), :title => t(".order_adjustments")} - if order.line_items.exists? %fieldset#order-total.no-border-bottom{"data-hook" => "order_details_total"} diff --git a/spec/helpers/admin/orders_helper_spec.rb b/spec/helpers/admin/orders_helper_spec.rb new file mode 100644 index 0000000000..6245e7ec62 --- /dev/null +++ b/spec/helpers/admin/orders_helper_spec.rb @@ -0,0 +1,25 @@ +require "spec_helper" + +describe Admin::OrdersHelper, type: :helper do + describe "#order_adjustments_for_display" do + let(:order) { create(:order) } + + it "selects eligible adjustments" do + adjustment = create(:adjustment, adjustable: order, amount: 1) + + expect(helper.order_adjustments_for_display(order)).to eq [adjustment] + end + + it "filters shipping method adjustments" do + create(:adjustment, adjustable: order, amount: 1, originator_type: "Spree::ShippingMethod") + + expect(helper.order_adjustments_for_display(order)).to eq [] + end + + it "filters zero tax rate adjustments" do + create(:adjustment, adjustable: order, amount: 0, originator_type: "Spree::TaxRate") + + expect(helper.order_adjustments_for_display(order)).to eq [] + end + end +end From d31b5efac6b454b9a0e043d0441d0cec1da58808 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 3 Apr 2019 11:15:54 +1100 Subject: [PATCH 09/12] Simplify orders helper --- app/helpers/admin/orders_helper.rb | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/app/helpers/admin/orders_helper.rb b/app/helpers/admin/orders_helper.rb index 9549f538c2..fa445739ad 100644 --- a/app/helpers/admin/orders_helper.rb +++ b/app/helpers/admin/orders_helper.rb @@ -4,16 +4,9 @@ module Admin # # We exclude shipping method adjustments because they are displayed in a # separate table together with the order line items. - # - # We also exclude tax rate adjustment with zero value. def order_adjustments_for_display(order) order.adjustments.eligible.select do |adjustment| - type = adjustment.originator_type - - is_shipping_method_adjustment = (type == 'Spree::ShippingMethod') - is_zero_tax_rate_adjustment = (type == 'Spree::TaxRate' && adjustment.amount.zero?) - - !is_shipping_method_adjustment && !is_zero_tax_rate_adjustment + adjustment.originator_type != "Spree::ShippingMethod" end end end From 72458da4659e3821f788591af33b192c4f958ae2 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 4 Apr 2019 16:26:52 +1100 Subject: [PATCH 10/12] Comply to code convention using I18n.t in specs --- spec/features/admin/orders_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/admin/orders_spec.rb b/spec/features/admin/orders_spec.rb index 8d387b2678..6e20f41cf1 100644 --- a/spec/features/admin/orders_spec.rb +++ b/spec/features/admin/orders_spec.rb @@ -267,7 +267,7 @@ feature %q{ end scenario "shows the order tax adjustments" do - within('fieldset', text: 'LINE ITEM ADJUSTMENTS') do + within('fieldset', text: I18n.t('spree.admin.orders.form.line_item_adjustments').upcase) do expect(page).to have_selector "td", match: :first, text: "Tax 1" expect(page).to have_selector "td.total", text: Spree::Money.new(10) end From 7b116c0982b61dd080253c7be2147c8558fefadf Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 4 Apr 2019 16:51:18 +1100 Subject: [PATCH 11/12] Remove unnecessary condition from spec --- spec/features/admin/orders_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/features/admin/orders_spec.rb b/spec/features/admin/orders_spec.rb index 6e20f41cf1..095457c4f9 100644 --- a/spec/features/admin/orders_spec.rb +++ b/spec/features/admin/orders_spec.rb @@ -255,7 +255,6 @@ feature %q{ scenario "shows the order non-tax adjustments" do within('table.index tbody') do @order.adjustments.eligible.each do |adjustment| - next if (adjustment.originator_type == 'Spree::TaxRate') && (adjustment.amount == 0) expect(page).to have_selector "td", match: :first, text: adjustment.label expect(page).to have_selector "td.total", text: adjustment.display_amount end From 84501b9e41eebd257883fbfabf74e66e056e9639 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 4 Apr 2019 17:01:17 +1100 Subject: [PATCH 12/12] Move feature spec to controller spec It reduces the runtime (3s instead of 10s). --- .../spree/admin/orders_controller_spec.rb | 17 +++++++++++++++++ spec/features/admin/orders_spec.rb | 13 ------------- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/spec/controllers/spree/admin/orders_controller_spec.rb b/spec/controllers/spree/admin/orders_controller_spec.rb index b827f3df0b..aa1bddba97 100644 --- a/spec/controllers/spree/admin/orders_controller_spec.rb +++ b/spec/controllers/spree/admin/orders_controller_spec.rb @@ -14,6 +14,23 @@ describe Spree::Admin::OrdersController, type: :controller do spree_get :edit, id: order }.to change { order.reload.state }.from("cart").to("complete") end + + describe "view" do + render_views + + it "shows only eligible adjustments" do + adjustment = create( + :adjustment, + adjustable: order, + label: "invalid adjustment", + amount: 0 + ) + + spree_get :edit, id: order + + expect(response.body).to_not match adjustment.label + end + end end context "#update" do diff --git a/spec/features/admin/orders_spec.rb b/spec/features/admin/orders_spec.rb index 095457c4f9..ad51abed4f 100644 --- a/spec/features/admin/orders_spec.rb +++ b/spec/features/admin/orders_spec.rb @@ -282,19 +282,6 @@ feature %q{ end end - scenario "shows only eligible adjustments" do - adjustment = create( - :adjustment, - adjustable: @order, - label: "invalid adjustment", - amount: 0 - ) - - visit spree.edit_admin_order_path(@order) - - expect(page).to have_no_content adjustment.label - end - scenario "cannot split the order in different stock locations" do # There's only 1 stock location in OFN, so the split functionality that comes with spree should be hidden expect(page).to_not have_selector '.split-item'