Merge pull request #236 from lnu-norge/fix-aggregation-destroying-unr…

…elated-stuff

Fix aggregation destroying unrelated stuff + terrible performance

Gemfile

@@ -68,6 +68,7 @@ end
 
 group :test do
   gem "capybara"
+  gem "rspec-benchmark"
   gem "rspec-rails"
   gem "selenium-webdriver"
   gem "vcr"

Gemfile.lock

@@ -101,6 +101,9 @@ GEM
       aws-eventstream (~> 1, >= 1.0.2)
     base64 (0.2.0)
     bcrypt (3.1.20)
+    benchmark-malloc (0.2.0)
+    benchmark-perf (0.6.0)
+    benchmark-trend (0.4.0)
     bigdecimal (3.1.6)
     bindex (0.8.1)
     bootsnap (1.17.1)
@@ -407,6 +410,11 @@ GEM
       rspec-core (~> 3.12.0)
       rspec-expectations (~> 3.12.0)
       rspec-mocks (~> 3.12.0)
+    rspec-benchmark (0.6.0)
+      benchmark-malloc (~> 0.2)
+      benchmark-perf (~> 0.6)
+      benchmark-trend (~> 0.4)
+      rspec (>= 3.0)
     rspec-core (3.12.2)
       rspec-support (~> 3.12.0)
     rspec-expectations (3.12.3)
@@ -575,6 +583,7 @@ DEPENDENCIES
   rails-i18n
   rails_real_favicon
   redis (~> 5.0)
+  rspec-benchmark
   rspec-rails
   rubocop
   rubocop-performance

app/models/space.rb

@@ -23,6 +23,15 @@ class Space < ApplicationRecord # rubocop:disable Metrics/ClassLength
           south_east_lat:,
           south_east_lng:)
   }
+  scope :with_aggregation_data, lambda {
+    preload(
+      :space_facilities,
+      :facility_reviews,
+      space_types: [
+        :facilities
+      ]
+    )
+  }
 
   has_many :space_types_relations, dependent: :destroy
   has_many :space_types, through: :space_types_relations, dependent: :destroy
@@ -80,7 +89,9 @@ def self.rect_of_spaces
   end
 
   def aggregate_facility_reviews(facilities: [])
-    Spaces::AggregateFacilityReviewsService.call(space: self, facilities:)
+    space = Space.with_aggregation_data.find(id)
+
+    Spaces::AggregateFacilityReviewsService.call(space:, facilities:)
   end
 
   def aggregate_star_rating

app/services/spaces/aggregate_facility_reviews_service.rb

@@ -1,10 +1,17 @@
 # frozen_string_literal: true
 
+require_relative "concerns/countable_reviews"
+
 module Spaces
   class AggregateFacilityReviewsService < ApplicationService
+    include CountableReviews
+
     def initialize(space:, facilities: [])
-      @space = Space.includes(:space_facilities, space_types: :facilities, facility_reviews: :facility).find(space.id)
+      @space = space
       @facilities = facilities.any? ? facilities : Facility.order(:created_at)
+      @space_facilities = @space.space_facilities
+      @space_types = @space.space_types
+      group_recent_facility_reviews_by_facility(count: 5)
 
       super()
     end
@@ -17,63 +24,64 @@ def call
 
     def aggregate_facilities
       SpaceFacility.transaction do
-        @space.space_facilities.where(facility_id: [@facilities.map(&:id)]).destroy_all
-
         @facilities.each do |facility|
           aggregate_reviews(facility)
         end
       end
     end
 
-    def aggregate_reviews(facility) # rubocop:disable Metrics/AbcSize
-      space_facility = SpaceFacility.create(space: @space, facility:)
-
-      reviews = @space.facility_reviews.where(facility:).order(created_at: :desc).limit(5)
-      count = reviews.count
-
+    def aggregate_reviews(facility)
+      space_facility = find_or_create_space_facility(facility)
+      reviews = most_recent_facility_reviews_for(facility)
       belongs_to_space_type = facility_belongs_to_space_type(facility)
 
-      return handle_zero_facility_reviews(space_facility, belongs_to_space_type) if count.zero?
+      return handle_zero_facility_reviews(space_facility, belongs_to_space_type) if reviews.blank?
 
       # Set criteria:
-      two_out_of_three = (count / 3.0 * 2.0).ceil
-      positive_threshold = reviews.positive.count >= two_out_of_three
-      impossible_threshold = reviews.impossible.count >= two_out_of_three
-      negative_threshold = reviews.negative.count >= two_out_of_three
+      two_out_of_three = (reviews.count / 3.0 * 2.0).ceil
+      positive_threshold = count_of_positive(reviews) >= two_out_of_three
+      impossible_threshold = count_of_impossible(reviews) >= two_out_of_three
+      negative_threshold = count_of_negative(reviews) >= two_out_of_three
 
       set_relevance(space_facility, belongs_to_space_type, positive_threshold)
