Merge pull request #4414 from rubyforgood/kp/merge-line-items-at-requ…

…est-save

Eagerly de-duplicating items when submitting request

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'] }

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?
 

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|

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|

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

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: "[email protected]")
-    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: "[email protected]")
     expect(mail_w_varied_quantities.body.encoded).to match('This is an email confirmation')

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

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

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
 

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' }