Fix access to exercise-specific RfC listing

.gitlab-ci.yml

@@ -26,7 +26,7 @@ rubocop:
     - DIFF=$(git diff --name-only --diff-filter=d $CI_MERGE_REQUEST_DIFF_BASE_SHA)
     - echo $DIFF
     - "if [[ ! -z $DIFF ]]; then bundle exec rubocop --force-exclusion --parallel --display-style-guide $DIFF; fi"
-
+  allow_failure: true
 
 rspec:
   stage: test

app/controllers/exercises_controller.rb

@@ -71,7 +71,7 @@ class ExercisesController < ApplicationController
     handle_exercise_tips
     return if performed?
 
-    myparam = exercise_params.present? ? exercise_params : {}
+    myparam = exercise_params.presence || {}
     checked_exercise_tags = @exercise_tags.select { |et| myparam[:tag_ids].include? et.tag.id.to_s }
     removed_exercise_tags = @exercise_tags.reject { |et| myparam[:tag_ids].include? et.tag.id.to_s }
 
@@ -105,18 +105,6 @@ class ExercisesController < ApplicationController
     end
   end
 
-  def requests_for_comments
-    authorize!
-    @search = RequestForComment
-              .with_last_activity
-              .where(exercise: @exercise)
-              .ransack(params[:q])
-    @request_for_comments = @search.result
-                                   .order('last_comment DESC')
-                                   .paginate(page: params[:page])
-    render 'request_for_comments/index'
-  end
-
   def export_external_check
     codeharbor_check = ExerciseService::CheckExternal.call(uuid: @exercise.uuid, codeharbor_link: current_user.codeharbor_link)
     render json: {
@@ -131,7 +119,7 @@ class ExercisesController < ApplicationController
           exported: false
         }
       )
-    }, status: 200
+    }, status: :ok
   end
 
   def export_external_confirm
@@ -159,7 +147,7 @@ class ExercisesController < ApplicationController
 
   def import_uuid_check
     user = user_from_api_key
-    return render json: {}, status: 401 if user.nil?
+    return render json: {}, status: :unauthorized if user.nil?
 
     uuid = params[:uuid]
     exercise = Exercise.find_by(uuid: uuid)
@@ -176,17 +164,17 @@ class ExercisesController < ApplicationController
     tempfile.rewind
 
     user = user_from_api_key
-    return render json: {}, status: 401 if user.nil?
+    return render json: {}, status: :unauthorized if user.nil?
 
     ActiveRecord::Base.transaction do
       exercise = ::ProformaService::Import.call(zip: tempfile, user: user)
       exercise.save!
-      return render json: {}, status: 201
+      return render json: {}, status: :created
     end
   rescue Proforma::ExerciseNotOwned
-    render json: {}, status: 401
+    render json: {}, status: :unauthorized
   rescue Proforma::ProformaError
-    render json: t('exercises.import_codeharbor.import_errors.invalid'), status: 400
+    render json: t('exercises.import_codeharbor.import_errors.invalid'), status: :bad_request
   rescue StandardError => e
     Sentry.capture_exception(e)
     render json: t('exercises.import_codeharbor.import_errors.internal_error'), status: 500
@@ -200,7 +188,7 @@ class ExercisesController < ApplicationController
   private :user_from_api_key
 
   def user_by_codeharbor_token(api_key)
-    link = CodeharborLink.find_by_api_key(api_key)
+    link = CodeharborLink.find_by(api_key: api_key)
     link&.user
   end
   private :user_by_codeharbor_token
@@ -249,8 +237,8 @@ class ExercisesController < ApplicationController
     exercise_tips.each do |exercise_tip|
       exercise_tip.symbolize_keys!
       current_exercise_tip = ExerciseTip.find_or_initialize_by(id: exercise_tip[:id],
-                                     exercise: @exercise,
+                                                               exercise: @exercise,
-                                     tip_id: exercise_tip[:tip_id])
+                                                               tip_id: exercise_tip[:tip_id])
       current_exercise_tip.parent_exercise_tip_id = parent_exercise_tip_id
       current_exercise_tip.rank = rank
       rank += 1
