Merge pull request #644 from YaleSTC/573_validations_lol

Fundamental Rewrite of Validation System

Author: squidgetx

app/controllers/application_controller.rb

@@ -131,7 +131,7 @@ def update_cart
     end
 
     # validate
-    errors = Reservation.validate_set(cart.reserver, cart.prepare_all)
+    errors = cart.validate_dates.concat(cart.validate_dates_and_items)
     # don't over-write flash if invalid date was set above
     flash[:error] ||= errors.to_sentence
     flash[:notice] = "Cart updated."

app/controllers/catalog_controller.rb

@@ -56,7 +56,7 @@ def search
     # (or displays the appropriate errors)
     def change_cart(action, item)
       cart.send(action, item)
-      errors = Reservation.validate_set(cart.reserver, cart.prepare_all)
+      errors = cart.validate_items.concat(cart.validate_dates_and_items)
       flash[:error] = errors.to_sentence
       flash[:notice] = "Cart updated."
 

app/controllers/reservations_controller.rb

@@ -49,7 +49,7 @@ def new
       redirect_to catalog_path
     else
       # error handling
-      @errors = Reservation.validate_set(cart.reserver, cart.prepare_all)
+      @errors = cart.validate_all
       unless @errors.empty?
         if can? :override, :reservation_errors
           flash[:error] = 'Are you sure you want to continue? Please review the errors below.'
@@ -69,7 +69,7 @@ def create
     Reservation.transaction do
       begin
         cart_reservations = cart.prepare_all
-        @errors = Reservation.validate_set(cart.reserver, cart_reservations)
+        @errors = cart.validate_all
         if @errors.empty?
           # If the reservation is a finalized reservation, save it as auto-approved ...
           params[:reservation][:approval_status] = "auto"
@@ -87,8 +87,6 @@ def create
         cart_reservations.each do |cart_res|
           @reservation = Reservation.new(params[:reservation])
           @reservation.equipment_model =  cart_res.equipment_model
-          # TODO: is this line needed? it's ugly. we should refactor if it's necessary.
-          @reservation.bypass_validations = true
           @reservation.save!
           successful_reservations << @reservation
         end
@@ -381,10 +379,8 @@ def renew
 
   def review
     set_reservation
-    array_for_validation = []
-    array_for_validation << @reservation
     @all_current_requests_by_user = @reservation.reserver.reservations.requested.delete_if{|res| res.id == @reservation.id}
-    @errors = Reservation.validate_set(@reservation.reserver, array_for_validation)
+    @errors = @reservation.validate
   end
 
   def approve_request

app/models/blackout.rb

@@ -14,8 +14,12 @@ class Blackout < ActiveRecord::Base
   validate :validate_end_date_before_start_date
   # this only matters if a user tries to inject into params because the datepicker
   # doesn't allow form submission of invalid dates
-  
-  scope :active, where("end_date >= ?", Date.today) 
+
+  scope :active, where("end_date >= ?", Date.today)
+  scope :for_date, lambda { |date| where("end_date >= ? and start_date <= ?", date, date) }
+  scope :hard, where(blackout_type: 'hard')
+  scope :soft, where(blackout_type: 'notice only')
+
   def self.blackouts_on_date(date) # Returns the blackout object that blacks out the day if the day is blacked out. Otherwise, returns nil.
     blackouts = []
     Blackout.all.each do |blackout|

app/models/cart.rb

@@ -1,5 +1,6 @@
 class Cart
   include ActiveModel::Validations
+  include CartValidations
   validates :reserver_id, :start_date, :due_date, presence: true
 
   attr_accessor :items, :start_date, :due_date, :reserver_id
@@ -18,17 +19,27 @@ def persisted?
 
   ## Item methods
 
+  def get_items
+    # Used in cart_validations
+    # Return items where the key is the full equipment model object
+    # uses 1 database call
+    full_hash = Hash.new
+    EquipmentModel.find(self.items.keys).each do |em|
+      full_hash[em] = self.items[em.id]
+    end
+    full_hash
+  end
   # Adds equipment model id to items hash
   def add_item(equipment_model)
     return if equipment_model.nil?
-    key = equipment_model.id.to_s
+    key = equipment_model.id
     self.items[key] = self.items[key] ? self.items[key] + 1 : 1
   end
 
   # Remove equipment model id from items hash, or decrement its count
   def remove_item(equipment_model)
     return if equipment_model.nil?
-    key = equipment_model.id.to_s
+    key = equipment_model.id
     self.items[key] = self.items[key] ? self.items[key] - 1 : 0
     self.items = self.items.except(key) if self.items[key] <= 0
   end

app/models/cart_validations.rb

