diff --git a/app/mailers/requests_confirmation_mailer.rb b/app/mailers/requests_confirmation_mailer.rb index 574c5cd280..71e5fb4758 100644 --- a/app/mailers/requests_confirmation_mailer.rb +++ b/app/mailers/requests_confirmation_mailer.rb @@ -9,6 +9,7 @@ def confirmation_email(request) private + # TODO: remove the need to de-duplicate items in the request def fetch_items(request) combined = combined_items(request) item_ids = combined&.map { |item| item['item_id'] } diff --git a/app/models/request.rb b/app/models/request.rb index c6963aeeb3..f4c2886bcc 100644 --- a/app/models/request.rb +++ b/app/models/request.rb @@ -33,6 +33,7 @@ class Request < ApplicationRecord enum status: { pending: 0, started: 1, fulfilled: 2, discarded: 3 }, _prefix: true validates :distribution_id, uniqueness: true, allow_nil: true + validate :item_requests_uniqueness_by_item_id before_save :sanitize_items_data include Filterable @@ -59,6 +60,13 @@ def user_email private + def item_requests_uniqueness_by_item_id + item_ids = item_requests.map(&:item_id) + if item_ids.uniq.length != item_ids.length + errors.add(:item_requests, "should have unique item_ids") + end + end + def sanitize_items_data return unless request_items && request_items_changed? diff --git a/app/services/partners/request_create_service.rb b/app/services/partners/request_create_service.rb index e5c4058083..8df880ebbf 100644 --- a/app/services/partners/request_create_service.rb +++ b/app/services/partners/request_create_service.rb @@ -54,16 +54,33 @@ def populate_item_request(partner_request) attrs['item_id'].blank? && attrs['quantity'].blank? end - item_requests = formatted_line_items.map do |ira| - Partners::ItemRequest.new( - item_id: ira['item_id'], - quantity: ira['quantity'], - children: ira['children'] || [], # will create ChildItemRequests if there are any - name: fetch_organization_item_name(ira['item_id']), - partner_key: fetch_organization_partner_key(ira['item_id']) - ) + items = {} + + formatted_line_items.each do |input_item| + pre_existing_entry = items[input_item['item_id']] + if pre_existing_entry + pre_existing_entry.quantity = (pre_existing_entry.quantity.to_i + input_item['quantity'].to_i).to_s + # NOTE: When this code was written (and maybe it's still the + # case as you read it!), the FamilyRequestsController does a + # ton of calculation to translate children to item quantities. + # If that logic is incorrect, there's not much we can do here + # to fix things. Could make sense to move more of that logic + # into one of the service objects that instantiate the Request + # object (either this one or the FamilyRequestCreateService). + pre_existing_entry.children = (pre_existing_entry.children + (input_item['children'] || [])).uniq + else + items[input_item['item_id']] = Partners::ItemRequest.new( + item_id: input_item['item_id'], + quantity: input_item['quantity'], + children: input_item['children'] || [], # will create ChildItemRequests if there are any + name: fetch_organization_item_name(input_item['item_id']), + partner_key: fetch_organization_partner_key(input_item['item_id']) + ) + end end + item_requests = items.values + partner_request.item_requests << item_requests partner_request.request_items = partner_request.item_requests.map do |ir| diff --git a/db/seeds.rb b/db/seeds.rb index f9686311a3..0a138b6301 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -353,7 +353,6 @@ def random_record_for_org(org, klass) updated_at: date ) - item_requests = [] pads = p.organization.items.find_by(name: 'Pads') new_item_request = Partners::ItemRequest.new( item_id: pads.id, @@ -367,9 +366,10 @@ def random_record_for_org(org, klass) ) partner_request.item_requests << new_item_request - Array.new(Faker::Number.within(range: 4..14)) do - item = p.organization.items.sample - new_item_request = Partners::ItemRequest.new( + items = p.organization.items.sample(Faker::Number.within(range: 4..14)) - [pads] + + partner_request.item_requests += items.map do |item| + Partners::ItemRequest.new( item_id: item.id, quantity: Faker::Number.within(range: 10..30), children: [], @@ -378,7 +378,6 @@ def random_record_for_org(org, klass) created_at: date, updated_at: date ) - partner_request.item_requests << new_item_request end partner_request.request_items = partner_request.item_requests.map do |ir| diff --git a/spec/factories/requests.rb b/spec/factories/requests.rb index 57dc94cf28..e8a28128d8 100644 --- a/spec/factories/requests.rb +++ b/spec/factories/requests.rb @@ -42,17 +42,6 @@ def random_request_items status { 'pending' } end - trait :with_duplicates do - request_items { - # get 3 unique item ids - keys = Item.active.pluck(:id).sample(3) - # add an extra of the first key, so we have one duplicated item - keys.push(keys[0]) - # give each item a quantity of 50 - keys.map { |k| { "item_id" => k, "quantity" => 50 } } - } - end - trait :with_varied_quantities do request_items { # get 10 unique item ids diff --git a/spec/mailers/requests_confirmation_mailer_spec.rb b/spec/mailers/requests_confirmation_mailer_spec.rb index f0e13c841e..c8578c8c2d 100644 --- a/spec/mailers/requests_confirmation_mailer_spec.rb +++ b/spec/mailers/requests_confirmation_mailer_spec.rb @@ -3,9 +3,7 @@ let(:request) { create(:request, organization: organization) } let(:mail) { RequestsConfirmationMailer.confirmation_email(request) } - let(:request_w_duplicates) { create(:request, :with_duplicates, organization: organization) } let(:request_w_varied_quantities) { create(:request, :with_varied_quantities, organization: organization) } - let(:mail_w_duplicates) { RequestsConfirmationMailer.confirmation_email(request_w_duplicates) } let(:mail_w_varied_quantities) { RequestsConfirmationMailer.confirmation_email(request_w_varied_quantities) } describe "#confirmation_email" do @@ -23,12 +21,6 @@ end end - it 'handles duplicates' do - organization.update!(email: "me@org.com") - expect(mail_w_duplicates.body.encoded).to match('This is an email confirmation') - expect(mail_w_duplicates.body.encoded).to match(' - 100') - end - it 'pairs the right quantities with the right item names' do organization.update!(email: "me@org.com") expect(mail_w_varied_quantities.body.encoded).to match('This is an email confirmation') diff --git a/spec/models/request_spec.rb b/spec/models/request_spec.rb index 512cac62e4..531c829313 100644 --- a/spec/models/request_spec.rb +++ b/spec/models/request_spec.rb @@ -56,6 +56,39 @@ end end + describe "validations" do + let(:item_one) { create(:item) } + let(:item_two) { create(:item) } + subject { build(:request, item_requests: item_requests) } + + context "when item_requests have unique item_ids" do + let(:item_requests) do + [ + create(:item_request, item: item_one, quantity: 5), + create(:item_request, item: item_two, quantity: 3) + ] + end + + it "is valid" do + expect(subject).to be_valid + end + end + + context "when item_requests do not have unique item_ids" do + let(:item_requests) do + [ + create(:item_request, item: item_one, quantity: 5), + create(:item_request, item: item_one, quantity: 3) + ] + end + + it "is not valid" do + expect(subject).to_not be_valid + expect(subject.errors[:item_requests]).to include("should have unique item_ids") + end + end + end + describe "versioning" do it { is_expected.to be_versioned } end diff --git a/spec/services/exports/export_request_service_spec.rb b/spec/services/exports/export_request_service_spec.rb index b36732996c..c6a1eb38e3 100644 --- a/spec/services/exports/export_request_service_spec.rb +++ b/spec/services/exports/export_request_service_spec.rb @@ -25,26 +25,25 @@ request_items: [{ item_id: 0, quantity: 200 }, { item_id: -1, quantity: 200 }]) end - let!(:duplicate_items) do + let!(:unique_items) do [ {item_id: item_3t.id, quantity: 2}, - {item_id: item_2t.id, quantity: 3}, - {item_id: item_3t.id, quantity: 5} + {item_id: item_2t.id, quantity: 3} ] end - let!(:duplicate_item_quantities) do - duplicate_items.each_with_object(Hash.new(0)) do |item, hsh| + let!(:item_quantities) do + unique_items.each_with_object(Hash.new(0)) do |item, hsh| hsh[item[:item_id]] += item[:quantity] end end - let!(:request_with_duplicate_items) do + let!(:request_with_items) do create( :request, :started, organization: org, - request_items: duplicate_items + request_items: unique_items ) end @@ -78,8 +77,8 @@ item_column_idx = expected_headers.each_with_index.to_h[Exports::ExportRequestService::DELETED_ITEMS_COLUMN_HEADER] expect(subject.fourth[item_column_idx]).to eq(400) - expect(subject.fifth[item_2t_column_idx]).to eq(duplicate_item_quantities[item_2t.id]) - expect(subject.fifth[item_3t_column_idx]).to eq(duplicate_item_quantities[item_3t.id]) + expect(subject.fifth[item_2t_column_idx]).to eq(item_quantities[item_2t.id]) + expect(subject.fifth[item_3t_column_idx]).to eq(item_quantities[item_3t.id]) end end end diff --git a/spec/services/partners/family_request_create_service_spec.rb b/spec/services/partners/family_request_create_service_spec.rb index ac22297dab..a7b5ca2275 100644 --- a/spec/services/partners/family_request_create_service_spec.rb +++ b/spec/services/partners/family_request_create_service_spec.rb @@ -55,6 +55,7 @@ let(:second_item_id) { items_to_request.second.id.to_i } let(:child1) { create(:partners_child) } let(:child2) { create(:partners_child) } + let(:child3) { create(:partners_child) } let(:family_requests_attributes) do [ { @@ -66,6 +67,11 @@ item_id: second_item_id, person_count: 2, children: [child1, child2] + }, + { + item_id: second_item_id, + person_count: 2, + children: [child1, child3] } ] end @@ -85,7 +91,12 @@ expect(first_item_request.children).to contain_exactly(child1) second_item_request = partner_request.item_requests.find_by(item_id: second_item_id) - expect(second_item_request.children).to contain_exactly(child1, child2) + # testing the de-duping logic of request create service + expect(second_item_request.children).to contain_exactly(child1, child2, child3) + expect(second_item_request.children.count).to eq(3) + + expect(first_item_request.quantity.to_i).to eq(first_item_request.item.default_quantity) + expect(second_item_request.quantity.to_i).to eq(second_item_request.item.default_quantity * 4) end end diff --git a/spec/services/partners/request_create_service_spec.rb b/spec/services/partners/request_create_service_spec.rb index abb1a3b20f..c3ab8f4fa2 100644 --- a/spec/services/partners/request_create_service_spec.rb +++ b/spec/services/partners/request_create_service_spec.rb @@ -86,6 +86,41 @@ expect(NotifyPartnerJob).to have_received(:perform_now).with(Request.last.id) end + it 'should have created item requests' do + subject + expect(Partners::ItemRequest.count).to eq(item_requests_attributes.count) + end + + context 'when we have duplicate item as part of request' do + let(:duplicate_item) { FactoryBot.create(:item) } + let(:unique_item) { FactoryBot.create(:item) } + let(:item_requests_attributes) do + [ + ActionController::Parameters.new( + item_id: duplicate_item.id, + quantity: 3 + ), + ActionController::Parameters.new( + item_id: unique_item.id, + quantity: 7 + ), + ActionController::Parameters.new( + item_id: duplicate_item.id, + quantity: 5 + ) + ] + end + it 'should add the quantity of the duplicate item' do + subject + aggregate_failures { + expect(Partners::ItemRequest.count).to eq(2) + expect(Partners::ItemRequest.find_by(item_id: duplicate_item.id).quantity).to eq("8") + expect(Partners::ItemRequest.find_by(item_id: unique_item.id).quantity).to eq("7") + expect(Partners::ItemRequest.first.item_id).to eq(duplicate_item.id) + } + end + end + context 'but a unexpected error occured during the save' do let(:error_message) { 'boom' }