From c32e7586cd8843772c524b3db96b6cbf11f4af52 Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Wed, 25 Nov 2020 21:19:18 +0100 Subject: [PATCH] Remove consumer_id from session * Each external (and also internal) user has a consumer attached. We don't need that information twice. --- app/controllers/concerns/lti.rb | 7 ++---- app/controllers/exercises_controller.rb | 22 +++++++++---------- .../remote_evaluation_controller.rb | 2 +- app/controllers/sessions_controller.rb | 4 ++-- app/helpers/lti_helper.rb | 9 ++++---- app/views/exercises/_editor_output.html.slim | 2 +- spec/concerns/lti_spec.rb | 7 ++---- spec/controllers/exercises_controller_spec.rb | 1 - spec/controllers/sessions_controller_spec.rb | 1 - 9 files changed, 22 insertions(+), 33 deletions(-) diff --git a/app/controllers/concerns/lti.rb b/app/controllers/concerns/lti.rb index 5c656509..b7df77c7 100644 --- a/app/controllers/concerns/lti.rb +++ b/app/controllers/concerns/lti.rb @@ -19,16 +19,14 @@ module Lti # exercise_id.nil? ==> the user has logged out. All session data is to be destroyed # exercise_id.exists? ==> the user has submitted the results of an exercise to the consumer. # Only the lti_parameters are deleted. - def clear_lti_session_data(exercise_id = nil, user_id = nil, consumer_id = nil) + def clear_lti_session_data(exercise_id = nil, user_id = nil) if (exercise_id.nil?) - session.delete(:consumer_id) session.delete(:external_user_id) session.delete(:embed_options) session.delete(:lti_exercise_id) session.delete(:lti_parameters_id) end - LtiParameter.where(consumers_id: consumer_id, - external_users_id: user_id, + LtiParameter.where(external_users_id: user_id, exercises_id: exercise_id).destroy_all end @@ -242,7 +240,6 @@ module Lti lti_parameters.save! @lti_parameters = lti_parameters - session[:consumer_id] = options[:consumer].id session[:external_user_id] = @current_user.id session[:lti_parameters_id] = lti_parameters.id end diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index b8bc9f32..b6c94f5c 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -393,7 +393,7 @@ class ExercisesController < ApplicationController def redirect_to_lti_return_path Raven.extra_context( - consumers_id: session[:consumer_id], + consumers_id: @submission.user&.consumer, external_users_id: @submission.user_id, exercises_id: @submission.exercise_id, session: session.to_hash, @@ -404,15 +404,13 @@ class ExercisesController < ApplicationController lti_parameters_id: session[:lti_parameters_id] ) - lti_parameter = LtiParameter.where(consumers_id: session[:consumer_id], - external_users_id: @submission.user_id, + lti_parameter = LtiParameter.where(external_users_id: @submission.user_id, exercises_id: @submission.exercise_id).last - path = lti_return_path(consumer_id: session[:consumer_id], - submission_id: @submission.id, - url: consumer_return_url(build_tool_provider(consumer: Consumer.find_by(id: session[:consumer_id]), + path = lti_return_path(submission_id: @submission.id, + url: consumer_return_url(build_tool_provider(consumer: @submission.user.consumer, parameters: lti_parameter.lti_parameters))) - clear_lti_session_data(@submission.exercise_id, @submission.user_id, session[:consumer_id]) + clear_lti_session_data(@submission.exercise_id, @submission.user_id) respond_to do |format| format.html { redirect_to(path) } format.json { render(json: {redirect: path}) } @@ -517,7 +515,7 @@ class ExercisesController < ApplicationController @submission = Submission.create(submission_params) score_submission(@submission) current_user = ExternalUser.find(@submission.user_id) - if !current_user.nil? && lti_outcome_service?(@submission.exercise_id, current_user.id, current_user.consumer_id) + if !current_user.nil? && lti_outcome_service?(@submission.exercise_id, current_user.id) transmit_lti_score else redirect_after_submit @@ -573,7 +571,7 @@ class ExercisesController < ApplicationController # redirect 10 percent pseudorandomly to the feedback page if current_user.respond_to? :external_id if @submission.redirect_to_feedback? && !@embed_options[:disable_redirect_to_feedback] - clear_lti_session_data(@submission.exercise_id, @submission.user_id, session[:consumer_id]) + clear_lti_session_data(@submission.exercise_id, @submission.user_id) redirect_to_user_feedback return end @@ -584,7 +582,7 @@ class ExercisesController < ApplicationController flash[:notice] = I18n.t('exercises.submit.full_score_redirect_to_own_rfc') flash.keep(:notice) - clear_lti_session_data(@submission.exercise_id, @submission.user_id, session[:consumer_id]) + clear_lti_session_data(@submission.exercise_id, @submission.user_id) respond_to do |format| format.html { redirect_to(rfc) } format.json { render(json: {redirect: url_for(rfc)}) } @@ -602,7 +600,7 @@ class ExercisesController < ApplicationController # increase counter 'times_featured' in rfc rfc.increment!(:times_featured) - clear_lti_session_data(@submission.exercise_id, @submission.user_id, session[:consumer_id]) + clear_lti_session_data(@submission.exercise_id, @submission.user_id) respond_to do |format| format.html { redirect_to(rfc) } format.json { render(json: {redirect: url_for(rfc)}) } @@ -613,7 +611,7 @@ class ExercisesController < ApplicationController else # redirect to feedback page if score is less than 100 percent if @exercise.needs_more_feedback?(@submission) && !@embed_options[:disable_redirect_to_feedback] - clear_lti_session_data(@submission.exercise_id, @submission.user_id, session[:consumer_id]) + clear_lti_session_data(@submission.exercise_id, @submission.user_id) redirect_to_user_feedback else redirect_to_lti_return_path diff --git a/app/controllers/remote_evaluation_controller.rb b/app/controllers/remote_evaluation_controller.rb index dd369113..804ed616 100644 --- a/app/controllers/remote_evaluation_controller.rb +++ b/app/controllers/remote_evaluation_controller.rb @@ -23,7 +23,7 @@ class RemoteEvaluationController < ApplicationController if @submission.present? current_user = @submission.user - if !current_user.nil? && lti_outcome_service?(@submission.exercise_id, current_user.id, current_user.consumer_id) + if !current_user.nil? && lti_outcome_service?(@submission.exercise_id, current_user.id) lti_response = send_score(@submission) if lti_response[:status] == 'success' and lti_response[:score_sent] != @submission.normalized_score diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 145c3fd8..7cfe0e41 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -24,7 +24,7 @@ class SessionsController < ApplicationController redirect_to(params[:custom_redirect_target]) else redirect_to(implement_exercise_path(@exercise), - notice: t("sessions.create_through_lti.session_#{lti_outcome_service?(@exercise.id, @current_user.id , @consumer.id) ? 'with' : 'without'}_outcome", + notice: t("sessions.create_through_lti.session_#{lti_outcome_service?(@exercise.id, @current_user.id) ? 'with' : 'without'}_outcome", consumer: @consumer)) end end @@ -40,7 +40,7 @@ class SessionsController < ApplicationController def destroy_through_lti @submission = Submission.find(params[:submission_id]) - clear_lti_session_data(@submission.exercise_id, @submission.user_id, params[:consumer_id]) + clear_lti_session_data(@submission.exercise_id, @submission.user_id) end def new diff --git a/app/helpers/lti_helper.rb b/app/helpers/lti_helper.rb index de7624c7..9d2f56b5 100644 --- a/app/helpers/lti_helper.rb +++ b/app/helpers/lti_helper.rb @@ -1,12 +1,11 @@ require 'oauth/request_proxy/action_controller_request' # Rails 5 changed `Rack::Request` to `ActionDispatch::Request` module LtiHelper - def lti_outcome_service?(exercise_id, external_user_id, consumer_id) - return false if external_user_id == '' || consumer_id == '' + def lti_outcome_service?(exercise_id, external_user_id) + return false if external_user_id == '' - lti_parameters = LtiParameter.where(consumers_id: consumer_id, - external_users_id: external_user_id, - exercises_id: exercise_id).lis_outcome_service_url?.last + lti_parameters = LtiParameter.where(external_users_id: external_user_id, + exercises_id: exercise_id).lis_outcome_service_url?.last !lti_parameters.nil? && lti_parameters.present? end end diff --git a/app/views/exercises/_editor_output.html.slim b/app/views/exercises/_editor_output.html.slim index d9c377f0..0803d0b6 100644 --- a/app/views/exercises/_editor_output.html.slim +++ b/app/views/exercises/_editor_output.html.slim @@ -39,7 +39,7 @@ div.h-100 id='output_sidebar_uncollapsed' class='d-none col-sm-12 enforce-bottom .progress-bar role='progressbar' br - - if lti_outcome_service?(@exercise.id, external_user_id, consumer_id) + - if lti_outcome_service?(@exercise.id, external_user_id) p.text-center = render('editor_button', classes: 'btn-lg btn-success d-none', data: {:'data-url' => submit_exercise_path(@exercise)}, icon: 'fa fa-send', id: 'submit', label: t('exercises.editor.submit')) - if @exercise.submission_deadline.present? || @exercise.late_submission_deadline.present? #deadline data-submission-deadline=@exercise.submission_deadline&.rfc2822 data-late-submission-deadline=@exercise.late_submission_deadline&.rfc2822 diff --git a/spec/concerns/lti_spec.rb b/spec/concerns/lti_spec.rb index ab02327a..5349ba9c 100644 --- a/spec/concerns/lti_spec.rb +++ b/spec/concerns/lti_spec.rb @@ -17,7 +17,6 @@ describe Lti do describe '#clear_lti_session_data' do it 'clears the session' do - expect(controller.session).to receive(:delete).with(:consumer_id) expect(controller.session).to receive(:delete).with(:external_user_id) expect(controller.session).to receive(:delete).with(:embed_options) expect(controller.session).to receive(:delete).with(:lti_exercise_id) @@ -112,9 +111,6 @@ describe Lti do context 'with an valid score' do context 'with a tool consumer' do - before(:each) do - controller.session[:consumer_id] = consumer.id - end context 'when grading is not supported' do it 'returns a corresponding status' do @@ -153,6 +149,8 @@ describe Lti do context 'without a tool consumer' do it 'returns a corresponding status' do + submission.user.consumer = nil + allow(submission).to receive(:normalized_score).and_return score expect(controller.send(:send_score, submission)[:status]).to eq('error') end @@ -166,7 +164,6 @@ describe Lti do it 'stores data in the session' do controller.instance_variable_set(:@current_user, FactoryBot.create(:external_user)) controller.instance_variable_set(:@exercise, FactoryBot.create(:fibonacci)) - expect(controller.session).to receive(:[]=).with(:consumer_id, anything) expect(controller.session).to receive(:[]=).with(:external_user_id, anything) expect(controller.session).to receive(:[]=).with(:lti_parameters_id, anything) controller.send(:store_lti_session_data, consumer: FactoryBot.build(:consumer), parameters: parameters) diff --git a/spec/controllers/exercises_controller_spec.rb b/spec/controllers/exercises_controller_spec.rb index 7a687e38..9d77ed24 100644 --- a/spec/controllers/exercises_controller_spec.rb +++ b/spec/controllers/exercises_controller_spec.rb @@ -237,7 +237,6 @@ describe ExercisesController do allow_any_instance_of(Submission).to receive(:normalized_score).and_return(1) expect(controller).to receive(:collect_test_results).and_return([{score: 1, weight: 1}]) expect(controller).to receive(:score_submission).and_call_original - controller.session[:consumer_id] = external_user.consumer_id end context 'when LTI outcomes are supported' do diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 0f51fbbc..88833c0f 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -200,7 +200,6 @@ describe SessionsController do before(:each) do #Todo replace session with lti_parameter - session[:consumer_id] = consumer.id #Todo create LtiParameter Object # session[:lti_parameters] = {} end