Eagerly de-duplicating items when submitting request #4414

patelkrunal31 · 2024-05-31T17:46:49Z

Resolves #4394

Description

In order to support the packs, we want the request items to be collapsed at save, so the banks only see one line per item in the request.

On save, the request items should be collapsed so that there is only one request item per item.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

We have added unit tests to cover all the scenario and verified manually by going through the flow on local machine.

Screenshots

scooter-dangle · 2024-05-31T19:05:40Z

app/services/partners/request_create_service.rb

+        if pre_existing_entry
+          pre_existing_entry.quantity = (pre_existing_entry.quantity.to_i + input_item['quantity'].to_i).to_s
+          # TODO: Is the following the correct way to merge?
+          pre_existing_entry.children += input_item['children'] || []


We aren't sure how this child field is meant to be used, so we weren't sure whether this merge method makes sense.

I think you don't need to do this. From what I can tell, each child has a field item_needed_diaperid (which seems to indicate that a child can only really be associated with a single item). So when the family request is created, the logic collects all the children in the family relevant to the given item ID and passes them in. In this case, since the item ID is the same, the list children should also be the same. Probably worth some footling around to verify.

Also probably should have tests around this.

We chatted with Brock. It generally shouldn't happen because today the family requests controller eagerly groups the requested items (but we obvs don't want to rely on that controller logic for the model to be correctly instantiated). We think the 'correct-ish' way to merge in children if it somehow occurred is to do it as a set-style operation. If the upstream code has mucked about with quantity and done it poorly, we won't be able to fix things here for it without causing more issues.

Might be a way in the future to pull more of that instantiation logic out of the controller, but we added a test for now to verify the behavior we've implemented.

scooter-dangle · 2024-05-31T19:08:46Z

app/services/partners/request_create_service.rb

+            name: fetch_organization_item_name(input_item['item_id']),
+            partner_key: fetch_organization_partner_key(input_item['item_id'])


