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)