From 4736127e1f00333de9ba89fbcdd6305924824102 Mon Sep 17 00:00:00 2001 From: Daniel Jackson Date: Fri, 1 Mar 2024 11:40:53 +0100 Subject: [PATCH 1/6] Add failing test that shows the bug in feature spec --- .../user_creates_facility_reviews_spec.rb | 33 +++++++++++++++++-- .../user_creates_facility_reviews_helpers.rb | 30 +++++++++++------ 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/spec/features/user_creates_facility_reviews_spec.rb b/spec/features/user_creates_facility_reviews_spec.rb index 69e54ae2..1e0a1082 100644 --- a/spec/features/user_creates_facility_reviews_spec.rb +++ b/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,34 @@ ) 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 + ) + + expect_to_only_change_facility_review_description( + space:, + facility: second_facility_to_review, + 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) diff --git a/spec/support/user_creates_facility_reviews_helpers.rb b/spec/support/user_creates_facility_reviews_helpers.rb index 2c601f31..c440c5e8 100644 --- a/spec/support/user_creates_facility_reviews_helpers.rb +++ b/spec/support/user_creates_facility_reviews_helpers.rb @@ -54,15 +54,19 @@ def expect_to_add_facility_review( def expect_to_add_multiple_facility_reviews( space:, - facilities_to_review: [] + facilities_to_review: [], + description_to_add: "" ) expect_changes_around_reviewing_facility( facility: facilities_to_review, space:, - count_that_changes_to_new_experience: facilities_to_review.count + count_that_changes_to_new_experience: facilities_to_review.count, + expected_new_description: description_to_add, + expected_description_count: facilities_to_review.count ) do click_to_create_multiple_facility_reviews( - facilities_to_review: + facilities_to_review:, + description_to_add: ) end end @@ -73,9 +77,9 @@ def click_to_create_new_facility_review(facility:, enter_data_about_facility(facility:, description_to_add:) end - def click_to_create_multiple_facility_reviews(facilities_to_review: []) + def click_to_create_multiple_facility_reviews(description_to_add:, facilities_to_review: []) facilities_to_review.each do |facility| - enter_data_about_facility(facility:) + enter_data_about_facility(facility:, description_to_add:) end end @@ -95,8 +99,14 @@ def enter_data_about_facility(facility:, description_to_add: "") submit_button.click end + expect_form_to_close_with_flash(facility:) + end + + def expect_form_to_close_with_flash(facility:) # Then wait until the save is done and flash message is shown expect(page).to have_text(I18n.t("reviews.added_review")) + # And for the form element defined above to be gone + expect(page).to have_no_selector("form", text: facility.title) end # rubocop:disable Metrics/ParameterLists, Metrics/MethodLength, Metrics/AbcSize # This is a test helper, it's fine @@ -108,7 +118,8 @@ def expect_changes_around_reviewing_facility( count_of_visible_facilities: space.relevant_space_facilities.count, expected_previous_experience: "unknown", expected_new_experience: "likely", - expected_new_description: "" + expected_new_description: "", + expected_description_count: 1 ) count_should_have_new_experience = @@ -179,15 +190,14 @@ def expect_changes_around_reviewing_facility( return if expected_new_description.blank? # Check that the description changed in the model - expect( - space_facilities_matching_facility.all? do |space_facility| - space_facility.description == expected_new_description + space_facilities_matching_facility.reload.all? do |space_facility| + space_facility.reload.description == expected_new_description end ).to be(true) # and in the rendered HTML: - expect(page).to have_text(expected_new_description) + expect(page).to have_text(expected_new_description, count: expected_description_count) end # rubocop:enable Metrics/ParameterLists, Metrics/MethodLength, Metrics/AbcSize end From e4a56a941cc92ef2032b9670332de48387c170bc Mon Sep 17 00:00:00 2001 From: Daniel Jackson Date: Fri, 1 Mar 2024 11:58:25 +0100 Subject: [PATCH 2/6] Stop destroying Space Facilities on Aggregation. We need a better way, currently we are losing data. --- .../aggregate_facility_reviews_service.rb | 4 +- ...aggregate_facility_reviews_service_spec.rb | 39 +++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/app/services/spaces/aggregate_facility_reviews_service.rb b/app/services/spaces/aggregate_facility_reviews_service.rb index 3caa5674..d80cb0dd 100644 --- a/app/services/spaces/aggregate_facility_reviews_service.rb +++ b/app/services/spaces/aggregate_facility_reviews_service.rb @@ -17,8 +17,6 @@ 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 @@ -26,7 +24,7 @@ def aggregate_facilities end def aggregate_reviews(facility) # rubocop:disable Metrics/AbcSize - space_facility = SpaceFacility.create(space: @space, facility:) + space_facility = SpaceFacility.find_or_create_by(space: @space, facility:) reviews = @space.facility_reviews.where(facility:).order(created_at: :desc).limit(5) count = reviews.count diff --git a/spec/services/spaces/aggregate_facility_reviews_service_spec.rb b/spec/services/spaces/aggregate_facility_reviews_service_spec.rb index 272f3e5a..c0c72258 100644 --- a/spec/services/spaces/aggregate_facility_reviews_service_spec.rb +++ b/spec/services/spaces/aggregate_facility_reviews_service_spec.rb @@ -157,4 +157,43 @@ 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 end From 709893637c961f3bb7ba5f5e0cce3e0302b1f547 Mon Sep 17 00:00:00 2001 From: Daniel Jackson Date: Fri, 1 Mar 2024 15:27:52 +0100 Subject: [PATCH 3/6] Add benchmarking specs for facility review aggregation. Currently at 180ms - 400ms for one space with 100 facilities. --- Gemfile | 1 + Gemfile.lock | 9 +++++++++ .../spaces/aggregate_facility_reviews_service_spec.rb | 10 ++++++++++ spec/spec_helper.rb | 3 +++ 4 files changed, 23 insertions(+) diff --git a/Gemfile b/Gemfile index 87af0d27..dd6f39c1 100644 --- a/Gemfile +++ b/Gemfile @@ -68,6 +68,7 @@ end group :test do gem "capybara" + gem "rspec-benchmark" gem "rspec-rails" gem "selenium-webdriver" gem "vcr" diff --git a/Gemfile.lock b/Gemfile.lock index 6c0e3f84..d19bdeae 100644 --- a/Gemfile.lock +++ b/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 diff --git a/spec/services/spaces/aggregate_facility_reviews_service_spec.rb b/spec/services/spaces/aggregate_facility_reviews_service_spec.rb index c0c72258..72a7faec 100644 --- a/spec/services/spaces/aggregate_facility_reviews_service_spec.rb +++ b/spec/services/spaces/aggregate_facility_reviews_service_spec.rb @@ -196,4 +196,14 @@ def experience(experience, other_facility = nil) # 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 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 81c0b8b6..a4d4c1a6 100644 --- a/spec/spec_helper.rb +++ b/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" From 320c35f5d5449085d7200db05112c0b135385164 Mon Sep 17 00:00:00 2001 From: Daniel Jackson Date: Fri, 1 Mar 2024 15:28:42 +0100 Subject: [PATCH 4/6] Add a little dev helper tool for benchmarking. Can probably be deleted later, but didn't want to delete it now :{ --- spec/support/performance_benchmark.rb | 55 +++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 spec/support/performance_benchmark.rb diff --git a/spec/support/performance_benchmark.rb b/spec/support/performance_benchmark.rb new file mode 100644 index 00000000..862c0f50 --- /dev/null +++ b/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 From 1eac656ef502bbfa9c03fb368a6fef5b3b962645 Mon Sep 17 00:00:00 2001 From: Daniel Jackson Date: Fri, 1 Mar 2024 15:29:15 +0100 Subject: [PATCH 5/6] Make aggregation a lot more performant, by skipping loads of steps to the db. Cutting from 200ms to 20ms mean time on 100 facilities. --- app/models/space.rb | 13 ++++- .../aggregate_facility_reviews_service.rb | 54 +++++++++++-------- .../spaces/concerns/countable_reviews.rb | 40 ++++++++++++++ 3 files changed, 84 insertions(+), 23 deletions(-) create mode 100644 app/services/spaces/concerns/countable_reviews.rb diff --git a/app/models/space.rb b/app/models/space.rb index d400fd28..8f5ea74f 100644 --- a/app/models/space.rb +++ b/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 diff --git a/app/services/spaces/aggregate_facility_reviews_service.rb b/app/services/spaces/aggregate_facility_reviews_service.rb index d80cb0dd..828250c1 100644 --- a/app/services/spaces/aggregate_facility_reviews_service.rb +++ b/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 @@ -23,55 +30,58 @@ def aggregate_facilities end end - def aggregate_reviews(facility) # rubocop:disable Metrics/AbcSize - space_facility = SpaceFacility.find_or_create_by(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 diff --git a/app/services/spaces/concerns/countable_reviews.rb b/app/services/spaces/concerns/countable_reviews.rb new file mode 100644 index 00000000..9d70671a --- /dev/null +++ b/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 From 772d171192170073db969e8d780b39b9d9616fbd Mon Sep 17 00:00:00 2001 From: Daniel Jackson Date: Fri, 1 Mar 2024 15:33:37 +0100 Subject: [PATCH 6/6] Fix so we assume the right count for a changed test --- spec/features/user_creates_facility_reviews_spec.rb | 4 +++- spec/support/user_creates_facility_reviews_helpers.rb | 7 ++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/spec/features/user_creates_facility_reviews_spec.rb b/spec/features/user_creates_facility_reviews_spec.rb index 1e0a1082..b1e6424e 100644 --- a/spec/features/user_creates_facility_reviews_spec.rb +++ b/spec/features/user_creates_facility_reviews_spec.rb @@ -79,12 +79,14 @@ expect_to_add_facility_review( space:, - facility: second_facility_to_review + 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 ) diff --git a/spec/support/user_creates_facility_reviews_helpers.rb b/spec/support/user_creates_facility_reviews_helpers.rb index c440c5e8..19dae87b 100644 --- a/spec/support/user_creates_facility_reviews_helpers.rb +++ b/spec/support/user_creates_facility_reviews_helpers.rb @@ -5,7 +5,8 @@ module UserCreatesFacilityReviewsHelpers def expect_to_only_change_facility_review_description( space:, facility:, - description_to_add: "" + description_to_add: "", + count_that_already_have_the_new_experience: 1 ) expected_previous_experience = @@ -18,7 +19,7 @@ def expect_to_only_change_facility_review_description( space:, facility:, description_to_add:, - count_that_already_have_the_new_experience: 1, + count_that_already_have_the_new_experience:, count_that_changes_to_new_experience: 0, expected_previous_experience: ) @@ -141,7 +142,7 @@ def expect_changes_around_reviewing_facility( # Expect that we have more than one facility visible: expect(count_of_visible_facilities).to be > 1 - # Expect that none of those facilities to show as reviewed so far in the model: + # Expect that none of those facilities show as reviewed so far in the model: expect( relevant_space_facilities.where(experience: expected_new_experience).count ).to eq(count_that_already_have_the_new_experience)