Merge pull request #4483 from jimmyli97/4172-fix-donation-validation

Fix #4172 donation validation display error

app/controllers/donations_controller.rb

@@ -90,14 +90,22 @@ def show
 
   def update
     @donation = Donation.find(params[:id])
+    @original_source = @donation.source
     ItemizableUpdateService.call(itemizable: @donation,
       params: donation_params,
       type: :increase,
       event_class: DonationEvent)
+    flash.clear
+    flash[:notice] = "Donation updated!"
     redirect_to donations_path
   rescue => e
     flash[:alert] = "Error updating donation: #{e.message}"
-    render "edit"
+    load_form_collections
+    # calling new(donation_params) triggers a validation error if line_item quantity is invalid
+    @previous_input = Donation.new(donation_params.except(:line_items_attributes))
+    @previous_input.line_items.build(donation_params[:line_items_attributes].values)
+
+    render "edit", status: :conflict
   end
 
   def destroy
@@ -129,14 +137,14 @@ def clean_donation_money_raised
     params[:donation][:money_raised] = money_raised.gsub(/[$,.]/, "") if money_raised
 
     money_raised_in_dollars = params[:donation][:money_raised_in_dollars]
-    params[:donation][:money_raised] = money_raised_in_dollars.gsub(/[$,]/, "").to_d * 100 if money_raised_in_dollars
+    params[:donation][:money_raised] = (money_raised_in_dollars.gsub(/[$,]/, "").to_d * 100).to_s if money_raised_in_dollars
   end
 
   def donation_params
     strip_unnecessary_params
     clean_donation_money_raised
     params = compact_line_items
-    params.require(:donation).permit(:source, :comment, :storage_location_id, :money_raised, :issued_at, :donation_site_id, :product_drive_id, :product_drive_participant_id, :manufacturer_id, line_items_attributes: %i(id item_id quantity _destroy)).merge(organization: current_organization)
+    params.require(:donation).permit(:source, :comment, :storage_location_id, :money_raised, :issued_at, :donation_site_id, :product_drive_id, :product_drive_participant_id, :manufacturer_id, line_items_attributes: %i(id item_id quantity)).merge(organization: current_organization)
   end
 
   def donation_item_params

app/views/donations/_donation_form.html.erb

@@ -6,6 +6,8 @@
       <div class="col-xs-8 col-md-6 col-lg-3">
         <%= f.input :source,
                     collection: Donation::SOURCES.values,
+                    selected: donation_form.source,
+                    include_blank: true,
                     label: "Source",
                     error: "What effort or initiative did this donation come from?",
                     wrapper: :input_group %>
@@ -16,6 +18,8 @@
       <div class="col-xs-8 col-md-6 col-lg-3">
         <%= f.association :donation_site,
                           collection: @donation_sites,
+                          selected: donation_form.donation_site_id,
+                          include_blank: true,
                           label: "Donation Site",
                           error: "Where was this donation dropped off?",
                           wrapper: :input_group %>
@@ -25,6 +29,8 @@
       <div class="col-xs-8 col-md-6 col-lg-3">
         <%= f.association :product_drive,
                           collection: @product_drives,
+                          selected: donation_form.product_drive_id,
+                          include_blank: true,
                           label_method: lambda { |x| "#{x.try(:name) }" },
                           label: "Product Drive",
                           error: "Which product drive was this from?",
@@ -33,6 +39,8 @@
       <div class="col-xs-8 col-md-6 col-lg-3">
         <%= f.association :product_drive_participant,
                           collection: @product_drive_participants,
+                          selected: donation_form.product_drive_participant_id,
+                          include_blank: true,
                           label_method: lambda { |x| "#{x.try(:business_name) }" },
                           label: "Product Drive Participant",
                           error: "Which product drive participant was this from?",
@@ -44,7 +52,8 @@
       <div class="col-xs-8 col-md-6 col-lg-3">
         <%= f.association :manufacturer,
                           collection: @manufacturers,
