-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add factorybot box traits #430
base: develop
Are you sure you want to change the base?
Conversation
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.
Thanks, @RachelWyatt !!! This looks like exactly what's been needed! I will go through it in more detail soon, but on initial pass it looks great. :) Hope you're well!
@maebeale awesome! Just let me know if you run across anything that needs updates. I'm doing great, I hope you're doing well too! |
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.
@RachelWyatt this is GREAT work!!! VERY excited to be getting these traits in order! Do you have capacity to make the changes per the comments?
|
||
trait :researched do | ||
research_in_progress | ||
aasm_state {"researched"} |
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 should roll up from box_items, which each have researched_at
, and a box is "researched" (and should get its own researched_at
populated when all of its box_items
are researched (or are box_items that don't need research. most items don't need research -- this is for those community references and personalized stuff.)
box_request_1.claim_review! | ||
box_request_1.complete_review! | ||
box = box_request_1.box | ||
box = build(:box, :design_in_progress) | ||
allow(box).to receive(:send_assembly_solicitation_email!) |
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 think this can be deleted now?
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.
The test fails when I delete allow(box).to receive(:send_assembly_solicitation_email!). Does it need to be added somewhere in the factory? If so, should I do that for all the emails that should be received?
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.
@RachelWyatt hmm... it should pass and shouldn't need to be in a factory. the allow, afaik, means that it's confirming that the method exists on box
, rather than that it was executed.
box.claim_design! | ||
box.check_has_box_items # make sure there are items | ||
# make sure at least one item needs research | ||
create(:box_item, box: box, inventory_type: inventory_type_research_needed) |
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 think we still need this line, bc otherwise the box won't transition to research_needed ?
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 think this is being handled in the factory bot designed
trait, where a box with status designed
has a box item that needs research:
trait :designed do
design_in_progress
designed_at { Time.now }
aasm_state { "designed"}
box_items {create_list(:box_item, 1, :research_needed)}
end
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.
OH. I see, @RachelWyatt .
designed boxes in practice should have at least 1 box_item, but not necessarily one that needs research.
i think that that should be a separate factory.
does designed technically require 1 box item according to assm stuff? (i thought it only needed designed_at and designed_by_id?)
Once the changes are made and all these tests still pass, then we're golden @RachelWyatt ! |
Because #432 fixed a bunch of tests and was merged in to develop after this PR was created, if you rebase this branch against develop and push it up, that should help get the circleci status to be green. If you have any problems with the rebase, please let me know |
I'll rebase and work on the comments this weekend! |
You're awesome, @RachelWyatt ! |
Box can now be created with following traits: - Reviewed, Design in Progress, Designed, Research in Progress, Researched, Assembly in Progress, Assembled, Shipping in Progress, Shipped, Follow Up in Progress, Followed Up
2fb7ad3
to
7d1765b
Compare
…igned_needs_research
Resolves #310
Description
Adds factory bot traits for a cleaner test suite and faster item creation.
The following traits are available:
Inventory Type
Creates an inventory type with
research_required
set appropriately to true or false.Box Item:
Creates a box item and the necessary inventory type (using inventory_type traits), and associates box item with inventory type.
Box Request:
Box:
Sets the state as appropriate and adds any needed fields for validity. Note that each successive trait inherits from the one before it, so
create(:box, :design_in_progress)
will also have all the associations ascreate(:box, :reviewed)
, even though they aren't all explicitly stated in the factory trait.Type of change
How Has This Been Tested?
The box_spec tests all pass as expected using the created FactoryBot traits.