WIP [Phase 1 Kit Refactor] Fix #3707 add itemizable item, migrate kit line items (backup db before merge)#4560
Closed
jimmyli97 wants to merge 31 commits intorubyforgood:mainfrom
Closed
WIP [Phase 1 Kit Refactor] Fix #3707 add itemizable item, migrate kit line items (backup db before merge)#4560jimmyli97 wants to merge 31 commits intorubyforgood:mainfrom
jimmyli97 wants to merge 31 commits intorubyforgood:mainfrom
Conversation
44 tasks
jimmyli97
changed the title
jimmyli97
changed the title
jimmyli97
force-pushed
the
3707-add-itemizable-item
branch
from
9f8473a to
6405280
Compare
jimmyli97
changed the title
jimmyli97
force-pushed
the
3707-add-itemizable-item
branch
2 times, most recently
from
539be25 to
930a6e4
Compare
jimmyli97
changed the title
jimmyli97
force-pushed
the
3707-add-itemizable-item
branch
6 times, most recently
from
4286e69 to
02ce2a3
Compare
jimmyli97
force-pushed
the
3707-add-itemizable-item
branch
2 times, most recently
from
d5c5828 to
811db7a
Compare
* 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
* Another typo in docs
* Clearer name * Add rspecs to test :housing_a_kit and :loose scopes
… 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
* 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)
* 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>
…_report_service_spec
…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)
* 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
* 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
* Call KitCreateService instead of create(:kit) * Kit must be created in same organization as TestInventory storage location * Fix references to line_items
jimmyli97
force-pushed
the
3707-add-itemizable-item
branch
from
3540e02 to
6d85a25
Compare
Contributor
Author
|
failing test is a flake, added to list |
* 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
Contributor
Author
|
both failed tests are flakes |
* Add testing adding multiple line items to kit system rspec
jimmyli97
force-pushed
the
3707-add-itemizable-item
branch
from
98b182f to
80dd89c
Compare
jimmyli97
changed the title
jimmyli97
changed the title
Collaborator
|
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. |
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 |
jimmyli97
changed the title
Collaborator
|
@jimmyli97 thank you very much for this work, but unfortunately we've changed too many related things and let this PR age a bit too much for us to be able to merge it. So I'm closing it (and another similar PR), though we'll have the code for inspiration if/when reforming kits comes back around as a priority. Thank you again! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #3707
Merges and closes #3750
Description
Merge #4585 and #4665 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:
Additional things done:
Type of change
How Has This Been Tested?