@@ -0,0 +1,111 @@
+module CartValidations
+
+  # These validation methods each run several validations within them
+  # for the sake of speed and query-saving. They are separated in 3 methods
+  # so that the cart doesn't have to validate items when only the dates are
+  # changed and vice versa. Each method returns an array of error messages
+  #
+  #
+  # Validate Dates (when dates are changed)
+  # ## start, end not on a blackout date
+  # ## user has no overdue (reserver is included in the date form)
+  #
+  # Validate Items (when items are added/removed)
+  # ## user doesn't have too many of each equipment model
+  # ## or each category
+  #
+  # Validate Dates and Items
+  # ## items are all available for the date range
+  # ## the duration of the date range is short enough
+  # ## none of the items should be renewed instead of re-reserved
+  #
+  # Validate All
+  # ## just runs everything
+
+
+  def validate_dates
+    errors = []
+
+    # blackouts
+    errors << "A reservation cannot start on #{self.start_date.to_date.strftime('%m/%d')}" if Blackout.hard.for_date(self.start_date).count > 0
+    errors << "A reservation cannot end on #{self.due_date.to_date.strftime('%m/%d')}" if Blackout.hard.for_date(self.due_date).count > 0
+
+    # no overdue reservations
+    errors << "This user has overdue reservations that prevent him/her from creating new ones" if Reservation.for_reserver(self.reserver_id).overdue.count > 0
+    return errors
+  end
+
+  def validate_items
+    errors = []
+    relevant = Reservation.for_reserver(self.reserver_id).not_returned
+    category = Hash.new
+
+    # get hash of model objects and quantities
+    models = self.get_items
+
+    # check max model count for each day in the range
+    # while simultaneously building a hash of category => quantity
+    models.each do |model, quantity|
+      max_models = model.maximum_per_user
+      self.start_date.to_date.upto(self.due_date.to_date) do |d|
+        if relevant.overlaps_with_date(d).for_eq_model(model).count + quantity > max_models
+          errors << "Cannot reserve more than #{max_models} #{model.name.pluralize}"
+          break
+        end
+      end
+
+      if category.include?(model.category)
+        category[model.category] += quantity
+      else
+        category[model.category] = quantity
+      end
+
+    end
+
+    # similarly check category maxes using a similar method
+    category.each do |cat, q|
+      max_cat = cat.maximum_per_user
+      self.start_date.to_date.upto(self.due_date.to_date) do |d|
+        count = 0
+        relevant.overlaps_with_date(d).each do |r|
+
+          count += 1 if r.equipment_model.category == cat
+        end
+        if count + q > max_cat
+          errors << "Cannot reserve more than #{max_cat} #{cat.name.pluralize}"
+          break
+        end
+      end
+    end
+
+    return errors
+  end
+
+  def validate_dates_and_items
+    user_reservations = Reservation.for_reserver(self.reserver_id).checked_out
+    errors = []
+    models = self.get_items
+    models.each do |model, quantity|
+
+      # check availability
+      errors << "That many #{model.name.pluralize} is not available for the given time range" if model.num_available(self.start_date, self.due_date) < quantity
+
+      # check maximum checkout length
+      max_length = model.category.max_checkout_length
+      max_length = Float::INFINITY if max_length == 'unrestricted'
+      errors << "#{model.name.titleize} can only be reserved for #{max_length} days" if self.duration > max_length
+
+      # if a reservation should be renewed instead of checked out
+      user_reservations.for_eq_model(model).each do |r|
+        errors << "#{model.name.titleize} should be renewed instead of re-checked out"  if r.due_date == self.start_date && r.is_eligible_for_renew?
+      end
+    end
+    return errors
+  end
+
+  def validate_all
+    errors = validate_dates
+    errors.concat(validate_items.to_a).concat(validate_dates_and_items.to_a)
+    return errors
+  end
+end

app/models/equipment_model.rb

@@ -169,15 +169,11 @@ def num_available(start_date, due_date)
       not_returned
     num_available_from_source(start_date, due_date, relevant_reservations)
   end
+
   # Returns true if the reserver is ineligible to checkout the model.
   def model_restricted?(reserver_id)
     reserver = User.find(reserver_id)
-    reqs = reserver.requirements
-    return false if self.requirements.blank?
-    self.requirements.each do |em_req|
-      return false if reqs.include?(em_req)
-    end
-    return true
+    !(self.requirements - reserver.requirements).empty?
   end
 
   # Returns the number of overdue objects for a given model,

app/models/reservation.rb

@@ -8,24 +8,11 @@ class Reservation < ActiveRecord::Base
   belongs_to :checkout_handler, class_name: 'User'
   belongs_to :checkin_handler, class_name: 'User'
 
