Revert 1550 refactor/create distributions (#1580)

* refactor: show the items that exceed available inventory (#1550) * Revert "refactor: show the items that exceed available inventory (#1550)" This reverts commit 9b065ea. * Re-enable distribution storage location totals, which weren't working. Makes JS slightly easier to read by removing ES shorthand. Uses sanitize instead of raw for alert output. Make error alerts not auto-hide. * Fix #update action. Extracts some insufficiency language to a method. Slight refactoring of the Service objects. Add better UI feedback to the edit & new actions, which required a tweaks to #update and #create * Minor style fixes in flash and redirects in distributions controller * Fixes several failing specs. * Adds ADRs. Creates an ADR about making the application fully-public * Fixes failing specs. Had to make a bunch pending because of capybara weirdness not being able to click on buttons. * Rubocop fixes. Specs are passing now after a rebase. Updates annotations. * Removes unnecessary requirements from the beginnings of some specs. Creates a new spec for the Distribution creation service. * Changes map...to_h to index_by to satisfy rubocop * Whitespace removal * Removes ADR files (wrong app) and fixes whitespace in JS * Update rubocop YML not to follow a cop that violates something we need, fixes whitespace. * Removes 2 unused methods from DistributionsController * Adds newline to .rspec. Removes a commented out line from distributions controller * Adds a space for styling to application.js * Fix wording in distribution system spec Co-authored-by: Gabriel Belgamo <[email protected]>
rubyforgood · Apr 2, 2020 · 45c996f · 45c996f
1 parent 29171ae
commit 45c996f
Show file tree

Hide file tree

Showing 33 changed files with 206 additions and 128 deletions.
diff --git a/.rspec b/.rspec
@@ -1,3 +1,4 @@
 --color
 --require rails_helper
 --format=documentation
+
diff --git a/.rubocop.yml b/.rubocop.yml
@@ -314,3 +314,5 @@ Style/SymbolArray:
   Enabled: false
 Rails/UniqueValidationWithoutIndex: # We use multiple unique validations where the uniqueness is scoped
   Enabled: false
+Lint/EnsureReturn: # The service objects return self in an ensure block. Not using an explicit return does not do correct behavior
+  Enabled: false
diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js
@@ -34,10 +34,10 @@ function isShortHeightScreen() {
   return $(window).height() < 768 && !isMobileResolution();
 }
 
-window.setTimeout(function () {
+window.setTimeout(function() {
   // When the user is given an error message, we should not auto-hide it so that
   // they can fully read it and potentially copy/paste it into an issue.
-  $(".alert").fadeTo(1000, 0).slideUp(1000, function () {
+  $(".alert").not(".error").fadeTo(1000, 0).slideUp(1000, function() {
     $(this).remove();
   });
 }, 2500);

diff --git a/app/assets/javascripts/distributions_and_transfers.js b/app/assets/javascripts/distributions_and_transfers.js
@@ -1,7 +1,7 @@
 // Place all the behaviors and hooks related to the matching controller here.
 // All this logic will automatically be available in application.js.
 
-const new_option = function(item, selected) {
+function new_option(item, selected) {
   if (selected == null) {
     selected = false;
   }
@@ -21,7 +21,7 @@ const new_option = function(item, selected) {
   return content;
 };
 
-const populate_dropdowns = (objects, inventory) =>
+function populate_dropdowns(objects, inventory) {
   objects.each(function(index, element) {
     const selected = Number(
       $(element)
@@ -39,7 +39,9 @@ const populate_dropdowns = (objects, inventory) =>
       .end()
       .append(options);
   });
-const request_storage_location_and_populate_item = function(item_to_populate) {
+}
+
+function request_storage_location_and_populate_item(item_to_populate) {
   const control = $("select.storage-location-source");
   if (control.length > 0 && control.val() !== "") {
     return $.ajax({
@@ -48,7 +50,7 @@ const request_storage_location_and_populate_item = function(item_to_populate) {
         .replace(":id", control.val()),
       dataType: "json",
       success(data) {
-        return populate_dropdowns(item_to_populate, data);
+        return populate_dropdowns($(item_to_populate), data);
       }
     });
   }

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
@@ -1,5 +1,6 @@
 # Base Controller. There is some magic in here that handles organization-scoping for routes
 class ApplicationController < ActionController::Base
+  add_flash_types :error
   include DateHelper
 
   protect_from_forgery with: :exception

diff --git a/app/controllers/audits_controller.rb b/app/controllers/audits_controller.rb
@@ -69,7 +69,7 @@ def create
     if @audit.save
       save_audit_status_and_redirect(params)
     else
-      flash[:error] = @audit.errors.collect { |model, message| "#{model}: " + message }
+      flash[:error] = "<ul><li>" + @audit.errors.collect { |model, message| "#{model}: " + message }.join("</li><li>") + "</li></ul>"
       set_storage_locations
       set_items
       @audit.line_items.build if @audit.line_items.empty?

diff --git a/app/controllers/distributions_controller.rb b/app/controllers/distributions_controller.rb
@@ -6,7 +6,6 @@
 class DistributionsController < ApplicationController
   include DateRangeHelper
   include DistributionHelper
-  rescue_from Errors::InsufficientAllotment, with: :insufficient_amount!
 
   def print
     @distribution = Distribution.find(params[:id])
@@ -26,11 +25,11 @@ def destroy
 
     if result.success?
       flash[:notice] = "Distribution #{params[:id]} has been reclaimed!"
-      redirect_to distributions_path
     else
       flash[:error] = "Could not destroy distribution #{params[:id]}. Please contact technical support."
-      redirect_to action: :edit
     end
+
+    redirect_to distributions_path
   end
 
   def index
@@ -58,12 +57,12 @@ def create
     result = DistributionCreateService.new(distribution_params.merge(organization: current_organization), request_id).call
 
     if result.success?
-      flash[:notice] = "Distribution created!"
       session[:created_distribution_id] = result.distribution.id
-      redirect_to(distributions_path) && return
+      redirect_to(distributions_path, notice: "Distribution created!") && return
     else
       @distribution = result.distribution
-      flash[:error] = "Sorry, we weren't able to save the distribution. \n #{@distribution.errors.full_messages.join(', ')} #{result.error}"
+      flash[:error] = insufficient_error_message(result.error.message)
+      # NOTE: Can we just do @distribution.line_items.build, regardless?
       @distribution.line_items.build if @distribution.line_items.count.zero?
       @items = current_organization.items.alphabetized
       @storage_locations = current_organization.storage_locations.alphabetized
@@ -95,32 +94,30 @@ def edit
       @items = current_organization.items.alphabetized
       @storage_locations = current_organization.storage_locations.alphabetized
     else
-      flash[:error] = 'To edit a distribution,
+      redirect_to distributions_path, error: 'To edit a distribution,
       you must be an organization admin or the current date must be later than today.'
-      redirect_to distributions_path
     end
   end
 
   def update
-    old_distribution = Distribution.includes(:line_items).includes(:storage_location).find(params[:id])
+    @distribution = Distribution.includes(:line_items).includes(:storage_location).find(params[:id])
 
-    result = DistributionUpdateService.new(old_distribution, distribution_params).call
+    result = DistributionUpdateService.new(@distribution, distribution_params).call
 
     if result.success?
-      @distribution = Distribution.includes(:line_items).includes(:storage_location).find(params[:id])
-      @line_items = @distribution.line_items
-
       if result.resend_notification?
         send_notification(current_organization.id, @distribution.id, subject: "Your Distribution New Schedule Date is #{@distribution.issued_at}")
       end
 
       schedule_reminder_email(@distribution.id)
 
-      flash[:notice] = "Distribution updated!"
-      render :show
+      redirect_to @distribution, notice: "Distribution updated!"
     else
-      flash[:error] = "Distribution could not be updated! Are you sure there are enough items in inventory to update this distribution?"
-      redirect_to action: :edit
+      flash[:error] = insufficient_error_message(result.error.message)
+      @distribution.line_items.build if @distribution.line_items.count.zero?
+      @items = current_organization.items.alphabetized
+      @storage_locations = current_organization.storage_locations.alphabetized
+      render :edit
     end
   end
 
@@ -146,22 +143,10 @@ def pickup_day
     @selected_date = pickup_day_params[:during]&.to_date || Time.zone.now.to_date
   end
 
-  # TODO: This shouldl probably be private
-  def insufficient_amount!
-    respond_to do |format|
-      format.html { render template: "errors/insufficient", layout: "layouts/application", status: :ok }
-      format.json { render nothing: true, status: :ok }
-    end
-  end
-
   private
 
-  # If a request id is provided, update the request with the newly created distribution's id
-  def update_request(request_atts, distribution_id)
-    return if request_atts.blank?
-
-    request = Request.find(request_atts[:id])
-    request.update(distribution_id: distribution_id, status: 'fulfilled')
+  def insufficient_error_message(details)
+    "Sorry, we weren't able to save the distribution. \n #{@distribution.errors.full_messages.join(', ')} #{details}"
   end
 
   def send_notification(org, dist, subject: 'Your Distribution')

diff --git a/app/models/barcode_item.rb b/app/models/barcode_item.rb
@@ -4,6 +4,7 @@
 #
 #  id               :integer          not null, primary key
 #  barcodeable_type :string           default("Item")
+#  global           :boolean          default(FALSE)
 #  quantity         :integer
 #  value            :string
 #  created_at       :datetime         not null

diff --git a/app/models/concerns/itemizable.rb b/app/models/concerns/itemizable.rb
@@ -14,6 +14,15 @@ module Itemizable
     end
 
     has_many :line_items, as: :itemizable, inverse_of: :itemizable do
+      def assign_insufficiency_errors(insufficiency_hash)
+        insufficiency_hash = insufficiency_hash.index_by { |i| i[:item_id] }
+        each do |line_item|
+          next unless insufficiency = insufficiency_hash.fetch(line_item.item_id)
+
+          line_item.errors.add(:quantity, :insufficient, message: "too high. Change to #{insufficiency[:quantity_on_hand]} or less")
+        end
+      end
+
       def combine!
         # Bail if there's nothing
         return if size.zero?

diff --git a/app/models/diaper_drive.rb b/app/models/diaper_drive.rb
@@ -2,12 +2,13 @@
 #
 # Table name: diaper_drives
 #
-#  id         :bigint           not null, primary key
-#  end_date   :date
-#  name       :string
-#  start_date :date
-#  created_at :datetime         not null
-#  updated_at :datetime         not null
+#  id              :bigint           not null, primary key
+#  end_date        :date
+#  name            :string
+#  start_date      :date
+#  created_at      :datetime         not null
+#  updated_at      :datetime         not null
+#  organization_id :bigint
 #
 
 class DiaperDrive < ApplicationRecord

diff --git a/app/models/organization.rb b/app/models/organization.rb
@@ -2,7 +2,7 @@
 #
 # Table name: organizations
 #
-#  id                       :integer          not null, primary key
+#  id                       :bigint           not null, primary key
 #  city                     :string
 #  deadline_day             :integer
 #  default_storage_location :integer

diff --git a/app/models/storage_location.rb b/app/models/storage_location.rb
@@ -146,7 +146,6 @@ def decrease_inventory(itemizable_array)
         quantity_requested: item_hash[:quantity]
       }
     end
-
     # NOTE: Could this be handled by a validation instead?
     # If we found any insufficiencies
     unless insufficient_items.empty?

diff --git a/app/services/distribution_create_service.rb b/app/services/distribution_create_service.rb
@@ -1,8 +1,11 @@
 class DistributionCreateService
+  attr_reader :distribution, :error
+
   def initialize(distribution_params, request_id = nil)
     @distribution = Distribution.new(distribution_params)
     @request = Request.find(request_id) if request_id
     @organization = @distribution.organization
+    @error = nil
   end
 
   def call
@@ -13,11 +16,19 @@ def call
       @distribution.reload
       @request&.update!(distribution_id: @distribution.id, status: 'fulfilled')
       PartnerMailerJob.perform_async(@organization.id, @distribution.id, 'Your Distribution') if Flipper.enabled?(:email_active)
-
-      OpenStruct.new(success?: true, distribution: @distribution)
     end
+  rescue Errors::InsufficientAllotment => e
+    @distribution.line_items.assign_insufficiency_errors(e.insufficient_items)
+    Rails.logger.error "[!] DistributionsController#create failed because of Insufficient Allotment #{@organization.short_name}: #{@distribution.errors.full_messages} [#{e.message}]"
+    @error = e
   rescue StandardError => e
-    Rails.logger.error "[!] DistributionsController#create failed to save distribution for #{@distribution.organization.short_name}: #{@distribution.errors.full_messages} [#{e.inspect}]"
-    OpenStruct.new(success: false, distribution: @distribution, error: e)
+    Rails.logger.error "[!] DistributionsController#create failed to save distribution for #{@organization.short_name}: #{@distribution.errors.full_messages} [#{e.inspect}]"
+    @error = e
+  ensure
+    return self
+  end
+
+  def success?
+    @error.nil?
   end
 end
diff --git a/app/services/distribution_update_service.rb b/app/services/distribution_update_service.rb
@@ -1,35 +1,44 @@
 class DistributionUpdateService
+  attr_reader :distribution, :error
+
   def initialize(old_distribution, new_distribution_params)
     @distribution = old_distribution
     @params = new_distribution_params
+    @organization = @distribution.organization
+    @error = nil
   end
 
+  # FIXME: This doesn't allow for the storage location to be changed.
   def call
     @distribution.transaction do
-      old_issued_at = @distribution.issued_at
+      @old_issued_at = @distribution.issued_at
       @distribution.storage_location.increase_inventory(@distribution.to_a)
 
       # Delete the line items -- they'll be replaced later
       @distribution.line_items.each(&:destroy!)
       @distribution.reload
-
       # Replace the current distribution with the new parameters
       @distribution.update! @params
       @distribution.reload
-      new_issued_at = @distribution.issued_at
+      @new_issued_at = @distribution.issued_at
       # Apply the new changes to the storage location inventory
       @distribution.storage_location.decrease_inventory(@distribution.to_a)
-      OpenStruct.new(success?: true, distribution: @distribution,
-                     resend_notification?: resend_notification?(old_issued_at, new_issued_at))
     end
+  rescue Errors::InsufficientAllotment => e
+    @distribution.line_items.assign_insufficiency_errors(e.insufficient_items)
+    Rails.logger.error "[!] DistributionsController#update failed because of Insufficient Allotment #{@organization.short_name}: #{@distribution.errors.full_messages} [#{e.message}]"
+    @error = e
   rescue StandardError => e
     Rails.logger.error "[!] DistributionsController#update failed to update distribution for #{@distribution.organization.short_name}: #{@distribution.errors.full_messages} [#{e.inspect}]"
-    OpenStruct.new(success: false, distribution: @distribution, error: e)
+  ensure
+    return self
   end
 
-  private
+  def success?
+    @error.nil?
+  end
 
-  def resend_notification?(old_issued_at, new_issued_at)
-    old_issued_at.to_date != new_issued_at.to_date
+  def resend_notification?
+    @old_issued_at.to_date != @new_issued_at.to_date
   end
 end
diff --git a/app/views/distributions/edit.html.erb b/app/views/distributions/edit.html.erb
@@ -68,7 +68,7 @@
                   <fieldset style="margin-bottom: 2rem;" class="form-inline">
                     <legend>Items in this distribution</legend>
                     <%= f.simple_fields_for :line_items do |item| %>
-                      <div id="distribution_line_items" data-capture-barcode="true">
+                      <div id="distribution_line_items" data-capture-barcode="true" class="line-item-fields">
                         <%= render 'line_items/line_item_fields', f: item %>
                       </div>
                     <% end %>

diff --git a/app/views/distributions/new.html.erb b/app/views/distributions/new.html.erb
@@ -58,7 +58,7 @@
                   <fieldset style="margin-bottom: 2rem;" class="form-inline">
                     <legend>Items in this distribution</legend>
                     <%= f.simple_fields_for :line_items do |item| %>
-                      <div id="distribution_line_items" data-capture-barcode="true">
+                      <div id="distribution_line_items" data-capture-barcode="true" class="line-item-fields">
                         <%= render 'line_items/line_item_fields', f: item %>
                       </div>
                     <% end %>

diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb
@@ -70,9 +70,9 @@
 
   <div class="content-wrapper">
     <% flash.each do |key, value| %>
-      <div class="alert alert-<%= flash_class(key) %> alert-dismissible fade show" role="alert">
+      <div class="<%= flash_class(key) %> alert-dismissible fade show" role="alert">
         <a href="#" class="close" data-dismiss="alert" aria-label="Close" style="text-decoration: none;"><%= fa_icon('times') %></a>
-        <%= value %>
+        <%= sanitize(value, tags: %w(ul li)) %>
       </div>
     <% end %>
     <%= yield %>

diff --git a/config/environments/test.rb b/config/environments/test.rb
@@ -1,3 +1,6 @@
+# 1 Apr 2020 [AMH] : Deprecation warnings have been VERY verbose lately. This hides them.
+Warning[:deprecated] = false
+
 Rails.application.configure do
   # Settings specified here will take precedence over those in config/application.rb.
 

diff --git a/spec/factories/barcode_items.rb b/spec/factories/barcode_items.rb
@@ -4,6 +4,7 @@
 #
 #  id               :integer          not null, primary key
 #  barcodeable_type :string           default("Item")
+#  global           :boolean          default(FALSE)
 #  quantity         :integer
 #  value            :string
 #  created_at       :datetime         not null