Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#148409] Reduce number of DB queries on Facility timeline page #1899

Merged
merged 9 commits into from
Mar 14, 2019
10 changes: 9 additions & 1 deletion app/controllers/facility_reservations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class FacilityReservationsController < ApplicationController
include NewInprocessController
include ProblemOrderDetailsController
include TabCountHelper
include Timelineable
helper TimelineHelper

admin_tab :all
before_action :authenticate_user!
Expand Down Expand Up @@ -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
Expand Down
9 changes: 6 additions & 3 deletions app/controllers/reservations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -13,6 +11,7 @@ class ReservationsController < ApplicationController

include TranslationHelper
include FacilityReservationsHelper
helper TimelineHelper

def initialize
super
Expand All @@ -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
Expand Down
22 changes: 0 additions & 22 deletions app/controllers/timelineable.rb

This file was deleted.

2 changes: 1 addition & 1 deletion app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/models/concerns/products/scheduling_support.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 6 additions & 10 deletions app/models/instrument.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -82,10 +82,6 @@ def quantity_as_time?
true
end

def has_alert?
alert.present?
end

private

def minimum_reservation_is_multiple_of_interval
Expand Down
4 changes: 2 additions & 2 deletions app/models/product.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 2 additions & 15 deletions app/models/schedule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion app/views/facility_reservations/timeline.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,4 @@

.clear
.span12.timeline-wrapper
= render partial: "shared/timeline/schedule", collection: @schedules, as: :schedule
= render partial: "schedule", collection: @schedules, as: :schedule
4 changes: 2 additions & 2 deletions app/views/instruments/schedule.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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?
Expand Down
7 changes: 7 additions & 0 deletions app/views/reservations/_schedule.html.haml
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion app/views/reservations/new.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/views/reservations/public_timeline.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,4 @@

.row
.span12.timeline-wrapper
= render partial: "shared/timeline/schedule", collection: @schedules, as: :schedule
= render partial: "schedule", collection: @schedules, as: :schedule
4 changes: 3 additions & 1 deletion app/views/shared/timeline/_instrument.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -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
3 changes: 0 additions & 3 deletions app/views/shared/timeline/_relay_switch.html.haml

This file was deleted.

14 changes: 1 addition & 13 deletions spec/models/instrument_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down