From 78e753ee1c3c2f5dfad7cc2f9511e2d492114f34 Mon Sep 17 00:00:00 2001 From: Em Barnard-Shao Date: Fri, 12 Sep 2025 15:35:44 -0400 Subject: [PATCH 1/9] Allow a hard limit on number of specific items per request --- app/controllers/items_controller.rb | 12 +++++-- app/models/item.rb | 9 +++-- app/models/item_unit.rb | 11 +++--- app/views/items/_form.html.erb | 35 ++++++++++++++++++- ...12155420_add_request_limit_to_item_unit.rb | 6 ++++ db/schema.rb | 4 ++- spec/factories/item_units.rb | 11 +++--- spec/factories/items.rb | 1 + spec/models/item_spec.rb | 1 + spec/models/item_unit_spec.rb | 11 +++--- 10 files changed, 78 insertions(+), 23 deletions(-) create mode 100644 db/migrate/20250912155420_add_request_limit_to_item_unit.rb diff --git a/app/controllers/items_controller.rb b/app/controllers/items_controller.rb index c87e608e92..bcee6f87d2 100644 --- a/app/controllers/items_controller.rb +++ b/app/controllers/items_controller.rb @@ -177,14 +177,20 @@ def item_params :distribution_quantity, :visible_to_partners, :active, - :additional_info + :additional_info, + :unit_request_limit ) end def request_unit_ids - params.require(:item).permit(request_unit_ids: []).fetch(:request_unit_ids, []) + params.require(:item).permit(unit_ids: []).fetch(:unit_ids, []).reject(&:blank?) end + def request_unit_limits + (params.require(:item).permit(unit_limits: {})[:unit_limits] || {}).to_h + end + + # We need to update both the item and the request_units together and fail together def update_item if Flipper.enabled?(:enable_packs) @@ -198,7 +204,7 @@ def update_item_and_request_units begin Item.transaction do @item.save! - @item.sync_request_units!(request_unit_ids) + @item.sync_request_units!(request_unit_ids, request_unit_limits) end rescue return false diff --git a/app/models/item.rb b/app/models/item.rb index 15e7b1382c..a5989a8f00 100644 --- a/app/models/item.rb +++ b/app/models/item.rb @@ -13,6 +13,7 @@ # package_size :integer # partner_key :string # reporting_category :string +# unit_request_limit :integer # value_in_cents :integer default(0) # visible_to_partners :boolean default(TRUE), not null # created_at :datetime not null @@ -186,10 +187,12 @@ def default_quantity distribution_quantity || 50 end - def sync_request_units!(unit_ids) + def sync_request_units!(unit_ids, limits = {}) request_units.clear - organization.request_units.where(id: unit_ids).pluck(:name).each do |name| - request_units.create!(name:) + organization.request_units.where(id: unit_ids).each do |unit| + item_unit = request_units.create!(name: unit.name) + limit = limits[unit.id.to_s] + item_unit.update!(request_limit: limit.to_i) if limit.present? end end diff --git a/app/models/item_unit.rb b/app/models/item_unit.rb index d9453394d2..f8301f22bf 100644 --- a/app/models/item_unit.rb +++ b/app/models/item_unit.rb @@ -2,11 +2,12 @@ # # Table name: item_units # -# id :bigint not null, primary key -# name :string not null -# created_at :datetime not null -# updated_at :datetime not null -# item_id :bigint +# id :bigint not null, primary key +# name :string not null +# request_limit :integer +# created_at :datetime not null +# updated_at :datetime not null +# item_id :bigint # class ItemUnit < ApplicationRecord belongs_to :item diff --git a/app/views/items/_form.html.erb b/app/views/items/_form.html.erb index 8dcbafde25..99aa14ea55 100644 --- a/app/views/items/_form.html.erb +++ b/app/views/items/_form.html.erb @@ -44,10 +44,43 @@ <%= f.input_field :package_size, class: "form-control", min: 0 %> <% end %> + <%= f.input :name, label: "Request limit (individual items)", wrapper: :input_group do %> + <%= f.input_field :unit_request_limit, class: "form-control", min: 0 %> + <% end %> + <% if Flipper.enabled?(:enable_packs) %> <%= f.input :request_units, label: "Additional Custom Request Units" do %> - <%= f.association :request_units, as: :check_boxes, collection: current_organization.request_units, checked: selected_item_request_units(@item), label_method: :name, value_method: :id, class: "form-check-input" %> + <% current_organization.request_units.each do |unit| %> + <% item_unit = @item.request_units.find { |item_unit| item_unit.name == unit.name } %> + <% selected = item_unit.present? %> + +
+ <%= check_box_tag "item[unit_ids][]", unit.id, selected, id: "unit_#{unit.id}", class: "form-check-input" %> + <%= label_tag "unit_#{unit.id}", unit.name, class: "form-check-label me-2" %> + + <%= number_field_tag "item[unit_limits][#{unit.id}]", + item_unit&.request_limit, + class: "form-control d-inline-block w-auto", + min: 0, + placeholder: "Limit", + disabled: !selected %> +
+ <% end %> + + + <%= hidden_field_tag "item[unit_ids][]", "" %> <% end %> + + <% end %> <%= f.input :visible, label: "Item is Visible to Partners?", wrapper: :input_group do %> diff --git a/db/migrate/20250912155420_add_request_limit_to_item_unit.rb b/db/migrate/20250912155420_add_request_limit_to_item_unit.rb new file mode 100644 index 0000000000..6c023a8ad0 --- /dev/null +++ b/db/migrate/20250912155420_add_request_limit_to_item_unit.rb @@ -0,0 +1,6 @@ +class AddRequestLimitToItemUnit < ActiveRecord::Migration[8.0] + def change + add_column :item_units, :request_limit, :integer, default: nil, null: true + add_column :items, :unit_request_limit, :integer, default: nil, null: true + end +end diff --git a/db/schema.rb b/db/schema.rb index b881fa8003..622b814625 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2025_08_11_094943) do +ActiveRecord::Schema[8.0].define(version: 2025_09_12_155420) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -387,6 +387,7 @@ t.bigint "item_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.integer "request_limit" t.index ["item_id"], name: "index_item_units_on_item_id" end @@ -408,6 +409,7 @@ t.integer "item_category_id" t.text "additional_info" t.string "reporting_category" + t.integer "unit_request_limit" t.index ["kit_id"], name: "index_items_on_kit_id" t.index ["organization_id"], name: "index_items_on_organization_id" t.index ["partner_key"], name: "index_items_on_partner_key" diff --git a/spec/factories/item_units.rb b/spec/factories/item_units.rb index 044955874d..c3918d1de9 100644 --- a/spec/factories/item_units.rb +++ b/spec/factories/item_units.rb @@ -2,11 +2,12 @@ # # Table name: item_units # -# id :bigint not null, primary key -# name :string not null -# created_at :datetime not null -# updated_at :datetime not null -# item_id :bigint +# id :bigint not null, primary key +# name :string not null +# request_limit :integer +# created_at :datetime not null +# updated_at :datetime not null +# item_id :bigint # FactoryBot.define do factory :item_unit do diff --git a/spec/factories/items.rb b/spec/factories/items.rb index 1f87ad94ea..08132bfc39 100644 --- a/spec/factories/items.rb +++ b/spec/factories/items.rb @@ -13,6 +13,7 @@ # package_size :integer # partner_key :string # reporting_category :string +# unit_request_limit :integer # value_in_cents :integer default(0) # visible_to_partners :boolean default(TRUE), not null # created_at :datetime not null diff --git a/spec/models/item_spec.rb b/spec/models/item_spec.rb index 5f9e28e485..7e49ad386d 100644 --- a/spec/models/item_spec.rb +++ b/spec/models/item_spec.rb @@ -13,6 +13,7 @@ # package_size :integer # partner_key :string # reporting_category :string +# unit_request_limit :integer # value_in_cents :integer default(0) # visible_to_partners :boolean default(TRUE), not null # created_at :datetime not null diff --git a/spec/models/item_unit_spec.rb b/spec/models/item_unit_spec.rb index be63d21e09..779480ae43 100644 --- a/spec/models/item_unit_spec.rb +++ b/spec/models/item_unit_spec.rb @@ -2,11 +2,12 @@ # # Table name: item_units # -# id :bigint not null, primary key -# name :string not null -# created_at :datetime not null -# updated_at :datetime not null -# item_id :bigint +# id :bigint not null, primary key +# name :string not null +# request_limit :integer +# created_at :datetime not null +# updated_at :datetime not null +# item_id :bigint # RSpec.describe ItemUnit, type: :model do context "Validations >" do From 47bd1e29b59b113c53f247667d98210476f55d7a Mon Sep 17 00:00:00 2001 From: Em Barnard-Shao Date: Sat, 13 Sep 2025 10:51:11 -0400 Subject: [PATCH 2/9] Add validation for item request limit on partners new request page --- .../partners/requests_controller.rb | 3 +- .../partners/request_create_service.rb | 17 +++++++++ .../individuals_requests/new.html.erb | 37 ++++++++++--------- app/views/partners/requests/new.html.erb | 5 --- 4 files changed, 38 insertions(+), 24 deletions(-) diff --git a/app/controllers/partners/requests_controller.rb b/app/controllers/partners/requests_controller.rb index 24bfe1cf67..af62703659 100644 --- a/app/controllers/partners/requests_controller.rb +++ b/app/controllers/partners/requests_controller.rb @@ -43,6 +43,7 @@ def create else @partner_request = create_service.partner_request @errors = create_service.errors + flash[:error] = @errors.full_messages.join(", ").capitalize + "." if @errors.present? fetch_items @@ -84,7 +85,7 @@ def fetch_items # hash of (item ID => hash of (request unit name => request unit plural name)) item_ids = @requestable_items.to_h.values if item_ids.present? - @item_units = Item.where(id: item_ids).to_h do |i| + @item_units = Item.where(id: item_ids).includes(:request_units).to_h do |i| [i.id, i.request_units.to_h { |u| [u.name, u.name.pluralize] }] end end diff --git a/app/services/partners/request_create_service.rb b/app/services/partners/request_create_service.rb index 3703cfd9e0..e4688c6aba 100644 --- a/app/services/partners/request_create_service.rb +++ b/app/services/partners/request_create_service.rb @@ -105,6 +105,23 @@ def populate_item_request(partner_request) }.compact end + # Validate if request quantity exceeds its request limit + partner_request.request_items.each do |ir| + item = Item.find(ir["item_id"]) + unit_type = ir["request_unit"] + quantity_requested = ir["quantity"].to_i + + limit = if unit_type == "" + item.unit_request_limit + else + item.request_units.where(name: unit_type)&.first&.request_limit + end + + if limit.present? && (quantity_requested > limit) + errors.add(:base, "please do not exceed #{limit} #{unit_type.pluralize} for #{item.name}") + end + end + partner_request end diff --git a/app/views/partners/individuals_requests/new.html.erb b/app/views/partners/individuals_requests/new.html.erb index 605418485a..e9ca8f92eb 100644 --- a/app/views/partners/individuals_requests/new.html.erb +++ b/app/views/partners/individuals_requests/new.html.erb @@ -24,29 +24,29 @@
+ data-controller="confirmation" + data-confirmation-pre-check-path-value="<%= validate_partners_individuals_requests_path(format: :json) %>">
<% if @errors.present? %> <%= render partial: 'partners/requests/error' %> <% end %> - <%= simple_form_for @request, url: partners_individuals_requests_path, html: {role: 'form', class: 'form-horizontal'}, - data: {controller: 'form-input', confirmation_target: "form"} do |form| %> + <%= simple_form_for @request, url: partners_individuals_requests_path, html: { role: 'form', class: 'form-horizontal' }, + data: { controller: 'form-input', confirmation_target: "form" } do |form| %> <%= form.input :comments, label: "Comments:", as: :text, class: "form-control", wrapper: :input_group %> - - - - + + + + - <%= render 'partners/individuals_requests/item_request', form: form %> + <%= render 'partners/individuals_requests/item_request', form: form %>
Item RequestedNumber of Individuals
Item RequestedNumber of Individuals
@@ -65,15 +65,16 @@
<% end %> - <%# Confirmation modal: See confirmation_controller.js for how this gets displayed %> - <%# and app/controllers/partners/individuals_requests_controller.rb#validate for how it gets populated. %> -
diff --git a/app/views/partners/requests/new.html.erb b/app/views/partners/requests/new.html.erb index 8392e17828..7e79dbbf33 100644 --- a/app/views/partners/requests/new.html.erb +++ b/app/views/partners/requests/new.html.erb @@ -27,11 +27,6 @@ data-controller="confirmation" data-confirmation-pre-check-path-value="<%= validate_partners_requests_path(format: :json) %>">
- - <% if @errors.present? %> - <%= render partial: 'partners/requests/error' %> - <% end %> - <%= simple_form_for @partner_request, url: partners_requests_path(@partner_request), html: {role: 'form', class: 'form-horizontal'}, method: :post, data: { controller: 'form-input', confirmation_target: "form" } do |form| %> <%= form.input :comments, label: "Comments:", as: :text, class: "form-control", wrapper: :input_group %> From 483afd0582f8ce37e4acb69da58adda9e71bd7ae Mon Sep 17 00:00:00 2001 From: Em Barnard-Shao Date: Sat, 13 Sep 2025 11:11:23 -0400 Subject: [PATCH 3/9] Fix validation for individual unit and change error message --- app/controllers/partners/requests_controller.rb | 2 +- app/services/partners/request_create_service.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/partners/requests_controller.rb b/app/controllers/partners/requests_controller.rb index af62703659..22f5628122 100644 --- a/app/controllers/partners/requests_controller.rb +++ b/app/controllers/partners/requests_controller.rb @@ -43,7 +43,7 @@ def create else @partner_request = create_service.partner_request @errors = create_service.errors - flash[:error] = @errors.full_messages.join(", ").capitalize + "." if @errors.present? + flash[:error] = @errors.full_messages.join("; ").capitalize + "." if @errors.present? fetch_items diff --git a/app/services/partners/request_create_service.rb b/app/services/partners/request_create_service.rb index e4688c6aba..c139d192f3 100644 --- a/app/services/partners/request_create_service.rb +++ b/app/services/partners/request_create_service.rb @@ -105,20 +105,20 @@ def populate_item_request(partner_request) }.compact end - # Validate if request quantity exceeds its request limit + # Validate if request quantity exceeds the request limit for the item and unit type partner_request.request_items.each do |ir| item = Item.find(ir["item_id"]) unit_type = ir["request_unit"] quantity_requested = ir["quantity"].to_i - limit = if unit_type == "" + limit = if unit_type.blank? item.unit_request_limit else item.request_units.where(name: unit_type)&.first&.request_limit end if limit.present? && (quantity_requested > limit) - errors.add(:base, "please do not exceed #{limit} #{unit_type.pluralize} for #{item.name}") + errors.add(:base, "#{item.name}: You requested #{quantity_requested} #{unit_type.pluralize}, but are limited to #{limit} #{unit_type.pluralize}") end end From 0b665d6a1fb0f484daa0e69aa1a9166e419a9add Mon Sep 17 00:00:00 2001 From: Em Barnard-Shao Date: Sat, 13 Sep 2025 11:24:11 -0400 Subject: [PATCH 4/9] Rubo linter --- app/controllers/items_controller.rb | 3 +-- app/models/item.rb | 2 +- app/services/partners/request_create_service.rb | 8 ++++---- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/controllers/items_controller.rb b/app/controllers/items_controller.rb index bcee6f87d2..9d9a2006bd 100644 --- a/app/controllers/items_controller.rb +++ b/app/controllers/items_controller.rb @@ -183,14 +183,13 @@ def item_params end def request_unit_ids - params.require(:item).permit(unit_ids: []).fetch(:unit_ids, []).reject(&:blank?) + params.require(:item).permit(unit_ids: []).fetch(:unit_ids, []).compact_blank end def request_unit_limits (params.require(:item).permit(unit_limits: {})[:unit_limits] || {}).to_h end - # We need to update both the item and the request_units together and fail together def update_item if Flipper.enabled?(:enable_packs) diff --git a/app/models/item.rb b/app/models/item.rb index a5989a8f00..e85df89343 100644 --- a/app/models/item.rb +++ b/app/models/item.rb @@ -189,7 +189,7 @@ def default_quantity def sync_request_units!(unit_ids, limits = {}) request_units.clear - organization.request_units.where(id: unit_ids).each do |unit| + organization.request_units.where(id: unit_ids).find_each do |unit| item_unit = request_units.create!(name: unit.name) limit = limits[unit.id.to_s] item_unit.update!(request_limit: limit.to_i) if limit.present? diff --git a/app/services/partners/request_create_service.rb b/app/services/partners/request_create_service.rb index c139d192f3..0d7a5b6723 100644 --- a/app/services/partners/request_create_service.rb +++ b/app/services/partners/request_create_service.rb @@ -112,10 +112,10 @@ def populate_item_request(partner_request) quantity_requested = ir["quantity"].to_i limit = if unit_type.blank? - item.unit_request_limit - else - item.request_units.where(name: unit_type)&.first&.request_limit - end + item.unit_request_limit + else + item.request_units.where(name: unit_type)&.first&.request_limit + end if limit.present? && (quantity_requested > limit) errors.add(:base, "#{item.name}: You requested #{quantity_requested} #{unit_type.pluralize}, but are limited to #{limit} #{unit_type.pluralize}") From 3a369a8cabe1ea90809daffe20c84a4bfb923cc2 Mon Sep 17 00:00:00 2001 From: Em Barnard-Shao Date: Sat, 13 Sep 2025 14:23:38 -0400 Subject: [PATCH 5/9] Fix tests --- .../exports/export_partners_csv_service.rb | 2 +- app/views/items/_form.html.erb | 27 +++--- spec/controllers/items_controller_spec.rb | 95 +++++++++++++------ 3 files changed, 81 insertions(+), 43 deletions(-) diff --git a/app/services/exports/export_partners_csv_service.rb b/app/services/exports/export_partners_csv_service.rb index d06876c363..0f3c4faeeb 100644 --- a/app/services/exports/export_partners_csv_service.rb +++ b/app/services/exports/export_partners_csv_service.rb @@ -186,7 +186,7 @@ def county_list_by_regions partners .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")) .to_h end diff --git a/app/views/items/_form.html.erb b/app/views/items/_form.html.erb index 99aa14ea55..a8b3dad8bc 100644 --- a/app/views/items/_form.html.erb +++ b/app/views/items/_form.html.erb @@ -62,25 +62,14 @@ item_unit&.request_limit, class: "form-control d-inline-block w-auto", min: 0, - placeholder: "Limit", - disabled: !selected %> + placeholder: "limit", + disabled: !selected, + autocomplete: "off" %>
<% end %> - <%= hidden_field_tag "item[unit_ids][]", "" %> <% end %> - - <% end %> <%= f.input :visible, label: "Item is Visible to Partners?", wrapper: :input_group do %> @@ -108,3 +97,13 @@ <% end %> + + \ No newline at end of file diff --git a/spec/controllers/items_controller_spec.rb b/spec/controllers/items_controller_spec.rb index 96fad6b4bc..fffde25793 100644 --- a/spec/controllers/items_controller_spec.rb +++ b/spec/controllers/items_controller_spec.rb @@ -52,38 +52,76 @@ end end - context "request units" do + context "changing request units (item units)" do before(:each) { Flipper.enable(:enable_packs) } - let(:item) { create(:item, organization:) } - let(:unit) { create(:unit, organization:) } - it "should add new item's request units" do - expect(item.request_units).to be_empty - request = put :update, params: { id: item.id, item: { request_unit_ids: [unit.id] } } - expect(request).to redirect_to(items_path) - expect(response).not_to have_error - expect(item.request_units.reload.pluck(:name)).to match_array [unit.name] + + let!(:pack_unit) { create(:unit, organization: organization, name: "pack") } + let!(:box_unit) { create(:unit, organization: organization, name: "box") } + let(:item) { create(:item, organization: organization, unit_request_limit: 100) } + + context "when checking pack" do + let(:params) do + { + id: item.id, + item: { + unit_ids: [pack_unit.id.to_s, ""], + unit_limits: { pack_unit.id.to_s => "500" } + } + } + end + + it "adds pack item unit to item" do + expect(item.request_units).to be_empty + + put :update, params: params + expect(response).to redirect_to(items_path) + expect(response).not_to have_error + expect(item.request_units.reload.pluck(:name)).to match_array [pack_unit.name] + expect(item.request_units.first.request_limit).to eq 500 + end end - it "should remove item request units" do - # add an existing unit - create(:item_unit, item:, name: unit.name) - expect(item.request_units.size).to eq 1 - request = put :update, params: { id: item.id, item: { request_unit_ids: [""] } } - expect(response).not_to have_error - expect(request).to redirect_to(items_path) - expect(item.request_units.reload).to be_empty + context "when already has pack selected and unselects pack" do + let!(:item_unit){ create(:item_unit, item: item, request_limit: 500, name: pack_unit.name) } + let(:params) do + { + id: item.id, + item: { unit_ids: [""], unit_limits: {} } + } + end + + it "removes item request units" do + expect(item.request_units.size).to eq 1 + + put :update, params: params + expect(response).not_to have_error + expect(response).to redirect_to(items_path) + expect(item.request_units.reload).to be_empty + end end - it "should add and remove request units at the same time" do - # attach a different unit to the item - unit_to_remove = create(:unit, organization:) - create(:item_unit, item:, name: unit_to_remove.name) - expect(item.request_units.pluck(:name)).to match_array [unit_to_remove.name] - request = put :update, params: { id: item.id, item: { request_unit_ids: [unit.id] } } - expect(response).not_to have_error - expect(request).to redirect_to(items_path) - # We should have removed the existing unit and replaced it with the new one - expect(item.request_units.reload.pluck(:name)).to match_array [unit.name] + context "when selecting pack and unselecting box" do + let!(:item_unit){ create(:item_unit, item: item, request_limit: 1500, name: box_unit.name) } + let(:params) do + { + id: item.id, + item: { + unit_ids: [pack_unit.id.to_s, ""], + unit_limits: { pack_unit.id.to_s => "200" } + } + } + end + + it "adds and removes request units at the same time" do + expect(item.request_units.pluck(:name)).to match_array [box_unit.name] + + put :update, params: params + expect(response).not_to have_error + expect(response).to redirect_to(items_path) + + expect(item.request_units.reload.pluck(:name)).to match_array [pack_unit.name] + expect(item.request_units.first.request_limit).to eq 200 + end end end end @@ -170,7 +208,8 @@ it "should accept request_unit ids and create request_units" do Flipper.enable(:enable_packs) unit = create(:unit, organization: organization) - item_params[:item] = item_params[:item].merge({request_unit_ids: [unit.id]}) + item_params[:item] = item_params[:item].merge({ unit_ids: [unit.id], unit_limits: {} }) + post :create, params: item_params expect(response).not_to have_error newly_created_item = Item.last From 494a691c241fabb96d84e68002d6cb0db8403ea6 Mon Sep 17 00:00:00 2001 From: Em Barnard-Shao Date: Sat, 13 Sep 2025 14:36:57 -0400 Subject: [PATCH 6/9] linter --- spec/controllers/items_controller_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/controllers/items_controller_spec.rb b/spec/controllers/items_controller_spec.rb index fffde25793..6a4181a7e3 100644 --- a/spec/controllers/items_controller_spec.rb +++ b/spec/controllers/items_controller_spec.rb @@ -64,7 +64,7 @@ { id: item.id, item: { - unit_ids: [pack_unit.id.to_s, ""], + unit_ids: [pack_unit.id.to_s, ""], unit_limits: { pack_unit.id.to_s => "500" } } } @@ -82,7 +82,7 @@ end context "when already has pack selected and unselects pack" do - let!(:item_unit){ create(:item_unit, item: item, request_limit: 500, name: pack_unit.name) } + let!(:item_unit) { create(:item_unit, item: item, request_limit: 500, name: pack_unit.name) } let(:params) do { id: item.id, @@ -101,12 +101,12 @@ end context "when selecting pack and unselecting box" do - let!(:item_unit){ create(:item_unit, item: item, request_limit: 1500, name: box_unit.name) } + let!(:item_unit) { create(:item_unit, item: item, request_limit: 1500, name: box_unit.name) } let(:params) do { id: item.id, item: { - unit_ids: [pack_unit.id.to_s, ""], + unit_ids: [pack_unit.id.to_s, ""], unit_limits: { pack_unit.id.to_s => "200" } } } From 103b5b3dac208d9088d612e234bc25ca1979a97d Mon Sep 17 00:00:00 2001 From: Em Barnard-Shao Date: Sat, 13 Sep 2025 15:57:04 -0400 Subject: [PATCH 7/9] Fix tests, remove duplicate error messages and reformat for readibility --- app/models/partners/item_request.rb | 10 +++++++--- app/models/request.rb | 2 +- .../partners/request_create_service.rb | 18 +++++++++++------- spec/requests/items_requests_spec.rb | 2 +- spec/requests/partners/requests_spec.rb | 12 ++---------- .../partners/request_create_service_spec.rb | 12 +++++++++++- spec/system/partners/requests_system_spec.rb | 2 +- 7 files changed, 34 insertions(+), 24 deletions(-) diff --git a/app/models/partners/item_request.rb b/app/models/partners/item_request.rb index 9c708ea655..8f406502d7 100644 --- a/app/models/partners/item_request.rb +++ b/app/models/partners/item_request.rb @@ -22,16 +22,20 @@ class ItemRequest < Base has_many :children, through: :child_item_requests validates :quantity, presence: true - validates :quantity, numericality: { only_integer: true, greater_than_or_equal_to: 1 } + validates :quantity, numericality: { + only_integer: true, + greater_than_or_equal_to: 1, + message: "quantity must be a whole number greater than or equal to 1" + } validates :name, presence: true validate :request_unit_is_supported def request_unit_is_supported - return if request_unit.blank? + return if request_unit.blank? || request_unit == "-1" # nothing selected names = item.request_units.map(&:name) unless names.include?(request_unit) - errors.add(:request_unit, "is not supported") + errors.add(:base, "#{request_unit} is not a supported unit type") end end diff --git a/app/models/request.rb b/app/models/request.rb index 457e0d805d..9e6a7a6ca5 100644 --- a/app/models/request.rb +++ b/app/models/request.rb @@ -68,7 +68,7 @@ def request_type_label def item_requests_uniqueness_by_item_id 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") end end diff --git a/app/services/partners/request_create_service.rb b/app/services/partners/request_create_service.rb index 0d7a5b6723..5ec13b8f56 100644 --- a/app/services/partners/request_create_service.rb +++ b/app/services/partners/request_create_service.rb @@ -65,9 +65,7 @@ def populate_item_request(partner_request) pre_existing_entry = items[input_item['item_id']] if pre_existing_entry - if pre_existing_entry.request_unit != input_item['request_unit'] - errors.add(:base, "Please use the same unit for every #{Item.find(input_item["item_id"]).name}") - else + unless pre_existing_entry.request_unit != input_item['request_unit'] pre_existing_entry.quantity = (pre_existing_entry.quantity.to_i + input_item['quantity'].to_i).to_s # NOTE: When this code was written (and maybe it's still the # case as you read it!), the FamilyRequestsController does a @@ -82,7 +80,7 @@ def populate_item_request(partner_request) end if input_item['request_unit'].to_s == '-1' # nothing selected - errors.add(:base, "Please select a unit for #{Item.find(input_item["item_id"]).name}") + errors.add(:base, "Please select a unit for #{Item.find_by_id(input_item["item_id"]).name}") end item_request = Partners::ItemRequest.new( @@ -105,9 +103,10 @@ def populate_item_request(partner_request) }.compact end - # Validate if request quantity exceeds the request limit for the item and unit type + # Validate request quantity doesn't exceed the request limit for the item and unit type partner_request.request_items.each do |ir| - item = Item.find(ir["item_id"]) + item = Item.find_by_id(ir["item_id"]) + next if item.nil? unit_type = ir["request_unit"] quantity_requested = ir["quantity"].to_i @@ -118,7 +117,12 @@ def populate_item_request(partner_request) end if limit.present? && (quantity_requested > limit) - errors.add(:base, "#{item.name}: You requested #{quantity_requested} #{unit_type.pluralize}, but are limited to #{limit} #{unit_type.pluralize}") + message = if unit_type.blank? + "#{item.name}: You requested #{quantity_requested}, but are limited to #{limit}" + else + "#{item.name}: You requested #{quantity_requested} #{unit_type&.pluralize}, but are limited to #{limit} #{unit_type&.pluralize}" + end + errors.add(:base, message) end end diff --git a/spec/requests/items_requests_spec.rb b/spec/requests/items_requests_spec.rb index bd8a34ced5..5fa6f637ef 100644 --- a/spec/requests/items_requests_spec.rb +++ b/spec/requests/items_requests_spec.rb @@ -100,7 +100,7 @@ get edit_item_path(item) parsed_body = Nokogiri::HTML(response.body) - checkboxes = parsed_body.css("input[type='checkbox'][name='item[request_unit_ids][]']") + checkboxes = parsed_body.css("input[type='checkbox'][name='item[unit_ids][]']") expect(checkboxes.length).to eq organization_units.length checkboxes.each do |checkbox| if checkbox['value'] == selected_unit.id.to_s diff --git a/spec/requests/partners/requests_spec.rb b/spec/requests/partners/requests_spec.rb index 55b88df00a..a5aa19b897 100644 --- a/spec/requests/partners/requests_spec.rb +++ b/spec/requests/partners/requests_spec.rb @@ -242,11 +242,7 @@ expect { post partners_requests_path, params: request_attributes }.to_not change { Request.count } expect(response).to be_unprocessable - expect(response.body).to include("Oops! Something went wrong with your Request") - expect(response.body).to include("Ensure each line item has a item selected AND a quantity greater than 0.") - expect(response.body).to include("Still need help? Please contact your essentials bank, #{partner.organization.name}") - expect(response.body).to include("Our email on record for them is:") - expect(response.body).to include(partner.organization.email) + expect(response.body).to include("quantity must be a whole number greater than or equal to 1") end end @@ -346,11 +342,7 @@ expect { post partners_requests_path, params: request_attributes }.to_not change { Request.count } expect(response).to be_unprocessable - expect(response.body).to include("Oops! Something went wrong with your Request") - expect(response.body).to include("Ensure each line item has a item selected AND a quantity greater than 0.") - expect(response.body).to include("Still need help? Please contact your essentials bank, #{partner.organization.name}") - expect(response.body).to include("Our email on record for them is:") - expect(response.body).to include(partner.organization.email) + expect(response.body).to include("Completely empty request") end end diff --git a/spec/services/partners/request_create_service_spec.rb b/spec/services/partners/request_create_service_spec.rb index d179fe8104..af6a6eb990 100644 --- a/spec/services/partners/request_create_service_spec.rb +++ b/spec/services/partners/request_create_service_spec.rb @@ -193,7 +193,8 @@ let(:partner) { create(:partner) } let(:request_type) { "child" } let(:comments) { Faker::Lorem.paragraph } - let(:item) { FactoryBot.create(:item) } + let(:item) { FactoryBot.create(:item, unit_request_limit: request_limit) } + let(:request_limit) { nil } let(:item_requests_attributes) do [ ActionController::Parameters.new( @@ -209,5 +210,14 @@ expect(subject.partner_request.item_requests.first.item.name).to eq(item.name) expect(subject.partner_request.item_requests.first.quantity).to eq("25") end + + context "request quantity exceeds the request limit for the item and unit type" do + let(:request_limit) { 10 } + + it "adds error" do + expect(subject.errors).not_to be_empty + expect(subject.errors[:base].first).to include "You requested 25, but are limited to 10" + end + end end end diff --git a/spec/system/partners/requests_system_spec.rb b/spec/system/partners/requests_system_spec.rb index 4238d7c25d..1762ab19cf 100644 --- a/spec/system/partners/requests_system_spec.rb +++ b/spec/system/partners/requests_system_spec.rb @@ -43,7 +43,7 @@ fill_in "request_item_requests_attributes_0_quantity", with: 50 click_on "Submit Essentials Request" - expect(page).to have_text "Please ensure a single unit is selected for each item that supports it." + expect(page).to have_text "Please select a unit for item 1." expect(Request.count).to eq(0) end From de85a7e89faac18dab8ffe70d6cc13e2a2b9dacc Mon Sep 17 00:00:00 2001 From: Em Barnard-Shao Date: Sat, 13 Sep 2025 19:41:59 -0400 Subject: [PATCH 8/9] Fix test --- app/services/partners/request_create_service.rb | 8 ++++---- spec/models/partners/item_request_spec.rb | 2 +- spec/models/request_spec.rb | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/services/partners/request_create_service.rb b/app/services/partners/request_create_service.rb index 5ec13b8f56..8f681e73f0 100644 --- a/app/services/partners/request_create_service.rb +++ b/app/services/partners/request_create_service.rb @@ -118,10 +118,10 @@ def populate_item_request(partner_request) if limit.present? && (quantity_requested > limit) message = if unit_type.blank? - "#{item.name}: You requested #{quantity_requested}, but are limited to #{limit}" - else - "#{item.name}: You requested #{quantity_requested} #{unit_type&.pluralize}, but are limited to #{limit} #{unit_type&.pluralize}" - end + "#{item.name}: You requested #{quantity_requested}, but are limited to #{limit}" + else + "#{item.name}: You requested #{quantity_requested} #{unit_type&.pluralize}, but are limited to #{limit} #{unit_type&.pluralize}" + end errors.add(:base, message) end end diff --git a/spec/models/partners/item_request_spec.rb b/spec/models/partners/item_request_spec.rb index 9d63cde2e4..f2c1cec395 100644 --- a/spec/models/partners/item_request_spec.rb +++ b/spec/models/partners/item_request_spec.rb @@ -37,7 +37,7 @@ item_request = build(:item_request, request_unit: "flat", request: request, item: item) expect(item_request.valid?).to eq(false) - expect(item_request.errors.full_messages).to eq(["Request unit is not supported"]) + expect(item_request.errors.full_messages).to eq(["flat is not a supported unit type"]) item_unit.update!(name: 'flat') item.reload diff --git a/spec/models/request_spec.rb b/spec/models/request_spec.rb index 473447797c..a37d6794a7 100644 --- a/spec/models/request_spec.rb +++ b/spec/models/request_spec.rb @@ -90,7 +90,7 @@ it "is not valid" do expect(subject).to_not be_valid - expect(subject.errors[:item_requests]).to include("should have unique item_ids") + expect(subject.errors[:base]).to include("Please ensure a single unit is selected for each item that supports it") end end From b1636811e03075e9f96ba0633b8f32f9ee625ee8 Mon Sep 17 00:00:00 2001 From: Em Barnard-Shao Date: Sat, 13 Sep 2025 20:09:00 -0400 Subject: [PATCH 9/9] Fix some things --- app/views/items/_form.html.erb | 2 +- spec/models/partners/item_request_spec.rb | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/views/items/_form.html.erb b/app/views/items/_form.html.erb index 1e7a0742de..90098195a8 100644 --- a/app/views/items/_form.html.erb +++ b/app/views/items/_form.html.erb @@ -106,4 +106,4 @@ if (limit) limit.disabled = !e.target.checked; } }); - \ No newline at end of file + diff --git a/spec/models/partners/item_request_spec.rb b/spec/models/partners/item_request_spec.rb index f2c1cec395..d3a29ed685 100644 --- a/spec/models/partners/item_request_spec.rb +++ b/spec/models/partners/item_request_spec.rb @@ -24,7 +24,11 @@ describe 'validations' do it { should validate_presence_of(:quantity) } - it { should validate_numericality_of(:quantity).only_integer.is_greater_than_or_equal_to(1) } + it { + should validate_numericality_of(:quantity) + .only_integer.is_greater_than_or_equal_to(1) + .with_message("quantity must be a whole number greater than or equal to 1") + } it { should validate_presence_of(:name) } it "should only be able to use item's request units" do