-
-
Notifications
You must be signed in to change notification settings - Fork 499
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
4745: Disable deactivate button for storage locations not deactivatable #4775
4745: Disable deactivate button for storage locations not deactivatable #4775
Conversation
c94abee
to
3b71eca
Compare
3b71eca
to
f6028f8
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.
Looks great from a functional pov. Over to @dorner for technical comments.
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.
Looks pretty good - had a suggestion.
<td><%= number_to_currency(storage_location.inventory_total_value_in_dollars(@inventory)) %></td> | ||
<td class="text-right"> | ||
<%= view_button_to storage_location %> | ||
<%= edit_button_to edit_storage_location_path(storage_location) %> | ||
<% if storage_location.discarded? %> | ||
<%= reactivate_button_to storage_location_reactivate_path(storage_location), { confirm: confirm_reactivate_msg(storage_location.name) } %> | ||
<% else %> | ||
<%= deactivate_button_to storage_location_deactivate_path(storage_location), { confirm: confirm_deactivate_msg(storage_location.name) } %> | ||
<%= deactivate_button_to storage_location_deactivate_path(storage_location), | ||
{ confirm: confirm_deactivate_msg(storage_location.name), enabled: @inventory_item_totals[storage_location.id].zero? } %> |
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.
Calculating the inventory total is super quick since it's all in memory. I think I'd rather not have the extra instance variable and do this check live.
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.
Removed that new instance variable.
…e-location-is-deactibatable
Thanks! |
@coalest: Your PR |
Resolves #4745
Description
We should only be able to deactivate storage locations with no inventory items.
Type of change
I guess a new feature?
How Has This Been Tested?
Added request specs for both paths (when
read_events
feature flag is enabled or disabled).Screenshots
Here are before and after screenshots. Note the red deactivate buttons become disabled for locations with nonzero inventory items.
Before:
After: