From 21c1a720267d934b9580c04d507f9b2329d207e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karel=20Pi=C4=8Dman?= Date: Tue, 14 Jan 2020 15:38:56 +0100 Subject: [PATCH] Errors handling --- app/controllers/dmsf_controller.rb | 32 +++++++++---------- app/controllers/dmsf_files_controller.rb | 26 +++++++-------- app/controllers/dmsf_files_copy_controller.rb | 4 +-- .../dmsf_folders_copy_controller.rb | 4 +-- app/controllers/dmsf_links_controller.rb | 6 ++-- app/controllers/dmsf_state_controller.rb | 2 +- app/controllers/dmsf_workflows_controller.rb | 28 ++++++++-------- app/helpers/dmsf_upload_helper.rb | 2 +- app/views/dmsf_upload/upload.js.erb | 2 +- .../controllers/issues_controller_hooks.rb | 8 ++--- test/functional/dmsf_controller_test.rb | 27 ++++++++-------- test/functional/dmsf_files_controller_test.rb | 10 +++--- 12 files changed, 76 insertions(+), 75 deletions(-) diff --git a/app/controllers/dmsf_controller.rb b/app/controllers/dmsf_controller.rb index 7e3e16ba..425f3000 100644 --- a/app/controllers/dmsf_controller.rb +++ b/app/controllers/dmsf_controller.rb @@ -89,14 +89,14 @@ class DmsfController < ApplicationController def download_email_entries # IE has got a tendency to cache files - expires_in(0.year, "must-revalidate" => true) + expires_in(0.year, 'must-revalidate' => true) send_file( params[:path], - :filename => 'Documents.zip', - :type => 'application/zip', - :disposition => 'attachment') + filename: 'Documents.zip', + type: 'application/zip', + disposition: 'attachment') rescue => e - flash[:errors] = e.message + flash[:error] = e.message end def entries_operation @@ -153,7 +153,7 @@ class DmsfController < ApplicationController rescue DmsfAccessError render_403 rescue StandardError => e - flash[:errors] = e.message + flash[:error] = e.message Rails.logger.error e.message end end @@ -170,7 +170,7 @@ class DmsfController < ApplicationController def entries_email if params[:email][:to].strip.blank? - flash[:errors] = l(:error_email_to_must_be_entered) + flash[:error] = l(:error_email_to_must_be_entered) else DmsfMailer.deliver_send_documents(@project, params[:email].permit!, User.current) File.delete(params[:email][:zipped_content]) @@ -249,7 +249,7 @@ class DmsfController < ApplicationController if result flash[:notice] = l(:notice_folder_deleted) else - flash[:errors] = @folder.errors.full_messages.to_sentence + flash[:error] = @folder.errors.full_messages.to_sentence end respond_to do |format| format.html do @@ -267,7 +267,7 @@ class DmsfController < ApplicationController if @folder.restore flash[:notice] = l(:notice_dmsf_folder_restored) else - flash[:errors] = @folder.errors.full_messages.to_sentence + flash[:error] = @folder.errors.full_messages.to_sentence end redirect_to :back end @@ -281,7 +281,7 @@ class DmsfController < ApplicationController if @project.save flash[:notice] = l(:notice_folder_details_were_saved) else - flash[:errors] = @project.errors.full_messages.to_sentence + flash[:error] = @project.errors.full_messages.to_sentence end end redirect_to dmsf_folder_path(:id => @project) @@ -339,7 +339,7 @@ class DmsfController < ApplicationController @folder.unlock! flash[:notice] = l(:notice_folder_unlocked) else - flash[:errors] = l(:error_only_user_that_locked_folder_can_unlock_it) + flash[:error] = l(:error_only_user_that_locked_folder_can_unlock_it) end end redirect_to :back @@ -455,7 +455,7 @@ class DmsfController < ApplicationController folder = DmsfFolder.find_by(id: id) if folder unless folder.restore - flash[:errors] = folder.errors.full_messages.to_sentence + flash[:error] = folder.errors.full_messages.to_sentence end else raise FileNotFound @@ -466,7 +466,7 @@ class DmsfController < ApplicationController file = DmsfFile.find_by(id: id) if file unless file.restore - flash[:errors] = file.errors.full_messages.to_sentence + flash[:error] = file.errors.full_messages.to_sentence end else raise FileNotFound @@ -477,7 +477,7 @@ class DmsfController < ApplicationController link = DmsfLink.find_by(id: id) if link unless link.restore - flash[:errors] = link.errors.full_messages.to_sentence + flash[:error] = link.errors.full_messages.to_sentence end else raise FileNotFound @@ -492,7 +492,7 @@ class DmsfController < ApplicationController folder = DmsfFolder.find_by(id: id) if folder unless folder.delete commit - flash[:errors] = folder.errors.full_messages.to_sentence + flash[:error] = folder.errors.full_messages.to_sentence return end elsif !commit @@ -544,7 +544,7 @@ class DmsfController < ApplicationController link = DmsfLink.find_by(id: id) link.delete commit if link end - if flash[:errors].blank? && flash[:warning].blank? + if flash[:error].blank? && flash[:warning].blank? flash[:notice] = l(:notice_entries_deleted) end end diff --git a/app/controllers/dmsf_files_controller.rb b/app/controllers/dmsf_files_controller.rb index eb7feb17..922ccce2 100644 --- a/app/controllers/dmsf_files_controller.rb +++ b/app/controllers/dmsf_files_controller.rb @@ -96,7 +96,7 @@ class DmsfFilesController < ApplicationController def create_revision if params[:dmsf_file_revision] if @file.locked_for_user? - flash[:errors] = l(:error_file_is_locked) + flash[:error] = l(:error_file_is_locked) else revision = DmsfFileRevision.new revision.title = params[:dmsf_file_revision][:title] @@ -186,10 +186,10 @@ class DmsfFilesController < ApplicationController Rails.logger.error "Could not send email notifications: #{e.message}" end else - flash[:errors] = @file.errors.full_messages.join(', ') + flash[:error] = @file.errors.full_messages.to_sentence end else - flash[:errors] = revision.errors.full_messages.join(', ') + flash[:error] = revision.errors.full_messages.to_sentence end end end @@ -217,8 +217,8 @@ class DmsfFilesController < ApplicationController end end else - msg = @file.errors.full_messages.join(', ') - flash[:errors] = msg + msg = @file.errors.full_messages.to_sentence + flash[:error] = msg Rails.logger.error msg end end @@ -243,10 +243,10 @@ class DmsfFilesController < ApplicationController end flash[:notice] = l(:notice_revision_deleted) else - flash[:errors] = @revision.errors.full_messages.join(', ') + flash[:error] = @revision.errors.full_messages.to_sentence end end - redirect_to :action => 'show', :id => @file + redirect_to action: 'show', id: @file end def obsolete_revision @@ -254,10 +254,10 @@ class DmsfFilesController < ApplicationController if @revision.obsolete flash[:notice] = l(:notice_revision_obsoleted) else - flash[:errors] = @revision.errors.full_messages.join(', ') + flash[:error] = @revision.errors.full_messages.to_sentence end end - redirect_to :action => 'show', :id => @file + redirect_to action: 'show', id: @file end def lock @@ -268,7 +268,7 @@ class DmsfFilesController < ApplicationController @file.lock! flash[:notice] = l(:notice_file_locked) rescue => e - flash[:errors] = e.message + flash[:error] = e.message end end redirect_to :back @@ -283,10 +283,10 @@ class DmsfFilesController < ApplicationController @file.unlock! flash[:notice] = l(:notice_file_unlocked) rescue => e - flash[:errors] = e.message + flash[:error] = e.message end else - flash[:errors] = l(:error_only_user_that_locked_file_can_unlock_it) + flash[:error] = l(:error_only_user_that_locked_file_can_unlock_it) end end redirect_to :back @@ -316,7 +316,7 @@ class DmsfFilesController < ApplicationController if @file.restore flash[:notice] = l(:notice_dmsf_file_restored) else - flash[:errors] = @file.errors.full_messages.to_sentence + flash[:error] = @file.errors.full_messages.to_sentence end redirect_to :back end diff --git a/app/controllers/dmsf_files_copy_controller.rb b/app/controllers/dmsf_files_copy_controller.rb index 1ea816ac..a58cc083 100644 --- a/app/controllers/dmsf_files_copy_controller.rb +++ b/app/controllers/dmsf_files_copy_controller.rb @@ -39,7 +39,7 @@ class DmsfFilesCopyController < ApplicationController def copy new_file = @file.copy_to(@target_project, @target_folder) unless new_file.errors.empty? - flash[:error] = new_file.errors.full_messages.join(', ') + flash[:error] = new_file.errors.full_messages.to_sentence redirect_to :action => 'new', :id => @file, :target_project_id => @target_project, :target_folder_id => @target_folder return @@ -50,7 +50,7 @@ class DmsfFilesCopyController < ApplicationController def move unless @file.move_to(@target_project, @target_folder) - flash[:error] = @file.errors.full_messages.join(', ') + flash[:error] = @file.errors.full_messages.to_sentence redirect_to :action => 'new', :id => @file, :target_project_id => @target_project, :target_folder_id => @target_folder return diff --git a/app/controllers/dmsf_folders_copy_controller.rb b/app/controllers/dmsf_folders_copy_controller.rb index 59b7372d..9d54e40b 100644 --- a/app/controllers/dmsf_folders_copy_controller.rb +++ b/app/controllers/dmsf_folders_copy_controller.rb @@ -39,7 +39,7 @@ class DmsfFoldersCopyController < ApplicationController def copy new_folder = @folder.copy_to(@target_project, @target_folder) unless new_folder.errors.empty? - flash[:error] = new_folder.errors.full_messages.join(', ') + flash[:error] = new_folder.errors.full_messages.to_sentence redirect_to :action => 'new', :id => @folder, :target_project_id => @target_project, :target_folder_id => @target_folder return @@ -55,7 +55,7 @@ class DmsfFoldersCopyController < ApplicationController flash[:notice] = l(:notice_successful_update) redirect_to dmsf_folder_path(:id => @target_project, :folder_id => @folder) else - flash[:error] = @folder.errors.full_messages.join(', ') + flash[:error] = @folder.errors.full_messages.to_sentence redirect_to :action => 'new', :id => @folder, :target_project_id => @target_project, :target_folder_id => @target_folder end diff --git a/app/controllers/dmsf_links_controller.rb b/app/controllers/dmsf_links_controller.rb index 51691da8..6bb20509 100644 --- a/app/controllers/dmsf_links_controller.rb +++ b/app/controllers/dmsf_links_controller.rb @@ -114,7 +114,7 @@ class DmsfLinksController < ApplicationController flash[:notice] = l(:notice_successful_create) else msg = @dmsf_link.errors.full_messages.to_sentence - flash[:errors] = msg + flash[:error] = msg Rails.logger.error msg end else @@ -144,7 +144,7 @@ class DmsfLinksController < ApplicationController if result flash[:notice] = l(:notice_successful_create) else - flash[:errors] = @dmsf_link.errors.full_messages.to_sentence + flash[:error] = @dmsf_link.errors.full_messages.to_sentence end end respond_to do |format| @@ -172,7 +172,7 @@ class DmsfLinksController < ApplicationController flash[:notice] = l(:notice_successful_delete) else @dmsf_link.errors.each do |e, msg| - flash[:errors] = msg + flash[:error] = msg end end end diff --git a/app/controllers/dmsf_state_controller.rb b/app/controllers/dmsf_state_controller.rb index 2c5859df..b8ae9d41 100644 --- a/app/controllers/dmsf_state_controller.rb +++ b/app/controllers/dmsf_state_controller.rb @@ -36,7 +36,7 @@ class DmsfStateController < ApplicationController if format_valid?(member.dmsf_title_format) && member.save flash[:notice] = l(:notice_your_preferences_were_saved) else - flash[:errors] = l(:notice_your_preferences_were_not_saved) + flash[:error] = l(:notice_your_preferences_were_not_saved) end else flash[:warning] = l(:user_is_not_project_member) diff --git a/app/controllers/dmsf_workflows_controller.rb b/app/controllers/dmsf_workflows_controller.rb index ce3de218..80aea615 100644 --- a/app/controllers/dmsf_workflows_controller.rb +++ b/app/controllers/dmsf_workflows_controller.rb @@ -204,12 +204,12 @@ class DmsfWorkflowsController < ApplicationController flash[:notice] = l(:notice_successful_update) end else - flash[:errors] = l(:error_workflow_assign) + flash[:error] = l(:error_workflow_assign) end end end rescue => e - flash[:errors] = e.message + flash[:error] = e.message end redirect_to :back return @@ -289,12 +289,12 @@ class DmsfWorkflowsController < ApplicationController if res flash[:notice] = l(:notice_successful_update) if @project - redirect_to settings_project_path(@project, :tab => 'dmsf_workflow') + redirect_to settings_project_path(@project, tab: 'dmsf_workflow') else redirect_to dmsf_workflows_path end else - flash[:errors] = @dmsf_workflow.errors.full_messages.to_sentence + flash[:error] = @dmsf_workflow.errors.full_messages.to_sentence redirect_to dmsf_workflow_path(@dmsf_workflow) end else @@ -307,10 +307,10 @@ class DmsfWorkflowsController < ApplicationController @dmsf_workflow.destroy flash[:notice] = l(:notice_successful_delete) rescue - flash[:errors] = l(:error_unable_delete_dmsf_workflow) + flash[:error] = l(:error_unable_delete_dmsf_workflow) end if @project - redirect_to settings_project_path(@project, :tab => 'dmsf_workflow') + redirect_to settings_project_path(@project, tab: 'dmsf_workflow') else redirect_to dmsf_workflows_path end @@ -351,11 +351,11 @@ class DmsfWorkflowsController < ApplicationController if ws.save @dmsf_workflow.dmsf_workflow_steps << ws else - flash[:errors] = ws.errors.full_messages.to_sentence + flash[:error] = ws.errors.full_messages.to_sentence end end else - flash[:errors] = l(:error_workflow_assign) + flash[:error] = l(:error_workflow_assign) end end respond_to do |format| @@ -373,7 +373,7 @@ class DmsfWorkflowsController < ApplicationController if n > params[:step].to_i ws.step = n - 1 unless ws.save - flash[:errors] = l(:notice_cannot_renumber_steps) + flash[:error] = l(:notice_cannot_renumber_steps) end end end @@ -384,7 +384,7 @@ class DmsfWorkflowsController < ApplicationController def reorder_steps if request.put? unless @dmsf_workflow.reorder_steps(params[:step].to_i, params[:dmsf_workflow][:position].to_i) - flash[:errors] = l(:notice_cannot_renumber_steps) + flash[:error] = l(:notice_cannot_renumber_steps) end end respond_to do |format| @@ -403,7 +403,7 @@ class DmsfWorkflowsController < ApplicationController @dmsf_workflow.notify_users(@project, revision, self) flash[:notice] = l(:notice_workflow_started) else - flash[:errors] = l(:notice_cannot_start_workflow) + flash[:error] = l(:notice_cannot_start_workflow) end end redirect_to :back @@ -419,11 +419,11 @@ class DmsfWorkflowsController < ApplicationController @dmsf_workflow.dmsf_workflow_steps.where(step: step.step).find_each do |s| s.name = step.name unless s.save - flash[:errors] = s.errors.full_messages.to_sentence + flash[:error] = s.errors.full_messages.to_sentence end end else - flash[:errors] = step.errors.full_messages.to_sentence + flash[:error] = step.errors.full_messages.to_sentence end end # Operators/Assignees @@ -434,7 +434,7 @@ class DmsfWorkflowsController < ApplicationController step.operator = operator.to_i step.user_id = params[:assignee][id] unless step.save - flash[:errors] = step.errors.full_messages.to_sentence + flash[:error] = step.errors.full_messages.to_sentence Rails.logger.error step.errors.full_messages.to_sentence end end diff --git a/app/helpers/dmsf_upload_helper.rb b/app/helpers/dmsf_upload_helper.rb index e1b2b0d7..49237d51 100644 --- a/app/helpers/dmsf_upload_helper.rb +++ b/app/helpers/dmsf_upload_helper.rb @@ -112,7 +112,7 @@ module DmsfUploadHelper end rescue => e Rails.logger.error e.message - controller.flash[:errors] = e.message if controller + controller.flash[:error] = e.message if controller failed_uploads.push(file) end else diff --git a/app/views/dmsf_upload/upload.js.erb b/app/views/dmsf_upload/upload.js.erb index e99533a1..19279d3a 100644 --- a/app/views/dmsf_upload/upload.js.erb +++ b/app/views/dmsf_upload/upload.js.erb @@ -23,7 +23,7 @@ var fileSpan = $('#dmsf_attachments_<%= j params[:attachment_id] %>'); <% if @attachment.new_record? %> fileSpan.hide(); - alert("<%= escape_javascript @attachment.errors.full_messages.join(', ') %>"); + alert("<%= escape_javascript @attachment.errors.full_messages.to_sentence %>"); <% else %> $('', { type: 'hidden', name: 'dmsf_attachments[<%= j params[:attachment_id] %>][token]' } ).val('<%= j @attachment.token %>').appendTo(fileSpan); fileSpan.find('a.dmsf-remove-upload') diff --git a/lib/redmine_dmsf/hooks/controllers/issues_controller_hooks.rb b/lib/redmine_dmsf/hooks/controllers/issues_controller_hooks.rb index 527ae744..833e3655 100644 --- a/lib/redmine_dmsf/hooks/controllers/issues_controller_hooks.rb +++ b/lib/redmine_dmsf/hooks/controllers/issues_controller_hooks.rb @@ -91,7 +91,7 @@ module RedmineDmsf if old_system_folder old_system_folder.title = "#{issue.id} - #{DmsfFolder::get_valid_title(issue.subject)}" unless old_system_folder.save - controller.flash[:errors] = old_system_folder.errors.full_messages.to_sentence + controller.flash[:error] = old_system_folder.errors.full_messages.to_sentence Rails.logger.error old_system_folder.errors.full_messages.to_sentence end end @@ -103,20 +103,20 @@ module RedmineDmsf old_system_folder.dmsf_folder_id = new_main_system_folder.id old_system_folder.project_id = project_id unless old_system_folder.save - controller.flash[:errors] = old_system_folder.errors.full_messages.to_sentence + controller.flash[:error] = old_system_folder.errors.full_messages.to_sentence Rails.logger.error old_system_folder.errors.full_messages.to_sentence end issue.dmsf_files.each do |dmsf_file| dmsf_file.project_id = project_id unless dmsf_file.save - controller.flash[:errors] = dmsf_file.errors.full_messages.to_sentence + controller.flash[:error] = dmsf_file.errors.full_messages.to_sentence Rails.logger.error dmsf_file.errors.full_messages.to_sentence end end issue.dmsf_links.each do | dmsf_link| dmsf_link.project_id = project_id unless dmsf_link.save - controller.flash[:errors] = dmsf_link.errors.full_messages.to_sentence + controller.flash[:error] = dmsf_link.errors.full_messages.to_sentence Rails.logger.error dmsf_link.errors.full_messages.to_sentence end end diff --git a/test/functional/dmsf_controller_test.rb b/test/functional/dmsf_controller_test.rb index ec0f00b4..5d55bd88 100644 --- a/test/functional/dmsf_controller_test.rb +++ b/test/functional/dmsf_controller_test.rb @@ -133,9 +133,9 @@ class DmsfControllerTest < RedmineDmsf::Test::TestCase def test_delete_not_empty # Permissions OK but the folder is not empty @role.add_permission! :folder_manipulation - get :delete, :params => {:id => @project, :folder_id => @folder1.id, :commit => false} + get :delete, params: { id: @project, folder_id: @folder1.id, commit: false} assert_response :redirect - assert_include l(:error_folder_is_not_empty), flash[:errors] + assert_include l(:error_folder_is_not_empty), flash[:error] end def test_delete_locked @@ -144,7 +144,7 @@ class DmsfControllerTest < RedmineDmsf::Test::TestCase @request.env['HTTP_REFERER'] = dmsf_folder_path(id: @project.id, folder_id: @folder2.id) get :delete, params: { id: @project, folder_id: @folder2.id, commit: false} assert_response :redirect - assert_include l(:error_folder_is_locked), flash[:errors] + assert_include l(:error_folder_is_locked), flash[:error] end def test_delete_ok @@ -185,10 +185,10 @@ class DmsfControllerTest < RedmineDmsf::Test::TestCase @request.env['HTTP_REFERER'] = dmsf_folder_path(:id => @project.id) @role.add_permission! :folder_manipulation @role.add_permission! :view_dmsf_files - get :entries_operation, :params => {:id => @project, :delete_entries => 'Delete', - :ids => ["folder-#{@folder1.id}", "file-#{@file1.id}", "folder-link-#{@folder_link1.id}", "file-link-#{@file_link2.id}"]} + get :entries_operation, params: { id: @project, delete_entries: 'Delete', + ids: ["folder-#{@folder1.id}", "file-#{@file1.id}", "folder-link-#{@folder_link1.id}", "file-link-#{@file_link2.id}"]} assert_response :redirect - assert_equal flash[:errors].to_s, l(:error_folder_is_not_empty) + assert_equal flash[:error].to_s, l(:error_folder_is_not_empty) end def test_delete_entries_ok @@ -197,21 +197,22 @@ class DmsfControllerTest < RedmineDmsf::Test::TestCase @role.add_permission! :view_dmsf_files @role.add_permission! :folder_manipulation @role.add_permission! :file_delete - flash[:errors] = nil - get :entries_operation, :params => {:id => @project, :delete_entries => 'Delete', - :ids => ["folder-#{@folder7.id}", "file-#{@file1.id}", "file-link-#{@file_link2.id}"]} + flash[:error] = nil + get :entries_operation, params: { id: @project, delete_entries: 'Delete', + ids: ["folder-#{@folder7.id}", "file-#{@file1.id}", "file-link-#{@file_link2.id}"]} assert_response :redirect - assert_nil flash[:errors] + assert_nil flash[:error] end def test_restore_entries # Restore @role.add_permission! :view_dmsf_files @request.env['HTTP_REFERER'] = trash_dmsf_path(:id => @project.id) - get :entries_operation, :params => {:id => @project, :restore_entries => 'Restore', - :ids => ["file-#{@file1.id}", "file-link-#{@file_link2.id}"]} + flash[:error] = nil + get :entries_operation, params: { id: @project, restore_entries: 'Restore', + ids: ["file-#{@file1.id}", "file-link-#{@file_link2.id}"]} assert_response :redirect - assert_nil flash[:errors] + assert_nil flash[:error] end def test_show diff --git a/test/functional/dmsf_files_controller_test.rb b/test/functional/dmsf_files_controller_test.rb index 720b7681..baf7b135 100644 --- a/test/functional/dmsf_files_controller_test.rb +++ b/test/functional/dmsf_files_controller_test.rb @@ -90,18 +90,18 @@ class DmsfFilesControllerTest < RedmineDmsf::Test::TestCase def delete_locked # Permissions OK but the file is locked @role.add_permission! :file_delete - delete @file, :params => {:commit => false} + delete @file, params: { commit: false } assert_response :redirect - assert_include l(:error_file_is_locked), flash[:errors] + assert_include l(:error_file_is_locked), flash[:error] end def delete_ok # Permissions OK and not locked - flash[:errors].clear + flash[:error].clear @file.unlock! - delete @file, :params => {:commit => false} + delete @file, params: { commit: false } assert_response :redirect - assert_equal 0, flash[:errors].size + assert_equal 0, flash[:error].size end def test_obsolete_revision_ok