Merge pull request #247 from lnu-norge/rethinking-filtering-by-facility

Faster search

app/controllers/concerns/accessible_active_personal_space_list.rb

@@ -13,7 +13,10 @@ def access_active_personal_list
     return @active_personal_space_list = nil if my_personal_space_list.blank?
 
     @active_personal_space_list = PersonalSpaceList.includes(:spaces,
-                                                             :this_lists_personal_data_on_spaces)
+                                                             {
+                                                               personal_space_lists_spaces:
+                                                                :personal_data_on_space_in_lists
+                                                             })
                                                    .find(my_personal_space_list.id)
   end
 end

app/controllers/spaces_controller.rb

@@ -15,7 +15,7 @@ def index
     # TODO: Make sure we can use .includes_data_for_filter_list here. That will mean we need
     # to rewrite the filter_and_order_by_facilities method. Probably that will happen if we
     # switch to a new search library.
-    @pagy, @spaces = pagy(@filtered_spaces)
+    @pagy, @spaces = pagy(@filtered_spaces.includes_data_for_filter_list)
     @markers = @spaces.map(&:render_map_marker)
   end
 

app/models/personal_data_on_space_in_list.rb

@@ -7,12 +7,22 @@ class PersonalDataOnSpaceInList < ApplicationRecord
 
   enum contact_status: { not_contacted: 0, said_no: 1, said_maybe: 2, said_yes: 3 }
 
+  after_create :update_personal_space_list_counters
+  after_update :update_personal_space_list_counters
+  after_destroy :update_personal_space_list_counters
+
   ICON_FOR_CONTACT_STATUS = {
     "not_contacted" => "unknown",
     "said_no" => "unlikely",
     "said_maybe" => "maybe",
     "said_yes" => "likely"
   }.freeze
+
+  private
+
+  def update_personal_space_list_counters
+    personal_space_list.update_counter_caches if personal_space_list.present?
+  end
 end
 
 # == Schema Information

app/models/personal_space_list.rb

@@ -27,6 +27,7 @@ def add_space(space)
 
   def remove_space(space)
     personal_space_lists_spaces.delete_by(space:)
+    update_counter_caches
   end
 
   def includes_space?(space)
@@ -46,28 +47,18 @@ def deactivate_for(user:)
     ActivePersonalSpaceList.where(user:).destroy_all
   end
 
-  def space_count
-    this_lists_personal_data_on_spaces.size
-  end
-
   def space_contacted_count
-    this_lists_personal_data_on_spaces.not_not_contacted.size
-  end
-
-  def space_not_contacted_count
-    this_lists_personal_data_on_spaces.not_contacted.size
-  end
-
-  def space_said_no_count
-    this_lists_personal_data_on_spaces.said_no.size
-  end
-
-  def space_said_maybe_count
-    this_lists_personal_data_on_spaces.said_maybe.size
+    space_count - space_not_contacted_count
   end
 
-  def space_said_yes_count
-    this_lists_personal_data_on_spaces.said_yes.size
+  def update_counter_caches
+    update!(
+      space_count: this_lists_personal_data_on_spaces.count,
+      space_not_contacted_count: this_lists_personal_data_on_spaces.not_contacted.count,
+      space_said_no_count: this_lists_personal_data_on_spaces.said_no.count,
+      space_said_maybe_count: this_lists_personal_data_on_spaces.said_maybe.count,
+      space_said_yes_count: this_lists_personal_data_on_spaces.said_yes.count
+    )
   end
 
   # self.METHOD_NAME makes this a Class method, the others are instance methods
@@ -121,12 +112,17 @@ def remove_from_shared_with_user(user:)
 #
 # Table name: personal_space_lists
 #
-#  id                 :bigint           not null, primary key
-#  shared_with_public :boolean          default(FALSE)
-#  title              :string
-#  created_at         :datetime         not null
-#  updated_at         :datetime         not null
-#  user_id            :bigint
+#  id                        :bigint           not null, primary key
+#  shared_with_public        :boolean          default(FALSE)
+#  space_count               :integer          default(0)
+#  space_not_contacted_count :integer          default(0)
+#  space_said_maybe_count    :integer          default(0)
+#  space_said_no_count       :integer          default(0)
+#  space_said_yes_count      :integer          default(0)
+#  title                     :string
+#  created_at                :datetime         not null
+#  updated_at                :datetime         not null
+#  user_id                   :bigint
 #
 # Indexes
 #

app/models/personal_space_lists_space.rb

@@ -10,10 +10,19 @@ class PersonalSpaceListsSpace < ApplicationRecord
            dependent: nil # Keep it around in case the space is added again
 
   after_create :set_up_personal_data_on_space_in_list
+  after_create :update_personal_space_list_counters
+  after_update :update_personal_space_list_counters
+  after_destroy :update_personal_space_list_counters
 
   def set_up_personal_data_on_space_in_list
     PersonalDataOnSpaceInList.find_or_create_by(personal_space_list:, space:)
   end
