From 8949f1dc2e0d38792ed91f14f7f90d2031cc7977 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 13 Jul 2023 15:01:00 +1000
Subject: [PATCH 1/9] Convert route to resource

I don't know why the route helper now has "index" in the name.
---
 app/views/spree/admin/shared/_product_sub_menu.html.haml | 2 +-
 config/routes/admin.rb                                   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/views/spree/admin/shared/_product_sub_menu.html.haml b/app/views/spree/admin/shared/_product_sub_menu.html.haml
index 3013263108..1215f1a02d 100644
--- a/app/views/spree/admin/shared/_product_sub_menu.html.haml
+++ b/app/views/spree/admin/shared/_product_sub_menu.html.haml
@@ -5,4 +5,4 @@
     = tab :variant_overrides, url: main_app.admin_inventory_path, match_path: '/inventory'
     = tab :import, url: main_app.admin_product_import_path, match_path: '/product_import'
     - if feature?(:admin_style_v3, spree_current_user)
-      = tab :products_v3, url: main_app.admin_products_v3_path
+      = tab :products_v3, url: main_app.admin_products_v3_index_path
diff --git a/config/routes/admin.rb b/config/routes/admin.rb
index 3f2259c35a..dea9207fb2 100644
--- a/config/routes/admin.rb
+++ b/config/routes/admin.rb
@@ -70,7 +70,7 @@ Openfoodnetwork::Application.routes.draw do
     post '/product_import/reset_absent', to: 'product_import#reset_absent_products', as: 'product_import_reset_async'
 
     constraints FeatureToggleConstraint.new(:admin_style_v3) do
-      get '/products_v3', to: 'products_v3#index'
+      resources :products_v3, only: :index
     end
 
     resources :variant_overrides do

From 2a4d5af5527a3e246c252309378f79dcae2b0bf1 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 21 Jul 2023 13:40:13 +1000
Subject: [PATCH 2/9] Remove redundant test

This is already covered in the following test.
---
 spec/system/admin/products_v3/products_spec.rb | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index c83e073fd9..d3d20e3f5e 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -84,10 +84,6 @@ describe 'As an admin, I can see the new product page' do
     end
 
     context "search by producer" do
-      it "has a producer select" do
-        expect(page).to have_selector "select#producer_id"
-      end
-
       it "can search for a product" do
         search_by_producer "Producer 1"
 

From 757ba2790867c525e310b588c86e7e5397515671 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 21 Jul 2023 13:34:32 +1000
Subject: [PATCH 3/9] Optimise spec: only create objects when needed

Moving the 'clear filters' and 'no results' tests up into the first context.
---
 .../system/admin/products_v3/products_spec.rb | 69 +++++++++----------
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index d3d20e3f5e..d1847c7d9a 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -11,15 +11,6 @@ describe 'As an admin, I can see the new product page' do
   70.times do |i|
     let!("product_#{i}".to_sym) { create(:simple_product, name: "product #{i}") }
   end
-  # create a product with a name that can be searched
-  let!(:product_by_name) { create(:simple_product, name: "searchable product") }
-  # create a product with a supplier that can be searched
-  let!(:producer) { create(:supplier_enterprise, name: "Producer 1") }
-  let!(:product_by_supplier) { create(:simple_product, supplier: producer) }
-  # create a product with a category that can be searched
-  let!(:product_by_category) {
-    create(:simple_product, primary_taxon: create(:taxon, name: "Category 1"))
-  }
 
   before do
     # activate feature toggle admin_style_v3 to use new admin interface
@@ -61,7 +52,10 @@ describe 'As an admin, I can see the new product page' do
       visit "/admin/products_v3"
     end
 
-    context "search by search term" do
+    context "product has searchable term" do
+      # create a product with a name that can be searched
+      let!(:product_by_name) { create(:simple_product, name: "searchable product") }
+
       it "can search for a product" do
         search_for "searchable product"
 
@@ -81,30 +75,7 @@ describe 'As an admin, I can see the new product page' do
         expect_page_to_be 1
         expect_products_count_to_be 1
       end
-    end
 
