raggedstaff/openfoodnetwork
1
0
Fork 0
mirror of https://github.com/openfoodfoundation/openfoodnetwork synced 2026-02-26 01:33:22 +00:00
Code Issues Packages Projects Releases Wiki Activity

Refactoring OrderCycleFormApplicator logic for improved update speed

Browse Source
This commit is contained in:
Rob Harrington
2016-06-17 10:48:18 +10:00
parent 6586e67a5c
commit 1e142aa628
3 changed files with 130 additions and 164 deletions
Download Patch File Download Diff File Expand all files Collapse all files

74
lib/open_food_network/order_cycle_form_applicator.rb
View File

@@ -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

6
spec/factories.rb
View File

@@ -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

214
spec/lib/open_food_network/order_cycle_form_applicator_spec.rb
View File

@@ -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