[4409] Only show fulfillment column if partner has default location #4466
Conversation
| end | ||
| end | ||
|
|
||
| context 'When partner or organization does not have a default storage location' do |
There was a problem hiding this comment.
This is a nitpick, but this should be 'When neither partner nor organization has a default storage location' -- i.e. that it has to be both the partner doesn't and the organization doesn't. Your description means, instead, if the partner doesn't have it or the organization doesn't have it, which is not quite the same thing.
There was a problem hiding this comment.
Fixed it here 90098cd
app/views/requests/show.html.erb
Outdated
| @@ -65,7 +65,11 @@ | |||
| <tr> | |||
| <th>Item</th> | |||
| <th>Quantity</th> | |||
| <th>Fulfillment Location Inventory</th> | |||
| <% default_storage_location = @request.organization.default_storage_location || | |||
| @request.partner.default_storage_location_id %> | |||
There was a problem hiding this comment.
I found one more thing on my last go-through -- and this is extremely nitpicky.
I think it would be better if we switched the two clauses in this or?
Why? You are only using the existence of a default storage location, but, If someone later decides to use the default storage location (say we decide to change the title) - it should match what we are using for getting the on_hand_for_location. In the probably rare case where the organization and partner both have default locations, we use the partner location.
There was a problem hiding this comment.
Sure. I fixed it here 9e77b48
There was a problem hiding this comment.
Getting very close!
There is another test in request_system_spec failing (at about line 153). Without looking into it too closely, I suspect the partner for that test has no default location. Please investigate and address. Also check to see if there are any other possibly related tests failing.
napster235
force-pushed
the
4409-only_show_fulfillment_column_if_partner_has_default_location
branch
from
cfd2265
to
a5c908e
Compare
|
@napster235: Your PR |
Resolves #4409
Description
Type of change
How Has This Been Tested?
Specs were added for the new functionality