Merge pull request #587 from YaleSTC/587_fix_cart_latency

[Fix Cart Latency] Remove CartReservations!

Author: squidgetx

app/controllers/application_controller.rb

@@ -66,7 +66,7 @@ def first_time_user
   def cart
     session[:cart] ||= Cart.new
     if session[:cart].reserver_id.nil?
-      session[:cart].set_reserver_id(current_user.id) if current_user
+      session[:cart].reserver_id = current_user.id if current_user
     end
     session[:cart]
   end
@@ -110,7 +110,7 @@ def check_view_mode
   end
 
   def fix_cart_date
-    cart.set_start_date(Date.today) if cart.start_date < Date.today
+    cart.start_date = (Date.today) if cart.start_date < Date.today
   end
 
   #-------- end before_filter methods --------#
@@ -120,16 +120,17 @@ def update_cart
     cart = session[:cart]
     flash.clear
     begin
-      cart.set_start_date(Date.strptime(params[:cart][:start_date_cart],'%m/%d/%Y'))
-      cart.set_due_date(Date.strptime(params[:cart][:due_date_cart],'%m/%d/%Y'))
-      cart.set_reserver_id(params[:reserver_id])
-    rescue ArgumentError => e
-      cart.set_start_date(Date.today)
+      cart.start_date = Date.strptime(params[:cart][:start_date_cart],'%m/%d/%Y')
+      cart.due_date = Date.strptime(params[:cart][:due_date_cart],'%m/%d/%Y')
+      cart.fix_due_date
+      cart.reserver_id = params[:reserver_id]
+    rescue ArgumentError
+      cart.start_date = Date.today
       flash[:error] = "Please enter a valid start or due date."
     end
 
     # validate
-    errors = Reservation.validate_set(cart.reserver, cart.cart_reservations)
+    errors = Reservation.validate_set(cart.reserver, cart.prepare_all)
     # don't over-write flash if invalid date was set above
     flash[:error] ||= errors.to_sentence
     flash[:notice] = "Cart updated."
@@ -143,13 +144,8 @@ def update_cart
   end
 
   def empty_cart
-    #destroy old cart reservations
-    current_cart = session[:cart]
-    CartReservation.where(reserver_id: current_cart.reserver.id).destroy_all
-
     session[:cart] = nil
     flash[:notice] = "Cart emptied."
-
     redirect_to root_path
   end
 

app/controllers/catalog_controller.rb

@@ -56,8 +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.cart_reservations)
+      errors = Reservation.validate_set(cart.reserver, cart.prepare_all)
       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.cart_reservations)
+      @errors = Reservation.validate_set(cart.reserver, cart.prepare_all)
       unless @errors.empty?
         if can? :override, :reservation_errors
           flash[:error] = 'Are you sure you want to continue? Please review the errors below.'
@@ -68,7 +68,8 @@ def create
     #using http://stackoverflow.com/questions/7233859/ruby-on-rails-updating-multiple-models-from-the-one-controller as inspiration
     Reservation.transaction do
       begin
-        @errors = Reservation.validate_set(cart.reserver, cart.cart_reservations)
+        cart_reservations = cart.prepare_all
+        @errors = Reservation.validate_set(cart.reserver, cart_reservations)
         if @errors.empty?
           # If the reservation is a finalized reservation, save it as auto-approved ...
           params[:reservation][:approval_status] = "auto"
@@ -83,7 +84,7 @@ def create
           success_message = "This request has been successfully submitted, and is now subject to approval by an administrator."
         end
 
-        cart.cart_reservations.each do |cart_res|
+        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.
@@ -92,7 +93,6 @@ def create
           successful_reservations << @reservation
         end
 
-        cart.items.each { |item| CartReservation.delete(item) }
         session[:cart] = Cart.new
 
         # emails are probably failing---this code was already commented out 2014.06.19, and we don't know why.

app/models/cart.rb

@@ -1,17 +1,12 @@
-#TODO: change set_reserver_id, set_start_date, and set_due_date to use update_all
-
 class Cart
   include ActiveModel::Validations
-  extend ActiveModel::Naming
-
   validates :reserver_id, :start_date, :due_date, presence: true
 
   attr_accessor :items, :start_date, :due_date, :reserver_id
-  attr_reader   :errors
 
   def initialize
     @errors = ActiveModel::Errors.new(self)
