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

Bank Org Request Export - Include Custom Units #4608

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

awwaiid
Copy link
Collaborator

@awwaiid awwaiid commented Aug 24, 2024

Implements #4405, adding custom units to request export.

  • This does not change the general output, which only includes items that are part of some exported request
  • If any item is configured to have custom units, we now include a column for each custom unit and no-unit, whether any requests made use of those or not
  • Also, if the item's custom-unit was deleted but it is referenced, go ahead and add a column for it anyway
  • Significantly dumbed down the corresponding spec

@awwaiid awwaiid requested review from dorner and cielf August 24, 2024 22:04
@awwaiid awwaiid marked this pull request as draft August 24, 2024 22:05
@awwaiid awwaiid linked an issue Aug 24, 2024 that may be closed by this pull request
3 tasks
@awwaiid awwaiid marked this pull request as ready for review August 24, 2024 22:42
@awwaiid awwaiid added this to the Request Units (Packs) milestone Aug 25, 2024
@cielf
Copy link
Collaborator

cielf commented Aug 27, 2024

Hrm. I did an export that looked clean, but then I added a request that just had pads with 233 units. I got a "no implicit conversion from nil to integer" on line 99 of export_request_service. (Note: I made partner group 1 have all the categories, and gave pads a category of Period Supplies first).

@awwaiid
Copy link
Collaborator Author

awwaiid commented Sep 1, 2024

@cielf Fixed! The issue was similar to the one in another PR, where "some item -" was being used instead of "some item" when the default-unit was selected.


# It's possible that the unit is no longer valid, so we'd
# add that individually
if item_request.request_unit.present? && !item.request_units.pluck(:name).include?(item_request.request_unit)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we grab all the request_units at the top rather than doing a separate DB query per item here?

"#{item_request.name} - #{item_request.request_unit}"
else
item_request.name
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment in the previous PR - maybe this method actually should go into the Item class.

)
end

let!(:request_4t) do
# create(:unit, organization: org, name: "pack")
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented line plz

Comment on lines +107 to +108
request_3t.partner.name,
request_3t.status.humanize,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we hardcode these 2 values? Same with the other requests.

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] #10 Add custom request units to Request export
3 participants