@@ -309,12 +297,10 @@ class ExercisesController < ApplicationController
       @course_token =
         if lti_json.nil?
           ''
+        elsif match = lti_json.match(%r{^.*courses/([a-z0-9\-]+)/sections})
+          match.captures.first
         else
-          if match = lti_json.match(%r{^.*courses/([a-z0-9\-]+)/sections})
+          ''
-            match.captures.first
-          else
-            ''
-          end
         end
     else
       # no consumer, therefore implementation with internal user
@@ -356,7 +342,7 @@ class ExercisesController < ApplicationController
   end
 
   def intervention
-    intervention = Intervention.find_by_name(params[:intervention_type])
+    intervention = Intervention.find_by(name: params[:intervention_type])
     if intervention.nil?
       render(json: {success: 'false', error: "undefined intervention #{params[:intervention_type]}"})
     else
@@ -471,12 +457,12 @@ class ExercisesController < ApplicationController
         interventions = UserExerciseIntervention.where('user_id = ?  AND exercise_id = ?', @external_user.id, @exercise.id)
         @all_events = (@submissions + interventions).sort_by(&:created_at)
         @deltas = @all_events.map.with_index do |item, index|
-          delta = item.created_at - @all_events[index - 1].created_at if index > 0
+          delta = item.created_at - @all_events[index - 1].created_at if index.positive?
           delta.nil? || (delta > StatisticsHelper::WORKING_TIME_DELTA_IN_SECONDS) ? 0 : delta
         end
         @working_times_until = []
         @all_events.each_with_index do |_, index|
-          @working_times_until.push((format_time_difference(@deltas[0..index].inject(:+)) if index > 0))
+          @working_times_until.push((format_time_difference(@deltas[0..index].inject(:+)) if index.positive?))
         end
       else
         final_submissions = Submission.where(user: @external_user, exercise_id: @exercise.id).in_study_group_of(current_user).final
@@ -493,11 +479,11 @@ class ExercisesController < ApplicationController
       user_statistics = {}
       additional_filter = if policy(@exercise).detailed_statistics?
                             ''
-                          elsif ! policy(@exercise).detailed_statistics? && current_user.study_groups.count > 0
+                          elsif !policy(@exercise).detailed_statistics? && current_user.study_groups.count.positive?
                             "AND study_group_id IN (#{current_user.study_groups.pluck(:id).join(', ')}) AND cause = 'submit'"
                           else
                             # e.g. internal user without any study groups, show no submissions
-                            "AND FALSE"
+                            'AND FALSE'
                           end
       query = "SELECT user_id, MAX(score) AS maximum_score, COUNT(id) AS runs
               FROM submissions WHERE exercise_id = #{@exercise.id} #{additional_filter} GROUP BY
@@ -536,7 +522,7 @@ class ExercisesController < ApplicationController
     else
       respond_to do |format|
         format.html { redirect_to(implement_exercise_path(@submission.exercise)) }
-        format.json { render(json: {message: I18n.t('exercises.submit.failure')}, status: 503) }
+        format.json { render(json: {message: I18n.t('exercises.submit.failure')}, status: :service_unavailable) }
       end
     end
   end
@@ -564,7 +550,7 @@ class ExercisesController < ApplicationController
   end
 
   def redirect_after_submit
-    Rails.logger.debug('Redirecting user with score:s ' + @submission.normalized_score.to_s)
+    Rails.logger.debug("Redirecting user with score:s #{@submission.normalized_score}")
     if @submission.normalized_score == 1.0
       # if user is external and has an own rfc, redirect to it and message him to clean up and accept the answer. (we need to check that the user is external,
       # otherwise an internal user could be shown a false rfc here, since current_user.id is polymorphic, but only makes sense for external users when used with rfcs.)

app/controllers/request_for_comments_controller.rb

@@ -1,9 +1,11 @@
+# frozen_string_literal: true
+
 class RequestForCommentsController < ApplicationController
   include SubmissionScoring
 
   before_action :require_user!
