3609 Inventory out should include items allocated to kits. Inventory in should include kits allocated #3641

zeeshan-haidar · 2023-06-04T16:43:18Z

Resolves #3609

Description:

It's just a quick draft PR to see if I am in the right direction with my approach
I created one model 'allocations' with enum allocation_type: ['inventory_in', 'inventory_out']

so far code added for allocation only, once got some feedback I will also add code for de-allocation

about 'inventory_out'

at time of kit allocation, I am creating one record for 'allocations' with type 'inventory out' with line_items equal to kit's line_items
having quantity in negative, quantity is based number of kits added

so in-case we add another kit with the same kit_id, it will just find this allocation record and adjust quantity of line_items

about 'inventory_in'

at time of kit allocation, creating one record for allocations with type 'inventory_in', then adding only one line_item with kit.item.id and kit quantity,

in-case user add another kit with same kit_id it will find the allocation record with kit_id and type 'inventory_in'
and adjust the first line_item's quantity

Question:

in-case invetory_in there is always one line_item for one record in allocation
so having has_many association for line_items doesn't makes sense here

instead of making itemizable type model we can have model similar to invetory_item for both invetory_in and invetory_out, wdyt?

dorner

Had some comments/suggestions. The overall approach looks good IMO.

dorner · 2023-06-05T13:19:36Z

app/models/allocation.rb

+#  organization_id     :bigint           not null
+#  storage_location_id :bigint           not null
+#
+class Allocation < ApplicationRecord


Can we call this KitAllocation to make it obvious that we're dealing with kits?

@dorner change applied

dorner · 2023-06-05T13:20:17Z

app/models/storage_location.rb

@@ -205,6 +206,38 @@ def decrease_inventory(itemizable_array)
    log
  end

+  def manage_inventory_out(line_items_array, kit_id, operation, adjust_by)


This method shouldn't be in the storage location model - it's only marginally related to it. Since it relates to multiple models, it probably should be in a service object.

Also, calling a method manage_x is a code smell indicating it's trying to do too much, or we're not really sure what it's doing. Can you come up with a different name?

@dorner change applied

dorner · 2023-06-05T13:22:21Z

app/models/storage_location.rb

+
+  def manage_inventory_in(associated_kit, kit_id, operation)
+    allocation = allocations.find_or_create_by!(kit_id: kit_id, organization_id: organization.id, allocation_type: "inventory_in")
+    adjust_by = associated_kit[0][:quantity]


Why is associated_kit an array of hashes? That's not indicative of the argument name.

change applied

dorner · 2023-06-05T13:23:23Z

app/models/storage_location.rb

+  def manage_inventory_in(associated_kit, kit_id, operation)
+    allocation = allocations.find_or_create_by!(kit_id: kit_id, organization_id: organization.id, allocation_type: "inventory_in")
+    adjust_by = associated_kit[0][:quantity]
+    adjust_by  = operation == 'allocate' ? adjust_by : adjust_by.to_i * (-1)


adjust_by = associated_kit[0][:quantity].to_i adjust_by *= -1 if operation == 'allocate'

change applied

…o 3609-kit-allocation-should-create-inventory-in-out

dorner

Just one question - otherwise it looks pretty good!

dorner · 2023-06-09T17:40:05Z

db/migrate/20230606061826_create_kit_allocations.rb

+      t.references :organization, null: false, foreign_key: true
+      t.references :storage_location, null: false, foreign_key: true
+      t.references :kit, null: false, foreign_key: true
+      t.integer :kit_allocation_type, null: false, default: 0


Is there a reason we can't use Postgres enums here?

@dorner updated the code with Postgres enum

…o 3609-kit-allocation-should-create-inventory-in-out

zeeshan-haidar · 2023-06-15T05:28:10Z

@dorner some specs are failing, I checked they are also failing in main branch where I have made no change, any thoughts?

…o 3609-kit-allocation-should-create-inventory-in-out

dorner · 2023-06-23T19:20:21Z

app/models/kit_allocation.rb

@@ -13,5 +13,4 @@
 class KitAllocation < ApplicationRecord
  include Itemizable
  belongs_to :storage_location
-  enum kit_allocation_type: {inventory_in: 0, inventory_out: 1}


Why was this renamed from kit_allocation_type? I feel like that was a more indicative name.

reused kit_allocation_type name

dorner · 2023-06-23T19:21:02Z

db/migrate/20230612075547_add_kit_allocation_type_to_kit_allocations.rb

+    safety_assured do
+      create_enum :kit_allocation_inventory, ["inventory_in", "inventory_out"]
+      change_table :kit_allocations do |t|
+        t.enum :inventory, enum_type: "kit_allocation_inventory", default: "inventory_in", null: false


kit_allocation_type was a brand new column introduced in this PR. There's no need to check in migrations that add the column, then remove it and replace it with a different one. Just change the migration itself and fix your local :)

dorner · 2023-06-23T19:21:15Z