-    @items = []
+    @items = Hash.new()
     @start_date = Date.today
     @due_date = Date.tomorrow
     @reserver_id = nil
@@ -21,123 +16,59 @@ def persisted?
     false
   end
 
-  ## Functions for error handling
-
-  def read_attribute_for_validation(attr)
-    send(attr)
-  end
-
-  def Cart.human_attribute_name(attr, options = {})
-    attr
-  end
-
-  def Cart.lookup_ancestors
-    [self]
-  end
-
   ## Item methods
 
-  # Adds CartReservation to database; saves ID into items array
+  # Adds equipment model id to items hash
   def add_item(equipment_model)
-    current_item = CartReservation.new(start_date: @start_date,
-      due_date: @due_date, reserver: self.reserver)
-    current_item.equipment_model = equipment_model
-    if current_item.save
-      @items << current_item.id
-    end
+    return if equipment_model.nil?
+    key = equipment_model.id.to_s
+    self.items[key] = self.items[key] ? self.items[key] + 1 : 1
   end
 
-  # Removes CartReservation from database and ID from items array
+  # Remove equipment model id from items hash, or decrement its count
   def remove_item(equipment_model)
-    to_be_deleted = nil
-    @items.each { |item| to_be_deleted = item if CartReservation.find(item).equipment_model == equipment_model }
-    CartReservation.delete(to_be_deleted)
-    @items.delete(to_be_deleted)
-  end
-
-  # Returns CartReservations that correspond to the IDs in the items array
-  def cart_reservations
-    CartReservation.where(id: @items)
-  end
-
-  # Returns a hash of the equipment models in the cart with their quantities
-  def models_with_quantities
-    models = Hash.new
-    cart_reservations.each { |res| models[res.equipment_model.id] = res.same_model_count(cart_reservations) }
-    models
+    return if equipment_model.nil?
+    key = equipment_model.id.to_s
+    self.items[key] = self.items[key] ? self.items[key] - 1 : 0
+    self.items = self.items.except(key) if self.items[key] <= 0
   end
 
   def empty?
     @items.empty?
   end
 
-  ## Date methods
-
-  # Sets start date and updates all CartReservations to match
-  def set_start_date(date)
-    date = date.to_time.in_time_zone
-    if date >= Date.today
-      @start_date = date
-      fix_due_date
-      cart_reservations.update_all({start_date: @start_date, due_date: @due_date})
-    end
-  end
-
-  # Sets due date and updates all CartReservations to match
-  def set_due_date(date)
-    date = date.to_time.in_time_zone
-    @due_date = date
-    fix_due_date
-    cart_reservations.update_all({start_date: @start_date, due_date: @due_date})
-  end
-
-  # If the dates were illogical, sets due date to day after start date
-  def fix_due_date
-    if @start_date.to_time.in_time_zone > @due_date
-      #TODO: allow admin to set default reservation length and respect that length here
-      @due_date = @start_date + 1.day
-    end
+  # remove all items from cart
+  def purge_all
+    @items = Hash.new()
   end
 
-  #Create an array of all the reservations that should be renewed instead of having a new reservation
-  def renewable_reservations
-    user_reservations = reserver.reservations
-    renewable_reservations = []
-    @items.each do |item|
-      cart_item_count = item.quantity #renew up to this many of the item
-      matching_reservations = user_reservations.each do |res|
-        # the end date should be the same as the start date
-        # the reservation should be renewable
-        # also the user should only renew as many reservations as they have in their cart
-        if (res.due_date.to_date == @start_date &&
-           res.equipment_model_id == item.equipment_model_id &&
-           cart_item_count > 0 &&
-           res.is_eligible_for_renew?)
-          renewable_reservations << res
-          cart_item_count-= 1
-        end
+  # return array of reservations crafted from the cart contents
+  def prepare_all
+    reservations = []
+    @items.each do |id, quantity|
+      quantity.times do
+        reservations << Reservation.new(reserver: self.reserver,
+                                        start_date: @start_date,
+                                        due_date: @due_date,
+                                        equipment_model_id: id)
       end
     end
-    return renewable_reservations
+    reservations
   end
 
-
   # Returns the cart's duration
   def duration #in days
     @due_date.to_date - @start_date.to_date + 1
   end
 
