Ensure unique units are used when creating a request [#4579] (#4835)

* Ensure unique units are used when creating a request [#4579]

* Clean out some lint [#4579]

* Update spec wording expectation [#4579]

* Handle items out of order intermittent [#4579]

* Unify call and initialize_only to use same logic [#4579]

* Fix create service to make in-memory request even if invalid

Errors are then reported up to the UI but not saved to the database;
this is what the controller and rails forms expects.

[#4579]

* Fix re-editing of request units in dynamic JS [#4579]

* Sate Rubocop! [#4579]

* Update family and individual request validation to match create [#4579]

* Only include request_unit on request_items when specified [#4579]

* Make request controllers match for general and specific types

[#4579]

Author: awwaiid

app/controllers/partners/family_requests_controller.rb

@@ -34,12 +34,14 @@ def create
     def validate
       family_requests_attributes = build_family_requests_attributes(params)
 
-      @partner_request = Partners::FamilyRequestCreateService.new(
+      create_service = Partners::FamilyRequestCreateService.new(
         partner_user_id: current_user.id,
         family_requests_attributes: family_requests_attributes,
         request_type: "child"
       ).initialize_only
-      if @partner_request.valid?
+
+      if create_service.errors.none?
+        @partner_request = create_service.partner_request
         @total_items = @partner_request.total_items
         @quota_exceeded = current_partner.quota_exceeded?(@total_items)
         body = render_to_string(template: 'partners/requests/validate', formats: [:html], layout: false)

app/controllers/partners/individuals_requests_controller.rb

@@ -34,13 +34,15 @@ def create
     end
 
     def validate
-      @partner_request = Partners::FamilyRequestCreateService.new(
+      create_service = Partners::FamilyRequestCreateService.new(
         partner_user_id: current_user.id,
         comments: individuals_request_params[:comments],
         family_requests_attributes: individuals_request_params[:items_attributes]&.values,
         request_type: "individual"
       ).initialize_only
-      if @partner_request.valid?
+
+      if create_service.errors.none?
+        @partner_request = create_service.partner_request
         @total_items = @partner_request.total_items
         @quota_exceeded = current_partner.quota_exceeded?(@total_items)
         body = render_to_string(template: 'partners/requests/validate', formats: [:html], layout: false)

app/controllers/partners/requests_controller.rb

@@ -43,14 +43,15 @@ def create
     end
 
     def validate
-      @partner_request = Partners::RequestCreateService.new(
+      create_service = Partners::RequestCreateService.new(
         request_type: "quantity",
         partner_user_id: current_user.id,
         comments: partner_request_params[:comments],
         item_requests_attributes: partner_request_params[:item_requests_attributes]&.values || []
       ).initialize_only
 
-      if @partner_request.valid?
+      if create_service.errors.none?
+        @partner_request = create_service.partner_request
         @total_items = @partner_request.total_items
         @quota_exceeded = current_partner.quota_exceeded?(@total_items)
         body = render_to_string(template: 'partners/requests/validate', formats: [:html], layout: false)

app/javascript/controllers/item_units_controller.js

@@ -5,13 +5,17 @@ export default class extends Controller {
   static targets = ["itemSelect", "requestSelect"]
   static values = {
     // hash of (item ID => hash of (request unit name => request unit plural name))
-    "itemUnits": Object
+    "itemUnits": Object,
+    "selectedItemUnits": String
   }
 
   addOption(val, text) {
     let option = document.createElement("option");
     option.value = val;
     option.text = text;
+    if (this.selectedItemUnitsValue === val) {
+      option.selected = true;
+    }
     this.requestSelectTarget.appendChild(option);
   }
 

app/services/partners/request_create_service.rb

@@ -13,22 +13,7 @@ def initialize(request_type:, partner_user_id:, comments: nil, item_requests_att
     end
 
     def call
-      @partner_request = ::Request.new(
-        partner_id: partner.id,
-        organization_id: organization_id,
-        comments: comments,
-        request_type: request_type,
-        partner_user_id: partner_user_id
-      )
-      @partner_request = populate_item_request(@partner_request)
-      @partner_request.assign_attributes(additional_attrs)
-
-      unless @partner_request.valid?
-        @partner_request.errors.each do |error|
-          errors.add(error.attribute, error.message)
-        end
-      end
-
+      initialize_only
       return self if errors.present?
 
       Request.transaction do
@@ -44,14 +29,23 @@ def call
     end
 
     def initialize_only
-      partner_request = ::Request.new(partner_id: partner.id,
+      @partner_request = ::Request.new(
+        partner_id: partner.id,
         organization_id: organization_id,
         comments: comments,
         request_type: request_type,
-        partner_user_id: partner_user_id)
-      partner_request = populate_item_request(partner_request)
-      partner_request.assign_attributes(additional_attrs)
-      partner_request
+        partner_user_id: partner_user_id
+      )
+      @partner_request = populate_item_request(partner_request)
+      @partner_request.assign_attributes(additional_attrs)
+
+      unless @partner_request.valid?
+        @partner_request.errors.each do |error|
+          errors.add(error.attribute, error.message)
+        end
+      end
+
+      self
     end
 
     private
@@ -68,42 +62,48 @@ def populate_item_request(partner_request)
 
       formatted_line_items.each do |input_item|
         pre_existing_entry = items[input_item['item_id']]
+
         if pre_existing_entry
-          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
-          # ton of calculation to translate children to item quantities.
-          # If that logic is incorrect, there's not much we can do here
-          # to fix things. Could make sense to move more of that logic
-          # into one of the service objects that instantiate the Request
-          # object (either this one or the FamilyRequestCreateService).
-          pre_existing_entry.children = (pre_existing_entry.children + (input_item['children'] || [])).uniq
-        else
-          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}")
+          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
+            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
+            # ton of calculation to translate children to item quantities.
+            # If that logic is incorrect, there's not much we can do here
+            # to fix things. Could make sense to move more of that logic
+            # into one of the service objects that instantiate the Request
+            # object (either this one or the FamilyRequestCreateService).
+            pre_existing_entry.children = (pre_existing_entry.children + (input_item['children'] || [])).uniq
             next
           end
-          items[input_item['item_id']] = Partners::ItemRequest.new(
-            item_id: input_item['item_id'],
-            request_unit: input_item['request_unit'],
-            quantity: input_item['quantity'],
-            children: input_item['children'] || [], # will create ChildItemRequests if there are any
-            name: fetch_organization_item_name(input_item['item_id']),
-            partner_key: fetch_organization_partner_key(input_item['item_id'])
-          )
         end
-      end
 
-      item_requests = items.values
+        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}")
+        end
 
-      partner_request.item_requests << item_requests
+        item_request = Partners::ItemRequest.new(
+          item_id: input_item['item_id'],
+          request_unit: input_item['request_unit'],
+          quantity: input_item['quantity'],
+          children: input_item['children'] || [], # will create ChildItemRequests if there are any
+          name: fetch_organization_item_name(input_item['item_id']),
+          partner_key: fetch_organization_partner_key(input_item['item_id'])
+        )
+        partner_request.item_requests << item_request
+        items[input_item['item_id']] = item_request
+      end
 
       partner_request.request_items = partner_request.item_requests.map do |ir|
         {
           item_id: ir.item_id,
-          quantity: ir.quantity
-        }
+          quantity: ir.quantity,
+          request_unit: ir.request_unit
+        }.compact
       end
+
       partner_request
     end
 

app/views/partners/requests/_error.html.erb

@@ -9,7 +9,7 @@
     <p class='text-lg font-bold'>
       Ensure each line item has a item selected AND a quantity greater than 0.
       <% if Flipper.enabled?(:enable_packs) && current_partner.organization.request_units.any? %>
-        Please ensure a unit is selected for each item that supports it.
+        Please ensure a single unit is selected for each item that supports it.
       <% end %>
     </p>
     <p class='text-md'>

app/views/partners/requests/_item_request.html.erb

@@ -1,5 +1,5 @@
 <%= form.fields_for :item_requests, defined?(object) ? object : nil  do |field| %>
-  <tr data-controller="item-units" data-item-units-item-units-value="<%= item_units.to_json %>">
+  <tr data-controller="item-units" data-item-units-item-units-value="<%= item_units.to_json %>" data-item-units-selected-item-units-value="<%= field.object.request_unit || -1 %>">
     <td>
       <%= field.label :item_id, "Item Requested", {class: 'sr-only'} %>
       <%= field.select :item_id, @requestable_items, {include_blank: 'Select an item'},

spec/requests/partners/requests_spec.rb

@@ -124,6 +124,7 @@
   describe "POST #create" do
     subject { post partners_requests_path, params: request_attributes }
     let(:item1) { create(:item, name: "First item", organization: organization) }
+    let(:item2) { create(:item, name: "Second item", organization: organization) }
 
     let(:request_attributes) do
       {
@@ -142,8 +143,15 @@
 
     before do
       sign_in(partner_user)
+
+      # Set up a variety of units that these two items are allowed to have
       FactoryBot.create(:unit, organization: organization, name: 'pack')
+      FactoryBot.create(:unit, organization: organization, name: 'box')
+      FactoryBot.create(:unit, organization: organization, name: 'notallowed')
       FactoryBot.create(:item_unit, item: item1, name: 'pack')
+      FactoryBot.create(:item_unit, item: item1, name: 'box')
+      FactoryBot.create(:item_unit, item: item2, name: 'pack')
+      FactoryBot.create(:item_unit, item: item2, name: 'box')
     end
 
     context 'when given valid parameters' do
@@ -185,6 +193,66 @@
       end
     end
 
+    context "when there are mixed units" do
+      context "on different items" do
+        let(:request_attributes) do
+          {
+            request: {
+              comments: Faker::Lorem.paragraph,
+              item_requests_attributes: {
+                "0" => {
+                  item_id: item1.id,
+                  request_unit: 'pack',
+                  quantity: 12
+                },
+                "1" => {
+                  item_id: item2.id,
+                  request_unit: 'box',
+                  quantity: 17
+                }
+              }
+            }
+          }
+        end
+
+        it "creates without error" do
+          Flipper.enable(:enable_packs)
+          expect { subject }.to change { Request.count }.by(1)
+          expect(response).to redirect_to(partners_request_path(Request.last.id))
+          expect(response.request.flash[:success]).to eql "Request was successfully created."
+        end
+      end
+
+      context "on the same item" do
+        let(:request_attributes) do
+          {
+            request: {
+              comments: Faker::Lorem.paragraph,
+              item_requests_attributes: {
+                "0" => {
+                  item_id: item1.id,
+                  request_unit: 'pack',
+                  quantity: 12
+                },
+                "1" => {
+                  item_id: item1.id,
+                  request_unit: 'box',
+                  quantity: 17
+                }
+              }
+            }
+          }
+        end
+
+        it "results in an error" do
+          Flipper.enable(:enable_packs)
+          expect { post partners_requests_path, params: request_attributes }.to_not change { Request.count }
+          expect(response).to be_unprocessable
+          expect(response.body).to include("Please ensure a single unit is selected for each item")
+        end
+      end
+    end
+
     context "when a request empty" do
       let(:request_attributes) do
         {

spec/services/partners/request_create_service_spec.rb

@@ -178,6 +178,7 @@
 
   describe "#initialize_only" do
     subject { described_class.new(**args).initialize_only }
+
     let(:args) do
       {
         partner_user_id: partner_user.id,
@@ -201,9 +202,10 @@
     end
 
     it "creates a partner request in memory only" do
-      expect(subject.id).to be_nil
-      expect(subject.item_requests.first.item.name).to eq(item.name)
-      expect(subject.item_requests.first.quantity).to eq("25")
+      expect { subject }.not_to change { Request.count }
+      expect(subject.partner_request.id).to be_nil
+      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
   end
 end

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 unit is selected for each item that supports it."
+        expect(page).to have_text "Please ensure a single unit is selected for each item that supports it."
         expect(Request.count).to eq(0)
       end