Properly nest UserExerciseFeedback

Fixes CODEOCEAN-108

app/controllers/concerns/redirect_behavior.rb

@@ -61,9 +61,9 @@ module RedirectBehavior
   def redirect_to_user_feedback
     uef = UserExerciseFeedback.find_by(exercise: @submission.exercise, user: current_user)
     url = if uef
-            edit_user_exercise_feedback_path(uef)
+            edit_exercise_user_exercise_feedback_path(uef, exercise_id: @submission.exercise)
           else
-            new_user_exercise_feedback_path(user_exercise_feedback: {exercise_id: @submission.exercise.id})
+            new_exercise_user_exercise_feedback_path(exercise_id: @submission.exercise)
           end
 
     respond_to do |format|

app/controllers/user_exercise_feedbacks_controller.rb

@@ -3,6 +3,7 @@
 class UserExerciseFeedbacksController < ApplicationController
   include CommonBehavior
 
+  before_action :set_exercise_and_authorize
   before_action :set_user_exercise_feedback, only: %i[edit update destroy]
   before_action :set_presets, only: %i[new edit create update]
 
@@ -23,24 +24,15 @@ class UserExerciseFeedbacksController < ApplicationController
   end
 
   def new
-    exercise_id = if params[:user_exercise_feedback].nil?
-                    params[:exercise_id]
-                  else
-                    params[:user_exercise_feedback][:exercise_id]
-                  end
-    @exercise = Exercise.find(exercise_id)
     @uef = UserExerciseFeedback.find_or_initialize_by(user: current_user, exercise: @exercise)
     authorize!
   end
 
-  def edit
+  def edit; end
-    authorize!
-  end
 
   def create
     Sentry.set_extras(params: uef_params)
 
-    @exercise = Exercise.find(uef_params[:exercise_id])
     rfc = RequestForComment.unsolved.where(exercise: @exercise, user: current_user).first
     submission = begin
       current_contributor.submissions.where(exercise: @exercise).order(created_at: :desc).first
@@ -48,9 +40,8 @@ class UserExerciseFeedbacksController < ApplicationController
       nil
     end
 
-    if @exercise
     @uef = UserExerciseFeedback.find_or_initialize_by(user: current_user, exercise: @exercise)
-      @uef.update(uef_params)
+    @uef.assign_attributes(uef_params)
     authorize!
     if validate_inputs(uef_params)
       path =
@@ -62,8 +53,7 @@ class UserExerciseFeedbacksController < ApplicationController
       create_and_respond(object: @uef, path: proc { path })
     else
       flash.now[:danger] = t('shared.message_failure')
-        redirect_back fallback_location: user_exercise_feedback_path(@uef)
+      redirect_back fallback_location: exercise_user_exercise_feedback_path(@uef)
-      end
     end
   end
 
@@ -75,7 +65,7 @@ class UserExerciseFeedbacksController < ApplicationController
     end
     rfc = RequestForComment.unsolved.where(exercise: @exercise, user: current_user).first
     authorize!
-    if @exercise && validate_inputs(uef_params)
+    if validate_inputs(uef_params)
       path =
         if rfc && submission && submission.normalized_score.to_d == BigDecimal('1.0')
           request_for_comment_path(rfc)
@@ -85,7 +75,7 @@ class UserExerciseFeedbacksController < ApplicationController
       update_and_respond(object: @uef, params: uef_params, path:)
     else
       flash.now[:danger] = t('shared.message_failure')
-      redirect_back fallback_location: user_exercise_feedback_path(@uef)
+      redirect_back fallback_location: exercise_user_exercise_feedback_path(@uef)
     end
   end
 
@@ -97,16 +87,19 @@ class UserExerciseFeedbacksController < ApplicationController
   private
 
   def authorize!
-    authorize(@uef || @uefs)
+    raise Pundit::NotAuthorizedError if @uef.present? && @uef.exercise != @exercise
+
+    authorize(@uef)
   end
 
-  def to_s
+  def set_exercise_and_authorize
-    name
+    @exercise = Exercise.find(params[:exercise_id])
+    authorize(@exercise, :implement?)
   end
 
   def set_user_exercise_feedback
     @uef = UserExerciseFeedback.find(params[:id])
-    @exercise = @uef.exercise
+    authorize!
   end
 
   def set_presets

app/models/user_exercise_feedback.rb

@@ -22,4 +22,8 @@ class UserExerciseFeedback < ApplicationRecord
       .order(created_at: :desc)
       .first
   end
+
+  def self.parent_resource
+    Exercise
+  end
 end

app/policies/user_exercise_feedback_policy.rb

@@ -8,4 +8,9 @@ class UserExerciseFeedbackPolicy < AdminOrAuthorPolicy
   def new?
     everyone
   end
+
+  def show?
+    # We don't have a show action, so no one can show a UserExerciseFeedback directly.
+    no_one
+  end
 end

app/views/user_exercise_feedbacks/_form.html.slim

@@ -1,4 +1,4 @@
-= form_for(@uef) do |f|
+= form_for([@exercise, @uef]) do |f|
   div
     h1 id="exercise-headline"
       = t('activerecord.models.user_exercise_feedback.one') + " " + @exercise.title

config/routes.rb

@@ -89,7 +89,7 @@ Rails.application.routes.draw do
       get :working_times
       post :intervention
       get :statistics
-      get :feedback
+      get :feedback, path: 'feedbacks'
       get :reload
       get 'study_group_dashboard/:study_group_id', to: 'exercises#study_group_dashboard'
       post :export_external_check
@@ -97,6 +97,7 @@ Rails.application.routes.draw do
     end
 
     resources :programming_groups
+    resources :user_exercise_feedbacks, except: %i[show index], path: 'feedbacks'
   end
 
   resources :programming_groups, except: %i[new create]
@@ -118,8 +119,6 @@ Rails.application.routes.draw do
 
   resources :tips
 
-  resources :user_exercise_feedbacks, except: %i[show index]
-
   resources :external_users, only: %i[index show], concerns: :statistics do
     resources :exercises do
       get :statistics, to: 'exercises#external_user_statistics', on: :member

spec/controllers/submissions_controller_spec.rb

@@ -144,13 +144,13 @@ RSpec.describe SubmissionsController do
         context 'without any previous feedback' do
           let(:uef) { nil }
 
-          expect_redirect { new_user_exercise_feedback_path(user_exercise_feedback: {exercise_id: submission.exercise.id}) }
+          expect_redirect { new_exercise_user_exercise_feedback_path(exercise_id: submission.exercise) }
         end
 
         context 'with a previous feedback for the same exercise' do
           let(:uef) { create(:user_exercise_feedback, exercise:, user: current_user) }
 
-          expect_redirect { edit_user_exercise_feedback_path(uef) }
+          expect_redirect { edit_exercise_user_exercise_feedback_path(uef, exercise_id: exercise) }
         end
       end