From 39564e612f338aff965a5ec5549e6fdbf2ad8702 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 3 Jul 2020 17:00:44 +1000
Subject: [PATCH 01/17] on admin product page, add selected filter to url and
 apply filter from url on page load

---
 .../admin/bulk_product_update.js.coffee       |  44 ++++---
 .../directives/select2_min_search.js.coffee   |   9 +-
 .../admin/products/index/_filters.html.haml   |   4 +-
 .../admin/bulk_product_update_spec.rb         |  58 ++++++---
 .../admin/bulk_product_update_spec.js.coffee  | 118 +++++++++++++++++-
 5 files changed, 189 insertions(+), 44 deletions(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index 94939f3f91..7571aaaaea 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -1,4 +1,4 @@
-angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout, $filter, $http, $window, BulkProducts, DisplayProperties, DirtyProducts, VariantUnitManager, StatusMessage, producers, Taxons, Columns, tax_categories, RequestMonitor, SortOptions, ErrorsParser) ->
+angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout, $filter, $http, $window, $location, $httpParamSerializer, BulkProducts, DisplayProperties, DirtyProducts, VariantUnitManager, StatusMessage, producers, Taxons, Columns, tax_categories, RequestMonitor, SortOptions, ErrorsParser) ->
   $scope.StatusMessage = StatusMessage
 
   $scope.columns = Columns.columns
@@ -13,34 +13,30 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
     {id: 100, name: t('js.admin.orders.index.per_page', results: 100)}
   ]
 
-  $scope.filterableColumns = [
-    { name: t("label_producers"),       db_column: "producer_name" },
-    { name: t("name"),           db_column: "name" }
-  ]
-
-  $scope.filterTypes = [
-    { name: t("equals"),         predicate: "eq" },
-    { name: t("contains"),       predicate: "cont" }
-  ]
-
-  $scope.optionTabs =
-    filters:        { title: t("filter_products"),   visible: false }
+  productFilters = ['producerFilter', 'categoryFilter', 'query', 'sorting', 'importDateFilter']
+  $scope.producerFilter = ""
+  $scope.categoryFilter = ""
+  $scope.importDateFilter = ""
+  $scope.query = ""
+  $scope.sorting = ""
 
   $scope.producers = producers
   $scope.taxons = Taxons.all
   $scope.tax_categories = tax_categories
-  $scope.producerFilter = ""
-  $scope.categoryFilter = ""
-  $scope.importDateFilter = ""
   $scope.page = 1
   $scope.per_page = 15
   $scope.products = BulkProducts.products
-  $scope.query = ""
   $scope.DisplayProperties = DisplayProperties
 
   $scope.sortOptions = SortOptions
 
+  loadFilterFromUrl = ->
+    filters = $location.search()
+    for filter in productFilters
+      $scope[filter] = if filters[filter] then filters[filter] else ""
+
   $scope.initialise = ->
+    loadFilterFromUrl()
     $scope.fetchProducts()
 
   $scope.$watchCollection '[query, producerFilter, categoryFilter, importDateFilter, per_page]', ->
@@ -50,6 +46,13 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
     $scope.page = newPage
     $scope.fetchProducts()
 
+  generateFilter = ->
+    filters = {}
+    for filter in productFilters
+      filters[filter] = $scope[filter] if $scope[filter]
+    
+    filters
+
   $scope.fetchProducts = ->
     removeClearedValues()
     params = {
@@ -62,6 +65,8 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
       per_page: $scope.per_page
     }
     RequestMonitor.load(BulkProducts.fetch(params).$promise).then ->
+      # update url with the filters used
+      $location.search(generateFilter())
       $scope.resetProducts()
 
   removeClearedValues = ->
@@ -122,8 +127,9 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
 
   $scope.editWarn = (product, variant) ->
     if confirm_unsaved_changes()
-      window.open(editProductUrl(product, variant), "_blank")
-
+      filterUrl = $httpParamSerializer(generateFilter())
+      filterUrl = "?#{filterUrl}" if filterUrl isnt ""
+      $window.location.href = "#{editProductUrl(product, variant)}#{filterUrl}"
 
   $scope.toggleShowAllVariants = ->
     showVariants = !DisplayProperties.showVariants 0
diff --git a/app/assets/javascripts/admin/directives/select2_min_search.js.coffee b/app/assets/javascripts/admin/directives/select2_min_search.js.coffee
index 1d55d886aa..0d9e9faa75 100644
--- a/app/assets/javascripts/admin/directives/select2_min_search.js.coffee
+++ b/app/assets/javascripts/admin/directives/select2_min_search.js.coffee
@@ -5,5 +5,10 @@ angular.module("ofn.admin").directive "ofnSelect2MinSearch", ->
       minimumResultsForSearch: attrs.ofnSelect2MinSearch
 
     ngModel.$formatters.push (value) ->
-      if (value)
-        element.select2('val', value);
\ No newline at end of file
+      # select2 populate options with a value like "number:3" or "string:category" but 
+      # select2('val', value) doesn't do the translation for us as one would expect 
+      if isNaN(value)
+        element.select2('val', "string:#{value}")
+      else
+        element.select2('val', "number:#{value}")
+
diff --git a/app/views/spree/admin/products/index/_filters.html.haml b/app/views/spree/admin/products/index/_filters.html.haml
index 0e7d47543d..4493e95258 100644
--- a/app/views/spree/admin/products/index/_filters.html.haml
+++ b/app/views/spree/admin/products/index/_filters.html.haml
@@ -18,9 +18,7 @@
     .filter_select.three.columns
       %label{ for: 'import_filter' } Import Date
       %br
-      %select.fullwidth{ id: 'import_date_filter', 'ofn-select2-min-search' => 5, ng: {model: 'importDateFilter', init: "importDates = #{@import_dates}; showLatestImport = #{@show_latest_import}"}}
-        %option{value: "{{date.id}}", ng: {repeat: "date in importDates" }}
-          {{date.name}}
+      %select.fullwidth{ id: 'import_date_filter', 'ofn-select2-min-search' => 5, ng: {model: 'importDateFilter', init: "importDates = #{@import_dates}; showLatestImport = #{@show_latest_import}", options: 'date.id as date.name for date in importDates'} }
 
     .filter_clear.three.columns.omega
       %label{ for: 'clear_all_filters' }
diff --git a/spec/features/admin/bulk_product_update_spec.rb b/spec/features/admin/bulk_product_update_spec.rb
index 553e2182aa..fdf7d57104 100644
--- a/spec/features/admin/bulk_product_update_spec.rb
+++ b/spec/features/admin/bulk_product_update_spec.rb
@@ -514,37 +514,61 @@ feature '
         login_as_admin_and_visit spree.admin_products_path
       end
 
-      it "shows an edit button for products, which takes the user to the standard edit page for that product in a new window" do
+      it "shows an edit button for products, which takes the user to the standard edit page for that product" do
         expect(page).to have_selector "a.edit-product", count: 2
 
-        new_window = window_opened_by do
-          within "tr#p_#{p1.id}" do
-            find("a.edit-product").click
-          end
+        within "tr#p_#{p1.id}" do
+          find("a.edit-product").click
         end
 
-        within_window new_window do
-          expect(URI.parse(current_url).path).to eq "/admin/products/#{p1.permalink}/edit"
-          page.execute_script('window.close()')
-        end
+        expect(URI.parse(current_url).path).to eq "/admin/products/#{p1.permalink}/edit"
       end
 
-      it "shows an edit button for variants, which takes the user to the standard edit page for that variant in a new window" do
+      it "shows an edit button for products, which takes the user to the standard edit page for that product, url includes selected filter" do
+        expect(page).to have_selector "a.edit-product", count: 2
+
+        # Set a filter
+        select2_select p1.supplier.name, from: "producer_filter"
+        apply_filters
+
+        within "tr#p_#{p1.id}" do
+          find("a.edit-product").click
+        end
+
+        uri = URI.parse(current_url)
+        expect("#{uri.path}?#{uri.query}").to eq "/admin/products/#{p1.permalink}/edit?producerFilter=#{p1.supplier.id}"
+      end
+
+      it "shows an edit button for variants, which takes the user to the standard edit page for that variant" do
         expect(page).to have_selector "a.view-variants"
         all("a.view-variants").each(&:click)
 
         expect(page).to have_selector "a.edit-variant", count: 2
 
-        new_window = window_opened_by do
-          within "tr#v_#{v1.id}" do
-            find("a.edit-variant").click
-          end
+        within "tr#v_#{v1.id}" do
+          find("a.edit-variant").click
         end
 
-        within_window new_window do
-          expect(URI.parse(current_url).path).to eq "/admin/products/#{v1.product.permalink}/variants/#{v1.id}/edit"
-          page.execute_script('window.close()')
+        uri = URI.parse(current_url)
+        expect(URI.parse(current_url).path).to eq "/admin/products/#{v1.product.permalink}/variants/#{v1.id}/edit"
+      end
+
+      it "shows an edit button for variants, which takes the user to the standard edit page for that variant, url includes selected filter" do
+        expect(page).to have_selector "a.view-variants"
+        all("a.view-variants").each(&:click)
+
+        expect(page).to have_selector "a.edit-variant", count: 2
+
+        # Set a filter
+        select2_select p1.supplier.name, from: "producer_filter"
+        apply_filters
+
+        within "tr#v_#{v1.id}" do
+          find("a.edit-variant").click
         end
+
+        uri = URI.parse(current_url)
+        expect("#{uri.path}?#{uri.query}").to eq "/admin/products/#{v1.product.permalink}/variants/#{v1.id}/edit?producerFilter=#{p1.supplier.id}"
       end
     end
 
diff --git a/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee
index d483bec6a8..d6f7456856 100644
--- a/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee
@@ -246,7 +246,7 @@ describe "filtering products for submission to database", ->
     ]
 
 describe "AdminProductEditCtrl", ->
-  $ctrl = $scope = $timeout = $httpBackend = BulkProducts = DirtyProducts = DisplayProperties = null
+  $ctrl = $scope = $timeout = $httpBackend = BulkProducts = DirtyProducts = DisplayProperties = windowStub = null
 
   beforeEach ->
     module "ofn.admin"
@@ -267,15 +267,36 @@ describe "AdminProductEditCtrl", ->
     DirtyProducts = _DirtyProducts_
     DisplayProperties = _DisplayProperties_
 
-    $ctrl "AdminProductEditCtrl", {$scope: $scope, $timeout: $timeout}
+    # Stub the window object so we don't get redirected when href is updated 
+    windowStub = {navigator: {userAgent: 'foo'}, location: {href: ''}}
+
+    $ctrl "AdminProductEditCtrl", {$scope: $scope, $timeout: $timeout, $window: windowStub}
   )
 
   describe "loading data upon initialisation", ->
-    it "gets a list of producers and then resets products with a list of data", ->
+    beforeEach ->
       spyOn($scope, "fetchProducts").and.returnValue "nothing"
+
+    it "gets a list of producers and then resets products with a list of data", ->
       $scope.initialise()
       expect($scope.fetchProducts.calls.count()).toBe 1
 
+    it "gets a list of products applying filters from the url", inject ($location) ->
+      query = 'lala' 
+      producerFilter = 2
+      categoryFilter = 5
+      sorting = 'name desc'
+      importDateFilter = '2020-06-08'
+      $location.search({query: query, producerFilter: producerFilter, categoryFilter: categoryFilter, sorting: sorting, importDateFilter: importDateFilter})
+
+      $scope.initialise()
+
+      expect($scope.query).toBe query
+      expect($scope.producerFilter).toBe producerFilter
+      expect($scope.categoryFilter).toBe categoryFilter
+      expect($scope.sorting).toBe sorting
+      expect($scope.importDateFilter).toBe importDateFilter
+
   describe "fetching products", ->
     $q = null
     deferred = null
@@ -295,6 +316,28 @@ describe "AdminProductEditCtrl", ->
       $scope.$digest()
       expect($scope.resetProducts).toHaveBeenCalled()
 
+    it "updates url wihth filter after data has been received", inject ($location, $window) ->
+      query = 'lala'
+      producerFilter = 2
+      categoryFilter = 5
+      sorting = 'name desc'
+      importDateFilter = '2020-06-08'
+
+      $scope.query = query
+      $scope.producerFilter = producerFilter
+      $scope.categoryFilter = categoryFilter
+      $scope.sorting = sorting
+      $scope.importDateFilter = importDateFilter
+
+      $scope.fetchProducts()
+      $scope.$digest()
+
+      encodedSorting = $window.encodeURIComponent(sorting)
+      encodedDate = $window.encodeURIComponent(importDateFilter)
+      expect($location.url()).toBe(
+        "?producerFilter=#{producerFilter}&categoryFilter=#{categoryFilter}&query=#{query}&sorting=#{encodedSorting}&importDateFilter=#{encodedDate}"
+      )
+
   describe "resetting products", ->
     beforeEach ->
       spyOn DirtyProducts, "clear"
@@ -893,7 +936,76 @@ describe "AdminProductEditCtrl", ->
             id: 13
             name: "P1"
 
+  describe "editWarn", ->
+    testProduct = testVariant = null
 
+    beforeEach ->
+      available_on = new Date()
+      testProduct =
+        id: 1
+        name: "TestProduct"
+        description: ""
+        available_on: available_on
+        deleted_at: null
+        permalink: 'test-product'
+        permalink_live: 'test-product'
+        meta_description: null
+        meta_keywords: null
+        tax_category_id: null
+        shipping_category_id: null
+        created_at: null
+        updated_at: null
+        on_hand: 0
+        on_demand: false
+        producer_id: 5
+        group_buy: null
+        group_buy_unit_size: null
+        master:
+          id: 2
+          unit_value: 250
+          unit_description: "foo"
+
+    describe 'product has variant', ->
+      it 'should load the edit product variant page', ->
+        testVariant =
+          id: 2
+          name: "TestVariant"
+
+        $scope.editWarn(testProduct, testVariant)
+
+        expect(windowStub.location.href).toBe(
+          "/admin/products/#{testProduct.permalink_live}/variants/#{testVariant.id}/edit"
+        )
+
+    describe 'product has no variant', ->
+      it 'should display unsaved changes confirmation if there are any DirtyProduct', inject ($window, DirtyProducts) ->
+        spyOn($window, 'confirm')
+        spyOn(DirtyProducts, 'count').and.returnValue 2
+
+        $scope.editWarn(testProduct, null)
+        expect($window.confirm).toHaveBeenCalled()
+
+      it 'should load the edit product page', inject ->
+        $scope.editWarn(testProduct, null)
+
+        expect(windowStub.location.href).toBe(
+          "/admin/products/#{testProduct.permalink_live}/edit"
+        )
+
+      it 'should load edit product page including the selected filters', inject ($httpParamSerializer) ->
+        query = 'lala'
+        category = 3
+        $scope.query = query
+        $scope.categoryFilter = category
+
+        # use $httpParamSerializer as it will sort parameters alphabetically
+        expectedFilter = $httpParamSerializer({ query: query, categoryFilter: category })
+
+        $scope.editWarn(testProduct, null)
+        
+        expect(windowStub.location.href).toBe(
+          "/admin/products/#{testProduct.permalink_live}/edit?#{expectedFilter}"
+        )
 
   describe "filtering products", ->
     describe "clearing filters", ->

From 6e5c168d3bcc768cc13bcc9571888bab61eeff12 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 3 Jul 2020 17:06:26 +1000
Subject: [PATCH 02/17] add filter parameters to link leading back to bulk
 import product page and preserve filter parameters when updating product

---
 .../spree/admin/products_controller.rb        | 29 +++++++++++++++++--
 app/views/spree/admin/products/edit.html.haml |  9 ++++--
 spec/features/admin/products_spec.rb          | 18 ++++++++++++
 3 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/app/controllers/spree/admin/products_controller.rb b/app/controllers/spree/admin/products_controller.rb
