Merge pull request #134 from openHPI/user_exercise_feedback_backend

User exercise feedback backend UI

app/controllers/exercises_controller.rb

@@ -24,7 +24,6 @@ class ExercisesController < ApplicationController
     3
   end
 
-
   def java_course_token
     "702cbd2a-c84c-4b37-923a-692d7d1532d0"
   end
@@ -387,10 +386,13 @@ class ExercisesController < ApplicationController
       # 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.)
       # redirect 10 percent pseudorandomly to the feedback page
       if current_user.respond_to? :external_id
-        if ((current_user.id + @submission.exercise.created_at.to_i) % 10 == 1)
+        if @submission.redirect_to_feedback?
           redirect_to_user_feedback
           return
-        elsif rfc = RequestForComment.unsolved.where(exercise_id: @submission.exercise, user_id: current_user.id).first
+        end
+
+        rfc = @submission.own_unsolved_rfc
+        if rfc
           # set a message that informs the user that his own RFC should be closed.
           flash[:notice] = I18n.t('exercises.submit.full_score_redirect_to_own_rfc')
           flash.keep(:notice)
@@ -400,9 +402,11 @@ class ExercisesController < ApplicationController
             format.json { render(json: {redirect: url_for(rfc)}) }
           end
           return
+        end
 
         # else: show open rfc for same exercise if available
-        elsif rfc = RequestForComment.unsolved.where(exercise_id: @submission.exercise).where.not(question: nil).order("RANDOM()").find { | rfc_element |(rfc_element.comments_count < 5) }
+        rfc = @submission.unsolved_rfc
+        unless rfc.nil?
           # set a message that informs the user that his score was perfect and help in RFC is greatly appreciated.
           flash[:notice] = I18n.t('exercises.submit.full_score_redirect_to_rfc')
           flash.keep(:notice)
@@ -416,7 +420,11 @@ class ExercisesController < ApplicationController
       end
     else
       # redirect to feedback page if score is less than 100 percent
+       if @exercise.needs_more_feedback?
          redirect_to_user_feedback
+       else
+         redirect_to_lti_return_path
+       end
        return
     end
     redirect_to_lti_return_path

app/controllers/user_exercise_feedbacks_controller.rb

@@ -2,6 +2,7 @@ class UserExerciseFeedbacksController < ApplicationController
   include CommonBehavior
 
   before_action :set_user_exercise_feedback, only: [:edit, :update]
+  before_action :set_user_exercise_feedback_by_id, only: [:show, :destroy]
 
   def comment_presets
     [[0,t('user_exercise_feedback.difficulty_easy')],
@@ -19,10 +20,15 @@ class UserExerciseFeedbacksController < ApplicationController
      [4,t('user_exercise_feedback.estimated_time_more_30')]]
   end
 
-  def authorize!
+  def index
-    authorize(@uef)
+    @search = UserExerciseFeedback.all.search params[:q]
+    @uefs = @search.result.includes(:execution_environment).order(:id).paginate(page: params[:page])
+    authorize!
+  end
+
+  def show
+    authorize!
   end
-  private :authorize!
 
   def create
     @exercise = Exercise.find(uef_params[:exercise_id])
@@ -49,7 +55,8 @@ class UserExerciseFeedbacksController < ApplicationController
   end
 
   def destroy
-    destroy_and_respond(object: @tag)
+    authorize!
+    destroy_and_respond(object: @uef)
   end
 
   def edit
@@ -58,11 +65,6 @@ class UserExerciseFeedbacksController < ApplicationController
     authorize!
   end
 
-  def uef_params
-    params[:user_exercise_feedback].permit(:feedback_text, :difficulty, :exercise_id, :user_estimated_worktime).merge(user_id: current_user.id, user_type: current_user.class.name)
-  end
-  private :uef_params
-
   def new
     @texts = comment_presets.to_a
     @times = time_presets.to_a
@@ -89,6 +91,12 @@ class UserExerciseFeedbacksController < ApplicationController
     end
   end
 
+  private
+
+  def authorize!
+    authorize(@uef || @uefs)
+  end
+
   def to_s
     name
   end
@@ -98,6 +106,14 @@ class UserExerciseFeedbacksController < ApplicationController
     @uef = UserExerciseFeedback.find_by(exercise_id: params[:user_exercise_feedback][:exercise_id], user: current_user)
   end
 
+  def set_user_exercise_feedback_by_id
+    @uef = UserExerciseFeedback.find(params[:id])
+  end
+
+  def uef_params
+    params[:user_exercise_feedback].permit(:feedback_text, :difficulty, :exercise_id, :user_estimated_worktime).merge(user_id: current_user.id, user_type: current_user.class.name)
+  end
+
   def validate_inputs(uef_params)
     begin
       if uef_params[:difficulty].to_i < 0 || uef_params[:difficulty].to_i >= comment_presets.size

app/models/exercise.rb

@@ -37,6 +37,8 @@ class Exercise < ActiveRecord::Base
 
   @working_time_statistics = nil
 
