5280 combine same items save (#5285)

* creates confirmation modal for transfers and sums up items

* call combine! on line_items to merge duplicates

* tests the merging of same items in modal and controller

* fixes linting

* fixes flakey capybara test

* updates transfer markdown documentation with new confirmation modal info

* fixes erb lint

* fixes wording on inventory_transfers md

* fixes unused test context

Author: john-huang-121

app/controllers/transfers_controller.rb

@@ -21,6 +21,7 @@ def index
 
   def create
     @transfer = current_organization.transfers.new(transfer_params)
+    @transfer.line_items.combine!
 
     TransferCreateService.call(@transfer)
     redirect_to transfers_path, notice: "#{@transfer.line_items.total} items have been transferred from #{@transfer.from.name} to #{@transfer.to.name}!"
@@ -57,6 +58,18 @@ def destroy
     redirect_to transfers_path
   end
 
+  def validate
+    @transfer = current_organization.transfers.new(transfer_params)
+    @transfer.line_items.combine!
+
+    if @transfer.valid?
+      body = render_to_string(partial: "transfers/validate_modal", formats: [:html], layout: false)
+      render json: {valid: true, body: body}
+    else
+      render json: {valid: false}, status: :unprocessable_entity
+    end
+  end
+
   private
 
   def load_form_collections

app/views/transfers/_validate_modal.html.erb

@@ -0,0 +1,42 @@
+<div class="modal-dialog modal-lg">
+    <div class="modal-content">
+      <div class="modal-header">
+        <h5 class="modal-title">Transfer Confirmation</h5>
+        <button id="modalClose" type="button" class="btn-close" data-bs-dismiss="modal" aria-label="Close"></button>
+      </div>
+      <div class="modal-body" style="max-height: 60vh; overflow-y: auto;">
+        <%# Items and quantities %>
+        <table class="table">
+          <thead>
+            <tr>
+              <th>Item Name</th>
+              <th>Total Items</th>
+            </tr>
+          </thead>
+          <tbody>
+            <% @transfer.line_items.each do |line_item| %>
+              <tr>
+                <td><%= line_item.name %></td>
+                <td><%= line_item.quantity %></td>
+              </tr>
+            <% end %>
+          </tbody>
+        </table>
+
+        <% if @transfer.comment.present? %>
+          <p><strong>Note:</strong> <%= @transfer.comment %></p>
+        <% end %>
+
+      <%# Confirmation message %>
+      <div class="message fs-5">
+        <p>Please confirm that the above list is what you meant to transfer and that the comment is correct.</p>
+      </div>
+    </div>
+
+    <%# Actions %>
+    <div class="modal-footer">
+      <button id="modalNo" type="button" class="btn btn-secondary" data-bs-dismiss="modal" aria-label="No I need to make changes">No, I need to make changes</button>
+      <button id="modalYes" type="button" class="btn btn-success" data-action="confirmation#submitForm">Yes, it's correct</button>
+    </div>
+  </div>
+</div>

app/views/transfers/new.html.erb

@@ -28,7 +28,9 @@
       <!-- left column -->
       <div class="col-md-12">
         <!-- jquery validation -->
-        <div class="card card-primary">
+        <div class="card card-primary"
+            data-controller="confirmation"
+            data-confirmation-pre-check-path-value="<%= validate_transfers_path(format: :json) %>">
           <!-- /.card-header -->
           <!-- form start -->
             <div class="card-body">
@@ -39,10 +41,9 @@
                     storage location to another.</p>
                 </div>
                 <div class="box-body">
-
                   <%= simple_form_for @transfer,
-                    data: { controller: "form-input" },
-                    html: { class: "storage-location-required", id: 'new_transfer' } do |f| %>
+                    data: { controller: "form-input", confirmation_target: "form" },
+                    html: { class: "storage-location-required", id: "new_transfer" } do |f| %>
                     <%= render partial: "storage_locations/source", object: f, locals: {label: "From storage location", error: "Which location are you moving inventory from?", association_field: :from} %>
 
                     <%= f.association :to,
@@ -69,10 +70,19 @@
 
                     </div>
                     <div class="card-footer">
-                      <%= submit_button %>
+                      <%= submit_button({}, { action: "click->confirmation#openModal" }) %>
                     </div>
                   <% end %>
                   </div>
+                <div
+                  id="transferConfirmationModal"
+                  class="modal confirm"
+                  aria-labelledby="transferConfirmationModal"
+                  aria-hidden="true"
+                  tabindex="-1"
+                  data-bs-backdrop="static"
+                  data-confirmation-target="modal">
+                </div>
             </div><!-- /.box -->
         </div>
       </div>

config/routes.rb

@@ -119,7 +119,9 @@ def set_up_flipper
     get :activity_graph
   end
 
-  resources :transfers, only: %i(index create new show destroy)
+  resources :transfers, only: %i(index create new show destroy) do
+    post :validate, on: :collection
+  end
 
   resources :storage_locations do
     put :deactivate

docs/user_guide/bank/images/inventory/inventory_transfers_confirmation_modal.png

@@ -20,7 +20,8 @@ The Comment field is a good place to note the reason for the Transfer, but you c
 
 Then select the Item and quantity for each Item that is being Transferred.  If you have [barcodes](inventory_barcodes.md) set up,  you can use your barcode reader to "boop" in the materials being Transferred.
 
-When you are done, click "Save".  The system will check that you have enough inventory in "From" to cover the Transfer.  If so, the inventory changes will take place immediately.
+When you are done, click "Save". If so, a confirmation modal will pop up. Review the contents of the transfer and click the 'Yes, it's correct' button to begin the transfer. The system will check that you have enough inventory in "From" to cover the Transfer before the inventory changes take place.
+![Transfer Modal](images/inventory/inventory_transfers_confirmation_modal.png)
 
 ## Viewing the details of a Transfer
 To view the details of a Transfer, click the "view" button beside it in the Transfers list.

docs/user_guide/bank/inventory_transfers.md

@@ -43,6 +43,31 @@
     end
 
     describe "POST #create" do
+      context 'when duplicate line_items with different quantities submitted' do
+        let!(:item1) { create(:item) }
+        let(:params) {
+          attributes_for(
+            :transfer,
+            organization_id: organization.id,
+            to_id: create(:storage_location, organization: organization).id,
+            from_id: create(:storage_location, organization: organization).id,
+            line_items_attributes: {
+              "0" => { item_id: item1.id, quantity: 2 },
+              "1" => { item_id: item1.id, quantity: 3 }
+            }
+          )
+        }
+
+        it "merges same line_items id items into one quantity" do
+          post :create, params: { transfer: params }
+
+          transfer = assigns(:transfer)
+
+          expect(transfer.line_items.size).to eq 1
+          expect(transfer.line_items.first.quantity).to eq 5
+        end
+      end
+
       it "redirects to #show when successful" do
         attributes = attributes_for(
           :transfer,
@@ -76,6 +101,34 @@
         expect(subject).to be_successful
       end
     end
+
+    describe "POST #validate" do
+      context 'when duplicate line_items with different quantities submitted' do
+        let!(:item1) { create(:item) }
+        let(:params) {
+          attributes_for(
+            :transfer,
+            organization_id: organization.id,
+            to_id: create(:storage_location, organization: organization).id,
+            from_id: create(:storage_location, organization: organization).id,
+            line_items_attributes: {
+              "0" => { item_id: item1.id, quantity: 2 },
+              "1" => { item_id: item1.id, quantity: 3 }
+            }
+          )
+        }
+
+        it "merges same line_items id items into one quantity" do
+          post :validate, params: { transfer: params }
+
+          transfer = assigns(:transfer)
+
+          expect(transfer.line_items.size).to eq 1
+          expect(transfer.line_items.first.quantity).to eq 5
+        end
+      end
+    end
+
     context "Looking at a different organization" do
       let(:object) do
         org = create(:organization)

spec/controllers/transfers_controller_spec.rb

@@ -348,11 +348,13 @@
 
     it "the user can make changes" do
       click_on "Edit", match: :first
-      expect do
-        fill_in "Agency representative", with: "SOMETHING DIFFERENT"
-        click_on "Save", match: :first
-        distribution.reload
-      end.to change { distribution.agency_rep }.to("SOMETHING DIFFERENT")
+      fill_in "Agency representative", with: "SOMETHING DIFFERENT"
+      click_on "Save", match: :first
+      # Make Capybara wait for events to finish before checking Db.
+      expect(page).to have_content("SOMETHING DIFFERENT")
+
+      distribution.reload
+      expect(distribution.agency_rep).to eq("SOMETHING DIFFERENT")
     end
 
     it "sends an email if reminders are enabled" do

spec/system/distribution_system_spec.rb

@@ -8,7 +8,7 @@
   end
   let(:item) { create(:item) }
 
-  def create_transfer(amount, from_name, to_name)
+  def create_transfer(amount, from_name, to_name, click_save: true, click_confirm: true)
     visit transfers_path
     click_link "New Transfer"
     within "form#new_transfer" do
@@ -18,13 +18,47 @@ def create_transfer(amount, from_name, to_name)
       select item.name, from: "transfer_line_items_attributes_0_item_id"
       fill_in "transfer_line_items_attributes_0_quantity", with: amount
     end
-    click_on "Save"
+    click_on "Save" if click_save
+    click_on "Yes, it's correct" if click_save && click_confirm
+  end
+
+  context "when transfer submitted" do
+    it "shows the confirmation page" do
+      from_storage_location = create(:storage_location, :with_items, item: item, name: "From me", organization: organization)
+      to_storage_location = create(:storage_location, :with_items, name: "To me", organization: organization)
+      create_transfer("10", from_storage_location.name, to_storage_location.name, click_confirm: false)
+
+      expect(page).to have_content("Transfer Confirmation")
+      expect(page).to have_content("Please confirm that the above list is what you meant to transfer and that the comment is correct.")
+      expect(page).to have_content("No, I need to make changes")
+      expect(page).to have_content("Yes, it's correct")
+    end
+
+    it "merges same items into one quantity" do
+      from_storage_location = create(:storage_location, :with_items, item: item, name: "From me", organization: organization)
+      to_storage_location = create(:storage_location, :with_items, name: "To me", organization: organization)
+
+      create_transfer("10", from_storage_location.name, to_storage_location.name, click_save: false, click_confirm: false)
+      # grabs the dynamically generated new item
+      click_on "Add Another Item"
+      new_select = all("select[id$='_item_id']").last
+      select_id = new_select[:id]
+      select item.name, from: select_id
+      index = select_id[/attributes_(\d+)_item_id$/, 1]
+      quantity_field_id = "transfer_line_items_attributes_#{index}_quantity"
+      fill_in quantity_field_id, with: "30"
+
+      click_on "Save"
+
+      expect(page).to have_content("40")
+    end
   end
 
   it "can transfer an inventory from a storage location to another as a user" do
     from_storage_location = create(:storage_location, :with_items, item: item, name: "From me", organization: organization)
     to_storage_location = create(:storage_location, :with_items, name: "To me", organization: organization)
     create_transfer("10", from_storage_location.name, to_storage_location.name)
+
     expect(page).to have_content("10 items have been transferred")
   end
 
@@ -96,7 +130,7 @@ def create_transfer(amount, from_name, to_name)
   # 4438 - Bug Fix
   it "add item button should be activated when from storage location is selected after rendering again on failure" do
     from_storage_location = create(:storage_location, :with_items, item: item, name: "From me", organization: organization)
-    create_transfer('', '', '')
+    create_transfer('', '', '', click_confirm: false)
     select from_storage_location.name, from: "From storage location"
     expect(page).not_to have_css("#__add_line_item.disabled")
   end
@@ -106,7 +140,7 @@ def create_transfer(amount, from_name, to_name)
     let!(:to_storage_location) { create(:storage_location, :with_items, name: "To me", organization: organization) }
 
     scenario "User can transfer an inventory from a storage location to another" do
-      create_transfer("100", from_storage_location.name, to_storage_location.name)
+      create_transfer("100", from_storage_location.name, to_storage_location.name, click_confirm: false)
       expect(page).to have_content("insufficient inventory")
     end
   end