-
       set_experience(space_facility, positive_threshold, impossible_threshold, negative_threshold)
     end
 
     # NB: Does not set relevance for the maybe scenario, that is set in set_experience
     def set_relevance(space_facility, belongs_to_space_type, positive_threshold)
-      return space_facility.not_relevant! unless positive_threshold || belongs_to_space_type
+      return space_facility.update(relevant: false) unless positive_threshold || belongs_to_space_type
 
-      space_facility.relevant!
+      space_facility.update(relevant: true)
     end
 
     def set_experience(space_facility, positive_threshold, impossible_threshold, negative_threshold)
-      return space_facility.likely! if positive_threshold
-      return space_facility.impossible! if impossible_threshold
-      return space_facility.unlikely! if negative_threshold
+      return space_facility.update(experience: "likely") if positive_threshold
+      return space_facility.update(experience: "impossible") if impossible_threshold
+      return space_facility.update(experience: "unlikely") if negative_threshold
 
       # Nothing else fits, so it's a _relevant_ maybe!
-      space_facility.maybe! && space_facility.relevant!
+      space_facility.update(experience: "maybe", relevant: true)
     end
 
     def handle_zero_facility_reviews(space_facility, belongs_to_space_type)
-      return space_facility.unknown! && space_facility.not_relevant! unless belongs_to_space_type
+      return space_facility.update(experience: "unknown", relevant: true) if belongs_to_space_type
 
-      space_facility.unknown! && space_facility.relevant!
+      space_facility.update(experience: "unknown", relevant: false)
     end
 
     def facility_belongs_to_space_type(facility)
-      @space.space_types.find do |space_type|
+      @space_types.find do |space_type|
         space_type.facilities.include? facility
       end.present?
     end
 
+    def find_or_create_space_facility(facility)
+      space_facility = @space_facilities.find { |sf| sf.facility_id == facility.id }
+      return space_facility if space_facility
+
+      SpaceFacility.create(facility_id: facility.id, space_id: @space.id)
+    end
+
     attr_reader :space
   end
 end

app/services/spaces/concerns/countable_reviews.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+module CountableReviews
+  private
+
+  def most_recent_facility_reviews_for(facility)
+    @grouped_facility_reviews[facility.id] || []
+  end
+
+  def group_recent_facility_reviews_by_facility(count: 5)
+    @grouped_facility_reviews = @space.facility_reviews
+                                      .order(created_at: :desc)
+                                      .group_by(&:facility_id)
+                                      .transform_values { |reviews| reviews.first(count) }
+  end
+
+  def facility_reviews_for(facility)
+    @last_five_facility_reviews_grouped_by_facility.find { |r| r.facility_id == facility.id }
+  end
+
+  def count_of_positive(reviews)
+    # These keep performance high for this service, by keeping db calls to
+    # a minimum. We could use the review.positive scope, but that would
+    # require a db call for each review, which would be slow. We're only
+    # counting in the already fetched list of reviews.
+    count_of_reviews_with_experience(reviews, "was_allowed")
+  end
+
+  def count_of_negative(reviews)
+    count_of_reviews_with_experience(reviews, "was_not_allowed")
+  end
+
+  def count_of_impossible(reviews)
+    count_of_reviews_with_experience(reviews, "was_not_available")
+  end
+
+  def count_of_reviews_with_experience(reviews, experience)
+    reviews.count { |r| r.experience == experience }
+  end
+end

spec/features/user_creates_facility_reviews_spec.rb

@@ -40,12 +40,13 @@
     )
   end
 
-  it "two facilities, at the same time" do
+  it "two facilities, after each other" do
     visit space_path(space)
 
     expect_to_add_multiple_facility_reviews(
       space:,
-      facilities_to_review: [facility_to_review, second_facility_to_review]
+      facilities_to_review: [facility_to_review, second_facility_to_review],
+      description_to_add: Faker::Lorem.sentence
     )
   end
 
@@ -64,6 +65,36 @@
     )
   end
 
