From 498d2fa5a842d099d07b6e3a93dbc612c04e8cf4 Mon Sep 17 00:00:00 2001 From: kkoehn Date: Mon, 22 Jul 2024 21:57:00 +0200 Subject: [PATCH] task import: handle errors during import better (#1439) * task import: handle errors during import better * fix flash messages, bump proforma version --- app/assets/javascripts/flash_message.coffee | 3 +- app/assets/javascripts/tasks_import.coffee | 3 -- app/controllers/tasks_controller.rb | 35 +++++++++++++-- config/locales/de/controllers/tasks.yml | 4 +- config/locales/de/proforma_errors.yml | 6 +++ config/locales/en/controllers/tasks.yml | 4 +- config/locales/en/proforma_errors.yml | 6 +++ spec/controllers/tasks_controller_spec.rb | 47 ++++++++++++++++++--- 8 files changed, 91 insertions(+), 17 deletions(-) create mode 100644 config/locales/de/proforma_errors.yml create mode 100644 config/locales/en/proforma_errors.yml diff --git a/app/assets/javascripts/flash_message.coffee b/app/assets/javascripts/flash_message.coffee index b4bc625f7..e51048ac4 100644 --- a/app/assets/javascripts/flash_message.coffee +++ b/app/assets/javascripts/flash_message.coffee @@ -1,7 +1,7 @@ show_ajax_message = (msg, type) -> cls = 'alert-success' if type == 'notice' cls = 'alert-danger' if type == 'alert' - $("#flash-message").html "
#{msg}
" + $("#flash-message").html "

#{decodeURIComponent(msg)}

" $("#flash-#{type}").delay(6000).slideUp 'medium' $(document).ajaxComplete (event, request) -> @@ -12,5 +12,4 @@ $(document).ajaxComplete (event, request) -> ready = -> $('#flash-message').children().first().delay(5000).slideUp 'medium' - $(document).on('turbolinks:load', ready) diff --git a/app/assets/javascripts/tasks_import.coffee b/app/assets/javascripts/tasks_import.coffee index 96f443297..46a266174 100644 --- a/app/assets/javascripts/tasks_import.coffee +++ b/app/assets/javascripts/tasks_import.coffee @@ -34,9 +34,6 @@ importStart = () -> data: formData, processData: false, contentType: false, - success: (response) -> - if response.status == 'failure' - alert(response.message) error: (_xhr, _textStatus, message) -> alert("#{I18n.t('common.javascripts.error')}: #{message}") }) diff --git a/app/controllers/tasks_controller.rb b/app/controllers/tasks_controller.rb index de5430995..0b9120a4d 100644 --- a/app/controllers/tasks_controller.rb +++ b/app/controllers/tasks_controller.rb @@ -89,7 +89,7 @@ def download send_data(zip_file.string, type: 'application/zip', filename: "task_#{@task.id}.zip", disposition: 'attachment') end - def import_start + def import_start # rubocop:disable Metric/AbcSize, Metrics/MethodLength zip_file = params[:zip_file] unless zip_file.is_a?(ActionDispatch::Http::UploadedFile) return render json: {status: 'failure', message: t('.choose_file_error')} @@ -100,9 +100,24 @@ def import_start respond_to do |format| format.js { render layout: false } end + rescue ProformaXML::ProformaError => e + messages = prettify_import_errors(e) + flash[:alert] = messages + render json: { + status: 'failure', + message: t('.error', error: messages), + actions: '', + } + rescue StandardError => e + Sentry.capture_exception(e) + render json: { + status: 'failure', + message: t('tasks.import.internal_error'), + actions: '', + } end - def import_confirm + def import_confirm # rubocop:disable Metric/AbcSize proforma_task = ProformaService::ProformaTaskFromCachedFile.call(**import_confirm_params.to_hash.symbolize_keys) task = ProformaService::ImportTask.call(proforma_task:, user: current_user) @@ -117,6 +132,13 @@ def import_confirm message: t('.error', title: proforma_task.title, error: e.message), actions: '', } + rescue StandardError => e + Sentry.capture_exception(e) + render json: { + status: 'failure', + message: t('tasks.import.internal_error'), + actions: '', + } end def import_uuid_check @@ -137,7 +159,7 @@ def import_external render json: t('.invalid'), status: :bad_request rescue StandardError => e Sentry.capture_exception(e) - render json: t('.internal_error'), status: :internal_server_error + render json: t('tasks.import.internal_error'), status: :internal_server_error end def export_external_start @@ -199,6 +221,13 @@ def generate_test private + def prettify_import_errors(error) + message = "#{t('proforma_errors.import')}
" + message + JSON.parse(error.message).map do |msg| + t("proforma_errors.#{msg}", default: msg) + end.join('
') + end + def load_and_authorize_task @task = Task.find(params[:id]) authorize @task diff --git a/config/locales/de/controllers/tasks.yml b/config/locales/de/controllers/tasks.yml index 60708c2bf..f41b16df0 100644 --- a/config/locales/de/controllers/tasks.yml +++ b/config/locales/de/controllers/tasks.yml @@ -9,15 +9,17 @@ de: export_external_confirm: error: 'Der Export der Aufgabe (%{title}) ist fehlgeschlagen.
Fehler: %{error}' success: Aufgabe (%{title}) erfolgreich exportiert. + import: + internal_error: Beim Import dieser Aufgabe ist auf CodeHarbor ein interner Fehler aufgetreten. import_confirm: error: 'Der Import der Aufgabe (%{title}) ist fehlgeschlagen.
Fehler: %{error}' success: Aufgabe (%{title}) erfolgreich importiert. import_external: - internal_error: Bei der Importierung der Aufgabe ist ein interner Fehler aufgetreten. invalid: Ungültige Aufgabe success: Erfolg import_start: choose_file_error: Sie müssen eine Datei auswählen. + error: 'Der Import konnte nicht gestartet werden.
Fehler: %{error}' proforma_service: convert_zip_to_proforma_tasks: nested_too_deep: Die ZIP-Datei ist zu tief verschachtelt. diff --git a/config/locales/de/proforma_errors.yml b/config/locales/de/proforma_errors.yml new file mode 100644 index 000000000..52cdae435 --- /dev/null +++ b/config/locales/de/proforma_errors.yml @@ -0,0 +1,6 @@ +--- +de: + proforma_errors: + import: Es ist ein Fehler beim Importieren der ProformaXML ZIP-Datei aufgetreten. + no task_xml found: Es konnte keine task.xml in der ZIP-Datei gefunden werden. + version not supported: Die Version dieses ProformaXML Dokuments wird nicht unterstützt. diff --git a/config/locales/en/controllers/tasks.yml b/config/locales/en/controllers/tasks.yml index 319d6c9e9..208cc48de 100644 --- a/config/locales/en/controllers/tasks.yml +++ b/config/locales/en/controllers/tasks.yml @@ -9,15 +9,17 @@ en: export_external_confirm: error: 'Export of task (%{title}) failed.
Error: %{error}' success: Task (%{title}) successfully exported. + import: + internal_error: An internal error occurred on CodeHarbor while importing the exercise. import_confirm: error: 'Import of task (%{title}) failed.
Error: %{error}' success: Task (%{title}) successfully imported. import_external: - internal_error: An internal error occurred on CodeHarbor while importing the exercise. invalid: Invalid exercise success: success import_start: choose_file_error: You need to choose a file. + error: 'Import of task could not be started.
Error: %{error}' proforma_service: convert_zip_to_proforma_tasks: nested_too_deep: ZIP is nested too deep. diff --git a/config/locales/en/proforma_errors.yml b/config/locales/en/proforma_errors.yml new file mode 100644 index 000000000..20144fa4c --- /dev/null +++ b/config/locales/en/proforma_errors.yml @@ -0,0 +1,6 @@ +--- +en: + proforma_errors: + import: An error occurred while importing your ProformaXML ZIP file. + no task_xml found: No task.xml could be found in the supplied ZIP file. + version not supported: The version of this ProformaXML document is not supported. diff --git a/spec/controllers/tasks_controller_spec.rb b/spec/controllers/tasks_controller_spec.rb index 42051852a..997795e89 100644 --- a/spec/controllers/tasks_controller_spec.rb +++ b/spec/controllers/tasks_controller_spec.rb @@ -722,7 +722,7 @@ context 'when zip_file is submitted' do let(:zip_file) {} - it 'renders correct json' do + it 'renders correct JSON' do post_request expect(JSON.parse(response.body, symbolize_names: true)).to eql({status: 'failure', message: 'You need to choose a file.'}) end @@ -731,11 +731,33 @@ context "when zip_file is 'undefined'" do let(:zip_file) { 'undefined' } - it 'renders correct json' do + it 'renders correct JSON' do post_request expect(JSON.parse(response.body, symbolize_names: true)).to eql({status: 'failure', message: 'You need to choose a file.'}) end end + + context 'when service throws version not supported error' do + before do + allow(ProformaService::CacheImportFile).to receive(:call).and_raise(ProformaXML::ProformaError.new(['version not supported'])) + end + + it 'renders correct JSON' do + post_request + expect(JSON.parse(response.body, symbolize_names: true)).to eql({status: 'failure', message: 'Import of task could not be started.
Error: An error occurred while importing your ProformaXML ZIP file.
The version of this ProformaXML document is not supported.', actions: ''}) + end + end + + context 'when service throws an unexpected error' do + before do + allow(ProformaService::CacheImportFile).to receive(:call).and_raise(StandardError) + end + + it 'renders correct JSON' do + post_request + expect(JSON.parse(response.body, symbolize_names: true)).to eql({status: 'failure', message: 'An internal error occurred on CodeHarbor while importing the exercise.', actions: ''}) + end + end end describe 'POST #import_confirm' do @@ -757,7 +779,7 @@ expect { post_request }.to change(Task, :count).by(1) end - it 'renders correct json' do + it 'renders correct JSON' do post_request expect(response.body).to include('successfully imported').and(include(I18n.t('tasks.import_actions.button.show_task')).and(include('Hide'))) end @@ -765,11 +787,22 @@ context 'when import raises a validation error' do before { allow(ProformaService::ImportTask).to receive(:call).and_raise(ActiveRecord::RecordInvalid) } - it 'renders correct json' do + it 'renders correct JSON' do post_request expect(response.body).to include('failed').and(include('Record invalid').and(include('"actions":""'))) end end + + context 'when service throws an unexpected error' do + before do + allow(ProformaService::ImportTask).to receive(:call).and_raise(StandardError) + end + + it 'renders correct JSON' do + post_request + expect(JSON.parse(response.body, symbolize_names: true)).to eql({status: 'failure', message: 'An internal error occurred on CodeHarbor while importing the exercise.', actions: ''}) + end + end end context 'when signed out' do @@ -910,7 +943,7 @@ let(:uuid_found) { true } let(:error) { nil } - it 'renders the correct contents as json' do + it 'renders the correct contents as JSON' do post_request expect(response.parsed_body.symbolize_keys[:message]).to eq('message') expect(response.parsed_body.symbolize_keys[:actions]).to( @@ -924,7 +957,7 @@ context 'when there is an error' do let(:error) { 'error' } - it 'renders the correct contents as json' do + it 'renders the correct contents as JSON' do post_request expect(response.parsed_body.symbolize_keys[:message]).to eq('message') expect(response.parsed_body.symbolize_keys[:actions]).to( @@ -939,7 +972,7 @@ context 'when uuid_found is false' do let(:uuid_found) { false } - it 'renders the correct contents as json' do + it 'renders the correct contents as JSON' do post_request expect(response.parsed_body.symbolize_keys[:message]).to eq('message') expect(response.parsed_body.symbolize_keys[:actions]).to(