Automatically submit LTI grade on each score run

With this commit, we refactor the overall score handling of CodeOcean. Previously, "Score" and "Submit" were two distinct actions, requiring users to confirm the LTI transmission of their score (after assessing their submission). This yielded many questions and was unnecessary, since LTI parameters are no longer expiring after each use. Therefore, we can now transmit the current grade on each score run with the very same LTI parameters. As a consequence, the LTI consumer gets a more detailed history of the scores, enabling further analytical insights.

For users, the previous "Submit" button got replaced with a notification that is shown as soon as the full score got reached. Then, learners can decide to "finalize" their work on the given exercise, which will initiate a redirect to a follow-up action (as defined in the RedirectBehavior). This RedirectBehavior has also been unified and simplified for better readability.

As part of this refactoring, we rephrased the notifications and UX workflow of a) the LTI transmission, b) the finalization of an exercise (measured by reaching the full score) and c) the deadline handling (on time, within grace period, too late). Those information are now separately shown, potentially resulting in multiple notifications. As a side effect, they are much better maintainable, and the LTI transmission is more decoupled from this notification handling.
This commit is contained in:
kiragrammel
2023-05-24 12:44:13 +02:00
committed by Sebastian Serth
parent 1e06ab3fa9
commit 175c8933f3
26 changed files with 779 additions and 408 deletions

View File

@ -114,17 +114,19 @@ RSpec.describe Lti do
it 'returns a corresponding status' do
allow_any_instance_of(IMS::LTI::ToolProvider).to receive(:outcome_service?).and_return(false)
allow(submission).to receive(:normalized_score).and_return score
expect(controller.send(:send_scores, submission).first[:status]).to eq('unsupported')
submit_info = controller.send(:send_scores, submission)
expect(submit_info[:users][:all]).to eq(submit_info[:users][:unsupported])
end
end
context 'when grading is supported' do
let(:response) { double }
let(:send_scores) { controller.send(:send_scores, submission).first }
let(:send_scores) { controller.send(:send_scores, submission) }
let(:score_sent) { score }
before do
allow_any_instance_of(IMS::LTI::ToolProvider).to receive(:outcome_service?).and_return(true)
allow_any_instance_of(IMS::LTI::ToolProvider).to receive(:post_replace_result!).with(score).and_return(response)
allow_any_instance_of(IMS::LTI::ToolProvider).to receive(:post_replace_result!).with(score_sent).and_return(response)
allow(response).to receive(:response_code).at_least(:once).and_return(200)
allow(response).to receive(:post_response).and_return(response)
allow(response).to receive(:body).at_least(:once).and_return('')
@ -133,14 +135,83 @@ RSpec.describe Lti do
end
it 'sends the score' do
expect_any_instance_of(IMS::LTI::ToolProvider).to receive(:post_replace_result!).with(score)
expect_any_instance_of(IMS::LTI::ToolProvider).to receive(:post_replace_result!).with(score_sent)
send_scores
end
it 'returns code, message, and status' do
expect(send_scores[:code]).to eq(response.response_code)
expect(send_scores[:message]).to eq(response.body)
expect(send_scores[:status]).to eq(response.code_major)
it 'returns code, message, deadline and status' do
expect(send_scores[:users][:all]).to eq(send_scores[:users][:success])
expect(send_scores[:deadline]).to eq(:none)
expect(send_scores[:detailed_results].first[:code]).to eq(response.response_code)
expect(send_scores[:detailed_results].first[:message]).to eq(response.body)
expect(send_scores[:detailed_results].first[:status]).to eq(response.code_major)
end
context 'when submission is before deadline' do
before do
allow(submission).to receive(:before_deadline?).and_return true
end
it 'returns deadline' do
expect(send_scores[:deadline]).to eq(:before_deadline)
end
end
context 'when submission is within grace period' do
let(:score_sent) { score * 0.8 }
before do
allow(submission).to receive_messages(before_deadline?: false, within_grace_period?: true)
end
it 'returns deadline and reduced score' do
expect(send_scores[:deadline]).to eq(:within_grace_period)
expect(send_scores[:score][:sent]).to eq(score * 0.8)
end
it 'sends the reduced score' do
expect_any_instance_of(IMS::LTI::ToolProvider).to receive(:post_replace_result!).with(score_sent)
send_scores
end
end
context 'when submission is after late deadline' do
let(:score_sent) { score * 0 }
before do
allow(submission).to receive_messages(before_deadline?: false,
within_grace_period?: false,
after_late_deadline?: true)
end
it 'returns deadline and reduced score' do
expect(send_scores[:deadline]).to eq(:after_late_deadline)
expect(send_scores[:score][:sent]).to eq(score * 0)
end
it 'sends the reduced score' do
expect_any_instance_of(IMS::LTI::ToolProvider).to receive(:post_replace_result!).with(score_sent)
send_scores
end
end
end
context 'when transmission fails' do
let(:send_scores) { controller.send(:send_scores, submission) }
let(:score_sent) { 0 }
before do
allow_any_instance_of(IMS::LTI::ToolProvider).to receive(:outcome_service?).and_return(true)
allow_any_instance_of(IMS::LTI::ToolProvider).to receive(:post_replace_result!).with(score_sent).and_raise(IMS::LTI::XMLParseError)
end
it 'does not raise an exception' do
expect { send_scores }.not_to raise_error
end
it 'returns an error status' do
expect(send_scores[:users][:all]).to eq(send_scores[:users][:error])
expect(send_scores[:detailed_results].first[:status]).to eq('error')
end
end
end
@ -150,7 +221,8 @@ RSpec.describe Lti do
submission.contributor.consumer = nil
allow(submission).to receive(:normalized_score).and_return score
expect(controller.send(:send_scores, submission).first[:status]).to eq('error')
submit_info = controller.send(:send_scores, submission)
expect(submit_info[:users][:all]).to eq(submit_info[:users][:unsupported])
end
end
end