index 76f0066d86..e131e10e25 100644
--- a/app/controllers/spree/admin/products_controller.rb
+++ b/app/controllers/spree/admin/products_controller.rb
@@ -31,6 +31,19 @@ module Spree
         }
       } }
 
+      respond_override update: { html: {
+        success: lambda {
+          redirect_to edit_admin_product_url(@product, @url_filter)
+        },
+        failure: lambda {
+          redirect_to edit_admin_product_url(@product, @url_filter)
+        }
+      } }
+
+      PRODUCT_FILTER = [
+        'query', 'producerFilter', 'categoryFilter', 'sorting', 'importDateFilter'
+      ].freeze
+
       def new
         @object.shipping_category = DefaultShippingCategory.find_or_create
         super
@@ -56,9 +69,17 @@ module Spree
         @show_latest_import = params[:latest_import] || false
       end
 
-      def update
-        original_supplier_id = @product.supplier_id
+      def edit
+        filters = product_filters(params)
+        @url_filters = filters.empty? ? "" : "?#{filters.to_query}"
 
+        super
+      end
+
+      def update
+        @url_filter = product_filters(request.query_parameters)
+
+        original_supplier_id = @product.supplier_id
         delete_stock_params_and_set_after do
           super
           if original_supplier_id != @product.supplier_id
@@ -258,6 +279,10 @@ module Spree
       def set_product_master_variant_price_to_zero
         @product.price = 0 if @product.price.nil?
       end
+
+      def product_filters(params)
+        params.select { |k, _v| PRODUCT_FILTER.include?(k) }
+      end
     end
   end
 end
diff --git a/app/views/spree/admin/products/edit.html.haml b/app/views/spree/admin/products/edit.html.haml
index 95ef4c061c..8967233715 100644
--- a/app/views/spree/admin/products/edit.html.haml
+++ b/app/views/spree/admin/products/edit.html.haml
@@ -1,5 +1,5 @@
 - content_for :page_actions do
-  %li= button_link_to t('admin.products.back_to_products_list'), admin_products_path, :icon => 'icon-arrow-left'
+  %li= button_link_to t('admin.products.back_to_products_list'), "#{admin_products_path}##{@url_filters}", :icon => 'icon-arrow-left'
   %li#new_product_link
     = button_link_to t(:new_product), new_object_url, { :icon => 'icon-plus', :id => 'admin_new_product' }
 
@@ -8,7 +8,10 @@
 = render :partial => 'spree/admin/shared/product_tabs', :locals => { :current => 'Product Details' }
 = render :partial => 'spree/shared/error_messages', :locals => { :target => @product }
 
-= form_for [:admin, @product], :method => :put, :html => { :multipart => true } do |f|
+= form_for [:admin, @product], :url => "#{admin_product_path(@product)}#{@url_filters}", :method => :put, :html => { :multipart => true } do |f|
   %fieldset.no-border-top{'ng-app' => 'admin.products'}
     = render :partial => 'form', :locals => { :f => f }
-    = render :partial => 'spree/admin/shared/edit_resource_links'
+    .form-buttons.filter-actions.actions
+      = button t(:update), 'icon-refresh'
+      %span.or= t(:or)
+      = button_link_to t(:cancel), "#{collection_url}##{@url_filters}", icon: 'icon-remove'
diff --git a/spec/features/admin/products_spec.rb b/spec/features/admin/products_spec.rb
index dd9bfafd2c..78b0c2f9e2 100644
--- a/spec/features/admin/products_spec.rb
+++ b/spec/features/admin/products_spec.rb
@@ -151,6 +151,24 @@ feature '
       expect(product.tax_category).to eq(tax_category)
     end
 