-  before_action :set_request_for_comment, only: [:show, :mark_as_solved, :set_thank_you_note]
+  before_action :set_request_for_comment, only: %i[show mark_as_solved set_thank_you_note]
-  before_action :set_study_group_grouping, only: %i[index get_my_comment_requests get_rfcs_with_my_comments]
+  before_action :set_study_group_grouping, only: %i[index get_my_comment_requests get_rfcs_with_my_comments get_rfcs_for_exercise]
 
   def authorize!
     authorize(@request_for_comments || @request_for_comment)
@@ -14,9 +16,9 @@ class RequestForCommentsController < ApplicationController
   # GET /request_for_comments.json
   def index
     @search = RequestForComment
-                  .last_per_user(2)
+              .last_per_user(2)
-                  .with_last_activity
+              .with_last_activity
-                  .ransack(params[:q])
+              .ransack(params[:q])
     @request_for_comments = @search.result
                                 .joins(:exercise)
                                 .where(exercises: {unpublished: false})
@@ -30,12 +32,12 @@ class RequestForCommentsController < ApplicationController
   # GET /my_request_for_comments
   def get_my_comment_requests
     @search = RequestForComment
-                  .with_last_activity
+              .with_last_activity
-                  .where(user: current_user)
+              .where(user: current_user)
-                  .ransack(params[:q])
+              .ransack(params[:q])
     @request_for_comments = @search.result
-                                .order('created_at DESC')
+                                   .order('created_at DESC')
-                                .paginate(page: params[:page])
+                                   .paginate(page: params[:page])
     authorize!
     render 'index'
   end
@@ -43,17 +45,33 @@ class RequestForCommentsController < ApplicationController
   # GET /my_rfc_activity
   def get_rfcs_with_my_comments
     @search = RequestForComment
-                  .with_last_activity
+              .with_last_activity
-                  .joins(:comments) # we don't need to outer join here, because we know the user has commented on these
+              .joins(:comments) # we don't need to outer join here, because we know the user has commented on these
-                  .where(comments: {user_id: current_user.id})
+              .where(comments: {user_id: current_user.id})
-                  .ransack(params[:q])
+              .ransack(params[:q])
     @request_for_comments = @search.result
-                                .order('last_comment DESC')
+                                   .order('last_comment DESC')
-                                .paginate(page: params[:page])
+                                   .paginate(page: params[:page])
     authorize!
     render 'index'
   end
 
+  # GET /exercises/:id/request_for_comments
+  def get_rfcs_for_exercise
+    exercise = Exercise.find(params[:exercise_id])
+    @search = 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])
+    # let the exercise decide, whether its rfcs should be visible
+    authorize(exercise)
+    render 'index'
+  end
+
   # GET /request_for_comments/1/mark_as_solved
   def mark_as_solved
     authorize!
@@ -73,7 +91,7 @@ class RequestForCommentsController < ApplicationController
     @request_for_comment.thank_you_note = params[:note]
 
     commenters = @request_for_comment.commenters
-    commenters.each {|commenter| UserMailer.send_thank_you_note(@request_for_comment, commenter).deliver_now}
+    commenters.each { |commenter| UserMailer.send_thank_you_note(@request_for_comment, commenter).deliver_now }
 
     respond_to do |format|
       if @request_for_comment.save
@@ -116,12 +134,13 @@ class RequestForCommentsController < ApplicationController
   end
 
   private
-    # Use callbacks to share common setup or constraints between actions.
+
+  # Use callbacks to share common setup or constraints between actions.
   def set_request_for_comment
     @request_for_comment = RequestForComment.find(params[:id])
   end
 
-    # Never trust parameters from the scary internet, only allow the white list through.
+  # Never trust parameters from the scary internet, only allow the white list through.
   def request_for_comment_params
     # The study_group_id might not be present in the session (e.g. for internal users), resulting in session[:study_group_id] = nil which is intended.
     params.require(:request_for_comment).permit(:exercise_id, :file_id, :question, :requested_at, :solved, :submission_id).merge(user_id: current_user.id, user_type: current_user.class.name)
@@ -133,6 +152,6 @@ class RequestForCommentsController < ApplicationController
     current_study_group = StudyGroup.find_by(id: session[:study_group_id])
     my_study_groups = current_user.study_groups.reject { |group| group == current_study_group }
     @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]]
