Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Phase 1 Kit Refactor] Fix #3707 add itemizable item, migrate kit line items (backup db before merge) #4560

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

[Phase 1 Kit Refactor] Fix #3707 add itemizable item, migrate kit line items (backup db before merge) #4560

wants to merge 31 commits into from

Conversation

jimmyli97
Copy link
Contributor

@jimmyli97 jimmyli97 commented Aug 1, 2024 •
edited
Loading

Resolves #3707
Merges and closes #3750

Description

Merge #4585 first
BACKUP database before merging - includes migration in e866880 which changes kit line items to point to the item housing the kit as their Itemizable.

From original issue #3707:

  • Adds itemizable module to base Item class
  • Adds migration to change kit line items to point to the item housing the kit
  • Update rspecs, views, KitCreateService, Allocation and Deallocation to point to the item housing the kit instead of the kit
  • Move validations from kit to Item

Additional things done:

  • Add validation to Item which ensures items which don't house kits don't have line items
  • Disabled Itemizable specs for Item temporarily (it will be easier to do these when working on phase 2)
  • Rename previous association Item.line_items to Item.contained_in_line_items to avoid name conflict
  • Add testing a kit to storage_location total_value spec
  • Add testing multiple line items when creating kit to kit system spec

Type of change

  • Breaking change

