Skip to content

Commit

Permalink
Considerable performance improvements for space listing in the table,…
Browse files Browse the repository at this point in the history
… from seconds to sub 1 second. Still WIP around facilities.
  • Loading branch information
DanielJackson-Oslo committed Apr 18, 2024
1 parent 3f1e53a commit eae8f47
Show file tree
Hide file tree
Showing 11 changed files with 150 additions and 92 deletions.
9 changes: 4 additions & 5 deletions app/controllers/concerns/filterable_spaces.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,14 @@ module FilterableSpaces
private

def filter_spaces
@spaces = Space.includes([:images]).order("star_rating DESC NULLS LAST")
@filterable_facility_categories = FacilityCategory.includes(:facilities).all
@filterable_space_types = SpaceType.all

filter_by_location
filter_by_title
filter_by_space_types
filter_and_order_by_facilities

@filterable_facility_categories = FacilityCategory.includes(:facilities).all
@filterable_space_types = SpaceType.all
filter_and_order_by_facilities
end

def filter_by_title
Expand Down Expand Up @@ -47,6 +46,6 @@ def filter_by_location
def filter_and_order_by_facilities
return if params[:facilities].blank?

@spaces = Space.filter_on_facilities(@spaces, params[:facilities])
@spaces.filter_and_order_by_facilities(params[:facilities])
end
end
26 changes: 22 additions & 4 deletions app/controllers/spaces_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ class SpacesController < BaseControllers::AuthenticateController # rubocop:disab
include DefineGroupedFacilitiesForSpace

def index
@spaces = Space.includes_data_for_filter_list
filter_spaces
end

Expand Down Expand Up @@ -104,14 +105,19 @@ def rect_for_spaces
end

def spaces_search
view_as = params[:view_as]

@spaces = Space.all.order_by_star_rating
filter_spaces
space_count = @spaces.count
spaces = @spaces.first(SPACE_SEARCH_PAGE_SIZE)

view_as = params[:view_as]
space_count = @spaces.size

spaces = preload_spaces_data_for_view(view_as)

markers = spaces.map(&:render_map_marker)

@experiences = FacilityReview::LIST_EXPERIENCES

facility_ids = params[:facilities]&.map(&:to_i) || []
render json: {
listing: render_to_string(
Expand All @@ -127,6 +133,18 @@ def spaces_search
}
end

def preload_spaces_data_for_view(view_as)
if view_as == "table"
@spaces
.includes_data_for_show
.limit(SPACE_SEARCH_PAGE_SIZE)
else
@spaces
.includes_data_for_filter_list
.limit(SPACE_SEARCH_PAGE_SIZE)
end
end

def check_duplicates
duplicates = duplicates_from_params

Expand All @@ -143,7 +161,7 @@ def check_duplicates
spaces: duplicates
}
),
count: duplicates.count
count: duplicates.size
}
end

Expand Down
149 changes: 97 additions & 52 deletions app/models/space.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,103 @@ class Space < ApplicationRecord # rubocop:disable Metrics/ClassLength
)
}

scope :with_images, lambda {
includes(
{
images: [
{
image_attachment: :blob
}
]
}
)
}

scope :order_by_star_rating, lambda {
order(
"spaces.star_rating DESC NULLS LAST"
)
}

scope :includes_data_for_filter_list, lambda {
with_images
.includes(
:reviews,
:space_facilities,
:space_types
)
}

scope :with_space_facilities, lambda {
includes(
{
space_facilities: {
facility: [
:facility_categories,
:facility_reviews
]
}
}
)
}

scope :with_reviews, lambda {
includes({
reviews: [
:user
]
})
}

scope :with_rich_text_from_space_group, lambda {
includes(
{
space_group: %i[
rich_text_how_to_book
rich_text_pricing
rich_text_who_can_use
rich_text_terms
rich_text_about
space_contacts
]
}
)
}

scope :includes_data_for_show, lambda {
with_all_rich_text
.with_rich_text_from_space_group
.with_images
.with_space_facilities
.with_reviews
.includes(
:space_types,
:space_contacts
)
}

