[#148409] Reduce number of DB queries on Facility timeline page (wyew…

…orks#1899) * Refactor away Timelinable module The indirection and hack (in ReservationsController, public_timeline calls timeline) here are not worth the DRY aspect, and the two pages are very different, so decoupling their implementation allows us to evolve and optimize the controller methods separately as it makes sense. * Separate facility and public schedule partials and get rid of conditional * Refactor association-like methods on Schedule to associations * Use .size on association in _schedule so we don’t have to execute a count every time * Eliminate N+1 for loading alerts when displaying timelines * Fold single-use partial _relay_switch into only call site * Avoid N+1 for relays on facility timeline * Avoid two unnecessary queries just to check if an instrument is offline The method #offline? needs to check whether there are any current offline reservations. By refactoring to an association and using #present?, Rails will memoize the association and check without another DB query. * Appease rubocop’s style preferences
SquaredLabs · Oct 21, 2020 · f9a18c9 · f9a18c9
1 parent 01a9acc
commit f9a18c9
Show file tree

Hide file tree

Showing 17 changed files with 46 additions and 81 deletions.
diff --git a/app/controllers/facility_reservations_controller.rb b/app/controllers/facility_reservations_controller.rb
@@ -6,7 +6,7 @@ class FacilityReservationsController < ApplicationController
   include NewInprocessController
   include ProblemOrderDetailsController
   include TabCountHelper
-  include Timelineable
+  helper TimelineHelper
 
   admin_tab     :all
   before_action :authenticate_user!
@@ -156,6 +156,14 @@ def destroy
     redirect_to facility_instrument_schedule_url
   end
 
+  def timeline
+    @display_datetime = parse_usa_date(params[:date]) || Time.current.beginning_of_day
+    @schedules = current_facility.schedules
+                                 .active
+                                 .includes(facility_visible_products: [:alert, :relay])
+                                 .order(:name)
+  end
+
   protected
 
   def reservation_params

diff --git a/app/controllers/reservations_controller.rb b/app/controllers/reservations_controller.rb
@@ -2,8 +2,6 @@
 
 class ReservationsController < ApplicationController
 
-  include Timelineable
-
   customer_tab  :all
   before_action :authenticate_user!, except: [:index]
   before_action :check_acting_as, only: [:switch_instrument, :list]
@@ -13,6 +11,7 @@ class ReservationsController < ApplicationController
 
   include TranslationHelper
   include FacilityReservationsHelper
+  helper TimelineHelper
 
   def initialize
     super
@@ -21,7 +20,11 @@ def initialize
 
   def public_timeline
     @public_timeline = true
-    timeline
+    @display_datetime = parse_usa_date(params[:date]) || Time.current.beginning_of_day
+    @schedules = current_facility.schedules
+                                 .active
+                                 .includes(publicly_visible_products: :alert)
+                                 .order(:name)
   end
 
   # GET /facilities/1/instruments/1/reservations.js?_=1279579838269&start=1279429200&end=1280034000

diff --git a/app/controllers/timelineable.rb b/app/controllers/timelineable.rb
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
@@ -42,7 +42,7 @@ def facility_product_path(facility, product)
   def warning_if_instrument_is_offline_or_partially_available(instrument)
     if instrument.offline?
       tooltip_icon "fa fa-exclamation-triangle icon-large", t("instruments.offline.note")
-    elsif instrument.has_alert?
+    elsif instrument.alert
       tooltip_icon "fa fa-exclamation-triangle partially-available-warning icon-large", instrument.alert.note
     end
   end

diff --git a/app/models/concerns/products/scheduling_support.rb b/app/models/concerns/products/scheduling_support.rb
@@ -59,7 +59,7 @@ def next_available_reservation(after: Time.zone.now, duration: 1.minute, options
   end
 
   def offline?
-    offline_reservations.current.any?
+    current_offline_reservations.present?
   end
 
   def offline_category

diff --git a/app/models/instrument.rb b/app/models/instrument.rb
@@ -9,12 +9,12 @@ class Instrument < Product
 
   RESERVE_INTERVALS = [1, 5, 10, 15, 30, 60].freeze
 
-  # Associations
-  # -------
-
-  has_many :instrument_price_policies, foreign_key: "product_id"
-  has_many :admin_reservations, foreign_key: "product_id"
-  has_many :offline_reservations, foreign_key: "product_id"
+  with_options foreign_key: "product_id" do |instrument|
+    instrument.has_many :admin_reservations
+    instrument.has_many :instrument_price_policies
+    instrument.has_many :offline_reservations
+    instrument.has_many :current_offline_reservations, -> { current }, class_name: "OfflineReservation"
+  end
   has_one :alert, dependent: :destroy, class_name: "InstrumentAlert"
 
   email_list_attribute :cancellation_email_recipients
@@ -82,10 +82,6 @@ def quantity_as_time?
     true
   end
 
-  def has_alert?
-    alert.present?
-  end
-
   private
 
   def minimum_reservation_is_multiple_of_interval

diff --git a/app/models/product.rb b/app/models/product.rb
@@ -292,8 +292,8 @@ def is_accessible_to_user?(user)
     !(is_archived? || (is_hidden? && !is_operator))
   end
 
-  def has_alert?
-    false
+  def alert
+    nil
   end
 
   protected

diff --git a/app/models/schedule.rb b/app/models/schedule.rb
@@ -2,31 +2,18 @@
 
 class Schedule < ApplicationRecord
 
-  # Associations
-  # --------
   belongs_to :facility
 
   has_many :products, class_name: "Instrument"
   has_many :reservations, through: :products
   has_many :admin_reservations, through: :products
+  has_many :publicly_visible_products, -> { active }, class_name: "Instrument"
+  has_many :facility_visible_products, -> { not_archived }, class_name: "Instrument"
 
-  # Validations
-  # --------
   validates_presence_of :facility
 
   scope :active, -> { where(id: Product.not_archived.with_schedule.select(:schedule_id)).order(:name) }
 
-  # Instance methods
-  # --------
-
-  def publicly_visible_products
-    products.active
-  end
-
-  def facility_visible_products
-    products.not_archived
-  end
-
   def shared?
     products.count > 1
   end

diff --git a/...views/shared/timeline/_schedule.html.haml → ...facility_reservations/_schedule.html.haml b/...views/shared/timeline/_schedule.html.haml → ...facility_reservations/_schedule.html.haml
@@ -1,8 +1,7 @@
-- products = public_timeline? ? schedule.publicly_visible_products : schedule.facility_visible_products
-- if products.count > 1
+- if schedule.facility_visible_products.size > 1
   .timeline-schedule
     %h3= schedule.name
-    = render partial: "shared/timeline/instrument", collection: products, as: :instrument
+    = render partial: "shared/timeline/instrument", collection: schedule.facility_visible_products, as: :instrument
 - else
   .timeline-single
-    = render partial: "shared/timeline/instrument", collection: products, as: :instrument
+    = render partial: "shared/timeline/instrument", collection: schedule.facility_visible_products, as: :instrument
diff --git a/app/views/facility_reservations/timeline.html.haml b/app/views/facility_reservations/timeline.html.haml
@@ -29,4 +29,4 @@
 
 .clear
 .span12.timeline-wrapper
-  = render partial: "shared/timeline/schedule", collection: @schedules, as: :schedule
+  = render partial: "schedule", collection: @schedules, as: :schedule
diff --git a/app/views/instruments/schedule.html.haml b/app/views/instruments/schedule.html.haml
@@ -29,7 +29,7 @@
       = link_to t(".bring_instrument_online"),
         facility_instrument_bring_online_path(current_facility, @product),
         method: :put, class: "btn-add"
-    - elsif @product.has_alert?
+    - elsif @product.alert
       = link_to t(".deactivate_instrument_alert"),
         facility_instrument_alert_path(current_facility, @product),
         method: :delete, class: "btn-add"
@@ -43,7 +43,7 @@
 
   = render_view_hook "after_offline_toggle"
 
-- if @product.has_alert?
+- if @product.alert
   %p.alert.alert-warning= t(".instrument_alert_is_active", note: @product.alert.note)
 
 - if @admin_reservations.empty?

diff --git a/app/views/reservations/_schedule.html.haml b/app/views/reservations/_schedule.html.haml
@@ -0,0 +1,7 @@
+- if schedule.publicly_visible_products.size > 1
+  .timeline-schedule
+    %h3= schedule.name
+    = render partial: "shared/timeline/instrument", collection: schedule.publicly_visible_products, as: :instrument
+- else
+  .timeline-single
+    = render partial: "shared/timeline/instrument", collection: schedule.publicly_visible_products, as: :instrument
diff --git a/app/views/reservations/new.html.haml b/app/views/reservations/new.html.haml
@@ -21,7 +21,7 @@
 
 - if @instrument.offline?
   %p.alert.alert-danger= text("instruments.offline.notice")
-- elsif @instrument.has_alert?
+- elsif @instrument.alert
   %p.alert.alert-warning= text("instruments.schedule.instrument_alert_is_active", note: @instrument.alert.note)
 
 = render "products_common/description", product: @instrument

diff --git a/app/views/reservations/public_timeline.html.haml b/app/views/reservations/public_timeline.html.haml
@@ -18,4 +18,4 @@
 
 .row
   .span12.timeline-wrapper
-    = render partial: "shared/timeline/schedule", collection: @schedules, as: :schedule
+    = render partial: "schedule", collection: @schedules, as: :schedule
diff --git a/app/views/shared/timeline/_instrument.html.haml b/app/views/shared/timeline/_instrument.html.haml
@@ -20,4 +20,6 @@
         .current_time{style: "left: #{datetime_left_position(@display_datetime, Time.current)}"}
 
     - if !public_timeline? && instrument.has_real_relay?
-      = render "shared/timeline/relay_switch", instrument: instrument
+      .relay_checkbox
+        = check_box_tag "relay[#{instrument.id}]", 1, false, :disabled => true, :"data-relay-url" => facility_instrument_switch_path(instrument.facility, instrument)
+        .loading_box
diff --git a/app/views/shared/timeline/_relay_switch.html.haml b/app/views/shared/timeline/_relay_switch.html.haml
diff --git a/spec/models/instrument_spec.rb b/spec/models/instrument_spec.rb
@@ -796,7 +796,7 @@
 
       it "switches the instrument to be online" do
         expect { instrument.online! }
-          .to change { instrument.online? }
+          .to change { instrument.reload.online? }
           .from(false).to(true)
           .and change { offline_reservation.reload.reserve_end_at }.from(nil)
       end
@@ -850,18 +850,6 @@
     end
   end
 
-  describe "#has_alert?" do
-    it "returns true when the instrument has an associated alert" do
-      subject.build_alert
-      expect(subject.has_alert?).to be true
-    end
-
-    it "returns false when the instrument does not have an associated alert" do
-      subject.alert = nil
-      expect(subject.has_alert?).to be false
-    end
-  end
-
   describe "issue_report_recipients" do
     before do
       instrument.issue_report_recipients = "test1@example.com, test2@example.com"