db/migrate/20230614070723_bring_inventory_in_and_out_to_invetory_level.rb

@@ -0,0 +1,92 @@
+class BringInventoryInAndOutToInvetoryLevel < ActiveRecord::Migration[7.0]
+  # this data migration is for the perpose to bring inventory_in and inventory_out


removed this migration

dorner · 2023-06-23T19:21:39Z

db/migrate/20230614070723_bring_inventory_in_and_out_to_invetory_level.rb

+class BringInventoryInAndOutToInvetoryLevel < ActiveRecord::Migration[7.0]
+  # this data migration is for the perpose to bring inventory_in and inventory_out
+  # to current inventory level so that in-case of kit de-allocation we do not face "inconsistency inventory in" error
+  class AllocationService


Is there a reason you copy/pasted this entire class into the migration?

removed this migration as per discussion with @cielf

dorner · 2023-06-23T19:22:25Z

db/migrate/20230614070723_bring_inventory_in_and_out_to_invetory_level.rb

+
+  def up
+    StorageLocation.all.each do |location|
+      location.inventory_items.select { |inventory_item| inventory_item.item.partner_key == "kit" }.each do |inventory_item|


Probably cleaner just to check if the item has a kit_id. You can do this with a join rather than a select after the fact:

location.inventory_items.joins(:item).merge(Item.kits)

removed this migration as per discussion with @cielf

dorner · 2023-06-23T19:24:30Z

db/migrate/20230614070723_bring_inventory_in_and_out_to_invetory_level.rb

+        kit_item = inventory_item.item
+        kit = kit_item.kit
+        if kit.present?
+          service = AllocationService.new(storage_location: location, kit: kit, increase_by: kit_quantity)


Are we 100% sure we need to do this for all existing items? I.e. are we sure that the inventory drift issue is a fact across all inventory?

i'm going to have to dig into this a bit, but I don't think the correction was part of the ask in the issue.

There is an argument for adding in the line items for the previously allocated kits - but I'm not yet convinced we should, because of intervening adjustments by the banks.

We talked about this in the Sunday planning meeting . Our take on it is that this migration should not go in, because of other things we can't fix, and resultant confusion for the banks.

@cielf removed data migration

…o 3656-add-shipped-to-delivery-method

…o 3609-kit-allocation-should-create-inventory-in-out # Conflicts: # db/schema.rb

…han-haidar/human-essentials into 3609-kit-allocation-should-create-inventory-in-out

dorner

I think this is good to go!

cielf · 2023-07-10T01:11:16Z

Thanks @zeeshan-haidar -- we pushed this to production this morning -- there are still some things to be done, though -- please check out the re-opened issue.

zeeshan-haidar closed this Jun 4, 2023

zeeshan-haidar reopened this Jun 4, 2023

zeeshan-haidar marked this pull request as draft June 4, 2023 16:46

zeeshan-haidar force-pushed the 3609-kit-allocation-should-create-inventory-in-out branch from 0a6ed90 to 42c5f9d Compare June 5, 2023 12:07

dorner requested changes Jun 5, 2023

View reviewed changes

zeeshan-haidar added 3 commits June 6, 2023 11:28

KitAllocation model added for inventory in and inventory out

83347ed

allocate inventory in and out actions added

2373afb

Deallocation for inventory in and inventory out added

3327d74

zeeshan-haidar force-pushed the 3609-kit-allocation-should-create-inventory-in-out branch from 42c5f9d to 3327d74 Compare June 7, 2023 05:04

zeeshan-haidar added 7 commits June 7, 2023 12:19

spec added for inventory in and inventory out when kit is allocated

8345ef6

fixed typo error

58c8862

test case added for kit with multiple line items

b30215b

testcases added for deallocating inventory_in and inventory_out

d8d1e7b

Merge branch 'main' of github.com:zeeshan-haidar/human-essentials int…

bef402e

…o 3609-kit-allocation-should-create-inventory-in-out

rubocop lint applied

ded1d9e

factory_bot lint fixed

e049dbc

zeeshan-haidar requested a review from dorner June 9, 2023 07:20

dorner reviewed Jun 9, 2023

View reviewed changes

zeeshan-haidar added 6 commits June 12, 2023 11:48

Merge branch 'main' of github.com:zeeshan-haidar/human-essentials int…

f3fb846

…o 3609-kit-allocation-should-create-inventory-in-out

removed ruby enum kit_allocation_type

301a67b

inventory enum added to kit_allocations

3912f08

Merge branch 'main' of github.com:zeeshan-haidar/human-essentials int…

11be7a6

…o 3609-kit-allocation-should-create-inventory-in-out

data migration for invetory_in and invetory_out

256dcf3

rubocop linting applied

592da55

zeeshan-haidar marked this pull request as ready for review June 15, 2023 05:25

zeeshan-haidar requested a review from dorner June 16, 2023 03:48

