Allow a hard limit on number of specific items per request - #5386
Open
embarnard wants to merge 18 commits into
Open
Allow a hard limit on number of specific items per request#5386embarnard wants to merge 18 commits into
embarnard wants to merge 18 commits into
Conversation
embarnard
marked this pull request as draft
September 12, 2025 19:36
embarnard
commented
Sep 13, 2025
| tabindex="-1" | ||
| data-bs-backdrop="static" | ||
| data-confirmation-target="modal"> | ||
| </div> |
Collaborator
Author
There was a problem hiding this comment.
just added a missing div in this file, rest of the changes are just spacing from auto-format
embarnard
commented
Sep 13, 2025
| data-confirmation-pre-check-path-value="<%= validate_partners_requests_path(format: :json) %>"> | ||
| <div class="card-body"> | ||
|
|
||
| <% if @errors.present? %> |
Collaborator
Author
There was a problem hiding this comment.
removed this error message in favor of flash messages instead
embarnard
commented
Sep 13, 2025
| item_ids = item_requests.map(&:item_id) | ||
| if item_ids.uniq.length != item_ids.length | ||
| errors.add(:item_requests, "should have unique item_ids") | ||
| errors.add(:base, "Please ensure a single unit is selected for each item that supports it") |
Collaborator
Author
There was a problem hiding this comment.
changed to :base since we are printing from the error.full_messages now so that it won't include the ugly and confusing item_requests before the error message
embarnard
commented
Sep 13, 2025
| .joins(profile: :counties) | ||
| .group(:id) | ||
| .pluck(Arel.sql("partners.id, STRING_AGG(counties.name, '; ' ORDER BY counties.region, counties.name) AS county_list")) | ||
| .pluck(Arel.sql("partners.id, STRING_AGG(counties.name, '; ' ORDER BY LOWER(counties.region), LOWER(counties.name)) AS county_list")) |
Collaborator
Author
There was a problem hiding this comment.
have no idea why this started failing in this PR but if a region or name is lower case and another is uppercase of the same letter the uppercase will take precedence in the order
embarnard
commented
Sep 13, 2025
| pre_existing_entry = items[input_item['item_id']] | ||
|
|
||
| if pre_existing_entry | ||
| if pre_existing_entry.request_unit != input_item['request_unit'] |
Collaborator
Author
There was a problem hiding this comment.
removed this because we basically add the same error in the model
awwaiid
self-requested a review
September 14, 2025 13:44
embarnard
marked this pull request as ready for review
September 14, 2025 13:49
Collaborator
Resolves conflicts from the removal of the enable_packs feature flag: - partners/requests_controller: keep main's flag-free fetch_items, keep this branch's includes(:request_units) to avoid the N+1 - items/_form: keep the new request limit field, drop the flag guard - items_controller_spec: keep this branch's rewritten request-unit tests, drop the Flipper.enable call - partners/requests_system_spec: take main's flattened tests (the packs on/off contexts are gone with the flag) while keeping this branch's expected message, "Please select a unit for item 1." db/schema.rb regenerated so the new columns sort alphabetically, and the model/factory/spec schema annotations updated to match.
This branch changed the partner CSV export's county ordering to sort case-insensitively, which has nothing to do with per-item request limits; the author noted at the time they could not tell why it had started failing. It is no longer needed: the export, partner request, county, county profile and distributions-by-county specs all pass without it (101 examples), so whatever it was compensating for has since been fixed on main. Dropping it keeps the branch to its own subject. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AoUPX63nd1LSkKDPEiQ7jt
The flash built the message with String#capitalize, which upcases the
first character and downcases everything after it. That mangled the
item names and the sentence the issue asks for: a request over the
limit read
Kids (size 1): you requested 100, but are limited to 50.
instead of the specified "[Item name]: You requested ... but are
limited to ...". With several errors joined by "; ", every message
after the first was flattened the same way.
upcase_first leaves the rest of the string alone, so the messages that
already begin with a capital keep their names intact and the ones that
begin with a unit name ("flat is not a supported unit type") still get
their first letter raised, which is what capitalize was there for.
The system spec expected the mangled "item 1" text; it now expects the
item's real name.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AoUPX63nd1LSkKDPEiQ7jt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #5269
Description
request_limittoItemUnitandunit_request_limittoItemfields to models"[Item name]: You requested [amount requested] [units if applicable], but are limited to [request limit] [units if applicable].
Type of change
How Has This Been Tested?
Screenshots
items/:id/editpage/partners/requestspageremoved this error message from
/partners/requestspage