-                          prompt: "Choose one...",
+                          selected: donation_form.manufacturer_id,
+                          include_blank: true,
                           label_method: lambda { |x| "#{x.try(:name) }" },
                           label: "Manufacturer",
                           error: "Which Manufacturer was this from?",
@@ -58,7 +67,7 @@
                           collection: @storage_locations,
                           label: "Storage Location",
                           error: "Where is it being stored?",
-                          selected: f.object.storage_location&.id || current_organization.intake_location,
+                          selected: donation_form.storage_location&.id || current_organization.intake_location,
                           include_blank: true,
                           wrapper: :input_group %>
       </div>
@@ -68,15 +77,18 @@
       <div class="col-xs-8 col-md-6 col-lg-3">
         <%= f.input :money_raised_in_dollars, as: :money_raised_in_dollars, wrapper: :input_group do %>
           <span class="input-group-text"><i class="fa fa-usd"></i></span>
-          <%= f.input_field :money_raised_in_dollars, class: "form-control" %>
+          <%= f.input_field :money_raised_in_dollars,
+                            class: "form-control",
+                            value: donation_form.money_raised_in_dollars %>
         <% end %>
       </div>
     </div>
 
     <div class="row">
       <div class="col-xs-8 col-md-6">
-        <%= f.input :comment,
-                    wrapper: :input_group %>
+        <%= f.input :comment, wrapper: :input_group do %>
+          <%= f.text_area :comment, value: donation_form.comment, cols: 90, rows: 2, class: "text optional form-control" %>
+        <% end %>
       </div>
     </div>
 
@@ -86,15 +98,16 @@
                     label: "Issued on",
                     as: :date,
                     html5: true,
-                    wrapper: :input_group %>
+                    wrapper: :input_group,
+                    input_html: { value: donation_form.issued_at } %>
       </div>
     </div>
 
     <fieldset style="margin-bottom: 2rem;" class='w-70'>
       <legend>Items in this donation</legend>
       <div id="donation_line_items" data-capture-barcode="true">
 
-        <%= render 'line_items/line_item_fields', form: f %>
+        <%= render 'line_items/line_item_fields', form: f, object: donation_form.line_items %>
       </div>
 
       <div class="row links justify-content-end">

app/views/donations/edit.html.erb

@@ -2,10 +2,10 @@
   <div class="container-fluid">
     <div class="row mb-2">
       <div class="col-sm-6">
-        <% content_for :title, "Edit - Donations - #{@donation.source} - #{current_organization.name}" %>
+        <% content_for :title, "Edit - Donations - #{@original_source ? @original_source : @donation.source} - #{current_organization.name}" %>
         <h1>
           Editing Donation
-          <small>from <%= @donation.source %></small>
+          <small>from <%= @original_source ? @original_source : @donation.source %></small>
         </h1>
       </div>
       <div class="col-sm-6">
@@ -16,7 +16,7 @@
           </li>
           <li class="breadcrumb-item"><%= link_to "Donations", donations_path %></li>
           <li class="breadcrumb-item">
-            <a href="#">Editing <%= @donation.source %> on <%= @donation.created_at.to_fs(:distribution_date) %></a></li>
+            <a href="#">Editing <%= @original_source ? @original_source : @donation.source %> on <%= @donation.created_at.to_fs(:distribution_date) %></a></li>
         </ol>
       </div>
     </div>
@@ -39,7 +39,7 @@
         <div class="card card-primary">
           <!-- /.card-header -->
           <div class="card-body">
-            <%= render partial: "donation_form", object: @donation %>
+            <%= render partial: "donation_form", object: @previous_input ? @previous_input : @donation %>
             <%= link_to "Add vendor", new_vendor_path, {:remote => true, "data-bs-toggle" => "modal", "data-bs-target" => "#modal-window", id: "new_vendor", "style" => "display: none;"} %>
           </div><!-- /.box -->
         </div>

spec/controllers/donations_controller_spec.rb

