From a69f39027a23452800c535751e6b2dc197db5200 Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Thu, 17 Oct 2024 14:14:14 +0200 Subject: [PATCH] RfC Search: Apply last_per_user filter after other search terms Without this change, we would first filter for the last 2 RfCs per record and then perform the search. Imagine, a user has three RfCs for exercises 1, 2, 3 (created in the order of the exercises). Now, when searching for open RfCs of the first exercise, the learner's RfC wasn't shown: The last_per_user would only return RfC for exercise 2 and 3, where the search term for exercise 1 would remove those two from the result set further. With the new version, we first apply the custom filter and then limit the result set by two RfCs per learner. We further took the opportunity to fix the broken sorting (by exercise name). The resulting code seems pretty complex, but I wasn't able to find a better, more suitable (and performant!) version. --- .../request_for_comments_controller.rb | 209 +++++++++++------- app/models/concerns/ransack_object.rb | 15 ++ app/models/exercise.rb | 1 + app/models/request_for_comment.rb | 144 +++++++++--- .../request_for_comments/index.html.slim | 8 +- 5 files changed, 264 insertions(+), 113 deletions(-) create mode 100644 app/models/concerns/ransack_object.rb diff --git a/app/controllers/request_for_comments_controller.rb b/app/controllers/request_for_comments_controller.rb index 1234c3088..6580df35a 100644 --- a/app/controllers/request_for_comments_controller.rb +++ b/app/controllers/request_for_comments_controller.rb @@ -15,58 +15,37 @@ def authorize! # GET /request_for_comments # GET /request_for_comments.json def index - @search = policy_scope(RequestForComment) - .last_per_user(2) - .joins(:exercise) - .where(exercises: {unpublished: false}) - .order(created_at: :desc) # Order for the LIMIT part of the query - .ransack(params[:q]) - - # This total is used later to calculate the total number of entries - request_for_comments = @search.result - # All conditions are included in the query, so get the number of requested records - .paginate(page: params[:page], per_page: per_page_param) + @search = ransack_search do |rfcs| + # Only show RfCs for published exercises + rfcs.where(exercises: {unpublished: false}) + end - @request_for_comments = RequestForComment.where(id: request_for_comments) - .with_last_activity # expensive query, so we only do it for the current page - .includes(submission: %i[study_group exercise contributor]) - .includes(:file, :comments, user: [:consumer]) - .order(created_at: :desc) # Order for the view - # We need to manually enable the pagination links. - .extending(WillPaginate::ActiveRecord::RelationMethods) - @request_for_comments.current_page = WillPaginate::PageNumber(params[:page] || 1) - @request_for_comments.limit_value = per_page_param - @request_for_comments.total_entries = @search.result.length + @request_for_comments = find_and_paginate_rfcs do |rfcs| + # Order for the view (and subqueries) + rfcs.order(created_at: :desc) + end authorize! end # GET /my_request_for_comments def my_comment_requests - @search = policy_scope(RequestForComment) - .joins(:submission) - .where(user: current_user) - .or(policy_scope(RequestForComment) - .joins(:submission) - .where(submissions: {contributor: current_user.programming_groups})) - .order(created_at: :desc) # Order for the LIMIT part of the query - .ransack(params[:q]) - - # This total is used later to calculate the total number of entries - request_for_comments = @search.result - # All conditions are included in the query, so get the number of requested records - .paginate(page: params[:page], per_page: per_page_param) - - @request_for_comments = RequestForComment.where(id: request_for_comments) - .with_last_activity - .includes(submission: %i[study_group exercise]) - .includes(:file, :comments, :user) - .order(created_at: :desc) # Order for the view - # We need to manually enable the pagination links. - .extending(WillPaginate::ActiveRecord::RelationMethods) - @request_for_comments.current_page = WillPaginate::PageNumber(params[:page] || 1) - @request_for_comments.limit_value = per_page_param - @request_for_comments.total_entries = @search.result.length + @search = ransack_search do |rfcs| + # Only show any RfC the user has created or any RfC for a submission of any programming group the user belongs to + rfcs.joins(:submission) + .where(user: current_user) + .or(policy_scope(RequestForComment) + .joins(:exercise) + .joins(:submission) + # Using the IDs here is much faster than using the polymorphic association. + # This is because the association would result in a nested loop join. + .where(submissions: {contributor_id: current_user.programming_group_ids})) + end + + @request_for_comments = find_and_paginate_rfcs(per_user: nil) do |rfcs| + # Order for the view (and subqueries) + rfcs.order(created_at: :desc) + end authorize! render 'index' @@ -74,26 +53,16 @@ def my_comment_requests # GET /my_rfc_activity def rfcs_with_my_comments - # As we order by `last_comment`, we need to include `with_last_activity` in the original query. - # Therefore, the optimization chosen above doesn't work here. - @search = policy_scope(RequestForComment) - .joins(:comments) # we don't need to outer join here, because we know the user has commented on these - .where(comments: {user: current_user}) - .ransack(params[:q]) - # This total is used later to calculate the total number of entries - request_for_comments = @search.result - .paginate(page: params[:page], per_page: per_page_param) - - @request_for_comments = RequestForComment.where(id: request_for_comments) - .with_last_activity - .includes(submission: %i[study_group exercise contributor]) - .includes(:file, :comments, user: [:consumer]) - .order(last_comment: :desc) - # We need to manually enable the pagination links. - .extending(WillPaginate::ActiveRecord::RelationMethods) - @request_for_comments.current_page = WillPaginate::PageNumber(params[:page] || 1) - @request_for_comments.limit_value = per_page_param - @request_for_comments.total_entries = @search.result.length + @search = ransack_search do |rfcs| + # Only show RfCs with comments by the user + rfcs.joins(:comments) + .where(comments: {user: current_user}) + end + + @request_for_comments = find_and_paginate_rfcs(per_user: nil) do |rfcs| + # Order for the view (and subqueries) + rfcs.order(last_activity: :desc) + end authorize! render 'index' @@ -102,16 +71,18 @@ def rfcs_with_my_comments # GET /exercises/:id/request_for_comments def rfcs_for_exercise exercise = Exercise.find(params[:exercise_id]) - @search = policy_scope(RequestForComment) - .with_last_activity - .where(exercise_id: exercise.id) - .ransack(params[:q]) - @request_for_comments = @search.result - .joins(:exercise) - .order(last_comment: :desc) - .paginate(page: params[:page], per_page: per_page_param) - # let the exercise decide, whether its rfcs should be visible authorize(exercise) + + @search = ransack_search do |rfcs| + # Only show RfCs belonging to the exercise + rfcs.where(exercise:) + end + + @request_for_comments = find_and_paginate_rfcs(per_user: nil) do |rfcs| + # Order for the view (and subqueries) + rfcs.order(last_activity: :desc) + end + render 'index' end @@ -215,4 +186,94 @@ def set_study_group_grouping @study_groups_grouping = [[t('request_for_comments.index.study_groups.current'), Array(current_study_group)], [t('request_for_comments.index.study_groups.my'), my_study_groups.reject {|group| group == current_study_group }]] end + + def ransack_search + # The `policy_scope` is used to enforce the consumer's rfc_visibility setting. + # This is defined through Pundit, see `RequestForCommentPolicy::Scope` in `policies/request_for_comment_policy.rb`. + rfcs = policy_scope(RequestForComment) + # The join is used for the where clause below but also ensures we are not showing RfCs without an exercise + .joins(:exercise) + + # Allow the caller to apply additional conditions to the search. + rfcs = yield rfcs if block_given? + + # Apply the actual search and sort options. + # The sort options are applied, so that the resulting Ransack::Search object contains all desired options. + # However, the actual sorting is deferred (through a call to `reorder(nil)`) and performed later. + # Still, for the view helper, we need to store the sort options in the search object. + rfcs.ransack(params[:q]) + end + + def ransack_sort + # Extract the sort options from the params and apply them to a new ransack search. + # This is used to apply the sort options to the search result *after* filtering the results. + RequestForComment.ransack(s: params.dig(:q, :s)) + end + + def find_and_paginate_rfcs(per_user: 2) + # 1. Get any sort options from the caller. + # For convenience, the caller can provide these options as ActiveRecord::Relation query methods. + desired_sort_options = yield RequestForComment if block_given? + desired_sort_options = desired_sort_options&.arel&.orders + + # 2. Apply the filter options to the RfCs as specified by Ransack. + matching_records_arel = RequestForComment.with_ransack_search_arel(@search) + # 3. Get the last n RfCs per user. A value of `nil` for `per_user` means all RfCs. + last_matching_records_arel = RequestForComment.with_last_per_user_arel(matching_records_arel, per_user) + + # 4. Apply the sort options to the RfCs and paginate the result set. + # We need to sort first, since the pagination is applied to the sorted result set. + # If the sort options include the `last_activity` attribute, we need to include it in the query. + if desired_sort_options&.any? {|sort| sort.expr.to_s == '"last_activity"' } + # Annotating RfCs with the last comment activity is an expensive operation, since it includes three joins. + # Therefore, we only do this if the sort options include the `last_activity` attribute. + annotated_last_matching_records_arel = RequestForComment.with_last_activity_arel(last_matching_records_arel) + # On the resulting query, we apply the sort options as specified by Ransack the caller. + sorted_last_matching_records_arel = RequestForComment.with_ransack_sort_arel(ransack_sort, desired_sort_options, annotated_last_matching_records_arel) + # Now, we paginate the result set and need to convert it to an Arel query. This is necessary for compatibility with the `else` branch. + annotated_final_result_set_arel = RequestForComment.from(sorted_last_matching_records_arel) + .paginate(page: params[:page], per_page: per_page_param) + .arel.as(RequestForComment.arel_table.name) + else + # Apply the sort options to the RfCs as specified by Ransack and the caller. + sorted_last_matching_records_arel = RequestForComment.with_ransack_sort_arel(ransack_sort, desired_sort_options, last_matching_records_arel) + # Now, we paginate the result set and simply extract the desired RfCs (their IDs, without the last comment activity). + selected_request_for_comments = RequestForComment.from(sorted_last_matching_records_arel) + .paginate(page: params[:page], per_page: per_page_param) + # For the final result set, we need to include the last comment activity as a preparation for the view. + # Since only `per_page_param` RfCs are selected, the performance impact is limited. + final_result_set_arel = RequestForComment.with_id_arel(selected_request_for_comments) + annotated_final_result_set_arel = RequestForComment.with_last_activity_arel(final_result_set_arel) + end + + # 5. Manually apply compatibility with WillPaginate and prefetch the necessary associations. + request_for_comments = RequestForComment.from(annotated_final_result_set_arel) + .includes(submission: %i[study_group exercise contributor]) + .includes(:file, :comments, user: [:consumer]) + .extending(WillPaginate::ActiveRecord::RelationMethods) + + # 6. Apply the sort options to **current page** of the result set. + # This is needed since the sorting is not kept when annotating the result set with the last comment activity. + # (We're grouping by the RfC attributes and the last comment activity.) + if ransack_sort.result.arel.join_sources.present? + request_for_comments = request_for_comments.joins(*ransack_sort.result.arel.join_sources) + .order(*ransack_sort.result.arel.orders) + end + + # 7. Similarly to the Ransack sort, we also need to reapply the sort options from the caller. + if desired_sort_options.present? + request_for_comments = request_for_comments.order(*desired_sort_options) + end + + # 8. We need to manually enable the pagination links. + request_for_comments.current_page = WillPaginate::PageNumber(params[:page] || 1) + request_for_comments.limit_value = per_page_param + request_for_comments.total_entries = RequestForComment.from(last_matching_records_arel).count + + # Debugging: Print the SQL query to the console. + Rails.logger.debug { request_for_comments.to_sql } + + # Return the paginated, sorted result set. + request_for_comments + end end diff --git a/app/models/concerns/ransack_object.rb b/app/models/concerns/ransack_object.rb new file mode 100644 index 000000000..46823e4fc --- /dev/null +++ b/app/models/concerns/ransack_object.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module RansackObject + # Case insensitive search with Postgres and Ransack. + # Adapted from https://activerecord-hackery.github.io/ransack/getting-started/simple-mode/#case-insensitive-sorting-in-postgresql + def self.included(base) + base.columns.each do |column| + next unless column.type == :string + + base.ransacker column.name.to_sym, type: :string do + Arel::Nodes::NamedFunction.new('LOWER', [base.arel_table[column.name]]) + end + end + end +end diff --git a/app/models/exercise.rb b/app/models/exercise.rb index 21f3018b3..41b78d777 100644 --- a/app/models/exercise.rb +++ b/app/models/exercise.rb @@ -7,6 +7,7 @@ class Exercise < ApplicationRecord include Creation include DefaultValues include TimeHelper + include RansackObject after_initialize :generate_token after_initialize :set_default_values diff --git a/app/models/request_for_comment.rb b/app/models/request_for_comment.rb index 38ca4c0d5..09488041a 100644 --- a/app/models/request_for_comment.rb +++ b/app/models/request_for_comment.rb @@ -75,24 +75,6 @@ def state(filter = RequestForComment::ALL) end end - def with_last_activity - joins('join "submissions" s on s.id = request_for_comments.submission_id ' \ - 'left outer join "files" f on f.context_id = s.id ' \ - 'left outer join "comments" c on c.file_id = f.id') - .group('request_for_comments.id') - .select('request_for_comments.*, max(c.updated_at) as last_comment') - end - - def last_per_user(count = 5) - from(row_number_user_sql, :request_for_comments) - .where(row_number: ..count) - .group('request_for_comments.id, request_for_comments.user_id, request_for_comments.user_type, ' \ - 'request_for_comments.exercise_id, request_for_comments.file_id, request_for_comments.question, ' \ - 'request_for_comments.created_at, request_for_comments.updated_at, request_for_comments.solved, ' \ - 'request_for_comments.full_score_reached, request_for_comments.submission_id, request_for_comments.row_number') - # ugly, but necessary - end - def ransackable_associations(_auth_object = nil) %w[exercise submission] end @@ -101,23 +83,115 @@ def ransackable_scopes(_auth_object = nil) %w[state] end - private - - def row_number_user_sql - select(' - request_for_comments.id, - request_for_comments.user_id, - request_for_comments.user_type, - request_for_comments.exercise_id, - request_for_comments.file_id, - request_for_comments.question, - request_for_comments.created_at, - request_for_comments.updated_at, - request_for_comments.solved, - request_for_comments.full_score_reached, - request_for_comments.submission_id, - row_number() OVER (PARTITION BY request_for_comments.user_id, request_for_comments.user_type ORDER BY request_for_comments.created_at DESC) as row_number - ') + def ransortable_attributes(_auth_object = nil) + %w[created_at] + end + + # @param [Arel::Nodes::TableAlias] from_arel_table The Arel table to select from. + # @param [Integer] count The number of RfCs to select per user. `nil` selects all. + # @return [Arel::Nodes::TableAlias] The resulting Arel query limited to `count` RfCs per user. + def with_last_per_user_arel(from_arel_table, count = 5) + # If no limit should be applied, we don't need to perform any calculations. + return from_arel_table if count.nil? + + row_number_column_name = :row_number + # We fist need to assign a row number to each RfC per user. + search_result_with_row_number_arel = RequestForComment.with_row_number_arel(from_arel_table, row_number_column_name) + # Then we can select the first `count` RfCs per user. + RequestForComment.with_first_n_row_numbers_arel(search_result_with_row_number_arel, row_number_column_name, count) + end + + # @param [RanSack::Search] search The Ransack search object with the search parameters. + # @return [Arel::Nodes::TableAlias] The resulting Arel query limited to the search parameters. + def with_ransack_search_arel(search) + rfc_table = RequestForComment.arel_table + # We need to use `reorder(nil)` to remove any existing order from the search. + # This is required to ensure the correct filtering and pagination is performed. + search.result.reorder(nil).arel.as(rfc_table.name) + end + + # @param [RanSack::Search] sort The Ransack search object with the sort parameters. Any search parameters are ignored. + # @param [Array] secondary_sort_options Additional sort options to apply after the Ransack sort. + # @param [Arel::Nodes::TableAlias] from_arel_table The Arel table to select from. + # @return [Arel::Nodes::TableAlias] The resulting Arel query sorted by the sort parameters. + def with_ransack_sort_arel(sort, secondary_sort_options, from_arel_table) + rfc_table = RequestForComment.arel_table + sorted_arel = sort.result.arel + + if secondary_sort_options.present? + sorted_arel = sorted_arel.order(*secondary_sort_options) + end + + sorted_arel.from(from_arel_table).as(rfc_table.name) + end + + # @param [Arel::Nodes::TableAlias] from_arel_table The Arel table to select from. + # @param [String] row_number_column_name The name of the column to store the row number in. + # @return [Arel::Nodes::TableAlias] The resulting Arel query annotated with the row number. + def with_row_number_arel(from_arel_table, row_number_column_name) + rfc_table = RequestForComment.arel_table + + row_number = Arel::Nodes::Window.new + .partition(rfc_table[:user_id], rfc_table[:user_type]) + # Since we want to show the n newest RfCs per user, we need to order by `created_at` in descending order. + # This is not related to the order of the RfCs in the result set (e.g., the Ransack ordering). + .order(rfc_table[:created_at].desc) + + row_number_function = Arel::Nodes::NamedFunction.new('row_number', []) + .over(row_number) + .as(row_number_column_name.to_s) + + rfc_table.project(rfc_table[Arel.star], row_number_function) + .from(from_arel_table).as(rfc_table.name) + end + + # @param [Arel::Nodes::TableAlias] from_arel_table The Arel table to select from. + # @param [String] row_number_column_name The name of the column to store the row number in. + # @param [Integer] max_row_number The maximum row number to select. + # @return [Arel::Nodes::TableAlias] The resulting Arel query limited to the first `max_row_number` RfCs. + def with_first_n_row_numbers_arel(from_arel_table, row_number_column_name, max_row_number) + rfc_table = RequestForComment.arel_table + column_names = RequestForComment.column_names.map {|name| rfc_table[name.to_sym] } + + rfc_table.project(*column_names) + .where(rfc_table[row_number_column_name].lteq(max_row_number)) + .group(rfc_table[row_number_column_name], *column_names) + .from(from_arel_table).as(rfc_table.name) + end + + # @param [Arel::Nodes::TableAlias] from_arel_table The Arel table to select from. + # @return [Arel::Nodes::TableAlias] The resulting Arel query annotated with the last activity timestamp of the RfC. + def with_last_activity_arel(from_arel_table) + rfc_table = RequestForComment.arel_table + submissions_table = Submission.arel_table + files_table = CodeOcean::File.arel_table + comments_table = Comment.arel_table + column_names = RequestForComment.column_names.map {|name| rfc_table[name.to_sym] } + + # If no comment is available yet, we want to show the last activity of the RfC. + # This behavior is inline with `views/request_for_comments/index.html.slim`. + last_activity = Arel::Nodes::NamedFunction.new('COALESCE', [comments_table[:updated_at].maximum, rfc_table[:updated_at]]) + + # We need to join the submissions, files, and comments tables to get the last activity timestamp of a comment. + # This query is rather expensive and should be performed as late as possible. + rfc_table + .project(*column_names, last_activity.as('last_activity')) + .join(submissions_table) + .on(submissions_table[:id].eq(rfc_table[:submission_id])) + .join(files_table, Arel::Nodes::OuterJoin) + .on(files_table[:context_id].eq(submissions_table[:id])) + .join(comments_table, Arel::Nodes::OuterJoin) + .on(comments_table[:file_id].eq(files_table[:id])) + .group(*column_names) + .from(from_arel_table).as(rfc_table.name) + end + + # @param [Array, Array>] rfc_ids The IDs of the RfCs to select. + # @return [Arel::Nodes::TableAlias] The resulting Arel query limited to the specified RfCs. + def with_id_arel(rfc_ids) + rfc_table = RequestForComment.arel_table + + RequestForComment.where(id: rfc_ids).arel.as(rfc_table.name) end end end diff --git a/app/views/request_for_comments/index.html.slim b/app/views/request_for_comments/index.html.slim index dbe0a3d92..d6d739444 100644 --- a/app/views/request_for_comments/index.html.slim +++ b/app/views/request_for_comments/index.html.slim @@ -8,7 +8,7 @@ h1 = RequestForComment.model_name.human(count: :other) = f.label(:exercise_title_cont, RequestForComment.human_attribute_name('exercise'), class: 'visually-hidden form-label') = f.search_field(:exercise_title_cont, class: 'form-control', placeholder: RequestForComment.human_attribute_name('exercise')) .col-auto.mt-3.mt-md-0 - = f.label(:title_cont, t('request_for_comments.solved'), class: 'visually-hidden form-label') + = f.label(:state, t('request_for_comments.solved'), class: 'visually-hidden form-label') = f.select(:state, [[t('request_for_comments.show_all'), RequestForComment::ALL], [t('request_for_comments.show_unsolved'), RequestForComment::ONGOING], [t('request_for_comments.show_soft_solved'), RequestForComment::SOFT_SOLVED], [t('request_for_comments.show_solved'), RequestForComment::SOLVED]]) - unless current_user.consumer.rfc_visibility_study_group? .row @@ -23,12 +23,12 @@ h1 = RequestForComment.model_name.human(count: :other) tr th i.fa-regular.fa-lightbulb aria-hidden='true' title = t('request_for_comments.solved') align='right' - th.sorttable_nosort = sort_link(@search, :title, RequestForComment.human_attribute_name('exercise')) + th.sorttable_nosort = sort_link(@search, :exercise_title, RequestForComment.human_attribute_name('exercise')) th = RequestForComment.human_attribute_name('question') th i.fa-solid.fa-comment aria-hidden='true' title = t('request_for_comments.comments') align='center' th = RequestForComment.human_attribute_name('username') - th = RequestForComment.human_attribute_name('requested_at') + th.sorttable_nosort = sort_link(@search, :created_at, RequestForComment.human_attribute_name('requested_at')) th = RequestForComment.human_attribute_name('last_update') tbody - @request_for_comments.each do |request_for_comment| @@ -49,6 +49,6 @@ h1 = RequestForComment.model_name.human(count: :other) td = request_for_comment.comments.size td = link_to_if(request_for_comment.user && policy(request_for_comment.user).show?, request_for_comment.user.displayname, request_for_comment.user) td = t('shared.time.before', time: distance_of_time_in_words_to_now(request_for_comment.created_at)) - td = t('shared.time.before', time: distance_of_time_in_words_to_now(request_for_comment.last_comment.nil? ? request_for_comment.updated_at : request_for_comment.last_comment)) + td = t('shared.time.before', time: distance_of_time_in_words_to_now(request_for_comment.last_activity.nil? ? request_for_comment.updated_at : request_for_comment.last_activity)) = render('shared/pagination', collection: @request_for_comments)