[#152604] Prevent abandoned reservation carts (#2292)

# Release Notes

Prevent abandoned carts created when clicking an instrument name. This should also help prevent order/detail IDs from growing too fast.

# Additional Context

Before, if you click an instrument, a new order and order detail are created. Users are often doing this to check the schedule for the instrument. If the user closes the tab comes back to the instrument, yet another order detail is created. We were deleting the cart if the user clicks "Cancel", but the order and order detail IDs were still eaten up.

Now, for normal single-reservation purchases, we go to a parallel controller where we do not create the order/detail prior to the user filling out the form. I did need to rework some of the code to avoid persisting and then rolling back the order/detail. Without these changes, the abandoned carts would not be created, but the IDs would still be used up.

For the order form (purchasing more than one or with other things) or part of a bundle, as well as edit/update, we are continuing to use the old controller.

Some offline discussion for context:

From Aaron:

> What we highly suspect is that users are clicking on the instrument links to check out the calendar, and then closing the tab. I assume that if a user clicks on an instrument name bringing them to a "Create Reservation" page, and then clicks "cancel" then this will not generate an abandoned cart? Clicking cancel from the one-off "Create Reservation" page generates the banner: "the product has been removed." I assume the Order Number has still been used up, but that this wouldn't create an "abandoned cart"?

Jason:

> You're right about the cancel button. I did a quick test: if you click "cancel" we do remove the order detail (so that number is used up), but we keep the order. That order gets re-used. So clicking an instrument creates, say, "100-100". Click cancel. Click the instrument again. You get "100-101". This won't count as an abandoned cart. However, if you click the browser's back button or close the tab, it leaves both of them hanging around. So "100-100", click on the instrument again, then you get "101-101". "100-100" is an abandoned cart.

app/controllers/instruments_controller.rb

@@ -23,12 +23,10 @@ def public_list
   # GET /facilities/:facility_id/instruments/:instrument_id
   def show
     instrument_for_cart = InstrumentForCart.new(@product)
+    # TODO: Remove this instance variable-not used anywhere but tests
     @add_to_cart = instrument_for_cart.purchasable_by?(acting_user, session_user)
     if @add_to_cart
-      redirect_to add_order_path(
-        acting_user.cart(session_user),
-        order: { order_details: [{ product_id: @product.id, quantity: 1 }] },
-      )
+      redirect_to new_facility_instrument_single_reservation_path(current_facility, @product)
     elsif instrument_for_cart.error_path
       redirect_to instrument_for_cart.error_path, notice: instrument_for_cart.error_message
     else

app/controllers/reservations_controller.rb

@@ -129,14 +129,11 @@ def create
   def new
     raise ActiveRecord::RecordNotFound unless @reservation.nil?
 
-    options = current_user.can_override_restrictions?(@instrument) ? {} : { user: acting_user }
-    next_available = @instrument.next_available_reservation(after: 1.minute.from_now, duration: default_reservation_mins.minutes, options: options)
-    @reservation = next_available || default_reservation
+    @reservation = NextAvailableReservationFinder.new(@instrument).next_available_for(current_user, acting_user)
     @reservation.order_detail = @order_detail
 
     authorize! :new, @reservation
 
-    @reservation.round_reservation_times
     unless @instrument.can_be_used_by?(@order_detail.user)
       flash[:notice] = text(".acting_as_not_on_approval_list")
     end
@@ -366,16 +363,7 @@ def save_reservation_and_order_detail
   end
 
   def set_windows
-    @max_window = max_reservation_window
-    @max_days_ago = session_user.operator_of?(@facility) ? -365 : 0
-    # initialize calendar time constraints
-    @min_date     = (Time.zone.now + @max_days_ago.days).strftime("%Y%m%d")
-    @max_date     = (Time.zone.now + @max_window.days).strftime("%Y%m%d")
-  end
-
-  def max_reservation_window
-    return 365 if session_user.operator_of?(@facility)
-    @reservation.longest_reservation_window(@order_detail.price_groups)
+    @reservation_window = ReservationWindow.new(@reservation, current_user)
   end
 
   def notice_for_reservation(reservation)
@@ -411,16 +399,6 @@ def helpers
     ActionController::Base.helpers
   end
 
-  def default_reservation_mins
-    @instrument.min_reserve_mins.to_i > 0 ? @instrument.min_reserve_mins : 30
-  end
-
-  def default_reservation
-    Reservation.new(product: @instrument,
-                    reserve_start_at: Time.zone.now,
-                    reserve_end_at: default_reservation_mins.minutes.from_now)
-  end
-
   # `1.week` causes a problem with daylight saving week since it's technically longer
   # than a week
   def month_view?

app/controllers/single_reservations_controller.rb

@@ -0,0 +1,62 @@
+class SingleReservationsController < ApplicationController
+
+  customer_tab  :all
+  before_action :authenticate_user!
+  load_resource :facility, find_by: :url_name
+  load_resource :instrument, through: :facility, find_by: :url_name
+  before_action :build_order
+  before_action { @submit_action = facility_instrument_single_reservations_path }
+
+  def new
+    @reservation = NextAvailableReservationFinder.new(@instrument).next_available_for(current_user, acting_user)
+    @reservation.order_detail = @order_detail
+
+    authorize! :new, @reservation
+
+    unless @instrument.can_be_used_by?(acting_user)
+      flash[:notice] = text(".acting_as_not_on_approval_list")
+    end
+    set_windows
+    render "reservations/new"
+  end
+
+  def create
+    creator = ReservationCreator.new(@order, @order_detail, params)
+    if creator.save(session_user)
+      @reservation = creator.reservation
+      authorize! :create, @reservation
+      flash[:notice] = I18n.t "controllers.reservations.create.success"
+      flash[:error] = I18n.t("controllers.reservations.create.admin_hold_warning") if creator.reservation.conflicting_admin_reservation?
+      redirect_to purchase_order_path(@order, params.permit(:send_notification))
+    else
+      @reservation = creator.reservation
+      flash.now[:error] = creator.error.html_safe
+      set_windows
+      render "reservations/new"
+    end
+  end
+
+  private
+
+  def ability_resource
+    @reservation
+  end
+
+  def build_order
+    @order = Order.new(
+      user: acting_user,
+      facility: current_facility,
+      created_by: session_user.id,
+    )
+    @order_detail = @order.order_details.build(
+      product: @instrument,
+      quantity: 1,
+      created_by: session_user.id,
+    )
+  end
+
+  def set_windows
+    @reservation_window = ReservationWindow.new(@reservation, current_user)
+  end
+
+end

app/models/order_detail.rb

@@ -48,7 +48,7 @@ def set_problem_order
   belongs_to :price_policy
   belongs_to :statement, inverse_of: :order_details
   belongs_to :journal
-  belongs_to :order, inverse_of: :order_details
+  belongs_to :order, inverse_of: :order_details, required: true
   belongs_to :assigned_user, class_name: "User", foreign_key: "assigned_user_id"
   belongs_to :created_by_user, class_name: "User", foreign_key: :created_by
   belongs_to :dispute_by, class_name: "User"
@@ -72,7 +72,7 @@ def set_problem_order
   # allow access to the vestal data.
   # once that data is no longer needed, this line and the
   # associated class can be removed
-  has_many   :vestal_versions, as: :versioned
+  has_many :vestal_versions, as: :versioned
 
   delegate :edit_url, to: :external_service_receiver, allow_nil: true
   delegate :in_cart?, :facility, :user, to: :order
@@ -100,7 +100,7 @@ def journal_or_statement_date
     journal_date || statement_date
   end
 
-  validates_presence_of :product_id, :order_id, :created_by
+  validates_presence_of :product_id, :created_by
   validates_numericality_of :quantity, only_integer: true, greater_than_or_equal_to: 1
   validates_numericality_of :actual_cost, greater_than_or_equal_to: 0, if: ->(o) { o.actual_cost_changed? && !o.actual_cost.nil? }
   validates_numericality_of :actual_subsidy, greater_than_or_equal_to: 0, if: ->(o) { o.actual_subsidy_changed? && !o.actual_cost.nil? }
@@ -434,6 +434,11 @@ def set_default_status!
     change_status! product.initial_order_status
   end
 
+  def valid_as_user?(user)
+    @being_purchased_by_admin = user.operator_of?(product.facility)
+    valid?
+  end
+
   def save_as_user(user)
     @being_purchased_by_admin = user.operator_of?(product.facility)
     save

app/models/reservation.rb

@@ -206,6 +206,16 @@ def assign_actuals_off_reserve
     self.actual_end_at   ||= reserve_end_at
   end
 
+  def valid_as_user?(user)
+    if user.operator_of?(product.facility)
+      self.reserved_by_admin = true
+      valid?
+    else
+      self.reserved_by_admin = false
+      valid?(context: :user_purchase)
+    end
+  end
+
   def save_as_user(user)
     if user.operator_of?(product.facility)
       self.reserved_by_admin = true

app/services/move_to_problem_queue.rb

@@ -20,8 +20,8 @@ def move!
 
     @order_detail.time_data.force_completion = @force
     @order_detail.complete!
-    # TODO: Can probably remove this at some point, but it's a safety check for now
     LogEvent.log(@order_detail, :problem_queue, @user, metadata: {cause: @cause})
+    # TODO: Can probably remove this at some point, but it's a safety check for now
     raise "Trying to move Order ##{@order_detail} to problem queue, but it's not a problem" unless @order_detail.problem?
 
     if OrderDetails::ProblemResolutionPolicy.new(@order_detail).user_can_resolve?

app/services/next_available_reservation_finder.rb

@@ -0,0 +1,30 @@
+# Used by the reservations controllers to find the default reservation times to display
+
+class NextAvailableReservationFinder
+  def initialize(product)
+    @product = product
+  end
+
+  def next_available_for(current_user, acting_user)
+    options = current_user.can_override_restrictions?(@product) ? {} : { user: acting_user }
+    next_available = @product.next_available_reservation(
+      after: 1.minute.from_now,
+      duration: default_reservation_mins.minutes,
+      options: options
+    )
+    next_available ||= default_reservation
+    next_available.round_reservation_times
+  end
+
+  private
+
+  def default_reservation
+    Reservation.new(instrument: @product,
+                    reserve_start_at: Time.current,
+                    reserve_end_at: default_reservation_mins.minutes.from_now)
+  end
+
+  def default_reservation_mins
+    @product.min_reserve_mins.to_i > 0 ? @product.min_reserve_mins : 30
+  end
+end

app/services/reservation_creator.rb

@@ -25,11 +25,13 @@ def save(session_user)
         # merge state can change after call to #save! due to OrderDetailObserver#before_save
         to_be_merged = @order_detail.order.to_be_merged?
 
-        save_reservation_and_order_detail(session_user)
+        raise ActiveRecord::RecordInvalid, @order_detail unless reservation_and_order_valid?(session_user)
 
         validator = OrderPurchaseValidator.new(@order_detail)
         raise ActiveRecord::RecordInvalid, @order_detail if validator.invalid?
 
+        save_reservation_and_order_detail(session_user)
+
         if to_be_merged
           # The purchase_order_path or cart_path will handle the backdating, but we need
           # to do this here for merged reservations.
@@ -51,7 +53,7 @@ def save(session_user)
   end
 
   def reservation
-    return @reservation if @reservation
+    return @reservation if defined?(@reservation)
     @reservation = @order_detail.build_reservation
     @reservation.assign_attributes(reservation_create_params)
     @reservation.assign_times_from_params(reservation_create_params)
@@ -68,12 +70,16 @@ def reservation_create_params
   end
 
   def update_order_account
-    if params[:order_account].present?
-      account = Account.find(params[:order_account].to_i)
-      if account != @order.account
-        @order.invalidate
-        @order.update_attributes!(account: account)
-      end
+    return if params[:order_account].blank?
+
+    account = Account.find(params[:order_account].to_i)
+    # If the account has changed, we need to re-do validations on the order. We're
+    # only saving the changes if the reservation is part of a cart. Otherwise, we
+    # just modify the object in-memory for redisplay.
+    if account != @order.account
+      @order.invalidate if @order.persisted?
+      @order.account = account
+      @order.save! if @order.persisted?
     end
   end
 
@@ -82,6 +88,10 @@ def backdate_reservation_if_necessary(session_user)
     @order_detail.backdate_to_complete!(@reservation.reserve_end_at) if facility_ability.can?(:order_in_past, @order) && @reservation.reserve_end_at < Time.zone.now
   end
 
+  def reservation_and_order_valid?(session_user)
+    reservation.valid_as_user?(session_user) && order_detail.valid_as_user?(session_user)
+  end
+
   def save_reservation_and_order_detail(session_user)
     reservation.save_as_user!(session_user)
     order_detail.assign_estimated_price(reservation.reserve_end_at)

app/services/reservation_window.rb

@@ -0,0 +1,29 @@
+class ReservationWindow
+  def initialize(reservation, user)
+    @reservation = reservation
+    @user = user
+  end
+
+  def max_window
+    return 365 if operator?
+    @reservation.longest_reservation_window(@reservation.user.price_groups)
+  end
+
+  def max_days_ago
+    operator? ? -365 : 0
+  end
+
+  def min_date
+    max_days_ago.days.from_now.strftime("%Y%m%d")
+  end
+
+  def max_date
+    max_window.days.from_now.strftime("%Y%m%d")
+  end
+
+  private
+
+  def operator?
+    @user.operator_of?(@reservation.facility)
+  end
+end

app/views/reservations/_js_variables.html.haml

@@ -1,9 +1,9 @@
 :javascript
   var events_path     = "#{facility_instrument_reservations_path(@instrument.facility, @instrument, format: 'js', with_details: @instrument.show_details?)}";
-  var minDate         = "#{@min_date}";
-  var maxDate         = "#{@max_date}";
-  var maxDaysFromNow  = #{@max_window};
-  var minDaysFromNow  = #{@max_days_ago};
+  var minDate         = "#{@reservation_window&.min_date}";
+  var maxDate         = "#{@reservation_window&.max_date}";
+  var maxDaysFromNow  = #{@reservation_window&.max_window};
+  var minDaysFromNow  = #{@reservation_window&.max_days_ago};
   var minTime         = #{@instrument.first_available_hour};
   var maxTime         = #{@instrument.last_available_hour+1};
   var isBundle        = #{@order_detail.bundled? || @order.order_details.count > 1};

app/views/reservations/new.html.haml

@@ -1,7 +1,7 @@
 = content_for :head_content do
   = render "shared/headers/calendar"
   = javascript_include_tag "reservations.js"
-  = render "js_variables"
+  = render "reservations/js_variables"
 
 = content_for :breadcrumb do
   %ul.breadcrumb
@@ -25,11 +25,10 @@
   %p.alert.alert-warning= text("instruments.schedule.instrument_alert_is_active", note: @instrument.alert.note)
 
 = render "products_common/description", product: @instrument
-
-= simple_form_for [@order, @order_detail, @reservation], html: { class: "js--reservationForm js--reservationUpdateCreateAndStart" } do |f|
+= simple_form_for [@order, @order_detail, @reservation], html: { class: "js--reservationForm js--reservationUpdateCreateAndStart" }, url: @submit_action do |f|
   = f.error_messages
-  = render "account_field", f: f unless @order_detail.bundled?
-  = render "reservation_fields", f: f
+  = render "reservations/account_field", f: f unless @order_detail.bundled?
+  = render "reservations/reservation_fields", f: f
 
   - if acting_as?
     .row
@@ -61,7 +60,7 @@
         = link_to t("shared.cancel"), facility_order_path(@instrument.facility, @order.merge_order)
       - elsif @order_detail.bundled?
         = link_to t("shared.cancel"), url_after_action
-      - else
+      - elsif @order.persisted?
         = link_to t("shared.cancel"),
           remove_order_path(@order, @order_detail, redirect_to: url_after_action),
           method: :put

config/routes.rb

@@ -83,6 +83,8 @@
       get :dashboard, to: "instruments_dashboard#dashboard", on: :collection
       get :public_dashboard, to: "instruments_dashboard#public_dashboard", on: :collection
 
+      resources :single_reservations, only: [:new, :create]
+
       collection do
         get "list", to: "instruments#public_list"
       end

spec/controllers/instruments_controller_spec.rb

@@ -108,12 +108,7 @@
 
         it "succeeds" do
           expect(flash).to be_empty
-          assert_redirected_to(
-            add_order_path(
-              Order.all.last,
-              order: { order_details: [product_id: instrument.id, quantity: 1] },
-            ),
-          )
+          assert_redirected_to new_facility_instrument_single_reservation_path(facility, instrument)
         end
       end
     end