-
-
Notifications
You must be signed in to change notification settings - Fork 491
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
Partner request units display (packs-4) #4399 #4531
Conversation
And also fix traits, which seem like they shouldn't have worked this way. [#4399]
|
||
trait :fulfilled 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.
I'm not sure these ever worked since they weren't nested inside of the factory
block.
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.
If they didn't work, can we just delete them?
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.
Sorry they WORK but they were being defined globally instead of scoped to only the Requests factory. So any other factory could have been using these traits on accident, though lucky us nothing else is using them. I verified that all of them ARE being used.
There are unfortunately existing specs which don't pass in valid data for building out the item_requests, so here we make it an explicit trait that must be requested. [#4399]
trait :with_item_requests do | ||
after(:build) do |request| |
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 ended up making this an explicit trait so that it doesn't interfere with existing specs that don't pass in valid data.
It is, alas, a tad hard to properly test this one out until packs-3 is also in place. |
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.
One silly question. :)
|
||
trait :fulfilled 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.
If they didn't work, can we just delete them?
@cielf any issue merging this? It seems really safe since the only behavior change is locked behind the feature flag. |
@awwaiid: Your PR |
Implements #4399, aka Packs-4.
Validated through interactive testing locally with mock data, along with unit tests.