Add row for grand totals to storage locations index view #4768
Conversation
tatheerf02
force-pushed
the
4520-storage-locations-totals
branch
2 times, most recently
from
751fa34
to
a46bd92
Compare
| def inventory_total_quantity(inventory = nil) | ||
| if inventory | ||
| inventory.quantity_for(storage_location: id) | ||
| else | ||
| size | ||
| end | ||
| end |
There was a problem hiding this comment.
Extracted this conditional to a method in the model to prevent duplication between the ERB files - do we like this approach?
There was a problem hiding this comment.
I'd rather just add a parameter to sizewhere you can pass the inventory in to avoid reloading it. I.e.
def size(inventory=nil)
inventory ||= View::Inventory.new(organization_id)
inventory.quantity_for(storage_location: id)
end
| within "tbody tr:last-child" do | ||
| expect(page).to have_content(100) # The expected total inventory | ||
| expect(page).to have_content("$100.00") # The expected total fair market value | ||
| end |
There was a problem hiding this comment.
Is there a better way to test the totals show up as expected without writing a system test?
There was a problem hiding this comment.
Yes! You can look at some other request tests to see how it parses the rendered body and checks for elements.
There was a problem hiding this comment.
Hey @tatheerf02 -- I'm noticing that the totals are not aligned with the inventory and fair market value -- I expect that the values for each storage location are not right-aligned as they should be.
Can you take care of that? It fits here, but I can put it to a new issue if need be.
Hey @cielf! Sorry I'm a lil confused, I see that all column values are currently left-aligned, and the grand totals follow that pattern. Are you suggesting they should all be right-aligned instead? Thanks! |
@tatheerf02 Yup -- that's exactly what I'm suggesting. Thank you. |
tatheerf02
force-pushed
the
4520-storage-locations-totals
branch
from
6e3b75b
to
7ae8045
Compare
|
@cielf How do you feel about something like this? I right-aligned the titles for those columns as well so they wouldn't look mismatched |
|
Looks very good to me! @dorner -- Do you have any technical concerns? |
| def inventory_total_quantity(inventory = nil) | ||
| if inventory | ||
| inventory.quantity_for(storage_location: id) | ||
| else | ||
| size | ||
| end | ||
| end |
There was a problem hiding this comment.
I'd rather just add a parameter to sizewhere you can pass the inventory in to avoid reloading it. I.e.
def size(inventory=nil)
inventory ||= View::Inventory.new(organization_id)
inventory.quantity_for(storage_location: id)
end| @@ -92,6 +92,23 @@ | |||
| end | |||
| end | |||
|
|
|||
| it "displays the correct grand totals across all storage locations" do | |||
There was a problem hiding this comment.
This should be a request spec, not a system spec since there's no interaction. Request specs are a lot faster and more lightweight.
| within "tbody tr:last-child" do | ||
| expect(page).to have_content(100) # The expected total inventory | ||
| expect(page).to have_content("$100.00") # The expected total fair market value | ||
| end |
There was a problem hiding this comment.
Yes! You can look at some other request tests to see how it parses the rendered body and checks for elements.
Resolves #4520
Description
Display another row at the bottom of the storage locations index that aggregates the total inventory quantity and dollar value across all locations, taking into account any filters applied.
Type of change
How Has This Been Tested?
Added a new system test that ensures the row is displayed on the index with the correct total values.
Updates existing tests to account for the new row in the table
Screenshots
📸 Storage locations index (no filters)
📸 Storage locations index (with filters)