+                              [t('request_for_comments.index.study_groups.my'), my_study_groups]]
   end
 end

app/policies/exercise_policy.rb

@@ -1,9 +1,11 @@
+# frozen_string_literal: true
+
 class ExercisePolicy < AdminOrAuthorPolicy
   def batch_update?
     admin?
   end
 
-  [:show?, :feedback?, :requests_for_comments?, :statistics?].each do |action|
+  %i[show? feedback? statistics? get_rfcs_for_exercise?].each do |action|
     define_method(action) { admin? || teacher_in_study_group? || teacher? && @record.public? || author? }
   end
 
@@ -19,15 +21,15 @@ class ExercisePolicy < AdminOrAuthorPolicy
     admin?
   end
 
-  [:clone?, :destroy?, :edit?, :update?].each do |action|
+  %i[clone? destroy? edit? update?].each do |action|
     define_method(action) { admin? || teacher_in_study_group? || author? }
   end
 
-  [:export_external_check?, :export_external_confirm?].each do |action|
+  %i[export_external_check? export_external_confirm?].each do |action|
     define_method(action) { (admin? || teacher_in_study_group? || author?) && @user.codeharbor_link }
   end
 
-  [:implement?, :working_times?, :intervention?, :search?, :submit?, :reload?].each do |action|
+  %i[implement? working_times? intervention? search? submit? reload?].each do |action|
     define_method(action) { everyone }
   end
 

app/policies/request_for_comment_policy.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class RequestForCommentPolicy < ApplicationPolicy
   def create?
     everyone

app/views/exercises/index.html.slim

@@ -45,7 +45,7 @@ h1 = Exercise.model_name.human(count: 2)
               ul.dropdown-menu.float-right role="menu"
                 li = link_to(t('shared.show'), exercise, 'data-turbolinks' => "false", class: 'dropdown-item') if policy(exercise).show?
                 li = link_to(t('activerecord.models.user_exercise_feedback.other'), feedback_exercise_path(exercise), class: 'dropdown-item') if policy(exercise).feedback?
-                li = link_to(t('activerecord.models.request_for_comment.other'), requests_for_comments_exercise_path(exercise), class: 'dropdown-item') if policy(exercise).requests_for_comments?
+                li = link_to(t('activerecord.models.request_for_comment.other'), rfcs_for_exercise_path(exercise), class: 'dropdown-item') if policy(exercise).get_rfcs_for_exercise?
                 li = link_to(t('shared.destroy'), exercise, data: {confirm: t('shared.confirm_destroy')}, method: :delete, class: 'dropdown-item') if policy(exercise).destroy?
                 li = link_to(t('.clone'), clone_exercise_path(exercise), data: {confirm: t('shared.confirm_destroy')}, method: :post, class: 'dropdown-item') if policy(exercise).clone?
                 li = link_to(t('exercises.export_codeharbor.label'), '', class: 'dropdown-item export-start', data: {'exercise-id' => exercise.id}) if policy(exercise).export_external_confirm?

app/views/exercises/show.html.slim

@@ -14,7 +14,7 @@ h1
       li = link_to(t('exercises.index.implement'), implement_exercise_path(@exercise), 'data-turbolinks' => "false", class: 'dropdown-item text-dark') if policy(@exercise).implement?
       li = link_to(t('shared.statistics'), statistics_exercise_path(@exercise), 'data-turbolinks' => "false", class: 'dropdown-item text-dark') if policy(@exercise).statistics?
       li = link_to(t('activerecord.models.user_exercise_feedback.other'), feedback_exercise_path(@exercise), class: 'dropdown-item text-dark') if policy(@exercise).feedback?
-      li = link_to(t('activerecord.models.request_for_comment.other'), requests_for_comments_exercise_path(@exercise), class: 'dropdown-item text-dark') if policy(@exercise).requests_for_comments?
+      li = link_to(t('activerecord.models.request_for_comment.other'), rfcs_for_exercise_path(@exercise), class: 'dropdown-item text-dark') if policy(@exercise).get_rfcs_for_exercise?
       li = link_to(t('shared.destroy'), @exercise, data: {confirm: t('shared.confirm_destroy')}, method: :delete, class: 'dropdown-item text-dark') if policy(@exercise).destroy?
       li = link_to(t('exercises.index.clone'), clone_exercise_path(@exercise), data: {confirm: t('shared.confirm_destroy')}, method: :post, class: 'dropdown-item text-dark') if policy(@exercise).clone?
       li = link_to(t('exercises.export_codeharbor.label'), '', class: 'dropdown-item export-start text-dark', data: {'exercise-id' => @exercise.id}) if policy(@exercise).export_external_confirm?