View File

@ -288,98 +288,6 @@ RSpec.describe ExercisesController do
end
end
describe 'POST #submit' do
let(:output) { {} }
let(:perform_request) { post :submit, format: :json, params: {id: exercise.id, submission: {cause: 'submit', exercise_id: exercise.id}} }
let(:contributor) { create(:external_user) }
let(:scoring_response) do
[{
status: :ok,
stdout: '',
stderr: '',
waiting_for_container_time: 0,
container_execution_time: 0,
file_role: 'teacher_defined_test',
count: 1,
failed: 0,
error_messages: [],
passed: 1,
score: 1.0,
filename: 'index.html_spec.rb',
message: 'Well done.',
weight: 2.0,
}]
end
before do
create(:lti_parameter, external_user: contributor, exercise:)
submission = build(:submission, exercise:, contributor:)
allow(submission).to receive_messages(normalized_score: 1, calculate_score: scoring_response, redirect_to_feedback?: false)
allow(Submission).to receive(:create).and_return(submission)
end
context 'when LTI outcomes are supported' do
before do
allow(controller).to receive(:lti_outcome_service?).and_return(true)
end
context 'when the score transmission succeeds' do
before do
allow(controller).to receive(:send_scores).and_return([{status: 'success'}])
perform_request
end
expect_assigns(exercise: :exercise)
it 'creates a submission' do
expect(assigns(:submission)).to be_a(Submission)
end
expect_json
expect_http_status(:ok)
end
context 'when the score transmission fails' do
before do
allow(controller).to receive(:send_scores).and_return([{status: 'unsupported'}])
perform_request
end
expect_assigns(exercise: :exercise)
it 'creates a submission' do
expect(assigns(:submission)).to be_a(Submission)
end
it 'returns an error message' do
expect(response.parsed_body).to eq('danger' => I18n.t('exercises.submit.failure'))
end
expect_json
end
end
context 'when LTI outcomes are not supported' do
before do
allow(controller).to receive(:lti_outcome_service?).and_return(false)
perform_request
end
expect_assigns(exercise: :exercise)
it 'creates a submission' do
expect(assigns(:submission)).to be_a(Submission)
end
it 'does not send scores' do
expect(controller).not_to receive(:send_scores)
end
expect_json
expect_http_status(:ok)
end
end
describe 'PUT #update' do
context 'with a valid exercise' do
let(:exercise_attributes) { build(:dummy).attributes }

