From d353dbaf5ba7efe9d6beb44631d4ab3b50c73bac Mon Sep 17 00:00:00 2001 From: Maximilian Grundke Date: Sun, 15 Oct 2017 18:23:58 +0200 Subject: [PATCH 01/12] Implement index action. Repair destroy --- .../user_exercise_feedbacks_controller.rb | 37 +++++++++++++------ app/policies/user_exercise_feedback_policy.rb | 10 +---- .../user_exercise_feedbacks/index.html.slim | 19 ++++++++++ config/locales/de.yml | 3 ++ config/locales/en.yml | 3 ++ 5 files changed, 52 insertions(+), 20 deletions(-) create mode 100644 app/views/user_exercise_feedbacks/index.html.slim diff --git a/app/controllers/user_exercise_feedbacks_controller.rb b/app/controllers/user_exercise_feedbacks_controller.rb index 0d1e1925..6ce02da7 100644 --- a/app/controllers/user_exercise_feedbacks_controller.rb +++ b/app/controllers/user_exercise_feedbacks_controller.rb @@ -1,7 +1,8 @@ class UserExerciseFeedbacksController < ApplicationController include CommonBehavior - before_action :set_user_exercise_feedback, only: [:edit, :update] + before_action :set_user_exercise_feedback, only: [:show, :edit, :update] + before_action :set_user_exercise_feedback_by_id, only: [:destroy] def comment_presets [[0,t('user_exercise_feedback.difficulty_easy')], @@ -19,10 +20,14 @@ class UserExerciseFeedbacksController < ApplicationController [4,t('user_exercise_feedback.estimated_time_more_30')]] end - def authorize! - authorize(@uef) + def index + @uefs = UserExerciseFeedback.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 +54,8 @@ class UserExerciseFeedbacksController < ApplicationController end def destroy - destroy_and_respond(object: @tag) + authorize! + destroy_and_respond(object: @uef) end def edit @@ -58,11 +64,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 +90,12 @@ class UserExerciseFeedbacksController < ApplicationController end end + private + + def authorize! + authorize(@uef || @uefs) + end + def to_s name end @@ -98,6 +105,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 +127,4 @@ class UserExerciseFeedbacksController < ApplicationController end 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/user_exercise_feedbacks/index.html.slim b/app/views/user_exercise_feedbacks/index.html.slim new file mode 100644 index 00000000..84f2a15f --- /dev/null +++ b/app/views/user_exercise_feedbacks/index.html.slim @@ -0,0 +1,19 @@ +h1 = UserExerciseFeedback.model_name.human(count: 2) + +.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/config/locales/de.yml b/config/locales/de.yml index 1e2b2337..523259e6 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -116,6 +116,9 @@ de: name: "Name" updated_at: "Letzte Änderung" exercises: "Aufgaben" + user_exercise_feedback: + user: "Autor" + exercise: "Aufgabe" models: code_harbor_link: one: CodeHarbor-Link diff --git a/config/locales/en.yml b/config/locales/en.yml index c55c90b0..4f4e0768 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -116,6 +116,9 @@ en: name: "Name" updated_at: "Last Update" exercises: "Exercises" + user_exercise_feedback: + user: "Author" + exercise: "Exercise" models: code_harbor_link: one: CodeHarbor Link From 12b9365e6eb8b4e1865fa443f9d98bead9fa72e7 Mon Sep 17 00:00:00 2001 From: Maximilian Grundke Date: Sun, 15 Oct 2017 18:55:33 +0200 Subject: [PATCH 02/12] Implement show action --- app/controllers/user_exercise_feedbacks_controller.rb | 4 ++-- app/views/user_exercise_feedbacks/show.html.slim | 7 +++++++ config/locales/de.yml | 1 + config/locales/en.yml | 1 + 4 files changed, 11 insertions(+), 2 deletions(-) create mode 100644 app/views/user_exercise_feedbacks/show.html.slim diff --git a/app/controllers/user_exercise_feedbacks_controller.rb b/app/controllers/user_exercise_feedbacks_controller.rb index 6ce02da7..29b718e9 100644 --- a/app/controllers/user_exercise_feedbacks_controller.rb +++ b/app/controllers/user_exercise_feedbacks_controller.rb @@ -1,8 +1,8 @@ class UserExerciseFeedbacksController < ApplicationController include CommonBehavior - before_action :set_user_exercise_feedback, only: [:show, :edit, :update] - before_action :set_user_exercise_feedback_by_id, only: [:destroy] + 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')], 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 523259e6..5e111be0 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -119,6 +119,7 @@ de: 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 4f4e0768..e77242dc 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -119,6 +119,7 @@ en: user_exercise_feedback: user: "Author" exercise: "Exercise" + feedback_text: "Feedback Text" models: code_harbor_link: one: CodeHarbor Link From 74a4313949438856124853e4a25ed8d3b12846ce Mon Sep 17 00:00:00 2001 From: Maximilian Grundke Date: Mon, 16 Oct 2017 11:34:57 +0200 Subject: [PATCH 03/12] Add feedback to navigation --- app/views/application/_navigation.html.slim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/application/_navigation.html.slim b/app/views/application/_navigation.html.slim index 19f10a85..b65ead89 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, ExternalUser, FileType, FileTemplate, InternalUser].sort_by { |model| model.model_name.human(count: 2) } + - models = [ExecutionEnvironment, Exercise, ExerciseCollection, ProxyExercise, Tag, Consumer, CodeHarborLink, ExternalUser, FileType, FileTemplate, InternalUser, UserExerciseFeedback].sort_by { |model| model.model_name.human(count: 2) } - models.each do |model| - if policy(model).index? li = link_to(model.model_name.human(count: 2), send(:"#{model.model_name.collection}_path")) From d162f78b67a78ea8340148835751480eccb7ff33 Mon Sep 17 00:00:00 2001 From: Maximilian Grundke Date: Mon, 16 Oct 2017 11:45:23 +0200 Subject: [PATCH 04/12] Add filters to index page --- app/controllers/user_exercise_feedbacks_controller.rb | 3 ++- app/models/user_exercise_feedback.rb | 3 ++- app/views/user_exercise_feedbacks/index.html.slim | 8 ++++++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/controllers/user_exercise_feedbacks_controller.rb b/app/controllers/user_exercise_feedbacks_controller.rb index 29b718e9..8abffd66 100644 --- a/app/controllers/user_exercise_feedbacks_controller.rb +++ b/app/controllers/user_exercise_feedbacks_controller.rb @@ -21,7 +21,8 @@ class UserExerciseFeedbacksController < ApplicationController end def index - @uefs = UserExerciseFeedback.order(:id).paginate(page: params[:page]) + @search = UserExerciseFeedback.all.search params[:q] + @uefs = @search.result.includes(:execution_environment).order(:id).paginate(page: params[:page]) authorize! 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/views/user_exercise_feedbacks/index.html.slim b/app/views/user_exercise_feedbacks/index.html.slim index 84f2a15f..52249229 100644 --- a/app/views/user_exercise_feedbacks/index.html.slim +++ b/app/views/user_exercise_feedbacks/index.html.slim @@ -1,5 +1,13 @@ 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 From 539b2931c31d1597a41e500677175db2822b24df Mon Sep 17 00:00:00 2001 From: Ralf Teusner Date: Wed, 25 Oct 2017 10:38:27 +0200 Subject: [PATCH 05/12] changed behaviour of user exercise feedback (max 20 is collected), added some constant for the integers used. --- app/controllers/exercises_controller.rb | 31 ++++++++++++++++--------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index 141b47ee..77b7bd7e 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -24,6 +24,9 @@ class ExercisesController < ApplicationController 3 end + MAX_EXERCISE_FEEDBACKS = 20 + MAX_COMMENTS_PER_REQUEST = 5 + def java_course_token "702cbd2a-c84c-4b37-923a-692d7d1532d0" @@ -382,7 +385,7 @@ 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 (((current_user.id + @submission.exercise.created_at.to_i) % 10 == 1) && @exercise.user_exercise_feedbacks.size <= MAX_EXERCISE_FEEDBACKS) redirect_to_user_feedback return elsif rfc = RequestForComment.unsolved.where(exercise_id: @submission.exercise, user_id: current_user.id).first @@ -397,22 +400,28 @@ class ExercisesController < ApplicationController return # 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) } - # 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) + elsif rfc = RequestForComment.unsolved.where(exercise_id: @submission.exercise).where.not(question: nil).order("RANDOM()").find { | rfc_element |(rfc_element.comments_count < MAX_COMMENTS_PER_REQUEST) } + if(not 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)}) } + respond_to do |format| + format.html { redirect_to(rfc) } + format.json { render(json: {redirect: url_for(rfc)}) } + end + return end - return end end else # redirect to feedback page if score is less than 100 percent - redirect_to_user_feedback - return + if @exercise.user_exercise_feedbacks.size <= 50 + redirect_to_user_feedback + else + redirect_to_lti_return_path + end + return end redirect_to_lti_return_path end From 97d840955756cc8b8baf05353c8d5d5bbad584bc Mon Sep 17 00:00:00 2001 From: Ralf Teusner Date: Wed, 1 Nov 2017 10:14:52 +0100 Subject: [PATCH 06/12] add association between exercise and user_exercise_feedback --- app/models/exercise.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/exercise.rb b/app/models/exercise.rb index 58d328ae..b7487399 100644 --- a/app/models/exercise.rb +++ b/app/models/exercise.rb @@ -20,6 +20,7 @@ class Exercise < ActiveRecord::Base has_many :exercise_tags has_many :tags, through: :exercise_tags accepts_nested_attributes_for :exercise_tags + has_many :user_exercise_feedbacks has_many :external_users, source: :user, source_type: ExternalUser, through: :submissions has_many :internal_users, source: :user, source_type: InternalUser, through: :submissions From ac14e2d0ca2e41eb4a03ed5add5f75e43a78b054 Mon Sep 17 00:00:00 2001 From: Ralf Teusner Date: Wed, 1 Nov 2017 11:57:56 +0100 Subject: [PATCH 07/12] first steps towards testing the redirect with regards to user_feedbacks --- spec/controllers/exercises_controller_spec.rb | 26 +++++++++++++++++++ spec/factories/exercise.rb | 18 +++++++++++++ spec/factories/user_exercise_feedback.rb | 7 +++++ 3 files changed, 51 insertions(+) create mode 100644 spec/factories/user_exercise_feedback.rb diff --git a/spec/controllers/exercises_controller_spec.rb b/spec/controllers/exercises_controller_spec.rb index 43dada43..dca3103b 100644 --- a/spec/controllers/exercises_controller_spec.rb +++ b/spec/controllers/exercises_controller_spec.rb @@ -313,4 +313,30 @@ describe ExercisesController do expect_template(:edit) end end + + describe 'redirect after submit' do + + let!(:submission_with_created_at_to_receive_feedback) { FactoryGirl.create(:submission, exercise_id: exercise.id, user_id: user.id, user_type: user.class.name, created_at: (10 - (user.id % 10))) } + + let(:exercise_with_user_feedbacks) { FactoryGirl.build(:dummy_with_user_feedbacks, submission: submission_with_created_at_to_receive_feedback) } + let(:exercise_with_21_user_feedbacks) { FactoryGirl.build(:dummy_with_user_feedbacks, user_exercise_feedback_count: 21) } + + + + it 'with less than maximal user feedbacks' do + exercise_with_user_feedbacks.send(:redirect_after_submit) + expect_redirect(new_user_exercise_feedback_path(user_exercise_feedback: {exercise_id: @exercise.id})) + end + + it 'with more than maximal user feedbacks' do + exercise_with_user_feedbacks.send(:redirect_after_submit) + expect_redirect(new_user_exercise_feedback_path(user_exercise_feedback: {exercise_id: @exercise.id})) + end + + + + expect_redirect(Exercise.last) + + end + end 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 From 3608712706c8ef0c503c09c1deee4967506e1f4b Mon Sep 17 00:00:00 2001 From: Ralf Teusner Date: Mon, 6 Nov 2017 15:38:50 +0100 Subject: [PATCH 08/12] forgot some minor changes in spec.. (still work in progress) --- spec/controllers/exercises_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/controllers/exercises_controller_spec.rb b/spec/controllers/exercises_controller_spec.rb index dca3103b..b509a33f 100644 --- a/spec/controllers/exercises_controller_spec.rb +++ b/spec/controllers/exercises_controller_spec.rb @@ -316,10 +316,10 @@ describe ExercisesController do describe 'redirect after submit' do - let!(:submission_with_created_at_to_receive_feedback) { FactoryGirl.create(:submission, exercise_id: exercise.id, user_id: user.id, user_type: user.class.name, created_at: (10 - (user.id % 10))) } + let!(:submission_with_created_at_to_receive_feedback) { FactoryGirl.create(:submission, exercise_id: exercise.id, user_id: user.id, user_type: user.class.name, created_at: (11 - (user.id % 10))) } let(:exercise_with_user_feedbacks) { FactoryGirl.build(:dummy_with_user_feedbacks, submission: submission_with_created_at_to_receive_feedback) } - let(:exercise_with_21_user_feedbacks) { FactoryGirl.build(:dummy_with_user_feedbacks, user_exercise_feedback_count: 21) } + let(:exercise_with_21_user_feedbacks) { FactoryGirl.build(:dummy_with_user_feedbacks, user_exercise_feedback_count: 21, submission: submission_with_created_at_to_receive_feedback)) } From c4cf11f29944074ab260be2d913ad7bac2e391ff Mon Sep 17 00:00:00 2001 From: Maximilian Grundke Date: Wed, 8 Nov 2017 15:39:04 +0100 Subject: [PATCH 09/12] Refactor user redirect after submission --- app/controllers/exercises_controller.rb | 33 ++++++++++++------------- app/models/exercise.rb | 6 +++++ app/models/submission.rb | 14 +++++++++++ 3 files changed, 36 insertions(+), 17 deletions(-) diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index 77b7bd7e..148f0178 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -24,10 +24,6 @@ class ExercisesController < ApplicationController 3 end - MAX_EXERCISE_FEEDBACKS = 20 - MAX_COMMENTS_PER_REQUEST = 5 - - def java_course_token "702cbd2a-c84c-4b37-923a-692d7d1532d0" end @@ -380,15 +376,18 @@ class ExercisesController < ApplicationController def redirect_after_submit Rails.logger.debug('Redirecting user with score:s ' + @submission.normalized_score.to_s) - if @submission.normalized_score == 1.0 + 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.) # 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) && @exercise.user_exercise_feedbacks.size <= MAX_EXERCISE_FEEDBACKS) + 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) @@ -398,20 +397,20 @@ 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 < MAX_COMMENTS_PER_REQUEST) } - if(not 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) + 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)}) } - end - return + respond_to do |format| + format.html {redirect_to(rfc)} + format.json {render(json: {redirect: url_for(rfc)})} end + return end end else 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 From 03141409e8bbeb6716248ebc5ae7275665a0163d Mon Sep 17 00:00:00 2001 From: Maximilian Grundke Date: Wed, 8 Nov 2017 15:41:48 +0100 Subject: [PATCH 10/12] Fix tests for user_exercise_feedback --- spec/controllers/exercises_controller_spec.rb | 26 ---------- spec/models/submission_spec.rb | 49 +++++++++++++++++++ 2 files changed, 49 insertions(+), 26 deletions(-) diff --git a/spec/controllers/exercises_controller_spec.rb b/spec/controllers/exercises_controller_spec.rb index b509a33f..43dada43 100644 --- a/spec/controllers/exercises_controller_spec.rb +++ b/spec/controllers/exercises_controller_spec.rb @@ -313,30 +313,4 @@ describe ExercisesController do expect_template(:edit) end end - - describe 'redirect after submit' do - - let!(:submission_with_created_at_to_receive_feedback) { FactoryGirl.create(:submission, exercise_id: exercise.id, user_id: user.id, user_type: user.class.name, created_at: (11 - (user.id % 10))) } - - let(:exercise_with_user_feedbacks) { FactoryGirl.build(:dummy_with_user_feedbacks, submission: submission_with_created_at_to_receive_feedback) } - let(:exercise_with_21_user_feedbacks) { FactoryGirl.build(:dummy_with_user_feedbacks, user_exercise_feedback_count: 21, submission: submission_with_created_at_to_receive_feedback)) } - - - - it 'with less than maximal user feedbacks' do - exercise_with_user_feedbacks.send(:redirect_after_submit) - expect_redirect(new_user_exercise_feedback_path(user_exercise_feedback: {exercise_id: @exercise.id})) - end - - it 'with more than maximal user feedbacks' do - exercise_with_user_feedbacks.send(:redirect_after_submit) - expect_redirect(new_user_exercise_feedback_path(user_exercise_feedback: {exercise_id: @exercise.id})) - end - - - - expect_redirect(Exercise.last) - - end - end 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 From 69250901a1f57ddf654f1bc43e031dd6b6ef36f4 Mon Sep 17 00:00:00 2001 From: Maximilian Grundke Date: Wed, 8 Nov 2017 21:24:46 +0100 Subject: [PATCH 11/12] Fix typo --- app/controllers/exercises_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index 148f0178..92bc5b97 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -376,7 +376,7 @@ class ExercisesController < ApplicationController def redirect_after_submit Rails.logger.debug('Redirecting user with score:s ' + @submission.normalized_score.to_s) - if submission.normalized_score == 1.0 + 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.) # redirect 10 percent pseudorandomly to the feedback page From 6161d6caaf1aba8a5c908bf17540e9697c1a0162 Mon Sep 17 00:00:00 2001 From: Ralf Teusner Date: Wed, 15 Nov 2017 14:17:55 +0100 Subject: [PATCH 12/12] clean up code, use method instead of magic constant. --- app/controllers/exercises_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index 92bc5b97..85ef3dbf 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -415,7 +415,7 @@ class ExercisesController < ApplicationController end else # redirect to feedback page if score is less than 100 percent - if @exercise.user_exercise_feedbacks.size <= 50 + if @exercise.needs_more_feedback? redirect_to_user_feedback else redirect_to_lti_return_path