-
-
Notifications
You must be signed in to change notification settings - Fork 478
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.
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
#4217 kit factory refactor #4585
base: main
Are you sure you want to change the base?
Conversation
* 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
59374ad
to
2c5ed05
Compare
the failing tests are flaky ones, added them to the list |
* 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)
1969e05
to
141d959
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I think this PR is trying to do a bit too many things at once - it might be easier to focus on things that (for example) aren't blocked by the PR you mention. Maybe the cleanup pieces could be on their own and the factory pieces could be separate.
@@ -38,9 +38,12 @@ def show | |||
@items = @base_item.items | |||
end | |||
|
|||
# TODO there are no buttons on the view to call this method, consider removing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to be removing the current approach to base items at some point in the future. I think for now it's safe to just leave this method alone if nothing calls it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app/services/kit_create_service.rb
Outdated
attr_reader :kit | ||
|
||
def self.FindOrCreateKitBaseItem! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idiomatic Ruby uses snake_case: find_or_create_kit_base_item!
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -31,47 +31,8 @@ def average_children_monthly | |||
total_children_served / 12.0 | |||
end | |||
|
|||
def disposable_diapers_from_kits_total |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were these lines removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
continued conversation in #4665
app/models/base_item.rb
Outdated
|
||
def self.seed_base_items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only used in specs and seeds. Not sure this belongs on the main BaseItem class. I'd probably create a module call Seeds
or something like that that can be referenced from those two contexts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{item_id: item2.id, quantity: 3} | ||
] | ||
|
||
kit = KitCreateService.new(organization_id: organization.id, kit_params: params).call.kit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way for us to just update the factory itself to call KitCreateService
? E.g. by overriding to_create
? https://github.com/thoughtbot/factory_bot/blob/main/GETTING_STARTED.md#custom-methods-to-persist-objects
If not, can we create a reusable method to do this work, allowing you just to pass in the LineItems you want to create, and provide a sane default if we don't care what they are?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered overriding to_create
but I don't think it'll work because the way it works right now KitCreateService creates its own instance of kit and I don't think it's worth the time to rejig it so that it can accept FactoryBot's instance, since it will be changed in the kit refactoring.
I created a helper method under spec/support/kit_helper.rb
in 730371a
spec/models/item_spec.rb
Outdated
@@ -57,6 +57,29 @@ | |||
expect(Item.by_size("4").length).to eq(2) | |||
end | |||
|
|||
it "->housing_a_kit returns all items which belongs_to (house) a kit" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, if the first word of the test description isn't "should", you should use the specify
keyword instead of it
. They are aliases of each other - it's just a readability thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split off the cleanup pieces into PR #4665 |
@jimmyli97 FYI: We had an urgent fix that required all the senior contributors this week , so we didn't get to look at this again. Hopefully this week will go better. |
Addresses #4217 for kits
Description
Kit :with_item trait removal conflicts with #4582, recommend merging that one first then I can merge and change their rspecs in this PR
Type of change
How Has This Been Tested?
passes all rspecs