config/locales/de.yml

@@ -726,6 +726,7 @@ de:
       all: "Alle Kommentaranfragen"
       get_rfcs_with_my_comments: Kommentaranfragen die ich kommentiert habe
       get_my_rfc_activity: "Meine Kommentaraktivität"
+      get_rfcs_for_exercise: "Übungsspezifische Kommentare"
       study_groups:
         placeholder: "Lerngruppe"
         current: "Aktuelle Lerngruppe"

config/locales/en.yml

@@ -726,6 +726,7 @@ en:
       get_my_comment_requests: My Requests for Comments
       get_rfcs_with_my_comments: Requests for Comments I have commented on
       get_my_rfc_activity: "My Comment Activity"
+      get_rfcs_for_exercise: "Exercise Comments"
       study_groups:
         placeholder: "Study group"
         current: "Current Study Group"

config/routes.rb

@@ -1,4 +1,6 @@
-FILENAME_REGEXP = /[\w\.]+/ unless Kernel.const_defined?(:FILENAME_REGEXP)
+# frozen_string_literal: true
+
+FILENAME_REGEXP = /[\w.]+/.freeze unless Kernel.const_defined?(:FILENAME_REGEXP)
 
 Rails.application.routes.draw do
   resources :error_template_attributes
@@ -16,18 +18,19 @@ Rails.application.routes.draw do
   resources :codeharbor_links, only: %i[new create edit update destroy]
   resources :request_for_comments, except: %i[edit destroy] do
     member do
-      get :mark_as_solved, defaults: { format: :json }
+      get :mark_as_solved, defaults: {format: :json}
-      post :set_thank_you_note, defaults: { format: :json }
+      post :set_thank_you_note, defaults: {format: :json}
     end
   end
-  resources :comments, defaults: { format: :json }
+  resources :comments, defaults: {format: :json}
   get '/my_request_for_comments', as: 'my_request_for_comments', to: 'request_for_comments#get_my_comment_requests'
   get '/my_rfc_activity', as: 'my_rfc_activity', to: 'request_for_comments#get_rfcs_with_my_comments'
+  get '/exercises/:exercise_id/request_for_comments', as: 'rfcs_for_exercise', to: 'request_for_comments#get_rfcs_for_exercise'
 
   delete '/comment_by_id', to: 'comments#destroy_by_id'
-  put '/comments', to: 'comments#update', defaults: { format: :json }
+  put '/comments', to: 'comments#update', defaults: {format: :json}
 
-  resources :subscriptions, only: [:create, :destroy] do
+  resources :subscriptions, only: %i[create destroy] do
     member do
       get :unsubscribe, to: 'subscriptions#destroy'
     end
@@ -55,7 +58,6 @@ Rails.application.routes.draw do
     end
   end
 
-
   resources :consumers
 
   resources :execution_environments do
@@ -71,7 +73,7 @@ Rails.application.routes.draw do
 
   resources :exercises do
     collection do
-      match '', to: 'exercises#batch_update', via: [:patch, :put]
+      match '', to: 'exercises#batch_update', via: %i[patch put]
     end
 
     member do
@@ -82,7 +84,6 @@ Rails.application.routes.draw do
       post :search
       get :statistics
       get :feedback
-      get :requests_for_comments
       get :reload
       post :submit
       get 'study_group_dashboard/:study_group_id', to: 'exercises#study_group_dashboard'
@@ -108,9 +109,9 @@ Rails.application.routes.draw do
 
   resources :tips
 
-  resources :user_exercise_feedbacks, except: [:show, :index]
+  resources :user_exercise_feedbacks, except: %i[show index]
 