zeeshan-haidar added 3 commits June 16, 2023 11:17

column shipping_cost added to distributions

595b96f

added shipping_cost field on distribution form

b2b377e

save shipping_cost to distribution

6588d3b

zeeshan-haidar added 2 commits June 23, 2023 11:54

rubocop linting fixed

fed4645

Merge branch 'main' of github.com:zeeshan-haidar/human-essentials int…

8332212

…o 3609-kit-allocation-should-create-inventory-in-out

dorner requested changes Jun 23, 2023

View reviewed changes

zeeshan-haidar added 21 commits June 26, 2023 13:45

Merge branch 'main' of github.com:zeeshan-haidar/human-essentials int…

55d892a

…o 3656-add-shipped-to-delivery-method

distribution stimulus controller added for shipping cost

17cc4de

renamed distribution controller to destribution-delivery

bbc1133

PR requested changes applied

5fb7597

PR requested changes applied

d584002

extra empty line removed

5613746

removed typo error

8536f17

PR requested change applied

e19f8cd

PR requested change applied

2f6305a

wrapped shipping cost with it's own div

4f5a04d

code refactored with PR requested changes

ca0df90

Merge branch 'main' of github.com:zeeshan-haidar/human-essentials int…

28d152f

…o 3656-add-shipped-to-delivery-method

rubocop linting fixed

7182d7c

Merge branch 'main' of github.com:zeeshan-haidar/human-essentials int…

1f140f0

…o 3656-add-shipped-to-delivery-method

code refactored

e972029

Merge branch 'main' of github.com:zeeshan-haidar/human-essentials int…

ae7bce2

…o 3609-kit-allocation-should-create-inventory-in-out # Conflicts: # db/schema.rb

removed inventory enum from kit_allocation

df1410e

added kit_allocation_type enum

7bb7a4e

renamed invetory enum to kit_allocation_type

e19f699

code refactored

6a14d6c

Merge branch '3656-add-shipped-to-delivery-method' of github.com:zees…

c298036

…han-haidar/human-essentials into 3609-kit-allocation-should-create-inventory-in-out

zeeshan-haidar requested a review from dorner July 4, 2023 06:36

dorner approved these changes Jul 6, 2023

View reviewed changes

dorner merged commit a99cae2 into rubyforgood:main Jul 6, 2023

dorner mentioned this pull request Jul 6, 2023

Migration failing on staging #3715

Closed

jimmyli97 mentioned this pull request Jul 25, 2024

[Kit Redesign] Phase 1 - Change itemizable source #3750

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3609 Inventory out should include items allocated to kits. Inventory in should include kits allocated #3641

3609 Inventory out should include items allocated to kits. Inventory in should include kits allocated #3641

zeeshan-haidar commented Jun 4, 2023 •

edited

Loading

dorner left a comment

dorner Jun 5, 2023

zeeshan-haidar Jun 9, 2023

dorner Jun 5, 2023

zeeshan-haidar Jun 9, 2023

dorner Jun 5, 2023

zeeshan-haidar Jun 9, 2023

dorner Jun 5, 2023

zeeshan-haidar Jun 9, 2023

dorner left a comment

dorner Jun 9, 2023

zeeshan-haidar Jun 15, 2023

zeeshan-haidar commented Jun 15, 2023

dorner Jun 23, 2023

zeeshan-haidar Jul 4, 2023 •

edited

Loading

dorner Jun 23, 2023

zeeshan-haidar Jul 4, 2023

dorner Jun 23, 2023

zeeshan-haidar Jul 4, 2023

dorner Jun 23, 2023

zeeshan-haidar Jul 4, 2023

dorner Jun 23, 2023

zeeshan-haidar Jul 4, 2023

dorner Jun 23, 2023

cielf Jun 23, 2023

cielf Jun 23, 2023

cielf Jun 25, 2023

zeeshan-haidar Jul 4, 2023

dorner left a comment

cielf commented Jul 10, 2023

		@@ -0,0 +1,92 @@
		class BringInventoryInAndOutToInvetoryLevel < ActiveRecord::Migration[7.0]
		# this data migration is for the perpose to bring inventory_in and inventory_out

3609 Inventory out should include items allocated to kits. Inventory in should include kits allocated #3641

3609 Inventory out should include items allocated to kits. Inventory in should include kits allocated #3641

Conversation

zeeshan-haidar commented Jun 4, 2023 • edited Loading

Description:

about 'inventory_out'

about 'inventory_in'

Question:

dorner 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

Choose a reason for hiding this comment

dorner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeeshan-haidar commented Jun 15, 2023

Choose a reason for hiding this comment

zeeshan-haidar Jul 4, 2023 • edited Loading

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

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

dorner left a comment

Choose a reason for hiding this comment

cielf commented Jul 10, 2023

zeeshan-haidar commented Jun 4, 2023 •

edited

Loading

zeeshan-haidar Jul 4, 2023 •

edited

Loading