From fe2637838725f9271c6133bcbb837cf6e7ea11fa Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Wed, 1 Nov 2023 22:10:04 +0100 Subject: [PATCH] Extract ScoringChecks and rework RemoteEvaluations --- app/controllers/concerns/scoring_checks.rb | 52 +++++ .../remote_evaluation_controller.rb | 74 +++--- app/controllers/submissions_controller.rb | 53 +---- config/locales/de.yml | 5 +- config/locales/en.yml | 5 +- .../remote_evaluation_controller_spec.rb | 213 ++++++++++++++++++ spec/factories/remote_evaluation_mapping.rb | 22 ++ 7 files changed, 331 insertions(+), 93 deletions(-) create mode 100644 app/controllers/concerns/scoring_checks.rb create mode 100644 spec/controllers/remote_evaluation_controller_spec.rb create mode 100644 spec/factories/remote_evaluation_mapping.rb diff --git a/app/controllers/concerns/scoring_checks.rb b/app/controllers/concerns/scoring_checks.rb new file mode 100644 index 00000000..83de94b7 --- /dev/null +++ b/app/controllers/concerns/scoring_checks.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module ScoringChecks + def check_submission(submit_info) + lti_check = check_lti_transmission(submit_info[:users]) + # If we got a `:scoring_failure` from the LTI check, we want to display this message exclusively. + return [lti_check] if lti_check.present? && lti_check[:status] == :scoring_failure + + # Otherwise, the score was sent successfully for the current user, + # or it was not attempted for any user (i.e., no `lis_outcome_service` was available). + # In any way, we want to check for further conditions and return all messages. + [ + lti_check, + check_scoring_too_late(submit_info), + check_full_score, + ] + end + + private + + def check_full_score + # The submission was not scored with the full score, hence the exercise is not finished yet. + return unless @submission.full_score? + + {status: :exercise_finished, url: finalize_submission_path(@submission)} + end + + def check_lti_transmission(scored_users) + if scored_users[:all] == scored_users[:error] || scored_users[:error].include?(current_user) + # The score was not sent for any user or sending the score for the current user failed. + # In the latter case, we want to encourage the current user to reopen the exercise through the LMS. + # Hence, we always display the most severe error message. + {status: :scoring_failure} + elsif scored_users[:all] != scored_users[:success] && scored_users[:success].include?(current_user) + # The score was sent successfully for current user. + # However, at the same time, the transmission failed for some other users. + # This could either be due to a temporary network error, which is unlikely, or a more "permanent" error. + # Permanent errors would be that the deadline has passed on the LMS (which would then not provide a `lis_outcome_service`), + # working together with an internal user, or with someone who has never opened the exercise before. + {status: :not_for_all_users_submitted, failed_users: scored_users[:error].map(&:displayname).join(', ')} + end + end + + def check_scoring_too_late(submit_info) + # The submission was either performed before any deadline or no deadline was configured at all for the current exercise. + return if %i[within_grace_period after_late_deadline].exclude? submit_info[:deadline] + # The `lis_outcome_service` was not provided by the LMS, hence we were not able to send any score. + return if submit_info[:users][:unsupported].include?(current_user) + + {status: :scoring_too_late, score_sent: (submit_info[:score][:sent] * 100).to_i} + end +end diff --git a/app/controllers/remote_evaluation_controller.rb b/app/controllers/remote_evaluation_controller.rb index 057de1c0..c3026e15 100644 --- a/app/controllers/remote_evaluation_controller.rb +++ b/app/controllers/remote_evaluation_controller.rb @@ -1,8 +1,9 @@ # frozen_string_literal: true class RemoteEvaluationController < ApplicationController - include RemoteEvaluationParameters include Lti + include ScoringChecks + include RemoteEvaluationParameters skip_after_action :verify_authorized skip_before_action :verify_authenticity_token @@ -11,66 +12,55 @@ class RemoteEvaluationController < ApplicationController # POST /evaluate def evaluate result = create_and_score_submission('remoteAssess') - status = if result.is_a?(Hash) && result.key?(:status) - result[:status] - else - 201 - end - render json: result, status: + # For this route, we don't want to display the LTI result, but only the result of the submission. + try_lti if result.key?(:feedback) + render json: result.fetch(:feedback, result), status: result.fetch(:status, 201) end # POST /submit def submit result = create_and_score_submission('remoteSubmit') - status = 201 - if @submission.present? - score_achieved_percentage = @submission.normalized_score - result = try_lti - result[:score] = score_achieved_percentage * 100 unless result[:score] - status = result[:status] - end - - render json: result, status: + result = try_lti if result.key?(:feedback) + render json: result, status: result.fetch(:status, 201) end + private + def try_lti - # TODO: Need to consider and support programming groups - if !@submission.user.nil? && lti_outcome_service?(@submission.exercise, @submission.user, @submission.study_group_id) - lti_responses = send_scores(@submission) - process_lti_response(lti_responses.first) - else - { - message: "Your submission was successfully scored with #{@submission.normalized_score * 100}%. " \ - 'However, your score could not be sent to the e-Learning platform. Please check ' \ - 'the submission deadline, reopen the exercise through the e-Learning platform and try again.', - status: 410, - } - end - end - private :try_lti + lti_responses = send_scores(@submission) + check_lti_results = check_lti_transmission(lti_responses[:users]) || {} + score = (@submission.normalized_score * 100).to_i - def process_lti_response(lti_response) - if (lti_response[:status] == 'success') && (lti_response[:score_sent] != @submission.normalized_score) - # Score has been reduced due to the passed deadline - {message: I18n.t('exercises.editor.submit_too_late', score_sent: lti_response[:score_sent] * 100), status: 207, score: lti_response[:score_sent] * 100} - elsif lti_response[:status] == 'success' - {message: I18n.t('sessions.destroy_through_lti.success_with_outcome', consumer: @submission.user.consumer.name), status: 202} + # Since we are in an API context, we only want to return a **single** JSON response. + # For simplicity, we always return the most severe error message. + if lti_responses[:users][:all] == lti_responses[:users][:unsupported] + # No LTI transmission was attempted, i.e., no `lis_outcome_service` was provided by the LMS. + {message: I18n.t('exercises.editor.submit_failure_remote', score:), status: 410, score:} + elsif check_lti_results[:status] == :scoring_failure + {message: I18n.t('exercises.editor.submit_failure_all'), status: 424, score:} + elsif check_lti_results[:status] == :not_for_all_users_submitted + {message: I18n.t('exercises.editor.submit_failure_other_users', user: check_lti_results[:failed_users]), status: 417, score:} + elsif check_scoring_too_late(lti_responses).present? + score_sent = (lti_responses[:score][:sent] * 100).to_i + {message: I18n.t('exercises.editor.submit_too_late', score_sent:), status: 207, score: score_sent} + elsif check_full_score.present? + {message: I18n.t('exercises.editor.exercise_finished_remote', consumer: current_user.consumer.name), status: 200, score:} else - {message: I18n.t('exercises.editor.submit_failure_all'), status: 424} + {message: I18n.t('sessions.destroy_through_lti.success_with_outcome', consumer: current_user.consumer.name), status: 202, score:} end - # TODO: Delete LTI parameters? end - private :process_lti_response def create_and_score_submission(cause) validation_token = remote_evaluation_params[:validation_token] if (remote_evaluation_mapping = RemoteEvaluationMapping.find_by(validation_token:)) + @current_user = remote_evaluation_mapping.user @submission = Submission.create(build_submission_params(cause, remote_evaluation_mapping)) - @submission.calculate_score(remote_evaluation_mapping.user) + feedback = @submission.calculate_score(remote_evaluation_mapping.user) + {message: I18n.t('exercises.editor.run_success'), status: 201, feedback:} else # TODO: better output # TODO: check token expired? - {message: 'No exercise found for this validation_token! Please keep out!', status: 401} + {message: I18n.t('exercises.editor.submit_no_validation_token'), status: 401} end rescue Runner::Error::RunnerInUse => e Rails.logger.debug { "Scoring a submission failed because the runner was already in use: #{e.message}" } @@ -79,7 +69,6 @@ class RemoteEvaluationController < ApplicationController Rails.logger.debug { "Runner error while scoring submission #{@submission.id}: #{e.message}" } {message: I18n.t('exercises.editor.depleted'), status: 503} end - private :create_and_score_submission def build_submission_params(cause, remote_evaluation_mapping) Sentry.set_user( @@ -98,5 +87,4 @@ class RemoteEvaluationController < ApplicationController reject_illegal_file_attributes(remote_evaluation_mapping.exercise, files_attributes) submission_params end - private :build_submission_params end diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 64684b61..3f935810 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -5,6 +5,7 @@ class SubmissionsController < ApplicationController include FileConversion include Lti include RedirectBehavior + include ScoringChecks include SubmissionParameters include Tubesock::Hijack @@ -266,7 +267,10 @@ class SubmissionsController < ApplicationController # To enable hints when scoring a submission, uncomment the next line: # send_hints(client_socket, StructuredError.where(submission: @submission)) - transmit_lti_score(client_socket) + # Finally, send the score to the LTI consumer and check for notifications + check_submission(send_scores(@submission)).compact.each do |notification| + client_socket&.send_data(notification&.merge(cmd: :status)&.to_json) + end rescue Runner::Error::RunnerInUse => e extract_durations(e) send_and_store client_socket, {cmd: :status, status: :runner_in_use} @@ -481,53 +485,6 @@ class SubmissionsController < ApplicationController } end - def check_scoring_too_late(submit_info) - # The submission was either performed before any deadline or no deadline was configured at all for the current exercise. - return if %i[within_grace_period after_late_deadline].exclude? submit_info[:deadline] - # The `lis_outcome_service` was not provided by the LMS, hence we were not able to send any score. - return if submit_info[:users][:unsupported].include?(current_user) - - {status: :scoring_too_late, score_sent: submit_info[:score][:sent]} - end - - def check_full_score - # The submission was not scored with the full score, hence the exercise is not finished yet. - return unless @submission.full_score? - - {status: :exercise_finished, url: finalize_submission_path(@submission)} - end - - def transmit_lti_score(client_socket) - submit_info = send_scores(@submission) - scored_users = submit_info[:users] - - notifications = [] - if scored_users[:all] == scored_users[:error] || scored_users[:error].include?(current_user) - # The score was not sent for any user or sending the score for the current user failed. - # In the latter case, we want to encourage the current user to reopen the exercise through the LMS. - # Hence, we always display the most severe error message. - notifications << {status: :scoring_failure} - elsif scored_users[:all] != scored_users[:success] && scored_users[:success].include?(current_user) - # The score was sent successfully for current user. - # However, at the same time, the transmission failed for some other users. - # This could either be due to a temporary network error, which is unlikely, or a more "permanent" error. - # Permanent errors would be that the deadline has passed on the LMS (which would then not provide a `lis_outcome_service`), - # working together with an internal user, or with someone who has never opened the exercise before. - notifications << {status: :not_for_all_users_submitted, failed_users: scored_users[:error].map(&:displayname).join(', ')} - end - - if notifications.empty? || notifications.first[:status] != :scoring_failure - # Either, the score was sent successfully for the current user, - # or it was not attempted for any user (i.e., no `lis_outcome_service`). - notifications << check_scoring_too_late(submit_info) - notifications << check_full_score - end - - notifications.compact.each do |notification| - client_socket&.send_data(notification&.merge(cmd: :status)&.to_json) - end - end - def retrieve_message_from_output(data, stream) parsed = JSON.parse(data) if parsed.instance_of?(Hash) && parsed.key?('cmd') diff --git a/config/locales/de.yml b/config/locales/de.yml index 2c797885..a753b8c1 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -391,6 +391,7 @@ de: exercise_finished: Herzlichen Glückwunsch! Sie haben die Aufgabe vollständig gelöst. Klicken Sie hier, um die Aufgabe jetzt abzuschließen. exercise_finished_redirect_to_rfc: Herzlichen Glückwunsch! Sie haben die Aufgabe vollständig gelöst und die Punkte übertragen. Ein anderer Teilnehmer hat eine Frage zu der von Ihnen gelösten Aufgabe. Er würde sich sicherlich sehr über Ihre Hilfe und Kommentare freuen. exercise_finished_redirect_to_own_rfc: Herzlichen Glückwunsch! Sie haben die Aufgabe vollständig gelöst und die Punkte übertragen. Ihre Frage ist damit wahrscheinlich gelöst? Falls ja, fügen Sie doch den entscheidenden Kniff als Antwort hinzu und markieren die Frage als gelöst, bevor Sie das Fenster schließen. + exercise_finished_remote: Herzlichen Glückwunsch! Sie haben die Aufgabe vollständig gelöst und die Punkte erfolgreich an %{consumer} übertragen. expand_action_sidebar: Aktions-Leiste Ausklappen expand_output_sidebar: Ausgabe-Leiste Ausklappen input: Ihre Eingabe @@ -414,7 +415,9 @@ de: stop: Stoppen submit_failure_all: Beim Übermitteln Ihrer Punktzahl ist ein Fehler aufgetreten. Bitte versuchen Sie es später erneut. submit_failure_other_users: "Die Punkteübertragung war nur teilweise erfolgreich. Die Punkte konnten nicht für %{user} übertragen werden. Diese Person(en) sollte die Aufgabe über die e-Learning Platform erneut öffnen und anschließend die Punkte selbst übermitteln." - submit_too_late: Ihre Abgabe wurde erfolgreich gespeichert, ging jedoch nach der Abgabefrist ein, sodass nur %{score_sent} Punkte übertragen wurden. + submit_failure_remote: "Ihre Abgabe wurde erfolgreich mit %{score}% bewertet. Ihr Ergebnis konnte jedoch nicht an die E-Learning-Plattform gesendet werden. Bitte überprüfen Sie die Abgabefrist, öffnen Sie die Übung über die E-Learning-Plattform und versuchen Sie es erneut." + submit_no_validation_token: Keine Übung für dieses Validierungstoken gefunden! + submit_too_late: Ihre Abgabe wurde erfolgreich gespeichert, ging jedoch nach der Abgabefrist ein, sodass nur %{score_sent}% übertragen wurden. deadline: Deadline test: Testen timeout: 'Ausführung gestoppt. Ihr Code hat die erlaubte Ausführungszeit von %{permitted_execution_time} Sekunden überschritten.' diff --git a/config/locales/en.yml b/config/locales/en.yml index 5bf553d5..9ae0335f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -389,6 +389,7 @@ en: download: Download dummy: No Action exercise_finished: Congratulations! You have completely solved this exercise. Please click here to finish the exercise now. + exercise_finished_remote: Congratulations! You have completely solved this exercise and submitted the points to %{consumer}. exercise_finished_redirect_to_rfc: Congratulations! You have completely solved this exercise and submitted the points. Another participant has a question concerning the exercise you just solved. Your help and comments will be greatly appreciated! exercise_finished_redirect_to_own_rfc: Congratulations! You have completely solved this exercise and submitted the points. Your question concerning the exercise is solved? If so, please share the essential insight with your fellows and mark the question as solved, before you close this window! expand_action_sidebar: Expand Action Sidebar @@ -414,7 +415,9 @@ en: stop: Stop submit_failure_all: An error occurred while transmitting your score. Please try again later. submit_failure_other_users: "The transmission of points was only partially successful. The score was not transmitted for %{user}. The user(s) should reopen the exercise via the e-learning platform and then try to submit the points themselves." - submit_too_late: Your submission was saved successfully but was received after the deadline, so that only %{score_sent} points were transmitted. + submit_failure_remote: "Your submission was successfully scored with %{score}%. However, your score could not be sent to the e-learning platform. Please check the submission deadline, reopen the exercise through the e-learning platform, and try again." + submit_no_validation_token: No exercise found for this validation token! Please keep out! + submit_too_late: Your submission was saved successfully but was received after the deadline, so that only %{score_sent}% were transmitted. deadline: Deadline test: Test timeout: 'Execution stopped. Your code exceeded the permitted execution time of %{permitted_execution_time} seconds.' diff --git a/spec/controllers/remote_evaluation_controller_spec.rb b/spec/controllers/remote_evaluation_controller_spec.rb new file mode 100644 index 00000000..bd73e836 --- /dev/null +++ b/spec/controllers/remote_evaluation_controller_spec.rb @@ -0,0 +1,213 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe RemoteEvaluationController do + let(:contributor) { create(:external_user) } + let(:exercise) { create(:hello_world) } + let(:programming_group) { nil } + + let(:validation_token) { create(:remote_evaluation_mapping, user: contributor, exercise:).validation_token } + let(:files_attributes) { {'0': {file_id: exercise.files.find_by(role: 'main_file').id, content: ''}} } + let(:remote_evaluation_params) { {remote_evaluation: {validation_token:, files_attributes:}} } + + 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 + + shared_examples 'response' do |info| + it 'returns a message and a status' do + options = {} + options[:score_sent] = (score_sent * 100).to_i if defined? score_sent + options[:score] = (score * 100).to_i if defined? score + options[:user] = users_error.map(&:displayname).join(', ') if defined? users_error + options[:consumer] = contributor.consumer.name if defined? contributor + + expect(response.parsed_body.symbolize_keys[:message]).to eq(I18n.t(info[:message], **options)) + expect(response.parsed_body.symbolize_keys[:status]).to eq(info[:status]) + end + end + + before do + allow_any_instance_of(ApplicationController).to receive(:current_user).and_return(contributor) + end + + describe '#POST submit' do + let(:perform_request) { proc { post :submit, params: remote_evaluation_params } } + + 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 + + context 'when remote evaluation mapping is available' do + context 'when the scoring is successful' do + let(:score) { 1 } + let(:score_sent) { score } + let(:deadline) { :none } + + before do + allow_any_instance_of(Submission).to receive_messages(calculate_score: calculate_response, score:) + allow_any_instance_of(described_class).to receive(:send_scores).and_return(scoring_response) + perform_request.call + end + + context 'when no LTI transmission was attempted' do + let(:users_success) { [] } + let(:users_error) { [] } + let(:users_unsupported) { [contributor] } + + it_behaves_like 'response', {message: 'exercises.editor.submit_failure_remote', status: 410} + end + + context 'when transmission of points failed for all users' do + let(:users_success) { [] } + let(:users_error) { [contributor] } + let(:users_unsupported) { [] } + + it_behaves_like 'response', {message: 'exercises.editor.submit_failure_all', status: 424} + end + + context 'when transmission of points failed for some users' do + let(:users_success) { [contributor] } + let(:users_error) { [create(:external_user)] } + let(:users_unsupported) { [] } + + it_behaves_like 'response', {message: 'exercises.editor.submit_failure_other_users', status: 417} + end + + context 'when the scoring was too late' do + let(:users_success) { [contributor] } + let(:users_error) { [] } + let(:users_unsupported) { [] } + let(:deadline) { :within_grace_period } + let(:score_sent) { score * 0.8 } + + it_behaves_like 'response', {message: 'exercises.editor.submit_too_late', status: 207} + + it 'sends a reduced score' do + expect(response.parsed_body.symbolize_keys[:score]).to eq((score_sent * 100).to_i) + end + end + + context 'when transmission of points was successful' do + let(:users_success) { [contributor] } + let(:users_error) { [] } + let(:users_unsupported) { [] } + + context 'when exercise is finished' do + it_behaves_like 'response', {message: 'exercises.editor.exercise_finished_remote', status: 200} + end + + context 'when exercise is not finished' do + let(:score) { 0.5 } + + it_behaves_like 'response', {message: 'sessions.destroy_through_lti.success_with_outcome', status: 202} + end + end + end + + context 'when the scoring was not successful' do + let(:users_success) { [contributor] } + let(:users_error) { [] } + let(:users_unsupported) { [] } + + before do + allow_any_instance_of(Submission).to receive(:calculate_score).and_raise(error) + perform_request.call + end + + context 'when the desired runner is already in use' do + let(:error) { Runner::Error::RunnerInUse } + + it_behaves_like 'response', {message: 'exercises.editor.runner_in_use', status: 409} + end + + context 'when no runner is available' do + let(:error) { Runner::Error::NotAvailable } + + it_behaves_like 'response', {message: 'exercises.editor.depleted', status: 503} + end + end + end + + context 'when remote evaluation mapping is not available' do + let(:validation_token) { nil } + + before { perform_request.call } + + it_behaves_like 'response', {message: 'exercises.editor.submit_no_validation_token', status: 401} + end + end + + describe '#POST evaluate' do + let(:perform_request) { proc { post :evaluate, params: remote_evaluation_params } } + + context 'when remote evaluation mapping is available' do + context 'when the scoring is successful' do + let(:score) { 1 } + + before do + allow_any_instance_of(Submission).to receive_messages(calculate_score: calculate_response, score:) + perform_request.call + end + + it 'returns the feedback' do + expect(response.body).to eq(calculate_response.to_json) + end + end + + context 'when the scoring was not successful' do + let(:users_success) { [contributor] } + let(:users_error) { [] } + let(:users_unsupported) { [] } + + before do + allow_any_instance_of(Submission).to receive(:calculate_score).and_raise(error) + perform_request.call + end + + context 'when the desired runner is already in use' do + let(:error) { Runner::Error::RunnerInUse } + + it_behaves_like 'response', {message: 'exercises.editor.runner_in_use', status: 409} + end + + context 'when no runner is available' do + let(:error) { Runner::Error::NotAvailable } + + it_behaves_like 'response', {message: 'exercises.editor.depleted', status: 503} + end + end + end + + context 'when remote evaluation mapping is not available' do + let(:validation_token) { nil } + + before { perform_request.call } + + it_behaves_like 'response', {message: 'exercises.editor.submit_no_validation_token', status: 401} + end + end +end diff --git a/spec/factories/remote_evaluation_mapping.rb b/spec/factories/remote_evaluation_mapping.rb new file mode 100644 index 00000000..2ee4b226 --- /dev/null +++ b/spec/factories/remote_evaluation_mapping.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :remote_evaluation_mapping, class: 'RemoteEvaluationMapping' do + created_by_external_user + validation_token { SecureRandom.urlsafe_base64 } + exercise factory: :math + + after(:create) do |remote_evaluation_mapping| + # Do not change anything if a study group was provided explicitly or user has no study groups + unless remote_evaluation_mapping.study_group_id.present? || remote_evaluation_mapping.user.study_groups.blank? + remote_evaluation_mapping.update!(study_group_id: remote_evaluation_mapping.user.study_groups.first.id) + end + + pg = remote_evaluation_mapping.user.programming_groups.find_by(exercise: remote_evaluation_mapping.exercise) + # Do not change anything if a programming group was provided explicitly or user has no programming group + unless remote_evaluation_mapping.programming_group_id.present? || pg.blank? + remote_evaluation_mapping.update!(programming_group_id: pg.id) + end + end + end +end