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

Custom Units / Packs fixes #5099

Merged
merged 25 commits into from
Mar 27, 2025
Merged

Custom Units / Packs fixes #5099

merged 25 commits into from
Mar 27, 2025

Conversation

awwaiid
Copy link
Collaborator

@awwaiid awwaiid commented Mar 16, 2025

Fixes for #5096

  • Allow a bank to have NO custom-units without breaking requests (with spec)
  • Pluralize units on request confirmation
  • Re-organize units display on the request detail page ("Adult Briefs (Medium/Large) - 33 snorves")
  • Verify (maybe I fixed?) that Partner's Request History page shows units
  • Capitalize units less
  • Allow banks to create requests on-behalf-of partners (with units)

@awwaiid awwaiid requested a review from cielf March 16, 2025 00:48
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.

"First Thing" from the issue is still going BOOM!

@awwaiid awwaiid requested a review from cielf March 25, 2025 19:18
@awwaiid
Copy link
Collaborator Author

awwaiid commented Mar 25, 2025

@cielf I had previously fixed the wrong bug, sorry! Please try again :)

Now also:

  • Removes forced-capitalization on units
  • Fixes item_request.name_with_units for deleted items

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.

Two more things....
1/ I just thought to test this -- you should not be able to enter unit as a custom unit, right? I don't know that it would break anything, but it's going to be confusing.
2/ Units is capitalized in the dropdown in request, and should not be.
Haven't tested the name with units issue live yet, but will.

@awwaiid
Copy link
Collaborator Author

awwaiid commented Mar 25, 2025

  • unit(s) is no longer allowed to be created as an organization unit (with spec)
  • "Unit" is now "unit" for the default in the drop-down

@awwaiid awwaiid requested a review from cielf March 25, 2025 23:31
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.

This looks pretty good to me, functionally (Good enough for our beta tester). Over to @dorner for technical comments!

@cielf cielf requested a review from dorner March 26, 2025 01:07
Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

Non-blocking comment. This is good to merge!

@dorner
Copy link
Collaborator

dorner commented Mar 27, 2025

@awwaiid can you look at the failing tests? I tried rerunning but they're still failing.

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.

'Sall good. We'll need to add enable_packs to flipper for prod/staging, and turn it on for staging - but those staging changes can be a separate PR.

@cielf cielf merged commit 5e80e11 into main Mar 27, 2025
23 checks passed
@cielf cielf deleted the packs-fixes branch March 27, 2025 14:28
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.

3 participants