From bc826f73a1f063c6f24edc3b99975b48edaef1f0 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 2 Oct 2019 01:44:00 +0100 Subject: [PATCH 01/25] Add temporary placeholder for API endpoint --- app/controllers/api/order_cycles_controller.rb | 14 ++++++++++++++ config/routes/api.rb | 2 ++ 2 files changed, 16 insertions(+) create mode 100644 app/controllers/api/order_cycles_controller.rb diff --git a/app/controllers/api/order_cycles_controller.rb b/app/controllers/api/order_cycles_controller.rb new file mode 100644 index 00000000000..43e17f57457 --- /dev/null +++ b/app/controllers/api/order_cycles_controller.rb @@ -0,0 +1,14 @@ +module Api + class OrderCyclesController < BaseController + respond_to :json + + def products + products = OpenFoodNetwork::ProductsRenderer.new(current_distributor, current_order_cycle, params).products_json + # products = ::ProductsFilterer.new(current_distributor, current_customer, products_json).call # TBD + + render json: products + rescue OpenFoodNetwork::ProductsRenderer::NoProducts + render status: :not_found, json: '' + end + end +end diff --git a/config/routes/api.rb b/config/routes/api.rb index 94b9e180f0b..c46a4abd768 100644 --- a/config/routes/api.rb +++ b/config/routes/api.rb @@ -44,6 +44,8 @@ resources :order_cycles do get :managed, on: :collection get :accessible, on: :collection + + get :products, on: :member end resource :status do From a1a5c3b7fe0fb7a3eb7a83326a2f652629dc8d58 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 2 Oct 2019 01:45:04 +0100 Subject: [PATCH 02/25] Add new Angular OrderCycleResource --- .../darkswarm/services/order_cycle_resource.js.coffee | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 app/assets/javascripts/darkswarm/services/order_cycle_resource.js.coffee diff --git a/app/assets/javascripts/darkswarm/services/order_cycle_resource.js.coffee b/app/assets/javascripts/darkswarm/services/order_cycle_resource.js.coffee new file mode 100644 index 00000000000..a92a7288feb --- /dev/null +++ b/app/assets/javascripts/darkswarm/services/order_cycle_resource.js.coffee @@ -0,0 +1,9 @@ +Darkswarm.factory 'OrderCycleResource', ($resource) -> + $resource('/api/order_cycles/:id', {}, { + 'products': + method: 'GET' + isArray: true + url: '/api/order_cycles/:id/products' + params: + id: '@id' + }) From 01d1e8243c0f6c0d26427a0991836918ba489149 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 2 Oct 2019 01:47:48 +0100 Subject: [PATCH 03/25] Add pagination to ProductsRenderer --- lib/open_food_network/products_renderer.rb | 24 +++++++++++++--------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb index 527385b6e83..32c4b90adb4 100644 --- a/lib/open_food_network/products_renderer.rb +++ b/lib/open_food_network/products_renderer.rb @@ -4,19 +4,20 @@ module OpenFoodNetwork class ProductsRenderer class NoProducts < RuntimeError; end - def initialize(distributor, order_cycle) + def initialize(distributor, order_cycle, params = {}) @distributor = distributor @order_cycle = order_cycle + @params = params end def products_json if products - enterprise_fee_calculator = EnterpriseFeeCalculator.new @distributor, @order_cycle + enterprise_fee_calculator = EnterpriseFeeCalculator.new distributor, order_cycle ActiveModel::ArraySerializer.new(products, each_serializer: Api::ProductSerializer, - current_order_cycle: @order_cycle, - current_distributor: @distributor, + current_order_cycle: order_cycle, + current_distributor: distributor, variants: variants_for_shop_by_id, master_variants: master_variants_for_shop_by_id, enterprise_fee_calculator: enterprise_fee_calculator,).to_json @@ -27,25 +28,28 @@ def products_json private + attr_reader :order_cycle, :distributor, :params + def products - return unless @order_cycle + return unless order_cycle @products ||= begin - scoper = ScopeProductToHub.new(@distributor) + scoper = ScopeProductToHub.new(distributor) distributed_products.products_relation. order(taxon_order). + ransack(params[:q]).result.page(params[:page] || 1).per(params[:per_page] || 10). each { |product| scoper.scope(product) } end end def distributed_products - OrderCycleDistributedProducts.new(@distributor, @order_cycle) + OrderCycleDistributedProducts.new(distributor, order_cycle) end def taxon_order - if @distributor.preferred_shopfront_taxon_order.present? - @distributor + if distributor.preferred_shopfront_taxon_order.present? + distributor .preferred_shopfront_taxon_order .split(",").map { |id| "primary_taxon_id=#{id} DESC" } .join(",") + ", name ASC" @@ -56,7 +60,7 @@ def taxon_order def variants_for_shop @variants_for_shop ||= begin - scoper = OpenFoodNetwork::ScopeVariantToHub.new(@distributor) + scoper = OpenFoodNetwork::ScopeVariantToHub.new(distributor) distributed_products.variants_relation. includes(:default_price, :stock_locations, :product). From fe0de988210304cc32efed261934ebcd206bfe9f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 2 Oct 2019 01:52:36 +0100 Subject: [PATCH 04/25] Add pagination in Angular and views --- .../controllers/products_controller.js.coffee | 82 ++++++++++++------- .../darkswarm/services/products.js.coffee | 29 ++++--- app/views/shop/products/_filters.html.haml | 4 +- app/views/shop/products/_form.html.haml | 8 +- .../products_controller_spec.js.coffee | 7 -- .../services/products_spec.js.coffee | 25 +++--- 6 files changed, 90 insertions(+), 65 deletions(-) diff --git a/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee b/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee index 3c32c83875f..0d4567afda0 100644 --- a/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee +++ b/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee @@ -1,41 +1,61 @@ -Darkswarm.controller "ProductsCtrl", ($scope, $filter, $rootScope, Products, OrderCycle, FilterSelectorsService, Cart, Taxons, Properties) -> +Darkswarm.controller "ProductsCtrl", ($scope, $filter, $rootScope, Products, OrderCycle, FilterSelectorsService, Cart, Dereferencer, Taxons, Properties, currentHub, $timeout) -> $scope.Products = Products $scope.Cart = Cart $scope.query = "" $scope.taxonSelectors = FilterSelectorsService.createSelectors() $scope.propertySelectors = FilterSelectorsService.createSelectors() $scope.filtersActive = true - $scope.limit = 10 + $scope.page = 1 + $scope.per_page = 10 $scope.order_cycle = OrderCycle.order_cycle - # $scope.infiniteDisabled = true - - # All of this logic basically just replicates the functionality filtering an ng-repeat - # except that it allows us to filter a separate list before rendering, meaning that - # we can get much better performance when applying filters by resetting the limit on the - # number of products being rendered each time a filter is changed. - - $scope.$watch "Products.loading", (newValue, oldValue) -> - $scope.updateFilteredProducts() - $scope.$broadcast("loadFilterSelectors") if !newValue - - $scope.incrementLimit = -> - if $scope.limit < Products.products.length - $scope.limit += 10 - $scope.updateVisibleProducts() - - $scope.$watch 'query', -> $scope.updateFilteredProducts() - $scope.$watchCollection 'activeTaxons', -> $scope.updateFilteredProducts() - $scope.$watchCollection 'activeProperties', -> $scope.updateFilteredProducts() - - $scope.updateFilteredProducts = -> - $scope.limit = 10 - f1 = $filter('products')(Products.products, $scope.query) - f2 = $filter('taxons')(f1, $scope.activeTaxons) - $scope.filteredProducts = $filter('properties')(f2, $scope.activeProperties) - $scope.updateVisibleProducts() - - $scope.updateVisibleProducts = -> - $scope.visibleProducts = $filter('limitTo')($scope.filteredProducts, $scope.limit) + + $scope.supplied_taxons = -> + return $scope.memoized_taxons if $scope.memoized_taxons != undefined + $scope.memoized_taxons = {} + currentHub.supplied_taxons.map( (taxon) -> + $scope.memoized_taxons[taxon.id] = Taxons.taxons_by_id[taxon.id] + ) + $scope.memoized_taxons + + $scope.supplied_properties = -> + return $scope.memoized_properties if $scope.memoized_properties != undefined + $scope.memoized_properties = {} + currentHub.supplied_properties.map( (property) -> + $scope.memoized_properties[property.id] = Properties.properties_by_id[property.id] + ) + $scope.memoized_properties + + $scope.loadMore = -> + if ($scope.page * $scope.per_page) <= Products.products.length + $scope.loadMoreProducts() + + $scope.$watch 'query', (newValue, oldValue) -> $scope.loadProducts() if newValue != oldValue + $scope.$watchCollection 'activeTaxons', (newValue, oldValue) -> $scope.loadProducts() if newValue != oldValue + $scope.$watchCollection 'activeProperties', (newValue, oldValue) -> $scope.loadProducts() if newValue != oldValue + + $scope.loadProducts = -> + $scope.page = 1 + params = { + id: $scope.order_cycle.order_cycle_id, + page: $scope.page, + per_page: $scope.per_page, + 'q[name_or_supplier_name_cont]': $scope.query, + 'q[properites_in_any][]': $scope.activeProperties, + 'q[primary_taxon_id_in_any][]': $scope.activeTaxons + } + Products.update(params) + + $scope.loadMoreProducts = -> + params = { + id: $scope.order_cycle.order_cycle_id, + page: $scope.page + 1, + per_page: $scope.per_page, + 'q[name_or_supplier_name_cont]': $scope.query, + 'q[properites_in_any][]': $scope.activeProperties, + 'q[primary_taxon_id_in_any][]': $scope.activeTaxons + } + Products.update(params, true) + $scope.page += 1 $scope.searchKeypress = (e)-> code = e.keyCode || e.which diff --git a/app/assets/javascripts/darkswarm/services/products.js.coffee b/app/assets/javascripts/darkswarm/services/products.js.coffee index 0a24cbd3743..9b7b5e59930 100644 --- a/app/assets/javascripts/darkswarm/services/products.js.coffee +++ b/app/assets/javascripts/darkswarm/services/products.js.coffee @@ -1,26 +1,33 @@ -Darkswarm.factory 'Products', ($resource, Shopfront, Dereferencer, Taxons, Properties, Cart, Variants) -> +Darkswarm.factory 'Products', (OrderCycleResource, OrderCycle, Shopfront, Dereferencer, Taxons, Properties, Cart, Variants) -> new class Products constructor: -> @update() - # TODO: don't need to scope this into object - # Already on object as far as controller scope is concerned - products: null + products: [] + fetched_products: [] loading: true - update: => + update: (params = {}, load_more = false) => @loading = true - @products = [] - $resource("/shop/products").query (products)=> - @products = products + order_cycle_id = OrderCycle.order_cycle.order_cycle_id + if order_cycle_id == undefined + @loading = false + return + + params['id'] = OrderCycle.order_cycle.order_cycle_id if params['id'] == undefined + + OrderCycleResource.products params, (data)=> + @products = [] unless load_more + @fetched_products = data @extend() @dereference() @registerVariants() + @products = @products.concat(@fetched_products) @loading = false extend: -> - for product in @products + for product in @fetched_products if product.variants?.length > 0 prices = (v.price for v in product.variants) product.price = Math.min.apply(null, prices) @@ -30,7 +37,7 @@ Darkswarm.factory 'Products', ($resource, Shopfront, Dereferencer, Taxons, Prope product.largeImage = product.images[0]?.large_url if product.images dereference: -> - for product in @products + for product in @fetched_products product.supplier = Shopfront.producers_by_id[product.supplier.id] Dereferencer.dereference product.taxons, Taxons.taxons_by_id @@ -40,7 +47,7 @@ Darkswarm.factory 'Products', ($resource, Shopfront, Dereferencer, Taxons, Prope # May return different objects! If the variant has already been registered # by another service, we fetch those registerVariants: -> - for product in @products + for product in @fetched_products if product.variants product.variant_names = "" product.variants = for variant in product.variants diff --git a/app/views/shop/products/_filters.html.haml b/app/views/shop/products/_filters.html.haml index 10c7c20debe..0f89c371665 100644 --- a/app/views/shop/products/_filters.html.haml +++ b/app/views/shop/products/_filters.html.haml @@ -1,5 +1,5 @@ .filter-shopfront.taxon-selectors.text-right - %single-line-selectors{ selectors: "taxonSelectors", objects: "Products.products | products:query | properties:activeProperties | taxonsOf", "active-selectors" => "activeTaxons"} + %single-line-selectors{ selectors: "taxonSelectors", objects: "supplied_taxons()", "active-selectors" => "activeTaxons"} .filter-shopfront.property-selectors.text-right - %single-line-selectors{ selectors: "propertySelectors", objects: "Products.products | products:query | taxons:activeTaxons | propertiesOf", "active-selectors" => "activeProperties"} + %single-line-selectors{ selectors: "propertySelectors", objects: "supplied_properties()", "active-selectors" => "activeProperties"} diff --git a/app/views/shop/products/_form.html.haml b/app/views/shop/products/_form.html.haml index 3a9738a532e..ed2b555f3e3 100644 --- a/app/views/shop/products/_form.html.haml +++ b/app/views/shop/products/_form.html.haml @@ -22,14 +22,14 @@ .small-12.medium-6.large-5.columns %input#search.text{"ng-model" => "query", placeholder: t(:products_search), - "ng-debounce" => "100", + "ng-debounce" => "200", "ofn-disable-enter" => true} .small-12.medium-6.large-6.large-offset-1.columns = render partial: "shop/products/filters" - %div.pad-top{ "infinite-scroll" => "incrementLimit()", "infinite-scroll-distance" => "1", "infinite-scroll-disabled" => 'filteredProducts.length <= limit' } - %product.animate-repeat{"ng-controller" => "ProductNodeCtrl", "ng-repeat" => "product in visibleProducts track by product.id", "id" => "product-{{ product.id }}"} + %div.pad-top{ "infinite-scroll" => "loadMore()", "infinite-scroll-distance" => "1", "infinite-scroll-disabled" => 'Products.loading' } + %product.animate-repeat{"ng-controller" => "ProductNodeCtrl", "ng-repeat" => "product in Products.products track by product.id", "id" => "product-{{ product.id }}"} = render "shop/products/summary" %shop-variant{variant: 'variant', "ng-repeat" => "variant in product.variants | orderBy: ['name_to_display','unit_value'] track by variant.id", "id" => "variant-{{ variant.id }}", "ng-class" => "{'out-of-stock': !variant.on_demand && variant.on_hand == 0}"} @@ -41,7 +41,7 @@ .small-12.columns.text-center %img.spinner{ src: "/assets/spinning-circles.svg" } - %div{"ng-show" => "filteredProducts.length == 0 && !Products.loading"} + %div{"ng-show" => "Products.products.length == 0 && !Products.loading"} .row.summary .small-12.columns %p.no-results diff --git a/spec/javascripts/unit/darkswarm/controllers/products_controller_spec.js.coffee b/spec/javascripts/unit/darkswarm/controllers/products_controller_spec.js.coffee index 1e188f4f161..3a7dd8ba1bd 100644 --- a/spec/javascripts/unit/darkswarm/controllers/products_controller_spec.js.coffee +++ b/spec/javascripts/unit/darkswarm/controllers/products_controller_spec.js.coffee @@ -27,13 +27,6 @@ describe 'ProductsCtrl', -> it 'fetches products from Products', -> expect(scope.Products.products).toEqual ['testy mctest'] - it "increments the limit up to the number of products", -> - scope.limit = 0 - scope.incrementLimit() - expect(scope.limit).toEqual 10 - scope.incrementLimit() - expect(scope.limit).toEqual 10 - it "blocks keypresses on code 13", -> event = keyCode: 13 diff --git a/spec/javascripts/unit/darkswarm/services/products_spec.js.coffee b/spec/javascripts/unit/darkswarm/services/products_spec.js.coffee index 0075750fba4..e9d2111d83d 100644 --- a/spec/javascripts/unit/darkswarm/services/products_spec.js.coffee +++ b/spec/javascripts/unit/darkswarm/services/products_spec.js.coffee @@ -1,6 +1,7 @@ describe 'Products service', -> $httpBackend = null Products = null + OrderCycle = {} Shopfront = null Variants = null Cart = null @@ -38,6 +39,9 @@ describe 'Products service', -> producers: id: 9, name: "Test" + OrderCycle = + order_cycle: + order_cycle_id: 1 module 'Darkswarm' module ($provide)-> @@ -46,6 +50,7 @@ describe 'Products service', -> $provide.value "taxons", taxons $provide.value "properties", properties $provide.value "Geo", Geo + $provide.value "OrderCycle", OrderCycle null inject ($injector, _$httpBackend_)-> @@ -57,60 +62,60 @@ describe 'Products service', -> $httpBackend = _$httpBackend_ it "Fetches products from the backend on init", -> - $httpBackend.expectGET("/shop/products").respond([product]) + $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) $httpBackend.flush() expect(Products.products[0].test).toEqual "cats" it "dereferences suppliers", -> Shopfront.producers_by_id = {id: 9, name: "test"} - $httpBackend.expectGET("/shop/products").respond([{supplier : {id: 9}, master: {}}]) + $httpBackend.expectGET("/api/order_cycles/1/products").respond([{supplier : {id: 9}, master: {}}]) $httpBackend.flush() expect(Products.products[0].supplier).toBe Shopfront.producers_by_id["9"] it "dereferences taxons", -> product.taxons = [2] - $httpBackend.expectGET("/shop/products").respond([product]) + $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) $httpBackend.flush() expect(Products.products[0].taxons[1]).toBe taxons[0] it "dereferences properties", -> product.properties_with_values = [1] - $httpBackend.expectGET("/shop/products").respond([product]) + $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) $httpBackend.flush() expect(Products.products[0].properties[1]).toBe properties[0] it "registers variants with Variants service", -> product.variants = [{id: 1}] - $httpBackend.expectGET("/shop/products").respond([product]) + $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) $httpBackend.flush() expect(Products.products[0].variants[0]).toBe Variants.variants[1] it "stores variant names", -> product.variants = [{id: 1, name_to_display: "one"}, {id: 2, name_to_display: "two"}] - $httpBackend.expectGET("/shop/products").respond([product]) + $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) $httpBackend.flush() expect(Products.products[0].variant_names).toEqual "one two " it "sets primaryImageOrMissing when no images are provided", -> - $httpBackend.expectGET("/shop/products").respond([product]) + $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) $httpBackend.flush() expect(Products.products[0].primaryImage).toBeUndefined() expect(Products.products[0].primaryImageOrMissing).toEqual "/assets/noimage/small.png" it "sets largeImage", -> - $httpBackend.expectGET("/shop/products").respond([productWithImage]) + $httpBackend.expectGET("/api/order_cycles/1/products").respond([productWithImage]) $httpBackend.flush() expect(Products.products[0].largeImage).toEqual("foo.png") describe "determining the price to display for a product", -> it "displays the product price when the product does not have variants", -> - $httpBackend.expectGET("/shop/products").respond([product]) + $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) $httpBackend.flush() expect(Products.products[0].price).toEqual 11.00 it "displays the minimum variant price when the product has variants", -> product.variants = [{price: 22}, {price: 33}] - $httpBackend.expectGET("/shop/products").respond([product]) + $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) $httpBackend.flush() expect(Products.products[0].price).toEqual 22 From 06e1f56ae9c4cfefe5f4f37f9cd272850f28651b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 2 Oct 2019 11:06:05 +0100 Subject: [PATCH 05/25] Extract filter list fetching into a separate endpoint --- .../order_cycle_controller.js.coffee | 1 + .../controllers/products_controller.js.coffee | 28 +++++++++---- .../services/order_cycle_resource.js.coffee | 12 ++++++ .../api/order_cycles_controller.rb | 41 +++++++++++++++++++ config/routes/api.rb | 2 + 5 files changed, 77 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/darkswarm/controllers/order_cycle_controller.js.coffee b/app/assets/javascripts/darkswarm/controllers/order_cycle_controller.js.coffee index d900fd0a274..55e44d037ef 100644 --- a/app/assets/javascripts/darkswarm/controllers/order_cycle_controller.js.coffee +++ b/app/assets/javascripts/darkswarm/controllers/order_cycle_controller.js.coffee @@ -32,3 +32,4 @@ Darkswarm.controller "OrderCycleChangeCtrl", ($scope, $timeout, OrderCycle, Prod Products.update() Cart.reloadFinalisedLineItems() ChangeableOrdersAlert.reload() + # Reload Filters from new endpoint after changing OC here diff --git a/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee b/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee index 0d4567afda0..36620b827e4 100644 --- a/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee +++ b/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee @@ -1,4 +1,4 @@ -Darkswarm.controller "ProductsCtrl", ($scope, $filter, $rootScope, Products, OrderCycle, FilterSelectorsService, Cart, Dereferencer, Taxons, Properties, currentHub, $timeout) -> +Darkswarm.controller "ProductsCtrl", ($scope, $filter, $rootScope, Products, OrderCycle, OrderCycleResource, FilterSelectorsService, Cart, Dereferencer, Taxons, Properties, currentHub, $timeout) -> $scope.Products = Products $scope.Cart = Cart $scope.query = "" @@ -12,17 +12,31 @@ Darkswarm.controller "ProductsCtrl", ($scope, $filter, $rootScope, Products, Ord $scope.supplied_taxons = -> return $scope.memoized_taxons if $scope.memoized_taxons != undefined $scope.memoized_taxons = {} - currentHub.supplied_taxons.map( (taxon) -> - $scope.memoized_taxons[taxon.id] = Taxons.taxons_by_id[taxon.id] - ) + + params = { + id: OrderCycle.order_cycle.order_cycle_id, + distributor: currentHub.id + } + OrderCycleResource.taxons params, (data)=> + data.map( (taxon) -> + $scope.memoized_taxons[taxon.id] = Taxons.taxons_by_id[taxon.id] + ) + $scope.memoized_taxons $scope.supplied_properties = -> return $scope.memoized_properties if $scope.memoized_properties != undefined $scope.memoized_properties = {} - currentHub.supplied_properties.map( (property) -> - $scope.memoized_properties[property.id] = Properties.properties_by_id[property.id] - ) + + params = { + id: OrderCycle.order_cycle.order_cycle_id, + distributor: currentHub.id + } + OrderCycleResource.properties params, (data)=> + data.map( (property) -> + $scope.memoized_properties[property.id] = Properties.properties_by_id[property.id] + ) + $scope.memoized_properties $scope.loadMore = -> diff --git a/app/assets/javascripts/darkswarm/services/order_cycle_resource.js.coffee b/app/assets/javascripts/darkswarm/services/order_cycle_resource.js.coffee index a92a7288feb..32d70d54a53 100644 --- a/app/assets/javascripts/darkswarm/services/order_cycle_resource.js.coffee +++ b/app/assets/javascripts/darkswarm/services/order_cycle_resource.js.coffee @@ -6,4 +6,16 @@ Darkswarm.factory 'OrderCycleResource', ($resource) -> url: '/api/order_cycles/:id/products' params: id: '@id' + 'taxons': + method: 'GET' + isArray: true + url: '/api/order_cycles/:id/taxons' + params: + id: '@id' + 'properties': + method: 'GET' + isArray: true + url: '/api/order_cycles/:id/properties' + params: + id: '@id' }) diff --git a/app/controllers/api/order_cycles_controller.rb b/app/controllers/api/order_cycles_controller.rb index 43e17f57457..34280e67711 100644 --- a/app/controllers/api/order_cycles_controller.rb +++ b/app/controllers/api/order_cycles_controller.rb @@ -1,7 +1,10 @@ module Api class OrderCyclesController < BaseController + include EnterprisesHelper respond_to :json + skip_authorization_check + def products products = OpenFoodNetwork::ProductsRenderer.new(current_distributor, current_order_cycle, params).products_json # products = ::ProductsFilterer.new(current_distributor, current_customer, products_json).call # TBD @@ -10,5 +13,43 @@ def products rescue OpenFoodNetwork::ProductsRenderer::NoProducts render status: :not_found, json: '' end + + def taxons + taxons = Spree::Taxon. + joins(:products). + where(spree_products: { id: distributed_products(distributor, order_cycle, customer) }). + select('DISTINCT spree_taxons.*') + + render json: ActiveModel::ArraySerializer.new(taxons, each_serializer: Api::TaxonSerializer) + end + + def properties + properties = Spree::Property. + joins(:products). + where(spree_products: { id: distributed_products(distributor, order_cycle, customer) }). + select('DISTINCT spree_properties.*') + + render json: ActiveModel::ArraySerializer.new( + properties, each_serializer: Api::PropertySerializer + ) + end + + private + + def distributor + Enterprise.find_by_id(params[:distributor]) + end + + def order_cycle + OrderCycle.find_by_id(params[:id]) + end + + def customer + @current_api_user.andand.customer_of(distributor) || nil + end + + def distributed_products(distributor, order_cycle, customer) + OrderCycleDistributedProducts.new(distributor, order_cycle, customer).products_relation + end end end diff --git a/config/routes/api.rb b/config/routes/api.rb index c46a4abd768..2b371e68329 100644 --- a/config/routes/api.rb +++ b/config/routes/api.rb @@ -46,6 +46,8 @@ get :accessible, on: :collection get :products, on: :member + get :taxons, on: :member + get :properties, on: :member end resource :status do From e96252f2eda25cf29da6721f3ebbd5b6844fffe3 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 2 Oct 2019 13:47:22 +0100 Subject: [PATCH 06/25] Add `tag_rules` logic to main query before pagination --- .../order_cycle_distributed_products.rb | 102 ++++++++++++++++-- lib/open_food_network/products_renderer.rb | 7 +- .../products_renderer_spec.rb | 5 +- .../order_cycle_distributed_products_spec.rb | 18 ++-- 4 files changed, 112 insertions(+), 20 deletions(-) diff --git a/app/services/order_cycle_distributed_products.rb b/app/services/order_cycle_distributed_products.rb index d2c9d3c4caf..3f8d3ddc9b8 100644 --- a/app/services/order_cycle_distributed_products.rb +++ b/app/services/order_cycle_distributed_products.rb @@ -2,9 +2,10 @@ # The stock-checking includes on_demand and stock level overrides from variant_overrides. class OrderCycleDistributedProducts - def initialize(distributor, order_cycle) + def initialize(distributor, order_cycle, customer) @distributor = distributor @order_cycle = order_cycle + @customer = customer end def products_relation @@ -12,26 +13,97 @@ def products_relation end def variants_relation - @order_cycle. - variants_distributed_by(@distributor). + order_cycle. + variants_distributed_by(distributor). merge(stocked_variants_and_overrides) end private + attr_reader :distributor, :order_cycle, :customer + def stocked_products - @order_cycle. - variants_distributed_by(@distributor). + order_cycle. + variants_distributed_by(distributor). merge(stocked_variants_and_overrides). select("DISTINCT spree_variants.product_id") end def stocked_variants_and_overrides - Spree::Variant. + stocked_variants = Spree::Variant. joins("LEFT OUTER JOIN variant_overrides ON variant_overrides.variant_id = spree_variants.id - AND variant_overrides.hub_id = #{@distributor.id}"). + AND variant_overrides.hub_id = #{distributor.id}"). joins(:stock_items). where(query_stock_with_overrides) + + if distributor_rules.any? + stocked_variants = apply_tag_rules(stocked_variants) + end + + stocked_variants + end + + def apply_tag_rules(stocked_variants) + stocked_variants.where(query_with_tag_rules) + end + + def distributor_rules + @distributor_rules ||= TagRule::FilterProducts.prioritised.for(distributor) + end + + def customer_tag_list + customer.andand.tag_list || [] + end + + def default_rule_tags + default_rules.map(&:preferred_variant_tags) + end + + def hide_rule_tags + hide_rules.map(&:preferred_variant_tags) + end + + def show_rule_tags + show_rules.map(&:preferred_variant_tags) + end + + def overrides_to_hide + @overrides_to_hide = VariantOverride.where(hub_id: distributor.id). + tagged_with(default_rule_tags + hide_rule_tags, any: true). + pluck(:id) + end + + def overrides_to_show + @overrides_to_show = VariantOverride.where(hub_id: distributor.id). + tagged_with(show_rule_tags, any: true). + pluck(:id) + end + + def customer_applicable_rules + # Rules which apply specifically to the current customer + @customer_applicable_rules ||= non_default_rules.select{ |rule| customer_tagged?(rule) } + end + + def default_rules + # These rules hide a variant_override with tag X + distributor_rules.select(&:is_default?) + end + + def non_default_rules + # These rules show or hide a variant_override with tag X for customer with tag Y + distributor_rules.reject(&:is_default?) + end + + def hide_rules + @hide_rules ||= customer_applicable_rules.select{ |rule| rule.preferred_matched_variants_visibility == 'hidden'} + end + + def show_rules + customer_applicable_rules - hide_rules + end + + def customer_tagged?(rule) + customer_tag_list.include? rule.preferred_customer_tags end def query_stock_with_overrides @@ -42,6 +114,22 @@ def query_stock_with_overrides AND #{variant_not_on_demand} AND #{variant_in_stock} ) )" end + def query_with_tag_rules + "#{variant_not_overriden} OR ( #{variant_overriden} + AND ( #{override_not_hidden_by_rule} + OR #{override_shown_by_rule} ) )" + end + + def override_not_hidden_by_rule + return "FALSE" unless overrides_to_hide.any? + "variant_overrides.id NOT IN (#{overrides_to_hide.join(',')})" + end + + def override_shown_by_rule + return "FALSE" unless overrides_to_show.any? + "variant_overrides.id IN (#{overrides_to_show.join(',')})" + end + def variant_not_overriden "variant_overrides.id IS NULL" end diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb index 32c4b90adb4..bafbe27a593 100644 --- a/lib/open_food_network/products_renderer.rb +++ b/lib/open_food_network/products_renderer.rb @@ -4,9 +4,10 @@ module OpenFoodNetwork class ProductsRenderer class NoProducts < RuntimeError; end - def initialize(distributor, order_cycle, params = {}) + def initialize(distributor, order_cycle, customer, params = {}) @distributor = distributor @order_cycle = order_cycle + @customer = customer @params = params end @@ -28,7 +29,7 @@ def products_json private - attr_reader :order_cycle, :distributor, :params + attr_reader :order_cycle, :distributor, :customer, :params def products return unless order_cycle @@ -44,7 +45,7 @@ def products end def distributed_products - OrderCycleDistributedProducts.new(distributor, order_cycle) + OrderCycleDistributedProducts.new(distributor, order_cycle, customer) end def taxon_order diff --git a/spec/lib/open_food_network/products_renderer_spec.rb b/spec/lib/open_food_network/products_renderer_spec.rb index 252f26f5178..dbc6d4b5357 100644 --- a/spec/lib/open_food_network/products_renderer_spec.rb +++ b/spec/lib/open_food_network/products_renderer_spec.rb @@ -6,7 +6,8 @@ module OpenFoodNetwork let(:distributor) { create(:distributor_enterprise) } let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor]) } let(:exchange) { order_cycle.exchanges.to_enterprises(distributor).outgoing.first } - let(:pr) { ProductsRenderer.new(distributor, order_cycle) } + let(:customer) { create(:customer) } + let(:pr) { ProductsRenderer.new(distributor, order_cycle, customer) } describe "sorting" do let(:t1) { create(:taxon) } @@ -89,7 +90,7 @@ module OpenFoodNetwork let!(:v2) { create(:variant, product: p, unit_value: 5) } # Not in exchange let!(:v3) { create(:variant, product: p, unit_value: 7, inventory_items: [create(:inventory_item, enterprise: hub, visible: true)]) } let!(:v4) { create(:variant, product: p, unit_value: 9, inventory_items: [create(:inventory_item, enterprise: hub, visible: false)]) } - let(:pr) { ProductsRenderer.new(hub, oc) } + let(:pr) { ProductsRenderer.new(hub, oc, customer) } let(:variants) { pr.send(:variants_for_shop_by_id) } it "scopes variants to distribution" do diff --git a/spec/services/order_cycle_distributed_products_spec.rb b/spec/services/order_cycle_distributed_products_spec.rb index b02baf49749..5356340c5d2 100644 --- a/spec/services/order_cycle_distributed_products_spec.rb +++ b/spec/services/order_cycle_distributed_products_spec.rb @@ -5,13 +5,14 @@ let(:distributor) { create(:distributor_enterprise) } let(:product) { create(:product) } let(:variant) { product.variants.first } + let(:customer) { create(:customer) } let(:order_cycle) do create(:simple_order_cycle, distributors: [distributor], variants: [variant]) end describe "product distributed by distributor in the OC" do it "returns products" do - expect(described_class.new(distributor, order_cycle).products_relation).to eq([product]) + expect(described_class.new(distributor, order_cycle, customer).products_relation).to eq([product]) end end @@ -25,7 +26,7 @@ end it "does not return product" do - expect(described_class.new(distributor, order_cycle).products_relation).to_not include product + expect(described_class.new(distributor, order_cycle, customer).products_relation).to_not include product end end @@ -36,19 +37,19 @@ end it "does not return product" do - expect(described_class.new(distributor, order_cycle).products_relation).to_not include product + expect(described_class.new(distributor, order_cycle, customer).products_relation).to_not include product end end describe "filtering products that are out of stock" do context "with regular variants" do it "returns product when variant is in stock" do - expect(described_class.new(distributor, order_cycle).products_relation).to include product + expect(described_class.new(distributor, order_cycle, customer).products_relation).to include product end it "does not return product when variant is out of stock" do variant.update_attribute(:on_hand, 0) - expect(described_class.new(distributor, order_cycle).products_relation).to_not include product + expect(described_class.new(distributor, order_cycle, customer).products_relation).to_not include product end end @@ -56,13 +57,13 @@ let!(:override) { create(:variant_override, hub: distributor, variant: variant, count_on_hand: 0) } it "does not return product when an override is out of stock" do - expect(described_class.new(distributor, order_cycle).products_relation).to_not include product + expect(described_class.new(distributor, order_cycle, customer).products_relation).to_not include product end it "returns product when an override is in stock" do variant.update_attribute(:on_hand, 0) override.update_attribute(:count_on_hand, 10) - expect(described_class.new(distributor, order_cycle).products_relation).to include product + expect(described_class.new(distributor, order_cycle, customer).products_relation).to include product end end end @@ -71,12 +72,13 @@ describe "#variants_relation" do let(:distributor) { create(:distributor_enterprise) } let(:oc) { create(:simple_order_cycle, distributors: [distributor], variants: [v1, v3]) } + let(:customer) { create(:customer) } let(:product) { create(:simple_product) } let!(:v1) { create(:variant, product: product) } let!(:v2) { create(:variant, product: product) } let!(:v3) { create(:variant, product: product) } let!(:vo) { create(:variant_override, hub: distributor, variant_id: v3.id, count_on_hand: 0) } - let(:variants) { described_class.new(distributor, oc).variants_relation } + let(:variants) { described_class.new(distributor, oc, customer).variants_relation } it "returns variants in the oc" do expect(variants).to include v1 From f134cd9473b9bdc7be819f9b5775adf4f707ecd6 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 4 Oct 2019 10:07:12 +0100 Subject: [PATCH 07/25] Extract tag_rule filtering into separate service --- .../order_cycle_distributed_products.rb | 85 +------------- app/services/product_tag_rules_filterer.rb | 111 ++++++++++++++++++ .../product_tag_rules_filterer_spec.rb | 98 ++++++++++++++++ 3 files changed, 210 insertions(+), 84 deletions(-) create mode 100644 app/services/product_tag_rules_filterer.rb create mode 100644 spec/services/product_tag_rules_filterer_spec.rb diff --git a/app/services/order_cycle_distributed_products.rb b/app/services/order_cycle_distributed_products.rb index 3f8d3ddc9b8..306233ea3ce 100644 --- a/app/services/order_cycle_distributed_products.rb +++ b/app/services/order_cycle_distributed_products.rb @@ -36,74 +36,7 @@ def stocked_variants_and_overrides joins(:stock_items). where(query_stock_with_overrides) - if distributor_rules.any? - stocked_variants = apply_tag_rules(stocked_variants) - end - - stocked_variants - end - - def apply_tag_rules(stocked_variants) - stocked_variants.where(query_with_tag_rules) - end - - def distributor_rules - @distributor_rules ||= TagRule::FilterProducts.prioritised.for(distributor) - end - - def customer_tag_list - customer.andand.tag_list || [] - end - - def default_rule_tags - default_rules.map(&:preferred_variant_tags) - end - - def hide_rule_tags - hide_rules.map(&:preferred_variant_tags) - end - - def show_rule_tags - show_rules.map(&:preferred_variant_tags) - end - - def overrides_to_hide - @overrides_to_hide = VariantOverride.where(hub_id: distributor.id). - tagged_with(default_rule_tags + hide_rule_tags, any: true). - pluck(:id) - end - - def overrides_to_show - @overrides_to_show = VariantOverride.where(hub_id: distributor.id). - tagged_with(show_rule_tags, any: true). - pluck(:id) - end - - def customer_applicable_rules - # Rules which apply specifically to the current customer - @customer_applicable_rules ||= non_default_rules.select{ |rule| customer_tagged?(rule) } - end - - def default_rules - # These rules hide a variant_override with tag X - distributor_rules.select(&:is_default?) - end - - def non_default_rules - # These rules show or hide a variant_override with tag X for customer with tag Y - distributor_rules.reject(&:is_default?) - end - - def hide_rules - @hide_rules ||= customer_applicable_rules.select{ |rule| rule.preferred_matched_variants_visibility == 'hidden'} - end - - def show_rules - customer_applicable_rules - hide_rules - end - - def customer_tagged?(rule) - customer_tag_list.include? rule.preferred_customer_tags + ProductTagRulesFilterer.new(distributor, customer, stocked_variants).call end def query_stock_with_overrides @@ -114,22 +47,6 @@ def query_stock_with_overrides AND #{variant_not_on_demand} AND #{variant_in_stock} ) )" end - def query_with_tag_rules - "#{variant_not_overriden} OR ( #{variant_overriden} - AND ( #{override_not_hidden_by_rule} - OR #{override_shown_by_rule} ) )" - end - - def override_not_hidden_by_rule - return "FALSE" unless overrides_to_hide.any? - "variant_overrides.id NOT IN (#{overrides_to_hide.join(',')})" - end - - def override_shown_by_rule - return "FALSE" unless overrides_to_show.any? - "variant_overrides.id IN (#{overrides_to_show.join(',')})" - end - def variant_not_overriden "variant_overrides.id IS NULL" end diff --git a/app/services/product_tag_rules_filterer.rb b/app/services/product_tag_rules_filterer.rb new file mode 100644 index 00000000000..b58cbc2c434 --- /dev/null +++ b/app/services/product_tag_rules_filterer.rb @@ -0,0 +1,111 @@ +# Takes a Spree::Variant AR object and filters results based on applicable tag rules. +# Tag rules exists in the context of enterprise, customer, and variant_overrides, +# and are applied to variant_overrides only. Returns a Spree::Variant AR object. + +class ProductTagRulesFilterer + def initialize(distributor, customer, variants_relation) + @distributor = distributor + @customer = customer + @variants_relation = variants_relation + end + + def call + return variants_relation unless distributor_rules.any? + + filter(variants_relation) + end + + private + + attr_accessor :distributor, :customer, :variants_relation + + def distributor_rules + @distributor_rules ||= TagRule::FilterProducts.prioritised.for(distributor).all + end + + def filter(variants_relation) + return variants_relation unless overrides_to_hide.any? + + variants_relation.where(query_with_tag_rules) + end + + def query_with_tag_rules + "#{variant_not_overriden} OR ( #{variant_overriden} + AND ( #{override_not_hidden_by_rule} + OR #{override_shown_by_rule} ) )" + end + + def variant_not_overriden + "variant_overrides.id IS NULL" + end + + def variant_overriden + "variant_overrides.id IS NOT NULL" + end + + def override_not_hidden_by_rule + return "FALSE" unless overrides_to_hide.any? + "variant_overrides.id NOT IN (#{overrides_to_hide.join(',')})" + end + + def override_shown_by_rule + return "FALSE" unless overrides_to_show.any? + "variant_overrides.id IN (#{overrides_to_show.join(',')})" + end + + def overrides_to_hide + @overrides_to_hide ||= VariantOverride.where(hub_id: distributor.id). + tagged_with(default_rule_tags + hide_rule_tags, any: true). + pluck(:id) + end + + def overrides_to_show + @overrides_to_show ||= VariantOverride.where(hub_id: distributor.id). + tagged_with(show_rule_tags, any: true). + pluck(:id) + end + + def default_rule_tags + default_rules.map(&:preferred_variant_tags) + end + + def hide_rule_tags + hide_rules.map(&:preferred_variant_tags) + end + + def show_rule_tags + show_rules.map(&:preferred_variant_tags) + end + + def default_rules + # These rules hide a variant_override with tag X and apply to all customers + distributor_rules.select(&:is_default?) + end + + def non_default_rules + # These rules show or hide a variant_override with tag X for customer with tag Y + distributor_rules.reject(&:is_default?) + end + + def customer_applicable_rules + # Rules which apply specifically to the current customer + @customer_applicable_rules ||= non_default_rules.select{ |rule| customer_tagged?(rule) } + end + + def hide_rules + @hide_rules ||= customer_applicable_rules. + select{ |rule| rule.preferred_matched_variants_visibility == 'hidden' } + end + + def show_rules + customer_applicable_rules - hide_rules + end + + def customer_tagged?(rule) + customer_tag_list.include? rule.preferred_customer_tags + end + + def customer_tag_list + customer.andand.tag_list || [] + end +end diff --git a/spec/services/product_tag_rules_filterer_spec.rb b/spec/services/product_tag_rules_filterer_spec.rb new file mode 100644 index 00000000000..026e0ec03b8 --- /dev/null +++ b/spec/services/product_tag_rules_filterer_spec.rb @@ -0,0 +1,98 @@ +require "spec_helper" + +describe ProductTagRulesFilterer do + describe "filtering by tag rules" do + let!(:distributor) { create(:distributor_enterprise) } + let(:product) { create(:product, supplier: distributor) } + let(:v1) { create(:variant, product: product) } + let(:v2) { create(:variant, product: product) } + let(:v3) { create(:variant, product: product) } + let(:v4) { create(:variant, product: product) } + let(:variant_hidden_by_default) { create(:variant_override, variant: v1, hub: distributor) } + let(:variant_hidden_by_rule) { create(:variant_override, variant: v2, hub: distributor) } + let(:variant_shown_by_rule) { create(:variant_override, variant: v3, hub: distributor) } + let(:variant_hidden_for_another_customer) { create(:variant_override, variant: v4, hub: distributor) } + let(:customer) { create(:customer, enterprise: distributor) } + let(:variants_relation) { + Spree::Variant.joins(:product).where("spree_products.supplier_id = ?", distributor.id) + } + let(:default_hide_rule) { + create(:filter_products_tag_rule, + enterprise: distributor, + is_default: true, + preferred_variant_tags: "hide_these_variants_from_everyone", + preferred_matched_variants_visibility: "hidden") + } + let!(:hide_rule) { + create(:filter_products_tag_rule, + enterprise: distributor, + preferred_variant_tags: "hide_these_variants", + preferred_customer_tags: "hide_from_these_customers", + preferred_matched_variants_visibility: "hidden" ) + } + let!(:show_rule) { + create(:filter_products_tag_rule, + enterprise: distributor, + preferred_variant_tags: "show_these_variants", + preferred_customer_tags: "show_for_these_customers", + preferred_matched_variants_visibility: "visible" ) + } + let!(:non_applicable_rule) { + create(:filter_products_tag_rule, + enterprise: distributor, + preferred_variant_tags: "hide_these_other_variants", + preferred_customer_tags: "hide_from_other_customers", + preferred_matched_variants_visibility: "hidden" ) + } + let(:filterer) { described_class.new(distributor, customer, variants_relation) } + + context "when the distributor has no rules" do + it "returns the relation unchanged" do + expect(filterer.call).to eq variants_relation + end + end + + describe "#customer_applicable_rules" do + it "returns a list of tags that apply to the current customer" do + customer.update_attribute(:tag_list, show_rule.preferred_customer_tags) + + customer_applicable_rules = filterer.__send__(:customer_applicable_rules) + expect(customer_applicable_rules).to eq [show_rule] + end + end + + describe "#overrides_to_hide" do + context "with default rules" do + it "lists overrides tagged as hidden for this customer" do + variant_hidden_by_default.update_attribute(:tag_list, default_hide_rule.preferred_variant_tags) + + overrides_to_hide = filterer.__send__(:overrides_to_hide) + expect(overrides_to_hide).to eq [variant_hidden_by_default.id] + end + end + + context "with default and specific rules" do + it "lists overrides tagged as hidden for this customer" do + customer.update_attribute(:tag_list, hide_rule.preferred_customer_tags) + variant_hidden_by_default.update_attribute(:tag_list, default_hide_rule.preferred_variant_tags) + variant_hidden_by_rule.update_attribute(:tag_list, hide_rule.preferred_variant_tags) + variant_hidden_for_another_customer.update_attribute(:tag_list, non_applicable_rule.preferred_variant_tags) + + overrides_to_hide = filterer.__send__(:overrides_to_hide) + expect(overrides_to_hide).to eq [variant_hidden_by_default.id, variant_hidden_by_rule.id] + end + end + + end + + describe "#overrides_to_show" do + it "lists overrides tagged as visible for this customer" do + customer.update_attribute(:tag_list, show_rule.preferred_customer_tags) + variant_shown_by_rule.update_attribute(:tag_list, show_rule.preferred_variant_tags) + + overrides_to_show = filterer.__send__(:overrides_to_show) + expect(overrides_to_show).to eq [variant_shown_by_rule.id] + end + end + end +end From da7456e6e09d5ccb4f7f980fa35c52b7afc0ec04 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 4 Oct 2019 12:04:56 +0100 Subject: [PATCH 08/25] Remove old shop/products route, action, and spec --- app/controllers/shop_controller.rb | 36 ----- config/routes.rb | 1 - spec/controllers/shop_controller_spec.rb | 162 ----------------------- spec/requests/shop_spec.rb | 64 --------- 4 files changed, 263 deletions(-) delete mode 100644 spec/requests/shop_spec.rb diff --git a/app/controllers/shop_controller.rb b/app/controllers/shop_controller.rb index 1d9d727aa2c..a2af6308631 100644 --- a/app/controllers/shop_controller.rb +++ b/app/controllers/shop_controller.rb @@ -9,19 +9,6 @@ def show redirect_to main_app.enterprise_shop_path(current_distributor) end - def products - renderer = OpenFoodNetwork::CachedProductsRenderer.new(current_distributor, - current_order_cycle) - - # If we add any more filtering logic, we should probably - # move it all to a lib class like 'CachedProductsFilterer' - products_json = filter(renderer.products_json) - - render json: products_json - rescue OpenFoodNetwork::CachedProductsRenderer::NoProducts - render status: :not_found, json: '' - end - def order_cycle if request.post? if oc = OrderCycle.with_distributor(@distributor).active.find_by_id(params[:order_cycle_id]) @@ -39,27 +26,4 @@ def order_cycle def changeable_orders_alert render layout: false end - - private - - def filtered_json(products_json) - if applicator.rules.any? - filter(products_json) - else - products_json - end - end - - def filter(products_json) - products_hash = JSON.parse(products_json) - applicator.filter!(products_hash) - JSON.unparse(products_hash) - end - - def applicator - return @applicator unless @applicator.nil? - @applicator = OpenFoodNetwork::TagRuleApplicator.new(current_distributor, - "FilterProducts", - current_customer.andand.tag_list) - end end diff --git a/config/routes.rb b/config/routes.rb index 6e7df6ea646..65a70165e1f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -33,7 +33,6 @@ end resource :shop, controller: "shop" do - get :products post :order_cycle get :order_cycle get :changeable_orders_alert diff --git a/spec/controllers/shop_controller_spec.rb b/spec/controllers/shop_controller_spec.rb index 64b4e9281b4..233fe9d36a0 100644 --- a/spec/controllers/shop_controller_spec.rb +++ b/spec/controllers/shop_controller_spec.rb @@ -82,167 +82,5 @@ expect(controller.current_order_cycle).to be_nil end end - - describe "returning products" do - let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor]) } - let(:exchange) { order_cycle.exchanges.to_enterprises(distributor).outgoing.first } - - describe "requests and responses" do - let(:product) { create(:product) } - - before do - exchange.variants << product.variants.first - end - - it "returns products via JSON" do - allow(controller).to receive(:current_order_cycle).and_return order_cycle - xhr :get, :products - expect(response).to be_success - end - - it "does not return products if no order cycle is selected" do - allow(controller).to receive(:current_order_cycle).and_return nil - xhr :get, :products - expect(response.status).to eq(404) - expect(response.body).to be_empty - end - end - end - - describe "determining rule relevance" do - let(:products_json) { double(:products_json) } - let(:applicator) { double(:applicator) } - - before do - allow(applicator).to receive(:rules) { tag_rules } - allow(controller).to receive(:applicator) { applicator } - allow(controller).to receive(:filter) { "some filtered json" } - end - - context "when no relevant rules exist" do - let(:tag_rules) { [] } - - it "does not attempt to apply any rules" do - controller.send(:filtered_json, products_json) - expect(expect(controller).to_not(have_received(:filter))) - end - - it "returns products as JSON" do - expect(controller.send(:filtered_json, products_json)).to eq products_json - end - end - - context "when relevant rules exist" do - let(:tag_rule) { create(:filter_products_tag_rule, preferred_customer_tags: "tag1", preferred_variant_tags: "tag1", preferred_matched_variants_visibility: "hidden" ) } - let(:tag_rules) { [tag_rule] } - - it "attempts to apply any rules" do - controller.send(:filtered_json, products_json) - expect(controller).to have_received(:filter).with(products_json) - end - - it "returns filtered JSON" do - expect(controller.send(:filtered_json, products_json)).to eq "some filtered json" - end - end - end - - describe "loading available order cycles" do - let(:user) { create(:user) } - before { allow(controller).to receive(:spree_current_user) { user } } - - context "when FilterProducts tag rules are in effect" do - let(:customer) { create(:customer, user: user, enterprise: distributor) } - let!(:tag_rule) { - create(:filter_products_tag_rule, - enterprise: distributor, - preferred_customer_tags: "member", - preferred_variant_tags: "members-only") - } - let!(:default_tag_rule) { - create(:filter_products_tag_rule, - enterprise: distributor, - is_default: true, - preferred_variant_tags: "members-only") - } - let(:product1) { { "id" => 1, "name" => 'product 1', "variants" => [{ "id" => 4, "tag_list" => ["members-only"] }] } } - let(:product2) { { "id" => 2, "name" => 'product 2', "variants" => [{ "id" => 5, "tag_list" => ["members-only"] }, { "id" => 9, "tag_list" => ["something"] }] } } - let(:product3) { { "id" => 3, "name" => 'product 3', "variants" => [{ "id" => 6, "tag_list" => ["something-else"] }] } } - let(:product2_without_v5) { { "id" => 2, "name" => 'product 2', "variants" => [{ "id" => 9, "tag_list" => ["something"] }] } } - let!(:products_array) { [product1, product2, product3] } - let!(:products_json) { JSON.unparse( products_array ) } - - before do - allow(controller).to receive(:current_order) { order } - end - - context "with a preferred visiblity of 'visible', default visibility of 'hidden'" do - before { tag_rule.update_attribute(:preferred_matched_variants_visibility, 'visible') } - before { default_tag_rule.update_attribute(:preferred_matched_variants_visibility, 'hidden') } - - let(:filtered_products) { JSON.parse(controller.send(:filter, products_json)) } - - context "when the customer is nil" do - it "applies default action (hide)" do - expect(controller.current_customer).to be nil - expect(filtered_products).to include product2_without_v5, product3 - expect(filtered_products).to_not include product1, product2 - end - end - - context "when the customer's tags match" do - before { customer.update_attribute(:tag_list, 'member') } - - it "applies the action (show)" do - expect(controller.current_customer).to eq customer - expect(filtered_products).to include product1, product2, product3 - end - end - - context "when the customer's tags don't match" do - before { customer.update_attribute(:tag_list, 'something') } - - it "applies the default action (hide)" do - expect(controller.current_customer).to eq customer - expect(filtered_products).to include product2_without_v5, product3 - expect(filtered_products).to_not include product1, product2 - end - end - end - - context "with a preferred visiblity of 'hidden', default visibility of 'visible'" do - before { tag_rule.update_attribute(:preferred_matched_variants_visibility, 'hidden') } - before { default_tag_rule.update_attribute(:preferred_matched_variants_visibility, 'visible') } - - let(:filtered_products) { JSON.parse(controller.send(:filter, products_json)) } - - context "when the customer is nil" do - it "applies default action (show)" do - expect(controller.current_customer).to be nil - expect(filtered_products).to include product1, product2, product3 - end - end - - context "when the customer's tags match" do - before { customer.update_attribute(:tag_list, 'member') } - - it "applies the action (hide)" do - expect(controller.current_customer).to eq customer - expect(filtered_products).to include product2_without_v5, product3 - expect(filtered_products).to_not include product1, product2 - end - end - - context "when the customer's tags don't match" do - before { customer.update_attribute(:tag_list, 'something') } - - it "applies the default action (show)" do - expect(controller.current_customer).to eq customer - expect(filtered_products).to include product1, product2, product3 - end - end - end - end - end end end diff --git a/spec/requests/shop_spec.rb b/spec/requests/shop_spec.rb deleted file mode 100644 index c73e8bd099f..00000000000 --- a/spec/requests/shop_spec.rb +++ /dev/null @@ -1,64 +0,0 @@ -require 'spec_helper' - -describe "Shop API", type: :request do - include ShopWorkflow - - describe "filtering products" do - let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) } - let(:supplier) { create(:supplier_enterprise) } - let(:oc1) { create(:simple_order_cycle, distributors: [distributor], coordinator: create(:distributor_enterprise), orders_close_at: 2.days.from_now) } - let(:p4) { create(:simple_product, on_demand: false) } - let(:p5) { create(:simple_product, on_demand: false) } - let(:p6) { create(:simple_product, on_demand: false) } - let(:p7) { create(:simple_product, on_demand: false) } - let(:v41) { p4.variants.first } - let(:v42) { create(:variant, product: p4, unit_value: 3, on_demand: false) } - let(:v43) { create(:variant, product: p4, unit_value: 4, on_demand: true) } - let(:v51) { p5.variants.first } - let(:v52) { create(:variant, product: p5) } - let(:v61) { p6.variants.first } - let(:v71) { p7.variants.first } - let(:order) { create(:order, distributor: distributor, order_cycle: oc1) } - - before do - set_order order - - v61.update_attribute(:on_hand, 1) - p6.destroy - v71.update_attribute(:on_hand, 1) - v41.update_attribute(:on_hand, 1) - v42.update_attribute(:on_hand, 0) - v43.update_attribute(:on_hand, 0) - v51.update_attribute(:on_hand, 1) - v52.update_attribute(:on_hand, 0) - v71.update_attribute(:on_hand, 1) - v71.update_attribute(:deleted_at, Time.zone.now) - exchange = Exchange.find(oc1.exchanges.to_enterprises(distributor).outgoing.first.id) - exchange.update_attribute :pickup_time, "frogs" - exchange.variants << v61 - exchange.variants << v41 - exchange.variants << v42 - exchange.variants << v43 - # v51 is in stock but not in distribution - # v52 is out of stock and in the distribution - # Neither should display, nor should their product, p5 - exchange.variants << v52 - exchange.variants << v71 - get products_shop_path - end - - it "filters products based on availability" do - # It shows on demand variants - expect(response.body).to include v43.options_text - # It does not show variants that are neither on hand or on demand - expect(response.body).not_to include v42.options_text - # It does not show products that have no available variants in this distribution - expect(response.body).not_to include p5.name - # It does not show deleted products - expect(response.body).not_to include p6.name - # It does not show deleted variants - expect(response.body).not_to include v71.name - expect(response.body).not_to include p7.name - end - end -end From ab330e882eb1745606739bbbdcd14205ed8b4ad7 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 4 Oct 2019 12:16:38 +0100 Subject: [PATCH 09/25] Remove product cache --- .rubocop_manual_todo.yml | 3 - .../admin/cache_settings_controller.rb | 27 -- .../api/order_cycles_controller.rb | 2 + app/controllers/shop_controller.rb | 2 - .../products_cache_integrity_checker_job.rb | 30 -- app/jobs/refresh_products_cache_job.rb | 21 - app/models/coordinator_fee.rb | 9 - app/models/enterprise_fee.rb | 8 - app/models/exchange.rb | 11 - app/models/exchange_fee.rb | 9 - app/models/inventory_item.rb | 10 - app/models/order_cycle.rb | 6 - app/models/producer_property.rb | 13 - app/models/spree/classification_decorator.rb | 7 - app/models/spree/image_decorator.rb | 9 - app/models/spree/option_type_decorator.rb | 7 - app/models/spree/option_value_decorator.rb | 18 - app/models/spree/preference_decorator.rb | 30 -- app/models/spree/price_decorator.rb | 6 - app/models/spree/product_decorator.rb | 17 +- .../spree/product_property_decorator.rb | 5 - app/models/spree/property_decorator.rb | 11 - app/models/spree/stock_movement_decorator.rb | 10 - app/models/spree/taxon_decorator.rb | 8 - app/models/spree/variant_decorator.rb | 28 +- app/models/variant_override.rb | 13 - app/serializers/api/product_serializer.rb | 42 +- app/views/admin/cache_settings/edit.html.haml | 31 -- .../shared/_configuration_menu.html.haml | 1 - config/locales/en.yml | 10 - config/routes/admin.rb | 2 - config/schedule.rb | 4 - .../cached_products_renderer.rb | 48 -- lib/open_food_network/products_cache.rb | 190 -------- .../products_cache_integrity_checker.rb | 34 -- .../products_cache_refreshment.rb | 42 -- lib/tasks/cache.rake | 19 - .../api/variants_controller_spec.rb | 18 - .../spree/admin/variants_controller_spec.rb | 10 - spec/features/admin/caching_spec.rb | 46 -- spec/features/admin/enterprises_spec.rb | 29 -- .../consumer/shopping/checkout_spec.rb | 40 -- ...oducts_cache_integrity_checker_job_spec.rb | 27 -- spec/jobs/refresh_products_cache_job_spec.rb | 53 --- .../cached_products_renderer_spec.rb | 134 ------ .../products_cache_refreshment_spec.rb | 69 --- .../open_food_network/products_cache_spec.rb | 431 ------------------ spec/models/coordinator_fee_spec.rb | 19 - spec/models/enterprise_fee_spec.rb | 6 - spec/models/exchange_fee_spec.rb | 19 - spec/models/exchange_spec.rb | 15 - spec/models/inventory_item_spec.rb | 16 - spec/models/order_cycle_spec.rb | 12 - spec/models/producer_property_spec.rb | 13 - spec/models/spree/classification_spec.rb | 13 - spec/models/spree/image_spec.rb | 18 - spec/models/spree/option_type_spec.rb | 28 -- spec/models/spree/option_value_spec.rb | 26 -- spec/models/spree/preference_spec.rb | 23 - spec/models/spree/price_spec.rb | 17 - spec/models/spree/product_property_spec.rb | 21 - spec/models/spree/product_spec.rb | 14 - spec/models/spree/property_spec.rb | 12 - spec/models/spree/taxon_spec.rb | 15 - spec/models/spree/variant_spec.rb | 34 -- spec/models/variant_override_spec.rb | 15 - 66 files changed, 18 insertions(+), 1918 deletions(-) delete mode 100644 app/controllers/admin/cache_settings_controller.rb delete mode 100644 app/jobs/products_cache_integrity_checker_job.rb delete mode 100644 app/jobs/refresh_products_cache_job.rb delete mode 100644 app/models/spree/option_value_decorator.rb delete mode 100644 app/models/spree/preference_decorator.rb delete mode 100644 app/models/spree/stock_movement_decorator.rb delete mode 100644 app/views/admin/cache_settings/edit.html.haml delete mode 100644 lib/open_food_network/cached_products_renderer.rb delete mode 100644 lib/open_food_network/products_cache.rb delete mode 100644 lib/open_food_network/products_cache_integrity_checker.rb delete mode 100644 lib/open_food_network/products_cache_refreshment.rb delete mode 100644 lib/tasks/cache.rake delete mode 100644 spec/features/admin/caching_spec.rb delete mode 100644 spec/jobs/products_cache_integrity_checker_job_spec.rb delete mode 100644 spec/jobs/refresh_products_cache_job_spec.rb delete mode 100644 spec/lib/open_food_network/cached_products_renderer_spec.rb delete mode 100644 spec/lib/open_food_network/products_cache_refreshment_spec.rb delete mode 100644 spec/lib/open_food_network/products_cache_spec.rb delete mode 100644 spec/models/coordinator_fee_spec.rb delete mode 100644 spec/models/exchange_fee_spec.rb delete mode 100644 spec/models/inventory_item_spec.rb delete mode 100644 spec/models/spree/option_type_spec.rb delete mode 100644 spec/models/spree/option_value_spec.rb delete mode 100644 spec/models/spree/preference_spec.rb delete mode 100644 spec/models/spree/product_property_spec.rb diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index 0804602b704..4b2c00700c4 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -262,13 +262,10 @@ Metrics/LineLength: - spec/helpers/order_cycles_helper_spec.rb - spec/helpers/spree/admin/base_helper_spec.rb - spec/jobs/confirm_order_job_spec.rb - - spec/jobs/products_cache_integrity_checker_job_spec.rb - - spec/jobs/refresh_products_cache_job_spec.rb - spec/jobs/subscription_confirm_job_spec.rb - spec/jobs/subscription_placement_job_spec.rb - spec/lib/open_food_network/address_finder_spec.rb - spec/lib/open_food_network/bulk_coop_report_spec.rb - - spec/lib/open_food_network/cached_products_renderer_spec.rb - spec/lib/open_food_network/customers_report_spec.rb - spec/lib/open_food_network/enterprise_fee_applicator_spec.rb - spec/lib/open_food_network/enterprise_fee_calculator_spec.rb diff --git a/app/controllers/admin/cache_settings_controller.rb b/app/controllers/admin/cache_settings_controller.rb deleted file mode 100644 index e922031f686..00000000000 --- a/app/controllers/admin/cache_settings_controller.rb +++ /dev/null @@ -1,27 +0,0 @@ -require 'open_food_network/products_cache_integrity_checker' - -module Admin - class CacheSettingsController < Spree::Admin::BaseController - def edit - @results = Exchange.cachable.map do |exchange| - checker = OpenFoodNetwork::ProductsCacheIntegrityChecker - .new(exchange.receiver, exchange.order_cycle) - - { - distributor: exchange.receiver, - order_cycle: exchange.order_cycle, - status: checker.ok?, - diff: checker.diff - } - end - end - - def update - Spree::Config.set(params[:preferences]) - - respond_to do |format| - format.html { redirect_to main_app.edit_admin_cache_settings_path } - end - end - end -end diff --git a/app/controllers/api/order_cycles_controller.rb b/app/controllers/api/order_cycles_controller.rb index 34280e67711..42eb6aa21e8 100644 --- a/app/controllers/api/order_cycles_controller.rb +++ b/app/controllers/api/order_cycles_controller.rb @@ -1,3 +1,5 @@ +require 'open_food_network/products_renderer' + module Api class OrderCyclesController < BaseController include EnterprisesHelper diff --git a/app/controllers/shop_controller.rb b/app/controllers/shop_controller.rb index a2af6308631..ee304d1f961 100644 --- a/app/controllers/shop_controller.rb +++ b/app/controllers/shop_controller.rb @@ -1,5 +1,3 @@ -require 'open_food_network/cached_products_renderer' - class ShopController < BaseController layout "darkswarm" before_filter :require_distributor_chosen, :set_order_cycles, except: :changeable_orders_alert diff --git a/app/jobs/products_cache_integrity_checker_job.rb b/app/jobs/products_cache_integrity_checker_job.rb deleted file mode 100644 index 1adf229c7cd..00000000000 --- a/app/jobs/products_cache_integrity_checker_job.rb +++ /dev/null @@ -1,30 +0,0 @@ -require 'open_food_network/products_cache_integrity_checker' - -ProductsCacheIntegrityCheckerJob = Struct.new(:distributor_id, :order_cycle_id) do - def perform - unless checker.ok? - exception = RuntimeError.new( - "Products JSON differs from cached version for distributor: #{distributor_id}, " \ - "order cycle: #{order_cycle_id}" - ) - - Bugsnag.notify(exception) do |report| - report.add_tab(:products_cache, diff: checker.diff.to_s(:text)) - end - end - end - - private - - def checker - OpenFoodNetwork::ProductsCacheIntegrityChecker.new(distributor, order_cycle) - end - - def distributor - Enterprise.find distributor_id - end - - def order_cycle - OrderCycle.find order_cycle_id - end -end diff --git a/app/jobs/refresh_products_cache_job.rb b/app/jobs/refresh_products_cache_job.rb deleted file mode 100644 index b08148b2806..00000000000 --- a/app/jobs/refresh_products_cache_job.rb +++ /dev/null @@ -1,21 +0,0 @@ -require 'open_food_network/products_renderer' - -RefreshProductsCacheJob = Struct.new(:distributor_id, :order_cycle_id) do - def perform - Rails.cache.write(key, products_json) - rescue ActiveRecord::RecordNotFound - true - end - - private - - def key - "products-json-#{distributor_id}-#{order_cycle_id}" - end - - def products_json - distributor = Enterprise.find distributor_id - order_cycle = OrderCycle.find order_cycle_id - OpenFoodNetwork::ProductsRenderer.new(distributor, order_cycle).products_json - end -end diff --git a/app/models/coordinator_fee.rb b/app/models/coordinator_fee.rb index 99aefc2595c..f15a1941587 100644 --- a/app/models/coordinator_fee.rb +++ b/app/models/coordinator_fee.rb @@ -1,13 +1,4 @@ class CoordinatorFee < ActiveRecord::Base belongs_to :order_cycle belongs_to :enterprise_fee - - after_save :refresh_products_cache - after_destroy :refresh_products_cache - - private - - def refresh_products_cache - order_cycle.refresh_products_cache - end end diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb index aaeb5101d4a..64c362b0fe5 100644 --- a/app/models/enterprise_fee.rb +++ b/app/models/enterprise_fee.rb @@ -10,10 +10,6 @@ class EnterpriseFee < ActiveRecord::Base has_many :exchange_fees, dependent: :destroy has_many :exchanges, through: :exchange_fees - after_save :refresh_products_cache - # After destroy, the products cache is refreshed via the after_destroy hook for - # coordinator_fees and exchange_fees - attr_accessible :enterprise_id, :fee_type, :name, :tax_category_id, :calculator_type, :inherits_tax_category FEE_TYPES = %w(packing transport admin sales fundraising).freeze @@ -59,8 +55,4 @@ def ensure_valid_tax_category_settings end true end - - def refresh_products_cache - OpenFoodNetwork::ProductsCache.enterprise_fee_changed self - end end diff --git a/app/models/exchange.rb b/app/models/exchange.rb index 78e9f4aba74..ce28a7255db 100644 --- a/app/models/exchange.rb +++ b/app/models/exchange.rb @@ -23,9 +23,6 @@ class Exchange < ActiveRecord::Base validates :order_cycle, :sender, :receiver, presence: true validates :sender_id, uniqueness: { scope: [:order_cycle_id, :receiver_id, :incoming] } - after_save :refresh_products_cache - after_destroy :refresh_products_cache_from_destroy - accepts_nested_attributes_for :variants scope :in_order_cycle, lambda { |order_cycle| where(order_cycle_id: order_cycle) } @@ -98,12 +95,4 @@ def role def participant incoming? ? sender : receiver end - - def refresh_products_cache - OpenFoodNetwork::ProductsCache.exchange_changed self - end - - def refresh_products_cache_from_destroy - OpenFoodNetwork::ProductsCache.exchange_destroyed self - end end diff --git a/app/models/exchange_fee.rb b/app/models/exchange_fee.rb index 27636f2f28c..ff9f12e8ddf 100644 --- a/app/models/exchange_fee.rb +++ b/app/models/exchange_fee.rb @@ -1,13 +1,4 @@ class ExchangeFee < ActiveRecord::Base belongs_to :exchange belongs_to :enterprise_fee - - after_save :refresh_products_cache - after_destroy :refresh_products_cache - - private - - def refresh_products_cache - exchange.refresh_products_cache - end end diff --git a/app/models/inventory_item.rb b/app/models/inventory_item.rb index e7aa9dccc0e..5e4bb4ef8b4 100644 --- a/app/models/inventory_item.rb +++ b/app/models/inventory_item.rb @@ -1,5 +1,3 @@ -require 'open_food_network/products_cache' - class InventoryItem < ActiveRecord::Base attr_accessible :enterprise, :enterprise_id, :variant, :variant_id, :visible @@ -13,12 +11,4 @@ class InventoryItem < ActiveRecord::Base scope :visible, -> { where(visible: true) } scope :hidden, -> { where(visible: false) } - - after_save :refresh_products_cache - - private - - def refresh_products_cache - OpenFoodNetwork::ProductsCache.inventory_item_changed self - end end diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 4a37a139f97..ce931e5ea2b 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -23,8 +23,6 @@ class OrderCycle < ActiveRecord::Base validates :name, :coordinator_id, presence: true validate :orders_close_at_after_orders_open_at? - after_save :refresh_products_cache - preference :product_selection_from_coordinator_inventory_only, :boolean, default: false scope :active, lambda { @@ -247,10 +245,6 @@ def coordinated_by?(user) coordinator.users.include? user end - def refresh_products_cache - OpenFoodNetwork::ProductsCache.order_cycle_changed self - end - def items_bought_by_user(user, distributor) # The Spree::Order.complete scope only checks for completed_at date # it does not ensure state is "complete" diff --git a/app/models/producer_property.rb b/app/models/producer_property.rb index 5f299b344fa..6f870b1ba9e 100644 --- a/app/models/producer_property.rb +++ b/app/models/producer_property.rb @@ -4,9 +4,6 @@ class ProducerProperty < ActiveRecord::Base default_scope order("#{table_name}.position") - after_save :refresh_products_cache - after_destroy :refresh_products_cache_from_destroy - scope :ever_sold_by, ->(shop) { joins(producer: { supplied_products: { variants: { exchanges: :order_cycle } } }). merge(Exchange.outgoing). @@ -29,14 +26,4 @@ def property_name=(name) Spree::Property.create(name: name, presentation: name) end end - - private - - def refresh_products_cache - OpenFoodNetwork::ProductsCache.producer_property_changed self - end - - def refresh_products_cache_from_destroy - OpenFoodNetwork::ProductsCache.producer_property_destroyed self - end end diff --git a/app/models/spree/classification_decorator.rb b/app/models/spree/classification_decorator.rb index 8cb9f7cc66a..fec270bc14b 100644 --- a/app/models/spree/classification_decorator.rb +++ b/app/models/spree/classification_decorator.rb @@ -2,16 +2,9 @@ belongs_to :product, class_name: "Spree::Product", touch: true before_destroy :dont_destroy_if_primary_taxon - after_destroy :refresh_products_cache - after_save :refresh_products_cache private - def refresh_products_cache - product = Spree::Product.with_deleted.find(product_id) if product.blank? - product.refresh_products_cache - end - def dont_destroy_if_primary_taxon if product.primary_taxon == taxon errors.add :base, I18n.t(:spree_classification_primary_taxon_error, taxon: taxon.name, product: product.name) diff --git a/app/models/spree/image_decorator.rb b/app/models/spree/image_decorator.rb index 90f62854db2..7d86aae3a63 100644 --- a/app/models/spree/image_decorator.rb +++ b/app/models/spree/image_decorator.rb @@ -1,7 +1,4 @@ Spree::Image.class_eval do - after_save :refresh_products_cache - after_destroy :refresh_products_cache - # Spree stores attachent definitions in JSON. This converts the style name and format to # strings. However, when paperclip encounters these, it doesn't recognise the format. # Here we solve that problem by converting format and style name to symbols. @@ -23,10 +20,4 @@ def self.reformat_styles end reformat_styles - - private - - def refresh_products_cache - viewable.try :refresh_products_cache - end end diff --git a/app/models/spree/option_type_decorator.rb b/app/models/spree/option_type_decorator.rb index 2ebdece88cc..1ef46caa4e7 100644 --- a/app/models/spree/option_type_decorator.rb +++ b/app/models/spree/option_type_decorator.rb @@ -1,12 +1,5 @@ module Spree OptionType.class_eval do has_many :products, through: :product_option_types - after_save :refresh_products_cache - - private - - def refresh_products_cache - products(:reload).each(&:refresh_products_cache) - end end end diff --git a/app/models/spree/option_value_decorator.rb b/app/models/spree/option_value_decorator.rb deleted file mode 100644 index 48d88ff26e1..00000000000 --- a/app/models/spree/option_value_decorator.rb +++ /dev/null @@ -1,18 +0,0 @@ -module Spree - OptionValue.class_eval do - after_save :refresh_products_cache - around_destroy :refresh_products_cache_from_destroy - - private - - def refresh_products_cache - variants(:reload).each(&:refresh_products_cache) - end - - def refresh_products_cache_from_destroy - vs = variants(:reload).to_a - yield - vs.each(&:refresh_products_cache) - end - end -end diff --git a/app/models/spree/preference_decorator.rb b/app/models/spree/preference_decorator.rb deleted file mode 100644 index e5ad7cbda23..00000000000 --- a/app/models/spree/preference_decorator.rb +++ /dev/null @@ -1,30 +0,0 @@ -require 'open_food_network/products_cache' - -module Spree - Preference.class_eval do - after_save :refresh_products_cache - - # When the setting preferred_product_selection_from_inventory_only has changed, we want to - # refresh all active exchanges for this enterprise. - def refresh_products_cache - if product_selection_from_inventory_only_changed? - OpenFoodNetwork::ProductsCache.distributor_changed(enterprise) - end - end - - private - - def product_selection_from_inventory_only_changed? - !!(key =~ product_selection_from_inventory_only_regex) - end - - def enterprise - enterprise_id = key.match(product_selection_from_inventory_only_regex)[1] - Enterprise.find(enterprise_id) - end - - def product_selection_from_inventory_only_regex - /^enterprise\/product_selection_from_inventory_only\/(\d+)$/ - end - end -end diff --git a/app/models/spree/price_decorator.rb b/app/models/spree/price_decorator.rb index 036daae4de7..6c67ca59492 100644 --- a/app/models/spree/price_decorator.rb +++ b/app/models/spree/price_decorator.rb @@ -2,8 +2,6 @@ module Spree Price.class_eval do acts_as_paranoid without_default_scope: true - after_save :refresh_products_cache - # Allow prices to access associated soft-deleted variants. def variant Spree::Variant.unscoped { super } @@ -16,9 +14,5 @@ def check_price self.currency = Spree::Config[:currency] end end - - def refresh_products_cache - variant.andand.refresh_products_cache - end end end diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb index ac7b8791a99..d6f6e311086 100644 --- a/app/models/spree/product_decorator.rb +++ b/app/models/spree/product_decorator.rb @@ -38,7 +38,6 @@ after_save :remove_previous_primary_taxon_from_taxons after_save :ensure_standard_variant after_save :update_units - after_save :refresh_products_cache # -- Joins scope :with_order_cycles_outer, -> { @@ -192,23 +191,17 @@ def variant_unit_option_type def destroy_with_delete_from_order_cycles transaction do - OpenFoodNetwork::ProductsCache.product_deleted(self) do - touch_distributors + touch_distributors - ExchangeVariant. - where('exchange_variants.variant_id IN (?)', variants_including_master.with_deleted. - select(:id)).destroy_all + ExchangeVariant. + where('exchange_variants.variant_id IN (?)', variants_including_master.with_deleted. + select(:id)).destroy_all - destroy_without_delete_from_order_cycles - end + destroy_without_delete_from_order_cycles end end alias_method_chain :destroy, :delete_from_order_cycles - def refresh_products_cache - OpenFoodNetwork::ProductsCache.product_changed self - end - private def set_available_on_to_now diff --git a/app/models/spree/product_property_decorator.rb b/app/models/spree/product_property_decorator.rb index 34432fb3101..58ee8fb1ada 100644 --- a/app/models/spree/product_property_decorator.rb +++ b/app/models/spree/product_property_decorator.rb @@ -1,10 +1,5 @@ module Spree ProductProperty.class_eval do belongs_to :product, class_name: "Spree::Product", touch: true - - after_save :refresh_products_cache - after_destroy :refresh_products_cache - - delegate :refresh_products_cache, to: :product end end diff --git a/app/models/spree/property_decorator.rb b/app/models/spree/property_decorator.rb index 20088ab243b..aeeb607af96 100644 --- a/app/models/spree/property_decorator.rb +++ b/app/models/spree/property_decorator.rb @@ -20,19 +20,8 @@ module Spree merge(OrderCycle.active) } - after_save :refresh_products_cache - - # When a Property is destroyed, dependent-destroy will destroy all ProductProperties, - # which will take care of refreshing the products cache - def property self end - - private - - def refresh_products_cache - product_properties(:reload).each(&:refresh_products_cache) - end end end diff --git a/app/models/spree/stock_movement_decorator.rb b/app/models/spree/stock_movement_decorator.rb deleted file mode 100644 index 40aa0499430..00000000000 --- a/app/models/spree/stock_movement_decorator.rb +++ /dev/null @@ -1,10 +0,0 @@ -Spree::StockMovement.class_eval do - after_save :refresh_products_cache - - private - - def refresh_products_cache - return if stock_item.variant.blank? - OpenFoodNetwork::ProductsCache.variant_changed stock_item.variant - end -end diff --git a/app/models/spree/taxon_decorator.rb b/app/models/spree/taxon_decorator.rb index d5790b2c1d6..ad2a85cfdb6 100644 --- a/app/models/spree/taxon_decorator.rb +++ b/app/models/spree/taxon_decorator.rb @@ -4,8 +4,6 @@ attachment_definitions[:icon][:path] = 'public/images/spree/taxons/:id/:style/:basename.:extension' attachment_definitions[:icon][:url] = '/images/spree/taxons/:id/:style/:basename.:extension' - after_save :refresh_products_cache - # Indicate which filters should be used for this taxon def applicable_filters fs = [] @@ -49,10 +47,4 @@ def self.distributed_taxons(which_taxons = :all) ts[t.enterprise_id.to_i] << t.id end end - - private - - def refresh_products_cache - products(:reload).each(&:refresh_products_cache) - end end diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 83c89122e6f..4c9d27d9b99 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -1,7 +1,6 @@ require 'open_food_network/enterprise_fee_calculator' require 'open_food_network/variant_and_line_item_naming' require 'concerns/variant_stock' -require 'open_food_network/products_cache' Spree::Variant.class_eval do extend Spree::LocalizedNumber @@ -30,7 +29,6 @@ before_validation :update_weight_from_unit_value, if: ->(v) { v.product.present? } after_save :update_units - after_save :refresh_products_cache around_destroy :destruction scope :with_order_cycles_inner, -> { joins(exchanges: :order_cycle) } @@ -108,14 +106,6 @@ def fees_by_type_for(distributor, order_cycle) OpenFoodNetwork::EnterpriseFeeCalculator.new(distributor, order_cycle).fees_by_type_for self end - def refresh_products_cache - if is_master? - product.refresh_products_cache - else - OpenFoodNetwork::ProductsCache.variant_changed self - end - end - private def update_weight_from_unit_value @@ -123,21 +113,7 @@ def update_weight_from_unit_value end def destruction - if is_master? - exchange_variants(:reload).destroy_all - yield - product.refresh_products_cache - - else - OpenFoodNetwork::ProductsCache.variant_destroyed(self) do - # Remove this association here instead of using dependent: :destroy because - # dependent-destroy acts before this around_filter is called, so ProductsCache - # has no way of knowing which exchanges the variant was a member of. - exchange_variants(:reload).destroy_all - - # Destroy the variant - yield - end - end + exchange_variants(:reload).destroy_all + yield end end diff --git a/app/models/variant_override.rb b/app/models/variant_override.rb index 4c367652fd9..9f0160202b9 100644 --- a/app/models/variant_override.rb +++ b/app/models/variant_override.rb @@ -12,9 +12,6 @@ class VariantOverride < ActiveRecord::Base # Default stock can be nil, indicating stock should not be reset or zero, meaning reset to zero. Need to ensure this can be set by the user. validates :default_stock, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true - after_save :refresh_products_cache_from_save - after_destroy :refresh_products_cache_from_destroy - default_scope where(permission_revoked_at: nil) scope :for_hubs, lambda { |hubs| @@ -73,14 +70,4 @@ def reset_stock! end self end - - private - - def refresh_products_cache_from_save - OpenFoodNetwork::ProductsCache.variant_override_changed self - end - - def refresh_products_cache_from_destroy - OpenFoodNetwork::ProductsCache.variant_override_destroyed self - end end diff --git a/app/serializers/api/product_serializer.rb b/app/serializers/api/product_serializer.rb index 91ff62f52ac..592b67ecdbd 100644 --- a/app/serializers/api/product_serializer.rb +++ b/app/serializers/api/product_serializer.rb @@ -1,43 +1,11 @@ require 'open_food_network/scope_variant_to_hub' class Api::ProductSerializer < ActiveModel::Serializer - # TODO - # Prices can't be cached? How? - def serializable_hash - cached_serializer_hash.merge(uncached_serializer_hash) - end - - private - - def cached_serializer_hash - Api::CachedProductSerializer.new(object, @options).serializable_hash - end - - def uncached_serializer_hash - Api::UncachedProductSerializer.new(object, @options).serializable_hash - end -end - -class Api::UncachedProductSerializer < ActiveModel::Serializer - attributes :price - - def price - if options[:enterprise_fee_calculator] - object.master.price + options[:enterprise_fee_calculator].indexed_fees_for(object.master) - else - object.master.price_with_fees(options[:current_distributor], options[:current_order_cycle]) - end - end -end - -class Api::CachedProductSerializer < ActiveModel::Serializer - # cached - # delegate :cache_key, to: :object include ActionView::Helpers::SanitizeHelper attributes :id, :name, :permalink, :meta_keywords attributes :group_buy, :notes, :description, :description_html - attributes :properties_with_values + attributes :properties_with_values, :price has_many :variants, serializer: Api::VariantSerializer has_one :master, serializer: Api::VariantSerializer @@ -70,4 +38,12 @@ def variants def master options[:master_variants][object.id].andand.first end + + def price + if options[:enterprise_fee_calculator] + object.master.price + options[:enterprise_fee_calculator].indexed_fees_for(object.master) + else + object.master.price_with_fees(options[:current_distributor], options[:current_order_cycle]) + end + end end diff --git a/app/views/admin/cache_settings/edit.html.haml b/app/views/admin/cache_settings/edit.html.haml deleted file mode 100644 index 222910be94d..00000000000 --- a/app/views/admin/cache_settings/edit.html.haml +++ /dev/null @@ -1,31 +0,0 @@ -- content_for :page_title do - = t(:cache_settings) - -= form_tag main_app.admin_cache_settings_path, :method => :put do - .field - = hidden_field_tag 'preferences[enable_products_cache?]', '0' - = check_box_tag 'preferences[enable_products_cache?]', '1', Spree::Config[:enable_products_cache?] - = label_tag nil, t('.enable_products_cache') - .form-buttons - = button t(:update), 'icon-refresh' - -%br -%br - -%h4= t(:cache_state) -%br -%table.index - %thead - %tr - %th= t('.distributor') - %th= t('.order_cycle') - %th= t('.status') - %th= t('.diff') - %tbody - - @results.each do |result| - %tr - %td= result[:distributor].name - %td= result[:order_cycle].name - %td= result[:status] ? t(:ok) : t('.error') - %td - %pre= result[:diff].to_s(:text) diff --git a/app/views/spree/admin/shared/_configuration_menu.html.haml b/app/views/spree/admin/shared/_configuration_menu.html.haml index 76bc3cdadda..7ec62c516eb 100644 --- a/app/views/spree/admin/shared/_configuration_menu.html.haml +++ b/app/views/spree/admin/shared/_configuration_menu.html.haml @@ -21,7 +21,6 @@ = configurations_sidebar_menu_item Spree.t(:shipping_categories), admin_shipping_categories_path = configurations_sidebar_menu_item t(:enterprise_fees), main_app.admin_enterprise_fees_path = configurations_sidebar_menu_item Spree.t(:analytics_trackers), admin_trackers_path - = configurations_sidebar_menu_item t('admin.cache_settings.edit.title'), main_app.edit_admin_cache_settings_path = configurations_sidebar_menu_item t('admin.contents.edit.title'), main_app.edit_admin_contents_path = configurations_sidebar_menu_item t('admin.invoice_settings.edit.title'), main_app.edit_admin_invoice_settings_path = configurations_sidebar_menu_item t('admin.matomo_settings.edit.title'), main_app.edit_admin_matomo_settings_path diff --git a/config/locales/en.yml b/config/locales/en.yml index 975cbad3dbd..2aae212b2bc 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -365,16 +365,6 @@ en: number_localization_settings: "Number Localization Settings" enable_localized_number: "Use the international thousand/decimal separator logic" - cache_settings: - edit: - title: "Caching" - distributor: "Distributor" - order_cycle: "Order Cycle" - status: "Status" - diff: "Diff" - error: "Error" - enable_products_cache: "Enable Products Cache?" - invoice_settings: edit: title: "Invoice Settings" diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 86a0ffbacaa..5b21d0baa2f 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -75,8 +75,6 @@ resource :contents - resource :cache_settings - resources :column_preferences, only: [], format: :json do put :bulk_update, on: :collection end diff --git a/config/schedule.rb b/config/schedule.rb index 9ad169cacff..b1c5928aa9a 100644 --- a/config/schedule.rb +++ b/config/schedule.rb @@ -12,10 +12,6 @@ job_type :enqueue_job, "cd :path; :environment_variable=:environment bundle exec script/enqueue :task :priority :output" -every 1.day, at: '01:00am' do - rake 'ofn:cache:check_products_integrity' -end - every 1.day, at: '2:45am' do rake 'db2fog:clean' if ENV['S3_BACKUPS_BUCKET'] end diff --git a/lib/open_food_network/cached_products_renderer.rb b/lib/open_food_network/cached_products_renderer.rb deleted file mode 100644 index 6d31705cd45..00000000000 --- a/lib/open_food_network/cached_products_renderer.rb +++ /dev/null @@ -1,48 +0,0 @@ -require 'open_food_network/products_renderer' - -# Wrapper for ProductsRenderer that caches the JSON output. -# ProductsRenderer::NoProducts is represented in the cache as nil, -# but re-raised to provide the same interface as ProductsRenderer. - -module OpenFoodNetwork - class CachedProductsRenderer - class NoProducts < RuntimeError; end - - def initialize(distributor, order_cycle) - @distributor = distributor - @order_cycle = order_cycle - end - - def products_json - raise NoProducts, I18n.t(:no_products) if @distributor.nil? || @order_cycle.nil? - - products_json = cached_products_json - - raise NoProducts, I18n.t(:no_products) if products_json.nil? - - products_json - end - - private - - def cached_products_json - return uncached_products_json unless use_cached_products? - - Rails.cache.fetch("products-json-#{@distributor.id}-#{@order_cycle.id}") do - begin - uncached_products_json - rescue ProductsRenderer::NoProducts - nil - end - end - end - - def use_cached_products? - Spree::Config[:enable_products_cache?] - end - - def uncached_products_json - ProductsRenderer.new(@distributor, @order_cycle).products_json - end - end -end diff --git a/lib/open_food_network/products_cache.rb b/lib/open_food_network/products_cache.rb deleted file mode 100644 index 089dc830308..00000000000 --- a/lib/open_food_network/products_cache.rb +++ /dev/null @@ -1,190 +0,0 @@ -require 'open_food_network/products_cache_refreshment' - -# When elements of the data model change, refresh the appropriate parts of the products cache. - -module OpenFoodNetwork - class ProductsCache - def self.variant_changed(variant) - exchanges_featuring_variants(variant.id).each do |exchange| - refresh_cache exchange.receiver, exchange.order_cycle - end - end - - def self.variant_destroyed(variant, &block) - exchanges = exchanges_featuring_variants(variant.id).to_a - - block.call - - exchanges.each do |exchange| - refresh_cache exchange.receiver, exchange.order_cycle - end - end - - def self.product_changed(product) - exchanges_featuring_variants(product.variants.map(&:id)).each do |exchange| - refresh_cache exchange.receiver, exchange.order_cycle - end - end - - def self.product_deleted(product, &block) - exchanges = exchanges_featuring_variants(product.reload.variants.map(&:id)).to_a - - block.call - - exchanges.each do |exchange| - refresh_cache exchange.receiver, exchange.order_cycle - end - end - - def self.variant_override_changed(variant_override) - exchanges_featuring_variants(variant_override.variant.id, distributor: variant_override.hub).each do |exchange| - refresh_cache exchange.receiver, exchange.order_cycle - end - end - - def self.variant_override_destroyed(variant_override) - variant_override_changed variant_override - end - - def self.producer_property_changed(producer_property) - products = producer_property.producer.supplied_products - variants = Spree::Variant. - where(is_master: false, deleted_at: nil). - where(product_id: products) - - exchanges_featuring_variants(variants.select(:id)).each do |exchange| - refresh_cache exchange.receiver, exchange.order_cycle - end - end - - def self.producer_property_destroyed(producer_property) - producer_property_changed producer_property - end - - def self.order_cycle_changed(order_cycle) - return if order_cycle.undated? - return if order_cycle.closed? - - order_cycle.exchanges.outgoing.each do |exchange| - refresh_cache exchange.receiver, order_cycle - end - end - - def self.exchange_changed(exchange) - if exchange.incoming - refresh_incoming_exchanges(Exchange.where(id: exchange)) - else - refresh_outgoing_exchange(exchange) - end - end - - def self.exchange_destroyed(exchange) - exchange_changed exchange - end - - def self.enterprise_fee_changed(enterprise_fee) - refresh_supplier_fee enterprise_fee - refresh_coordinator_fee enterprise_fee - refresh_distributor_fee enterprise_fee - end - - def self.distributor_changed(enterprise) - Exchange.cachable.where(receiver_id: enterprise).each do |exchange| - refresh_cache exchange.receiver, exchange.order_cycle - end - end - - def self.inventory_item_changed(inventory_item) - exchanges_featuring_variants(inventory_item.variant.id, - distributor: inventory_item.enterprise).each do |exchange| - refresh_cache exchange.receiver, exchange.order_cycle - end - end - - def self.exchanges_featuring_variants(variant_ids, distributor: nil) - exchanges = Exchange. - outgoing. - with_any_variant(variant_ids). - joins(:order_cycle). - merge(OrderCycle.dated). - merge(OrderCycle.not_closed) - - exchanges = exchanges.to_enterprise(distributor) if distributor - - exchanges - end - private_class_method :exchanges_featuring_variants - - def self.refresh_incoming_exchanges(exchanges) - outgoing_exchanges_affected_by(exchanges).each do |outgoing_exchange| - refresh_cache(outgoing_exchange.receiver, outgoing_exchange.order_cycle) - end - end - private_class_method :refresh_incoming_exchanges - - def self.refresh_outgoing_exchange(exchange) - return if exchange.order_cycle.undated? - return if exchange.order_cycle.closed? - - refresh_cache(exchange.receiver, exchange.order_cycle) - end - private_class_method :refresh_outgoing_exchange - - def self.refresh_supplier_fee(enterprise_fee) - refresh_incoming_exchanges(enterprise_fee.exchanges) - end - private_class_method :refresh_supplier_fee - - def self.refresh_coordinator_fee(enterprise_fee) - enterprise_fee.order_cycles.each do |order_cycle| - order_cycle_changed order_cycle - end - end - private_class_method :refresh_coordinator_fee - - def self.refresh_distributor_fee(enterprise_fee) - enterprise_fee.exchange_fees. - joins(exchange: :order_cycle). - merge(Exchange.outgoing). - merge(OrderCycle.dated). - merge(OrderCycle.not_closed). - each do |exf| - - refresh_cache exf.exchange.receiver, exf.exchange.order_cycle - end - end - private_class_method :refresh_distributor_fee - - def self.incoming_exchanges(exchanges) - exchanges. - incoming. - joins(:order_cycle). - merge(OrderCycle.dated). - merge(OrderCycle.not_closed) - end - private_class_method :incoming_exchanges - - def self.outgoing_exchanges_affected_by(exchanges) - incoming = incoming_exchanges(exchanges) - order_cycle_ids = incoming.select(:order_cycle_id) - variant_ids = ExchangeVariant.where(exchange_id: incoming).select(:variant_id) - Exchange.outgoing. - in_order_cycle(order_cycle_ids). - with_any_variant(variant_ids). - except(:select).select("DISTINCT receiver_id, order_cycle_id") - end - private_class_method :outgoing_exchanges_affected_by - - def self.outgoing_exchanges_with_variants(order_cycle, variant_ids) - order_cycle.exchanges.outgoing. - joins(:exchange_variants). - where('exchange_variants.variant_id IN (?)', variant_ids) - end - private_class_method :outgoing_exchanges_with_variants - - def self.refresh_cache(distributor, order_cycle) - ProductsCacheRefreshment.refresh distributor, order_cycle - end - private_class_method :refresh_cache - end -end diff --git a/lib/open_food_network/products_cache_integrity_checker.rb b/lib/open_food_network/products_cache_integrity_checker.rb deleted file mode 100644 index a47ea804e52..00000000000 --- a/lib/open_food_network/products_cache_integrity_checker.rb +++ /dev/null @@ -1,34 +0,0 @@ -require 'open_food_network/products_renderer' - -module OpenFoodNetwork - class ProductsCacheIntegrityChecker - def initialize(distributor, order_cycle) - @distributor = distributor - @order_cycle = order_cycle - end - - def ok? - diff.none? - end - - def diff - @diff ||= Diffy::Diff.new pretty(cached_json), pretty(rendered_json) - end - - private - - def cached_json - Rails.cache.read("products-json-#{@distributor.id}-#{@order_cycle.id}") || {}.to_json - end - - def rendered_json - OpenFoodNetwork::ProductsRenderer.new(@distributor, @order_cycle).products_json - rescue OpenFoodNetwork::ProductsRenderer::NoProducts - nil - end - - def pretty(json) - JSON.pretty_generate JSON.parse json - end - end -end diff --git a/lib/open_food_network/products_cache_refreshment.rb b/lib/open_food_network/products_cache_refreshment.rb deleted file mode 100644 index 3bcde00f8de..00000000000 --- a/lib/open_food_network/products_cache_refreshment.rb +++ /dev/null @@ -1,42 +0,0 @@ -# When enqueuing a job to refresh the products cache for a particular distribution, there -# is no benefit in having more than one job waiting in the queue to be run. - -# Imagine that an admin updates a product. This calls for the products cache to be -# updated, otherwise customers will see stale data. - -# Now while that update is running, the admin makes another change to the product. Since this change -# has been made after the previous update started running, the already-running update will not -# include that change - we need another job. So we enqueue another one. - -# Before that job starts running, our zealous admin makes yet another change. This time, there -# is a job running *and* there is a job that has not yet started to run. In this case, there's no -# benefit in enqueuing another job. When the previously enqueued job starts running, it will pick up -# our admin's update and include it. So we ignore this change (from a cache refreshment perspective) -# and go home happy to have saved our job worker's time. - -module OpenFoodNetwork - class ProductsCacheRefreshment - def self.refresh(distributor, order_cycle) - job = refresh_job(distributor, order_cycle) - enqueue_job(job) unless pending_job?(job) - end - - def self.refresh_job(distributor, order_cycle) - RefreshProductsCacheJob.new(distributor.id, order_cycle.id) - end - private_class_method :refresh_job - - def self.pending_job?(job) - Delayed::Job. - where(locked_at: nil). - where(handler: job.to_yaml). - exists? - end - private_class_method :pending_job? - - def self.enqueue_job(job) - Delayed::Job.enqueue job, priority: 10 - end - private_class_method :enqueue_job - end -end diff --git a/lib/tasks/cache.rake b/lib/tasks/cache.rake deleted file mode 100644 index 535f19079b7..00000000000 --- a/lib/tasks/cache.rake +++ /dev/null @@ -1,19 +0,0 @@ -require 'open_food_network/products_cache_integrity_checker' - -namespace :ofn do - namespace :cache do - desc 'check the integrity of the products cache' - task check_products_integrity: :environment do - Exchange.cachable.each do |exchange| - Delayed::Job.enqueue ProductsCacheIntegrityCheckerJob.new(exchange.receiver_id, exchange.order_cycle_id), priority: 20 - end - end - - desc 'warm the products cache' - task warm_products: :environment do - Exchange.cachable.each do |exchange| - Delayed::Job.enqueue RefreshProductsCacheJob.new(exchange.receiver_id, exchange.order_cycle_id), priority: 10 - end - end - end -end diff --git a/spec/controllers/api/variants_controller_spec.rb b/spec/controllers/api/variants_controller_spec.rb index 9964d40c1b6..857254ca152 100644 --- a/spec/controllers/api/variants_controller_spec.rb +++ b/spec/controllers/api/variants_controller_spec.rb @@ -115,15 +115,6 @@ expect { variant.reload }.not_to raise_error expect(variant.deleted_at).to be_nil end - - context 'when the variant is not the master' do - before { variant.update_attribute(:is_master, false) } - - it 'refreshes the cache' do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) - spree_delete :soft_delete, variant_id: variant.id, product_id: variant.product.permalink, format: :json - end - end end context "as an administrator" do @@ -150,15 +141,6 @@ expect(assigns(:variant).errors[:product]).to include "must have at least one variant" end - context 'when the variant is not the master' do - before { variant.update_attribute(:is_master, false) } - - it 'refreshes the cache' do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) - spree_delete :soft_delete, variant_id: variant.id, product_id: variant.product.permalink, format: :json - end - end - context "deleted variants" do before do variant.update_column(:deleted_at, Time.zone.now) diff --git a/spec/controllers/spree/admin/variants_controller_spec.rb b/spec/controllers/spree/admin/variants_controller_spec.rb index 4d4d1049cd1..e489e5364b5 100644 --- a/spec/controllers/spree/admin/variants_controller_spec.rb +++ b/spec/controllers/spree/admin/variants_controller_spec.rb @@ -60,11 +60,6 @@ module Admin expect(response).to render_template('spree/admin/shared/_destroy') end - it 'refreshes the cache' do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) - spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'js' - end - it 'destroys all its exchanges' do exchange = create(:exchange) variant.exchanges << exchange @@ -99,11 +94,6 @@ module Admin ) end - it 'refreshes the cache' do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) - spree_delete :destroy, id: variant.id, product_id: variant.product.permalink, format: 'js' - end - it 'destroys all its exchanges' do exchange = create(:exchange) variant.exchanges << exchange diff --git a/spec/features/admin/caching_spec.rb b/spec/features/admin/caching_spec.rb deleted file mode 100644 index 64d420a1022..00000000000 --- a/spec/features/admin/caching_spec.rb +++ /dev/null @@ -1,46 +0,0 @@ -require 'spec_helper' -require 'open_food_network/products_renderer' - -feature 'Caching' do - include AuthenticationWorkflow - include WebHelper - - before { quick_login_as_admin } - - describe "displaying integrity checker results" do - let(:distributor) { create(:distributor_enterprise) } - let(:order_cycle) { create(:open_order_cycle, distributors: [distributor]) } - - it "displays results when things are good" do - # Given matching data - Rails.cache.write "products-json-#{distributor.id}-#{order_cycle.id}", "[1, 2, 3]\n" - allow(OpenFoodNetwork::ProductsRenderer).to receive(:new) { - double(:pr, products_json: "[1, 2, 3]\n") - } - - # When I visit the cache status page - visit spree.admin_path - click_link 'Configuration' - click_link 'Caching' - - # Then I should see some status information - expect(page).to have_content "OK" - end - - it "displays results when there are errors" do - # Given matching data - Rails.cache.write "products-json-#{distributor.id}-#{order_cycle.id}", "[1, 2, 3]\n" - allow(OpenFoodNetwork::ProductsRenderer).to receive(:new) { - double(:pr, products_json: "[1, 3]\n") - } - - # When I visit the cache status page - visit spree.admin_path - click_link 'Configuration' - click_link 'Caching' - - # Then I should see some status information - expect(page).to have_content "Error" - end - end -end diff --git a/spec/features/admin/enterprises_spec.rb b/spec/features/admin/enterprises_spec.rb index f965dcb3efb..4244e265e72 100644 --- a/spec/features/admin/enterprises_spec.rb +++ b/spec/features/admin/enterprises_spec.rb @@ -281,35 +281,6 @@ end end - describe "inventory settings", js: true do - let!(:enterprise) { create(:distributor_enterprise) } - let!(:product) { create(:simple_product) } - let!(:order_cycle) { create(:simple_order_cycle, distributors: [enterprise], variants: [product.variants.first]) } - - before do - Delayed::Job.destroy_all - quick_login_as_admin - - # This test relies on preference persistence, so we'll turn it on for this spec only. - # It will be turned off again automatically by reset_spree_preferences in spec_helper. - Spree::Preferences::Store.instance.persistence = true - end - - it "refreshes the cache when I change what products appear on my shopfront" do - # Given a product that's not in my inventory, but is in an active order cycle - - # When I change which products appear on the shopfront - visit edit_admin_enterprise_path(enterprise) - within(".side_menu") { click_link 'Inventory Settings' } - choose 'enterprise_preferred_product_selection_from_inventory_only_1' - - # Then a job should have been enqueued to refresh the cache - expect do - click_button 'Update' - end.to enqueue_job RefreshProductsCacheJob, distributor_id: enterprise.id, order_cycle_id: order_cycle.id - end - end - context "as an Enterprise user", js: true do let(:supplier1) { create(:supplier_enterprise, name: 'First Supplier') } let(:supplier2) { create(:supplier_enterprise, name: 'Another Supplier') } diff --git a/spec/features/consumer/shopping/checkout_spec.rb b/spec/features/consumer/shopping/checkout_spec.rb index 8f1d8e945d4..61a89bc6adc 100644 --- a/spec/features/consumer/shopping/checkout_spec.rb +++ b/spec/features/consumer/shopping/checkout_spec.rb @@ -180,46 +180,6 @@ end end - context "in the shopfront with cache enabled" do - around do |example| - original_config = Spree::Config[:enable_products_cache?] - example.run - Spree::Config[:enable_products_cache?] = original_config - end - - let(:control_product) { create(:taxed_product, supplier: supplier, price: 110, zone: zone, tax_rate_amount: 0.1) } - let(:control_variant) { control_product.variants.first } - let!(:order_cycle) { create(:simple_order_cycle, suppliers: [supplier], distributors: [distributor], coordinator: create(:distributor_enterprise), variants: [variant, control_variant]) } - - it "does not show item after all stock of an item is checked out (tesging cache update on checkout)" do - Spree::Config[:enable_products_cache?] = true - variant.update_attributes on_hand: 5 - flush_jobs - visit shop_path - - fill_in "variants[#{variant.id}]", with: '5' - wait_until { !cart_dirty } - - visit checkout_path - checkout_as_guest - - fill_out_details - fill_out_billing_address - choose free_shipping.name - choose check_without_fee.name - - place_order - expect(page).to have_content "Your order has been processed successfully" - - flush_jobs - visit shop_path - # The presence of the control product ensures the list of products is fully loaded - # before we verify the sold product is not present - expect(page).to have_content control_product.name - expect(page).not_to have_content product.name - end - end - context "on the checkout page" do before do visit checkout_path diff --git a/spec/jobs/products_cache_integrity_checker_job_spec.rb b/spec/jobs/products_cache_integrity_checker_job_spec.rb deleted file mode 100644 index 19d5a79953c..00000000000 --- a/spec/jobs/products_cache_integrity_checker_job_spec.rb +++ /dev/null @@ -1,27 +0,0 @@ -require 'spec_helper' -require 'open_food_network/products_renderer' - -describe ProductsCacheIntegrityCheckerJob do - describe "reporting on differences between the products cache and the current products" do - let(:distributor) { create(:distributor_enterprise) } - let(:order_cycle) { create(:simple_order_cycle) } - let(:job) { ProductsCacheIntegrityCheckerJob.new distributor.id, order_cycle.id } - let(:cache_key) { "products-json-#{distributor.id}-#{order_cycle.id}" } - - before do - Rails.cache.write(cache_key, "[1, 2, 3]\n") - allow(OpenFoodNetwork::ProductsRenderer).to receive(:new) { double(:pr, products_json: "[1, 3]\n") } - end - - it "reports errors" do - expect(Bugsnag).to receive(:notify) - run_job job - end - - it "deals with nil cached_json" do - Rails.cache.delete(cache_key) - expect(Bugsnag).to receive(:notify) - run_job job - end - end -end diff --git a/spec/jobs/refresh_products_cache_job_spec.rb b/spec/jobs/refresh_products_cache_job_spec.rb deleted file mode 100644 index c2dd66ff066..00000000000 --- a/spec/jobs/refresh_products_cache_job_spec.rb +++ /dev/null @@ -1,53 +0,0 @@ -require 'spec_helper' -require 'open_food_network/products_renderer' - -describe RefreshProductsCacheJob do - let(:distributor) { create(:distributor_enterprise) } - let(:order_cycle) { create(:simple_order_cycle) } - - context 'when the enterprise and the order cycle exist' do - before do - refresh_products_cache_job = instance_double(OpenFoodNetwork::ProductsRenderer, products_json: 'products') - allow(OpenFoodNetwork::ProductsRenderer).to receive(:new).with(distributor, order_cycle) { refresh_products_cache_job } - end - - it 'renders products and writes them to cache' do - run_job RefreshProductsCacheJob.new distributor.id, order_cycle.id - expect(Rails.cache.read("products-json-#{distributor.id}-#{order_cycle.id}")).to eq 'products' - end - end - - context 'when the order cycle does not exist' do - before do - allow(OrderCycle) - .to receive(:find) - .with(order_cycle.id) - .and_raise(ActiveRecord::RecordNotFound) - end - - it 'does not raise' do - expect { - run_job RefreshProductsCacheJob.new(distributor.id, order_cycle.id) - }.not_to raise_error - end - - it 'returns true' do - refresh_products_cache_job = RefreshProductsCacheJob.new(distributor.id, order_cycle.id) - expect(refresh_products_cache_job.perform).to eq(true) - end - end - - describe "fetching products JSON" do - let(:job) { RefreshProductsCacheJob.new(distributor.id, order_cycle.id) } - let(:products_renderer) { instance_double(OpenFoodNetwork::ProductsRenderer, products_json: nil) } - - before do - allow(OpenFoodNetwork::ProductsRenderer).to receive(:new).with(distributor, order_cycle) { products_renderer } - end - - it "fetches products JSON" do - job.perform - expect(OpenFoodNetwork::ProductsRenderer).to have_received(:new).with(distributor, order_cycle) { products_renderer } - end - end -end diff --git a/spec/lib/open_food_network/cached_products_renderer_spec.rb b/spec/lib/open_food_network/cached_products_renderer_spec.rb deleted file mode 100644 index 82d9302016b..00000000000 --- a/spec/lib/open_food_network/cached_products_renderer_spec.rb +++ /dev/null @@ -1,134 +0,0 @@ -require 'spec_helper' -require 'open_food_network/cached_products_renderer' - -module OpenFoodNetwork - describe CachedProductsRenderer do - let(:distributor) { double(:distributor, id: 123) } - let(:order_cycle) { double(:order_cycle, id: 456) } - let(:cached_products_renderer) { CachedProductsRenderer.new(distributor, order_cycle) } - - # keeps global state unchanged - around do |example| - original_config = Spree::Config[:enable_products_cache?] - example.run - Spree::Config[:enable_products_cache?] = original_config - end - - describe "#products_json" do - let(:products_renderer) do - double(ProductsRenderer, products_json: 'uncached products') - end - - before do - allow(ProductsRenderer) - .to receive(:new) - .with(distributor, order_cycle) { products_renderer } - end - - context "products cache toggle" do - before do - Rails.cache.write "products-json-#{distributor.id}-#{order_cycle.id}", 'products' - end - - context "disabled" do - before do - Spree::Config[:enable_products_cache?] = false - end - - it "returns uncached products JSON" do - expect(cached_products_renderer.products_json).to eq 'uncached products' - end - end - - context "enabled" do - before do - Spree::Config[:enable_products_cache?] = true - end - - it "returns the cached JSON" do - expect(cached_products_renderer.products_json).to eq 'products' - end - end - end - - context "products cache enabled" do - before do - Spree::Config[:enable_products_cache?] = true - end - - describe "when the distribution is not set" do - let(:cached_products_renderer) { CachedProductsRenderer.new(nil, nil) } - - it "raises an exception and returns no products" do - expect { cached_products_renderer.products_json }.to raise_error CachedProductsRenderer::NoProducts - end - end - - describe "when the products JSON is already cached" do - before do - Rails.cache.write "products-json-#{distributor.id}-#{order_cycle.id}", 'products' - end - - it "returns the cached JSON" do - expect(cached_products_renderer.products_json).to eq 'products' - end - - it "raises an exception when there are no products" do - Rails.cache.write "products-json-#{distributor.id}-#{order_cycle.id}", nil - expect { cached_products_renderer.products_json }.to raise_error CachedProductsRenderer::NoProducts - end - end - - describe "when the products JSON is not cached" do - let(:cache_key) { "products-json-#{distributor.id}-#{order_cycle.id}" } - let(:cached_json) { Rails.cache.read(cache_key) } - let(:cache_present) { Rails.cache.exist?(cache_key) } - let(:products_renderer) do - double(ProductsRenderer, products_json: 'fresh products') - end - - before do - Rails.cache.delete(cache_key) - - allow(ProductsRenderer) - .to receive(:new) - .with(distributor, order_cycle) { products_renderer } - end - - describe "when there are products" do - it "returns products as JSON" do - expect(cached_products_renderer.products_json).to eq 'fresh products' - end - - it "caches the JSON" do - cached_products_renderer.products_json - expect(cached_json).to eq 'fresh products' - end - end - - describe "when there are no products" do - let(:products_renderer) { double(ProductsRenderer) } - - before do - allow(products_renderer).to receive(:products_json).and_raise ProductsRenderer::NoProducts - - allow(ProductsRenderer) - .to receive(:new) - .with(distributor, order_cycle) { products_renderer } - end - - it "raises an error" do - expect { cached_products_renderer.products_json }.to raise_error CachedProductsRenderer::NoProducts - end - - it "caches the products as nil" do - expect { cached_products_renderer.products_json }.to raise_error CachedProductsRenderer::NoProducts - expect(cache_present).to be - expect(cached_json).to be_nil - end - end - end - end - end - end -end diff --git a/spec/lib/open_food_network/products_cache_refreshment_spec.rb b/spec/lib/open_food_network/products_cache_refreshment_spec.rb deleted file mode 100644 index 74998928d5f..00000000000 --- a/spec/lib/open_food_network/products_cache_refreshment_spec.rb +++ /dev/null @@ -1,69 +0,0 @@ -require 'spec_helper' -require 'open_food_network/products_cache_refreshment' - -module OpenFoodNetwork - describe ProductsCacheRefreshment do - let(:distributor) { create(:distributor_enterprise) } - let(:order_cycle) { create(:simple_order_cycle) } - - before { Delayed::Job.destroy_all } - - describe "when there are no tasks enqueued" do - it "enqueues the task" do - expect do - ProductsCacheRefreshment.refresh distributor, order_cycle - end.to enqueue_job RefreshProductsCacheJob - end - - it "enqueues the job with a lower than default priority" do - ProductsCacheRefreshment.refresh distributor, order_cycle - job = Delayed::Job.last - expect(job.priority).to be > Delayed::Worker.default_priority - end - end - - describe "when there is an enqueued task, and it is running" do - before do - job = Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id - job.update_attributes! locked_by: 'asdf', locked_at: Time.now - end - - it "enqueues another task" do - expect do - ProductsCacheRefreshment.refresh distributor, order_cycle - end.to enqueue_job RefreshProductsCacheJob - end - end - - describe "when there are two enqueued tasks, and one is running" do - before do - job1 = Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id - job1.update_attributes! locked_by: 'asdf', locked_at: Time.now - job2 = Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id - end - - it "does not enqueue another task" do - expect do - ProductsCacheRefreshment.refresh distributor, order_cycle - end.not_to enqueue_job RefreshProductsCacheJob - end - end - - describe "enqueuing tasks with different distributions" do - let(:distributor2) { create(:distributor_enterprise) } - let(:order_cycle2) { create(:simple_order_cycle) } - - before do - job1 = Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id - job1.update_attributes! locked_by: 'asdf', locked_at: Time.now - job2 = Delayed::Job.enqueue RefreshProductsCacheJob.new distributor.id, order_cycle.id - end - - it "ignores tasks with differing distributions when choosing whether to enqueue a job" do - expect do - ProductsCacheRefreshment.refresh distributor2, order_cycle2 - end.to enqueue_job RefreshProductsCacheJob - end - end - end -end diff --git a/spec/lib/open_food_network/products_cache_spec.rb b/spec/lib/open_food_network/products_cache_spec.rb deleted file mode 100644 index 47d18fa8baf..00000000000 --- a/spec/lib/open_food_network/products_cache_spec.rb +++ /dev/null @@ -1,431 +0,0 @@ -require 'open_food_network/products_cache' -require 'spec_helper' - -module OpenFoodNetwork - describe ProductsCache do - describe "when a variant changes" do - let(:variant) { create(:variant) } - let(:variant_undistributed) { create(:variant) } - let(:supplier) { create(:supplier_enterprise) } - let(:coordinator) { create(:distributor_enterprise) } - let(:distributor) { create(:distributor_enterprise) } - let(:oc_undated) { create(:undated_order_cycle, distributors: [distributor], variants: [variant]) } - let(:oc_upcoming) { create(:upcoming_order_cycle, suppliers: [supplier], coordinator: coordinator, distributors: [distributor], variants: [variant]) } - let(:oc_open) { create(:open_order_cycle, distributors: [distributor], variants: [variant]) } - let(:oc_closed) { create(:closed_order_cycle, distributors: [distributor], variants: [variant]) } - - it "refreshes distributions with upcoming order cycles" do - oc_upcoming - expect(ProductsCache).to receive(:refresh_cache).with(distributor, oc_upcoming) - ProductsCache.variant_changed variant - end - - it "refreshes distributions with open order cycles" do - oc_open - expect(ProductsCache).to receive(:refresh_cache).with(distributor, oc_open) - ProductsCache.variant_changed variant - end - - it "does not refresh distributions with undated order cycles" do - oc_undated - expect(ProductsCache).not_to receive(:refresh_cache).with(distributor, oc_undated) - ProductsCache.variant_changed variant - end - - it "does not refresh distributions with closed order cycles" do - oc_closed - expect(ProductsCache).not_to receive(:refresh_cache).with(distributor, oc_closed) - ProductsCache.variant_changed variant - end - - it "limits refresh to outgoing exchanges" do - oc_upcoming - expect(ProductsCache).not_to receive(:refresh_cache).with(coordinator, oc_upcoming) - ProductsCache.variant_changed variant - end - - it "does not refresh distributions where the variant does not appear" do - oc_undated; oc_upcoming; oc_open; oc_closed - variant_undistributed - expect(ProductsCache).not_to receive(:refresh_cache) - ProductsCache.variant_changed variant_undistributed - end - end - - describe "when a variant is destroyed" do - let(:variant) { create(:variant) } - let(:distributor) { create(:distributor_enterprise) } - let!(:oc) { create(:open_order_cycle, distributors: [distributor], variants: [variant]) } - - it "refreshes the cache based on exchanges the variant was in before destruction" do - expect(ProductsCache).to receive(:refresh_cache).with(distributor, oc) - variant.destroy - end - - it "performs the cache refresh after the variant has been destroyed" do - expect(ProductsCache).to receive(:refresh_cache).with(distributor, oc) do - expect(Spree::Variant.where(id: variant.id)).to be_empty - end - - variant.destroy - end - end - - describe "when a product changes" do - let(:product) { create(:simple_product) } - let(:v1) { create(:variant, product: product) } - let(:v2) { create(:variant, product: product) } - let(:d1) { create(:distributor_enterprise) } - let(:d2) { create(:distributor_enterprise) } - let(:oc) { create(:open_order_cycle) } - let!(:ex1) { create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d1, variants: [v1]) } - let!(:ex2) { create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d2, variants: [v1, v2]) } - - before { product.reload } - - it "refreshes the distribution each variant appears in, once each" do - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once - expect(ProductsCache).to receive(:refresh_cache).with(d2, oc).once - ProductsCache.product_changed product - end - end - - describe "when a product is deleted" do - let(:product) { create(:simple_product) } - let(:variant) { create(:variant, product: product) } - let(:distributor) { create(:distributor_enterprise) } - let!(:oc) { create(:open_order_cycle, distributors: [distributor], variants: [variant]) } - - it "refreshes the cache based on exchanges the variant was in before destruction" do - expect(ProductsCache).to receive(:refresh_cache).with(distributor, oc) - product.destroy - end - - it "performs the cache refresh after the product has been removed from the order cycle" do - expect(ProductsCache).to receive(:refresh_cache).with(distributor, oc) do - expect(product.reload.deleted_at).not_to be_nil - end - - product.destroy - end - end - - describe "when a variant override changes" do - let(:variant) { create(:variant) } - let(:d1) { create(:distributor_enterprise) } - let(:d2) { create(:distributor_enterprise) } - let!(:vo) { create(:variant_override, variant: variant, hub: d1) } - let!(:oc) { create(:open_order_cycle, distributors: [d1, d2], variants: [variant]) } - - it "refreshes the distributions that the variant override affects" do - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once - ProductsCache.variant_override_changed vo - end - - it "does not refresh other distributors of the variant" do - expect(ProductsCache).to receive(:refresh_cache).with(d2, oc).never - ProductsCache.variant_override_changed vo - end - end - - describe "when a variant override is destroyed" do - let(:vo) { double(:variant_override) } - - it "performs the same refresh as a variant override change" do - expect(ProductsCache).to receive(:variant_override_changed).with(vo) - ProductsCache.variant_override_destroyed vo - end - end - - describe "when a producer property is changed" do - let(:s) { create(:supplier_enterprise) } - let(:pp) { s.producer_properties.last } - let(:product) { create(:simple_product, supplier: s) } - let(:v1) { create(:variant, product: product) } - let(:v2) { create(:variant, product: product) } - let(:v_deleted) { create(:variant, product: product) } - let(:d1) { create(:distributor_enterprise) } - let(:d2) { create(:distributor_enterprise) } - let(:d3) { create(:distributor_enterprise) } - let(:oc) { create(:open_order_cycle) } - let!(:ex1) { create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d1, variants: [v1]) } - let!(:ex2) { create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d2, variants: [v1, v2]) } - let!(:ex3) { create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d3, variants: [product.master, v_deleted]) } - - before do - v_deleted.deleted_at = Time.now - v_deleted.save - s.set_producer_property :organic, 'NASAA 12345' - end - - it "refreshes the distributions the supplied variants appear in" do - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once - expect(ProductsCache).to receive(:refresh_cache).with(d2, oc).once - ProductsCache.producer_property_changed pp - end - - it "doesn't respond to master or deleted variants" do - expect(ProductsCache).to receive(:refresh_cache).with(d3, oc).never - ProductsCache.producer_property_changed pp - end - end - - describe "when a producer property is destroyed" do - let(:producer_property) { double(:producer_property) } - - it "triggers the same update as a change to the producer property" do - expect(ProductsCache).to receive(:producer_property_changed).with(producer_property) - ProductsCache.producer_property_destroyed producer_property - end - end - - describe "when an order cycle is changed" do - let(:variant) { create(:variant) } - let(:s) { create(:supplier_enterprise) } - let(:c) { create(:distributor_enterprise) } - let(:d1) { create(:distributor_enterprise) } - let(:d2) { create(:distributor_enterprise) } - let!(:oc_open) { create(:open_order_cycle, suppliers: [s], coordinator: c, distributors: [d1, d2], variants: [variant]) } - let!(:oc_upcoming) { create(:upcoming_order_cycle, suppliers: [s], coordinator: c, distributors: [d1, d2], variants: [variant]) } - - before do - oc_open.reload - oc_upcoming.reload - end - - it "updates each outgoing distribution in an upcoming order cycle" do - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc_upcoming).once - expect(ProductsCache).to receive(:refresh_cache).with(d2, oc_upcoming).once - ProductsCache.order_cycle_changed oc_upcoming - end - - it "updates each outgoing distribution in an open order cycle" do - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc_open).once - expect(ProductsCache).to receive(:refresh_cache).with(d2, oc_open).once - ProductsCache.order_cycle_changed oc_open - end - - it "does nothing when the order cycle has been made undated" do - expect(ProductsCache).to receive(:refresh_cache).never - oc_open.orders_open_at = oc_open.orders_close_at = nil - oc_open.save! - end - - it "does nothing when the order cycle has been closed" do - expect(ProductsCache).to receive(:refresh_cache).never - oc_open.orders_open_at = 2.weeks.ago - oc_open.orders_close_at = 1.week.ago - oc_open.save! - end - - it "does not update incoming exchanges" do - expect(ProductsCache).to receive(:refresh_cache).with(c, oc_open).never - ProductsCache.order_cycle_changed oc_open - end - end - - describe "when an exchange is changed" do - let(:s) { create(:supplier_enterprise) } - let(:c) { create(:distributor_enterprise) } - let(:d1) { create(:distributor_enterprise) } - let(:d2) { create(:distributor_enterprise) } - let(:v) { create(:variant) } - let(:oc) { create(:open_order_cycle, coordinator: c) } - - describe "incoming exchanges" do - let!(:ex1) { create(:exchange, order_cycle: oc, sender: s, receiver: c, incoming: true, variants: [v]) } - let!(:ex2) { create(:exchange, order_cycle: oc, sender: c, receiver: d1, incoming: false, variants: [v]) } - let!(:ex3) { create(:exchange, order_cycle: oc, sender: c, receiver: d2, incoming: false, variants: []) } - - before { oc.reload } - - it "updates distributions that include one of the supplier's variants" do - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once - ProductsCache.exchange_changed ex1 - end - - it "doesn't update distributions that don't include any of the supplier's variants" do - expect(ProductsCache).to receive(:refresh_cache).with(d2, oc).never - ProductsCache.exchange_changed ex1 - end - end - - describe "outgoing exchanges" do - let!(:ex) { create(:exchange, order_cycle: oc, sender: c, receiver: d1, incoming: false) } - - it "does not update for undated order cycles" do - oc.update_attributes! orders_open_at: nil, orders_close_at: nil - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).never - ProductsCache.exchange_changed ex - end - - it "updates for upcoming order cycles" do - oc.update_attributes! orders_open_at: 1.week.from_now, orders_close_at: 2.weeks.from_now - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once - ProductsCache.exchange_changed ex - end - - it "updates for open order cycles" do - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once - ProductsCache.exchange_changed ex - end - - it "does not update for closed order cycles" do - oc.update_attributes! orders_open_at: 2.weeks.ago, orders_close_at: 1.week.ago - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).never - ProductsCache.exchange_changed ex - end - end - end - - describe "when an exchange is destroyed" do - let(:exchange) { double(:exchange) } - - it "triggers the same update as a change to the exchange" do - expect(ProductsCache).to receive(:exchange_changed).with(exchange) - ProductsCache.exchange_destroyed exchange - end - end - - describe "when an enterprise fee is changed" do - let(:s) { create(:supplier_enterprise) } - let(:c) { create(:distributor_enterprise) } - let(:d1) { create(:distributor_enterprise) } - let(:d2) { create(:distributor_enterprise) } - let(:ef) { create(:enterprise_fee) } - let(:ef_coord) { create(:enterprise_fee, order_cycles: [oc]) } - let(:oc) { create(:open_order_cycle, coordinator: c) } - - describe "updating exchanges when it's a supplier fee" do - let(:v) { create(:variant) } - let!(:ex1) { create(:exchange, order_cycle: oc, sender: s, receiver: c, incoming: true, variants: [v], enterprise_fees: [ef]) } - let!(:ex2) { create(:exchange, order_cycle: oc, sender: c, receiver: d1, incoming: false, variants: [v]) } - let!(:ex3) { create(:exchange, order_cycle: oc, sender: c, receiver: d2, incoming: false, variants: []) } - - before { ef.reload } - - describe "updating distributions that include one of the supplier's variants" do - it "does not update undated order cycles" do - oc.update_attributes! orders_open_at: nil, orders_close_at: nil - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).never - ProductsCache.enterprise_fee_changed ef - end - - it "updates upcoming order cycles" do - oc.update_attributes! orders_open_at: 1.week.from_now, orders_close_at: 2.weeks.from_now - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once - ProductsCache.enterprise_fee_changed ef - end - - it "updates open order cycles" do - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once - ProductsCache.enterprise_fee_changed ef - end - - it "does not update closed order cycles" do - oc.update_attributes! orders_open_at: 2.weeks.ago, orders_close_at: 1.week.ago - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).never - ProductsCache.enterprise_fee_changed ef - end - end - - it "doesn't update distributions that don't include any of the supplier's variants" do - expect(ProductsCache).to receive(:refresh_cache).with(d2, oc).never - ProductsCache.enterprise_fee_changed ef - end - end - - it "updates order cycles when it's a coordinator fee" do - ef_coord - expect(ProductsCache).to receive(:order_cycle_changed).with(oc).once - ProductsCache.enterprise_fee_changed ef_coord - end - - describe "updating exchanges when it's a distributor fee" do - let(:ex0) { create(:exchange, order_cycle: oc, sender: s, receiver: c, incoming: true, enterprise_fees: [ef]) } - let(:ex1) { create(:exchange, order_cycle: oc, sender: c, receiver: d1, incoming: false, enterprise_fees: [ef]) } - let(:ex2) { create(:exchange, order_cycle: oc, sender: c, receiver: d2, incoming: false, enterprise_fees: []) } - - describe "updating distributions that include the fee" do - it "does not update undated order cycles" do - oc.update_attributes! orders_open_at: nil, orders_close_at: nil - ex1 - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).never - ProductsCache.enterprise_fee_changed ef - end - - it "updates upcoming order cycles" do - oc.update_attributes! orders_open_at: 1.week.from_now, orders_close_at: 2.weeks.from_now - ex1 - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once - ProductsCache.enterprise_fee_changed ef - end - - it "updates open order cycles" do - ex1 - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).once - ProductsCache.enterprise_fee_changed ef - end - - it "does not update closed order cycles" do - oc.update_attributes! orders_open_at: 2.weeks.ago, orders_close_at: 1.week.ago - ex1 - expect(ProductsCache).to receive(:refresh_cache).with(d1, oc).never - ProductsCache.enterprise_fee_changed ef - end - end - - it "doesn't update exchanges that don't include the fee" do - ex1; ex2 - expect(ProductsCache).to receive(:refresh_cache).with(d2, oc).never - ProductsCache.enterprise_fee_changed ef - end - - it "doesn't update incoming exchanges" do - ex0 - expect(ProductsCache).to receive(:refresh_cache).with(c, oc).never - ProductsCache.enterprise_fee_changed ef - end - end - end - - describe "when a distributor enterprise is changed" do - let(:d) { create(:distributor_enterprise) } - let(:oc) { create(:open_order_cycle, distributors: [d]) } - - it "updates each distribution the enterprise is active in" do - expect(ProductsCache).to receive(:refresh_cache).with(d, oc) - ProductsCache.distributor_changed d - end - end - - describe "when an inventory item is changed" do - let!(:d) { create(:distributor_enterprise) } - let!(:v) { create(:variant) } - let!(:oc1) { create(:open_order_cycle, distributors: [d], variants: [v]) } - let(:oc2) { create(:open_order_cycle, distributors: [d], variants: []) } - let!(:ii) { create(:inventory_item, enterprise: d, variant: v) } - - it "updates each distribution for that enterprise+variant" do - expect(ProductsCache).to receive(:refresh_cache).with(d, oc1) - ProductsCache.inventory_item_changed ii - end - - it "doesn't update distributions that don't feature the variant" do - oc2 - expect(ProductsCache).to receive(:refresh_cache).with(d, oc2).never - ProductsCache.inventory_item_changed ii - end - end - - describe "refreshing the cache" do - let(:distributor) { double(:distributor) } - let(:order_cycle) { double(:order_cycle) } - - it "notifies ProductsCacheRefreshment" do - expect(ProductsCacheRefreshment).to receive(:refresh).with(distributor, order_cycle) - ProductsCache.send(:refresh_cache, distributor, order_cycle) - end - end - end -end diff --git a/spec/models/coordinator_fee_spec.rb b/spec/models/coordinator_fee_spec.rb deleted file mode 100644 index 2a54a0e96ce..00000000000 --- a/spec/models/coordinator_fee_spec.rb +++ /dev/null @@ -1,19 +0,0 @@ -require 'spec_helper' - -describe CoordinatorFee do - describe "products caching" do - let(:order_cycle) { create(:simple_order_cycle) } - let(:enterprise_fee) { create(:enterprise_fee) } - - it "refreshes the products cache on change" do - expect(OpenFoodNetwork::ProductsCache).to receive(:order_cycle_changed).with(order_cycle) - order_cycle.coordinator_fees << enterprise_fee - end - - it "refreshes the products cache on destruction" do - order_cycle.coordinator_fees << enterprise_fee - expect(OpenFoodNetwork::ProductsCache).to receive(:order_cycle_changed).with(order_cycle) - order_cycle.coordinator_fee_refs.first.destroy - end - end -end diff --git a/spec/models/enterprise_fee_spec.rb b/spec/models/enterprise_fee_spec.rb index c36b1003323..4c09ccd62cc 100644 --- a/spec/models/enterprise_fee_spec.rb +++ b/spec/models/enterprise_fee_spec.rb @@ -12,12 +12,6 @@ describe "callbacks" do let(:ef) { create(:enterprise_fee) } - it "refreshes the products cache when saved" do - expect(OpenFoodNetwork::ProductsCache).to receive(:enterprise_fee_changed).with(ef) - ef.name = 'foo' - ef.save - end - it "removes itself from order cycle coordinator fees when destroyed" do oc = create(:simple_order_cycle, coordinator_fees: [ef]) diff --git a/spec/models/exchange_fee_spec.rb b/spec/models/exchange_fee_spec.rb deleted file mode 100644 index 12bcd0c5e37..00000000000 --- a/spec/models/exchange_fee_spec.rb +++ /dev/null @@ -1,19 +0,0 @@ -require 'spec_helper' - -describe ExchangeFee do - describe "products caching" do - let(:exchange) { create(:exchange) } - let(:enterprise_fee) { create(:enterprise_fee) } - - it "refreshes the products cache on change" do - expect(OpenFoodNetwork::ProductsCache).to receive(:exchange_changed).with(exchange) - exchange.enterprise_fees << enterprise_fee - end - - it "refreshes the products cache on destruction" do - exchange.enterprise_fees << enterprise_fee - expect(OpenFoodNetwork::ProductsCache).to receive(:exchange_changed).with(exchange) - exchange.reload.exchange_fees.destroy_all - end - end -end diff --git a/spec/models/exchange_spec.rb b/spec/models/exchange_spec.rb index f7add7ae471..bb344044692 100644 --- a/spec/models/exchange_spec.rb +++ b/spec/models/exchange_spec.rb @@ -91,21 +91,6 @@ end end - describe "products caching" do - let!(:exchange) { create(:exchange) } - - it "refreshes the products cache on change" do - expect(OpenFoodNetwork::ProductsCache).to receive(:exchange_changed).with(exchange) - exchange.pickup_time = 'asdf' - exchange.save - end - - it "refreshes the products cache on destruction" do - expect(OpenFoodNetwork::ProductsCache).to receive(:exchange_destroyed).with(exchange) - exchange.destroy - end - end - describe "scopes" do let(:supplier) { create(:supplier_enterprise) } let(:coordinator) { create(:distributor_enterprise, is_primary_producer: true) } diff --git a/spec/models/inventory_item_spec.rb b/spec/models/inventory_item_spec.rb deleted file mode 100644 index 99a16d3fdbb..00000000000 --- a/spec/models/inventory_item_spec.rb +++ /dev/null @@ -1,16 +0,0 @@ -require 'spec_helper' -require 'open_food_network/products_cache' - -describe InventoryItem do - describe "caching" do - let(:ii) { create(:inventory_item) } - - it "refreshes the products cache on save" do - expect(OpenFoodNetwork::ProductsCache).to receive(:inventory_item_changed).with(ii) - ii.visible = false - ii.save - end - - # Inventory items are not destroyed - end -end diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index fef465a81f6..42fc8759119 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -27,18 +27,6 @@ oc.save! end - describe "products cache" do - let(:oc) { create(:open_order_cycle) } - - it "refreshes the products cache on save" do - expect(OpenFoodNetwork::ProductsCache).to receive(:order_cycle_changed).with(oc) - oc.name = 'asdf' - oc.save - end - - # On destroy, we're removing distributions, so no updates to the products cache are required - end - it "has exchanges" do oc = create(:simple_order_cycle) diff --git a/spec/models/producer_property_spec.rb b/spec/models/producer_property_spec.rb index 66be85b985a..a39d5fd609f 100644 --- a/spec/models/producer_property_spec.rb +++ b/spec/models/producer_property_spec.rb @@ -71,17 +71,4 @@ end end end - - describe "products caching" do - it "refreshes the products cache on change" do - expect(OpenFoodNetwork::ProductsCache).to receive(:producer_property_changed).with(pp) - pp.value = 123 - pp.save - end - - it "refreshes the products cache on destruction" do - expect(OpenFoodNetwork::ProductsCache).to receive(:producer_property_destroyed).with(pp) - pp.destroy - end - end end diff --git a/spec/models/spree/classification_spec.rb b/spec/models/spree/classification_spec.rb index c93825aeaa2..4d0f089e567 100644 --- a/spec/models/spree/classification_spec.rb +++ b/spec/models/spree/classification_spec.rb @@ -11,18 +11,5 @@ module Spree expect(classification.destroy).to be false expect(classification.errors.messages[:base]).to eq(["Taxon #{taxon.name} is the primary taxon of #{product.name} and cannot be deleted"]) end - - describe "callbacks" do - it "refreshes the products cache on save" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) - classification - end - - it "refreshes the products cache on destroy" do - classification - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) - classification.destroy - end - end end end diff --git a/spec/models/spree/image_spec.rb b/spec/models/spree/image_spec.rb index ee256cf7cb8..1ad60cbe573 100644 --- a/spec/models/spree/image_spec.rb +++ b/spec/models/spree/image_spec.rb @@ -14,23 +14,5 @@ module Spree expect(Image.format_styles(formatted)).to eq(mini: ["48x48>", :png]) end end - - describe "callbacks" do - let!(:product) { create(:simple_product) } - - let!(:image_file) { File.open("#{Rails.root}/app/assets/images/logo-white.png") } - let!(:image) { Image.create(viewable_id: product.master.id, viewable_type: 'Spree::Variant', alt: "image", attachment: image_file) } - - it "refreshes the products cache when changed" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) - image.alt = 'asdf' - image.save - end - - it "refreshes the products cache when destroyed" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) - image.destroy - end - end end end diff --git a/spec/models/spree/option_type_spec.rb b/spec/models/spree/option_type_spec.rb deleted file mode 100644 index cfc06547335..00000000000 --- a/spec/models/spree/option_type_spec.rb +++ /dev/null @@ -1,28 +0,0 @@ -require 'spec_helper' - -module Spree - describe OptionType do - describe "products cache" do - let!(:product) { create(:simple_product, option_types: [option_type]) } - let(:variant) { product.variants.first } - let(:option_type) { create(:option_type) } - let(:option_value) { create(:option_value, option_type: option_type) } - - before do - option_type.reload - variant.option_values << option_value - end - - it "refreshes the products cache on change, via product" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) - option_type.name = 'foo' - option_type.save! - end - - it "refreshes the products cache on destruction, via option value destruction" do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) - option_type.destroy - end - end - end -end diff --git a/spec/models/spree/option_value_spec.rb b/spec/models/spree/option_value_spec.rb deleted file mode 100644 index f784e08af26..00000000000 --- a/spec/models/spree/option_value_spec.rb +++ /dev/null @@ -1,26 +0,0 @@ -require 'spec_helper' - -module Spree - describe OptionValue do - describe "products cache" do - let(:variant) { create(:variant) } - let(:option_value) { create(:option_value) } - - before do - variant.option_values << option_value - option_value.reload - end - - it "refreshes the products cache on change, via variant" do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) - option_value.name = 'foo' - option_value.save! - end - - it "refreshes the products cache on destruction, via variant" do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) - option_value.destroy - end - end - end -end diff --git a/spec/models/spree/preference_spec.rb b/spec/models/spree/preference_spec.rb deleted file mode 100644 index e60080f95dc..00000000000 --- a/spec/models/spree/preference_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -require 'spec_helper' - -module Spree - describe Preference do - describe "refreshing the products cache" do - it "reports when product_selection_from_inventory_only has changed" do - p = Preference.new(key: 'enterprise/product_selection_from_inventory_only/123') - expect(p.send(:product_selection_from_inventory_only_changed?)).to be true - end - - it "reports when product_selection_from_inventory_only has not changed" do - p = Preference.new(key: 'enterprise/shopfront_message/123') - expect(p.send(:product_selection_from_inventory_only_changed?)).to be false - end - - it "looks up the referenced enterprise" do - e = create(:distributor_enterprise) - p = Preference.new(key: "enterprise/product_selection_from_inventory_only/#{e.id}") - expect(p.send(:enterprise)).to eql e - end - end - end -end diff --git a/spec/models/spree/price_spec.rb b/spec/models/spree/price_spec.rb index 45af706c8da..8bf76d39327 100644 --- a/spec/models/spree/price_spec.rb +++ b/spec/models/spree/price_spec.rb @@ -5,23 +5,6 @@ module Spree let(:variant) { create(:variant) } let(:price) { variant.default_price } - describe "callbacks" do - it "refreshes the products cache on change" do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) - price.amount = 123 - price.save - end - - # Do not refresh on price destruction - this (only?) happens when variant is destroyed, - # and in that case the variant will take responsibility for refreshing the cache - - it "does not refresh the cache when variant is not set" do - # Creates a price without the back link to variant - create(:product, master: create(:variant)) - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).never - end - end - context "when variant is soft-deleted" do before do variant.destroy diff --git a/spec/models/spree/product_property_spec.rb b/spec/models/spree/product_property_spec.rb deleted file mode 100644 index 1c9799ab296..00000000000 --- a/spec/models/spree/product_property_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -require 'spec_helper' - -module Spree - describe ProductProperty do - describe "callbacks" do - let(:product) { product_property.product } - let(:product_property) { create(:product_property) } - - it "refreshes the products cache on save, via Product" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) - product_property.value = 123 - product_property.save - end - - it "refreshes the products cache on destroy" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) - product_property.destroy - end - end - end -end diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 2c3b7064828..a8c6368a70e 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -172,20 +172,6 @@ module Spree describe "callbacks" do let(:product) { create(:simple_product) } - it "refreshes the products cache on save" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) - product.name = 'asdf' - product.save - end - - it "refreshes the products cache on delete" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_deleted).with(product) - product.destroy - end - - # On destroy, all distributed variants are refreshed by a Variant around_destroy - # callback, so we don't need to do anything on the product model. - describe "touching affected enterprises when the product is deleted" do let(:product) { create(:simple_product) } let(:supplier) { product.supplier } diff --git a/spec/models/spree/property_spec.rb b/spec/models/spree/property_spec.rb index 509b98cc1fa..a155cbfa455 100644 --- a/spec/models/spree/property_spec.rb +++ b/spec/models/spree/property_spec.rb @@ -95,17 +95,5 @@ module Spree end end end - - describe "callbacks" do - let(:property) { product_property.property } - let(:product) { product_property.product } - let(:product_property) { create(:product_property) } - - it "refreshes the products cache on save" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) - property.name = 'asdf' - property.save - end - end end end diff --git a/spec/models/spree/taxon_spec.rb b/spec/models/spree/taxon_spec.rb index 5c52dd0173f..5a565c51460 100644 --- a/spec/models/spree/taxon_spec.rb +++ b/spec/models/spree/taxon_spec.rb @@ -6,21 +6,6 @@ module Spree let!(:t1) { create(:taxon) } let!(:t2) { create(:taxon) } - describe "callbacks" do - let!(:p2) { create(:simple_product, taxons: [t1], primary_taxon: t2) } - - it "refreshes the products cache on save" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(p2) - t1.name = 'asdf' - t1.save - end - - it "refreshes the products cache on destroy" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(p2) - t1.destroy - end - end - describe "finding all supplied taxons" do let!(:p1) { create(:simple_product, supplier: e, taxons: [t1, t2]) } diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index c59b0795b1f..39ac4bcead9 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -1,6 +1,5 @@ require 'spec_helper' require 'open_food_network/option_value_namer' -require 'open_food_network/products_cache' module Spree describe Variant do @@ -165,39 +164,6 @@ module Spree end end - describe "callbacks" do - let(:variant) { create(:variant) } - - it "refreshes the products cache on save" do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).with(variant) - variant.sku = 'abc123' - variant.save - end - - it "refreshes the products cache on destroy" do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) - variant.destroy - end - - context "when it is the master variant" do - let(:product) { create(:simple_product) } - let(:master) { product.master } - - it "refreshes the products cache for the entire product on save" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_changed).never - master.sku = 'abc123' - master.save - end - - it "refreshes the products cache for the entire product on destroy" do - expect(OpenFoodNetwork::ProductsCache).to receive(:product_changed).with(product) - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).never - master.destroy - end - end - end - describe "indexing variants by id" do let!(:v1) { create(:variant) } let!(:v2) { create(:variant) } diff --git a/spec/models/variant_override_spec.rb b/spec/models/variant_override_spec.rb index ecc2aff73e4..4c5e5e71afc 100644 --- a/spec/models/variant_override_spec.rb +++ b/spec/models/variant_override_spec.rb @@ -113,21 +113,6 @@ end end - describe "callbacks" do - let!(:vo) { create(:variant_override, hub: hub, variant: variant) } - - it "refreshes the products cache on save" do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_override_changed).with(vo) - vo.price = 123.45 - vo.save - end - - it "refreshes the products cache on destroy" do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_override_destroyed).with(vo) - vo.destroy - end - end - describe "delegated price" do let!(:variant_with_price) { create(:variant, price: 123.45) } let(:price_object) { variant_with_price.default_price } From c54cff10d4214978645fbe9be6dbecf942a12423 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 4 Oct 2019 18:24:46 +0100 Subject: [PATCH 10/25] Adjust API endpoint params --- .../darkswarm/services/products.js.coffee | 5 +++-- .../api/order_cycles_controller.rb | 8 +++++-- lib/open_food_network/products_renderer.rb | 2 +- .../services/products_spec.js.coffee | 21 ++++++++++--------- 4 files changed, 21 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/darkswarm/services/products.js.coffee b/app/assets/javascripts/darkswarm/services/products.js.coffee index 9b7b5e59930..59bd19efb4d 100644 --- a/app/assets/javascripts/darkswarm/services/products.js.coffee +++ b/app/assets/javascripts/darkswarm/services/products.js.coffee @@ -1,4 +1,4 @@ -Darkswarm.factory 'Products', (OrderCycleResource, OrderCycle, Shopfront, Dereferencer, Taxons, Properties, Cart, Variants) -> +Darkswarm.factory 'Products', (OrderCycleResource, OrderCycle, Shopfront, currentHub, Dereferencer, Taxons, Properties, Cart, Variants) -> new class Products constructor: -> @update() @@ -15,7 +15,8 @@ Darkswarm.factory 'Products', (OrderCycleResource, OrderCycle, Shopfront, Derefe @loading = false return - params['id'] = OrderCycle.order_cycle.order_cycle_id if params['id'] == undefined + params['id'] = order_cycle_id + params['distributor'] = currentHub.id OrderCycleResource.products params, (data)=> @products = [] unless load_more diff --git a/app/controllers/api/order_cycles_controller.rb b/app/controllers/api/order_cycles_controller.rb index 42eb6aa21e8..685d72176bc 100644 --- a/app/controllers/api/order_cycles_controller.rb +++ b/app/controllers/api/order_cycles_controller.rb @@ -8,8 +8,12 @@ class OrderCyclesController < BaseController skip_authorization_check def products - products = OpenFoodNetwork::ProductsRenderer.new(current_distributor, current_order_cycle, params).products_json - # products = ::ProductsFilterer.new(current_distributor, current_customer, products_json).call # TBD + products = OpenFoodNetwork::ProductsRenderer.new( + distributor, + order_cycle, + customer, + params + ).products_json render json: products rescue OpenFoodNetwork::ProductsRenderer::NoProducts diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb index bafbe27a593..48a35a1b117 100644 --- a/lib/open_food_network/products_renderer.rb +++ b/lib/open_food_network/products_renderer.rb @@ -12,7 +12,7 @@ def initialize(distributor, order_cycle, customer, params = {}) end def products_json - if products + if order_cycle && distributor && products enterprise_fee_calculator = EnterpriseFeeCalculator.new distributor, order_cycle ActiveModel::ArraySerializer.new(products, diff --git a/spec/javascripts/unit/darkswarm/services/products_spec.js.coffee b/spec/javascripts/unit/darkswarm/services/products_spec.js.coffee index e9d2111d83d..d1b9f38bb6d 100644 --- a/spec/javascripts/unit/darkswarm/services/products_spec.js.coffee +++ b/spec/javascripts/unit/darkswarm/services/products_spec.js.coffee @@ -12,6 +12,7 @@ describe 'Products service', -> properties = null taxons = null Geo = {} + endpoint = "/api/order_cycles/1/products?distributor=1" beforeEach -> product = @@ -62,60 +63,60 @@ describe 'Products service', -> $httpBackend = _$httpBackend_ it "Fetches products from the backend on init", -> - $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) + $httpBackend.expectGET(endpoint).respond([product]) $httpBackend.flush() expect(Products.products[0].test).toEqual "cats" it "dereferences suppliers", -> Shopfront.producers_by_id = {id: 9, name: "test"} - $httpBackend.expectGET("/api/order_cycles/1/products").respond([{supplier : {id: 9}, master: {}}]) + $httpBackend.expectGET(endpoint).respond([{supplier : {id: 9}, master: {}}]) $httpBackend.flush() expect(Products.products[0].supplier).toBe Shopfront.producers_by_id["9"] it "dereferences taxons", -> product.taxons = [2] - $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) + $httpBackend.expectGET(endpoint).respond([product]) $httpBackend.flush() expect(Products.products[0].taxons[1]).toBe taxons[0] it "dereferences properties", -> product.properties_with_values = [1] - $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) + $httpBackend.expectGET(endpoint).respond([product]) $httpBackend.flush() expect(Products.products[0].properties[1]).toBe properties[0] it "registers variants with Variants service", -> product.variants = [{id: 1}] - $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) + $httpBackend.expectGET(endpoint).respond([product]) $httpBackend.flush() expect(Products.products[0].variants[0]).toBe Variants.variants[1] it "stores variant names", -> product.variants = [{id: 1, name_to_display: "one"}, {id: 2, name_to_display: "two"}] - $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) + $httpBackend.expectGET(endpoint).respond([product]) $httpBackend.flush() expect(Products.products[0].variant_names).toEqual "one two " it "sets primaryImageOrMissing when no images are provided", -> - $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) + $httpBackend.expectGET(endpoint).respond([product]) $httpBackend.flush() expect(Products.products[0].primaryImage).toBeUndefined() expect(Products.products[0].primaryImageOrMissing).toEqual "/assets/noimage/small.png" it "sets largeImage", -> - $httpBackend.expectGET("/api/order_cycles/1/products").respond([productWithImage]) + $httpBackend.expectGET(endpoint).respond([productWithImage]) $httpBackend.flush() expect(Products.products[0].largeImage).toEqual("foo.png") describe "determining the price to display for a product", -> it "displays the product price when the product does not have variants", -> - $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) + $httpBackend.expectGET(endpoint).respond([product]) $httpBackend.flush() expect(Products.products[0].price).toEqual 11.00 it "displays the minimum variant price when the product has variants", -> product.variants = [{price: 22}, {price: 33}] - $httpBackend.expectGET("/api/order_cycles/1/products").respond([product]) + $httpBackend.expectGET(endpoint).respond([product]) $httpBackend.flush() expect(Products.products[0].price).toEqual 22 From dd6d0d25dac7e32b6b28f778a7677f1ed4db6bbc Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 4 Oct 2019 18:25:04 +0100 Subject: [PATCH 11/25] Fix problematic feature specs --- spec/features/consumer/shopping/products_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/features/consumer/shopping/products_spec.rb b/spec/features/consumer/shopping/products_spec.rb index 6282d0ba316..726346a9ed3 100644 --- a/spec/features/consumer/shopping/products_spec.rb +++ b/spec/features/consumer/shopping/products_spec.rb @@ -32,7 +32,9 @@ visit shop_path select "monday", from: "order_cycle_id" + expect(page).to have_content product.name click_link product.name + expect(page).to have_selector '.reveal-modal' modal_should_be_open_for product @@ -48,7 +50,9 @@ visit shop_path select "monday", from: "order_cycle_id" + expect(page).to have_content product.name click_link product.name + expect(page).to have_selector '.reveal-modal' modal_should_be_open_for product From 2539b84b3374f1d0a28739586c53e72a25759d4d Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 6 Oct 2019 22:30:56 +0100 Subject: [PATCH 12/25] Fix `product.meta_keywords` not searchable --- .../darkswarm/controllers/products_controller.js.coffee | 4 ++-- spec/features/consumer/shopping/shopping_spec.rb | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee b/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee index 36620b827e4..f5ae87dbe4a 100644 --- a/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee +++ b/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee @@ -53,7 +53,7 @@ Darkswarm.controller "ProductsCtrl", ($scope, $filter, $rootScope, Products, Ord id: $scope.order_cycle.order_cycle_id, page: $scope.page, per_page: $scope.per_page, - 'q[name_or_supplier_name_cont]': $scope.query, + 'q[name_or_meta_keywords_or_supplier_name_cont]': $scope.query, 'q[properites_in_any][]': $scope.activeProperties, 'q[primary_taxon_id_in_any][]': $scope.activeTaxons } @@ -64,7 +64,7 @@ Darkswarm.controller "ProductsCtrl", ($scope, $filter, $rootScope, Products, Ord id: $scope.order_cycle.order_cycle_id, page: $scope.page + 1, per_page: $scope.per_page, - 'q[name_or_supplier_name_cont]': $scope.query, + 'q[name_or_meta_keywords_or_supplier_name_cont]': $scope.query, 'q[properites_in_any][]': $scope.activeProperties, 'q[primary_taxon_id_in_any][]': $scope.activeTaxons } diff --git a/spec/features/consumer/shopping/shopping_spec.rb b/spec/features/consumer/shopping/shopping_spec.rb index 2809d8f1875..989eea5e4e5 100644 --- a/spec/features/consumer/shopping/shopping_spec.rb +++ b/spec/features/consumer/shopping/shopping_spec.rb @@ -214,10 +214,6 @@ fill_in "search", with: "Meer" # For product named "Meercats" expect(page).to have_content product2.name expect(page).not_to have_content product.name - - fill_in "search", with: "Dome" # For product with meta_keywords "Domestic" - expect(page).to have_content product.name - expect(page).not_to have_content product2.name end it "returns search results for products where the search term matches one of the product's variant names" do From c6ce51612986da99e7289822b223ea888752c599 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 6 Oct 2019 22:33:16 +0100 Subject: [PATCH 13/25] Fix prices not updating with new exchange fees when changing OC --- app/assets/javascripts/templates/shop_variant.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/templates/shop_variant.html.haml b/app/assets/javascripts/templates/shop_variant.html.haml index 3214612e880..71ca3c33ec4 100644 --- a/app/assets/javascripts/templates/shop_variant.html.haml +++ b/app/assets/javascripts/templates/shop_variant.html.haml @@ -17,7 +17,7 @@ .small-4.medium-2.large-2.columns.variant-price .table-cell.price %i.ofn-i_009-close - {{ ::variant.price_with_fees | localizeCurrency }} + {{ variant.price_with_fees | localizeCurrency }} -# Now in a template in app/assets/javascripts/templates ! %price-breakdown{"price-breakdown" => "_", variant: "variant", From 573a69477fe81fa0d3e23ce977bb27c3e0625187 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 7 Oct 2019 02:18:25 +0100 Subject: [PATCH 14/25] Fix filters not updating on OC change --- .../order_cycle_controller.js.coffee | 7 ++-- .../controllers/products_controller.js.coffee | 35 +++++++++---------- app/views/shop/products/_filters.html.haml | 8 ++--- 3 files changed, 24 insertions(+), 26 deletions(-) diff --git a/app/assets/javascripts/darkswarm/controllers/order_cycle_controller.js.coffee b/app/assets/javascripts/darkswarm/controllers/order_cycle_controller.js.coffee index 55e44d037ef..6547237c2db 100644 --- a/app/assets/javascripts/darkswarm/controllers/order_cycle_controller.js.coffee +++ b/app/assets/javascripts/darkswarm/controllers/order_cycle_controller.js.coffee @@ -1,4 +1,4 @@ -Darkswarm.controller "OrderCycleCtrl", ($scope, $timeout, OrderCycle) -> +Darkswarm.controller "OrderCycleCtrl", ($scope, $rootScope, $timeout, OrderCycle) -> $scope.order_cycle = OrderCycle.order_cycle $scope.OrderCycle = OrderCycle @@ -6,11 +6,12 @@ Darkswarm.controller "OrderCycleCtrl", ($scope, $timeout, OrderCycle) -> # This is a hack. We should probably write our own "popover" directive # That takes an expression instead of a trigger, and binds to that $timeout => + $rootScope.$broadcast 'orderCycleSelected' if !$scope.OrderCycle.selected() $("#order_cycle_id").trigger("openTrigger") -Darkswarm.controller "OrderCycleChangeCtrl", ($scope, $timeout, OrderCycle, Products, Variants, Cart, ChangeableOrdersAlert) -> +Darkswarm.controller "OrderCycleChangeCtrl", ($scope, $rootScope, $timeout, OrderCycle, Products, Variants, Cart, ChangeableOrdersAlert) -> # Track previous order cycle id for use with revertOrderCycle() $scope.previous_order_cycle_id = OrderCycle.order_cycle.order_cycle_id $scope.$watch 'order_cycle.order_cycle_id', (newValue, oldValue)-> @@ -32,4 +33,4 @@ Darkswarm.controller "OrderCycleChangeCtrl", ($scope, $timeout, OrderCycle, Prod Products.update() Cart.reloadFinalisedLineItems() ChangeableOrdersAlert.reload() - # Reload Filters from new endpoint after changing OC here + $rootScope.$broadcast 'orderCycleSelected' diff --git a/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee b/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee index f5ae87dbe4a..01048caf199 100644 --- a/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee +++ b/app/assets/javascripts/darkswarm/controllers/products_controller.js.coffee @@ -8,37 +8,34 @@ Darkswarm.controller "ProductsCtrl", ($scope, $filter, $rootScope, Products, Ord $scope.page = 1 $scope.per_page = 10 $scope.order_cycle = OrderCycle.order_cycle + $scope.supplied_taxons = {} + $scope.supplied_properties = {} - $scope.supplied_taxons = -> - return $scope.memoized_taxons if $scope.memoized_taxons != undefined - $scope.memoized_taxons = {} + $rootScope.$on "orderCycleSelected", -> + $scope.update_filters() + $scope.clearAll() - params = { - id: OrderCycle.order_cycle.order_cycle_id, - distributor: currentHub.id - } - OrderCycleResource.taxons params, (data)=> - data.map( (taxon) -> - $scope.memoized_taxons[taxon.id] = Taxons.taxons_by_id[taxon.id] - ) + $scope.update_filters = -> + order_cycle_id = OrderCycle.order_cycle.order_cycle_id - $scope.memoized_taxons + return unless order_cycle_id - $scope.supplied_properties = -> - return $scope.memoized_properties if $scope.memoized_properties != undefined - $scope.memoized_properties = {} + $scope.supplied_taxons = {} + $scope.supplied_properties = {} params = { - id: OrderCycle.order_cycle.order_cycle_id, + id: order_cycle_id, distributor: currentHub.id } + OrderCycleResource.taxons params, (data)=> + data.map( (taxon) -> + $scope.supplied_taxons[taxon.id] = Taxons.taxons_by_id[taxon.id] + ) OrderCycleResource.properties params, (data)=> data.map( (property) -> - $scope.memoized_properties[property.id] = Properties.properties_by_id[property.id] + $scope.supplied_properties[property.id] = Properties.properties_by_id[property.id] ) - $scope.memoized_properties - $scope.loadMore = -> if ($scope.page * $scope.per_page) <= Products.products.length $scope.loadMoreProducts() diff --git a/app/views/shop/products/_filters.html.haml b/app/views/shop/products/_filters.html.haml index 0f89c371665..8d7f03f16e0 100644 --- a/app/views/shop/products/_filters.html.haml +++ b/app/views/shop/products/_filters.html.haml @@ -1,5 +1,5 @@ -.filter-shopfront.taxon-selectors.text-right - %single-line-selectors{ selectors: "taxonSelectors", objects: "supplied_taxons()", "active-selectors" => "activeTaxons"} +.filter-shopfront.taxon-selectors.text-right{ng: {show: 'supplied_taxons && !Products.loading'}} + %single-line-selectors{ selectors: "taxonSelectors", objects: "supplied_taxons", "active-selectors" => "activeTaxons"} -.filter-shopfront.property-selectors.text-right - %single-line-selectors{ selectors: "propertySelectors", objects: "supplied_properties()", "active-selectors" => "activeProperties"} +.filter-shopfront.property-selectors.text-right{ng: {show: 'supplied_properties && !Products.loading'}} + %single-line-selectors{ selectors: "propertySelectors", objects: "supplied_properties", "active-selectors" => "activeProperties"} From f2affe80cde0db0cf9c87726579ea666ff3705b7 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 7 Oct 2019 10:13:21 +0100 Subject: [PATCH 15/25] Reduce assignment branching and complexity for ProductsRenderer#products --- lib/open_food_network/products_renderer.rb | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb index 48a35a1b117..a3642a26b5a 100644 --- a/lib/open_food_network/products_renderer.rb +++ b/lib/open_food_network/products_renderer.rb @@ -35,15 +35,20 @@ def products return unless order_cycle @products ||= begin - scoper = ScopeProductToHub.new(distributor) + results = distributed_products.products_relation.order(taxon_order) - distributed_products.products_relation. - order(taxon_order). - ransack(params[:q]).result.page(params[:page] || 1).per(params[:per_page] || 10). - each { |product| scoper.scope(product) } + filter_and_paginate(results).each { |product| product_scoper.scope(product) } end end + def product_scoper + ScopeProductToHub.new(distributor) + end + + def filter_and_paginate(query) + query.ransack(params[:q]).result.page(params[:page] || 1).per(params[:per_page] || 10) + end + def distributed_products OrderCycleDistributedProducts.new(distributor, order_cycle, customer) end From d45403f1d4fb452defdb664b0124154a3479efb8 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 7 Oct 2019 13:29:05 +0100 Subject: [PATCH 16/25] Add specs for Api::OrderCyclesController --- .../api/order_cycles_controller_spec.rb | 158 ++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 spec/controllers/api/order_cycles_controller_spec.rb diff --git a/spec/controllers/api/order_cycles_controller_spec.rb b/spec/controllers/api/order_cycles_controller_spec.rb new file mode 100644 index 00000000000..e6b3bf7eba5 --- /dev/null +++ b/spec/controllers/api/order_cycles_controller_spec.rb @@ -0,0 +1,158 @@ +require "spec_helper" + +module Api + describe OrderCyclesController, type: :controller do + let!(:distributor) { create(:distributor_enterprise) } + let!(:order_cycle) { create(:simple_order_cycle, distributors: [distributor]) } + let!(:exchange) { order_cycle.exchanges.to_enterprises(distributor).outgoing.first } + let!(:taxon1) { create(:taxon, name: 'Meat') } + let!(:taxon2) { create(:taxon, name: 'Vegetables') } + let!(:taxon3) { create(:taxon, name: 'Cake') } + let!(:property1) { create(:property, presentation: 'Organic') } + let!(:property2) { create(:property, presentation: 'Dairy-Free') } + let!(:property3) { create(:property, presentation: 'May Contain Nuts') } + let!(:product1) { create(:product, primary_taxon: taxon1, properties: [property1]) } + let!(:product2) { create(:product, primary_taxon: taxon2, properties: [property2]) } + let!(:product3) { create(:product, primary_taxon: taxon2) } + let!(:product4) { create(:product, primary_taxon: taxon3, properties: [property3]) } + let!(:user) { create(:user) } + let(:customer) { create(:customer, user: user, enterprise: distributor) } + + before do + exchange.variants << product1.variants.first + exchange.variants << product2.variants.first + exchange.variants << product3.variants.first + allow(controller).to receive(:spree_current_user) { user } + end + + describe "#products" do + it "loads products for distributed products in the order cycle" do + api_get :products, id: order_cycle.id, distributor: distributor.id + + product_ids = json_response.map{ |product| product['id'] } + expect(product_ids).to include product1.id, product2.id, product3.id + end + + context "with variant overrides" do + let!(:vo1) { + create(:variant_override, + hub: distributor, + variant: product1.variants.first, + price: 1234.56) + } + let!(:vo2) { + create(:variant_override, + hub: distributor, + variant: product2.variants.first, + count_on_hand: 0) + } + + it "returns results scoped with variant overrides" do + api_get :products, id: order_cycle.id, distributor: distributor.id + + overidden_product = json_response.select{ |product| product['id'] == product1.id } + expect(overidden_product[0]['variants'][0]['price']).to eq vo1.price.to_s + end + + it "does not return products where the variant overrides are out of stock" do + api_get :products, id: order_cycle.id, distributor: distributor.id + + product_ids = json_response.map{ |product| product['id'] } + expect(product_ids).to_not include product2.id + end + end + + context "when tag rules apply" do + let!(:vo1) { + create(:variant_override, + hub: distributor, + variant: product1.variants.first) + } + let!(:vo2) { + create(:variant_override, + hub: distributor, + variant: product2.variants.first) + } + let!(:vo3) { + create(:variant_override, + hub: distributor, + variant: product3.variants.first) + } + let(:default_hide_rule) { + create(:filter_products_tag_rule, + enterprise: distributor, + is_default: true, + preferred_variant_tags: "hide_these_variants_from_everyone", + preferred_matched_variants_visibility: "hidden") + } + let!(:hide_rule) { + create(:filter_products_tag_rule, + enterprise: distributor, + preferred_variant_tags: "hide_these_variants", + preferred_customer_tags: "hide_from_these_customers", + preferred_matched_variants_visibility: "hidden" ) + } + let!(:show_rule) { + create(:filter_products_tag_rule, + enterprise: distributor, + preferred_variant_tags: "show_these_variants", + preferred_customer_tags: "show_for_these_customers", + preferred_matched_variants_visibility: "visible" ) + } + + it "does not return variants hidden by general rules" do + vo1.update_attribute(:tag_list, default_hide_rule.preferred_variant_tags) + + api_get :products, id: order_cycle.id, distributor: distributor.id + + product_ids = json_response.map{ |product| product['id'] } + expect(product_ids).to_not include product1.id + end + + it "does not return variants hidden for this specific customer" do + vo2.update_attribute(:tag_list, hide_rule.preferred_variant_tags) + customer.update_attribute(:tag_list, hide_rule.preferred_customer_tags) + + api_get :products, id: order_cycle.id, distributor: distributor.id + + product_ids = json_response.map{ |product| product['id'] } + expect(product_ids).to_not include product2.id + end + + it "returns hidden variants made visible for this specific customer" do + vo1.update_attribute(:tag_list, default_hide_rule.preferred_variant_tags) + vo3.update_attribute(:tag_list, "#{show_rule.preferred_variant_tags},#{default_hide_rule.preferred_variant_tags}") + customer.update_attribute(:tag_list, show_rule.preferred_customer_tags) + + api_get :products, id: order_cycle.id, distributor: distributor.id + + product_ids = json_response.map{ |product| product['id'] } + expect(product_ids).to_not include product1.id + expect(product_ids).to include product3.id + end + end + end + + describe "#taxons" do + it "loads taxons for distributed products in the order cycle" do + api_get :taxons, id: order_cycle.id, distributor: distributor.id + + taxons = json_response.map{ |taxon| taxon['name'] } + + expect(json_response.length).to be 2 + expect(taxons).to include taxon1.name, taxon2.name + end + end + + describe "#properties" do + it "loads properties for distributed products in the order cycle" do + api_get :properties, id: order_cycle.id, distributor: distributor.id + + properties = json_response.map{ |property| property['name'] } + + expect(json_response.length).to be 2 + expect(properties).to include property1.presentation, property2.presentation + end + end + end +end From b3c89a9d6cec80a49f4ae1d341e860ac9076406f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 7 Oct 2019 13:40:20 +0100 Subject: [PATCH 17/25] Move OpenFoodNetwork::ProductsRenderer (lib) to ProductsRenderer (service) and refactor --- .rubocop_manual_todo.yml | 4 - .../api/order_cycles_controller.rb | 6 +- app/services/products_renderer.rb | 97 ++++++++++++++ lib/open_food_network/products_renderer.rb | 93 -------------- .../products_renderer_spec.rb | 120 ------------------ spec/services/products_renderer_spec.rb | 117 +++++++++++++++++ 6 files changed, 216 insertions(+), 221 deletions(-) create mode 100644 app/services/products_renderer.rb delete mode 100644 lib/open_food_network/products_renderer.rb delete mode 100644 spec/lib/open_food_network/products_renderer_spec.rb create mode 100644 spec/services/products_renderer_spec.rb diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index 4b2c00700c4..cceb5b58026 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -152,7 +152,6 @@ Metrics/LineLength: - lib/open_food_network/payments_report.rb - lib/open_food_network/permalink_generator.rb - lib/open_food_network/products_cache.rb - - lib/open_food_network/products_renderer.rb - lib/open_food_network/reports/bulk_coop_allocation_report.rb - lib/open_food_network/reports/line_items.rb - lib/open_food_network/sales_tax_report.rb @@ -282,7 +281,6 @@ Metrics/LineLength: - spec/lib/open_food_network/permissions_spec.rb - spec/lib/open_food_network/products_and_inventory_report_spec.rb - spec/lib/open_food_network/products_cache_spec.rb - - spec/lib/open_food_network/products_renderer_spec.rb - spec/lib/open_food_network/proxy_order_syncer_spec.rb - spec/lib/open_food_network/scope_variant_to_hub_spec.rb - spec/lib/open_food_network/subscription_payment_updater_spec.rb @@ -629,7 +627,6 @@ Metrics/MethodLength: - lib/open_food_network/payments_report.rb - lib/open_food_network/permissions.rb - lib/open_food_network/products_and_inventory_report.rb - - lib/open_food_network/products_renderer.rb - lib/open_food_network/rack_request_blocker.rb - lib/open_food_network/reports/bulk_coop_allocation_report.rb - lib/open_food_network/reports/bulk_coop_supplier_report.rb @@ -685,7 +682,6 @@ Metrics/ModuleLength: - spec/controllers/api/orders_controller_spec.rb - spec/controllers/spree/api/products_controller_spec.rb - spec/lib/open_food_network/address_finder_spec.rb - - spec/lib/open_food_network/cached_products_renderer_spec.rb - spec/lib/open_food_network/customers_report_spec.rb - spec/lib/open_food_network/enterprise_fee_calculator_spec.rb - spec/lib/open_food_network/option_value_namer_spec.rb diff --git a/app/controllers/api/order_cycles_controller.rb b/app/controllers/api/order_cycles_controller.rb index 685d72176bc..e9d5c996a02 100644 --- a/app/controllers/api/order_cycles_controller.rb +++ b/app/controllers/api/order_cycles_controller.rb @@ -1,5 +1,3 @@ -require 'open_food_network/products_renderer' - module Api class OrderCyclesController < BaseController include EnterprisesHelper @@ -8,7 +6,7 @@ class OrderCyclesController < BaseController skip_authorization_check def products - products = OpenFoodNetwork::ProductsRenderer.new( + products = ProductsRenderer.new( distributor, order_cycle, customer, @@ -16,7 +14,7 @@ def products ).products_json render json: products - rescue OpenFoodNetwork::ProductsRenderer::NoProducts + rescue ProductsRenderer::NoProducts render status: :not_found, json: '' end diff --git a/app/services/products_renderer.rb b/app/services/products_renderer.rb new file mode 100644 index 00000000000..7373ca1980c --- /dev/null +++ b/app/services/products_renderer.rb @@ -0,0 +1,97 @@ +require 'open_food_network/scope_product_to_hub' + +class ProductsRenderer + class NoProducts < RuntimeError; end + DEFAULT_PAGE = 1 + DEFAULT_PER_PAGE = 10 + + def initialize(distributor, order_cycle, customer, params = {}) + @distributor = distributor + @order_cycle = order_cycle + @customer = customer + @params = params + end + + def products_json + raise NoProducts unless order_cycle && distributor && products + + ActiveModel::ArraySerializer.new(products, + each_serializer: Api::ProductSerializer, + current_order_cycle: order_cycle, + current_distributor: distributor, + variants: variants_for_shop_by_id, + master_variants: master_variants_for_shop_by_id, + enterprise_fee_calculator: enterprise_fee_calculator).to_json + end + + private + + attr_reader :order_cycle, :distributor, :customer, :params + + def products + return unless order_cycle + + @products ||= begin + results = distributed_products.products_relation.order(taxon_order) + + filter_and_paginate(results).each { |product| product_scoper.scope(product) } + end + end + + def product_scoper + OpenFoodNetwork::ScopeProductToHub.new(distributor) + end + + def enterprise_fee_calculator + OpenFoodNetwork::EnterpriseFeeCalculator.new distributor, order_cycle + end + + def filter_and_paginate(query) + query. + ransack(params[:q]). + result. + page(params[:page] || DEFAULT_PAGE). + per(params[:per_page] || DEFAULT_PER_PAGE) + end + + def distributed_products + OrderCycleDistributedProducts.new(distributor, order_cycle, customer) + end + + def taxon_order + if distributor.preferred_shopfront_taxon_order.present? + distributor + .preferred_shopfront_taxon_order + .split(",").map { |id| "primary_taxon_id=#{id} DESC" } + .join(",") + ", name ASC" + else + "name ASC" + end + end + + def variants_for_shop + @variants_for_shop ||= begin + scoper = OpenFoodNetwork::ScopeVariantToHub.new(distributor) + + distributed_products.variants_relation. + includes(:default_price, :stock_locations, :product). + where(product_id: products). + each { |v| scoper.scope(v) } + end + end + + def variants_for_shop_by_id + index_by_product_id variants_for_shop.reject(&:is_master) + end + + def master_variants_for_shop_by_id + index_by_product_id variants_for_shop.select(&:is_master) + end + + def index_by_product_id(variants) + variants.each_with_object({}) do |v, vs| + vs[v.product_id] ||= [] + vs[v.product_id] << v + end + end +end diff --git a/lib/open_food_network/products_renderer.rb b/lib/open_food_network/products_renderer.rb deleted file mode 100644 index a3642a26b5a..00000000000 --- a/lib/open_food_network/products_renderer.rb +++ /dev/null @@ -1,93 +0,0 @@ -require 'open_food_network/scope_product_to_hub' - -module OpenFoodNetwork - class ProductsRenderer - class NoProducts < RuntimeError; end - - def initialize(distributor, order_cycle, customer, params = {}) - @distributor = distributor - @order_cycle = order_cycle - @customer = customer - @params = params - end - - def products_json - if order_cycle && distributor && products - enterprise_fee_calculator = EnterpriseFeeCalculator.new distributor, order_cycle - - ActiveModel::ArraySerializer.new(products, - each_serializer: Api::ProductSerializer, - current_order_cycle: order_cycle, - current_distributor: distributor, - variants: variants_for_shop_by_id, - master_variants: master_variants_for_shop_by_id, - enterprise_fee_calculator: enterprise_fee_calculator,).to_json - else - raise NoProducts - end - end - - private - - attr_reader :order_cycle, :distributor, :customer, :params - - def products - return unless order_cycle - - @products ||= begin - results = distributed_products.products_relation.order(taxon_order) - - filter_and_paginate(results).each { |product| product_scoper.scope(product) } - end - end - - def product_scoper - ScopeProductToHub.new(distributor) - end - - def filter_and_paginate(query) - query.ransack(params[:q]).result.page(params[:page] || 1).per(params[:per_page] || 10) - end - - def distributed_products - OrderCycleDistributedProducts.new(distributor, order_cycle, customer) - end - - def taxon_order - if distributor.preferred_shopfront_taxon_order.present? - distributor - .preferred_shopfront_taxon_order - .split(",").map { |id| "primary_taxon_id=#{id} DESC" } - .join(",") + ", name ASC" - else - "name ASC" - end - end - - def variants_for_shop - @variants_for_shop ||= begin - scoper = OpenFoodNetwork::ScopeVariantToHub.new(distributor) - - distributed_products.variants_relation. - includes(:default_price, :stock_locations, :product). - where(product_id: products). - each { |v| scoper.scope(v) } - end - end - - def variants_for_shop_by_id - index_by_product_id variants_for_shop.reject(&:is_master) - end - - def master_variants_for_shop_by_id - index_by_product_id variants_for_shop.select(&:is_master) - end - - def index_by_product_id(variants) - variants.each_with_object({}) do |v, vs| - vs[v.product_id] ||= [] - vs[v.product_id] << v - end - end - end -end diff --git a/spec/lib/open_food_network/products_renderer_spec.rb b/spec/lib/open_food_network/products_renderer_spec.rb deleted file mode 100644 index dbc6d4b5357..00000000000 --- a/spec/lib/open_food_network/products_renderer_spec.rb +++ /dev/null @@ -1,120 +0,0 @@ -require 'spec_helper' -require 'open_food_network/products_renderer' - -module OpenFoodNetwork - describe ProductsRenderer do - let(:distributor) { create(:distributor_enterprise) } - let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor]) } - let(:exchange) { order_cycle.exchanges.to_enterprises(distributor).outgoing.first } - let(:customer) { create(:customer) } - let(:pr) { ProductsRenderer.new(distributor, order_cycle, customer) } - - describe "sorting" do - let(:t1) { create(:taxon) } - let(:t2) { create(:taxon) } - let!(:p1) { create(:product, name: "abc", primary_taxon_id: t2.id) } - let!(:p2) { create(:product, name: "def", primary_taxon_id: t1.id) } - let!(:p3) { create(:product, name: "ghi", primary_taxon_id: t2.id) } - let!(:p4) { create(:product, name: "jkl", primary_taxon_id: t1.id) } - - before do - exchange.variants << p1.variants.first - exchange.variants << p2.variants.first - exchange.variants << p3.variants.first - exchange.variants << p4.variants.first - end - - it "sorts products by the distributor's preferred taxon list" do - allow(distributor).to receive(:preferred_shopfront_taxon_order) { "#{t1.id},#{t2.id}" } - products = pr.send(:products) - expect(products).to eq([p2, p4, p1, p3]) - end - - it "alphabetizes products by name when taxon list is not set" do - allow(distributor).to receive(:preferred_shopfront_taxon_order) { "" } - products = pr.send(:products) - expect(products).to eq([p1, p2, p3, p4]) - end - end - - context "JSON tests" do - let(:product) { create(:product) } - let(:variant) { product.variants.first } - - before do - exchange.variants << variant - end - - it "only returns products for the current order cycle" do - expect(pr.products_json).to include product.name - end - - it "doesn't return products not in stock" do - variant.update_attribute(:on_demand, false) - variant.update_attribute(:on_hand, 0) - expect(pr.products_json).not_to include product.name - end - - it "strips html from description" do - product.update_attribute(:description, "turtles frogs") - json = pr.products_json - expect(json).to include "frogs" - expect(json).not_to include " frogs") + json = products_renderer.products_json + expect(json).to include "frogs" + expect(json).not_to include "