We're relying on the fact that a single request doesn't come from more than one partner (which is why we're not doing any merging of name / partner).

yeah. Probably all of this needs additional refactoring well beyond this de-duplication.

app/mailers/requests_confirmation_mailer.rb

dorner · 2024-05-31T21:19:00Z

app/services/partners/request_create_service.rb

+        if pre_existing_entry
+          pre_existing_entry.quantity = (pre_existing_entry.quantity.to_i + input_item['quantity'].to_i).to_s
+          # TODO: Is the following the correct way to merge?
+          pre_existing_entry.children += input_item['children'] || []


I think you don't need to do this. From what I can tell, each child has a field item_needed_diaperid (which seems to indicate that a child can only really be associated with a single item). So when the family request is created, the logic collects all the children in the family relevant to the given item ID and passes them in. In this case, since the item ID is the same, the list children should also be the same. Probably worth some footling around to verify.

dorner · 2024-05-31T21:19:47Z

app/services/partners/request_create_service.rb

+        if pre_existing_entry
+          pre_existing_entry.quantity = (pre_existing_entry.quantity.to_i + input_item['quantity'].to_i).to_s
+          # TODO: Is the following the correct way to merge?
+          pre_existing_entry.children += input_item['children'] || []


Also probably should have tests around this.

scooter-dangle · 2024-06-01T16:38:26Z

Added data migration in #4417. Dunno if that's the right way to do that, but I don't want us to have to worry about data persisting that would fail the newly added validation.

…this logic https://github.com/rubyforgood/human-essentials/pull/3437/files

… occur in production

awwaiid

One change

awwaiid · 2024-06-01T18:47:10Z

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


Right -- this is the one that could be annoying if we didn't do the data cleanup.

... but I haven't found any places where we actually do editing of item requests after create. So probably fine actually.

I changed it to be a validation on item_requests.

awwaiid · 2024-06-01T18:48:49Z

app/models/request.rb

@@ -59,6 +60,13 @@ def user_email

  private

+  def item_requests_uniqueness_by_item_id
+    item_ids = request_items.map { |item| item["item_id"] }


Switch this to item_requests

awwaiid · 2024-06-01T18:52:12Z

app/services/partners/request_create_service.rb

+      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


.to_s meh, but I get it ... the actual database for Partner::ItemRequest quantity is a string! hmmmmm

soooo annoying that the schema uses string for quantity

awwaiid · 2024-06-01T18:53:02Z

app/services/partners/request_create_service.rb

+            name: fetch_organization_item_name(input_item['item_id']),
+            partner_key: fetch_organization_partner_key(input_item['item_id'])


yeah. Probably all of this needs additional refactoring well beyond this de-duplication.

app/mailers/requests_confirmation_mailer.rb

scooter-dangle · 2024-06-01T21:54:11Z

Blerg. Found a place where we can end up adding dup items in the seed:

human-essentials/db/seeds.rb

Lines 370 to 382 in 0b88184

    
           Array.new(Faker::Number.within(range: 4..14)) do 
        
             item = p.organization.items.sample 
        
             new_item_request = Partners::ItemRequest.new( 
        
               item_id: item.id, 
        
               quantity: Faker::Number.within(range: 10..30), 
        
               children: [], 
        
               name: item.name, 
        
               partner_key: item.partner_key, 
        
               created_at: date, 
        
               updated_at: date 
        
             ) 
        
             partner_request.item_requests << new_item_request 
        
           end

Will need to pre-sample all the items we intend to add so that we don't end up adding dups.

EDIT: I'll add a quick fix for this one.

awwaiid

This all works and is good; we still have an issue with some historical data separately. This will prevent and assume the prevention of duplicates going-forward.

However, I'd still like a @dorner check before we merge since it's a bit wide-sweeping.

dorner · 2024-06-07T19:19:11Z

I'm good for this. Is it blocked on #4417 ?

awwaiid · 2024-06-08T01:54:30Z

No, this specific merging action isn't blocked -- it will prevent new duplicates and old-duplicates aren't affected by this since they (contents) aren't edited after create. We still want to de-duplicate the historical records for cleanliness, and it will allow the mailer refactor and a cleaner assumption.

patelkrunal31 force-pushed the kp/merge-line-items-at-request-save branch from 578d3d5 to ac33f88 Compare May 31, 2024 18:40

scooter-dangle reviewed May 31, 2024

View reviewed changes

patelkrunal31 force-pushed the kp/merge-line-items-at-request-save branch from 0c9828f to ee2341f Compare May 31, 2024 19:50

patelkrunal31 marked this pull request as ready for review May 31, 2024 20:51

patelkrunal31 requested review from scooter-dangle, dorner, awwaiid and cielf May 31, 2024 20:51

dorner requested changes May 31, 2024

View reviewed changes

patelkrunal31 requested a review from dorner June 1, 2024 15:46

patelkrunal31 and others added 8 commits June 1, 2024 12:45

Eagerly de-duplicating items when submitting request

1a5aeea

Adding validation on Request model to have unique request for item

d181cee

Since we are no longer storing duplicate items per request, removing …

6e44bce

…this logic https://github.com/rubyforgood/human-essentials/pull/3437/files

Removing the need to have a test for duplicate items in the request

4b5e42f

squashme! tweak child merge logic and post disclaimer

e206bd6

Testing the merging logic for duplicate child request which shouldn't…

789d9af

… occur in production

squashme! fix operator precedence of child merge

67e45eb

Adding a documentation on the fetch_items method

0b88184

patelkrunal31 force-pushed the kp/merge-line-items-at-request-save branch from e2bbc6a to 0b88184 Compare June 1, 2024 16:45

awwaiid requested changes Jun 1, 2024

View reviewed changes

Adding validation on item_reqests instead of request_items

1503829

patelkrunal31 requested a review from awwaiid June 1, 2024 19:45

awwaiid reviewed Jun 1, 2024

View reviewed changes

app/mailers/requests_confirmation_mailer.rb Show resolved Hide resolved

awwaiid requested changes Jun 1, 2024

View reviewed changes

app/mailers/requests_confirmation_mailer.rb Show resolved Hide resolved

Removing the refactor on request mailer to de-dupe

a72d9c0

squashme! don't create dup'd items when seeding requests

44cb8f2

awwaiid approved these changes Jun 2, 2024

View reviewed changes

dorner approved these changes Jun 7, 2024

View reviewed changes

awwaiid merged commit e391667 into main Jun 8, 2024
38 checks passed

awwaiid deleted the kp/merge-line-items-at-request-save branch June 8, 2024 01:54

awwaiid added this to the Request Units (Packs) milestone Jul 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eagerly de-duplicating items when submitting request #4414

Eagerly de-duplicating items when submitting request #4414

patelkrunal31 commented May 31, 2024 •

edited

Loading

scooter-dangle May 31, 2024

dorner May 31, 2024

dorner May 31, 2024

scooter-dangle Jun 1, 2024

scooter-dangle May 31, 2024

awwaiid Jun 1, 2024

dorner May 31, 2024

dorner May 31, 2024

scooter-dangle commented Jun 1, 2024

awwaiid left a comment

awwaiid Jun 1, 2024

awwaiid Jun 1, 2024

patelkrunal31 Jun 1, 2024

awwaiid Jun 1, 2024

awwaiid Jun 1, 2024

scooter-dangle Jun 1, 2024

awwaiid Jun 1, 2024

scooter-dangle commented Jun 1, 2024 •

edited

Loading

awwaiid left a comment

dorner commented Jun 7, 2024

awwaiid commented Jun 8, 2024

		name: fetch_organization_item_name(input_item['item_id']),
		partner_key: fetch_organization_partner_key(input_item['item_id'])

Eagerly de-duplicating items when submitting request #4414

Eagerly de-duplicating items when submitting request #4414

Conversation

patelkrunal31 commented May 31, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scooter-dangle commented Jun 1, 2024

awwaiid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scooter-dangle commented Jun 1, 2024 • edited Loading

awwaiid left a comment

Choose a reason for hiding this comment

dorner commented Jun 7, 2024

awwaiid commented Jun 8, 2024

patelkrunal31 commented May 31, 2024 •

edited

Loading

scooter-dangle commented Jun 1, 2024 •

edited

Loading