scope :filter_and_order_by_facilities, lambda { |filtered_facilities|
where(space_facilities: { relevant: true, facility_id: filtered_facilities })
.select("spaces.*, (
(
SELECT
SUM(
CASE
WHEN space_facilities.experience = 'likely' THEN 3.0
WHEN space_facilities.experience = 'maybe' THEN 2.0
WHEN space_facilities.experience = 'unknown' THEN 1.0
WHEN space_facilities.experience = 'unlikely' THEN -1.0
WHEN space_facilities.experience = 'impossible' THEN -2.0
ELSE 0
END
)
FROM space_facilities
WHERE space_facilities.space_id = spaces.id)
+ COALESCE((spaces.star_rating - 2.9) / 10, 0)
) AS score")
.order(score: :desc)
}

has_rich_text :how_to_book
has_rich_text :who_can_use
has_rich_text :pricing
Expand All @@ -66,10 +163,6 @@ class Space < ApplicationRecord # rubocop:disable Metrics/ClassLength
aggregate_star_rating
end

def reviews
Review.includes([:user]).where(space_id: id)
end

def reviews_for_facility(facility)
space_facilities.find_by(facility:).experience
end
Expand Down Expand Up @@ -127,54 +220,6 @@ def potential_duplicates
Spaces::DuplicateDetectorService.call(self)
end

# Move this somewhere better, either a service or figure out a way to make it a scope
# NOTE: this expects a scope for spaces but returns an array
# preferably we would find some way to return a scope too
def self.filter_on_facilities(spaces, filtered_facilities)
results = spaces.includes(:space_facilities)
.where(space_facilities: { relevant: true, facility_id: filtered_facilities })
.filter_map(&:score_by_filter_on_facilities)

results.sort_by(&:score).map(&:space)
end

def score_by_filter_on_facilities
space_score = 0.0
# The more correct matches the lower the number.
# this is so the sort_by later will be correct as it sorts by lowest first
# we could do a reverse on the result of sort_by but this will incur
# a performance overhead

space_facilities.each do |space_facility|
space_score -= score_from_experience(space_facility)
end

# Add a score for the star_rating to use as a tie-breaker.
space_score -= score_from_star_rating

OpenStruct.new(score: space_score, space: self) # rubocop:disable Style/OpenStructUse
end

def score_from_experience(space_facility)
return 3.0 if space_facility.likely?
return 2.0 if space_facility.maybe?
return 1.0 if space_facility.unknown?
return -1.0 if space_facility.unlikely?

-2.0 if space_facility.impossible?
end

def score_from_star_rating
# Star ratings under 3 should score so the space is filtered lower,
# star ratings at 3 or over should place the search result higher.
#
# Star ratings go from 1-5, if present, so just subtract 2.9 to get the right
# sorting, then divide by 10 to make it less relevant than scores based on matches
return 0 if star_rating.blank?

(star_rating - 2.9) / 10
end

# Space Facilities that are typically relevant for the space
# Either because they share a space type with the space
# or because someone has said that they are relevant for
Expand Down
2 changes: 1 addition & 1 deletion app/services/spaces/aggregate_star_rating_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def initialize(space:)
end

def call
if space.reviews.count.zero?
if space.reviews.empty?
space.update!(star_rating: nil)
return
end
Expand Down
2 changes: 1 addition & 1 deletion app/views/space_images/_image_slider.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
<% end %>
<% end %>
<div class="whitespace-nowrap justify-self-end ml-auto rounded bg-black bg-opacity-80 text-white text-opacity-80 py-1 px-2">
<%= index + 1 %> / <%= images.count %>
<%= index + 1 %> / <%= images.size %>
</div>
</div>
<noscript>
Expand Down
6 changes: 3 additions & 3 deletions app/views/spaces/index/_space_listing.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@

<div class="flex gap-4 mb-2.5">

<div class="flex gap-2" title="<%= t('space_show.number_of_reviews', count: space.reviews.count) %>">
<div class="flex gap-2" title="<%= t('space_show.number_of_reviews', count: space.reviews.size) %>">
<%= inline_svg_tag 'star', class: "#{space.star_rating ? 'text-pink-500' : 'text-gray-300'}" %>
<span>
<%= space.star_rating_s %>
<% if space.reviews.count > 0 %>
<span class="opacity-50">(<%= space.reviews.count %>)</span>
<% if space.reviews.size > 0 %>
<span class="opacity-50">(<%= space.reviews.size %>)</span>
<% end %>
</span>
</div>
Expand Down
8 changes: 4 additions & 4 deletions app/views/spaces/index/_space_listings.html.erb
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
<%# locals: (view_as:, spaces:, space_count:, page_size:, filtered_facilities: %>
<% if view_as == "table" %>
<% @spaces = spaces %>
<%= render partial: "spaces/list_views/space_table_view" %>
<%= render "spaces/list_views/space_table_view", spaces:, filtered_facilities: %>
<% else # default to map view %>
<%= render partial: 'spaces/index/space_listing_header', locals: { space_count: space_count } %>
<%= render 'spaces/index/space_listing_header', space_count: %>
<% spaces.each do |space| %>
<%= render partial: "spaces/index/space_listing", locals: { space: space, filtered_facilities: filtered_facilities } %>
<%= render "spaces/index/space_listing", space: space, filtered_facilities: %>
<% end %>
<% end %>
<% if space_count > page_size %>
Expand Down
12 changes: 4 additions & 8 deletions app/views/spaces/list_views/_space.html.erb
Original file line number Diff line number Diff line change
@@ -1,9 +1,4 @@
<%#
Depends on:
- @space
Styled in part by content.scss
%>
<%# locals: ( ) %>
<tr>