-    context "search by producer" do
-      it "can search for a product" do
-        search_by_producer "Producer 1"
-
-        expect(page).to have_select "producer_id", selected: "Producer 1"
-        expect_page_to_be 1
-        expect_products_count_to_be 1
-      end
-    end
-
-    context "search by category" do
-      it "can search for a product" do
-        search_by_category "Category 1"
-
-        expect(page).to have_select "category_id", selected: "Category 1"
-        expect_page_to_be 1
-        expect_products_count_to_be 1
-        expect(page).to have_selector "table.products tbody tr td", text: product_by_category.name
-      end
-    end
-
-    context "clear filters" do
       it "can clear filters" do
         search_for "searchable product"
         expect(page).to have_field "search_term", with: "searchable product"
@@ -117,15 +88,43 @@ describe 'As an admin, I can see the new product page' do
         expect_page_to_be 1
         expect_products_count_to_be 15
       end
-    end
 
-    context "no results" do
       it "shows a message when there are no results" do
         search_for "no results"
         expect(page).to have_content "No products found for your search criteria"
         expect(page).to have_link "Clear search"
       end
     end
+
+    context "product has producer" do
+      # create a product with a supplier that can be searched
+      let!(:producer) { create(:supplier_enterprise, name: "Producer 1") }
+      let!(:product_by_supplier) { create(:simple_product, supplier: producer) }
+
+      it "can search for a product" do
+        search_by_producer "Producer 1"
+
+        expect(page).to have_select "producer_id", selected: "Producer 1"
+        expect_page_to_be 1
+        expect_products_count_to_be 1
+      end
+    end
+
+    context "product has category" do
+      # create a product with a category that can be searched
+      let!(:product_by_category) {
+        create(:simple_product, primary_taxon: create(:taxon, name: "Category 1"))
+      }
+
+      it "can search for a product" do
+        search_by_category "Category 1"
+
+        expect(page).to have_select "category_id", selected: "Category 1"
+        expect_page_to_be 1
+        expect_products_count_to_be 1
+        expect(page).to have_selector "table.products tbody tr td", text: product_by_category.name
+      end
+    end
   end
 
   def expect_page_to_be(page_number)

From 9a3820db4ffad38c981019bf847a1a2478047ddb Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 21 Jul 2023 12:43:51 +1000
Subject: [PATCH 4/9] Tidy up spec

Although 'describe' and 'context' are the same simple constructs to label groups of examples, to humans they mean:
 * Describe a particular domain of functionality
 * Context means a different environment, IE something has been set up differently (generally with before and/or let blocks)

Also the default 'before' is :each, so we don't need to specify it.
---
 spec/system/admin/products_v3/products_spec.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index d1847c7d9a..c1aab0fde8 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -23,8 +23,8 @@ describe 'As an admin, I can see the new product page' do
     expect(page).to have_content "Bulk Edit Products"
   end
 
-  context "pagination" do
-    before :each do
+  describe "pagination" do
+    before do
       visit "/admin/products_v3"
     end
 
@@ -47,8 +47,8 @@ describe 'As an admin, I can see the new product page' do
     end
   end
 
-  context "search" do
-    before :each do
+  describe "search" do
+    before do
       visit "/admin/products_v3"
     end
 

From 6dbfb36e521e2279b1fcd2e98526e752854581fa Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 21 Jul 2023 12:48:31 +1000
Subject: [PATCH 5/9] Spec: use url helpers

To be consistent with other specs.
---
 spec/system/admin/products_v3/products_spec.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index c1aab0fde8..4e7b7ecc97 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -19,13 +19,13 @@ describe 'As an admin, I can see the new product page' do
   end
 
   it "can see the new product page" do
-    visit "/admin/products_v3"
+    visit admin_products_v3_index_url
     expect(page).to have_content "Bulk Edit Products"
   end
 
   describe "pagination" do
     before do
-      visit "/admin/products_v3"
+      visit admin_products_v3_index_url
     end
 
     it "has a pagination, has 15 products per page by default and can change the page" do
@@ -49,7 +49,7 @@ describe 'As an admin, I can see the new product page' do
 
   describe "search" do
     before do