+  MAX_EXERCISE_FEEDBACKS = 20
+
 
   def average_percentage
     if average_score and maximum_score != 0.0 and submissions.exists?(cause: 'submit')
@@ -362,4 +364,8 @@ class Exercise < ActiveRecord::Base
   end
   private :valid_main_file?
 
+  def needs_more_feedback
+    user_exercise_feedbacks.size <= MAX_EXERCISE_FEEDBACKS
+  end
+
 end

app/models/submission.rb

@@ -18,6 +18,8 @@ class Submission < ActiveRecord::Base
   validates :cause, inclusion: {in: CAUSES}
   validates :exercise_id, presence: true
 
+  MAX_COMMENTS_ON_RECOMMENDED_RFC = 5
+
   def build_files_hash(files, attribute)
     files.map(&attribute.to_proc).zip(files).to_h
   end
@@ -53,4 +55,16 @@ class Submission < ActiveRecord::Base
   def to_s
     Submission.model_name.human
   end
+
+  def redirect_to_feedback?
+    ((user_id + exercise.created_at.to_i) % 10 == 1) && exercise.needs_more_feedback
+  end
+
+  def own_unsolved_rfc
+    RequestForComment.unsolved.where(exercise_id: exercise, user_id: user_id).first
+  end
+
+  def unsolved_rfc
+    RequestForComment.unsolved.where(exercise_id: exercise).where.not(question: nil).order("RANDOM()").find { | rfc_element |(rfc_element.comments_count < MAX_COMMENTS_ON_RECOMMENDED_RFC) }
+  end
 end

app/models/user_exercise_feedback.rb

@@ -2,6 +2,7 @@ class UserExerciseFeedback < ActiveRecord::Base
   include Creation
 
   belongs_to :exercise
+  has_one :execution_environment, through: :exercise
 
   validates :user_id, uniqueness: { scope: [:exercise_id, :user_type] }
 

app/policies/user_exercise_feedback_policy.rb

@@ -1,8 +1,4 @@
-class UserExerciseFeedbackPolicy < ApplicationPolicy
+class UserExerciseFeedbackPolicy < AdminOrAuthorPolicy
-  def author?
-    @user == @record.author
-  end
-  private :author?
 
   def create?
     everyone
@@ -12,8 +8,4 @@ class UserExerciseFeedbackPolicy < ApplicationPolicy
     everyone
   end
 
-  [:show? ,:destroy?, :edit?, :update?].each do |action|
-    define_method(action) { admin? || author?}
-  end
-
 end

app/views/application/_navigation.html.slim

@@ -8,7 +8,7 @@
         - if current_user.admin?
           li = link_to(t('breadcrumbs.dashboard.show'), admin_dashboard_path)
           li.divider