+
+  private
+
+  def update_personal_space_list_counters
+    personal_space_list.update_counter_caches
+  end
 end
 
 # == Schema Information

app/models/space.rb

@@ -128,27 +128,16 @@ class Space < ApplicationRecord # rubocop:disable Metrics/ClassLength
   }
 
   scope :filter_and_order_by_facilities, lambda { |facility_ids|
-    # NB: This grouping means that the results are not countable with .size or .count
-    # And cannot be used with includes...
-    # We really need to fix this in the current rewrite of search
-    group(:id)
-      .joins(:space_facilities)
-      .where(space_facilities: { relevant: true, facility_id: facility_ids })
-      .order(Arel.sql("
-                    SUM(
-                                     CASE
-                                        WHEN space_facilities.experience = 4 THEN 3.0 -- likely
-                                        WHEN space_facilities.experience = 3 THEN 2.0 -- maybe
-                                        WHEN space_facilities.experience = 2 THEN -1.0 -- unlikely
-                                        WHEN space_facilities.experience = 1 THEN -2.0 -- impossible
-                                        WHEN space_facilities.relevant THEN 1.0 -- unknown experience, but relevant
-                                        ELSE 0.0 -- unknown experience, irrelevant facility
-                                      END
-                    )
-                DESC"),
-             Arel.sql(
-               "COALESCE((spaces.star_rating - 2.9) / 10, 0) DESC"
-             ))
+    joins(:space_facilities)
+      .where(space_facilities: { facility_id: facility_ids, relevant: true })
+      .group("spaces.id")
+      # First sort by facility score, then by star rating to break any ties
+      # Star ratings below 3 should be sorted below those who have no rating
+      # Coalesce makes sure that 0 is returned if there is no rating
+      .order(
+        Arel.sql("SUM(space_facilities.score) DESC, COALESCE((spaces.star_rating - 2.9), 0) DESC")
+      )
+      .select("spaces.*")
   }
 
   has_rich_text :how_to_book

app/models/space_facility.rb

@@ -1,10 +1,17 @@
 # frozen_string_literal: true
 
 class SpaceFacility < ApplicationRecord
-  enum experience: { unknown: 0, impossible: 1, unlikely: 2, maybe: 3, likely: 4 }
-
   belongs_to :facility
   belongs_to :space
+
+  enum experience: { unknown: 0, impossible: 1, unlikely: 2, maybe: 3, likely: 4 }
+
+  after_create :calculate_score
+  after_update :calculate_score, if: lambda {
+    saved_change_to_attribute?(:experience) ||
+      saved_change_to_attribute?(:relevant)
+  }
+
   validates :space, uniqueness: { scope: [:facility] }
 
   scope :impossible, -> { where(experience: :impossible) }
@@ -34,6 +41,29 @@ def user_review(user)
       space:
     )
   end
+
+  def calculate_score
+    update!(score: calculate_score_from_experience)
+  end
+
+  private
+
+  def calculate_score_from_experience
+    case experience
+    when "impossible"
+      -2
+    when "unlikely"
+      -1
+    when "maybe"
+      2
+    when "likely"
+      3
+    else
+      return 1 if relevant?
+
+      0
+    end
+  end
 end
 
 # == Schema Information
@@ -44,6 +74,7 @@ def user_review(user)
 #  description :string
 #  experience  :integer
 #  relevant    :boolean          default(FALSE)
+#  score       :integer          default(0)
 #  created_at  :datetime         not null
 #  updated_at  :datetime         not null
 #  facility_id :bigint           not null

db/migrate/20240925180933_add_score_to_space_facility.rb

@@ -0,0 +1,18 @@
+class AddScoreToSpaceFacility < ActiveRecord::Migration[7.1]
+  def up
+    add_column :space_facilities, :score, :integer, default: 0
+
+    # In a transaction, calculate score for all space facilities
+    SpaceFacility.transaction do
+      count = SpaceFacility.count.to_i
+      SpaceFacility.all.each_with_index do |space_facility, index|
+        print  "\rCalculating score for #{index + 1} / #{count}        "
+        space_facility.calculate_score
+      end
+    end
+  end
+
+  def down
+    remove_column :space_facilities, :score
+  end
+end

db/migrate/20240926071852_add_counter_caches_to_personal_space_list.rb

