From 19e3dc077e9d60664ba0d73033f10cc8f5eeae2e Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 31 Mar 2024 17:04:15 +0500 Subject: [PATCH 1/6] 12314: remove Rails/HelperInstanceVariable from rubocop todo --- .rubocop_todo.yml | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 80de3c4a6e..a949cc0528 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -401,16 +401,6 @@ Rails/HasManyOrHasOneDependent: - 'app/models/spree/tax_rate.rb' - 'app/models/spree/variant.rb' -# Offense count: 26 -# Configuration parameters: Include. -# Include: app/helpers/**/*.rb -Rails/HelperInstanceVariable: - Exclude: - - 'app/helpers/injection_helper.rb' - - 'app/helpers/shared_helper.rb' - - 'app/helpers/spree/admin/orders_helper.rb' - - 'app/helpers/spree/orders_helper.rb' - # Offense count: 8 # Configuration parameters: Include. # Include: spec/**/*.rb, test/**/*.rb From 3bb44cfe6df7c010bff7b4a81e40b811f5bbdd7d Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 31 Mar 2024 17:07:00 +0500 Subject: [PATCH 2/6] 12314 - fix order helper rubocop errors - remove the direct access of @order instance variable - add an attr_reader for order and use it instead --- app/helpers/spree/admin/orders_helper.rb | 42 +++++++++++++----------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/app/helpers/spree/admin/orders_helper.rb b/app/helpers/spree/admin/orders_helper.rb index 893a417297..f85cea6e66 100644 --- a/app/helpers/spree/admin/orders_helper.rb +++ b/app/helpers/spree/admin/orders_helper.rb @@ -5,18 +5,18 @@ module Spree module OrdersHelper def event_links links = [] - links << cancel_event_link if @order.can_cancel? - links << resume_event_link if @order.can_resume? + links << cancel_event_link if order.can_cancel? + links << resume_event_link if order.can_resume? links.join(' ').html_safe # rubocop:disable Rails/OutputSafety end def generate_invoice_button(order) if order.distributor.can_invoice? - button_link_to t(:create_or_update_invoice), generate_admin_order_invoices_path(@order), + button_link_to t(:create_or_update_invoice), generate_admin_order_invoices_path(order), data: { method: 'post' }, icon: 'icon-plus' else button_link_to t(:create_or_update_invoice), "#", data: { - confirm: t(:must_have_valid_business_number, enterprise_name: @order.distributor.name) + confirm: t(:must_have_valid_business_number, enterprise_name: order.distributor.name) }, icon: 'icon-plus' end end @@ -25,18 +25,20 @@ module Spree Spree::Money.new(line_item.price * quantity, currency: line_item.currency) end - def order_links(order) - @order ||= order + def order_links(current_order) + @order ||= current_order links = [] links << edit_order_link unless action_name == "edit" - links.concat(complete_order_links) if @order.complete? || @order.resumed? - links << ship_order_link if @order.ready_to_ship? - links << cancel_order_link if @order.can_cancel? + links.concat(complete_order_links) if order.complete? || order.resumed? + links << ship_order_link if order.ready_to_ship? + links << cancel_order_link if order.can_cancel? links end private + attr_reader :order + def complete_order_links [resend_confirmation_link] + invoice_links end @@ -48,7 +50,7 @@ module Spree end def send_invoice_link - if @order.distributor.can_invoice? + if order.distributor.can_invoice? send_invoice_link_with_url else send_invoice_link_without_url @@ -56,7 +58,7 @@ module Spree end def print_invoice_link - if @order.distributor.can_invoice? + if order.distributor.can_invoice? print_invoice_link_with_url else notify_about_required_enterprise_number @@ -65,20 +67,20 @@ module Spree def edit_order_link { name: t(:edit_order), - url: spree.edit_admin_order_path(@order), + url: spree.edit_admin_order_path(order), icon: 'icon-edit' } end def resend_confirmation_link { name: t(:resend_confirmation), - url: spree.resend_admin_order_path(@order), + url: spree.resend_admin_order_path(order), icon: 'icon-email', confirm: t(:confirm_resend_order_confirmation) } end def send_invoice_link_with_url { name: t(:send_invoice), - url: invoice_admin_order_path(@order), + url: invoice_admin_order_path(order), icon: 'icon-email', confirm: t(:confirm_send_invoice) } end @@ -87,12 +89,12 @@ module Spree { name: t(:send_invoice), url: "#", icon: 'icon-email', - confirm: t(:must_have_valid_business_number, enterprise_name: @order.distributor.name) } + confirm: t(:must_have_valid_business_number, enterprise_name: order.distributor.name) } end def print_invoice_link_with_url { name: t(:print_invoice), - url: spree.print_admin_order_path(@order), + url: spree.print_admin_order_path(order), icon: 'icon-print', target: "_blank" } end @@ -101,7 +103,7 @@ module Spree { name: t(:print_invoice), url: "#", icon: 'icon-print', - confirm: t(:must_have_valid_business_number, enterprise_name: @order.distributor.name) } + confirm: t(:must_have_valid_business_number, enterprise_name: order.distributor.name) } end def ship_order_link @@ -112,14 +114,14 @@ module Spree def cancel_order_link { name: t(:cancel_order), - url: spree.fire_admin_order_path(@order.number, e: 'cancel'), + url: spree.fire_admin_order_path(order.number, e: 'cancel'), icon: 'icon-trash' } end def cancel_event_link event_label = I18n.t("cancel", scope: "actions") button_link_to(event_label, - fire_admin_order_url(@order, e: "cancel"), + fire_admin_order_url(order, e: "cancel"), method: :put, icon: "icon-cancel", form_id: "cancel_order_form") end @@ -127,7 +129,7 @@ module Spree event_label = I18n.t("resume", scope: "actions") confirm_message = I18n.t("admin.orders.edit.order_sure_want_to", event: event_label) button_link_to(event_label, - fire_admin_order_url(@order, e: "resume"), + fire_admin_order_url(order, e: "resume"), method: :put, icon: "icon-resume", data: { confirm: confirm_message }) end From ac1595e718fd1bf15eeec1503fa6bdcc4fa83489 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Sun, 31 Mar 2024 18:08:22 +0500 Subject: [PATCH 3/6] 12314 - remove shared distributor partial - this was only used in the enterprise show view - the above view was deleted here 4f2389e25766a59846e6c13e28064c5bb7242897 - by removing this, we can remove distributor_link_class method - it will also fix the rubocop error --- app/helpers/shared_helper.rb | 10 --------- app/views/shared/_distributor.html.haml | 3 --- spec/helpers/shared_helper_spec.rb | 27 ------------------------- 3 files changed, 40 deletions(-) delete mode 100644 app/views/shared/_distributor.html.haml delete mode 100644 spec/helpers/shared_helper_spec.rb diff --git a/app/helpers/shared_helper.rb b/app/helpers/shared_helper.rb index 17bd12adbe..63e5498409 100644 --- a/app/helpers/shared_helper.rb +++ b/app/helpers/shared_helper.rb @@ -1,16 +1,6 @@ # frozen_string_literal: true module SharedHelper - def distributor_link_class(distributor) - cart = current_order(true) - @active_distributors ||= Enterprise.distributors_with_active_order_cycles - - klass = "shop-distributor" - klass += " empties-cart" unless cart.line_items.empty? || cart.distributor == distributor - klass += @active_distributors.include?(distributor) ? ' active' : ' inactive' - klass - end - def enterprise_user? spree_current_user&.enterprises&.count.to_i > 0 end diff --git a/app/views/shared/_distributor.html.haml b/app/views/shared/_distributor.html.haml deleted file mode 100644 index ea1eeae0a4..0000000000 --- a/app/views/shared/_distributor.html.haml +++ /dev/null @@ -1,3 +0,0 @@ -= succeed ',' do - = link_to "#{distributor.name}".html_safe, enterprise_shop_path(distributor), {class: distributor_link_class(distributor)} -%span.secondary= distributor.city diff --git a/spec/helpers/shared_helper_spec.rb b/spec/helpers/shared_helper_spec.rb deleted file mode 100644 index 7d61898f93..0000000000 --- a/spec/helpers/shared_helper_spec.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe SharedHelper, type: :helper do - it "does not require emptying the cart when it is empty" do - d = double(:distributor) - order = double(:order, line_items: []) - allow(helper).to receive(:current_order) { order } - expect(helper.distributor_link_class(d)).not_to match(/empties-cart/) - end - - it "does not require emptying the cart when we are on the same distributor" do - d = double(:distributor) - order = double(:order, line_items: [double(:line_item)], distributor: d) - allow(helper).to receive(:current_order) { order } - expect(helper.distributor_link_class(d)).not_to match(/empties-cart/) - end - - it "requires emptying the cart otherwise" do - d1 = double(:distributor) - d2 = double(:distributor) - order = double(:order, line_items: [double(:line_item)], distributor: d2) - allow(helper).to receive(:current_order) { order } - expect(helper.distributor_link_class(d1)).to match(/empties-cart/) - end -end From 4ba6afa6652cf8fc4b3f8070bc2159299f9b8055 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Mon, 1 Apr 2024 02:19:14 +0500 Subject: [PATCH 4/6] 12314 - fix Rails/HelperInstanceVariable error - InjectionHelper - OrdersHelper --- app/helpers/injection_helper.rb | 3 +-- app/helpers/spree/orders_helper.rb | 21 +++++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/helpers/injection_helper.rb b/app/helpers/injection_helper.rb index cb191f1367..63a0797992 100644 --- a/app/helpers/injection_helper.rb +++ b/app/helpers/injection_helper.rb @@ -154,7 +154,6 @@ module InjectionHelper end def enterprise_injection_data - @enterprise_injection_data ||= OpenFoodNetwork::EnterpriseInjectionData.new - { data: @enterprise_injection_data } + @enterprise_injection_data ||= { data: OpenFoodNetwork::EnterpriseInjectionData.new } end end diff --git a/app/helpers/spree/orders_helper.rb b/app/helpers/spree/orders_helper.rb index 63a4682bc2..60de9e33a1 100644 --- a/app/helpers/spree/orders_helper.rb +++ b/app/helpers/spree/orders_helper.rb @@ -17,17 +17,18 @@ module Spree def changeable_orders # Only returns open order for the current user + shop + oc combo - return @changeable_orders unless @changeable_orders.nil? - return @changeable_orders = [] unless spree_current_user && - current_distributor && current_order_cycle - return @changeable_orders = [] unless current_distributor.allow_order_changes? + @changeable_orders ||= if spree_current_user && + current_distributor&.allow_order_changes? && current_order_cycle - @changeable_orders = Spree::Order.complete.where( - state: 'complete', - user_id: spree_current_user.id, - distributor_id: current_distributor.id, - order_cycle_id: current_order_cycle.id - ) + Spree::Order.complete.where( + state: 'complete', + user_id: spree_current_user.id, + distributor_id: current_distributor.id, + order_cycle_id: current_order_cycle.id + ) + else + [] + end end def changeable_orders_link_path From 05f373f541e790906f70f6897b63530c0ad2fa62 Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Mon, 1 Apr 2024 06:41:41 +0500 Subject: [PATCH 5/6] 12314 - add order parameter in OrdersHelper methods --- app/helpers/spree/admin/orders_helper.rb | 55 ++++++++++----------- app/views/spree/admin/orders/edit.html.haml | 2 +- 2 files changed, 27 insertions(+), 30 deletions(-) diff --git a/app/helpers/spree/admin/orders_helper.rb b/app/helpers/spree/admin/orders_helper.rb index f85cea6e66..71970c53d3 100644 --- a/app/helpers/spree/admin/orders_helper.rb +++ b/app/helpers/spree/admin/orders_helper.rb @@ -3,10 +3,10 @@ module Spree module Admin module OrdersHelper - def event_links + def event_links(order) links = [] - links << cancel_event_link if order.can_cancel? - links << resume_event_link if order.can_resume? + links << cancel_event_link(order) if order.can_cancel? + links << resume_event_link(order) if order.can_resume? links.join(' ').html_safe # rubocop:disable Rails/OutputSafety end @@ -25,81 +25,78 @@ module Spree Spree::Money.new(line_item.price * quantity, currency: line_item.currency) end - def order_links(current_order) - @order ||= current_order + def order_links(order) links = [] - links << edit_order_link unless action_name == "edit" - links.concat(complete_order_links) if order.complete? || order.resumed? + links << edit_order_link(order) unless action_name == "edit" + links.concat(complete_order_links(order)) if order.complete? || order.resumed? links << ship_order_link if order.ready_to_ship? - links << cancel_order_link if order.can_cancel? + links << cancel_order_link(order) if order.can_cancel? links end private - attr_reader :order - - def complete_order_links - [resend_confirmation_link] + invoice_links + def complete_order_links(order) + [resend_confirmation_link(order)] + invoice_links(order) end - def invoice_links + def invoice_links(order) return [] unless Spree::Config[:enable_invoices?] - [send_invoice_link, print_invoice_link] + [send_invoice_link(order), print_invoice_link(order)] end - def send_invoice_link + def send_invoice_link(order) if order.distributor.can_invoice? - send_invoice_link_with_url + send_invoice_link_with_url(order) else - send_invoice_link_without_url + send_invoice_link_without_url(order) end end - def print_invoice_link + def print_invoice_link(order) if order.distributor.can_invoice? - print_invoice_link_with_url + print_invoice_link_with_url(order) else - notify_about_required_enterprise_number + notify_about_required_enterprise_number(order) end end - def edit_order_link + def edit_order_link(order) { name: t(:edit_order), url: spree.edit_admin_order_path(order), icon: 'icon-edit' } end - def resend_confirmation_link + def resend_confirmation_link(order) { name: t(:resend_confirmation), url: spree.resend_admin_order_path(order), icon: 'icon-email', confirm: t(:confirm_resend_order_confirmation) } end - def send_invoice_link_with_url + def send_invoice_link_with_url(order) { name: t(:send_invoice), url: invoice_admin_order_path(order), icon: 'icon-email', confirm: t(:confirm_send_invoice) } end - def send_invoice_link_without_url + def send_invoice_link_without_url(order) { name: t(:send_invoice), url: "#", icon: 'icon-email', confirm: t(:must_have_valid_business_number, enterprise_name: order.distributor.name) } end - def print_invoice_link_with_url + def print_invoice_link_with_url(order) { name: t(:print_invoice), url: spree.print_admin_order_path(order), icon: 'icon-print', target: "_blank" } end - def notify_about_required_enterprise_number + def notify_about_required_enterprise_number(order) { name: t(:print_invoice), url: "#", icon: 'icon-print', @@ -112,20 +109,20 @@ module Spree icon: 'icon-truck' } end - def cancel_order_link + def cancel_order_link(order) { name: t(:cancel_order), url: spree.fire_admin_order_path(order.number, e: 'cancel'), icon: 'icon-trash' } end - def cancel_event_link + def cancel_event_link(order) event_label = I18n.t("cancel", scope: "actions") button_link_to(event_label, fire_admin_order_url(order, e: "cancel"), method: :put, icon: "icon-cancel", form_id: "cancel_order_form") end - def resume_event_link + def resume_event_link(order) event_label = I18n.t("resume", scope: "actions") confirm_message = I18n.t("admin.orders.edit.order_sure_want_to", event: event_label) button_link_to(event_label, diff --git a/app/views/spree/admin/orders/edit.html.haml b/app/views/spree/admin/orders/edit.html.haml index c74f2e6bcb..eb10841bbc 100644 --- a/app/views/spree/admin/orders/edit.html.haml +++ b/app/views/spree/admin/orders/edit.html.haml @@ -5,7 +5,7 @@ - content_for :page_actions do - if can?(:fire, @order) - %li= event_links + %li= event_links(@order) = render partial: 'spree/admin/shared/order_links' - if can?(:admin, Spree::Order) %li From 7b4b7c5f45e9c60612ef1ce443a7ed59401a6deb Mon Sep 17 00:00:00 2001 From: Ahmed Ejaz Date: Mon, 1 Apr 2024 21:51:46 +0500 Subject: [PATCH 6/6] 12314 - Fix specs for orders helper --- app/helpers/spree/orders_helper.rb | 2 +- spec/helpers/spree/orders_helper_spec.rb | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/helpers/spree/orders_helper.rb b/app/helpers/spree/orders_helper.rb index 60de9e33a1..d3d1a155c1 100644 --- a/app/helpers/spree/orders_helper.rb +++ b/app/helpers/spree/orders_helper.rb @@ -18,7 +18,7 @@ module Spree def changeable_orders # Only returns open order for the current user + shop + oc combo @changeable_orders ||= if spree_current_user && - current_distributor&.allow_order_changes? && current_order_cycle + current_order_cycle && current_distributor&.allow_order_changes? Spree::Order.complete.where( state: 'complete', diff --git a/spec/helpers/spree/orders_helper_spec.rb b/spec/helpers/spree/orders_helper_spec.rb index 1b47d33a0e..a44bd9afab 100644 --- a/spec/helpers/spree/orders_helper_spec.rb +++ b/spec/helpers/spree/orders_helper_spec.rb @@ -31,6 +31,11 @@ describe Spree::OrdersHelper, type: :helper do before { allow(current_distributor).to receive(:allow_order_changes?) { false } } it { expect(helper.changeable_orders).to eq [] } end + + context "when a current_distributor is not defined" do + let(:current_distributor) { nil } + it { expect(helper.changeable_orders).to eq [] } + end end context "when a current_order_cycle is not defined" do @@ -38,11 +43,6 @@ describe Spree::OrdersHelper, type: :helper do it { expect(helper.changeable_orders).to eq [] } end end - - context "when a current_distributor is not defined" do - let(:current_distributor) { nil } - it { expect(helper.changeable_orders).to eq [] } - end end context "when spree_current_user is not defined" do