Skip to content
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

#4398: Support units in requests #4510

Merged
merged 12 commits into from
Aug 19, 2024
Merged

#4398: Support units in requests #4510

merged 12 commits into from
Aug 19, 2024

Conversation

dorner
Copy link
Collaborator

@dorner dorner commented Jul 12, 2024

Resolves #4398

Description

This shows a Units field if the partner's organization has any custom request units.

If the selected item has any request units, the Unit drop-down is shown and the first custom request unit is selected. Otherwise, the drop-down is not shown.

If the enable_packs flag is off, no behavior change is shown.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Local testing. Still needs specs created.

Screenshots

image

@dorner
Copy link
Collaborator Author

dorner commented Jul 12, 2024

If anyone wants to grab this and finish writing the specs before I can get to it, feel free :)

# Conflicts:
#	db/migrate/20240704214509_backfill_partner_child_requested_items.rb
@dorner dorner changed the title [WIP] #4398: Support units in requests #4398: Support units in requests Jul 19, 2024
@dorner dorner marked this pull request as ready for review July 19, 2024 20:57
@dorner dorner requested review from awwaiid and cielf July 19, 2024 20:57
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. I'm not sure if that the first custom unit should be the default, or if it should be units. I think it's a question for the stakeholders. (The initial vision forced a choice)

cielf
cielf previously requested changes Jul 19, 2024
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I was just playing around, and this sequence causes an Oops! Something went wrong.

Pick an item that does have units (e.g. Pads in our seed). Fill in the quantity. Then change to an item that does not have units and submit.

@awwaiid awwaiid added this to the Request Units (Packs) milestone Jul 21, 2024
@dorner
Copy link
Collaborator Author

dorner commented Jul 26, 2024

@cielf pushed the fix!

@cielf
Copy link
Collaborator

cielf commented Jul 27, 2024

The test that is failing has packs in its description, so might be a real failure?

@dorner
Copy link
Collaborator Author

dorner commented Aug 2, 2024

Should be fixed now!

@cielf
Copy link
Collaborator

cielf commented Aug 3, 2024

LGTM @awwaiid could you take a look?

Comment on lines 16 to 19
# hash of (item ID => hash of (request unit name => request unit plural name))
@item_units = current_partner.organization.items.to_h do |i|
[i.id, i.request_units.to_h { |u| [u.name, u.name.pluralize] }]
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is identical to the one in create please extract it into a private method.

<td>
<%= field.label :item_id, "Item Requested", {class: 'sr-only'} %>
<%= field.select :item_id, @requestable_items, {include_blank: 'Select an item'}, {class: 'form-control'} %>
<%= field.select :item_id, @requestable_items, {include_blank: 'Select an item'},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here we already only show the @requestable_items in the item select drop-down, so that already narrows down what we might show in the units drop down. So the generated @item_units having all possible items it works fine, but could be narrowed down to building off of @requestable_items

@cielf
Copy link
Collaborator

cielf commented Aug 5, 2024

Further thoughts on the default -- if we have multiple custom units the first one may or may not be the most used. I'm wondering if we should default at all. (Or we may yet need to add a default unit indicator). We'll figure this out after talking to the business on Wednesday.

@cielf
Copy link
Collaborator

cielf commented Aug 8, 2024

Further thoughts on the default -- if we have multiple custom units the first one may or may not be the most used. I'm wondering if we should default at all. (Or we may yet need to add a default unit indicator). We'll figure this out after talking to the business on Wednesday.

@dorner We talked to the business Wednesday, and they said they'd rather have the units not initially selected, but that, of course, they have to select something. This is because a single bank will have partners who do packs and partners that don't and they want to avoid a default that would be a mistake.

@dorner
Copy link
Collaborator Author

dorner commented Aug 11, 2024

All comments should be addressed! @awwaiid @cielf

Copy link
Collaborator

@awwaiid awwaiid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good and manual testing also worked; I triggered a re-run of the failing test in case it is a flake.

@awwaiid
Copy link
Collaborator

awwaiid commented Aug 19, 2024

@dorner I keep trying combinations of re-running these failing tests and trying to get it to fail locally and so far no luck :(

@awwaiid
Copy link
Collaborator

awwaiid commented Aug 19, 2024

Ah, I got it to fail and then fixed it!

Copy link
Collaborator

@awwaiid awwaiid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sightly tricky since I added a bit, but looks good :)

@awwaiid awwaiid merged commit c2782e7 into main Aug 19, 2024
38 checks passed
@awwaiid awwaiid deleted the 4398-pack-requests branch August 19, 2024 15:03
Copy link
Contributor

@dorner: Your PR #4398: Support units in requests is part of today's Human Essentials production release: 2024.08.25.
Thank you very much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PACKS] #3 Allowing the selection of custom request units in partner request
3 participants