diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index 969e5e4f..94678315 100644 --- a/app/controllers/exercises_controller.rb +++ b/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,24 +402,30 @@ 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) respond_to do |format| - format.html { redirect_to(rfc) } - format.json { render(json: {redirect: url_for(rfc)}) } + format.html {redirect_to(rfc)} + format.json {render(json: {redirect: url_for(rfc)})} end return end end else # redirect to feedback page if score is less than 100 percent - redirect_to_user_feedback - return + if @exercise.needs_more_feedback? + redirect_to_user_feedback + else + redirect_to_lti_return_path + end + return end redirect_to_lti_return_path end diff --git a/app/controllers/user_exercise_feedbacks_controller.rb b/app/controllers/user_exercise_feedbacks_controller.rb index 0d1e1925..8abffd66 100644 --- a/app/controllers/user_exercise_feedbacks_controller.rb +++ b/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! - authorize(@uef) + def index + @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 @@ -112,4 +128,4 @@ class UserExerciseFeedbacksController < ApplicationController end end -end \ No newline at end of file +end diff --git a/app/models/exercise.rb b/app/models/exercise.rb index b7487399..06ed391f 100644 --- a/app/models/exercise.rb +++ b/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 diff --git a/app/models/submission.rb b/app/models/submission.rb index 69a48307..38ea16d6 100644 --- a/app/models/submission.rb +++ b/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 diff --git a/app/models/user_exercise_feedback.rb b/app/models/user_exercise_feedback.rb index 78d84972..5694573b 100644 --- a/app/models/user_exercise_feedback.rb +++ b/app/models/user_exercise_feedback.rb @@ -2,10 +2,11 @@ class UserExerciseFeedback < ActiveRecord::Base include Creation belongs_to :exercise + has_one :execution_environment, through: :exercise validates :user_id, uniqueness: { scope: [:exercise_id, :user_type] } def to_s "User Exercise Feedback" end -end \ No newline at end of file +end diff --git a/app/policies/user_exercise_feedback_policy.rb b/app/policies/user_exercise_feedback_policy.rb index 20a89a6e..a570a28c 100644 --- a/app/policies/user_exercise_feedback_policy.rb +++ b/app/policies/user_exercise_feedback_policy.rb @@ -1,8 +1,4 @@ -class UserExerciseFeedbackPolicy < ApplicationPolicy - def author? - @user == @record.author - end - private :author? +class UserExerciseFeedbackPolicy < AdminOrAuthorPolicy 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 diff --git a/app/views/application/_navigation.html.slim b/app/views/application/_navigation.html.slim index 668ccab4..a2604c7a 100644 --- a/app/views/application/_navigation.html.slim +++ b/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? diff --git a/app/views/user_exercise_feedbacks/index.html.slim b/app/views/user_exercise_feedbacks/index.html.slim new file mode 100644 index 00000000..52249229 --- /dev/null +++ b/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) diff --git a/app/views/user_exercise_feedbacks/show.html.slim b/app/views/user_exercise_feedbacks/show.html.slim new file mode 100644 index 00000000..2c53e47f --- /dev/null +++ b/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) diff --git a/config/locales/de.yml b/config/locales/de.yml index a7a2b436..41d1097b 100644 --- a/config/locales/de.yml +++ b/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 diff --git a/config/locales/en.yml b/config/locales/en.yml index 7eba1527..ec6cf155 100644 --- a/config/locales/en.yml +++ b/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 diff --git a/spec/factories/exercise.rb b/spec/factories/exercise.rb index ea588faf..9d8ac5b1 100644 --- a/spec/factories/exercise.rb +++ b/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 diff --git a/spec/factories/user_exercise_feedback.rb b/spec/factories/user_exercise_feedback.rb new file mode 100644 index 00000000..b5a376cd --- /dev/null +++ b/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 \ No newline at end of file diff --git a/spec/models/submission_spec.rb b/spec/models/submission_spec.rb index 64f4e49e..227c6b99 100644 --- a/spec/models/submission_spec.rb +++ b/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