<th scope="row">
Expand Down Expand Up @@ -41,8 +36,9 @@
<%= render 'space_contacts/index' %>
</td>

<% FacilityCategory.all.each do |facility_category| %>
<% @filterable_facility_categories.each do |facility_category| %>
<td>
<% if false %>
<div>
<ul>
<%
Expand Down Expand Up @@ -81,7 +77,7 @@
</details>
<% end %>
</div>

<% end %>

</td>
<% end %>
Expand Down
16 changes: 8 additions & 8 deletions app/views/spaces/list_views/_space_table_view.html.erb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<%# locals: (spaces:, filtered_facilities:) %>
<table class="space_table border-collapse">
<thead>
<tr>
Expand All @@ -14,7 +15,7 @@
Kontaktinfo
</th>

<% FacilityCategory.all.each do |facility_category| %>
<% @filterable_facility_categories.each do |facility_category| %>
<th scope="col">
Fasiliteter: <%= facility_category.title %>
</th>
Expand All @@ -40,17 +41,16 @@
</tr>
</thead>
<tbody>
<% @spaces.each do |space| %>
<% spaces.each do |space| %>
<%
@space = space
@grouped_relevant_facilities = @space.relevant_space_facilities(grouped: true, user: current_user)
@non_relevant_facilities = @space.non_relevant_space_facilities
@grouped_non_relevant_facilities = @space.group_space_facilities(ungrouped_facilities: @non_relevant_facilities,
user: current_user)
@experiences = FacilityReview::LIST_EXPERIENCES
# @grouped_relevant_facilities = @space.relevant_space_facilities(grouped: true, user: current_user)
#@non_relevant_facilities = @space.non_relevant_space_facilities
#@grouped_non_relevant_facilities = @space.group_space_facilities(ungrouped_facilities: @non_relevant_facilities,
# user: current_user)
%>
<% @space_contact = SpaceContact.new(space_id: @space.id, space_group_id: @space.space_group_id) %>
<%= render partial: "spaces/list_views/space", locals: { space: space } %>
<%= render partial: "spaces/list_views/space" %>
<% end %>
</tbody>
</table>
6 changes: 3 additions & 3 deletions app/views/spaces/new/_space_duplicate_list.html.erb
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
<div class="rounded-lg mb-4 px-4 py-6 bg-pink-50 -mx-4">
<div data-duplicate-checker-target="duplicates">
<h2 class="no-mt"><%= t("space_create.any_of_these", count: spaces.count) %></h2>
<h2 class="no-mt"><%= t("space_create.any_of_these", count: spaces.size) %></h2>
<div class="flex flex-wrap gap-4 mb-4 align-stretch">
<% spaces.each do |space| %>
<%= render partial: "spaces/new/space_duplicate_item", locals: { space: space, filtered_facilities: nil } %>
<% end %>
</div>
</div>
<button class="button submit" type="button" data-duplicate-checker-target="ignoreDuplicatesButton" data-action="click->duplicate-checker#ignoreDuplicates">
<%= t('space_create.none_are_duplicates', count: spaces.count) %>
<%= t('space_create.none_are_duplicates', count: spaces.size) %>
</button>
<button class="button button--white hidden" type="button" data-duplicate-checker-target="showDuplicatesButton" data-action="click->duplicate-checker#unIgnoreDuplicates">
<%= t('space_create.show_duplicates_again', count: spaces.count) %>
<%= t('space_create.show_duplicates_again', count: spaces.size) %>
</button>
</div>
Loading

0 comments on commit eae8f47

Please sign in to comment.