-  resources :external_users, only: [:index, :show], concerns: :statistics do
+  resources :external_users, only: %i[index show], concerns: :statistics do
     resources :exercises, concerns: :statistics
     member do
       get :tag_statistics
@@ -118,28 +119,28 @@ Rails.application.routes.draw do
   end
 
   namespace :code_ocean do
-    resources :files, only: [:create, :destroy]
+    resources :files, only: %i[create destroy]
   end
 
   resources :file_types
 
   resources :internal_users do
     member do
-      match 'activate', to: 'internal_users#activate', via: [:get, :patch, :put]
+      match 'activate', to: 'internal_users#activate', via: %i[get patch put]
-      match 'reset_password', to: 'internal_users#reset_password', via: [:get, :patch, :put]
+      match 'reset_password', to: 'internal_users#reset_password', via: %i[get patch put]
     end
   end
 
-  match '/forgot_password', as: 'forgot_password', to: 'internal_users#forgot_password', via: [:get, :post]
+  match '/forgot_password', as: 'forgot_password', to: 'internal_users#forgot_password', via: %i[get post]
 
-  resources :sessions, only: [:create, :destroy, :new]
+  resources :sessions, only: %i[create destroy new]
 
   post '/lti/launch', as: 'lti_launch', to: 'sessions#create_through_lti'
   get '/lti/return', as: 'lti_return', to: 'sessions#destroy_through_lti'
   get '/sign_in', as: 'sign_in', to: 'sessions#new'
-  match '/sign_out', as: 'sign_out', to: 'sessions#destroy', via: [:get, :delete]
+  match '/sign_out', as: 'sign_out', to: 'sessions#destroy', via: %i[get delete]
 
-  resources :submissions, only: [:create, :index, :show] do
+  resources :submissions, only: %i[create index show] do
     member do
       get 'download', as: :download, action: :download
       get 'download/:filename', as: :download_file, constraints: {filename: FILENAME_REGEXP}, action: :download_file
@@ -152,12 +153,12 @@ Rails.application.routes.draw do
     end
   end
 
-  resources :study_groups, only: [:index, :show, :edit, :destroy, :update]
+  resources :study_groups, only: %i[index show edit destroy update]
 
   resources :events, only: [:create]
 
-  post "/evaluate", to: 'remote_evaluation#evaluate', via: [:post]
+  post '/evaluate', to: 'remote_evaluation#evaluate', via: [:post]
-  post "/submit", to: 'remote_evaluation#submit', via: [:post]
+  post '/submit', to: 'remote_evaluation#submit', via: [:post]
 
   mount ActionCable.server => '/cable'
   mount RailsAdmin::Engine => '/rails_admin', as: 'rails_admin'

spec/controllers/request_for_comments_controller_spec.rb

@@ -24,7 +24,7 @@ describe RequestForCommentsController do
       rfc_other_study_group.user.update(study_groups: [another_study_group])
       rfc_other_study_group.submission.update(study_group: another_study_group)
 
-      get :index, params: {"q[submission_study_group_id_in][]": my_study_group.id}
+      get :index, params: { "q[submission_study_group_id_in][]": my_study_group.id }
 
       expect(assigns(:request_for_comments)).to eq([rfc_within_my_study_group])
     end
@@ -43,4 +43,14 @@ describe RequestForCommentsController do
     expect_status(200)
     expect_template(:index)
   end
+
+  describe 'GET #get_rfcs_for_exercise' do
+    before do
+      exercise = FactoryBot.create(:even_odd)
+      get :get_rfcs_for_exercise, params: { exercise_id: exercise.id }
+    end
+
+    expect_status(200)
+    expect_template(:index)
+  end
 end

spec/policies/exercise_policy_spec.rb

@@ -14,7 +14,7 @@ let(:exercise) { FactoryBot.build(:dummy, public: true) }
     end
   end
 
-  [:create?, :index?, :new?, :statistics?, :feedback?, :requests_for_comments?].each do |action|
+  [:create?, :index?, :new?, :statistics?, :feedback?, :get_rfcs_for_exercise?].each do |action|
     permissions(action) do
       it 'grants access to admins' do
         expect(subject).to permit(FactoryBot.build(:admin), exercise)