-  validates :equipment_model, presence: true
+  validates :equipment_model, :start_date, :due_date, presence: true
+  validate :start_date_before_due_date
+  validate :matched_object_and_model
+  validate :not_in_past, :available, on: :create
 
-  # If there is no equipment model, don't run the validations that would break
-  with_options if: :not_empty? do |r|
-    r.validate  :start_date_before_due_date?, :matched_object_and_model?,
-                :available?
-  end
-
-  # These can't be nested with the above block because with_options clobbers
-  # nested options that are the same (i.e., :if and :if)
-  with_options if: Proc.new {|r| r.not_empty? && !r.bypass_validations} do |r|
-    r.with_options on: :create do |r|
-      r.validate  :not_in_past?, :not_renewable?, :no_overdue_reservations?,
-                  :duration_allowed?, :start_date_is_not_blackout?,
-                  :due_date_is_not_blackout?, :quantity_eq_model_allowed?,
-                  :quantity_cat_allowed?
-    end
-  end
 
   nilify_blanks only: [:notes]
 
@@ -57,10 +44,11 @@ class Reservation < ActiveRecord::Base
   scope :for_reserver, lambda { |reserver| where(reserver_id: reserver) }
   scope :reserved_in_date_range, lambda { |start_date, end_date|
     where("start_date < ? and due_date > ? and (approval_status = ? or approval_status = ?)", end_date, start_date, 'auto', 'approved') }
+  scope :overlaps_with_date, lambda{ |date| where("start_date <= ? and due_date >= ?",date.to_datetime,date.to_datetime) }
 
   #TODO: Why the duplication in checkout_handler and checkout_handler_id (etc)?
-  attr_accessible :checkout_handler, :checkout_handler_id,
-                  :checkin_handler, :checkin_handler_id, :approval_status,
+  attr_accessible :checkout_handler_id,
+                  :checkin_handler_id, :approval_status,
                   :checked_out, :checked_in, :equipment_object,
                   :equipment_object_id, :notes, :notes_unsent, :times_renewed
 
@@ -79,6 +67,18 @@ def reserver
               affiliation: "Deleted")
   end
 
+  def validate
+    # Convert reservation to a cart object and run validations on it
+    # For hard validations, use reservation.valid
+    temp_cart = Cart.new
+    temp_cart.start_date = self.start_date
+    temp_cart.due_date = self.due_date
+    temp_cart.reserver_id = self.reserver_id
+    temp_cart.items = { self.equipment_model.id => 1 }
+    temp_cart.validate_all
+  end
+
+
   def status
     if checked_out.nil?
       if approval_status == 'auto' or approval_status == 'approved'
@@ -95,32 +95,6 @@ def status
     end
   end
 
-  ## Set validation
-  # Checks all validations for
-  # the array of reservations passed in (use with cart.cart_reservations)
-  # Returns an array of error messages or [] if reservations are all valid
-  def self.validate_set(user, res_array = [])
-    errors = []
-    #User reservation validations
-    errors << user.name + " has overdue reservations that prevent new ones from being created. " unless user.reservations.overdue.empty?
-
-    res_array.each do |res|
-      errors << "Reservation cannot be made in the past. " unless res.not_in_past?
-      errors << "Reservation start date must be before due date. " unless res.start_date_before_due_date?
-      errors << "Reservation must be for a piece of equipment. " unless res.not_empty?
-      errors << "#{res.equipment_object.name} must be of type #{res.equipment_model.name}. " unless res.matched_object_and_model?
-      errors << "#{res.equipment_model.name} should be renewed instead of re-checked out. " unless res.not_renewable?
-      errors << "#{res.equipment_model.name} cannot be reserved for more than #{res.equipment_model.category.maximum_checkout_length.to_s} days at a time. " unless res.duration_allowed?
-      errors << "#{res.equipment_model.name} is not available for the full time period requested. " unless res.available?(res_array)
-      errors << "A reservation cannot start on #{res.start_date.strftime('%m/%d')} because equipment cannot be picked up on that date. " unless res.start_date_is_not_blackout?
-      errors << "A reservation cannot end on #{res.due_date.strftime('%m/%d')} because equipment cannot be returned on that date. " unless res.due_date_is_not_blackout?
-      errors << "Cannot reserve more than #{res.equipment_model.maximum_per_user.to_s} #{res.equipment_model.name.pluralize}. " unless res.quantity_eq_model_allowed? res_array
-      errors << "Cannot reserve more than #{res.equipment_model.category.maximum_per_user.to_s} #{res.equipment_model.category.name.pluralize}. " unless res.quantity_cat_allowed? res_array
-    end
-    errors.uniq
-  end
-
-
   def checkout_object_uniqueness(reservations)
     object_ids_taken = []
     reservations.each do |r|