From 3ffd04913572902756a13e83ed1de073bc44016b Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 2 Nov 2017 11:52:20 +0100 Subject: [PATCH 1/6] Remove commented out code --- app/views/shop/products/_form.html.haml | 7 ------- 1 file changed, 7 deletions(-) diff --git a/app/views/shop/products/_form.html.haml b/app/views/shop/products/_form.html.haml index 75d1f79188..8bf8f8dcbb 100644 --- a/app/views/shop/products/_form.html.haml +++ b/app/views/shop/products/_form.html.haml @@ -33,13 +33,6 @@ = 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.count_on_hand == 0}"} - -# Load more button, which can be used to initiate infinite scrolling. - -# %product{ "ng-hide" => "Products.loading || !infiniteDisabled || limit >= filteredProducts.length" } - -# .row.summary - -# .small-12.columns.text-center - -# %a.button{ ng: { click: 'infiniteDisabled = false; incrementLimit();' } } - -# Load More Products - %product{"ng-show" => "Products.loading"} .row.summary .small-12.columns.text-center From 6a830a3843e145be7cba54625cb9764dc557bc53 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 2 Nov 2017 11:52:34 +0100 Subject: [PATCH 2/6] Isolate ArraySerializer case in injection helper This makes this special case with the ArraySerializer stand out so that the reader notices it. --- app/helpers/injection_helper.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/helpers/injection_helper.rb b/app/helpers/injection_helper.rb index b750d3f46b..5b6186ff3e 100644 --- a/app/helpers/injection_helper.rb +++ b/app/helpers/injection_helper.rb @@ -87,10 +87,12 @@ module InjectionHelper def inject_json_ams(name, data, serializer, opts = {}) if data.is_a?(Array) - json = ActiveModel::ArraySerializer.new(data, {each_serializer: serializer}.merge(opts)).to_json - else - json = serializer.new(data, opts).to_json + opts = { each_serializer: serializer }.merge(opts) + serializer = ActiveModel::ArraySerializer end + + serializer_instance = serializer.new(data, opts) + json = serializer_instance.to_json render partial: "json/injection_ams", locals: {name: name, json: json} end From 4f03a2d25cfbf6b39eae6aec41ca2c0897c8fee0 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 2 Nov 2017 16:27:12 +0100 Subject: [PATCH 3/6] Remove unnecessary require --- spec/lib/open_food_network/cached_products_renderer_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/lib/open_food_network/cached_products_renderer_spec.rb b/spec/lib/open_food_network/cached_products_renderer_spec.rb index 5346db610a..c4ba9b9a33 100644 --- a/spec/lib/open_food_network/cached_products_renderer_spec.rb +++ b/spec/lib/open_food_network/cached_products_renderer_spec.rb @@ -1,6 +1,5 @@ require 'spec_helper' require 'open_food_network/cached_products_renderer' -require 'open_food_network/products_renderer' module OpenFoodNetwork describe CachedProductsRenderer do From 26a4ee0171dfef5a6582b1ff2d7f78ab6397e34a Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 2 Nov 2017 16:27:55 +0100 Subject: [PATCH 4/6] Do not stub object under test --- .../cached_products_renderer_spec.rb | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/spec/lib/open_food_network/cached_products_renderer_spec.rb b/spec/lib/open_food_network/cached_products_renderer_spec.rb index c4ba9b9a33..cf8533f321 100644 --- a/spec/lib/open_food_network/cached_products_renderer_spec.rb +++ b/spec/lib/open_food_network/cached_products_renderer_spec.rb @@ -9,8 +9,14 @@ module OpenFoodNetwork describe "fetching cached products JSON" do context "when in testing / development" do + let(:products_renderer) do + double(ProductsRenderer, products_json: 'uncached products') + end + before do - allow(cpr).to receive(:uncached_products_json) { "uncached products" } + allow(ProductsRenderer) + .to receive(:new) + .with(distributor, order_cycle) { products_renderer } end it "returns uncaches products JSON" do @@ -50,10 +56,16 @@ module OpenFoodNetwork 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) - cpr.stub(:uncached_products_json) { 'fresh products' } + + allow(ProductsRenderer) + .to receive(:new) + .with(distributor, order_cycle) { products_renderer } end describe "when there are products" do @@ -73,7 +85,15 @@ module OpenFoodNetwork end describe "when there are no products" do - before { cpr.stub(:uncached_products_json).and_raise ProductsRenderer::NoProducts } + 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 { cpr.products_json }.to raise_error CachedProductsRenderer::NoProducts From 98603c4042e8f903c9540085ed534365b2e0e71c Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 2 Nov 2017 16:51:56 +0100 Subject: [PATCH 5/6] Do not test private methods --- .../cached_products_renderer_spec.rb | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/spec/lib/open_food_network/cached_products_renderer_spec.rb b/spec/lib/open_food_network/cached_products_renderer_spec.rb index cf8533f321..b9c8af7322 100644 --- a/spec/lib/open_food_network/cached_products_renderer_spec.rb +++ b/spec/lib/open_food_network/cached_products_renderer_spec.rb @@ -7,7 +7,7 @@ module OpenFoodNetwork let(:order_cycle) { double(:order_cycle, id: 456) } let(:cpr) { CachedProductsRenderer.new(distributor, order_cycle) } - describe "fetching cached products JSON" do + describe "#products_json" do context "when in testing / development" do let(:products_renderer) do double(ProductsRenderer, products_json: 'uncached products') @@ -133,17 +133,5 @@ module OpenFoodNetwork cpr.send(:log_warning) end end - - describe "fetching uncached products from ProductsRenderer" do - let(:pr) { double(:products_renderer, products_json: 'uncached products') } - - before do - ProductsRenderer.stub(:new) { pr } - end - - it "returns the uncached products" do - expect(cpr.send(:uncached_products_json)).to eq 'uncached products' - end - end end end From 84e4ebef084eec69897a03e7fa5c134d5bb861ac Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 2 Nov 2017 16:52:49 +0100 Subject: [PATCH 6/6] Do not notify Bugsnag of a cache miss It's not the responsibility of a error tracking software to track neither cache misses nor logs. That is what log monitoring is for. --- .../cached_products_renderer.rb | 8 ----- .../cached_products_renderer_spec.rb | 30 ------------------- 2 files changed, 38 deletions(-) diff --git a/lib/open_food_network/cached_products_renderer.rb b/lib/open_food_network/cached_products_renderer.rb index 858b326b47..733455ed73 100644 --- a/lib/open_food_network/cached_products_renderer.rb +++ b/lib/open_food_network/cached_products_renderer.rb @@ -26,17 +26,9 @@ module OpenFoodNetwork private - def log_warning - if Rails.env.production? || Rails.env.staging? - Bugsnag.notify RuntimeError.new("Live server MISS on products cache for distributor: #{@distributor.id}, order cycle: #{@order_cycle.id}") - end - end - def cached_products_json if Rails.env.production? || Rails.env.staging? Rails.cache.fetch("products-json-#{@distributor.id}-#{@order_cycle.id}") do - log_warning - begin uncached_products_json rescue ProductsRenderer::NoProducts diff --git a/spec/lib/open_food_network/cached_products_renderer_spec.rb b/spec/lib/open_food_network/cached_products_renderer_spec.rb index b9c8af7322..8a3e72a775 100644 --- a/spec/lib/open_food_network/cached_products_renderer_spec.rb +++ b/spec/lib/open_food_network/cached_products_renderer_spec.rb @@ -77,11 +77,6 @@ module OpenFoodNetwork cpr.products_json expect(cached_json).to eq 'fresh products' end - - it "logs a warning" do - cpr.should_receive :log_warning - cpr.products_json - end end describe "when there are no products" do @@ -104,34 +99,9 @@ module OpenFoodNetwork expect(cache_present).to be expect(cached_json).to be_nil end - - it "logs a warning" do - cpr.should_receive :log_warning - expect { cpr.products_json }.to raise_error CachedProductsRenderer::NoProducts - end end end end end - - describe "logging a warning" do - it "logs a warning when in production" do - Rails.env.stub(:production?) { true } - expect(Bugsnag).to receive(:notify) - cpr.send(:log_warning) - end - - it "logs a warning when in staging" do - Rails.env.stub(:production?) { false } - Rails.env.stub(:staging?) { true } - expect(Bugsnag).to receive(:notify) - cpr.send(:log_warning) - end - - it "does not log a warning in development or test" do - expect(Bugsnag).to receive(:notify).never - cpr.send(:log_warning) - end - end end end