@@ -0,0 +1,64 @@
+class AddCounterCachesToPersonalSpaceList < ActiveRecord::Migration[7.1]
+  def up
+    add_column :personal_space_lists, :space_count, :integer, default: 0
+    add_column :personal_space_lists, :space_not_contacted_count, :integer, default: 0
+    add_column :personal_space_lists, :space_said_no_count, :integer, default: 0
+    add_column :personal_space_lists, :space_said_maybe_count, :integer, default: 0
+    add_column :personal_space_lists, :space_said_yes_count, :integer, default: 0
+
+    # Update the counter caches
+    execute <<-SQL
+      UPDATE personal_space_lists
+      SET
+        space_count = (
+          SELECT COUNT(*)
+          FROM personal_space_lists_spaces
+          WHERE personal_space_lists_spaces.personal_space_list_id = personal_space_lists.id
+        ),
+        space_not_contacted_count = (
+          SELECT COUNT(*)
+          FROM personal_space_lists_spaces
+          JOIN personal_data_on_space_in_lists ON
+            personal_data_on_space_in_lists.space_id = personal_space_lists_spaces.space_id AND
+            personal_data_on_space_in_lists.personal_space_list_id = personal_space_lists_spaces.personal_space_list_id
+          WHERE personal_space_lists_spaces.personal_space_list_id = personal_space_lists.id
+            AND personal_data_on_space_in_lists.contact_status = 0
+        ),
+        space_said_no_count = (
+          SELECT COUNT(*)
+          FROM personal_space_lists_spaces
+          JOIN personal_data_on_space_in_lists ON
+            personal_data_on_space_in_lists.space_id = personal_space_lists_spaces.space_id AND
+            personal_data_on_space_in_lists.personal_space_list_id = personal_space_lists_spaces.personal_space_list_id
+          WHERE personal_space_lists_spaces.personal_space_list_id = personal_space_lists.id
+            AND personal_data_on_space_in_lists.contact_status = 1
+        ),
+        space_said_maybe_count = (
+          SELECT COUNT(*)
+          FROM personal_space_lists_spaces
+          JOIN personal_data_on_space_in_lists ON
+            personal_data_on_space_in_lists.space_id = personal_space_lists_spaces.space_id AND
+            personal_data_on_space_in_lists.personal_space_list_id = personal_space_lists_spaces.personal_space_list_id
+          WHERE personal_space_lists_spaces.personal_space_list_id = personal_space_lists.id
+            AND personal_data_on_space_in_lists.contact_status = 2
+        ),
+        space_said_yes_count = (
+          SELECT COUNT(*)
+          FROM personal_space_lists_spaces
+          JOIN personal_data_on_space_in_lists ON
+            personal_data_on_space_in_lists.space_id = personal_space_lists_spaces.space_id AND
+            personal_data_on_space_in_lists.personal_space_list_id = personal_space_lists_spaces.personal_space_list_id
+          WHERE personal_space_lists_spaces.personal_space_list_id = personal_space_lists.id
+            AND personal_data_on_space_in_lists.contact_status = 3
+        )
+    SQL
+  end
+
+  def down
+    remove_column :personal_space_lists, :space_count
+    remove_column :personal_space_lists, :space_not_contacted_count
+    remove_column :personal_space_lists, :space_said_no_count
+    remove_column :personal_space_lists, :space_said_maybe_count
+    remove_column :personal_space_lists, :space_said_yes_count
+  end
+end

spec/fabricators/space_facility_fabricator.rb

@@ -14,6 +14,7 @@
 #  description :string
 #  experience  :integer
 #  relevant    :boolean          default(FALSE)
+#  score       :integer          default(0)
 #  created_at  :datetime         not null
 #  updated_at  :datetime         not null
 #  facility_id :bigint           not null

spec/models/personal_space_list_spec.rb

@@ -93,6 +93,7 @@
       contact_status: "said_no"
     )
 
+    users_space_list.reload
     expect(users_space_list.space_not_contacted_count).to eq(1)
     expect(users_space_list.space_contacted_count).to eq(1)
     expect(users_space_list.space_said_no_count).to eq(1)
@@ -103,6 +104,7 @@
       contact_status: "said_maybe"
     )
 
+    users_space_list.reload
     expect(users_space_list.space_not_contacted_count).to eq(1)
     expect(users_space_list.space_contacted_count).to eq(1)
     expect(users_space_list.space_said_no_count).to eq(0)
@@ -113,6 +115,7 @@
       contact_status: "said_yes"
     )
 
+    users_space_list.reload
     expect(users_space_list.space_not_contacted_count).to eq(1)
     expect(users_space_list.space_contacted_count).to eq(1)
     expect(users_space_list.space_said_no_count).to eq(0)
@@ -135,6 +138,7 @@
       contact_status: "said_no"
     )
 
+    users_space_list.reload
     expect(users_space_list.space_count).to eq(2)
     expect(users_space_list.space_not_contacted_count).to eq(1)
     expect(users_space_list.space_contacted_count).to eq(1)
@@ -143,6 +147,7 @@
     # Removing the space changes the counts
     users_space_list.remove_space(space_to_add_to_list)
 
+    users_space_list.reload
     expect(users_space_list.space_count).to eq(1)
     expect(users_space_list.space_not_contacted_count).to eq(1)
     expect(users_space_list.space_contacted_count).to eq(0)
@@ -151,6 +156,7 @@
     # Re-adding the space remembers the state and count we had
     users_space_list.add_space(space_to_add_to_list)
 
+    users_space_list.reload
     expect(users_space_list.space_count).to eq(2)
     expect(users_space_list.space_not_contacted_count).to eq(1)
     expect(users_space_list.space_contacted_count).to eq(1)