Skip to content

Conversation

@awwaiid
Copy link
Collaborator

@awwaiid awwaiid commented May 11, 2025

First pass at removing request_item everywhere.

  • Main reads on the Bank's Request controller/view
  • Update distribution PDF to read from item_request
  • Update confirmation email to read from item_request
  • Fix specs to make the correct mock data

@awwaiid awwaiid marked this pull request as ready for review May 11, 2025 15:48
@awwaiid awwaiid requested a review from dorner May 11, 2025 15: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.

Passes functional check. On to @dorner for technical comments.

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.

Minor suggestion, otherwise looks good. 🎉

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.

one last miss!

def name_with_unit(quantity_override = nil)
if Flipper.enabled?(:enable_packs) && request_unit.present?
"#{item&.name || name} - #{request_unit.pluralize(quantity_override || quantity.to_i)}"
"#{item_name} - #{request_unit.pluralize(quantity_override || quantity.to_i)}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you forgot to use the new method here? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is one of the display helpers:

  • ItemRequest#item_name gives the item's name and falls back to the local copy
  • ItemRequest#quantity_with_units shows the qty and units but not the name
  • ItemRequest#name_with_unit shows the name and the unit but not qty

@janeewheatley
Copy link
Collaborator

@awwaiid @dorner does this one still need more work?

@janeewheatley janeewheatley requested a review from dorner January 13, 2026 19:50
@dorner
Copy link
Collaborator

dorner commented Jan 23, 2026

@janeewheatley @awwaiid yep, my most recent comments have not been addressed.

@awwaiid
Copy link
Collaborator Author

awwaiid commented Jan 25, 2026

@dorner mmmm I think the one you mentioned shouldn't or can't use the other display helper (and this is a different display helper). Eh?

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.

5 participants