-  ## Reserver methods
-
-  #TODO: should only have to set reserver OR reserver_id
-  # Sets reserver id and updates the CartReservations to match
-  def set_reserver_id(user_id)
-    @reserver_id = user_id
-    cart_reservations.update_all(reserver_id: @reserver_id)
-  end
-
   # Returns the reserver
   def reserver
-    reserver = User.find(@reserver_id)
+    User.find(@reserver_id)
+  end
+
+  def fix_due_date
+    if @start_date > @due_date
+      @due_date = @start_date + 1.day
+    end
   end
 end

app/models/cart_reservation.rb

@@ -105,11 +105,11 @@ def self.validate_set(user, res_array = [])
     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? if self.class == CartReservation
+      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? if self.class == CartReservation
+      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?
@@ -162,7 +162,6 @@ def is_eligible_for_renew?
 
     max_renewal_days = self.equipment_model.maximum_renewal_days_before_due
     max_renewal_days = Float::INFINITY if max_renewal_days == 'unrestricted'
-
     return ((self.due_date.to_date - Date.today).to_i < max_renewal_days ) &&
       (self.times_renewed < max_renewal_times)
   end

app/models/reservation.rb

@@ -58,7 +58,6 @@ def matched_object_and_model?
   # Checks that the reservation is not renewable
   #TODO: allow admin override
   def not_renewable?
-    return true unless self.class == CartReservation || self.status == "reserved"
     reserver.reservations.each do |res|
       if res.equipment_model == self.equipment_model && res.due_date.to_date == self.start_date.to_date && res.is_eligible_for_renew?
         errors.add(:base, res.equipment_model.name + " should be renewed instead of re-checked out.\n")
@@ -184,7 +183,7 @@ def get_overlapping_reservations(reservations)
     #include all reservations made by user
     reservations.concat(reserver.reservations)
     reservations.uniq!
-    reservations.select{ |res| res.overlaps_with?(self) && (res.class == CartReservation || res.checked_in == nil) }
+    reservations.select{ |res| res.overlaps_with?(self) && (res.checked_in == nil) }
   end
 
 

app/models/reservation_validations.rb

@@ -1,9 +1,9 @@
 <div id="list_items_in_cart">
-  <% if cart.cart_reservations.size == 0 %>
+  <% if cart.items.empty? %>
     <span id="cart_is_empty">(empty)</span>
   <% else %>
     <table id="table_list_items" class="table table-bordered table-striped table-condensed">
-      <% cart.models_with_quantities.each_pair do |model_id, quantity| %>
+      <% cart.items.each do |model_id, quantity| %>
         <tr>
           <td colspan="2"><%= quantity %> x
             <div id="remove_button"><%= link_to "Remove", remove_from_cart_path(EquipmentModel.find(model_id)),
@@ -14,4 +14,4 @@
       <% end %>
     </table>
   <% end %>
-</div>
+</div>

app/views/reservations/_list_items_in_cart.html.erb

@@ -30,7 +30,7 @@
   </p>
   <p>
     <ul class="rounded-list">
-      <% cart.models_with_quantities.each_pair do |model_id, quantity| %>
+      <% cart.items.each do |model_id, quantity| %>
         <li data-value="<%= quantity %>"><%= link_to EquipmentModel.find(model_id).name, EquipmentModel.find(model_id) %></li>
       <% end %>
     </ul>

app/views/reservations/_new_request.html.erb

@@ -30,7 +30,7 @@
   </p>
   <p>
     <ul class="rounded-list">
-      <% cart.models_with_quantities.each_pair do |model_id, quantity| %>
+      <% cart.items.each do |model_id, quantity| %>
         <li data-value="<%= quantity %>"><%= link_to EquipmentModel.find(model_id).name, EquipmentModel.find(model_id) %></li>
       <% end %>
     </ul>

app/views/reservations/_new_reservation.html.erb

@@ -1,5 +1,5 @@
 <% if params[:from_cart] == "true" %>
-    <% session[:cart].set_reserver_id(@user.id) %>
+    <% session[:cart].reserver_id = @user.id %>
     $('#userModal').modal('hide');
     $(window.location.reload());
 <% elsif params[:action] == "update" %>

app/views/users/create_success.js.erb

@@ -0,0 +1,9 @@
+class RemoveCartReservations < ActiveRecord::Migration
+  def up
+    drop_table :cart_reservations
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end