fix: renames Itemizable.to_a to line_item_hashes (#3909) #4031
Conversation
elasticspoon
force-pushed
the
3909-rename-Itemizable#to_a-to-line_item_hashes(itemizable-array)
branch
2 times, most recently
from
963c445 to
9ba1614
Compare
|
As I was reading the code to better understand what was going on I ended up making a few small refactors within |
There was a problem hiding this comment.
@elasticspoon this is an amazing first pass! Thanks so much for your effort in this. I've left a couple of comments and a question for CL :)
elasticspoon
force-pushed
the
3909-rename-Itemizable#to_a-to-line_item_hashes(itemizable-array)
branch
2 times, most recently
from
3bee0c9 to
56a033c
Compare
elasticspoon
force-pushed
the
3909-rename-Itemizable#to_a-to-line_item_hashes(itemizable-array)
branch
2 times, most recently
from
0a2afcb to
97b6156
Compare
Itemizable has a method to_a which returns an array of hashes representing the line items in that itemizable object. to_a is meant to convert an object to an array - instead, this returns an array of child members of the record. This renames the method so that when we work with the line items inside an Itemizable, we explicitly use a array of hashes rather than the parent Itemizable itself. The main methods affected are by this change are StorageLocation.increase/decrease. These methods were overloaded accepting both Itemizable objects and arrays of hashes. Now, they will exclusively accept arrays of hashes. The Itemizable.to_a method is not fully removed, it currently logs an error and calls the correct method. Fixes rubyforgood#3909
This adds a test for the situation where an item is in the line_item_hash in the decrease inventory method but does not exist in the database. Previously this code path was untested.
This commit simplifies the behavior of the increase and decrease methods by extracting the behavior of updating the quantity of a single item out. The new methods generalizes the behavior of all updates and accepts a boolean to determine if the item quantity should increase or decrease. The method amount_insufficient was similarly extracted out of decrease_inventory.
This commit does a small performance optimization on the validation for empty inventory. It performs faster in all situation except a very large inventory that is completely empty. It also renames the fuctions to better reflect the validation.
This reverts commit 8609d72.
line_item_hashes reflects the return type of the value rather than its meaning. line_item_values better reflects the meaning of the function.
Adds the flag :deprecate_to_a. With flag on it will raise an error and log a warning. With flag off it will silently call the correct method.
Adds tests to itemizable concern to ensure flipper flag works.
elasticspoon
force-pushed
the
3909-rename-Itemizable#to_a-to-line_item_hashes(itemizable-array)
branch
from
97b6156 to
9caee96
Compare
|
Closed the final nit. Hopefully we are all set now! |
There was a problem hiding this comment.
Looks good to me! @cielf any issues with merging?
@elasticspoon thanks for your hard work on this. FYI, in the future please don't force push branches that have pull requests, as it breaks GitHub's "view commits since my last review" feature. 🙂
Ok. What the suggested flow then? I thought I was rebasing my commits on top of main. Does that break it? Or did I accidentally force push? Or does rebasing the branch on Github force push? |
|
Yes, whenever you rebase you're actually creating new commits. The only way to keep commits is to merge main back into your branch if you want it there. |
|
Makes sense. Github just stores the SHA of the last commit you reviewed and when I rebase it creates a bunch of new commits since then, so the diff will show all that info. That's kind of unfortunate. Apparently you can use something like the script mentioned here but that is local Github CLI only. I will stick to merging in the future then :) |
|
@dorner I think we can go ahead -- it's behind a flag, so we can shut it off quickly once we turn it on. Naive question -- does the flag automatically get turned on when it deploys to staging? |
|
No, it does not. |
elasticspoon
deleted the
3909-rename-Itemizable#to_a-to-line_item_hashes(itemizable-array)
branch
|
@elasticspoon: Your PR |
Description
Itemizablehas a methodto_awhich returns an array of hashes representing the line items in thatItemizableobject.to_ais meant to convert an object to an array - instead, this returns an array of child members of the record.This renames the method to
Itemizable.line_item_hashesso that when we work with the line items inside anItemizable, we explicitly use a array of hashes rather than the parentItemizableitself.Tradeoffs and Considerations
The main methods affected are by this change are
StorageLocation.increase_inventory/StorageLocation.decrease_inventory. These methods were overloaded accepting bothItemizableobjects and arrays of hashes.Now, they will exclusively accept arrays of hashes. But this causes a bunch of ugly looking code because it now passes in the array directly.
There are several other potential ways to approach this:
increaseanddecreasecould remain overloaded, check if they were passed an array orItemizableand behave differently. I think this results in complection, the method knows too much about what it is being given. It would cause issues if the method was passed an array of something other than hashes.increase and decreasecould expect anItemizableand callline_item_hashesthemselves. This cleans up a lot of the code. However, there are several instances where the methods are passed arrays of hashes (here and here). I am not sure how to approach making in those places.increase_inventorycould beincrease_inventory(itemizable_array:)to better reflect its behavior. That would make it even more verbose.I am totally open to modifying how this is approached.
Note:
to_amethod is not actually removed. I removed all calls to it that I could find but if called it currently logs an error and callsline_item_hashes. This is a quick change to fully remove it.I was worried that some calls might not get caught by tests and I did not want the application to blow up with a no method error. I can also change that as needed.
Resolves #3909
Type of change
How Has This Been Tested?
I ran the full suite. But I did not write any tests specifically for my changes, I am not sure how to directly test the renaming of a method. I also clicked around the site a bit as different users to see if
to_awould get called and it seems fine.However, I was having a lot of flakiness issues with the systems specs (dashboard, admin/users_system). This was happening even on the main branch for me, so I don't think its related to my code. I think the tests are just not playing nice with me using docker.
I was also unable to run the CSV parsing tests, the setting for downloads to automatically download was not working.