How Has This Been Tested?

  • passes test suite
  • manual testing:
    • kits show up correctly Kits index
    • kit item shows up in Item index
    • kit creation, allocation, deallocation work
    • kit and kit item can be deactivated and reactivated (noticed a bug present on main branch where on reactivating kit item the corresponding kit doesn't get reactivated, but this should be fixed by phase 2 refactor)
    • kits affect inventory items correctly on Adjustment, Audit, Transfer
    • kits appear on Donations, Purchases, Distributions, Requests, Reports

@jimmyli97 jimmyli97 marked this pull request as draft August 1, 2024 00:33
@jimmyli97 jimmyli97 mentioned this pull request Aug 1, 2024
Open
44 tasks
@jimmyli97 jimmyli97 changed the title WIP fix #3707 add itemizable item WIP fix #3707 address #4217 add itemizable item Aug 1, 2024
@jimmyli97 jimmyli97 changed the title WIP fix #3707 address #4217 add itemizable item WIP fix #3707 address #4217 kit refactor: add itemizable item Aug 1, 2024
@jimmyli97 jimmyli97 changed the title WIP fix #3707 address #4217 kit refactor: add itemizable item WIP fix #3707 address #4217 add itemizable item, migrate kit line items (needs backup db before merge) Aug 1, 2024
@jimmyli97 jimmyli97 force-pushed the 3707-add-itemizable-item branch 2 times, most recently from 539be25 to 930a6e4 Compare August 1, 2024 19:08
@jimmyli97 jimmyli97 changed the title WIP fix #3707 address #4217 add itemizable item, migrate kit line items (needs backup db before merge) WIP fix #3707 add itemizable item, migrate kit line items (backup db before merge) Aug 8, 2024
@jimmyli97 jimmyli97 force-pushed the 3707-add-itemizable-item branch 6 times, most recently from 4286e69 to 02ce2a3 Compare August 16, 2024 02:11
@jimmyli97 jimmyli97 force-pushed the 3707-add-itemizable-item branch 2 times, most recently from d5c5828 to 811db7a Compare August 16, 2024 03:42
jimmyli97 and others added 13 commits August 28, 2024 23:25
@jimmyli97
Update bundler version
6096b19
@jimmyli97
REFACTOR clean up unused line item trait introduced in rubyforgood#4163
ab89456
@jimmyli97
REFACTOR replace kit :with_item with KitCreateService, hard code rspec
4a9dbc7 
* Replace :with_item trait with calls to KitCreateService to make it easy
  to change line_item itemizable behavior later
* Remove unnecessary :with_item traits
* Hard code all rspecs matching against kit values, finishes rubyforgood#4217 for kits
@jimmyli97
FIX typo in params in donation and purchase controllers
8fed381 
* Another typo in docs
@jimmyli97
DOCS add comment clarifying itemizable works with line items
3a8cf5c
@jimmyli97
REFACTOR rename Item:kits scope to :housing_a_kit, add rspecs
3751852 
* Clearer name
* Add rspecs to test :housing_a_kit and :loose scopes
@jimmyli97
REFACTOR DRY up kit base_item, seed_base_items, prevent kit base_item…
437ff5b 
… deletion

* Move seed_base_items code into one static function
* Move kit base item creation code into one static function
* Add code to prevent calling destroy on kit base item and corresponding rspec
* Added comments - not sure about whether other base item request specs are useful or what the purpose of destroy is in the controller if it can't be called
@jimmyli97
Add rspec for item.is_in_kit?
297ac00
@jimmyli97
Fix lint
9014f25
@jimmyli97
rename findorcreatebaseitem to add !
ea03ecc
@jimmyli97
REFACTOR remove unused code childrenservedreportservice
b33f085 
* All SQL code is duplicated in AcquisitionReportService
* RSpec is close enough to what is in AcquisitionReportService Spec that it can be removed without merging in
  (only difference is Diapers - Adult Briefs category is not created, but that shouldn't matter
  because the SQL looks for %diaper% so this category isn't testing anything different)
@jimmyli97
Fix bin/setup so it's working with seed_base_item move
96f9af0
@zeeshan-haidar @jimmyli97
added itemizable module to base item class, remove itemizable from kit
4df8a68
jimmyli97 and others added 15 commits August 28, 2024 23:25
@jimmyli97 @zeeshan-haidar
Migrate kit line items Itemizable to refer to kit's Item instead of kit
e866880 
* original commit: fe64cb6
* this commit isn't necessary: a357379
  when testing item_ids were not changed by this migration

Co-authored-by: zeeshan-haidar <zeshan.webdeveloper@gmail.com>
@jimmyli97 @zeeshan-haidar
items_housing_a_kit MVC changes
9ee5ddb 
* Update KitCreateService, Kit view/controller, Item view/controller logic to use item_housing_a_kit and kit.item.line_items
  (based on: 88c6eba)
* Update kit rspecs to use kit_item_line_items
* Update item.is_in_kit logic

Co-authored-by: zeeshan-haidar <zeshan.webdeveloper@gmail.com>
@jimmyli97
Remove line items from kits factory
87675e6
@jimmyli97
Move validations to item, validate items that don't house kits don't …
76e5f17 
…have line items
@jimmyli97
Fix AcquisitionReportService Kit SQL Query
60e9cdb
@jimmyli97
Fix Allocate and Deallocate Kit services and specs to use kit.item.li…
233d267 
…ne_values
@jimmyli97
Temporarily disable itemizable specs until working on KitItem in ruby…
412fc7e 
…forgood#3652
@jimmyli97
Fix item rspecs to use KitCreateService
76fb73c
@jimmyli97
Update migration so after merged main migrations
1da991c
@jimmyli97
REFACTOR merge setup code and use KitCreateService in children_served…
5869922 
…_report_service_spec
@jimmyli97
FIX SQL queries in AcquisitionReportService and children_served_repor…
db8d603 
…t_service

* Fix bug with not using updated Item.Disposable scope because code is duplicated
  (added comment that this needs to be merged later)
* Rewrite ChildrenServedReportService for item itemizable changes
  (added comment that this code is almost exact duplicate of AcquisitionReportService)
@jimmyli97
Fix various rspecs
f191cf9 
* KitCreateService shouldn't try to find organization until calling valid?
* rename Item.line_items association to Item.contained_in_line_items to avoid name conflict, add rspec
@jimmyli97
Edit kit value specs to clarify use of value_in_cents
4a31fe6 
* move value_per_itemizable spec to item specs
* as far as I can tell, code calls kit.value_in_cents and ignores value_per_itemizable so not sure this spec is needed
* add kit to storage_location total_value spec to clarify usage of value_in_cents
@jimmyli97
Remove unused by_partner_key kit scope
8c08c4f
@jimmyli97
Fix various rspecs
6d85a25 
* Call KitCreateService instead of create(:kit)
* Kit must be created in same organization as TestInventory storage location
* Fix references to line_items
@jimmyli97
Copy link
Contributor Author

failing test is a flake, added to list

@jimmyli97
Change param for is_in_kit, can_deactivate, can_delete to refer to item
49ac15c 
* Change param for item is_in_kit, can_deactivate_or_delete can_delete to refer to items housing kits
* Add rspec to is_in_kit for testing when given list of kits
* Add rspec for item index view to test when kit is created
@jimmyli97
Copy link
Contributor Author

both failed tests are flakes

@jimmyli97
FIX bug where you can't add additional line items when creating kit
80dd89c 
* Add testing adding multiple line items to kit system rspec
@jimmyli97 jimmyli97 changed the title WIP fix #3707 add itemizable item, migrate kit line items (backup db before merge) Fix #3707 add itemizable item, migrate kit line items (backup db before merge) Aug 30, 2024
@jimmyli97 jimmyli97 changed the title Fix #3707 add itemizable item, migrate kit line items (backup db before merge) [Phase 1 Kit Refactor] Fix #3707 add itemizable item, migrate kit line items (backup db before merge) Aug 30, 2024
@jimmyli97 jimmyli97 marked this pull request as ready for review August 30, 2024 02:24
@cielf
Copy link
Collaborator

cielf commented Aug 30, 2024

Hey @jimmyli97 -- just to manage expectations. @dorner is our lead on the kit rework, so we'll want to make sure he has a final review on this. He's more than usually busy at work this/next week, so it may take some time to get it through.

@jimmyli97
Copy link
Contributor Author

@cielf no worries I understand. The roadmap he wrote was pretty clear, I can probably keep working on phase 2 and database cleanup when I get a chance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Kit Redesign] Phase 1 - Change itemizable source
3 participants
@jimmyli97 @cielf @zeeshan-haidar