+  it "two facilities, adding a description to the second one after the fact" do
+    visit space_path(space)
+
+    description_of_first_facility = Faker::Lorem.sentence
+    description_of_second_facility = Faker::Lorem.sentence
+
+    expect_to_add_facility_review(
+      space:,
+      facility: facility_to_review,
+      description_to_add: description_of_first_facility
+    )
+
+    expect_to_add_facility_review(
+      space:,
+      facility: second_facility_to_review,
+      count_that_already_have_the_new_experience: 1
+    )
+
+    expect_to_only_change_facility_review_description(
+      space:,
+      facility: second_facility_to_review,
+      count_that_already_have_the_new_experience: 2,
+      description_to_add: description_of_second_facility
+    )
+
+    # Expect both descriptions to be present:
+    expect(page).to have_content(description_of_first_facility)
+    expect(page).to have_content(description_of_second_facility)
+  end
+
   it "a single facility twice, adding a description the second time" do
     visit space_path(space)
 

spec/services/spaces/aggregate_facility_reviews_service_spec.rb

@@ -157,4 +157,53 @@ def experience(experience, other_facility = nil)
 
     expect(space.reload.relevance_of_facility(facility)).to be(false)
   end
+
+  it "allows setting descriptions for facilities, and keeping them after specific aggregation" do
+    description = "This is a description"
+    space.aggregate_facility_reviews(facilities: [facility])
+
+    space_facility = SpaceFacility.find_by(space:, facility:)
+    space_facility.update!(description:)
+
+    space.aggregate_facility_reviews(facilities: [facility])
+
+    expect(space_facility.reload.description).to eq(description)
+  end
+
+  it "does not touch the descriptions of space facilities, even after a broad aggregation" do
+    before_space_facility = SpaceFacility.find_by(space:, facility:)
+    before_space_facility.update!(description: "old description")
+
+    expect(before_space_facility.reload.description).to eq("old description")
+
+    space.aggregate_facility_reviews
+
+    after_space_facility = SpaceFacility.find_by(space:, facility:)
+    expect(after_space_facility.reload.description).to eq("old description")
+  end
+
+  it "does not update a space facility that does not need to change" do
+    experience :was_allowed
+    space_facility = SpaceFacility.find_by(space:, facility:)
+    expect(space_facility.experience).to eq("likely")
+
+    experience :was_allowed
+
+    # No new ID
+    after_space_facility = SpaceFacility.find_by(space:, facility:)
+    expect(after_space_facility.id).to eq(space_facility.id)
+
+    # And still the same experience:
+    expect(space_facility.experience).to eq("likely")
+  end
+
+  it "is performant for a changing a single facility, even if there are a large amount of other facilities" do
+    Array.new(10) { Fabricate(:facility, space_types: [space_type]) }
+    expect { space.aggregate_facility_reviews(facilities: [facility]) }.to perform_under(20).ms
+  end
+
+  it "is performant for a large amount of facilities" do
+    Array.new(10) { Fabricate(:facility, space_types: [space_type]) }
+    expect { space.aggregate_facility_reviews }.to perform_under(30).ms
+  end
 end

spec/spec_helper.rb

@@ -15,8 +15,11 @@
 # it.
 #
 # See http://rubydoc.info/gems/rspec-core/RSpec/Core/Configuration
+require "rspec-benchmark"
 
 RSpec.configure do |config|
+  config.include RSpec::Benchmark::Matchers
+
   # Set locale, to make sure the tests run correctly:
   config.before(:suite) do
     Faker::Config.locale = "en"

spec/support/performance_benchmark.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+require "benchmark"
+class PerformanceBenchmarkHighest
+  def initialize(disabled: false)
+    @start_times = {}
+    @scores = []
+    @disabled = disabled
+  end
+
+  def track_score_for(title:, &block)
+    return yield block if @disabled
+
+    time = Benchmark.measure do
+      yield block
+    end
+    @scores << { title:, time: time.real, ms: (time.real * 1000).round(2) }
+  end
+
+  def start_timer_for(title)
+    return if @disabled
+
+    @start_times[title] = Time.zone.now
+  end
+
+  def end_timer_for(title)
+    return if @disabled
+
+    time = Time.zone.now - @start_times[title]
+    add_time(title:, time:)
+
+    @start_times[title] = nil
+  end
+
+  def highest_scores
+    return if @disabled
+
+    high_scores = @scores.group_by { |s| s[:title] }.map do |title, scores|
+      highest = "#{scores.max_by { |s| s[:time] }[:ms]}ms"
+      { "#{title}": highest, count: scores.count, total: "#{scores.sum { |s| s[:ms] }.round(2)}ms" }
+    end
+    puts "Highest times for each benchmark:"
+    puts high_scores
+  end
+
+  private
+
+  def add_time(title:, time:)
+    @scores << { title:, time:, ms: (time * 1000).round(2) }
+  end
+
+  def filter_callers_to_only_project_files
+    caller.select { |c| c.include?("/app/") }
+  end
+end