From 1e142aa6285dda5ebd7365a62bdda1f3e945cbf6 Mon Sep 17 00:00:00 2001 From: Rob Harrington Date: Fri, 17 Jun 2016 10:48:18 +1000 Subject: [PATCH 1/3] Refactoring OrderCycleFormApplicator logic for improved update speed --- .../order_cycle_form_applicator.rb | 74 ++---- spec/factories.rb | 6 +- .../order_cycle_form_applicator_spec.rb | 214 +++++++++--------- 3 files changed, 130 insertions(+), 164 deletions(-) diff --git a/lib/open_food_network/order_cycle_form_applicator.rb b/lib/open_food_network/order_cycle_form_applicator.rb index 2cccc4a4fa..1df429b4fb 100644 --- a/lib/open_food_network/order_cycle_form_applicator.rb +++ b/lib/open_food_network/order_cycle_form_applicator.rb @@ -132,71 +132,43 @@ module OpenFoodNetwork editable_variants_for_outgoing_exchanges_to(receiver).pluck(:id) end - def find_incoming_exchange(attrs) - @order_cycle.exchanges. - where(:sender_id => attrs[:enterprise_id], :receiver_id => @order_cycle.coordinator_id, :incoming => true).first - end - - def find_outgoing_exchange(attrs) - @order_cycle.exchanges. - where(:sender_id => @order_cycle.coordinator_id, :receiver_id => attrs[:enterprise_id], :incoming => false).first - end - - def persisted_variants_hash(exchange) - return {} unless exchange - - # When we have permission to edit a variant, mark it for removal here, assuming it will be included again if that is what the use wants - # When we don't have permission to edit a variant and it is already in the exchange, keep it in the exchange. - method_name = "editable_variant_ids_for_#{ exchange.incoming? ? 'incoming' : 'outgoing' }_exchange_between" - editable = send(method_name, exchange.sender, exchange.receiver) - Hash[ exchange.variants.map { |v| [v.id, editable.exclude?(v.id)] } ] + def find_exchange(sender_id, receiver_id, incoming) + @order_cycle.exchanges.find_by_sender_id_and_receiver_id_and_incoming(sender_id, receiver_id, incoming) end def incoming_exchange_variant_ids(attrs) - exchange = find_incoming_exchange(attrs) - variants = persisted_variants_hash(exchange) - - sender = exchange.andand.sender || Enterprise.find(attrs[:enterprise_id]) + sender = Enterprise.find(attrs[:enterprise_id]) receiver = @order_cycle.coordinator - permitted = editable_variant_ids_for_incoming_exchange_between(sender, receiver) + exchange = find_exchange(sender.id, receiver.id, true) - # Only change visibility for variants I have permission to edit - attrs[:variants].each do |variant_id, value| - variants[variant_id.to_i] = value if permitted.include?(variant_id.to_i) - end + requested_ids = attrs[:variants].select{ |k,v| v }.keys.map(&:to_i) # Only the ids the user has requested + existing_ids = exchange.present? ? exchange.variants.pluck(:id) : [] # The ids that already exist + editable_ids = editable_variant_ids_for_incoming_exchange_between(sender, receiver) # The ids we are allowed to add/remove - variants_to_a variants + result = existing_ids + + result |= (requested_ids & editable_ids) # add any requested & editable ids that are not yet in the exchange + result -= ((result & editable_ids) - requested_ids) # remove any editable ids that were not specifically mentioned in the request + + result end def outgoing_exchange_variant_ids(attrs) - exchange = find_outgoing_exchange(attrs) - variants = persisted_variants_hash(exchange) - sender = @order_cycle.coordinator - receiver = exchange.andand.receiver || Enterprise.find(attrs[:enterprise_id]) - permitted = editable_variant_ids_for_outgoing_exchange_between(sender, receiver) + receiver = Enterprise.find(attrs[:enterprise_id]) + exchange = find_exchange(sender.id, receiver.id, false) - # Only change visibility for variants I have permission to edit - attrs[:variants].each do |variant_id, value| - variant_id = variant_id.to_i + requested_ids = attrs[:variants].select{ |k,v| v }.keys.map(&:to_i) # Only the ids the user has requested + existing_ids = exchange.present? ? exchange.variants.pluck(:id) : [] # The ids that already exist + editable_ids = editable_variant_ids_for_outgoing_exchange_between(sender, receiver) # The ids we are allowed to add/remove - variants = update_outgoing_variants(variants, permitted, variant_id, value) - end + result = existing_ids - variants_to_a variants - end + result |= (requested_ids & editable_ids) # add any requested & editable ids that are not yet in the exchange + result -= (result - incoming_variant_ids) # remove any ids not in incoming exchanges + result -= ((result & editable_ids) - requested_ids) # remove any editable ids that were not specifically mentioned in the request - def update_outgoing_variants(variants, permitted, variant_id, value) - if !incoming_variant_ids.include? variant_id - # When a variant has been removed from incoming but remains - # in outgoing, remove it from outgoing too - variants[variant_id] = false - - elsif permitted.include? variant_id - variants[variant_id] = value - end - - variants + result end def incoming_variant_ids diff --git a/spec/factories.rb b/spec/factories.rb index 6d931ee2d0..5729d6c12f 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -105,10 +105,10 @@ FactoryGirl.define do end factory :exchange, :class => Exchange do - order_cycle { OrderCycle.first || FactoryGirl.create(:simple_order_cycle) } - sender { FactoryGirl.create(:enterprise) } - receiver { FactoryGirl.create(:enterprise) } incoming false + order_cycle { OrderCycle.first || FactoryGirl.create(:simple_order_cycle) } + sender { incoming ? FactoryGirl.create(:enterprise) : order_cycle.coordinator } + receiver { incoming ? order_cycle.coordinator : FactoryGirl.create(:enterprise) } end factory :variant_override, :class => VariantOverride do diff --git a/spec/lib/open_food_network/order_cycle_form_applicator_spec.rb b/spec/lib/open_food_network/order_cycle_form_applicator_spec.rb index fe3155c5e5..80fef2b60e 100644 --- a/spec/lib/open_food_network/order_cycle_form_applicator_spec.rb +++ b/spec/lib/open_food_network/order_cycle_form_applicator_spec.rb @@ -144,128 +144,122 @@ module OpenFoodNetwork end end - describe "finding alterable exchange variants" do - let(:coordinator_mock) { double(:enterprise) } - let(:oc) { double(:oc, coordinator: coordinator_mock ) } + describe "updating the list of variants for a given outgoing exchange" do + let!(:v1) { create(:variant) } # Not Existing + Request Add + Editable + Incoming + let!(:v2) { create(:variant) } # Not Existing + Request Add + Not Editable + Incoming + let!(:v3) { create(:variant) } # Existing + Request Add + Editable + Incoming + let!(:v4) { create(:variant) } # Existing + Not mentioned + Editable + Incoming + let!(:v5) { create(:variant) } # Existing + Request Remove + Editable + Incoming + let!(:v6) { create(:variant) } # Existing + Request Remove + Not Editable + Incoming + let!(:v7) { create(:variant) } # Existing + Request Add + Not Editable + Not Incoming + let!(:v8) { create(:variant) } # Existing + Request Add + Editable + Not Incoming + let!(:v9) { create(:variant) } # Not Existing + Request Add + Editable + Not Incoming + let!(:exchange) { create(:exchange, incoming: false, variant_ids: [v3.id, v4.id, v5.id, v6.id, v7.id, v8.id]) } + let!(:oc) { exchange.order_cycle } + let!(:enterprise) { exchange.receiver } + let!(:coordinator) { oc.coordinator } let!(:applicator) { OrderCycleFormApplicator.new(oc, user) } - - describe "converting the existing variants of an exchange to a hash" do - context "when nil is passed in" do - let(:hash) { applicator.send(:persisted_variants_hash, nil) } - - it "returns an empty hash" do - expect(hash).to eq({}) - end - end - - context "when an exchange is passed in" do - let(:v1) { create(:variant) } - let(:v2) { create(:variant) } - let(:v3) { create(:variant) } - let(:exchange) { create(:exchange, variants: [v1, v2, v3]) } - let(:hash) { applicator.send(:persisted_variants_hash, exchange) } - - before do - allow(applicator).to receive(:editable_variant_ids_for_outgoing_exchange_between) { [ v1.id, v2.id ] } - end - - it "returns a hash with variant ids as keys" do - expect(hash.length).to be 3 - expect(hash.keys).to include v1.id, v2.id, v3.id - end - - it "editable variant ids are set to false" do - expect(hash[v1.id]).to be false - expect(hash[v2.id]).to be false - end - - it "and non-editable variant ids are set to true" do - expect(hash[v3.id]).to be true - end - end + let(:ids) do + applicator.send(:outgoing_exchange_variant_ids, { + :enterprise_id => enterprise.id, + :variants => { + "#{v1.id}" => true, + "#{v2.id}" => true, + "#{v3.id}" => true, + "#{v5.id}" => false, + "#{v6.id}" => false, + "#{v7.id}" => true, + "#{v8.id}" => true, + "#{v9.id}" => true + } + }) end - context "where a matching exchange does not exist" do - let(:enterprise_mock) { double(:enterprise) } - before do - applicator.stub(:find_outgoing_exchange) { nil } - applicator.stub(:incoming_variant_ids) { [1, 2, 3, 4] } - expect(applicator).to receive(:editable_variant_ids_for_outgoing_exchange_between). - with(coordinator_mock, enterprise_mock) { [1, 2, 3] } - end - - it "converts exchange variant ids hash to an array of ids" do - applicator.stub(:persisted_variants_hash) { {} } - expect(Enterprise).to receive(:find) { enterprise_mock } - ids = applicator.send(:outgoing_exchange_variant_ids, {:enterprise_id => 123, :variants => {'1' => true, '2' => false, '3' => true}}) - expect(ids).to eq [1, 3] - end - - it "restricts exchange variant ids to those editable by the current user" do - applicator.stub(:persisted_variants_hash) { {4 => true} } - expect(Enterprise).to receive(:find) { enterprise_mock } - ids = applicator.send(:outgoing_exchange_variant_ids, {:enterprise_id => 123, :variants => {'1' => true, '2' => false, '3' => true, '4' => false}}) - expect(ids).to eq [1, 3, 4] - end - - it "overrides existing variants based on submitted data, when user has permission" do - applicator.stub(:persisted_variants_hash) { {2 => true} } - expect(Enterprise).to receive(:find) { enterprise_mock} - ids = applicator.send(:outgoing_exchange_variant_ids, {:enterprise_id => 123, :variants => {'1' => true, '2' => false, '3' => true}}) - expect(ids).to eq [1, 3] - end + before do + allow(applicator).to receive(:incoming_variant_ids) { [v1.id, v2.id, v3.id, v4.id, v5.id, v6.id] } + allow(applicator).to receive(:editable_variant_ids_for_outgoing_exchange_between) { [v1.id, v3.id, v4.id, v5.id, v8.id, v9.id]} end - context "where a matching exchange exists" do - let(:enterprise_mock) { double(:enterprise) } - let(:exchange_mock) { double(:exchange) } + it "updates the list of variants for the exchange" do + # Adds variants that are editable + expect(ids).to include v1.id - before do - applicator.stub(:find_outgoing_exchange) { exchange_mock } - applicator.stub(:incoming_variant_ids) { [1, 2, 3, 4] } - allow(applicator).to receive(:editable_variant_ids_for_outgoing_exchange_between). - with(coordinator_mock, enterprise_mock) { [1, 2, 3] } - end + # Does not add variants that are not editable + expect(ids).to_not include v2.id - it "converts exchange variant ids hash to an array of ids" do - applicator.stub(:persisted_variants_hash) { {} } - expect(exchange_mock).to receive(:receiver) { enterprise_mock } - ids = applicator.send(:outgoing_exchange_variant_ids, {:enterprise_id => 123, :variants => {'1' => true, '2' => false, '3' => true}}) - expect(ids).to eq [1, 3] - end + # Keeps existing variants, when they are explicitly mentioned in the request + expect(ids).to include v3.id - it "restricts exchange variant ids to those editable by the current user" do - applicator.stub(:persisted_variants_hash) { {4 => true} } - expect(exchange_mock).to receive(:receiver) { enterprise_mock } - ids = applicator.send(:outgoing_exchange_variant_ids, {:enterprise_id => 123, :variants => {'1' => true, '2' => false, '3' => true, '4' => false}}) - expect(ids).to eq [1, 3, 4] - end + # Removes existing variants that are editable, when they are not mentioned in the request + expect(ids).to_not include v4.id - it "overrides existing variants based on submitted data, when user has permission" do - applicator.stub(:persisted_variants_hash) { {2 => true} } - expect(exchange_mock).to receive(:receiver) { enterprise_mock } - ids = applicator.send(:outgoing_exchange_variant_ids, {:enterprise_id => 123, :variants => {'1' => true, '2' => false, '3' => true}}) - expect(ids).to eq [1, 3] - end + # Removes existing variants that are editable, when the request explicitly removes them + expect(ids).to_not include v5.id - it "removes variants which the user has permission to remove and that are not included in the submitted data" do - allow(exchange_mock).to receive(:incoming?) { false } - allow(exchange_mock).to receive(:variants) { [double(:variant, id: 1), double(:variant, id: 2), double(:variant, id: 3)] } - allow(exchange_mock).to receive(:sender) { coordinator_mock } - allow(exchange_mock).to receive(:receiver) { enterprise_mock } - applicator.stub(:incoming_variant_ids) { [1, 2, 3] } - ids = applicator.send(:outgoing_exchange_variant_ids, {:enterprise_id => 123, :variants => {'1' => true, '3' => true}}) - expect(ids).to eq [1, 3] - end + # Keeps existing variants that are not editable + expect(ids).to include v6.id - it "removes variants which are not included in incoming exchanges" do - applicator.stub(:incoming_variant_ids) { [1, 2] } - applicator.stub(:persisted_variants_hash) { {3 => true} } - expect(exchange_mock).to receive(:receiver) { enterprise_mock } - ids = applicator.send(:outgoing_exchange_variant_ids, {:enterprise_id => 123, :variants => {'1' => true, '2' => false, '3' => true}}) - expect(ids).to eq [1] - end + # Removes existing variants that are not in an incoming exchange, regardless of whether they are not editable + expect(ids).to_not include v7.id, v8.id + + # Does not add variants that are not in an incoming exchange + expect(ids).to_not include v9.id + end + end + + describe "updating the list of variants for a given incoming exchange" do + let!(:v1) { create(:variant) } # Not Existing + Request Add + Editable + let!(:v2) { create(:variant) } # Not Existing + Request Add + Not Editable + let!(:v3) { create(:variant) } # Existing + Request Add + Editable + let!(:v4) { create(:variant) } # Existing + Request Remove + Not Editable + let!(:v5) { create(:variant) } # Existing + Request Remove + Editable + let!(:v6) { create(:variant) } # Existing + Request Remove + Not Editable + let!(:v7) { create(:variant) } # Existing + Not mentioned + Editable + let!(:exchange) { create(:exchange, incoming: true, variant_ids: [v3.id, v4.id, v5.id, v6.id, v7.id]) } + let!(:oc) { exchange.order_cycle } + let!(:enterprise) { exchange.sender } + let!(:coordinator) { oc.coordinator } + let!(:applicator) { OrderCycleFormApplicator.new(oc, user) } + let(:ids) do + applicator.send(:incoming_exchange_variant_ids, { + :enterprise_id => enterprise.id, + :variants => { + "#{v1.id}" => true, + "#{v2.id}" => true, + "#{v3.id}" => true, + "#{v4.id}" => false, + "#{v5.id}" => false, + "#{v6.id}" => false + } + }) + end + + before do + allow(applicator).to receive(:editable_variant_ids_for_incoming_exchange_between) { [v1.id, v3.id, v5.id, v7.id]} + end + + it "updates the list of variants for the exchange" do + # Adds variants that are editable + expect(ids).to include v1.id + + # Does not add variants that are not editable + expect(ids).to_not include v2.id + + # Keeps existing variants, if they are editable and requested + expect(ids).to include v3.id + + # Keeps existing variants if they are non-editable, regardless of request + expect(ids).to include v4.id + + # Removes existing variants that are editable, when the request explicitly removes them + expect(ids).to_not include v5.id + + # Keeps existing variants that are not editable + expect(ids).to include v6.id + + # Removes existing variants that are editable, when they are not mentioned in the request + expect(ids).to_not include v7.id end end From e40ecae681421652d92217551db1d0c55c94bb60 Mon Sep 17 00:00:00 2001 From: Rob Harrington Date: Fri, 17 Jun 2016 11:51:27 +1000 Subject: [PATCH 2/3] Removing inline styles for links dropdown --- .../javascripts/templates/admin/links_dropdown.html.haml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/templates/admin/links_dropdown.html.haml b/app/assets/javascripts/templates/admin/links_dropdown.html.haml index 1f44f2418c..6d0ba060e2 100644 --- a/app/assets/javascripts/templates/admin/links_dropdown.html.haml +++ b/app/assets/javascripts/templates/admin/links_dropdown.html.haml @@ -3,8 +3,8 @@ %i.icon-check Actions %i{ 'ng-class' => "expanded && 'icon-caret-up' || !expanded && 'icon-caret-down'" } - %div.menu{ 'ng-show' => "expanded", style: 'width: 200px' } - %a.menu_item{ 'ng-repeat' => "link in links", href: '{{link.url}}', target: "{{link.target || '_self'}}", data: { method: "{{ link.method || 'get' }}", confirm: "{{link.confirm}}" }, style: 'display: inline-block; width: 100%' } - %span{ :style => 'text-align: center; display: inline-block; width: 20%'} + %div.menu{ 'ng-show' => "expanded" } + %a.menu_item{ 'ng-repeat' => "link in links", href: '{{link.url}}', target: "{{link.target || '_self'}}", data: { method: "{{ link.method || 'get' }}", confirm: "{{link.confirm}}" } } + %span %i{ ng: { class: "link.icon" } } - %span{ style: "display: inline-block; width: auto"} {{ link.name }} + %span {{ link.name }} From d28c0159ab87ec949e641caab6f4f4323521e2e3 Mon Sep 17 00:00:00 2001 From: Rob Harrington Date: Fri, 17 Jun 2016 12:06:01 +1000 Subject: [PATCH 3/3] Use have_selector x, count: y; instead of all(x).count.should == y --- spec/features/admin/payment_method_spec.rb | 2 +- spec/features/admin/reports_spec.rb | 2 +- spec/features/admin/shipping_methods_spec.rb | 2 +- spec/features/consumer/shopping/checkout_spec.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/features/admin/payment_method_spec.rb b/spec/features/admin/payment_method_spec.rb index fba8e69401..45703a4d07 100644 --- a/spec/features/admin/payment_method_spec.rb +++ b/spec/features/admin/payment_method_spec.rb @@ -135,7 +135,7 @@ feature %q{ pm2 visit spree.admin_payment_methods_path - page.all('td', text: 'Two').count.should == 1 + page.should have_selector 'td', text: 'Two', count: 1 end diff --git a/spec/features/admin/reports_spec.rb b/spec/features/admin/reports_spec.rb index 2bd39a3e38..e2c991a185 100644 --- a/spec/features/admin/reports_spec.rb +++ b/spec/features/admin/reports_spec.rb @@ -124,7 +124,7 @@ feature %q{ table.sort.should == [ ["Hub", "Code", "First Name", "Last Name", "Supplier", "Product", "Variant", "Quantity", "TempControlled?"] ].sort - all('table#listing_orders tbody tr').count.should == 5 # Totals row per order + page.should have_selector 'table#listing_orders tbody tr', count: 5 # Totals row per order end scenario "Pack By Supplier" do diff --git a/spec/features/admin/shipping_methods_spec.rb b/spec/features/admin/shipping_methods_spec.rb index 646b383c21..46816f5737 100644 --- a/spec/features/admin/shipping_methods_spec.rb +++ b/spec/features/admin/shipping_methods_spec.rb @@ -123,7 +123,7 @@ feature 'shipping methods' do visit spree.admin_shipping_methods_path - page.all('td', text: 'Two').count.should == 1 + page.should have_selector 'td', text: 'Two', count: 1 end pending "shows me only shipping methods for the enterprise I select" do diff --git a/spec/features/consumer/shopping/checkout_spec.rb b/spec/features/consumer/shopping/checkout_spec.rb index cbb4d12554..10147b21a4 100644 --- a/spec/features/consumer/shopping/checkout_spec.rb +++ b/spec/features/consumer/shopping/checkout_spec.rb @@ -300,7 +300,7 @@ feature "As a consumer I want to check out my cart", js: true do # Does not show duplicate shipping fee visit checkout_path - page.all("th", text: "Shipping").count.should == 1 + page.should have_selector "th", text: "Shipping", count: 1 end end end