@@ -58,7 +58,6 @@
         line_item = donation.line_items.first
         line_item_params = {
           "0" => {
-            "_destroy" => "false",
             item_id: line_item.item_id,
             quantity: "15",
             id: line_item.id
@@ -82,7 +81,6 @@
           line_item = donation.line_items.first
           line_item_params = {
             "0" => {
-              "_destroy" => "false",
               item_id: line_item.item_id,
               quantity: "8",
               id: line_item.id
@@ -111,40 +109,34 @@
           line_item = donation.line_items.first
           line_item_params = {
             "0" => {
-              "_destroy" => "false",
               item_id: line_item.item_id,
               quantity: "1",
               id: line_item.id
             }
           }
           donation_params = { source: donation.source, storage_location: new_storage_location, line_items_attributes: line_item_params }
           put :update, params: { id: donation.id, donation: donation_params }
-          expect(response).not_to redirect_to(anything)
+          expect(response).not_to redirect_to(edit_donation_path(donation))
           expect(original_storage_location.size).to eq 5
           expect(new_storage_location.size).to eq 0
           expect(donation.reload.line_items.first.quantity).to eq 10
         end
       end
 
       describe "when removing a line item" do
-        it "updates storage invetory item quantity correctly" do
-          donation = create(:donation, :with_items, item_quantity: 10)
-          line_item = donation.line_items.first
-          line_item_params = {
-            "0" => {
-              "_destroy" => "true",
-              item_id: line_item.item_id,
-              id: line_item.id
-            }
-          }
-          donation_params = { source: donation.source, line_items_attributes: line_item_params }
+        it "updates storage inventory item quantity correctly" do
+          item_id = 1
+          item_quantity = 10
+          donation = create(:donation, :with_items, item: create(:item, id: item_id), item_quantity: item_quantity)
+          # if all line items including blanks are deleted line_items_attributes parameter is not sent
+          donation_params = { source: donation.source }
           expect do
             put :update, params: { id: donation.id, donation: donation_params }
-          end.to change { donation.storage_location.inventory_items.first.quantity }.by(-10)
+          end.to change { donation.storage_location.inventory_items.first.quantity }.by(-1 * item_quantity)
             .and change {
                    View::Inventory.new(donation.organization_id)
-                     .quantity_for(storage_location: donation.storage_location_id, item_id: line_item.item_id)
-                 }.by(-10)
+                     .quantity_for(storage_location: donation.storage_location_id, item_id: item_id)
+                 }.by(-1 * item_quantity)
         end
       end
     end

spec/factories/donations.rb

@@ -42,15 +42,15 @@
     end
 
     trait :with_items do
+      transient do
+        item_quantity { 100 }
+        item { nil }
+      end
       storage_location do
         create :storage_location, :with_items,
                item: item || create(:item, value_in_cents: 100),
                organization: organization
       end
-      transient do
-        item_quantity { 100 }
-        item { nil }
-      end
 
       after(:build) do |donation, evaluator|
         event_item = View::Inventory.new(donation.organization_id)

spec/requests/donations_requests_spec.rb

@@ -192,5 +192,83 @@
         expect(response.body).to_not include("you’ll need to make an adjustment to the inventory as well.")
       end
     end
+
+    # Bug fix - Issue #4172
+    context "when donated items are distributed to less than donated amount " \
+    "and you edit the donation to less than distributed amount" do
+      it "shows a warning and displays original names and amounts and info the user entered" do
+        item_name = "Brightbloom Seed"
+        item = create(:item, organization: organization, name: item_name)
+        storage_location = create(:storage_location, :with_items, item: item, item_quantity: 0, organization: organization)
+        extra_item_name = "Extra Item"
+        extra_item = create(:item, organization: organization, name: extra_item_name)
+        item_to_delete_name = "Item To Delete"
+        item_to_delete = create(:item, organization: organization, name: item_to_delete_name)
+        TestInventory.create_inventory(organization, { storage_location.id => { item.id => 0, extra_item.id => 1 } })
+        original_quantity = 100
+        original_source = Donation::SOURCES[:manufacturer]
+        original_date = DateTime.new(2024)
+        donation = build(:manufacturer_donation, issued_at: original_date, organization: organization, storage_location: storage_location, source: original_source)
+        donation.line_items << build(:line_item, item: item, quantity: original_quantity)
+        donation.line_items << build(:line_item, item: item_to_delete, quantity: 1)
+
+        DonationCreateService.call(donation)
+
+        # simulate distribution by setting item inventory to 10
+        TestInventory.create_inventory(organization, { storage_location.id => { item.id => 10, extra_item.id => 1 } })
+
+        edited_source = Donation::SOURCES[:product_drive]
+        edited_source_drive_name = "Test Product Drive"
+        edited_source_drive = create(:product_drive, name: edited_source_drive_name, organization: organization)
+        edited_source_drive_participant_business_name = "Test Business Name"
+        edited_source_drive_participant = create(:product_drive_participant, business_name: edited_source_drive_participant_business_name, organization: organization)
+        edited_storage_location_name = "Test Storage"
+        edited_storage_location = create(:storage_location, name: edited_storage_location_name, organization: organization)
+        edited_money = 10.0
+        edited_comment = "New test comment"
+        edited_date = "2019-01-01"
+        extra_quantity = 1
+        edited_quantity = 1
+
+        # deleted item is removed from line_items_attributes entirely
+        edited_donation = {
+          source: edited_source,
+          product_drive_id: edited_source_drive.id,
+          product_drive_participant_id: edited_source_drive_participant.id,
+          storage_location_id: edited_storage_location.id,
+          money_raised_in_dollars: edited_money,
+          comment: edited_comment,
+          issued_at: edited_date,
+          line_items_attributes: {
+            "0": { item_id: item.id, quantity: edited_quantity },
+            "1": { item_id: extra_item.id, quantity: extra_quantity }
+          }
+        }
+
+        put donation_path(id: donation.id, donation: edited_donation)
+
+        if Event.read_events?(organization)
+          expect(flash[:alert]).to include("Error updating donation: Could not reduce quantity")
+        else # TODO remove this branch when switching to events
+          expect(flash[:alert]).to include("Error updating donation: Requested items exceed the available inventory")
+        end
+
+        expect(response.body).to include("Edit - Donations - #{original_source}")
+        expect(response.body).to include("Editing Donation\n          <small>from #{original_source}")
+        expect(response.body).to include("<li class=\"breadcrumb-item\">\n            <a href=\"#\">Editing #{original_source}")
+        expect(response.body).to include("<option selected=\"selected\" value=\"#{edited_source}\">#{edited_source}</option>")
+        expect(response.body).to include("<option selected=\"selected\" value=\"#{edited_source_drive.id}\">#{edited_source_drive_name}</option>")
+        expect(response.body).to include("<option selected=\"selected\" value=\"#{edited_source_drive_participant.id}\">#{edited_source_drive_participant_business_name}</option>")
+        expect(response.body).to include("<option selected=\"selected\" value=\"#{edited_storage_location.id}\">#{edited_storage_location_name}</option>")
+        expect(response.body).to include(edited_comment)
+        expect(response.body).to include("value=\"#{edited_money}\" type=\"text\" name=\"donation[money_raised_in_dollars]")
+        expect(response.body).to include(edited_date)
+        expect(response.body).to include("<option selected=\"selected\" value=\"#{item.id}\">#{item_name}</option>")
+        expect(response.body).to include("value=\"#{edited_quantity}\" name=\"donation[line_items_attributes][0][quantity]")
+        expect(response.body).to include("<option selected=\"selected\" value=\"#{extra_item.id}\">#{extra_item_name}</option>")
+        expect(response.body).to include("value=\"#{extra_quantity}\" name=\"donation[line_items_attributes][1][quantity]")
+        expect(response.body).not_to include("<option selected=\"selected\" value=\"#{item_to_delete.id}\">#{item_to_delete_name}</option>")
+      end
+    end
   end
 end