From fe93e3ed974fc45212102d466e80f9140493c714 Mon Sep 17 00:00:00 2001 From: filipefurtad0 Date: Tue, 18 Jun 2024 19:32:44 -0600 Subject: [PATCH 1/8] Restructures spec Separating into different contexts, with and without invoices feature enabled --- spec/system/admin/orders/bulk_actions_spec.rb | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/spec/system/admin/orders/bulk_actions_spec.rb b/spec/system/admin/orders/bulk_actions_spec.rb index e93cfe4b5f..4df6040459 100644 --- a/spec/system/admin/orders/bulk_actions_spec.rb +++ b/spec/system/admin/orders/bulk_actions_spec.rb @@ -337,11 +337,6 @@ RSpec.describe ' end end context "the distributor of one of the order didn't set the ABN" do - before do - order4.distributor.update(abn: "123456789") - order5.distributor.update(abn: nil) - end - shared_examples "should not print the invoice" do it "should render a warning message" do page.find(order4_selector).click @@ -363,9 +358,20 @@ RSpec.describe ' } must have a valid ABN before invoices can be used." end end - it_behaves_like "should not print the invoice" - context "with legal invoices feature", feature: :invoices do - it_behaves_like "should not print the invoice" + + context "ABN is nil" do + before do + order4.distributor.update(abn: "123456789") + order5.distributor.update(abn: nil) + end + + context "with legal invoices feature disabled" do + it_behaves_like "should not print the invoice" + end + + context "with legal invoices feature", feature: :invoices do + it_behaves_like "should not print the invoice" + end end end end From d5b77a4f80689a11989dced007c1bb75c4644f75 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 19 Jun 2024 11:45:28 +1000 Subject: [PATCH 2/8] Ensure all assets are compiled on test page visit --- spec/system/consumer/caching/shops_caching_spec.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/spec/system/consumer/caching/shops_caching_spec.rb b/spec/system/consumer/caching/shops_caching_spec.rb index baaf3302e3..d9cda44d54 100644 --- a/spec/system/consumer/caching/shops_caching_spec.rb +++ b/spec/system/consumer/caching/shops_caching_spec.rb @@ -14,12 +14,13 @@ RSpec.describe "Shops caching", caching: true do } describe "caching enterprises AMS data" do - it "caches data for all enterprises, with the provided options" do + before do # Trigger lengthy tasks like JS compilation before testing caching: visit shops_path Rails.cache.clear + end - # Now run the test, hopefully in a timely manner: + it "caches data for all enterprises, with the provided options" do visit shops_path key, options = CacheService::FragmentCaching.ams_shops @@ -73,6 +74,10 @@ RSpec.describe "Shops caching", caching: true do before do exchange.variants << product.variants.first + + # Trigger lengthy tasks like JS compilation before testing caching: + visit enterprise_shop_path(distributor) + Rails.cache.clear end it "caches rendered response for taxons and properties, with the provided options" do From 59b6cdaf01497153a8429a9b8bc23e6ae0172cf0 Mon Sep 17 00:00:00 2001 From: filipefurtad0 Date: Tue, 18 Jun 2024 19:46:36 -0600 Subject: [PATCH 3/8] Extends tests to cases where ABN is an empty string Removes pending and changes testcase As discussed on Slack we will not fix old invoice model, for ref. see: https://openfoodnetwork.slack.com/archives/CG7NJ966B/p1718781775612759?thread_ts=1718249609.127149&cid=CG7NJ966B --- spec/system/admin/orders/bulk_actions_spec.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/spec/system/admin/orders/bulk_actions_spec.rb b/spec/system/admin/orders/bulk_actions_spec.rb index 4df6040459..ef97e219bd 100644 --- a/spec/system/admin/orders/bulk_actions_spec.rb +++ b/spec/system/admin/orders/bulk_actions_spec.rb @@ -373,6 +373,22 @@ RSpec.describe ' it_behaves_like "should not print the invoice" end end + + context "ABN is an empty string" do + before do + order4.distributor.update(abn: "123456789") + order5.distributor.update(abn: "") + end + + context "with legal invoices feature disabled" do + it_behaves_like "can bulk print invoices from 2 orders" + end + + context "with legal invoices feature", feature: :invoices do + before { pending("#12373") } + it_behaves_like "should not print the invoice" + end + end end end end From 3380175c30ab537080a77e60c3a62d2aaba2d61a Mon Sep 17 00:00:00 2001 From: filipefurtad0 Date: Wed, 19 Jun 2024 16:52:36 -0600 Subject: [PATCH 4/8] Restructures spec to accommodate shared example on bulk printing --- spec/system/admin/orders/bulk_actions_spec.rb | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/spec/system/admin/orders/bulk_actions_spec.rb b/spec/system/admin/orders/bulk_actions_spec.rb index ef97e219bd..4568374c17 100644 --- a/spec/system/admin/orders/bulk_actions_spec.rb +++ b/spec/system/admin/orders/bulk_actions_spec.rb @@ -140,25 +140,29 @@ RSpec.describe ' end end - it "can bulk send confirmation email from 2 orders" do - page.find("#listing_orders tbody tr:nth-child(1) input[name='bulk_ids[]']").click - page.find("#listing_orders tbody tr:nth-child(2) input[name='bulk_ids[]']").click + shared_examples "can bulk send confirmation email from 2 orders" do + it "bulk prints invoices in pdf format" do + page.find("#listing_orders tbody tr:nth-child(1) input[name='bulk_ids[]']").click + page.find("#listing_orders tbody tr:nth-child(2) input[name='bulk_ids[]']").click - page.find("span.icon-reorder", text: "ACTIONS").click - within ".ofn-drop-down .menu" do - page.find("span", text: "Resend Confirmation").click + page.find("span.icon-reorder", text: "ACTIONS").click + within ".ofn-drop-down .menu" do + page.find("span", text: "Resend Confirmation").click + end + + expect(page).to have_content "Are you sure you want to proceed?" + + within ".reveal-modal" do + expect { + find_button("Confirm").click + }.to enqueue_job(ActionMailer::MailDeliveryJob).exactly(:twice) + end + + expect(page).to have_content "Confirmation emails sent for 2 orders." end - - expect(page).to have_content "Are you sure you want to proceed?" - - within ".reveal-modal" do - expect { - find_button("Confirm").click - }.to enqueue_job(ActionMailer::MailDeliveryJob).exactly(:twice) - end - - expect(page).to have_content "Confirmation emails sent for 2 orders." end + + it_behaves_like "can bulk send confirmation email from 2 orders" end context "can bulk print invoices" do From d1560ce15e8056c05d6dbb66f99a12030382dae7 Mon Sep 17 00:00:00 2001 From: filipefurtad0 Date: Wed, 19 Jun 2024 17:42:30 -0600 Subject: [PATCH 5/8] Adds shared examples for bulk send invoices in PDF --- spec/system/admin/orders/bulk_actions_spec.rb | 79 +++++++++++++------ 1 file changed, 55 insertions(+), 24 deletions(-) diff --git a/spec/system/admin/orders/bulk_actions_spec.rb b/spec/system/admin/orders/bulk_actions_spec.rb index 4568374c17..17fdc62575 100644 --- a/spec/system/admin/orders/bulk_actions_spec.rb +++ b/spec/system/admin/orders/bulk_actions_spec.rb @@ -105,14 +105,7 @@ RSpec.describe ' Spree::Config[:enterprise_number_required_on_invoices?] = false end - context "with multiple orders with differents states" do - before do - order2.update(state: "complete") - order3.update(state: "resumed") - order4.update(state: "canceled") - order5.update(state: "payment") - end - + shared_examples "can bulk send confirmation email from 2 orders" do it "can bulk send invoices per email, but only for the 'complete' or 'resumed' ones" do within "#listing_orders" do page.find("input[name='bulk_ids[]'][value='#{order2.id}']").click @@ -122,6 +115,7 @@ RSpec.describe ' end page.find("span.icon-reorder", text: "ACTIONS").click + within ".ofn-drop-down .menu" do page.find("span", text: "Send Invoices").click end @@ -140,29 +134,66 @@ RSpec.describe ' end end - shared_examples "can bulk send confirmation email from 2 orders" do - it "bulk prints invoices in pdf format" do - page.find("#listing_orders tbody tr:nth-child(1) input[name='bulk_ids[]']").click - page.find("#listing_orders tbody tr:nth-child(2) input[name='bulk_ids[]']").click + context "with multiple orders with differents states" do + before do + order2.update(state: "canceled") + order3.update(state: "payment") + order4.update(state: "complete") + order5.update(state: "resumed") + end - page.find("span.icon-reorder", text: "ACTIONS").click - within ".ofn-drop-down .menu" do - page.find("span", text: "Resend Confirmation").click + it_behaves_like "can bulk send confirmation email from 2 orders" + + describe "ABN" do + context "ABN is not required" do + before do + allow(Spree::Config).to receive(:enterprise_number_required_on_invoices?) + .and_return false + end + it_behaves_like "can bulk send confirmation email from 2 orders" end + context "ABN is required" do + before do + allow(Spree::Config).to receive(:enterprise_number_required_on_invoices?) + .and_return true + end + context "All the distributors setup the ABN" do + before do + order4.distributor.update(abn: "123456789") + order5.distributor.update(abn: "987654321") + end + context "all the orders are invoiceable (completed/resumed)" do + it_behaves_like "can bulk send confirmation email from 2 orders" + end + end + context "the distributor of one of the order didn't set the ABN" do + context "ABN is nil" do + before do + order4.distributor.update(abn: "123456789") + order5.distributor.update(abn: nil) + end - expect(page).to have_content "Are you sure you want to proceed?" + context "with legal invoices feature disabled" do + before { pending("Emails are not sent in this case") } + it_behaves_like "can bulk send confirmation email from 2 orders" + end + end - within ".reveal-modal" do - expect { - find_button("Confirm").click - }.to enqueue_job(ActionMailer::MailDeliveryJob).exactly(:twice) + context "ABN is an empty string" do + before do + order4.distributor.update(abn: "123456789") + order5.distributor.update(abn: "") + end + + context "with legal invoices feature disabled" do + before { pending("Emails are not sent in this case") } + it_behaves_like "can bulk send confirmation email from 2 orders" + end + end + end end - - expect(page).to have_content "Confirmation emails sent for 2 orders." end end - - it_behaves_like "can bulk send confirmation email from 2 orders" end context "can bulk print invoices" do From 48a39f20e824829daf3152de25c82f40107b1e29 Mon Sep 17 00:00:00 2001 From: filipefurtad0 Date: Wed, 19 Jun 2024 19:30:42 -0600 Subject: [PATCH 6/8] Adds regression spec for #12591 --- spec/system/admin/products_spec.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/system/admin/products_spec.rb b/spec/system/admin/products_spec.rb index 6749306522..2ca528c261 100644 --- a/spec/system/admin/products_spec.rb +++ b/spec/system/admin/products_spec.rb @@ -197,6 +197,30 @@ RSpec.describe ' expect(page).to have_content "Unit value is not a number" end + it "creating product with empty product category" do + pending("#12591") + + login_as_admin + visit spree.admin_products_path + + click_link 'New Product' + + fill_in 'product_name', with: 'Hot Cakes' + select 'New supplier', from: 'product_supplier_id' + select "Weight (kg)", from: 'product_variant_unit_with_scale' + fill_in "product_unit_value", with: '1' + fill_in 'product_price', with: '1.99' + fill_in 'product_on_hand', with: 0 + check 'product_on_demand' + select 'Test Tax Category', from: 'product_tax_category_id' + fill_in_trix_editor 'product_description', + with: 'In demand, and on_demand! The hottest cakes in town.' + click_button 'Create' + + expect(current_path).to eq spree.admin_products_path + expect(page).to have_content "Product Category must exist" + end + describe "localization settings" do shared_examples "with different price values" do |localized_number, price| context "when enable_localized_number is set to #{localized_number}" do From 659111932c24fd9ea8f1c4a1edc330199275df9e Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Fri, 21 Jun 2024 23:04:33 +0200 Subject: [PATCH 7/8] Fixes Rails/RelativeDateConstant offense - Cop: Rails/RelativeDateConstant - raises offense if Constant is relative data (ie: since, ago) - Reason: relative data will be evaluated only once - BUT here, Date should not be evaluated in a class method, and have a different - value for each call. But the data should be the same for an instance - Therefore: move the ago in init method - Cf. https://docs.rubocop.org/rubocop-rails/cops_rails.html#railsrelativedateconstant - Since there is no constant to be called form a class, but a date from an instance, the spec has been modified accordingly. The RemoveTransientData.new.call had to be splitted. --- .rubocop_todo.yml | 6 ---- lib/tasks/data/remove_transient_data.rb | 16 ++++++--- .../tasks/data/remove_transient_data_spec.rb | 35 +++++++++---------- 3 files changed, 28 insertions(+), 29 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 04deca0de9..f9e352aef2 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -654,12 +654,6 @@ Rails/PluckInWhere: Exclude: - 'app/models/spree/variant.rb' -# Offense count: 1 -# This cop supports unsafe autocorrection (--autocorrect-all). -Rails/RelativeDateConstant: - Exclude: - - 'lib/tasks/data/remove_transient_data.rb' - # Offense count: 56 # This cop supports unsafe autocorrection (--autocorrect-all). # Configuration parameters: Include. diff --git a/lib/tasks/data/remove_transient_data.rb b/lib/tasks/data/remove_transient_data.rb index 1a519fae8f..a5a330642e 100644 --- a/lib/tasks/data/remove_transient_data.rb +++ b/lib/tasks/data/remove_transient_data.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class RemoveTransientData - RETENTION_PERIOD = 3.months.ago.to_date + RETENTION_PERIOD = 3.months # This model lets us operate on the sessions DB table using ActiveRecord's # methods within the scope of this service. This relies on the AR's @@ -9,12 +9,18 @@ class RemoveTransientData class Session < ApplicationRecord end + attr_reader :expiration_date + + def initialize + @expiration_date = RETENTION_PERIOD.ago.to_date + end + def call Rails.logger.info("#{self.class.name}: processing") - Spree::StateChange.where("created_at < ?", RETENTION_PERIOD).delete_all - Spree::LogEntry.where("created_at < ?", RETENTION_PERIOD).delete_all - Session.where("updated_at < ?", RETENTION_PERIOD).delete_all + Spree::StateChange.where("created_at < ?", @expiration_date).delete_all + Spree::LogEntry.where("created_at < ?", @expiration_date).delete_all + Session.where("updated_at < ?", @expiration_date).delete_all clear_old_cart_data! end @@ -23,7 +29,7 @@ class RemoveTransientData def clear_old_cart_data! old_carts = Spree::Order. - where("spree_orders.state = 'cart' AND spree_orders.updated_at < ?", RETENTION_PERIOD). + where("spree_orders.state = 'cart' AND spree_orders.updated_at < ?", @expiration_date). merge(orders_without_payments) old_cart_line_items = Spree::LineItem.where(order_id: old_carts) diff --git a/spec/lib/tasks/data/remove_transient_data_spec.rb b/spec/lib/tasks/data/remove_transient_data_spec.rb index ff2667d749..e568453da4 100644 --- a/spec/lib/tasks/data/remove_transient_data_spec.rb +++ b/spec/lib/tasks/data/remove_transient_data_spec.rb @@ -5,8 +5,6 @@ require 'tasks/data/remove_transient_data' RSpec.describe RemoveTransientData do describe '#call' do - let(:retention_period) { RemoveTransientData::RETENTION_PERIOD } - before do allow(Spree::StateChange).to receive(:delete_all) allow(Spree::LogEntry).to receive(:delete_all) @@ -14,24 +12,28 @@ RSpec.describe RemoveTransientData do allow(Rails.logger).to receive(:info) end - it 'deletes state changes older than rentention_period' do - Spree::StateChange.create(created_at: retention_period - 1.day) + it 'deletes state changes older than retention period' do + remover = RemoveTransientData.new + Spree::StateChange.create(created_at: remover.expiration_date - 1.day) + remover.call - RemoveTransientData.new.call expect(Spree::StateChange.all).to be_empty end - it 'deletes log entries older than retention_period' do - Spree::LogEntry.create(created_at: retention_period - 1.day) + it 'deletes log entries older than retention period' do + remover = RemoveTransientData.new + Spree::LogEntry.create(created_at: remover.expiration_date - 1.day) - expect { RemoveTransientData.new.call } + expect { remover.call } .to change { Spree::LogEntry.count }.by(-1) end - it 'deletes sessions older than retention_period' do - RemoveTransientData::Session.create(session_id: 1, updated_at: retention_period - 1.day) + it 'deletes sessions older than retention period' do + remover = RemoveTransientData.new + RemoveTransientData::Session.create(session_id: 1, + updated_at: remover.expiration_date - 1.day) - RemoveTransientData.new.call + remover.call expect(RemoveTransientData::Session.all).to be_empty end @@ -44,16 +46,13 @@ RSpec.describe RemoveTransientData do let!(:line_item) { create(:line_item, order: cart, variant:) } let!(:adjustment) { create(:adjustment, order: cart) } - let!(:old_cart) { create(:order, state: 'cart', updated_at: retention_period - 1.day) } + let!(:remover) { RemoveTransientData.new } + let!(:old_cart) { create(:order, state: 'cart', updated_at: remover.expiration_date - 1.day) } let!(:old_line_item) { create(:line_item, order: old_cart, variant:) } let!(:old_adjustment) { create(:adjustment, order: old_cart) } - before do - old_cart.update_columns(updated_at: retention_period - 1.day) - end - - it 'deletes cart orders and related objects older than retention_period' do - RemoveTransientData.new.call + it 'deletes cart orders and related objects older than retention period' do + remover.call expect{ cart.reload }.not_to raise_error expect{ line_item.reload }.not_to raise_error From 980ce95ab208a3a478c906d35ecf668acb230143 Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Mon, 24 Jun 2024 10:37:43 +0200 Subject: [PATCH 8/8] Requested changes - replace direct instance variable use by instance getter use --- lib/tasks/data/remove_transient_data.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/tasks/data/remove_transient_data.rb b/lib/tasks/data/remove_transient_data.rb index a5a330642e..ffe136cd08 100644 --- a/lib/tasks/data/remove_transient_data.rb +++ b/lib/tasks/data/remove_transient_data.rb @@ -18,9 +18,9 @@ class RemoveTransientData def call Rails.logger.info("#{self.class.name}: processing") - Spree::StateChange.where("created_at < ?", @expiration_date).delete_all - Spree::LogEntry.where("created_at < ?", @expiration_date).delete_all - Session.where("updated_at < ?", @expiration_date).delete_all + Spree::StateChange.where("created_at < ?", expiration_date).delete_all + Spree::LogEntry.where("created_at < ?", expiration_date).delete_all + Session.where("updated_at < ?", expiration_date).delete_all clear_old_cart_data! end @@ -29,7 +29,7 @@ class RemoveTransientData def clear_old_cart_data! old_carts = Spree::Order. - where("spree_orders.state = 'cart' AND spree_orders.updated_at < ?", @expiration_date). + where("spree_orders.state = 'cart' AND spree_orders.updated_at < ?", expiration_date). merge(orders_without_payments) old_cart_line_items = Spree::LineItem.where(order_id: old_carts)