-
-
Notifications
You must be signed in to change notification settings - Fork 473
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
Show units column and custom units in banks' request view #4539
Show units column and custom units in banks' request view #4539
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.
Overall looks good - had one suggestion re the tests.
@cielf it's good for you to test out.
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 good to me!
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! We'll merge it after the deploy today probably.
@@ -65,6 +65,11 @@ | |||
<tr> | |||
<th>Item</th> | |||
<th>Quantity</th> | |||
<% custom_units = Flipper.enabled?(:enable_packs) && |
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.
Nice way to do this, setting the custom_units
to both indicate that the feature is enabled and that there is anything worth displaying.
Question about requested changes: I made and pushed the change, hit the "Resolve conversation" button, and re-requested a review from @dorner. I noticed that the PR still says "Changes requested" and I was wondering is there anything else I have to do, something that I missed, or will that go away once Daniel reviews the PR? |
I believe the changes requested stay up until that reviewer has ok'd them. |
All good here, thanks! |
Ran into a problem with this one in staging testing. We can still put it to prod since we are not turning on the flag until we've done an end to end full test. It looks like the first instance of a particular item's item request in the list is determining how the units are displayed for all of that item's item requests. Have put in a proto-issue. |
@norrismei: Your PR |
Resolves #4402
Description
Right now when a bank views a request, if custom units is enabled for the bank and there is an item with custom units (e.g. Packs) in the request, the custom unit does not show up next to quantity. This PR adds a units column if applicable and displays the custom unit for any items that have a request unit.
Type of change
How Has This Been Tested?
I created new tests for the behaviors I was expecting. I'm new to Rspec so feel free to point things out that can be improved. Are there any other tests that you feel should be included?
To manually test the change, I first signed in as a bank admin, navigated to the Requests, and clicked one to see in detail and verify there is no units column.
Then I pointed my browser to
http://localhost:3000/flipper
and logged inuserid: admin
password: password
I added the feature
enable_packs
and fully enabled it.Back on the bank's view of the request, I refreshed the page and the
Units (if applicable)
column appeared, as well as the custom unit ofPacks
for Pads.When I went back to Flipper and deleted the
enable_packs
feature and refreshed the bank's page, theUnits (if applicable)
column disappeared as expected.Screenshots
Before change (no column)
After change (column, plus custom unit)