+    scenario "editing a product comming from the bulk product update page with filter" do
+      product = create(:simple_product, name: 'a product', supplier: @supplier2)
+
+      filter_query = "producerFilter=2"
+      visit "#{spree.edit_admin_product_path(product)}?#{filter_query}"
+
+      click_button 'Update'
+      expect(flash_message).to eq('Product "a product" has been successfully updated!')
+
+      # Check the url still includes the filters
+      uri = URI.parse(current_url)
+      expect("#{uri.path}?#{uri.query}").to eq "#{spree.edit_admin_product_path(product)}?#{filter_query}"
+
+      # Link back to the bulk product update page should include the filters
+      expect(page).to have_link(I18n.t('admin.products.back_to_products_list'), href: %r{admin\/products#\?#{filter_query}})
+      expect(page).to have_link(I18n.t(:cancel), href: %r{admin\/products#\?#{filter_query}})
+    end
+
     scenario "editing product group buy options" do
       product = product = create(:simple_product, supplier: @supplier2)
 

From 1a186affcf528793096709fc5d096bd762173976 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 10 Jul 2020 15:38:56 +1000
Subject: [PATCH 03/17] refactor create and update to get rid of
 respond_override

---
 .../spree/admin/products_controller.rb        | 64 +++++++------------
 1 file changed, 22 insertions(+), 42 deletions(-)

diff --git a/app/controllers/spree/admin/products_controller.rb b/app/controllers/spree/admin/products_controller.rb
index e131e10e25..406c997fdb 100644
--- a/app/controllers/spree/admin/products_controller.rb
+++ b/app/controllers/spree/admin/products_controller.rb
@@ -10,36 +10,11 @@ module Spree
       include OrderCyclesHelper
       include EnterprisesHelper
 
-      create.before :create_before
-      update.before :update_before
-
       before_action :load_data
       before_action :load_form_data, only: [:index, :new, :create, :edit, :update]
       before_action :load_spree_api_key, only: [:index, :variant_overrides]
       before_action :strip_new_properties, only: [:create, :update]
 
-      respond_override create: { html: {
-        success: lambda {
-          if params[:button] == "add_another"
-            redirect_to new_admin_product_path
-          else
-            redirect_to admin_products_path
-          end
-        },
-        failure: lambda {
-          render :new
-        }
-      } }
-
-      respond_override update: { html: {
-        success: lambda {
-          redirect_to edit_admin_product_url(@product, @url_filter)
-        },
-        failure: lambda {
-          redirect_to edit_admin_product_url(@product, @url_filter)
-        }
-      } }
-
       PRODUCT_FILTER = [
         'query', 'producerFilter', 'categoryFilter', 'sorting', 'importDateFilter'
       ].freeze
@@ -51,7 +26,20 @@ module Spree
 
       def create
         delete_stock_params_and_set_after do
-          super
+          @prototype = Spree::Prototype.find(params[:product][:prototype_id]) if \
+            params[:product][:prototype_id].present?
+
+          @object.attributes = permitted_resource_params
+          if @object.save
+            flash[:success] = flash_message_for(@object, :successfully_created)
+            if params[:button] == "add_another"
+              redirect_to new_admin_product_path
+            else
+              redirect_to admin_products_path
+            end
+          else
+            render :new
+          end
         end
       rescue Paperclip::Errors::NotIdentifiedByImageMagickError
         invoke_callbacks(:create, :fails)
@@ -81,10 +69,15 @@ module Spree
 
         original_supplier_id = @product.supplier_id
         delete_stock_params_and_set_after do
-          super
-          if original_supplier_id != @product.supplier_id
-            ExchangeVariantDeleter.new.delete(@product)
+          params[:product] ||= {} if params[:clear_product_properties]
+          if @object.update(permitted_resource_params)
+            if original_supplier_id != @product.supplier_id
+              ExchangeVariantDeleter.new.delete(@product)
+            end
+
+            flash[:success] = flash_message_for(@object, :successfully_updated)
           end
+          redirect_to edit_admin_product_url(@object, @url_filter)
         end
       end
 
@@ -156,19 +149,6 @@ module Spree
         @collection
       end
 
-      def create_before
-        return if params[:product][:prototype_id].blank?
-
-        @prototype = Spree::Prototype.find(params[:product][:prototype_id])
-      end
-
-      def update_before
-        # We only reset the product properties if we're receiving a post from the form on that tab
-        return unless params[:clear_product_properties]
-
-        params[:product] ||= {}
-      end
-
       def product_includes
         [{ variants: [:images, { option_values: :option_type }] },
          { master: [:images, :default_price] }]

From f75aaf0b457f2973115024ea21ade75096d71b4d Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 17 Jul 2020 16:12:56 +1000
Subject: [PATCH 04/17] extract product filter functionality to a helper

---
 .../spree/admin/products_controller.rb         | 13 ++-----------
 .../spree/admin/product_filter_helper.rb       | 15 +++++++++++++++
 app/views/spree/admin/products/edit.html.haml  |  6 +++---
 .../spree/admin/shared/_product_tabs.html.haml | 12 ++++++------
 spec/features/admin/products_spec.rb           |  8 ++++++++
 .../spree/admin/product_filter_helper_spec.rb  | 18 ++++++++++++++++++
 6 files changed, 52 insertions(+), 20 deletions(-)
 create mode 100644 app/helpers/spree/admin/product_filter_helper.rb
 create mode 100644 spec/helpers/spree/admin/product_filter_helper_spec.rb

diff --git a/app/controllers/spree/admin/products_controller.rb b/app/controllers/spree/admin/products_controller.rb
index 406c997fdb..81a561f94d 100644
--- a/app/controllers/spree/admin/products_controller.rb
+++ b/app/controllers/spree/admin/products_controller.rb
@@ -9,16 +9,13 @@ module Spree
       include OpenFoodNetwork::SpreeApiKeyLoader
       include OrderCyclesHelper
       include EnterprisesHelper
+      include ProductFilterHelper
 
       before_action :load_data
       before_action :load_form_data, only: [:index, :new, :create, :edit, :update]
       before_action :load_spree_api_key, only: [:index, :variant_overrides]
       before_action :strip_new_properties, only: [:create, :update]
 
-      PRODUCT_FILTER = [
-        'query', 'producerFilter', 'categoryFilter', 'sorting', 'importDateFilter'
-      ].freeze
-
       def new
         @object.shipping_category = DefaultShippingCategory.find_or_create
         super
@@ -58,9 +55,7 @@ module Spree
       end
 
       def edit
-        filters = product_filters(params)
-        @url_filters = filters.empty? ? "" : "?#{filters.to_query}"
-
+        @url_filters = product_filters(params)
         super
       end
 
@@ -259,10 +254,6 @@ module Spree
       def set_product_master_variant_price_to_zero
         @product.price = 0 if @product.price.nil?
       end
-
-      def product_filters(params)
-        params.select { |k, _v| PRODUCT_FILTER.include?(k) }
-      end
     end
   end
 end
diff --git a/app/helpers/spree/admin/product_filter_helper.rb b/app/helpers/spree/admin/product_filter_helper.rb
new file mode 100644
index 0000000000..644cdc1d56
--- /dev/null
+++ b/app/helpers/spree/admin/product_filter_helper.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+module Spree
+  module Admin
+    module ProductFilterHelper
+      PRODUCT_FILTER = [
+        'query', 'producerFilter', 'categoryFilter', 'sorting', 'importDateFilter'
+      ].freeze
+
+      def product_filters(params)
+        params.select { |k, _v| PRODUCT_FILTER.include?(k) }
+      end
+    end
+  end
+end
diff --git a/app/views/spree/admin/products/edit.html.haml b/app/views/spree/admin/products/edit.html.haml
index 8967233715..70dad9be28 100644
--- a/app/views/spree/admin/products/edit.html.haml
+++ b/app/views/spree/admin/products/edit.html.haml
@@ -1,5 +1,5 @@
 - content_for :page_actions do
-  %li= button_link_to t('admin.products.back_to_products_list'), "#{admin_products_path}##{@url_filters}", :icon => 'icon-arrow-left'
+  %li= button_link_to t('admin.products.back_to_products_list'), "#{admin_products_path}#{(@url_filters.empty? ? "" : "#?#{@url_filters.to_query}")}", :icon => 'icon-arrow-left'
   %li#new_product_link
     = button_link_to t(:new_product), new_object_url, { :icon => 'icon-plus', :id => 'admin_new_product' }
 
@@ -8,10 +8,10 @@
 = render :partial => 'spree/admin/shared/product_tabs', :locals => { :current => 'Product Details' }
 = render :partial => 'spree/shared/error_messages', :locals => { :target => @product }
 
-= form_for [:admin, @product], :url => "#{admin_product_path(@product)}#{@url_filters}", :method => :put, :html => { :multipart => true } do |f|
+= form_for [:admin, @product], :url => admin_product_path(@product, @url_filters), :method => :put, :html => { :multipart => true } do |f|
   %fieldset.no-border-top{'ng-app' => 'admin.products'}
     = render :partial => 'form', :locals => { :f => f }
     .form-buttons.filter-actions.actions
       = button t(:update), 'icon-refresh'
       %span.or= t(:or)
-      = button_link_to t(:cancel), "#{collection_url}##{@url_filters}", icon: 'icon-remove'
+      = button_link_to t(:cancel), "#{collection_url}#{(@url_filters.empty? ? "" : "#?#{@url_filters.to_query}")}", icon: 'icon-remove'
diff --git a/app/views/spree/admin/shared/_product_tabs.html.haml b/app/views/spree/admin/shared/_product_tabs.html.haml
index a16dad80d4..406b8a550c 100644
--- a/app/views/spree/admin/shared/_product_tabs.html.haml
+++ b/app/views/spree/admin/shared/_product_tabs.html.haml
@@ -12,22 +12,22 @@
       - if can?(:admin, Spree::Product)
         - klass = current == 'Product Details' ? 'active' : ''
         %li{:class => klass}
-          = link_to_with_icon 'icon-edit', t('admin.products.tabs.product_details'), edit_admin_product_url(@product)
+          = link_to_with_icon 'icon-edit', t('admin.products.tabs.product_details'), edit_admin_product_url(@product, @url_filters)
       - if can?(:admin, Spree::Image)
         - klass = current == 'Images' ? 'active' : ''
         %li{:class => klass}
-          = link_to_with_icon 'icon-picture', t('admin.products.tabs.images'), admin_product_images_url(@product)
+          = link_to_with_icon 'icon-picture', t('admin.products.tabs.images'), admin_product_images_url(@product, @url_filters)
       - if can?(:admin, Spree::Variant)
         - klass = current == 'Variants' ? 'active' : ''
         %li{:class => klass}
-          = link_to_with_icon 'icon-th-large', t('admin.products.tabs.variants'),  admin_product_variants_url(@product)
+          = link_to_with_icon 'icon-th-large', t('admin.products.tabs.variants'),  admin_product_variants_url(@product, @url_filters)
       - if can?(:admin, Spree::ProductProperty)
         - klass = current == 'Product Properties' ? 'active' : ''
         %li{:class => klass}
-          = link_to_with_icon 'icon-tasks', t('admin.products.tabs.product_properties'), admin_product_product_properties_url(@product)
+          = link_to_with_icon 'icon-tasks', t('admin.products.tabs.product_properties'), admin_product_product_properties_url(@product, @url_filters)
       - klass = current == 'Group Buy Options' ? 'active' : ''
       %li{:class => klass}
-        = link_to_with_icon 'icon-tasks', t('admin.products.tabs.group_buy_options'), group_buy_options_admin_product_url(@product)
+        = link_to_with_icon 'icon-tasks', t('admin.products.tabs.group_buy_options'), group_buy_options_admin_product_url(@product, @url_filters)
       - klass = current == t(:search) ? 'active' : ''
       %li{:class => klass}
-        = link_to_with_icon 'icon-tasks', t(:search), seo_admin_product_url(@product)
+        = link_to_with_icon 'icon-tasks', t(:search), seo_admin_product_url(@product, @url_filters)
diff --git a/spec/features/admin/products_spec.rb b/spec/features/admin/products_spec.rb
index 78b0c2f9e2..06914be930 100644
--- a/spec/features/admin/products_spec.rb
+++ b/spec/features/admin/products_spec.rb
@@ -167,6 +167,14 @@ feature '
       # Link back to the bulk product update page should include the filters
       expect(page).to have_link(I18n.t('admin.products.back_to_products_list'), href: %r{admin\/products#\?#{filter_query}})
       expect(page).to have_link(I18n.t(:cancel), href: %r{admin\/products#\?#{filter_query}})
+
+      # Sidebar link should include the filters
+      expect(page).to have_link(I18n.t('admin.products.tabs.product_details'), href: %r{admin\/products\/#{product.permalink}\/edit\?#{filter_query}})
+      expect(page).to have_link(I18n.t('admin.products.tabs.images'), href: %r{admin\/products\/#{product.permalink}\/images\?#{filter_query}})
+      expect(page).to have_link(I18n.t('admin.products.tabs.variants'), href: %r{admin\/products\/#{product.permalink}\/variants\?#{filter_query}})
+      expect(page).to have_link(I18n.t('admin.products.tabs.product_properties'), href: %r{admin\/products\/#{product.permalink}\/product_properties\?#{filter_query}})
+      expect(page).to have_link(I18n.t('admin.products.tabs.group_buy_options'), href: %r{admin\/products\/#{product.permalink}\/group_buy_options\?#{filter_query}})
+      expect(page).to have_link(I18n.t(:search), href: %r{admin\/products\/#{product.permalink}\/seo\?#{filter_query}})
     end
 
     scenario "editing product group buy options" do
diff --git a/spec/helpers/spree/admin/product_filter_helper_spec.rb b/spec/helpers/spree/admin/product_filter_helper_spec.rb
new file mode 100644
index 0000000000..d9177b9da0
--- /dev/null
+++ b/spec/helpers/spree/admin/product_filter_helper_spec.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Spree::Admin::ProductFilterHelper, type: :helper do
+  describe 'product_filters' do
+    it 'should return only parameters included in PRODUCT_FILTER' do
+      parameters = { 'query' => 'test', 'categoryFilter' => 2, 'randomFilter' => 5 }
+
+      filters = helper.product_filters(parameters)
+
+      puts filters.inspect
+      expect(filters.key?('query')).to be true
+      expect(filters.key?('categoryFilter')).to be true
+      expect(filters.key?('randomFilter')).to be false
+    end
+  end
+end

From 459708dbc8b7ad050b7e0ffb55f4329f78639d60 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 17 Jul 2020 17:10:32 +1000
Subject: [PATCH 05/17] add product filter parameters on the various product
 variants pages, so that the bulk import product page filters can be preserved

---
 .../spree/admin/variants_controller.rb        | 43 ++++++++++-
 app/views/spree/admin/variants/edit.html.haml |  7 +-
 .../spree/admin/variants/index.html.haml      |  9 ++-
 app/views/spree/admin/variants/new.html.haml  |  8 +-
 spec/features/admin/variants_spec.rb          | 76 ++++++++++++++++---
 5 files changed, 122 insertions(+), 21 deletions(-)

diff --git a/app/controllers/spree/admin/variants_controller.rb b/app/controllers/spree/admin/variants_controller.rb
index 0936ffd155..0cda767181 100644
--- a/app/controllers/spree/admin/variants_controller.rb
+++ b/app/controllers/spree/admin/variants_controller.rb
@@ -4,14 +4,51 @@ module Spree
   module Admin
     class VariantsController < ResourceController
       helper 'spree/products'
+      include ProductFilterHelper
+
       belongs_to 'spree/product', find_by: :permalink
       new_action.before :new_before
 
+      def index
+        @url_filters = product_filters(request.query_parameters)
+      end
+
+      def edit
+        @url_filters = product_filters(request.query_parameters)
+
+        super
+      end
+
+      def update
+        @url_filter = product_filters(request.query_parameters)
+
+        if @object.update(permitted_resource_params)
+          flash[:success] = flash_message_for(@object, :successfully_updated)
+          redirect_to admin_product_variants_url(params[:product_id], @url_filter)
+        else
+          redirect_to edit_admin_product_variant_url(params[:product_id], @object, @url_filter)
+        end
+      end
+
+      def new
+        @url_filters = product_filters(request.query_parameters)
+
+        super
+      end
+
       def create
+        @url_filter = product_filters(request.query_parameters)
+
         on_demand = params[:variant].delete(:on_demand)
         on_hand = params[:variant].delete(:on_hand)
 
-        super
+        @object.attributes = permitted_resource_params
+        if @object.save
+          flash[:success] = flash_message_for(@object, :successfully_created)
+          redirect_to admin_product_variants_url(params[:product_id], @url_filter)
+        else
+          redirect_to new_admin_product_variant_url(params[:product_id], @url_filter)
+        end
 
         return unless @object.present? && @object.valid?
 
@@ -26,6 +63,8 @@ module Spree
       end
 
       def destroy
+        @url_filter = product_filters(request.query_parameters)
+
         @variant = Spree::Variant.find(params[:id])
         flash[:success] = if VariantDeleter.new.delete(@variant)
                             Spree.t('notice_messages.variant_deleted')
@@ -34,7 +73,7 @@ module Spree
                           end
 
         respond_with(@variant) do |format|
-          format.html { redirect_to admin_product_variants_url(params[:product_id]) }
+          format.html { redirect_to admin_product_variants_url(params[:product_id], @url_filter) }
         end
       end
 
diff --git a/app/views/spree/admin/variants/edit.html.haml b/app/views/spree/admin/variants/edit.html.haml
index 057c8a9d10..5a90078848 100644
--- a/app/views/spree/admin/variants/edit.html.haml
+++ b/app/views/spree/admin/variants/edit.html.haml
@@ -4,8 +4,11 @@
 
 = render partial: 'spree/shared/error_messages', locals: { target: @variant }
 
-= form_for [:admin, @product, @variant] do |f|
+= form_for [:admin, @product, @variant], :url => admin_product_variant_path(@product, @variant, @url_filters) do |f|
   %fieldset.no-border-top
     %div
       = render partial: 'form', locals: { f: f }
-    = render partial: 'spree/admin/shared/edit_resource_links'
+  .form-buttons.filter-actions.actions
+    = button t(:update), 'icon-refresh'
+    %span.or= t(:or)
+    = button_link_to t(:cancel), collection_url(@url_filters), icon: 'icon-remove'
diff --git a/app/views/spree/admin/variants/index.html.haml b/app/views/spree/admin/variants/index.html.haml
index ce9c1edd86..fa2d206749 100644
--- a/app/views/spree/admin/variants/index.html.haml
+++ b/app/views/spree/admin/variants/index.html.haml
@@ -27,9 +27,10 @@
         %td.align-center= variant.display_price.to_html
         %td.align-center= variant.sku
         %td.actions
-          = link_to_edit(variant, no_text: true) unless variant.deleted?
-          = link_to_delete(variant, no_text: true) unless variant.deleted?
+          = link_to_with_icon('icon-edit', Spree.t(:edit), edit_object_url(variant, @url_filters), no_text: true) unless variant.deleted?
+          = link_to_delete(variant, { url: object_url(variant, @url_filters), no_text: true }) unless variant.deleted?
 
+// TODO this
 - if @product.empty_option_values?
   %p.first_add_option_types.no-objects-found
     = t('.to_add_variants_you_must_first_define')
@@ -41,6 +42,6 @@
   - content_for :page_actions do
     %ul.inline-menu
       %li#new_var_link
-        = link_to_with_icon('icon-plus', t('.new_variant'), new_admin_product_variant_url(@product), class: 'button')
+        = link_to_with_icon('icon-plus', t('.new_variant'), new_admin_product_variant_url(@product, @url_filters), class: 'button')
 
-      %li= link_to_with_icon('icon-filter', @deleted.blank? ? t('.show_deleted') : t('.show_active'), admin_product_variants_url(@product, deleted: @deleted.blank? ? "on" : "off"), class: 'button')
+      %li= link_to_with_icon('icon-filter', @deleted.blank? ? t('.show_deleted') : t('.show_active'), admin_product_variants_url(@product, @url_filters.merge(deleted: @deleted.blank? ? "on" : "off")), class: 'button')
diff --git a/app/views/spree/admin/variants/new.html.haml b/app/views/spree/admin/variants/new.html.haml
index 1ff9927713..30105a5c20 100644
--- a/app/views/spree/admin/variants/new.html.haml
+++ b/app/views/spree/admin/variants/new.html.haml
@@ -1,7 +1,11 @@
 = render partial: 'spree/shared/error_messages', locals: { target: @variant }
 
-= form_for [:admin, @product, @variant] do |f|
+= form_for [:admin, @product, @variant], :url => admin_product_variants_path(@product, @url_filters) do |f|
   %fieldset{'data-hook' => "admin_variant_new_form"}
     %legend{align: "center"}= t('.new_variant')
     = render partial: 'form', locals: { f: f }
-    = render partial: 'spree/admin/shared/new_resource_links'
+
+  .form-buttons.filter-actions.actions
+    = button t('actions.create'), 'icon-ok'
+    %span.or= t(:or)
+    = button_link_to t('actions.cancel'), collection_url(@url_filters), icon: 'icon-remove'
diff --git a/spec/features/admin/variants_spec.rb b/spec/features/admin/variants_spec.rb
index 2b5df3c0aa..0ec536bd99 100644
--- a/spec/features/admin/variants_spec.rb
+++ b/spec/features/admin/variants_spec.rb
@@ -7,23 +7,77 @@ feature '
   include AuthenticationHelper
   include WebHelper
 
-  scenario "creating a new variant" do
-    # Given a product with a unit-related option type
-    product = create(:simple_product, variant_unit: "weight", variant_unit_scale: "1")
+  describe "new variant", js: true do
+    scenario "creating a new variant" do
+      # Given a product with a unit-related option type
+      product = create(:simple_product, variant_unit: "weight", variant_unit_scale: "1")
 
-    # When I create a variant on the product
-    login_as_admin_and_visit spree.admin_product_variants_path product
-    click_link 'New Variant'
 
-    fill_in 'unit_value_human', with: '1'
-    fill_in 'variant_unit_description', with: 'foo'
-    click_button 'Create'
+      # When I create a variant on the product
+      login_as_admin_and_visit spree.admin_product_variants_path product
+      click_link 'New Variant'
 
-    # Then the variant should have been created
-    expect(page).to have_content "Variant \"#{product.name}\" has been successfully created!"
+      fill_in 'unit_value_human', with: '1'
+      fill_in 'variant_unit_description', with: 'foo'
+      click_button 'Create'
+
+      # Then the variant should have been created
+      expect(page).to have_content "Variant \"#{product.name}\" has been successfully created!"
+    end
+
+    scenario "creating a new variant from product variant page with filter" do
+      # Given a product with a unit-related option type
+      product = create(:simple_product, variant_unit: "weight", variant_unit_scale: "1")
+      filter = { producerFilter: 2 }
+
+      # When I create a variant on the product
+      login_as_admin_and_visit spree.admin_product_variants_path(product, filter)
+
+      click_link 'New Variant'
+
+      # Cancel link should include product filter
+      expect(page).to have_link(I18n.t('actions.cancel'), href: %r{variants\?#{filter.to_query}})
+    end
+  end
+
+  describe "viewing product variant" do
+    scenario "when the product page has a product filter" do
+      # Given a product with a unit-related option type
+      product = create(:simple_product, variant_unit: "weight", variant_unit_scale: "1")
+      filter = { producerFilter: 2 }
+
+      # When I create a variant on the product
+      login_as_admin_and_visit spree.admin_product_variants_path(product, filter)
+
+      visit spree.admin_product_variants_path(product, filter)
+
+      expect(page).to have_link("New Variant", href: %r{variants\/new\?#{filter.to_query}})
+      expect(page).to have_link("Show Deleted", href: %r{variants\?deleted=on&#{filter.to_query}})
+
+      # Variant link should include product filter
+      variant = product.variants.first
+      expect(page).to have_link(
+        I18n.t(:edit), href: %r{variants\/#{variant.id}\/edit\?#{filter.to_query}}
+      )
+      expect(page).to have_link(
+        I18n.t(:delete), href: %r{variants\/#{variant.id}\?#{filter.to_query}}
+      )
+    end
   end
 
   describe "editing unit value and description for a variant", js: true do
+    scenario "when the product variant page has product filter" do
+      product = create(:simple_product, variant_unit: "weight", variant_unit_scale: "1")
+      filter = { producerFilter: 2 }
+
+      # When I create a variant on the product
+      login_as_admin_and_visit spree.admin_product_variants_path(product, filter)
+      page.find('table.index .icon-edit').click
+
+      # Cancel link should include product filter
+      expect(page).to have_link(I18n.t('actions.cancel'), href: %r{variants\?#{filter.to_query}})
+    end
+
     scenario "when variant_unit is weight" do
       # Given a product with unit-related option types, with a variant
       product = create(:simple_product, variant_unit: "weight", variant_unit_scale: "1")

From 684ae2ca22a5b24959371720237a2f41350329a8 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 24 Jul 2020 15:25:37 +1000
Subject: [PATCH 06/17] update product feature test to use ulr helpers

---
 .../admin/bulk_product_update_spec.rb         |  8 ++---
 spec/features/admin/products_spec.rb          | 31 +++++++++++++------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/spec/features/admin/bulk_product_update_spec.rb b/spec/features/admin/bulk_product_update_spec.rb
index fdf7d57104..6265c204eb 100644
--- a/spec/features/admin/bulk_product_update_spec.rb
+++ b/spec/features/admin/bulk_product_update_spec.rb
@@ -521,7 +521,7 @@ feature '
           find("a.edit-product").click
         end
 
-        expect(URI.parse(current_url).path).to eq "/admin/products/#{p1.permalink}/edit"
+        expect(URI.parse(current_url).path).to eq spree.edit_admin_product_path(v1.product.permalink)
       end
 
       it "shows an edit button for products, which takes the user to the standard edit page for that product, url includes selected filter" do
@@ -536,7 +536,7 @@ feature '
         end
 
         uri = URI.parse(current_url)
-        expect("#{uri.path}?#{uri.query}").to eq "/admin/products/#{p1.permalink}/edit?producerFilter=#{p1.supplier.id}"
+        expect("#{uri.path}?#{uri.query}").to eq spree.edit_admin_product_path(v1.product.permalink, producerFilter: p1.supplier.id)
       end
 
       it "shows an edit button for variants, which takes the user to the standard edit page for that variant" do
@@ -550,7 +550,7 @@ feature '
         end
 
         uri = URI.parse(current_url)
-        expect(URI.parse(current_url).path).to eq "/admin/products/#{v1.product.permalink}/variants/#{v1.id}/edit"
+        expect(URI.parse(current_url).path).to eq spree.edit_admin_product_variant_path(v1.product.permalink, v1.id)
       end
 
       it "shows an edit button for variants, which takes the user to the standard edit page for that variant, url includes selected filter" do
@@ -568,7 +568,7 @@ feature '
         end
 
         uri = URI.parse(current_url)
-        expect("#{uri.path}?#{uri.query}").to eq "/admin/products/#{v1.product.permalink}/variants/#{v1.id}/edit?producerFilter=#{p1.supplier.id}"
+        expect("#{uri.path}?#{uri.query}").to eq spree.edit_admin_product_variant_path(v1.product.permalink, v1.id, producerFilter: p1.supplier.id)
       end
     end
 
diff --git a/spec/features/admin/products_spec.rb b/spec/features/admin/products_spec.rb
index 06914be930..fd6be7a41d 100644
--- a/spec/features/admin/products_spec.rb
+++ b/spec/features/admin/products_spec.rb
@@ -162,19 +162,30 @@ feature '
 
       # Check the url still includes the filters
       uri = URI.parse(current_url)
-      expect("#{uri.path}?#{uri.query}").to eq "#{spree.edit_admin_product_path(product)}?#{filter_query}"
+      expect("#{uri.path}?#{uri.query}").to eq spree.edit_admin_product_path(product, producerFilter: 2)
 
       # Link back to the bulk product update page should include the filters
-      expect(page).to have_link(I18n.t('admin.products.back_to_products_list'), href: %r{admin\/products#\?#{filter_query}})
-      expect(page).to have_link(I18n.t(:cancel), href: %r{admin\/products#\?#{filter_query}})
+      expected_admin_product_url = Regexp.new(Regexp.escape("#{spree.admin_products_path}#?#{filter_query}"))
+      expect(page).to have_link(I18n.t('admin.products.back_to_products_list'), href: expected_admin_product_url)
+      expect(page).to have_link(I18n.t(:cancel), href: expected_admin_product_url)
 
-      # Sidebar link should include the filters
-      expect(page).to have_link(I18n.t('admin.products.tabs.product_details'), href: %r{admin\/products\/#{product.permalink}\/edit\?#{filter_query}})
-      expect(page).to have_link(I18n.t('admin.products.tabs.images'), href: %r{admin\/products\/#{product.permalink}\/images\?#{filter_query}})
-      expect(page).to have_link(I18n.t('admin.products.tabs.variants'), href: %r{admin\/products\/#{product.permalink}\/variants\?#{filter_query}})
-      expect(page).to have_link(I18n.t('admin.products.tabs.product_properties'), href: %r{admin\/products\/#{product.permalink}\/product_properties\?#{filter_query}})
-      expect(page).to have_link(I18n.t('admin.products.tabs.group_buy_options'), href: %r{admin\/products\/#{product.permalink}\/group_buy_options\?#{filter_query}})
-      expect(page).to have_link(I18n.t(:search), href: %r{admin\/products\/#{product.permalink}\/seo\?#{filter_query}})
+      expected_product_url = Regexp.new(Regexp.escape(spree.edit_admin_product_path(product.permalink, producerFilter: 2)))
+      expect(page).to have_link(I18n.t('admin.products.tabs.product_details'), href: expected_product_url)
+
+      expected_product_image_url = Regexp.new(Regexp.escape(spree.admin_product_images_path(product.permalink, producerFilter: 2)))
+      expect(page).to have_link(I18n.t('admin.products.tabs.images'), href: expected_product_image_url)
+
+      expected_product_variant_url = Regexp.new(Regexp.escape(spree.admin_product_variants_path(product.permalink, producerFilter: 2)))
+      expect(page).to have_link(I18n.t('admin.products.tabs.variants'), href: expected_product_variant_url)
+
+      expected_product_properties_url = Regexp.new(Regexp.escape(spree.admin_product_product_properties_path(product.permalink, producerFilter: 2)))
+      expect(page).to have_link(I18n.t('admin.products.tabs.product_properties'), href: expected_product_properties_url)
+
+      expected_product_group_buy_option_url = Regexp.new(Regexp.escape(spree.group_buy_options_admin_product_path(product.permalink, producerFilter: 2)))
+      expect(page).to have_link(I18n.t('admin.products.tabs.group_buy_options'), href: expected_product_group_buy_option_url)
+
+      expected_product_seo_url = Regexp.new(Regexp.escape(spree.seo_admin_product_path(product.permalink, producerFilter: 2)))
+      expect(page).to have_link(I18n.t(:search), href: expected_product_seo_url)
     end
 
     scenario "editing product group buy options" do

From 9b26ff2fa48d2447eb6e0450535b0422e3a58b5b Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 31 Jul 2020 15:56:19 +1000
Subject: [PATCH 07/17] move product filter helper to a service

---
 .../spree/admin/products_controller.rb        | 14 ++++-----
 .../spree/admin/variants_controller.rb        | 29 +++++++------------
 .../spree/admin/product_filter_helper.rb      | 15 ----------
 app/services/product_filters.rb               | 11 +++++++
 spec/services/product_filters_spec.rb         | 17 +++++++++++
 5 files changed, 45 insertions(+), 41 deletions(-)
 delete mode 100644 app/helpers/spree/admin/product_filter_helper.rb
 create mode 100644 app/services/product_filters.rb
 create mode 100644 spec/services/product_filters_spec.rb

diff --git a/app/controllers/spree/admin/products_controller.rb b/app/controllers/spree/admin/products_controller.rb
index 81a561f94d..b8b22597d8 100644
--- a/app/controllers/spree/admin/products_controller.rb
+++ b/app/controllers/spree/admin/products_controller.rb
@@ -9,7 +9,6 @@ module Spree
       include OpenFoodNetwork::SpreeApiKeyLoader
       include OrderCyclesHelper
       include EnterprisesHelper
-      include ProductFilterHelper
 
       before_action :load_data
       before_action :load_form_data, only: [:index, :new, :create, :edit, :update]
@@ -18,13 +17,13 @@ module Spree
 
       def new
         @object.shipping_category = DefaultShippingCategory.find_or_create
-        super
       end
 
       def create
         delete_stock_params_and_set_after do
-          @prototype = Spree::Prototype.find(params[:product][:prototype_id]) if \
-            params[:product][:prototype_id].present?
+          if params[:product][:prototype_id].present?
+            @prototype = Spree::Prototype.find(params[:product][:prototype_id])
+          end
 
           @object.attributes = permitted_resource_params
           if @object.save
@@ -55,12 +54,11 @@ module Spree
       end
 
       def edit
-        @url_filters = product_filters(params)
-        super
+        @url_filters = ::ProductFilters.new.extract(params)
       end
 
       def update
-        @url_filter = product_filters(request.query_parameters)
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
 
         original_supplier_id = @product.supplier_id
         delete_stock_params_and_set_after do
@@ -72,7 +70,7 @@ module Spree
 
             flash[:success] = flash_message_for(@object, :successfully_updated)
           end
-          redirect_to edit_admin_product_url(@object, @url_filter)
+          redirect_to edit_admin_product_url(@object, @url_filters)
         end
       end
 
diff --git a/app/controllers/spree/admin/variants_controller.rb b/app/controllers/spree/admin/variants_controller.rb
index 0cda767181..302b3e62a5 100644
--- a/app/controllers/spree/admin/variants_controller.rb
+++ b/app/controllers/spree/admin/variants_controller.rb
@@ -4,40 +4,35 @@ module Spree
   module Admin
     class VariantsController < ResourceController
       helper 'spree/products'
-      include ProductFilterHelper
 
       belongs_to 'spree/product', find_by: :permalink
       new_action.before :new_before
 
       def index
-        @url_filters = product_filters(request.query_parameters)
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
       end
 
       def edit
-        @url_filters = product_filters(request.query_parameters)
-
-        super
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
       end
 
       def update
-        @url_filter = product_filters(request.query_parameters)
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
 
         if @object.update(permitted_resource_params)
           flash[:success] = flash_message_for(@object, :successfully_updated)
-          redirect_to admin_product_variants_url(params[:product_id], @url_filter)
+          redirect_to admin_product_variants_url(params[:product_id], @url_filters)
         else
-          redirect_to edit_admin_product_variant_url(params[:product_id], @object, @url_filter)
+          redirect_to edit_admin_product_variant_url(params[:product_id], @object, @url_filters)
         end
       end
 
       def new
-        @url_filters = product_filters(request.query_parameters)
-
-        super
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
       end
 
       def create
-        @url_filter = product_filters(request.query_parameters)
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
 
         on_demand = params[:variant].delete(:on_demand)
         on_hand = params[:variant].delete(:on_hand)
@@ -45,9 +40,9 @@ module Spree
         @object.attributes = permitted_resource_params
         if @object.save
           flash[:success] = flash_message_for(@object, :successfully_created)
-          redirect_to admin_product_variants_url(params[:product_id], @url_filter)
+          redirect_to admin_product_variants_url(params[:product_id], @url_filters)
         else
-          redirect_to new_admin_product_variant_url(params[:product_id], @url_filter)
+          redirect_to new_admin_product_variant_url(params[:product_id], @url_filters)
         end
 
         return unless @object.present? && @object.valid?
@@ -63,7 +58,7 @@ module Spree
       end
 
       def destroy
-        @url_filter = product_filters(request.query_parameters)
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
 
         @variant = Spree::Variant.find(params[:id])
         flash[:success] = if VariantDeleter.new.delete(@variant)
@@ -72,9 +67,7 @@ module Spree
                             Spree.t('notice_messages.variant_not_deleted')
                           end
 
-        respond_with(@variant) do |format|
-          format.html { redirect_to admin_product_variants_url(params[:product_id], @url_filter) }
-        end
+        redirect_to admin_product_variants_url(params[:product_id], @url_filters)
       end
 
       protected
diff --git a/app/helpers/spree/admin/product_filter_helper.rb b/app/helpers/spree/admin/product_filter_helper.rb
deleted file mode 100644
index 644cdc1d56..0000000000
--- a/app/helpers/spree/admin/product_filter_helper.rb
+++ /dev/null
@@ -1,15 +0,0 @@
-# frozen_string_literal: true
-
-module Spree
-  module Admin
-    module ProductFilterHelper
-      PRODUCT_FILTER = [
-        'query', 'producerFilter', 'categoryFilter', 'sorting', 'importDateFilter'
-      ].freeze
-
-      def product_filters(params)
-        params.select { |k, _v| PRODUCT_FILTER.include?(k) }
-      end
-    end
-  end
-end
diff --git a/app/services/product_filters.rb b/app/services/product_filters.rb
new file mode 100644
index 0000000000..3a3c09ee63
--- /dev/null
+++ b/app/services/product_filters.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class ProductFilters
+  PRODUCT_FILTERS = [
+    'query', 'producerFilter', 'categoryFilter', 'sorting', 'importDateFilter'
+  ].freeze
+
+  def extract(params)
+    params.select { |key, _value| PRODUCT_FILTERS.include?(key) }
+  end
+end
diff --git a/spec/services/product_filters_spec.rb b/spec/services/product_filters_spec.rb
new file mode 100644
index 0000000000..056e3c1681
--- /dev/null
+++ b/spec/services/product_filters_spec.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ProductFilters do
+  describe "extract" do
+    it "should return a hash including only key from ProductFilters::PRODUCT_FILTERS" do
+      params = { 'id' => 20, 'producerFilter' => 2, 'categoryFilter' => 5 }
+
+      filters = ProductFilters.new.extract(params)
+
+      expect(filters).not_to include 'id'
+      expect(filters).to include 'producerFilter'
+      expect(filters).to include 'categoryFilter'
+    end
+  end
+end

From a6444e76a567b573a05edeacce61dc0ac456a6bf Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 31 Jul 2020 16:13:38 +1000
Subject: [PATCH 08/17] add product filter parameters on the various product
 image pages, so that the bulk import product page filters can be preserved

---
 .../spree/admin/images_controller.rb          |  52 ++++++++-
 app/views/spree/admin/images/edit.html.haml   |   6 +-
 app/views/spree/admin/images/index.html.haml  |   8 +-
 app/views/spree/admin/images/new.html.haml    |   4 +-
 spec/features/admin/products_spec.rb          | 109 ++++++++++++++++--
 5 files changed, 157 insertions(+), 22 deletions(-)

diff --git a/app/controllers/spree/admin/images_controller.rb b/app/controllers/spree/admin/images_controller.rb
index 8f47fe3576..c73e4e6826 100644
--- a/app/controllers/spree/admin/images_controller.rb
+++ b/app/controllers/spree/admin/images_controller.rb
@@ -8,9 +8,55 @@ module Spree
 
       before_action :load_data
 
-      create.before :set_viewable
-      update.before :set_viewable
-      destroy.before :destroy_before
+      def index
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
+      end
+
+      def new
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
+
+        render layout: !request.xhr?
+      end
+
+      def create
+        @url_filters = ::ProductFilters.new.extract(params)
+        set_viewable
+
+        @object.attributes = permitted_resource_params
+        if @object.save
+          flash[:success] = flash_message_for(@object, :successfully_created)
+          redirect_to admin_product_images_url(params[:product_id], @url_filters)
+        else
+          respond_with(@object)
+        end
+      end
+
+      def edit
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
+      end
+
+      def update
+        @url_filters = ::ProductFilters.new.extract(params)
+        set_viewable
+
+        if @object.update(permitted_resource_params)
+          flash[:success] = flash_message_for(@object, :successfully_updated)
+          redirect_to admin_product_images_url(params[:product_id], @url_filters)
+        else
+          respond_with(@object)
+        end
+      end
+
+      def destroy
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
+        destroy_before
+
+        if @object.destroy
+          flash[:success] = flash_message_for(@object, :successfully_removed)
+        end
+
+        redirect_to admin_product_images_url(params[:product_id], @url_filters)
+      end
 
       private
 
diff --git a/app/views/spree/admin/images/edit.html.haml b/app/views/spree/admin/images/edit.html.haml
index 9b691479b4..4a7910a223 100644
--- a/app/views/spree/admin/images/edit.html.haml
+++ b/app/views/spree/admin/images/edit.html.haml
@@ -3,9 +3,9 @@
 = render partial: 'spree/shared/error_messages', locals: { target: @image }
 
 - content_for :page_actions do
-  %li= button_link_to t('spree.back_to_images_list'), admin_product_images_url(@product), icon: 'icon-arrow-left'
+  %li= button_link_to t('spree.back_to_images_list'), admin_product_images_url(@product, @url_filters), icon: 'icon-arrow-left'
 
-= form_for [:admin, @product, @image], html: { multipart: true } do |f|
+= form_for [:admin, @product, @image], url: admin_product_image_path(@product, @image, @url_filters), html: { multipart: true } do |f|
   %fieldset
     %legend{align: "center"}= @image.attachment_file_name
     .field.alpha.three.columns.align-center
@@ -18,4 +18,4 @@
     .form-buttons.filter-actions.actions
       = button t('spree.actions.update'), 'icon-refresh'
       %span.or= t('spree.or')
-      = link_to t('spree.actions.cancel'), admin_product_images_url(@product), id: 'cancel_link', class: 'button icon-remove'
+      = link_to t('spree.actions.cancel'), admin_product_images_url(@product, @url_filters), id: 'cancel_link', class: 'button icon-remove'
diff --git a/app/views/spree/admin/images/index.html.haml b/app/views/spree/admin/images/index.html.haml
index 37cc53c70b..7f777c573a 100644
--- a/app/views/spree/admin/images/index.html.haml
+++ b/app/views/spree/admin/images/index.html.haml
@@ -2,7 +2,7 @@
 = render partial: 'spree/admin/shared/product_tabs', locals: { current: 'Images'}
 
 - content_for :page_actions do
-  %li= link_to_with_icon('icon-plus', t('spree.new_image'), new_admin_product_image_url(@product), id: 'new_image_link', class: 'button')
+  %li= link_to_with_icon('icon-plus', t('spree.new_image'), new_admin_product_image_url(@product, @url_filters), id: 'new_image_link', class: 'button')
 
 #images
 
@@ -11,7 +11,7 @@
     = t('spree.no_images_found')
     \.
 - else
-  %table.index.sortable{ "data-sortable-link" => "#{update_positions_admin_product_images_url(@product)}" }
+  %table.index.sortable{ "data-sortable-link" => "#{update_positions_admin_product_images_url(@product, @url_filters)}" }
     %colgroup
       %col{ style: "width: 5%" }/
       %col{ style: "width: 10%" }/
@@ -36,5 +36,5 @@
           %td= options_text_for(image)
           %td= image.alt
           %td.actions
-            = link_to_with_icon 'icon-edit', t('spree.edit'), edit_admin_product_image_url(@product, image), no_text: true, data: { action: 'edit'}
-            = link_to_delete image, { url: admin_product_image_url(@product, image), no_text: true }
+            = link_to_with_icon 'icon-edit', t('spree.edit'), edit_admin_product_image_url(@product, image, @url_filters), no_text: true, data: { action: 'edit'}
+            = link_to_delete image, { url: admin_product_image_url(@product, image, @url_filters), no_text: true }
diff --git a/app/views/spree/admin/images/new.html.haml b/app/views/spree/admin/images/new.html.haml
index 4026cef6fb..651bddf3e1 100644
--- a/app/views/spree/admin/images/new.html.haml
+++ b/app/views/spree/admin/images/new.html.haml
@@ -1,10 +1,10 @@
-= form_for [:admin, @product, @image], html: { multipart: true } do |f|
+= form_for [:admin, @product, @image], url: admin_product_images_path(@product, @image, @url_filters), html: { multipart: true } do |f|
   %fieldset
     %legend{ align: "center" }= t('spree.new_image')
     = render partial: 'form', locals: { f: f }
     .form-buttons.filter-actions.actions
       = button t('spree.actions.update'), 'icon-refresh'
       %span.or= t('spree.or')
-      = link_to_with_icon 'icon-remove', t('spree.actions.cancel'), admin_product_images_url(@product), id: 'cancel_link', class: 'button'
+      = link_to_with_icon 'icon-remove', t('spree.actions.cancel'), admin_product_images_url(@product, @url_filters), id: 'cancel_link', class: 'button'
 
 = javascript_include_tag 'admin/spree/images/new.js'
diff --git a/spec/features/admin/products_spec.rb b/spec/features/admin/products_spec.rb
index fd6be7a41d..ca54f162e0 100644
--- a/spec/features/admin/products_spec.rb
+++ b/spec/features/admin/products_spec.rb
@@ -94,6 +94,7 @@ feature '
 
   context "as an enterprise user" do
     let!(:tax_category) { create(:tax_category) }
+    let(:filter) { { producerFilter: 2 } }
 
     before do
       @new_user = create(:user)
@@ -154,8 +155,7 @@ feature '
     scenario "editing a product comming from the bulk product update page with filter" do
       product = create(:simple_product, name: 'a product', supplier: @supplier2)
 
-      filter_query = "producerFilter=2"
-      visit "#{spree.edit_admin_product_path(product)}?#{filter_query}"
+      visit spree.edit_admin_product_path(product, filter)
 
       click_button 'Update'
       expect(flash_message).to eq('Product "a product" has been successfully updated!')
@@ -165,26 +165,26 @@ feature '
       expect("#{uri.path}?#{uri.query}").to eq spree.edit_admin_product_path(product, producerFilter: 2)
 
       # Link back to the bulk product update page should include the filters
-      expected_admin_product_url = Regexp.new(Regexp.escape("#{spree.admin_products_path}#?#{filter_query}"))
+      expected_admin_product_url = Regexp.new(Regexp.escape("#{spree.admin_products_path}#?#{filter.to_query}"))
       expect(page).to have_link(I18n.t('admin.products.back_to_products_list'), href: expected_admin_product_url)
       expect(page).to have_link(I18n.t(:cancel), href: expected_admin_product_url)
 
-      expected_product_url = Regexp.new(Regexp.escape(spree.edit_admin_product_path(product.permalink, producerFilter: 2)))
+      expected_product_url = Regexp.new(Regexp.escape(spree.edit_admin_product_path(product.permalink, filter)))
       expect(page).to have_link(I18n.t('admin.products.tabs.product_details'), href: expected_product_url)
 
-      expected_product_image_url = Regexp.new(Regexp.escape(spree.admin_product_images_path(product.permalink, producerFilter: 2)))
+      expected_product_image_url = Regexp.new(Regexp.escape(spree.admin_product_images_path(product.permalink, filter)))
       expect(page).to have_link(I18n.t('admin.products.tabs.images'), href: expected_product_image_url)
 
-      expected_product_variant_url = Regexp.new(Regexp.escape(spree.admin_product_variants_path(product.permalink, producerFilter: 2)))
+      expected_product_variant_url = Regexp.new(Regexp.escape(spree.admin_product_variants_path(product.permalink, filter)))
       expect(page).to have_link(I18n.t('admin.products.tabs.variants'), href: expected_product_variant_url)
 
-      expected_product_properties_url = Regexp.new(Regexp.escape(spree.admin_product_product_properties_path(product.permalink, producerFilter: 2)))
+      expected_product_properties_url = Regexp.new(Regexp.escape(spree.admin_product_product_properties_path(product.permalink, filter)))
       expect(page).to have_link(I18n.t('admin.products.tabs.product_properties'), href: expected_product_properties_url)
 
-      expected_product_group_buy_option_url = Regexp.new(Regexp.escape(spree.group_buy_options_admin_product_path(product.permalink, producerFilter: 2)))
+      expected_product_group_buy_option_url = Regexp.new(Regexp.escape(spree.group_buy_options_admin_product_path(product.permalink, filter)))
       expect(page).to have_link(I18n.t('admin.products.tabs.group_buy_options'), href: expected_product_group_buy_option_url)
 
-      expected_product_seo_url = Regexp.new(Regexp.escape(spree.seo_admin_product_path(product.permalink, producerFilter: 2)))
+      expected_product_seo_url = Regexp.new(Regexp.escape(spree.seo_admin_product_path(product.permalink, filter)))
       expect(page).to have_link(I18n.t(:search), href: expected_product_seo_url)
     end
 
@@ -239,7 +239,7 @@ feature '
       expect(p.reload.property('fooprop')).to be_nil
     end
 
-    scenario "loading new image page", js: true do
+    scenario "loading new product image page", js: true do
       product = create(:simple_product, supplier: @supplier2)
 
       visit spree.admin_product_images_path(product)
@@ -249,6 +249,80 @@ feature '
       expect(page).to have_selector "#image_attachment"
     end
 
+    scenario "loading new procut image page including url filters", js: true do
+      product = create(:simple_product, supplier: @supplier2)
+
+      visit spree.admin_product_images_path(product, filter)
+
+      page.find('a#new_image_link').click
+
+      uri = URI.parse(current_url)
+      # we stay on the same url as the new image content is loaded via an ajax call
+      expect("#{uri.path}?#{uri.query}").to eq spree.admin_product_images_path(product, filter)
+
+      expected_cancel_link = Regexp.new(Regexp.escape(spree.admin_product_images_path(product, filter)))
+      expect(page).to have_link(I18n.t(:cancel), href: expected_cancel_link)
+    end
+
+    scenario "upload a new product image including url filters", js: true do
+      file_path = Rails.root + "spec/support/fixtures/thinking-cat.jpg"
+      product = create(:simple_product, supplier: @supplier2)
+
+      visit spree.admin_product_images_path(product, filter)
+
+      page.find('a#new_image_link').click
+
+      attach_file('image_attachment', file_path)
+      click_button "Update"
+
+      uri = URI.parse(current_url)
+      expect("#{uri.path}?#{uri.query}").to eq spree.admin_product_images_path(product, filter)
+    end
+
+    scenario "loading image page including url filter", js: true do
+      product = create(:simple_product, supplier: @supplier2)
+
+      visit spree.admin_product_images_path(product, filter)
+
+      expected_new_image_link = Regexp.new(Regexp.escape(spree.new_admin_product_image_path(product, filter)))
+      expect(page).to have_link(I18n.t('spree.new_image'), href: expected_new_image_link)
+    end
+
+    scenario "loading edit product image page including url filter", js: true do
+      product = create(:simple_product, supplier: @supplier2)
+      image = File.open(File.expand_path('../../../app/assets/images/logo-white.png', __dir__))
+      image_object = Spree::Image.create(viewable_id: product.master.id, viewable_type: 'Spree::Variant', alt: "position 1", attachment: image, position: 1)
+
+      visit spree.admin_product_images_path(product, filter)
+
+      page.find("a.icon-edit").click
+
+      uri = URI.parse(current_url)
+      expect("#{uri.path}?#{uri.query}").to eq spree.edit_admin_product_image_path(product, image_object, filter)
+
+      expected_cancel_link = Regexp.new(Regexp.escape(spree.admin_product_images_path(product, filter)))
+      expect(page).to have_link(I18n.t(:cancel), href: expected_cancel_link)
+      expect(page).to have_link("Back To Images List", href: expected_cancel_link)
+    end
+
+    scenario "updating a product image including url filter", js: true do
+      product = create(:simple_product, supplier: @supplier2)
+      image = File.open(File.expand_path('../../../app/assets/images/logo-white.png', __dir__))
+      image_object = Spree::Image.create(viewable_id: product.master.id, viewable_type: 'Spree::Variant', alt: "position 1", attachment: image, position: 1)
+
+      file_path = Rails.root + "spec/support/fixtures/thinking-cat.jpg"
+
+      visit spree.admin_product_images_path(product, filter)
+
+      page.find("a.icon-edit").click
+
+      attach_file('image_attachment', file_path)
+      click_button "Update"
+
+      uri = URI.parse(current_url)
+      expect("#{uri.path}?#{uri.query}").to eq spree.admin_product_images_path(product, filter)
+    end
+
     scenario "deleting product images", js: true do
       product = create(:simple_product, supplier: @supplier2)
       image = File.open(File.expand_path('../../../app/assets/images/logo-white.png', __dir__))
@@ -265,5 +339,20 @@ feature '
       expect(page).to_not have_selector "table.index td img"
       expect(product.reload.images.count).to eq 0
     end
+
+    scenario "deleting product image including url filter", js: true do
+      product = create(:simple_product, supplier: @supplier2)
+      image = File.open(File.expand_path('../../../app/assets/images/logo-white.png', __dir__))
+      Spree::Image.create(viewable_id: product.master.id, viewable_type: 'Spree::Variant', alt: "position 1", attachment: image, position: 1)
+
+      visit spree.admin_product_images_path(product, filter)
+
+      accept_alert do
+        page.find('a.delete-resource').click
+      end
+
+      uri = URI.parse(current_url)
+      expect("#{uri.path}?#{uri.query}").to eq spree.admin_product_images_path(product, filter)
+    end
   end
 end

From 9bc928fd4830bf6259aa372360185f4cd6dd8e17 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 31 Jul 2020 16:50:29 +1000
Subject: [PATCH 09/17] update product variants feature test to use ulr helpers

---
 spec/features/admin/variants_spec.rb | 36 +++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/spec/features/admin/variants_spec.rb b/spec/features/admin/variants_spec.rb
index 0ec536bd99..1247db8d27 100644
--- a/spec/features/admin/variants_spec.rb
+++ b/spec/features/admin/variants_spec.rb
@@ -35,8 +35,14 @@ feature '
 
       click_link 'New Variant'
 
+      uri = URI.parse(current_url)
+      expect("#{uri.path}?#{uri.query}").to eq spree.new_admin_product_variant_path(product, filter)
+
       # Cancel link should include product filter
-      expect(page).to have_link(I18n.t('actions.cancel'), href: %r{variants\?#{filter.to_query}})
+      expected_cancel_url = Regexp.new(
+        Regexp.escape(spree.admin_product_variants_path(product, filter))
+      )
+      expect(page).to have_link(I18n.t('actions.cancel'), href: expected_cancel_url)
     end
   end
 
@@ -51,17 +57,28 @@ feature '
 
       visit spree.admin_product_variants_path(product, filter)
 
-      expect(page).to have_link("New Variant", href: %r{variants\/new\?#{filter.to_query}})
-      expect(page).to have_link("Show Deleted", href: %r{variants\?deleted=on&#{filter.to_query}})
+      expected_new_url = Regexp.new(
+        Regexp.escape(spree.new_admin_product_variant_path(product, filter))
+      )
+      expect(page).to have_link("New Variant", href: expected_new_url)
+
+      expected_show_delete_url = Regexp.new(
+        Regexp.escape(spree.admin_product_variants_path(product, { deleted: 'on' }.merge(filter)))
+      )
+      expect(page).to have_link("Show Deleted", href: expected_show_delete_url)
 
       # Variant link should include product filter
       variant = product.variants.first
-      expect(page).to have_link(
-        I18n.t(:edit), href: %r{variants\/#{variant.id}\/edit\?#{filter.to_query}}
+
+      expected_edit_url = Regexp.new(
+        Regexp.escape(spree.edit_admin_product_variant_path(product, variant, filter))
       )
-      expect(page).to have_link(
-        I18n.t(:delete), href: %r{variants\/#{variant.id}\?#{filter.to_query}}
+      expect(page).to have_link(I18n.t(:edit), href: expected_edit_url)
+
+      expected_delete_url = Regexp.new(
+        Regexp.escape(spree.admin_product_variant_path(product, variant, filter))
       )
+      expect(page).to have_link(I18n.t(:delete), href: expected_delete_url)
     end
   end
 
@@ -75,7 +92,10 @@ feature '
       page.find('table.index .icon-edit').click
 
       # Cancel link should include product filter
-      expect(page).to have_link(I18n.t('actions.cancel'), href: %r{variants\?#{filter.to_query}})
+      expected_cancel_url = Regexp.new(
+        Regexp.escape(spree.admin_product_variants_path(product, filter))
+      )
+      expect(page).to have_link(I18n.t('actions.cancel'), href: expected_cancel_url)
     end
 
     scenario "when variant_unit is weight" do

From c6e1f458cc93979e4b43d54cf99ceb4827047de8 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 7 Aug 2020 14:46:31 +1000
Subject: [PATCH 10/17] add product filter parameters on the various product
 properties pages, so that the bulk import product page filters can be
 preserved

---
 .../admin/product_properties_controller.rb    | 14 +++++
 .../_product_property_fields.html.haml        |  2 +-
 .../admin/product_properties/index.html.haml  |  7 ++-
 spec/features/admin/products_spec.rb          | 59 +++++++++++++++++--
 4 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/app/controllers/spree/admin/product_properties_controller.rb b/app/controllers/spree/admin/product_properties_controller.rb
index b236b7be7e..c376b15b51 100644
--- a/app/controllers/spree/admin/product_properties_controller.rb
+++ b/app/controllers/spree/admin/product_properties_controller.rb
@@ -5,6 +5,20 @@ module Spree
       before_action :find_properties
       before_action :setup_property, only: [:index]
 
+      def index
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
+      end
+
+      def destroy
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
+
+        if @object.destroy
+          flash[:success] = flash_message_for(@object, :successfully_removed)
+        end
+        # if destroy fails it won't show any errors to the user
+        redirect_to admin_product_product_properties_url(params[:product_id], @url_filters)
+      end
+
       private
 
       def find_properties
diff --git a/app/views/spree/admin/product_properties/_product_property_fields.html.haml b/app/views/spree/admin/product_properties/_product_property_fields.html.haml
index d4362c4366..758225a756 100644
--- a/app/views/spree/admin/product_properties/_product_property_fields.html.haml
+++ b/app/views/spree/admin/product_properties/_product_property_fields.html.haml
@@ -11,4 +11,4 @@
     = f.text_field :value, class: 'autocomplete'
   %td.actions
     - if f.object.persisted?
-      = link_to_delete f.object, no_text: true
+      = link_to_delete f.object, { url: admin_product_product_property_url(@product, f.object, @url_filters), no_text: true } 
diff --git a/app/views/spree/admin/product_properties/index.html.haml b/app/views/spree/admin/product_properties/index.html.haml
index 87886cb37b..a595ceadc7 100644
--- a/app/views/spree/admin/product_properties/index.html.haml
+++ b/app/views/spree/admin/product_properties/index.html.haml
@@ -9,7 +9,7 @@
     %li
       = link_to_add_fields t('.add_product_properties'), 'tbody#product_properties', class: 'icon-plus button'
 
-= form_for @product, url: admin_product_url(@product), method: :put do |f|
+= form_for @product, url: admin_product_url(@product, @url_filters), method: :put do |f|
   %fieldset.no-border-top
     .add_product_properties
     = image_tag 'select2-spinner.gif', plugin: 'spree', style: 'display:none;', id: 'busy_indicator'
@@ -46,7 +46,10 @@
               %td.actions
 
 
-    = render partial: 'spree/admin/shared/edit_resource_links'
+    .form-buttons.filter-actions.actions
+      = button t('spree.actions.update'), 'icon-refresh'
+      %span.or= t('spree.or')
+      = link_to t('spree.actions.cancel'), admin_product_product_properties_url(@product, @url_filters), id: 'cancel_link', class: 'button icon-remove'
 
     = hidden_field_tag 'clear_product_properties', 'true'
 
diff --git a/spec/features/admin/products_spec.rb b/spec/features/admin/products_spec.rb
index ca54f162e0..b6a3696fd3 100644
--- a/spec/features/admin/products_spec.rb
+++ b/spec/features/admin/products_spec.rb
@@ -162,7 +162,7 @@ feature '
 
       # Check the url still includes the filters
       uri = URI.parse(current_url)
-      expect("#{uri.path}?#{uri.query}").to eq spree.edit_admin_product_path(product, producerFilter: 2)
+      expect("#{uri.path}?#{uri.query}").to eq spree.edit_admin_product_path(product, filter)
 
       # Link back to the bulk product update page should include the filters
       expected_admin_product_url = Regexp.new(Regexp.escape("#{spree.admin_products_path}#?#{filter.to_query}"))
@@ -217,13 +217,25 @@ feature '
       expect(product.meta_keywords).to eq('Product Search Keywords')
     end
 
+    scenario "loading product properties page including url filters", js: true do
+      product = create(:simple_product, supplier: @supplier2)
+      visit spree.admin_product_product_properties_path(product, filter)
+
+      uri = URI.parse(current_url)
+      # we stay on the same url as the new image content is loaded via an ajax call
+      expect("#{uri.path}?#{uri.query}").to eq spree.admin_product_product_properties_path(product, filter)
+
+      expected_cancel_link = Regexp.new(Regexp.escape(spree.admin_product_product_properties_path(product, filter)))
+      expect(page).to have_link(I18n.t(:cancel), href: expected_cancel_link)
+    end
+
     scenario "deleting product properties", js: true do
       # Given a product with a property
-      p = create(:simple_product, supplier: @supplier2)
-      p.set_property('fooprop', 'fooval')
+      product = create(:simple_product, supplier: @supplier2)
+      product.set_property('fooprop', 'fooval')
 
       # When I navigate to the product properties page
-      visit spree.admin_product_product_properties_path(p)
+      visit spree.admin_product_product_properties_path(product)
       expect(page).to have_select2 'product_product_properties_attributes_0_property_name', selected: 'fooprop'
       expect(page).to have_field 'product_product_properties_attributes_0_value', with: 'fooval'
 
@@ -236,7 +248,42 @@ feature '
       # Then the property should have been deleted
       expect(page).not_to have_field 'product_product_properties_attributes_0_property_name', with: 'fooprop'
       expect(page).not_to have_field 'product_product_properties_attributes_0_value', with: 'fooval'
-      expect(p.reload.property('fooprop')).to be_nil
+      expect(product.reload.property('fooprop')).to be_nil
+    end
+
+    scenario "deleting product properties including url filters", js: true do
+      # Given a product with a property
+      product = create(:simple_product, supplier: @supplier2)
+      product.set_property('fooprop', 'fooval')
+
+      # When I navigate to the product properties page
+      visit spree.admin_product_product_properties_path(product, filter)
+
+      # And I delete the property
+      accept_alert do
+        page.all('a.delete-resource').first.click
+      end
+
+      uri = URI.parse(current_url)
+      expect("#{uri.path}?#{uri.query}").to eq spree.admin_product_product_properties_path(product, filter)
+    end
+
+    scenario "adding product properties including url filters", js: true do
+      # Given a product
+      product = create(:simple_product, supplier: @supplier2)
+      product.set_property('fooprop', 'fooval')
+
+      # When I navigate to the product properties page
+      visit spree.admin_product_product_properties_path(product, filter)
+
+      # And I add a property
+      select 'fooprop', from: 'product_product_properties_attributes_0_property_name'
+      fill_in 'product_product_properties_attributes_0_value', with: 'fooval2'
+
+      click_button 'Update'
+
+      uri = URI.parse(current_url)
+      expect("#{uri.path}?#{uri.query}").to eq spree.edit_admin_product_path(product, filter)
     end
 
     scenario "loading new product image page", js: true do
@@ -249,7 +296,7 @@ feature '
       expect(page).to have_selector "#image_attachment"
     end
 
-    scenario "loading new procut image page including url filters", js: true do
+    scenario "loading new product image page including url filters", js: true do
       product = create(:simple_product, supplier: @supplier2)
 
       visit spree.admin_product_images_path(product, filter)

From bba683469bda4ee4e27014b1c28d2c2116119ec4 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 7 Aug 2020 16:03:43 +1000
Subject: [PATCH 11/17] add product filter parameters on the group buy options
 and search pages, so that the bulk import product page filters can be
 preserved

---
 .../spree/admin/products_controller.rb        |  8 ++++
 .../products/group_buy_options.html.haml      | 13 +++--
 app/views/spree/admin/products/seo.html.haml  | 11 +++--
 spec/features/admin/products_spec.rb          | 47 ++++++++++++++++++-
 4 files changed, 69 insertions(+), 10 deletions(-)

diff --git a/app/controllers/spree/admin/products_controller.rb b/app/controllers/spree/admin/products_controller.rb
index b8b22597d8..0556c25a1a 100644
--- a/app/controllers/spree/admin/products_controller.rb
+++ b/app/controllers/spree/admin/products_controller.rb
@@ -100,6 +100,14 @@ module Spree
         redirect_to edit_admin_product_url(@new)
       end
 
+      def group_buy_options
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
+      end
+
+      def seo
+        @url_filters = ::ProductFilters.new.extract(request.query_parameters)
+      end
+
       protected
 
       def find_resource
diff --git a/app/views/spree/admin/products/group_buy_options.html.haml b/app/views/spree/admin/products/group_buy_options.html.haml
index fc7cc0107f..7cc3348d66 100644
--- a/app/views/spree/admin/products/group_buy_options.html.haml
+++ b/app/views/spree/admin/products/group_buy_options.html.haml
@@ -1,8 +1,11 @@
 = render partial: 'spree/admin/shared/product_sub_menu'
-= render :partial => 'spree/admin/shared/product_tabs', :locals => { :current => 'Group Buy Options' }
-= render :partial => 'spree/shared/error_messages', :locals => { :target => @product }
+= render partial: 'spree/admin/shared/product_tabs', locals: { :current => 'Group Buy Options' }
+= render partial: 'spree/shared/error_messages', locals: { :target => @product }
 
-= form_for [:admin, @product], :method => :put, :html => { :multipart => true } do |f|
+= form_for [:admin, @product], url: admin_product_url(@product, @url_filters),  method: :put, html: { :multipart => true } do |f|
   %fieldset.no-border-top
-    = render :partial => 'group_buy_form', :locals => { :f => f }
-    = render :partial => 'spree/admin/shared/edit_resource_links'
+    = render partial: 'group_buy_form', locals: { f: f }
+    .form-buttons.filter-actions.actions
+      = button t('spree.actions.update'), 'icon-refresh'
+      %span.or= t('spree.or')
+      = link_to t('spree.actions.cancel'), edit_admin_product_url(@product, @url_filters), id: 'cancel_link', class: 'button icon-remove'
diff --git a/app/views/spree/admin/products/seo.html.haml b/app/views/spree/admin/products/seo.html.haml
index 8f9a89a01b..40172c7c95 100644
--- a/app/views/spree/admin/products/seo.html.haml
+++ b/app/views/spree/admin/products/seo.html.haml
@@ -1,9 +1,12 @@
 = render partial: 'spree/admin/shared/product_sub_menu'
-= render :partial => 'spree/admin/shared/product_tabs', :locals => { :current => t(:search) }
-= render :partial => 'spree/shared/error_messages', :locals => { :target => @product }
+= render partial: 'spree/admin/shared/product_tabs', locals: { current: t(:search) }
+= render partial: 'spree/shared/error_messages', locals: { target: @product }
 
 %div{ 'ng-app' => 'ofn.admin' }
-  = form_for [:admin, @product], :method => :put, :html => { :multipart => true } do |f|
+  = form_for [:admin, @product], url: admin_product_url(@product, @url_filters),  method: :put, html: { :multipart => true } do |f|
     %fieldset.no-border-top
       = render :partial => 'seo_form', :locals => { :f => f }
-      = render :partial => 'spree/admin/shared/edit_resource_links'
+      .form-buttons.filter-actions.actions
+        = button t('spree.actions.update'), 'icon-refresh'
+        %span.or= t('spree.or')
+        = link_to t('spree.actions.cancel'), edit_admin_product_url(@product, @url_filters), id: 'cancel_link', class: 'button icon-remove'
diff --git a/spec/features/admin/products_spec.rb b/spec/features/admin/products_spec.rb
index b6a3696fd3..5e4abbc310 100644
--- a/spec/features/admin/products_spec.rb
+++ b/spec/features/admin/products_spec.rb
@@ -204,8 +204,30 @@ feature '
       expect(product.group_buy_unit_size).to eq(10.0)
     end
 
-    scenario "editing product Search" do
+    scenario "loading editing product group buy options with url filters" do
       product = product = create(:simple_product, supplier: @supplier2)
+
+      visit spree.group_buy_options_admin_product_path(product, filter)
+
+      expected_cancel_link = Regexp.new(Regexp.escape(spree.edit_admin_product_path(product, filter)))
+      expect(page).to have_link(I18n.t(:cancel), href: expected_cancel_link)
+    end
+
+    scenario "editing product group buy options with url filter" do
+      product = product = create(:simple_product, supplier: @supplier2)
+
+      visit spree.group_buy_options_admin_product_path(product, filter)
+      choose('product_group_buy_1')
+      fill_in 'Bulk unit size', with: '10'
+
+      click_button 'Update'
+
+      uri = URI.parse(current_url)
+      expect("#{uri.path}?#{uri.query}").to eq spree.edit_admin_product_path(product, filter)
+    end
+
+    scenario "editing product Search" do
+      product = create(:simple_product, supplier: @supplier2)
       visit spree.edit_admin_product_path product
       within('#sidebar') { click_link 'Search' }
       fill_in 'Product Search Keywords', with: 'Product Search Keywords'
@@ -217,6 +239,29 @@ feature '
       expect(product.meta_keywords).to eq('Product Search Keywords')
     end
 
+    scenario "loading editing product Search with url filters" do
+      product = create(:simple_product, supplier: @supplier2)
+
+      visit spree.seo_admin_product_path(product, filter)
+
+      expected_cancel_link = Regexp.new(Regexp.escape(spree.edit_admin_product_path(product, filter)))
+      expect(page).to have_link(I18n.t(:cancel), href: expected_cancel_link)
+    end
+
+    scenario "editing product Search with url filter" do
+      product = create(:simple_product, supplier: @supplier2)
+
+      visit spree.seo_admin_product_path(product, filter)
+
+      fill_in 'Product Search Keywords', with: 'Product Search Keywords'
+      fill_in 'Notes', with: 'Just testing Notes'
+
+      click_button 'Update'
+
+      uri = URI.parse(current_url)
+      expect("#{uri.path}?#{uri.query}").to eq spree.edit_admin_product_path(product, filter)
+    end
+
     scenario "loading product properties page including url filters", js: true do
       product = create(:simple_product, supplier: @supplier2)
       visit spree.admin_product_product_properties_path(product, filter)

From 7356d0fe77e87d597dd163d81d4d9d8bd6cc20b7 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 7 Aug 2020 18:10:51 +1000
Subject: [PATCH 12/17] move url filter functionality to service
 ProductFiltersService

---
 .../admin/bulk_product_update.js.coffee       | 63 ++++++++---------
 .../directives/select2_min_search.js.coffee   |  4 +-
 .../admin/services/product_filters.js.coffee  | 23 +++++++
 .../admin/products/index/_filters.html.haml   |  8 +--
 .../admin/bulk_product_update_spec.js.coffee  | 49 ++++++-------
 .../services/product_filters_spec.js.coffee   | 68 +++++++++++++++++++
 6 files changed, 151 insertions(+), 64 deletions(-)
 create mode 100644 app/assets/javascripts/admin/services/product_filters.js.coffee
 create mode 100644 spec/javascripts/unit/admin/services/product_filters_spec.js.coffee

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index 7571aaaaea..5c011de5bb 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -1,4 +1,4 @@
-angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout, $filter, $http, $window, $location, $httpParamSerializer, BulkProducts, DisplayProperties, DirtyProducts, VariantUnitManager, StatusMessage, producers, Taxons, Columns, tax_categories, RequestMonitor, SortOptions, ErrorsParser) ->
+angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout, $filter, $http, $window, $location, BulkProducts, DisplayProperties, DirtyProducts, VariantUnitManager, StatusMessage, producers, Taxons, Columns, tax_categories, RequestMonitor, SortOptions, ErrorsParser, ProductFiltersService) ->
   $scope.StatusMessage = StatusMessage
 
   $scope.columns = Columns.columns
@@ -14,11 +14,13 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
   ]
 
   productFilters = ['producerFilter', 'categoryFilter', 'query', 'sorting', 'importDateFilter']
-  $scope.producerFilter = ""
-  $scope.categoryFilter = ""
-  $scope.importDateFilter = ""
-  $scope.query = ""
-  $scope.sorting = ""
+  $scope.q = {
+    producerFilter: ""
+    categoryFilter: ""
+    importDateFilter: ""
+    query: ""
+    sorting: ""
+  }
 
   $scope.producers = producers
   $scope.taxons = Taxons.all
@@ -30,13 +32,8 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
 
   $scope.sortOptions = SortOptions
 
-  loadFilterFromUrl = ->
-    filters = $location.search()
-    for filter in productFilters
-      $scope[filter] = if filters[filter] then filters[filter] else ""
-
   $scope.initialise = ->
-    loadFilterFromUrl()
+    $scope.q = ProductFiltersService.loadFromUrl($location.search())
     $scope.fetchProducts()
 
   $scope.$watchCollection '[query, producerFilter, categoryFilter, importDateFilter, per_page]', ->
@@ -50,33 +47,33 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
     filters = {}
     for filter in productFilters
       filters[filter] = $scope[filter] if $scope[filter]
-    
+
     filters
 
   $scope.fetchProducts = ->
     removeClearedValues()
     params = {
-      'q[name_cont]': $scope.query,
-      'q[supplier_id_eq]': $scope.producerFilter,
-      'q[primary_taxon_id_eq]': $scope.categoryFilter,
-      'q[s]': $scope.sorting,
-      import_date: $scope.importDateFilter,
+      'q[name_cont]': $scope.q.query,
+      'q[supplier_id_eq]': $scope.q.producerFilter,
+      'q[primary_taxon_id_eq]': $scope.q.categoryFilter,
+      'q[s]': $scope.q.sorting,
+      import_date: $scope.q.importDateFilter,
       page: $scope.page,
       per_page: $scope.per_page
     }
     RequestMonitor.load(BulkProducts.fetch(params).$promise).then ->
       # update url with the filters used
-      $location.search(generateFilter())
+      $location.search(ProductFiltersService.generate($scope.q))
       $scope.resetProducts()
 
   removeClearedValues = ->
-    delete $scope.producerFilter if $scope.producerFilter == "0"
-    delete $scope.categoryFilter if $scope.categoryFilter == "0"
-    delete $scope.importDateFilter if $scope.importDateFilter == "0"
+    delete $scope.q.producerFilter if $scope.q.producerFilter == "0"
+    delete $scope.q.categoryFilter if $scope.q.categoryFilter == "0"
+    delete $scope.q.importDateFilter if $scope.q.importDateFilter == "0"
 
   $timeout ->
     if $scope.showLatestImport
-      $scope.importDateFilter = $scope.importDates[1].id
+      $scope.q.importDateFilter = $scope.importDates[1].id
 
   $scope.resetProducts = ->
     DirtyProducts.clear()
@@ -106,10 +103,10 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
     $scope.visibleTab = tab
 
   $scope.resetSelectFilters = ->
-    $scope.query = ""
-    $scope.producerFilter = "0"
-    $scope.categoryFilter = "0"
-    $scope.importDateFilter = "0"
+    $scope.q.query = ""
+    $scope.q.producerFilter = "0"
+    $scope.q.categoryFilter = "0"
+    $scope.q.importDateFilter = "0"
     $scope.fetchProducts()
 
   $scope.$watch 'sortOptions', (sort) ->
@@ -127,9 +124,7 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
 
   $scope.editWarn = (product, variant) ->
     if confirm_unsaved_changes()
-      filterUrl = $httpParamSerializer(generateFilter())
-      filterUrl = "?#{filterUrl}" if filterUrl isnt ""
-      $window.location.href = "#{editProductUrl(product, variant)}#{filterUrl}"
+      $window.location.href = ProductFiltersService.buildUrl(editProductUrl(product, variant), $scope.q)
 
   $scope.toggleShowAllVariants = ->
     showVariants = !DisplayProperties.showVariants 0
@@ -226,10 +221,10 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
       data:
         products: productsToSubmit
         filters:
-          'q[name_cont]': $scope.query
-          'q[supplier_id_eq]': $scope.producerFilter
-          'q[primary_taxon_id_eq]': $scope.categoryFilter
-          import_date: $scope.importDateFilter
+          'q[name_cont]': $scope.q.query
+          'q[supplier_id_eq]': $scope.q.producerFilter
+          'q[primary_taxon_id_eq]': $scope.q.categoryFilter
+          import_date: $scope.q.importDateFilter
         page: $scope.page
         per_page: $scope.per_page
     ).success((data) ->
diff --git a/app/assets/javascripts/admin/directives/select2_min_search.js.coffee b/app/assets/javascripts/admin/directives/select2_min_search.js.coffee
index 0d9e9faa75..dac4aa1b7f 100644
--- a/app/assets/javascripts/admin/directives/select2_min_search.js.coffee
+++ b/app/assets/javascripts/admin/directives/select2_min_search.js.coffee
@@ -5,8 +5,8 @@ angular.module("ofn.admin").directive "ofnSelect2MinSearch", ->
       minimumResultsForSearch: attrs.ofnSelect2MinSearch
 
     ngModel.$formatters.push (value) ->
-      # select2 populate options with a value like "number:3" or "string:category" but 
-      # select2('val', value) doesn't do the translation for us as one would expect 
+      # select2 populates options with a value like "number:3" or "string:category" but
+      # select2('val', value) doesn't do the type conversion for us as one would expect
       if isNaN(value)
         element.select2('val', "string:#{value}")
       else
diff --git a/app/assets/javascripts/admin/services/product_filters.js.coffee b/app/assets/javascripts/admin/services/product_filters.js.coffee
new file mode 100644
index 0000000000..c6d3574df0
--- /dev/null
+++ b/app/assets/javascripts/admin/services/product_filters.js.coffee
@@ -0,0 +1,23 @@
+angular.module("ofn.admin").factory "ProductFiltersService", ($httpParamSerializer) ->
+  new class ProductFiltersService
+    productFilters: ['producerFilter', 'categoryFilter', 'query', 'sorting', 'importDateFilter']
+
+    loadFromUrl: (filters) ->
+      loadedFilters = {}
+      for filter in @productFilters
+        loadedFilters[filter] = if filters[filter] then filters[filter] else ""
+
+      loadedFilters
+
+    generate: (ctrlFilters) ->
+      filters = {}
+      for filter in @productFilters
+        filters[filter] = ctrlFilters[filter] if ctrlFilters[filter]
+
+      filters
+
+    buildUrl: (baseUrl, ctrlFilters) ->
+      filterUrl = $httpParamSerializer(@generate(ctrlFilters))
+      filterUrl = "?#{filterUrl}" if filterUrl isnt ""
+
+      "#{baseUrl}#{filterUrl}"
diff --git a/app/views/spree/admin/products/index/_filters.html.haml b/app/views/spree/admin/products/index/_filters.html.haml
index 4493e95258..8c08b3af1b 100644
--- a/app/views/spree/admin/products/index/_filters.html.haml
+++ b/app/views/spree/admin/products/index/_filters.html.haml
@@ -5,20 +5,20 @@
     .quick_search.three.columns.alpha
       %label{ for: 'quick_filter' }
       %br
-      %input.quick-search.fullwidth{ ng: {model: 'query'}, name: "quick_filter", type: 'text', placeholder: t('admin.quick_search'), "ng-keypress" => "$event.keyCode === 13 && fetchProducts()" }
+      %input.quick-search.fullwidth{ ng: {model: 'q.query'}, name: "quick_filter", type: 'text', placeholder: t('admin.quick_search'), "ng-keypress" => "$event.keyCode === 13 && fetchProducts()" }
     .one.columns &nbsp;
     .filter_select.three.columns
       %label{ for: 'producer_filter' }= t 'producer'
       %br
-      %select.fullwidth{ id: 'producer_filter', 'ofn-select2-min-search' => 5, ng: {model: 'producerFilter', options: 'producer.id as producer.name for producer in producers'} }
+      %select.fullwidth{ id: 'producer_filter', 'ofn-select2-min-search' => 5, ng: {model: 'q.producerFilter', options: 'producer.id as producer.name for producer in producers'} }
     .filter_select.three.columns
       %label{ for: 'category_filter' }= t 'category'
       %br
-      %select.fullwidth{ id: 'category_filter', 'ofn-select2-min-search' => 5, ng: {model: 'categoryFilter', options: 'taxon.id as taxon.name for taxon in taxons'} }
+      %select.fullwidth{ id: 'category_filter', 'ofn-select2-min-search' => 5, ng: {model: 'q.categoryFilter', options: 'taxon.id as taxon.name for taxon in taxons'} }
     .filter_select.three.columns
       %label{ for: 'import_filter' } Import Date
       %br
-      %select.fullwidth{ id: 'import_date_filter', 'ofn-select2-min-search' => 5, ng: {model: 'importDateFilter', init: "importDates = #{@import_dates}; showLatestImport = #{@show_latest_import}", options: 'date.id as date.name for date in importDates'} }
+      %select.fullwidth{ id: 'import_date_filter', 'ofn-select2-min-search' => 5, ng: {model: 'q.importDateFilter', init: "importDates = #{@import_dates}; showLatestImport = #{@show_latest_import}", options: 'date.id as date.name for date in importDates'} }
 
     .filter_clear.three.columns.omega
       %label{ for: 'clear_all_filters' }
diff --git a/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee
index d6f7456856..b6dc92e051 100644
--- a/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee
@@ -246,7 +246,7 @@ describe "filtering products for submission to database", ->
     ]
 
 describe "AdminProductEditCtrl", ->
-  $ctrl = $scope = $timeout = $httpBackend = BulkProducts = DirtyProducts = DisplayProperties = windowStub = null
+  $ctrl = $scope = $timeout = $httpBackend = BulkProducts = DirtyProducts = DisplayProperties = ProductFiltersService = windowStub = null
 
   beforeEach ->
     module "ofn.admin"
@@ -258,7 +258,7 @@ describe "AdminProductEditCtrl", ->
       $provide.value 'columns', []
       null
 
-  beforeEach inject((_$controller_, _$timeout_, $rootScope, _$httpBackend_, _BulkProducts_, _DirtyProducts_, _DisplayProperties_) ->
+  beforeEach inject((_$controller_, _$timeout_, $rootScope, _$httpBackend_, _BulkProducts_, _DirtyProducts_, _DisplayProperties_, _ProductFiltersService_) ->
     $scope = $rootScope.$new()
     $ctrl = _$controller_
     $timeout = _$timeout_
@@ -266,8 +266,9 @@ describe "AdminProductEditCtrl", ->
     BulkProducts = _BulkProducts_
     DirtyProducts = _DirtyProducts_
     DisplayProperties = _DisplayProperties_
+    ProductFiltersService = _ProductFiltersService_
 
-    # Stub the window object so we don't get redirected when href is updated 
+    # Stub the window object so we don't get redirected when href is updated
     windowStub = {navigator: {userAgent: 'foo'}, location: {href: ''}}
 
     $ctrl "AdminProductEditCtrl", {$scope: $scope, $timeout: $timeout, $window: windowStub}
@@ -282,7 +283,7 @@ describe "AdminProductEditCtrl", ->
       expect($scope.fetchProducts.calls.count()).toBe 1
 
     it "gets a list of products applying filters from the url", inject ($location) ->
-      query = 'lala' 
+      query = 'lala'
       producerFilter = 2
       categoryFilter = 5
       sorting = 'name desc'
@@ -291,11 +292,11 @@ describe "AdminProductEditCtrl", ->
 
       $scope.initialise()
 
-      expect($scope.query).toBe query
-      expect($scope.producerFilter).toBe producerFilter
-      expect($scope.categoryFilter).toBe categoryFilter
-      expect($scope.sorting).toBe sorting
-      expect($scope.importDateFilter).toBe importDateFilter
+      expect($scope.q.query).toBe query
+      expect($scope.q.producerFilter).toBe producerFilter
+      expect($scope.q.categoryFilter).toBe categoryFilter
+      expect($scope.q.sorting).toBe sorting
+      expect($scope.q.importDateFilter).toBe importDateFilter
 
   describe "fetching products", ->
     $q = null
@@ -316,18 +317,18 @@ describe "AdminProductEditCtrl", ->
       $scope.$digest()
       expect($scope.resetProducts).toHaveBeenCalled()
 
-    it "updates url wihth filter after data has been received", inject ($location, $window) ->
+    it "updates url with filter after data has been received", inject ($location, $window) ->
       query = 'lala'
       producerFilter = 2
       categoryFilter = 5
       sorting = 'name desc'
       importDateFilter = '2020-06-08'
 
-      $scope.query = query
-      $scope.producerFilter = producerFilter
-      $scope.categoryFilter = categoryFilter
-      $scope.sorting = sorting
-      $scope.importDateFilter = importDateFilter
+      $scope.q.query = query
+      $scope.q.producerFilter = producerFilter
+      $scope.q.categoryFilter = categoryFilter
+      $scope.q.sorting = sorting
+      $scope.q.importDateFilter = importDateFilter
 
       $scope.fetchProducts()
       $scope.$digest()
@@ -995,14 +996,14 @@ describe "AdminProductEditCtrl", ->
       it 'should load edit product page including the selected filters', inject ($httpParamSerializer) ->
         query = 'lala'
         category = 3
-        $scope.query = query
-        $scope.categoryFilter = category
+        $scope.q.query = query
+        $scope.q.categoryFilter = category
 
         # use $httpParamSerializer as it will sort parameters alphabetically
         expectedFilter = $httpParamSerializer({ query: query, categoryFilter: category })
 
         $scope.editWarn(testProduct, null)
-        
+
         expect(windowStub.location.href).toBe(
           "/admin/products/#{testProduct.permalink_live}/edit?#{expectedFilter}"
         )
@@ -1010,13 +1011,13 @@ describe "AdminProductEditCtrl", ->
   describe "filtering products", ->
     describe "clearing filters", ->
       it "resets filter variables", ->
-        $scope.query = "lala"
-        $scope.producerFilter = "5"
-        $scope.categoryFilter = "6"
+        $scope.q.query = "lala"
+        $scope.q.producerFilter = "5"
+        $scope.q.categoryFilter = "6"
         $scope.resetSelectFilters()
-        expect($scope.query).toBe ""
-        expect($scope.producerFilter).toBeUndefined
-        expect($scope.categoryFilter).toBeUndefined
+        expect($scope.q.query).toBe ""
+        expect($scope.q.producerFilter).toBeUndefined
+        expect($scope.q.categoryFilter).toBeUndefined
 
 
 describe "converting arrays of objects with ids to an object with ids as keys", ->
diff --git a/spec/javascripts/unit/admin/services/product_filters_spec.js.coffee b/spec/javascripts/unit/admin/services/product_filters_spec.js.coffee
new file mode 100644
index 0000000000..3ffd895f45
--- /dev/null
+++ b/spec/javascripts/unit/admin/services/product_filters_spec.js.coffee
@@ -0,0 +1,68 @@
+describe "ProductFiltersService service", ->
+  ProductFiltersService = null
+
+  beforeEach ->
+    module "ofn.admin"
+
+  beforeEach inject (_ProductFiltersService_) ->
+    ProductFiltersService = _ProductFiltersService_
+
+  describe "loadFromUrl", ->
+    it "should return a hash with value populated for filters existing in parameter", ->
+      producerFilter = 2
+      query = 'fruit'
+
+      filters = ProductFiltersService.loadFromUrl(producerFilter: producerFilter, query: query)
+
+      expect(filters.producerFilter).toBe producerFilter
+      expect(filters.query).toBe query
+
+    it "should return a hash with empty value for filters missing from parameter", ->
+      filters = ProductFiltersService.loadFromUrl({})
+
+      expect(filters.producerFilter).toBe ""
+      expect(filters.query).toBe ""
+      expect(filters.categoryFilter).toBe ""
+      expect(filters.sorting).toBe ""
+      expect(filters.importDateFilter).toBe ""
+
+  describe "generate", ->
+    it 'should filter given hash with productFilters', ->
+      producerFilter = 2
+      query = 'fruit'
+
+      filters = ProductFiltersService.generate(
+        producerFilter: producerFilter, query: query, otherParam: 'otherParam'
+      )
+
+      expect(filters.producerFilter).toBe producerFilter
+      expect(filters.query).toBe query
+      expect(filters.otherParam).toBe undefined
+
+  describe "buildUrl", ->
+    it 'should return a url adding filters to the baseUrl', inject ($httpParamSerializer) ->
+      query = 'lala'
+      producerFilter = 2
+      categoryFilter = 5
+      sorting = 'name desc'
+      importDateFilter = '2020-06-08'
+      filters = {
+        producerFilter: producerFilter
+        categoryFilter: categoryFilter
+        query: query
+        sorting: sorting
+        importDateFilter: importDateFilter
+      }
+      baseUrl = "openfoodnetwork.org.au"
+
+      url = ProductFiltersService.buildUrl(baseUrl, filters)
+
+      expectedFilters = $httpParamSerializer(filters)
+      expect(url).toBe("#{baseUrl}?#{expectedFilters}")
+
+    it 'should return baseUrl if filters are empty', ->
+      baseUrl = "openfoodnetwork.org.au"
+
+      url = ProductFiltersService.buildUrl(baseUrl, {})
+      expect(url).toBe baseUrl
+

From c3279941f53cb4e1cd30c219f28301cd96b82ed2 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Sat, 15 Aug 2020 12:05:38 +1000
Subject: [PATCH 13/17] Remove product filter helper spec as it's not needed
 anymore

---
 .../spree/admin/product_filter_helper_spec.rb  | 18 ------------------
 1 file changed, 18 deletions(-)
 delete mode 100644 spec/helpers/spree/admin/product_filter_helper_spec.rb

diff --git a/spec/helpers/spree/admin/product_filter_helper_spec.rb b/spec/helpers/spree/admin/product_filter_helper_spec.rb
deleted file mode 100644
index d9177b9da0..0000000000
--- a/spec/helpers/spree/admin/product_filter_helper_spec.rb
+++ /dev/null
@@ -1,18 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe Spree::Admin::ProductFilterHelper, type: :helper do
-  describe 'product_filters' do
-    it 'should return only parameters included in PRODUCT_FILTER' do
-      parameters = { 'query' => 'test', 'categoryFilter' => 2, 'randomFilter' => 5 }
-
-      filters = helper.product_filters(parameters)
-
-      puts filters.inspect
-      expect(filters.key?('query')).to be true
-      expect(filters.key?('categoryFilter')).to be true
-      expect(filters.key?('randomFilter')).to be false
-    end
-  end
-end

From 44487af2c8a7f804f313d1221d2474974be7ed48 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 21 Aug 2020 11:39:51 +1000
Subject: [PATCH 14/17] remove dead filter code

---
 .../javascripts/admin/bulk_product_update.js.coffee       | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index 5c011de5bb..ea455f5bc5 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -13,7 +13,6 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
     {id: 100, name: t('js.admin.orders.index.per_page', results: 100)}
   ]
 
-  productFilters = ['producerFilter', 'categoryFilter', 'query', 'sorting', 'importDateFilter']
   $scope.q = {
     producerFilter: ""
     categoryFilter: ""
@@ -43,13 +42,6 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
     $scope.page = newPage
     $scope.fetchProducts()
 
-  generateFilter = ->
-    filters = {}
-    for filter in productFilters
-      filters[filter] = $scope[filter] if $scope[filter]
-
-    filters
-
   $scope.fetchProducts = ->
     removeClearedValues()
     params = {

From f71013c5145c9b633aa409726cc9238ffd8d6009 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 21 Aug 2020 11:46:36 +1000
Subject: [PATCH 15/17] rename query filter where it was missed, fix
 bulk_update_product spec

---
 .../javascripts/admin/bulk_product_update.js.coffee       | 2 +-
 .../spree/admin/products/index/_indicators.html.haml      | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index ea455f5bc5..be29b8d19a 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -35,7 +35,7 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
     $scope.q = ProductFiltersService.loadFromUrl($location.search())
     $scope.fetchProducts()
 
-  $scope.$watchCollection '[query, producerFilter, categoryFilter, importDateFilter, per_page]', ->
+  $scope.$watchCollection '[q.query, q.producerFilter, q.categoryFilter, q.importDateFilter, per_page]', ->
     $scope.page = 1 # Reset page when changing filters for new search
 
   $scope.changePage = (newPage) ->
diff --git a/app/views/spree/admin/products/index/_indicators.html.haml b/app/views/spree/admin/products/index/_indicators.html.haml
index 930f4b4337..9c002b1f1f 100644
--- a/app/views/spree/admin/products/index/_indicators.html.haml
+++ b/app/views/spree/admin/products/index/_indicators.html.haml
@@ -3,12 +3,12 @@
   %img.spinner{ src: image_path("spinning-circles.svg") }
   %h1= t('.title')
 
-%div.sixteen.columns.alpha{ 'ng-show' => '!RequestMonitor.loading && products.length == 0 && query.length==0' }
+%div.sixteen.columns.alpha{ 'ng-show' => '!RequestMonitor.loading && products.length == 0 && q.query.length == 0' }
   %h1#no_results= t('.no_products')
 
-%div.sixteen.columns.alpha{ 'ng-show' => '!RequestMonitor.loading && products.length == 0 && query.length!=0' }
+%div.sixteen.columns.alpha{ 'ng-show' => '!RequestMonitor.loading && products.length == 0 && q.query.length != 0' }
   %h1#no_results
     = t('.no_results')
     '
-    {{query}}
-    '
\ No newline at end of file
+    {{q.query}}
+    '

From 6564ea7b0001f2f6ee429ce20d499e30e12ef0b3 Mon Sep 17 00:00:00 2001
From: Gaetan Riou <gaetan.riou@gmail.com>
Date: Fri, 21 Aug 2020 12:13:16 +1000
Subject: [PATCH 16/17] rename ProductFiltersService to ProductFiltersUrl

---
 .../admin/bulk_product_update.js.coffee        |  8 ++++----
 .../admin/services/product_filters.js.coffee   |  4 ++--
 .../admin/bulk_product_update_spec.js.coffee   |  6 +++---
 .../services/product_filters_spec.js.coffee    | 18 +++++++++---------
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee
index be29b8d19a..e46c52b71d 100644
--- a/app/assets/javascripts/admin/bulk_product_update.js.coffee
+++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee
@@ -1,4 +1,4 @@
-angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout, $filter, $http, $window, $location, BulkProducts, DisplayProperties, DirtyProducts, VariantUnitManager, StatusMessage, producers, Taxons, Columns, tax_categories, RequestMonitor, SortOptions, ErrorsParser, ProductFiltersService) ->
+angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout, $filter, $http, $window, $location, BulkProducts, DisplayProperties, DirtyProducts, VariantUnitManager, StatusMessage, producers, Taxons, Columns, tax_categories, RequestMonitor, SortOptions, ErrorsParser, ProductFiltersUrl) ->
   $scope.StatusMessage = StatusMessage
 
   $scope.columns = Columns.columns
@@ -32,7 +32,7 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
   $scope.sortOptions = SortOptions
 
   $scope.initialise = ->
-    $scope.q = ProductFiltersService.loadFromUrl($location.search())
+    $scope.q = ProductFiltersUrl.loadFromUrl($location.search())
     $scope.fetchProducts()
 
   $scope.$watchCollection '[q.query, q.producerFilter, q.categoryFilter, q.importDateFilter, per_page]', ->
@@ -55,7 +55,7 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
     }
     RequestMonitor.load(BulkProducts.fetch(params).$promise).then ->
       # update url with the filters used
-      $location.search(ProductFiltersService.generate($scope.q))
+      $location.search(ProductFiltersUrl.generate($scope.q))
       $scope.resetProducts()
 
   removeClearedValues = ->
@@ -116,7 +116,7 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout
 
   $scope.editWarn = (product, variant) ->
     if confirm_unsaved_changes()
-      $window.location.href = ProductFiltersService.buildUrl(editProductUrl(product, variant), $scope.q)
+      $window.location.href = ProductFiltersUrl.buildUrl(editProductUrl(product, variant), $scope.q)
 
   $scope.toggleShowAllVariants = ->
     showVariants = !DisplayProperties.showVariants 0
diff --git a/app/assets/javascripts/admin/services/product_filters.js.coffee b/app/assets/javascripts/admin/services/product_filters.js.coffee
index c6d3574df0..069a077c0f 100644
--- a/app/assets/javascripts/admin/services/product_filters.js.coffee
+++ b/app/assets/javascripts/admin/services/product_filters.js.coffee
@@ -1,5 +1,5 @@
-angular.module("ofn.admin").factory "ProductFiltersService", ($httpParamSerializer) ->
-  new class ProductFiltersService
+angular.module("ofn.admin").factory "ProductFiltersUrl", ($httpParamSerializer) ->
+  new class ProductFiltersUrl
     productFilters: ['producerFilter', 'categoryFilter', 'query', 'sorting', 'importDateFilter']
 
     loadFromUrl: (filters) ->
diff --git a/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee
index b6dc92e051..f7e7fd45b4 100644
--- a/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee
+++ b/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee
@@ -246,7 +246,7 @@ describe "filtering products for submission to database", ->
     ]
 
 describe "AdminProductEditCtrl", ->
-  $ctrl = $scope = $timeout = $httpBackend = BulkProducts = DirtyProducts = DisplayProperties = ProductFiltersService = windowStub = null
+  $ctrl = $scope = $timeout = $httpBackend = BulkProducts = DirtyProducts = DisplayProperties = ProductFiltersUrl = windowStub = null
 
   beforeEach ->
     module "ofn.admin"
@@ -258,7 +258,7 @@ describe "AdminProductEditCtrl", ->
       $provide.value 'columns', []
       null
 
-  beforeEach inject((_$controller_, _$timeout_, $rootScope, _$httpBackend_, _BulkProducts_, _DirtyProducts_, _DisplayProperties_, _ProductFiltersService_) ->
+  beforeEach inject((_$controller_, _$timeout_, $rootScope, _$httpBackend_, _BulkProducts_, _DirtyProducts_, _DisplayProperties_, _ProductFiltersUrl_) ->
     $scope = $rootScope.$new()
     $ctrl = _$controller_
     $timeout = _$timeout_
@@ -266,7 +266,7 @@ describe "AdminProductEditCtrl", ->
     BulkProducts = _BulkProducts_
     DirtyProducts = _DirtyProducts_
     DisplayProperties = _DisplayProperties_
-    ProductFiltersService = _ProductFiltersService_
+    ProductFiltersUrl = _ProductFiltersUrl_
 
     # Stub the window object so we don't get redirected when href is updated
     windowStub = {navigator: {userAgent: 'foo'}, location: {href: ''}}
diff --git a/spec/javascripts/unit/admin/services/product_filters_spec.js.coffee b/spec/javascripts/unit/admin/services/product_filters_spec.js.coffee
index 3ffd895f45..e40b76e496 100644
--- a/spec/javascripts/unit/admin/services/product_filters_spec.js.coffee
+++ b/spec/javascripts/unit/admin/services/product_filters_spec.js.coffee
@@ -1,24 +1,24 @@
-describe "ProductFiltersService service", ->
-  ProductFiltersService = null
+describe "ProductFiltersUrl service", ->
+  ProductFiltersUrl = null
 
   beforeEach ->
     module "ofn.admin"
 
-  beforeEach inject (_ProductFiltersService_) ->
-    ProductFiltersService = _ProductFiltersService_
+  beforeEach inject (_ProductFiltersUrl_) ->
+    ProductFiltersUrl = _ProductFiltersUrl_
 
   describe "loadFromUrl", ->
     it "should return a hash with value populated for filters existing in parameter", ->
       producerFilter = 2
       query = 'fruit'
 
-      filters = ProductFiltersService.loadFromUrl(producerFilter: producerFilter, query: query)
+      filters = ProductFiltersUrl.loadFromUrl(producerFilter: producerFilter, query: query)
 
       expect(filters.producerFilter).toBe producerFilter
       expect(filters.query).toBe query
 
     it "should return a hash with empty value for filters missing from parameter", ->
-      filters = ProductFiltersService.loadFromUrl({})
+      filters = ProductFiltersUrl.loadFromUrl({})
 
       expect(filters.producerFilter).toBe ""
       expect(filters.query).toBe ""
@@ -31,7 +31,7 @@ describe "ProductFiltersService service", ->
       producerFilter = 2
       query = 'fruit'
 
-      filters = ProductFiltersService.generate(
+      filters = ProductFiltersUrl.generate(
         producerFilter: producerFilter, query: query, otherParam: 'otherParam'
       )
 
@@ -55,7 +55,7 @@ describe "ProductFiltersService service", ->
       }
       baseUrl = "openfoodnetwork.org.au"
 
-      url = ProductFiltersService.buildUrl(baseUrl, filters)
+      url = ProductFiltersUrl.buildUrl(baseUrl, filters)
 
       expectedFilters = $httpParamSerializer(filters)
       expect(url).toBe("#{baseUrl}?#{expectedFilters}")
@@ -63,6 +63,6 @@ describe "ProductFiltersService service", ->
     it 'should return baseUrl if filters are empty', ->
       baseUrl = "openfoodnetwork.org.au"
 
-      url = ProductFiltersService.buildUrl(baseUrl, {})
+      url = ProductFiltersUrl.buildUrl(baseUrl, {})
       expect(url).toBe baseUrl
 

From 6f59158153373f7428cbaf627541cf3d882c4041 Mon Sep 17 00:00:00 2001
From: Luis Ramos <luisramos0@gmail.com>
Date: Thu, 10 Sep 2020 17:36:25 +0100
Subject: [PATCH 17/17] Remove useless TODOs

---
 app/models/enterprise.rb                       | 5 +----
 app/models/spree/order_decorator.rb            | 4 ----
 app/views/checkout/_payment.html.haml          | 2 --
 app/views/home/_tagline.html.haml              | 1 -
 app/views/spree/admin/variants/index.html.haml | 1 -
 5 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb
index 0e60be9a14..ec832e8b43 100644
--- a/app/models/enterprise.rb
+++ b/app/models/enterprise.rb
@@ -7,10 +7,7 @@ class Enterprise < ActiveRecord::Base
   preference :shopfront_taxon_order, :string, default: ""
   preference :shopfront_order_cycle_order, :string, default: "orders_close_at"
 
-  # This is hopefully a temporary measure, pending the arrival of multiple named inventories
-  # for shops. We need this here to allow hubs to restrict visible variants to only those in
-  # their inventory if they so choose
-  # TODO: delegate this to a separate model instead of abusing Preferences.
+  # Allow hubs to restrict visible variants to only those in their inventory
   preference :product_selection_from_inventory_only, :boolean, default: false
 
   has_paper_trail only: [:owner_id, :sells], on: [:update]
diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb
index bf125da21d..2b033065cc 100644
--- a/app/models/spree/order_decorator.rb
+++ b/app/models/spree/order_decorator.rb
@@ -194,8 +194,6 @@ Spree::Order.class_eval do
   end
 
   # After changing line items of a completed order
-  # TODO: perhaps this should be triggered from a controller
-  # rather than an after_save callback?
   def update_shipping_fees!
     shipments.each do |shipment|
       next if shipment.shipped?
@@ -222,8 +220,6 @@ Spree::Order.class_eval do
   end
 
   # After changing line items of a completed order
-  # TODO: perhaps this should be triggered from a controller
-  # rather than an after_save callback?
   def update_payment_fees!
     payments.each do |payment|
       next if payment.completed?
diff --git a/app/views/checkout/_payment.html.haml b/app/views/checkout/_payment.html.haml
index 928578158f..cd4b7c5009 100644
--- a/app/views/checkout/_payment.html.haml
+++ b/app/views/checkout/_payment.html.haml
@@ -13,8 +13,6 @@
       "ng-class" => "{valid: payment.$valid, open: accordion.payment}"}
       = render 'checkout/accordion_heading'
 
-      -# TODO render this in Angular instead of server-side
-      -# The problem being how to render the partials
       .row
         .small-12.medium-12.large-6.columns
           - available_payment_methods.each do |method|
diff --git a/app/views/home/_tagline.html.haml b/app/views/home/_tagline.html.haml
index 5afa5fe21d..b20a3dc799 100644
--- a/app/views/home/_tagline.html.haml
+++ b/app/views/home/_tagline.html.haml
@@ -2,7 +2,6 @@
   .row
     .small-12.text-center.columns
       %h1
-        / TODO: Rohan - logo asset & width is content manageable:
         %img{src: image_path("logo-white-notext.png"), title: Spree::Config.site_name}
         %br/
         %a.button.transparent{href: "/shops"}
diff --git a/app/views/spree/admin/variants/index.html.haml b/app/views/spree/admin/variants/index.html.haml
index fa2d206749..9be92b7d5c 100644
--- a/app/views/spree/admin/variants/index.html.haml
+++ b/app/views/spree/admin/variants/index.html.haml
@@ -30,7 +30,6 @@
           = link_to_with_icon('icon-edit', Spree.t(:edit), edit_object_url(variant, @url_filters), no_text: true) unless variant.deleted?
           = link_to_delete(variant, { url: object_url(variant, @url_filters), no_text: true }) unless variant.deleted?
 
-// TODO this
 - if @product.empty_option_values?
   %p.first_add_option_types.no-objects-found
     = t('.to_add_variants_you_must_first_define')