-        - models = [ExecutionEnvironment, Exercise, ExerciseCollection, ProxyExercise, Tag, Consumer, CodeHarborLink,
+        - models = [ExecutionEnvironment, Exercise, ExerciseCollection, ProxyExercise, Tag, Consumer, CodeHarborLink, UserExerciseFeedback,
                 ErrorTemplate, ErrorTemplateAttribute, ExternalUser, FileType, FileTemplate, InternalUser].sort_by {|model| model.model_name.human(count: 2) }
         - models.each do |model|
           - if policy(model).index?

app/views/user_exercise_feedbacks/index.html.slim

@@ -0,0 +1,27 @@
+h1 = UserExerciseFeedback.model_name.human(count: 2)
+
+= render(layout: 'shared/form_filters') do |f|
+  .form-group
+    = f.label(:execution_environment_id_eq, t('activerecord.attributes.exercise.execution_environment'), class: 'sr-only')
+    = f.collection_select(:execution_environment_id_eq, ExecutionEnvironment.with_exercises, :id, :name, class: 'form-control', prompt: t('activerecord.attributes.exercise.execution_environment'))
+  .form-group
+    = f.label(:exercise_title_cont, t('activerecord.attributes.request_for_comments.exercise'), class: 'sr-only')
+    = f.search_field(:exercise_title_cont, class: 'form-control', placeholder: t('activerecord.attributes.request_for_comments.exercise'))
+
+.table-responsive
+  table.table
+    thead
+      tr
+        th colspan=2 = t('activerecord.attributes.user_exercise_feedback.user')
+        th = t('activerecord.attributes.user_exercise_feedback.exercise')
+        th colspan=2 = t('shared.actions')
+    tbody
+      - @uefs.each do |uef|
+        tr
+          td = uef.user.id
+          td = uef.user.name
+          td = link_to(uef.exercise.title, uef.exercise)
+          td = link_to(t('shared.show'), uef)
+          td = link_to(t('shared.destroy'), uef, data: {confirm: t('shared.confirm_destroy')}, method: :delete)
+
+= render('shared/pagination', collection: @uefs)

app/views/user_exercise_feedbacks/show.html.slim

@@ -0,0 +1,7 @@
+h2 = @uef
+
+= row(label: 'activerecord.attributes.user_exercise_feedback.exercise', value: link_to(@uef.exercise.title, @uef.exercise))
+= row(label: 'user_exercise_feedback.user', value: @uef.user)
+= row(label: 'activerecord.attributes.user_exercise_feedback.feedback_text', value: @uef.feedback_text)
+= row(label: 'user_exercise_feedback.difficulty', value: @uef.difficulty)
+= row(label: 'user_exercise_feedback.working_time', value: @uef.user_estimated_worktime)

config/locales/de.yml

@@ -126,6 +126,10 @@ de:
         name: "Name"
         updated_at: "Letzte Änderung"
         exercises: "Aufgaben"
+      user_exercise_feedback:
+        user: "Autor"
+        exercise: "Aufgabe"
+        feedback_text: "Feedback Text"
     models:
       code_harbor_link:
         one: CodeHarbor-Link

config/locales/en.yml

@@ -126,6 +126,10 @@ en:
         name: "Name"
         updated_at: "Last Update"
         exercises: "Exercises"
+      user_exercise_feedback:
+        user: "Author"
+        exercise: "Exercise"
+        feedback_text: "Feedback Text"
     models:
       code_harbor_link:
         one: CodeHarbor Link

spec/factories/exercise.rb

@@ -38,6 +38,24 @@ FactoryGirl.define do
     association :execution_environment, factory: :ruby
     instructions
     title 'Dummy'
+
+    factory :dummy_with_user_feedbacks do
+      # user_feedbacks_count is declared as a transient attribute and available in
+      # attributes on the factory, as well as the callback via the evaluator
+      transient do
+        user_feedbacks_count 5
+      end
+
+      # the after(:create) yields two values; the exercise instance itself and the
+      # evaluator, which stores all values from the factory, including transient
+      # attributes; `create_list`'s second argument is the number of records
+      # to create and we make sure the user_exercise_feedback is associated properly to the exercise
+      after(:create) do |exercise, evaluator|
+        create_list(:user_exercise_feedback, evaluator.user_feedbacks_count, exercise: exercise)
+      end
+
+    end
+
   end
 
   factory :even_odd, class: Exercise do

spec/factories/user_exercise_feedback.rb

@@ -0,0 +1,7 @@
+FactoryGirl.define do
+  factory :user_exercise_feedback, class: UserExerciseFeedback do
+    created_by_external_user
+    feedback_text 'Most suitable exercise ever'
+  end
+
+end

spec/models/submission_spec.rb

@@ -85,4 +85,53 @@ describe Submission do
       expect(submission.to_s).to eq(described_class.model_name.human)
     end
   end
+
+  describe '#redirect_to_feedback?' do
+
+    context 'with no exercise feedback' do
+      let(:exercise) {FactoryGirl.create(:dummy)}
+      let(:user) {FactoryGirl.build(:external_user, id: (11 - exercise.created_at.to_i % 10) % 10)}
+      let(:submission) {FactoryGirl.build(:submission, exercise: exercise, user: user)}
+
+      it 'sends 10% of users to feedback page' do
+        expect(submission.send(:redirect_to_feedback?)).to be_truthy
+      end
+
+      it 'does not redirect other users' do
+        9.times do |i|
+          submission = FactoryGirl.build(:submission, exercise: exercise, user: FactoryGirl.build(:external_user, id: (11 - exercise.created_at.to_i % 10) - i - 1))
+          expect(submission.send(:redirect_to_feedback?)).to be_falsey
+        end
+      end
+    end
+
+    context 'with little exercise feedback' do
+      let(:exercise) {FactoryGirl.create(:dummy_with_user_feedbacks)}
+      let(:user) {FactoryGirl.build(:external_user, id: (11 - exercise.created_at.to_i % 10) % 10)}
+      let(:submission) {FactoryGirl.build(:submission, exercise: exercise, user: user)}
+
+      it 'sends 10% of users to feedback page' do
+        expect(submission.send(:redirect_to_feedback?)).to be_truthy
+      end
+
+      it 'does not redirect other users' do
+        9.times do |i|
+          submission = FactoryGirl.build(:submission, exercise: exercise, user: FactoryGirl.build(:external_user, id: (11 - exercise.created_at.to_i % 10) - i - 1))
+          expect(submission.send(:redirect_to_feedback?)).to be_falsey
+        end
+      end
+    end
+
+    context 'with enough exercise feedback' do
+      let(:exercise) {FactoryGirl.create(:dummy_with_user_feedbacks, user_feedbacks_count: 42)}
+      let(:user) {FactoryGirl.create(:external_user)}
+
+      it 'sends nobody to feedback page' do
+        30.times do |i|
+          submission = FactoryGirl.create(:submission, exercise: exercise, user: FactoryGirl.create(:external_user))
+          expect(submission.send(:redirect_to_feedback?)).to be_falsey
+        end
+      end
+    end
+  end
 end