View File

@ -6,7 +6,8 @@ RSpec.describe SubmissionsController do
render_views
let(:exercise) { create(:math) }
let(:submission) { create(:submission, exercise:, contributor:) }
let(:cause) { 'save' }
let(:submission) { create(:submission, exercise:, contributor:, cause:) }
shared_examples 'a regular user' do |record_not_found_status_code|
describe 'POST #create' do
@ -39,6 +40,15 @@ RSpec.describe SubmissionsController do
end
end
describe 'GET #download' do
let(:perform_request) { proc { get :download, params: {id: submission.id} } }
before { perform_request.call }
expect_assigns(submission: :submission)
expect_http_status(:ok)
end
describe 'GET #download_file' do
context 'with an invalid filename' do
before { get :download_file, params: {filename: SecureRandom.hex, id: submission.id, format: :json} }
@ -98,6 +108,84 @@ RSpec.describe SubmissionsController do
end
end
describe 'GET #finalize' do
let(:perform_request) { proc { get :finalize, params: {id: submission.id} } }
let(:cause) { 'assess' }
context 'when the request is performed' do
before { perform_request.call }
expect_assigns(submission: :submission)
expect_redirect
end
it 'updates cause to submit' do
expect { perform_request.call && submission.reload }.to change(submission, :cause).from('assess').to('submit')
end
context 'when contributing to a community solution is possible' do
let!(:community_solution) { CommunitySolution.create(exercise:) }
before do
allow(Java21Study).to receive(:allow_redirect_to_community_solution?).and_return(true)
perform_request.call
end
expect_redirect { edit_community_solution_path(community_solution, lock_id: CommunitySolutionLock.last) }
end
context 'when sharing exercise feedback is desired' do
before do
uef&.save!
allow_any_instance_of(Submission).to receive(:redirect_to_feedback?).and_return(true)
perform_request.call
end
context 'without any previous feedback' do
let(:uef) { nil }
expect_redirect { new_user_exercise_feedback_path(user_exercise_feedback: {exercise_id: submission.exercise.id}) }
end
context 'with a previous feedback for the same exercise' do
let(:uef) { create(:user_exercise_feedback, exercise:, user: current_user) }
expect_redirect { edit_user_exercise_feedback_path(uef) }
end
end
context 'with an RfC' do
before do
rfc.save!
allow_any_instance_of(Submission).to receive(:redirect_to_feedback?).and_return(false)
perform_request.call
end
context 'when an own RfC is unsolved' do
let(:rfc) { create(:rfc, user: current_user, exercise:, submission:) }
expect_flash_message(:notice, I18n.t('exercises.editor.exercise_finished_redirect_to_own_rfc'))
expect_redirect { request_for_comment_url(rfc) }
end
context 'when another RfC is unsolved' do
let(:rfc) { create(:rfc, exercise:) }
expect_flash_message(:notice, I18n.t('exercises.editor.exercise_finished_redirect_to_rfc'))
expect_redirect { request_for_comment_url(rfc) }
end
end
context 'when neither a community solution, feedback nor RfC is available' do
before do
allow_any_instance_of(Submission).to receive(:redirect_to_feedback?).and_return(false)
perform_request.call
end
expect_redirect { lti_return_path(submission_id: submission.id) }
end
end
describe 'GET #render_file' do
let(:file) { submission.files.first }
let(:signed_url) { AuthenticatedUrlHelper.sign(render_submission_url(submission, filename), submission) }
@ -259,8 +347,9 @@ RSpec.describe SubmissionsController do
context 'with an admin user' do
let(:contributor) { create(:admin) }
let(:current_user) { contributor }
before { allow(controller).to receive(:current_user).and_return(contributor) }
before { allow(controller).to receive_messages(current_user:) }
describe 'GET #index' do
before do
@ -280,10 +369,9 @@ RSpec.describe SubmissionsController do
let(:group_author) { create(:external_user) }
let(:other_group_author) { create(:external_user) }
let(:contributor) { create(:programming_group, exercise:, users: [group_author, other_group_author]) }
let(:current_user) { group_author }
before do
allow(controller).to receive_messages(current_contributor: contributor, current_user: group_author)
end
before { allow(controller).to receive_messages(current_contributor: contributor, current_user:) }
it_behaves_like 'a regular user', :unauthorized
it_behaves_like 'denies access for regular, non-admin users'
@ -291,10 +379,9 @@ RSpec.describe SubmissionsController do
context 'with a learner' do
let(:contributor) { create(:external_user) }
let(:current_user) { contributor }
before do
allow(controller).to receive_messages(current_user: contributor)
end
before { allow(controller).to receive_messages(current_user:) }
it_behaves_like 'a regular user', :unauthorized
it_behaves_like 'denies access for regular, non-admin users'