-      visit "/admin/products_v3"
+      visit admin_products_v3_index_url
     end
 
     context "product has searchable term" do

From e816228959103e5576b92aa4d15d3ae2907503e1 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Fri, 21 Jul 2023 14:19:24 +1000
Subject: [PATCH 6/9] Sort products by name in ascending order

The order is specified above in fetch_products. I'm guessing this line was unintentional?

Original requirment doesn't say ascending but I think it's safe to assume (issue#10694).
---
 app/reflexes/products_reflex.rb                |  2 +-
 spec/system/admin/products_v3/products_spec.rb | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb
index 443ac5f0d3..295341bc02 100644
--- a/app/reflexes/products_reflex.rb
+++ b/app/reflexes/products_reflex.rb
@@ -96,7 +96,7 @@ class ProductsReflex < ApplicationReflex
   end
 
   def ransack_query
-    query = { s: "name desc" }
+    query = {}
     query.merge!(supplier_id_in: @producer_id) if @producer_id.present?
     if @search_term.present?
       query.merge!(Spree::Variant::SEARCH_KEY => @search_term)
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 4e7b7ecc97..915de05c64 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -23,6 +23,21 @@ describe 'As an admin, I can see the new product page' do
     expect(page).to have_content "Bulk Edit Products"
   end
 
+  describe "sorting" do
+    let!(:product_z) { create(:simple_product, name: "Zucchini") }
+    let!(:product_a) { create(:simple_product, name: "Apples") }
+
+    before do
+      visit admin_products_v3_index_url
+    end
+
+    it "Should sort products alphabetically by default" do
+      expect(page).to have_selector "table.products tbody tr td", text: "Apples"
+      # other products push later one to next page
+      expect(page).not_to have_selector "table.products tbody tr td", text: "Zucchini"
+    end
+  end
+
   describe "pagination" do
     before do
       visit admin_products_v3_index_url

From 0f086df12bd3ff15127822128dce7f9699e50536 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 26 Jul 2023 12:06:18 +1000
Subject: [PATCH 7/9] Setup StimulusReflex testing [add gem]

Surprisingly, the StimulusReflex framework [doesn't have many resources for testing](https://docs.stimulusreflex.com/appendices/testing.html), but thankfully someone's made a gem.
---
 Gemfile                               | 1 +
 Gemfile.lock                          | 3 +++
 spec/reflex_helper.rb                 | 4 ++++
 spec/reflexes/products_reflex_spec.rb | 7 +++++++
 4 files changed, 15 insertions(+)
 create mode 100644 spec/reflex_helper.rb
 create mode 100644 spec/reflexes/products_reflex_spec.rb

diff --git a/Gemfile b/Gemfile
index 6eab0a3447..b0fd553864 100644
--- a/Gemfile
+++ b/Gemfile
@@ -159,6 +159,7 @@ group :test, :development do
   gem 'rspec-retry', require: false
   gem 'rswag-specs'
   gem 'shoulda-matchers'
+  gem 'stimulus_reflex_testing'
   gem 'timecop'
 end
 
diff --git a/Gemfile.lock b/Gemfile.lock
index 8b2d96200f..f2cb0b2d1a 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -705,6 +705,8 @@ GEM
       rack (>= 2, < 4)
       railties (>= 5.2, < 8)
       redis (>= 4.0, < 6.0)
+    stimulus_reflex_testing (0.3.0)
+      stimulus_reflex (>= 3.3.0)
     stringex (2.8.6)
     stripe (8.6.0)
     swd (1.3.0)
@@ -899,6 +901,7 @@ DEPENDENCIES
   spring-commands-rspec
   state_machines-activerecord
   stimulus_reflex (= 3.5.0.rc3)
+  stimulus_reflex_testing
   stringex (~> 2.8.5)
   stripe
   timecop
diff --git a/spec/reflex_helper.rb b/spec/reflex_helper.rb
new file mode 100644
index 0000000000..4f36f3b6f8
--- /dev/null
+++ b/spec/reflex_helper.rb
@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+require "base_spec_helper"
+require "stimulus_reflex_testing/rspec"
diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb
new file mode 100644
index 0000000000..a19076a378
--- /dev/null
+++ b/spec/reflexes/products_reflex_spec.rb
@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+require "reflex_helper"
+
+describe ProductsReflex, type: :reflex do
+
+end

From 1155bd42ea983f52b1c81da8fb358b244cd40af1 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 26 Jul 2023 12:08:37 +1000
Subject: [PATCH 8/9] Reflex test to reveal collation sorting varies on
 different OS...

https://dba.stackexchange.com/questions/106964/why-is-my-postgresql-order-by-case-insensitive

Uncommented, the spec fails on macOS (BSD).. but succeeds in Ubuntu (Linux). Weird.
---
 spec/reflexes/products_reflex_spec.rb         | 28 +++++++++++++++++++
 .../system/admin/products_v3/products_spec.rb |  4 +--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb
index a19076a378..4b1b6a6b90 100644
--- a/spec/reflexes/products_reflex_spec.rb
+++ b/spec/reflexes/products_reflex_spec.rb
@@ -3,5 +3,33 @@
 require "reflex_helper"
 
 describe ProductsReflex, type: :reflex do
+  let(:current_user) { create(:admin_user) } # todo: set up an enterprise user to test permissions
+  let(:context) {
+    { url: admin_products_v3_index_url, connection: { current_user: } }
+  }
 
+  before do
+    # activate feature toggle admin_style_v3 to use new admin interface
+    Flipper.enable(:admin_style_v3)
+  end
+
+  describe 'fetch' do
+    subject{ build_reflex(method_name: :fetch, **context) }
+
+    describe "sorting" do
+      let!(:product_z) { create(:simple_product, name: "Zucchini") }
+      # let!(:product_b) { create(:simple_product, name: "bananas") } # Fails on macOS
+      let!(:product_a) { create(:simple_product, name: "Apples") }
+
+      it "Should sort products alphabetically by default" do
+        subject.run(:fetch)
+
+        expect(subject.get(:products).to_a).to eq [
+          product_a,
+          # product_b,
+          product_z,
+        ]
+      end
+    end
+  end
 end
diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 915de05c64..5e97ca6910 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -24,7 +24,7 @@ describe 'As an admin, I can see the new product page' do
   end
 
   describe "sorting" do
-    let!(:product_z) { create(:simple_product, name: "Zucchini") }
+    let!(:product_z) { create(:simple_product, name: "zucchini") }
     let!(:product_a) { create(:simple_product, name: "Apples") }
 
     before do
@@ -34,7 +34,7 @@ describe 'As an admin, I can see the new product page' do
     it "Should sort products alphabetically by default" do
       expect(page).to have_selector "table.products tbody tr td", text: "Apples"
       # other products push later one to next page
-      expect(page).not_to have_selector "table.products tbody tr td", text: "Zucchini"
+      expect(page).not_to have_selector "table.products tbody tr td", text: "zucchini"
     end
   end
 

From b49de7d49eafbd8988f910abc288cf909d5d73d0 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Wed, 16 Aug 2023 11:13:54 +1000
Subject: [PATCH 9/9] Simplify spec

What if Zucchini didn't appear at all? Better to test that the two products appear on the same page, in the correct order.
---
 spec/system/admin/products_v3/products_spec.rb | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb
index 5e97ca6910..1935200e6d 100644
--- a/spec/system/admin/products_v3/products_spec.rb
+++ b/spec/system/admin/products_v3/products_spec.rb
@@ -24,7 +24,7 @@ describe 'As an admin, I can see the new product page' do
   end
 
   describe "sorting" do
-    let!(:product_z) { create(:simple_product, name: "zucchini") }
+    let!(:product_z) { create(:simple_product, name: "Bananas") }
     let!(:product_a) { create(:simple_product, name: "Apples") }
 
     before do
@@ -32,9 +32,7 @@ describe 'As an admin, I can see the new product page' do
     end
 
     it "Should sort products alphabetically by default" do
-      expect(page).to have_selector "table.products tbody tr td", text: "Apples"
-      # other products push later one to next page
-      expect(page).not_to have_selector "table.products tbody tr td", text: "zucchini"
+      expect(page).to have_content /Apples.*Bananas/
     end
   end