From 5fe12bfa783ac6d119790e706449273449ba907d Mon Sep 17 00:00:00 2001 From: Tom Staubitz Date: Thu, 12 Jan 2017 18:12:36 +0100 Subject: [PATCH] fixing tests p1 --- app/controllers/concerns/lti.rb | 14 +-- app/controllers/exercises_controller.rb | 6 +- app/controllers/sessions_controller.rb | 5 +- app/helpers/lti_helper.rb | 2 +- app/views/exercises/_editor.html.slim | 5 +- ...> 20170112151637_create_lti_parameters.rb} | 3 +- db/structure.sql | 111 +----------------- spec/concerns/lti_spec.rb | 12 +- spec/factories/lti_parameter.rb | 12 ++ spec/factories/submission.rb | 2 +- spec/lib/docker_client_spec.rb | 4 +- 11 files changed, 41 insertions(+), 135 deletions(-) rename db/migrate/{20161214144837_create_lti_parameters.rb => 20170112151637_create_lti_parameters.rb} (88%) create mode 100644 spec/factories/lti_parameter.rb diff --git a/app/controllers/concerns/lti.rb b/app/controllers/concerns/lti.rb index 073eb319..434a106f 100644 --- a/app/controllers/concerns/lti.rb +++ b/app/controllers/concerns/lti.rb @@ -18,14 +18,13 @@ 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) - @current_user = ExternalUser.find(@submission.user_id) + def clear_lti_session_data(exercise_id = nil, user_id = nil, consumer_id = nil) if (exercise_id.nil?) session.delete(:consumer_id) session.delete(:external_user_id) else - LtiParameter.destroy_all(consumers_id: @consumer.id, - external_user_id: @current_user.external_id, + LtiParameter.destroy_all(consumers_id: consumer_id, + external_users_id: user_id, exercises_id: exercise_id) end end @@ -99,12 +98,12 @@ module Lti end private :return_to_consumer - def send_score(exercise_id, score) + def send_score(exercise_id, score, user_id) ::NewRelic::Agent.add_custom_parameters({ score: score, session: session }) fail(Error, "Score #{score} must be between 0 and #{MAXIMUM_SCORE}!") unless (0..MAXIMUM_SCORE).include?(score) lti_parameter = LtiParameter.where(consumers_id: session[:consumer_id], - external_user_id: @current_user.external_id, + external_users_id: user_id, exercises_id: exercise_id).first consumer = Consumer.find_by(id: session[:consumer_id]) @@ -123,7 +122,6 @@ module Lti def set_current_user @current_user = ExternalUser.find_or_create_by(consumer_id: @consumer.id, external_id: @provider.user_id) - #TODO is this really necessary, and does it work at all? @current_user.update(email: external_user_email(@provider), name: external_user_name(@provider)) end private :set_current_user @@ -133,7 +131,7 @@ module Lti exercise_id = exercise.id unless exercise.nil? lti_parameters = LtiParameter.find_or_create_by(consumers_id: options[:consumer].id, - external_user_id: options[:parameters][:user_id].to_s, + external_users_id: @current_user.id, #options[:parameters][:user_id].to_s, exercises_id: exercise_id) lti_parameters.lti_parameters = options[:parameters].slice(*SESSION_PARAMETERS).to_json diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index 36a0ecbe..4e27d013 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -158,7 +158,7 @@ class ExercisesController < ApplicationController def redirect_to_lti_return_path lti_parameter = LtiParameter.where(consumers_id: session[:consumer_id], - external_user_id: @current_user.external_id, + external_users_id: @current_user.id, exercises_id: @submission.exercise_id).first path = lti_return_path(consumer_id: session[:consumer_id], @@ -228,7 +228,7 @@ class ExercisesController < ApplicationController def submit @submission = Submission.create(submission_params) score_submission(@submission) - if lti_outcome_service?(@submission.exercise_id, @current_user.external_id, @current_user.consumer_id) + if lti_outcome_service?(@submission.exercise_id, @current_user.id, @current_user.consumer_id) transmit_lti_score else redirect_after_submit @@ -237,7 +237,7 @@ class ExercisesController < ApplicationController def transmit_lti_score ::NewRelic::Agent.add_custom_parameters({ submission: @submission.id, normalized_score: @submission.normalized_score }) - response = send_score(@submission.exercise_id, @submission.normalized_score) + response = send_score(@submission.exercise_id, @submission.normalized_score, @submission.user_id) if response[:status] == 'success' redirect_after_submit diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 47445b68..e6bdac8c 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -22,7 +22,7 @@ class SessionsController < ApplicationController store_lti_session_data(consumer: @consumer, parameters: params) store_nonce(params[:oauth_nonce]) redirect_to(implement_exercise_path(@exercise), - notice: t("sessions.create_through_lti.session_#{lti_outcome_service?(@exercise.id, @current_user.external_id , @consumer.id) ? 'with' : 'without'}_outcome", + notice: t("sessions.create_through_lti.session_#{lti_outcome_service?(@exercise.id, @current_user.id , @consumer.id) ? 'with' : 'without'}_outcome", consumer: @consumer)) end @@ -36,9 +36,8 @@ class SessionsController < ApplicationController end def destroy_through_lti - @consumer = Consumer.find_by(id: params[:consumer_id]) @submission = Submission.find(params[:submission_id]) - clear_lti_session_data(@submission.exercise_id) + clear_lti_session_data(@submission.exercise_id, @submission.user_id, params[:consumer_id]) end def new diff --git a/app/helpers/lti_helper.rb b/app/helpers/lti_helper.rb index 42ccae48..f46974cd 100644 --- a/app/helpers/lti_helper.rb +++ b/app/helpers/lti_helper.rb @@ -3,7 +3,7 @@ module LtiHelper return false if external_user_id == '' || consumer_id == '' lti_parameters = LtiParameter.where(consumers_id: consumer_id, - external_user_id: external_user_id, + external_users_id: external_user_id, exercises_id: exercise_id).lis_outcome_service_url? !lti_parameters.nil? && lti_parameters.size > 0 end diff --git a/app/views/exercises/_editor.html.slim b/app/views/exercises/_editor.html.slim index aad007ee..56986673 100644 --- a/app/views/exercises/_editor.html.slim +++ b/app/views/exercises/_editor.html.slim @@ -1,6 +1,7 @@ -- external_user_id = @current_user.respond_to?(:external_id) ? @current_user.external_id : '' #'tests' #(@current_user.uuid.present? ? @current_user.uuid : '') +- external_user_external_id = @current_user.respond_to?(:external_id) ? @current_user.external_id : '' #'tests' #(@current_user.uuid.present? ? @current_user.uuid : '') +- external_user_id = @current_user.respond_to?(:external_id) ? @current_user.id : '' #'tests' #(@current_user.uuid.present? ? @current_user.uuid : '') - consumer_id = @current_user.respond_to?(:external_id) ? @current_user.consumer_id : '' #'tests' #(@current_user.uuid.present? ? @current_user.uuid : '') -#editor.row data-exercise-id=exercise.id data-message-depleted=t('exercises.editor.depleted') data-message-timeout=t('exercises.editor.timeout', permitted_execution_time: @exercise.execution_environment.permitted_execution_time) data-errors-url=execution_environment_errors_path(exercise.execution_environment) data-submissions-url=submissions_path data-user-id=@current_user.id data-user-external-id=external_user_id +#editor.row data-exercise-id=exercise.id data-message-depleted=t('exercises.editor.depleted') data-message-timeout=t('exercises.editor.timeout', permitted_execution_time: @exercise.execution_environment.permitted_execution_time) data-errors-url=execution_environment_errors_path(exercise.execution_environment) data-submissions-url=submissions_path data-user-id=@current_user.id data-user-external-id=external_user_external_id div id="sidebar" class=(@exercise.hide_file_tree ? 'sidebar-col-collapsed' : 'sidebar-col') = render('editor_file_tree', exercise: @exercise, files: @files) div id='output_sidebar' class='output-col-collapsed' = render('exercises/editor_output', external_user_id: external_user_id, consumer_id: consumer_id ) div id='frames' class='editor-col' diff --git a/db/migrate/20161214144837_create_lti_parameters.rb b/db/migrate/20170112151637_create_lti_parameters.rb similarity index 88% rename from db/migrate/20161214144837_create_lti_parameters.rb rename to db/migrate/20170112151637_create_lti_parameters.rb index 053149a6..c7613edf 100644 --- a/db/migrate/20161214144837_create_lti_parameters.rb +++ b/db/migrate/20170112151637_create_lti_parameters.rb @@ -1,11 +1,10 @@ class CreateLtiParameters < ActiveRecord::Migration def change create_table :lti_parameters do |t| - t.string :external_user_id + t.belongs_to :external_users t.belongs_to :consumers t.belongs_to :exercises t.jsonb :lti_parameters, null: false, default: '{}' - t.timestamps end end diff --git a/db/structure.sql b/db/structure.sql index 298a1e32..4b290bdc 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -485,43 +485,13 @@ CREATE SEQUENCE internal_users_id_seq ALTER SEQUENCE internal_users_id_seq OWNED BY internal_users.id; --- --- Name: internal_users_teams; Type: TABLE; Schema: public; Owner: - --- - -CREATE TABLE internal_users_teams ( - id integer NOT NULL, - internal_user_id integer, - team_id integer -); - - --- --- Name: internal_users_teams_id_seq; Type: SEQUENCE; Schema: public; Owner: - --- - -CREATE SEQUENCE internal_users_teams_id_seq - START WITH 1 - INCREMENT BY 1 - NO MINVALUE - NO MAXVALUE - CACHE 1; - - --- --- Name: internal_users_teams_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: - --- - -ALTER SEQUENCE internal_users_teams_id_seq OWNED BY internal_users_teams.id; - - -- -- Name: lti_parameters; Type: TABLE; Schema: public; Owner: - -- CREATE TABLE lti_parameters ( id integer NOT NULL, - external_user_id character varying, + external_users_id integer, consumers_id integer, exercises_id integer, lti_parameters jsonb DEFAULT '{}'::jsonb NOT NULL, @@ -591,7 +561,7 @@ ALTER SEQUENCE request_for_comments_id_seq OWNED BY request_for_comments.id; -- CREATE TABLE schema_migrations ( - version character varying NOT NULL + version character varying(255) NOT NULL ); @@ -630,37 +600,6 @@ CREATE SEQUENCE submissions_id_seq ALTER SEQUENCE submissions_id_seq OWNED BY submissions.id; --- --- Name: teams; Type: TABLE; Schema: public; Owner: - --- - -CREATE TABLE teams ( - id integer NOT NULL, - name character varying, - created_at timestamp without time zone, - updated_at timestamp without time zone -); - - --- --- Name: teams_id_seq; Type: SEQUENCE; Schema: public; Owner: - --- - -CREATE SEQUENCE teams_id_seq - START WITH 1 - INCREMENT BY 1 - NO MINVALUE - NO MAXVALUE - CACHE 1; - - --- --- Name: teams_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: - --- - -ALTER SEQUENCE teams_id_seq OWNED BY teams.id; - - -- -- Name: testruns; Type: TABLE; Schema: public; Owner: - -- @@ -779,13 +718,6 @@ ALTER TABLE ONLY hints ALTER COLUMN id SET DEFAULT nextval('hints_id_seq'::regcl ALTER TABLE ONLY internal_users ALTER COLUMN id SET DEFAULT nextval('internal_users_id_seq'::regclass); --- --- Name: id; Type: DEFAULT; Schema: public; Owner: - --- - -ALTER TABLE ONLY internal_users_teams ALTER COLUMN id SET DEFAULT nextval('internal_users_teams_id_seq'::regclass); - - -- -- Name: id; Type: DEFAULT; Schema: public; Owner: - -- @@ -807,13 +739,6 @@ ALTER TABLE ONLY request_for_comments ALTER COLUMN id SET DEFAULT nextval('reque ALTER TABLE ONLY submissions ALTER COLUMN id SET DEFAULT nextval('submissions_id_seq'::regclass); --- --- Name: id; Type: DEFAULT; Schema: public; Owner: - --- - -ALTER TABLE ONLY teams ALTER COLUMN id SET DEFAULT nextval('teams_id_seq'::regclass); - - -- -- Name: id; Type: DEFAULT; Schema: public; Owner: - -- @@ -917,14 +842,6 @@ ALTER TABLE ONLY internal_users ADD CONSTRAINT internal_users_pkey PRIMARY KEY (id); --- --- Name: internal_users_teams_pkey; Type: CONSTRAINT; Schema: public; Owner: - --- - -ALTER TABLE ONLY internal_users_teams - ADD CONSTRAINT internal_users_teams_pkey PRIMARY KEY (id); - - -- -- Name: lti_parameters_pkey; Type: CONSTRAINT; Schema: public; Owner: - -- @@ -949,14 +866,6 @@ ALTER TABLE ONLY submissions ADD CONSTRAINT submissions_pkey PRIMARY KEY (id); --- --- Name: teams_pkey; Type: CONSTRAINT; Schema: public; Owner: - --- - -ALTER TABLE ONLY teams - ADD CONSTRAINT teams_pkey PRIMARY KEY (id); - - -- -- Name: testruns_pkey; Type: CONSTRAINT; Schema: public; Owner: - -- @@ -1028,20 +937,6 @@ CREATE INDEX index_internal_users_on_remember_me_token ON internal_users USING b CREATE INDEX index_internal_users_on_reset_password_token ON internal_users USING btree (reset_password_token); --- --- Name: index_internal_users_teams_on_internal_user_id; Type: INDEX; Schema: public; Owner: - --- - -CREATE INDEX index_internal_users_teams_on_internal_user_id ON internal_users_teams USING btree (internal_user_id); - - --- --- Name: index_internal_users_teams_on_team_id; Type: INDEX; Schema: public; Owner: - --- - -CREATE INDEX index_internal_users_teams_on_team_id ON internal_users_teams USING btree (team_id); - - -- -- Name: unique_schema_migrations; Type: INDEX; Schema: public; Owner: - -- @@ -1223,5 +1118,5 @@ INSERT INTO schema_migrations (version) VALUES ('20160704143402'); INSERT INTO schema_migrations (version) VALUES ('20160907123009'); -INSERT INTO schema_migrations (version) VALUES ('20161214144837'); +INSERT INTO schema_migrations (version) VALUES ('20170112151637'); diff --git a/spec/concerns/lti_spec.rb b/spec/concerns/lti_spec.rb index 87c11d02..27708866 100644 --- a/spec/concerns/lti_spec.rb +++ b/spec/concerns/lti_spec.rb @@ -17,11 +17,8 @@ describe Lti do describe '#clear_lti_session_data' do it 'clears the session' do - #Todo replace session with lti_parameter 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(:lti_parameters) - #Todo check that there are no more LtiParameters for this user/consumer/(exercise?) controller.send(:clear_lti_session_data) end end @@ -108,10 +105,13 @@ describe Lti do describe '#send_score' do let(:consumer) { FactoryGirl.create(:consumer) } let(:score) { 0.5 } + #let(:exercise) { FactoryGirl.create(:math) } + let(:submission) { FactoryGirl.create(:submission) } + let!(:lti_parameter) { FactoryGirl.create(:lti_parameter)} context 'with an invalid score' do it 'raises an exception' do - expect { controller.send(:send_score, Lti::MAXIMUM_SCORE * 2) }.to raise_error(Lti::Error) + expect { controller.send(:send_score, Lti::MAXIMUM_SCORE * 2, submission.exercise_id, submission.user_id) }.to raise_error(Lti::Error) end end @@ -168,9 +168,10 @@ describe Lti do describe '#store_lti_session_data' do #Todo replace session with lti_parameter let(:parameters) { {} } + before_count = LtiParameter.count before(:each) { controller.instance_variable_set(:@current_user, FactoryGirl.create(:external_user)) } #Todo do this with lti_parameter object - # after(:each) { controller.send(:store_lti_session_data, consumer: FactoryGirl.build(:consumer), parameters: parameters) } + after(:each) { controller.send(:store_lti_session_data, consumer: FactoryGirl.build(:consumer), parameters: parameters) } it 'stores data in the session' do #Todo replace session with lti_parameter @@ -178,6 +179,7 @@ describe Lti do expect(controller.session).to receive(:[]=).with(:external_user_id, anything) # expect(controller.session).to receive(:[]=).with(:lti_parameters, kind_of(Hash)) #Todo it creates an LtiParameter Object + expect(LtiParameter.count).to eq(before_count + 1) end it 'stores only selected tuples' do diff --git a/spec/factories/lti_parameter.rb b/spec/factories/lti_parameter.rb new file mode 100644 index 00000000..d80e85e4 --- /dev/null +++ b/spec/factories/lti_parameter.rb @@ -0,0 +1,12 @@ +FactoryGirl.define do + factory :lti_parameter do + association :consumers_id, factory: :consumer + association :exercises_id, factory: :math + association :external_users_id, factory: :external_user + + + trait :lti_parameters do + JSON.parse('{"lis_result_sourcedid": "c2db0c7c-4411-4b27-a52b-ddfc3dc32065", "lis_outcome_service_url": "http://172.16.54.235:3000/courses/0132156a-9afb-434d-83cc-704780104105/sections/21c6c6f4-1fb6-43b4-af3c-04fdc098879e/items/999b1fe6-d4b6-47b7-a577-ea2b4b1041ec/tool_grading", "launch_presentation_return_url": "http://172.16.54.235:3000/courses/0132156a-9afb-434d-83cc-704780104105/sections/21c6c6f4-1fb6-43b4-af3c-04fdc098879e/items/999b1fe6-d4b6-47b7-a577-ea2b4b1041ec/tool_return"}') + end + end +end \ No newline at end of file diff --git a/spec/factories/submission.rb b/spec/factories/submission.rb index b3ef8a0e..cd4e34e1 100644 --- a/spec/factories/submission.rb +++ b/spec/factories/submission.rb @@ -2,7 +2,7 @@ FactoryGirl.define do factory :submission do cause 'save' created_by_external_user - association :exercise, factory: :fibonacci + association :exercise, factory: :math after(:create) do |submission| submission.exercise.files.editable.visible.each do |file| diff --git a/spec/lib/docker_client_spec.rb b/spec/lib/docker_client_spec.rb index e9343f9a..feb457f6 100644 --- a/spec/lib/docker_client_spec.rb +++ b/spec/lib/docker_client_spec.rb @@ -3,8 +3,8 @@ require 'seeds_helper' describe DockerClient, docker: true do let(:command) { 'whoami' } - let(:docker_client) { described_class.new(execution_environment: FactoryGirl.build(:ruby), user: FactoryGirl.build(:admin)) } - let(:execution_environment) { FactoryGirl.build(:ruby) } + let(:docker_client) { described_class.new(execution_environment: FactoryGirl.build(:java), user: FactoryGirl.build(:admin)) } + let(:execution_environment) { FactoryGirl.build(:java) } let(:image) { double } let(:submission) { FactoryGirl.create(:submission) } let(:workspace_path) { '/tmp' }