View File

@ -109,13 +109,4 @@ RSpec.describe 'Editor', :js do
expect(page).not_to have_content(I18n.t('exercises.editor.score'))
end
end
it 'contains a button for submitting the exercise' do
submission = build(:submission, contributor:, exercise:)
allow(submission).to receive(:calculate_score).and_return(scoring_response)
allow(Submission).to receive(:find).and_return(submission)
click_button(I18n.t('exercises.editor.score'))
expect(page).not_to have_content(I18n.t('exercises.editor.tooltips.exercise_deadline_passed'))
expect(page).to have_content(I18n.t('exercises.editor.submit'))
end
end

317
spec/features/score_spec.rb Normal file
View File

@ -0,0 +1,317 @@
# frozen_string_literal: true
require 'rails_helper'
RSpec.describe 'Score', :js do
let(:exercise) { create(:hello_world) }
let(:contributor) { create(:external_user) }
let(:submission) { create(:submission, exercise:, contributor:, score:) }
before do
allow_any_instance_of(ApplicationController).to receive(:current_user).and_return(contributor)
allow(Submission).to receive(:find).and_return(submission)
visit(implement_exercise_path(exercise))
end
shared_examples 'exercise finished notification' do
it "shows an 'exercise finished' notification" do
# Text needs to be split because it includes the embedded URL in the HTML which is not shown in the notification.
# We compare the shown notification text and the URL separately.
expect(page).to have_content(I18n.t('exercises.editor.exercise_finished').split('.').first)
expect(page).to have_link(nil, href: finalize_submission_path(submission))
end
end
shared_examples 'no exercise finished notification' do
it "does not show an 'exercise finished' notification" do
# Text needs to be split because it includes the embedded URL in the HTML which is not shown in the notification.
# We compare the shown notification text and the URL separately.
expect(page).not_to have_content(I18n.t('exercises.editor.exercise_finished').split('.').first)
expect(page).not_to have_link(nil, href: finalize_submission_path(submission))
end
end
shared_examples 'notification' do |message_key|
it "shows a '#{message_key.split('.').last}' notification" do
options = {}
options[:score_sent] = (score_sent * 100).to_i if defined? score_sent
options[:user] = users_error.map(&:displayname).join(', ') if defined? users_error
expect(page).to have_content(I18n.t(message_key, **options))
end
end
shared_examples 'no notification' do |message_key|
it "does not show a '#{message_key.split('.').last}' notification" do
expect(page).not_to have_content(I18n.t(message_key))
end
end
context 'when scoring is successful' do
let(:lti_outcome_service?) { true }
let(:scoring_response) do
{
users: {all: users_success + users_error + users_unsupported, success: users_success, error: users_error, unsupported: users_unsupported},
score: {original: score, sent: score_sent},
deadline:,
detailed_results: [],
}
end
let(:calculate_response) do
[{
status: :ok,
stdout: '',
stderr: '',
waiting_for_container_time: 0,
container_execution_time: 0,
file_role: :teacher_defined_test,
count: 1,
failed: 0,
error_messages: [],
passed: 1,
score:,
filename: 'exercise_spec.rb',
message: 'Well done.',
weight: 1.0,
hidden_feedback: false,
exit_code: 0,
}]
end
before do
allow_any_instance_of(LtiHelper).to receive(:lti_outcome_service?).and_return(lti_outcome_service?)
allow(submission).to receive(:calculate_score).and_return(calculate_response)
allow_any_instance_of(SubmissionsController).to receive(:send_scores).and_return(scoring_response)
click_button(I18n.t('exercises.editor.score'))
end
shared_context 'when full score reached' do
let(:score) { 1 }
end
shared_context 'when full score is not reached' do
let(:score) { 0 }
end
shared_context 'when scored without deadline' do
let(:deadline) { :none }
let(:score_sent) { score }
end
shared_context 'when scored before deadline' do
let(:deadline) { :before_deadline }
let(:score_sent) { score }
end
shared_context 'when scored within grace period' do
let(:deadline) { :within_grace_period }
let(:score_sent) { score * 0.8 }
end
shared_context 'when scored after late deadline' do
let(:deadline) { :after_late_deadline }
let(:score_sent) { score * 0 }
end
context 'when the LTI outcome service is supported' do
describe 'LTI failure' do
let(:users_success) { [] }
let(:users_error) { [contributor] }
let(:users_unsupported) { [] }
context 'when full score is reached' do
include_context 'when full score reached'
%w[without_deadline before_deadline within_grace_period after_late_deadline].each do |scenario|
context "when scored #{scenario.tr('_', ' ')}" do
include_context "when scored #{scenario.tr('_', ' ')}"
it_behaves_like 'no exercise finished notification'
it_behaves_like 'notification', 'exercises.editor.submit_failure_all'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_other_users'
it_behaves_like 'no notification', 'exercises.editor.submit_too_late'
end
end
end
context 'when full score is not reached' do
include_context 'when full score is not reached'
%w[without_deadline before_deadline within_grace_period after_late_deadline].each do |scenario|
context "when scored #{scenario.tr('_', ' ')}" do
include_context "when scored #{scenario.tr('_', ' ')}"
it_behaves_like 'no exercise finished notification'
it_behaves_like 'notification', 'exercises.editor.submit_failure_all'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_other_users'
it_behaves_like 'no notification', 'exercises.editor.submit_too_late'
end
end
end
end
describe 'LTI success' do
let(:users_success) { [contributor] }
let(:users_error) { [] }
let(:users_unsupported) { [] }
context 'when full score is reached' do
include_context 'when full score reached'
%w[without_deadline before_deadline].each do |scenario|
context "when scored #{scenario.tr('_', ' ')}" do
include_context "when scored #{scenario.tr('_', ' ')}"
it_behaves_like 'exercise finished notification'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_all'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_other_users'
it_behaves_like 'no notification', 'exercises.editor.submit_too_late'
end
end
%w[within_grace_period after_late_deadline].each do |scenario|
context "when scored #{scenario.tr('_', ' ')}" do
include_context "when scored #{scenario.tr('_', ' ')}"
it_behaves_like 'exercise finished notification'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_all'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_other_users'
it_behaves_like 'notification', 'exercises.editor.submit_too_late'
end
end
end
context 'when full score is not reached' do
include_context 'when full score is not reached'
%w[within_grace_period after_late_deadline].each do |scenario|
context "when scored #{scenario.tr('_', ' ')}" do
include_context "when scored #{scenario.tr('_', ' ')}"
it_behaves_like 'no exercise finished notification'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_all'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_other_users'
it_behaves_like 'notification', 'exercises.editor.submit_too_late'
end
end
end
end
describe 'LTI success for current contributor and failure for other' do
let(:users_success) { [contributor] }
let(:users_error) { [create(:external_user)] }
let(:users_unsupported) { [] }
context 'when full score is reached' do
include_context 'when full score reached'
%w[without_deadline before_deadline].each do |scenario|
context "when scored #{scenario.tr('_', ' ')}" do
include_context "when scored #{scenario.tr('_', ' ')}"
it_behaves_like 'exercise finished notification'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_all'
it_behaves_like 'notification', 'exercises.editor.submit_failure_other_users'
it_behaves_like 'no notification', 'exercises.editor.submit_too_late'
end
end
%w[within_grace_period after_late_deadline].each do |scenario|
context "when scored #{scenario.tr('_', ' ')}" do
include_context "when scored #{scenario.tr('_', ' ')}"
it_behaves_like 'exercise finished notification'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_all'
it_behaves_like 'notification', 'exercises.editor.submit_failure_other_users'
it_behaves_like 'notification', 'exercises.editor.submit_too_late'
end
end
end
context 'when full score is not reached' do
include_context 'when full score is not reached'
%w[within_grace_period after_late_deadline].each do |scenario|
context "when scored #{scenario.tr('_', ' ')}" do
include_context "when scored #{scenario.tr('_', ' ')}"
it_behaves_like 'no exercise finished notification'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_all'
it_behaves_like 'notification', 'exercises.editor.submit_failure_other_users'
it_behaves_like 'notification', 'exercises.editor.submit_too_late'
end
end
end
end
end
context 'when the LTI outcomes are not supported' do
let(:lti_outcome_service?) { false }
let(:users_success) { [] }
let(:users_error) { [] }
let(:users_unsupported) { [contributor] }
context 'when full score is reached' do
include_context 'when full score reached'
%w[without_deadline before_deadline within_grace_period after_late_deadline].each do |scenario|
context "when scored #{scenario.tr('_', ' ')}" do
include_context "when scored #{scenario.tr('_', ' ')}"
it_behaves_like 'exercise finished notification'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_all'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_other_users'
it_behaves_like 'no notification', 'exercises.editor.submit_too_late'
end
end
end
context 'when full score is not reached' do
include_context 'when full score is not reached'
%w[without_deadline before_deadline within_grace_period after_late_deadline].each do |scenario|
context "when scored #{scenario.tr('_', ' ')}" do
include_context "when scored #{scenario.tr('_', ' ')}"
it_behaves_like 'no exercise finished notification'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_all'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_other_users'
it_behaves_like 'no notification', 'exercises.editor.submit_too_late'
end
end
end
end
end
context 'when scoring is not successful' do
let(:score) { 0 }
context 'when the desired runner is already in use' do
before do
allow(submission).to receive(:calculate_score).and_raise(Runner::Error::RunnerInUse)
click_button(I18n.t('exercises.editor.score'))
end
it_behaves_like 'notification', 'exercises.editor.runner_in_use'
it_behaves_like 'no exercise finished notification'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_all'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_other_users'
it_behaves_like 'no notification', 'exercises.editor.submit_too_late'
end
context 'when no runner is available' do
before do
allow(submission).to receive(:calculate_score).and_raise(Runner::Error::NotAvailable)
click_button(I18n.t('exercises.editor.score'))
end
it_behaves_like 'notification', 'exercises.editor.depleted'
it_behaves_like 'no exercise finished notification'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_all'
it_behaves_like 'no notification', 'exercises.editor.submit_failure_other_users'
it_behaves_like 'no notification', 'exercises.editor.submit_too_late'
end
end
end

View File

@ -188,29 +188,6 @@ RSpec.describe ExercisePolicy do
end
end
permissions :submit? do
context 'when teacher-defined assessments are available' do
before do
create(:test_file, context: exercise)
exercise.reload
end
it 'grants access to anyone' do
%i[admin external_user teacher].each do |factory_name|
expect(policy).to permit(create(factory_name), exercise)
end
end
end
context 'when teacher-defined assessments are not available' do
it 'does not grant access to anyone' do
%i[admin external_user teacher].each do |factory_name|
expect(policy).not_to permit(create(factory_name), exercise)
end
end
end
end
describe ExercisePolicy::Scope do
describe '#resolve' do
let(:admin) { create(:admin) }

View File

@ -13,7 +13,7 @@ RSpec.describe SubmissionPolicy do
end
end
%i[download_file? render_file? run? score? show? statistics? stop? test?].each do |action|
%i[download? download_file? download_submission_file? render_file? run? score? show? statistics? stop? test? insights? finalize?].each do |action|
permissions(action) do
let(:exercise) { build(:math) }
let(:group_author) { build(:external_user) }