From 0d5f50395e6e3cf2081a2d75b6cd7b21ab3865d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Tue, 23 Apr 2019 11:21:29 +0200 Subject: [PATCH] [29253] Render error message in production on pending migrations Creates a new warning bar (previously top-shelf item) that renders a warning when migrations are pending. These did raise exceptions before in production and would result in users running in internal errors due to pending migration on some actions on the instance. https://community.openproject.com/wp/29253 --- .../stylesheets/content/_notifications.sass | 23 ++-- app/assets/stylesheets/layout/_index.sass | 2 +- .../{_top_shelf.sass => _warning_bar.sass} | 30 ++--- app/controllers/application_controller.rb | 72 +---------- app/helpers/error_message_helper.rb | 9 +- app/helpers/errors_helper.rb | 112 ++++++++++++++++++ app/helpers/migrations_helper.rb | 35 ++++++ app/views/common/_error_base.html.erb | 17 ++- app/views/common/_validation_error.html.erb | 2 +- app/views/common/error.html.erb | 10 +- app/views/layouts/base.html.erb | 2 +- app/views/projects/_form.html.erb | 2 +- .../warning_bar/_pending_migrations.html.erb | 9 ++ .../_unsupported_browser.html.erb | 2 +- app/views/warning_bar/_warning_bar.html.erb | 4 + config/locales/en.yml | 4 + lib/open_project/configuration.rb | 5 +- lib/open_project/database.rb | 30 ++++- lib/open_project/logging/log_delegator.rb | 6 +- lib/open_project/static/links.rb | 4 + .../avatars/spec/features/my_avatar_spec.rb | 2 +- .../avatars/spec/features/user_avatar_spec.rb | 4 +- .../synchronized_groups_controller.rb | 4 +- .../spec/features/block_editing_spec.rb | 2 +- .../spec/features/permissions_spec.rb | 4 +- .../spec/features/manage_webhooks_spec.rb | 2 +- spec/controllers/news_controller_spec.rb | 2 +- .../oauth_applications_management_spec.rb | 2 +- spec/features/users/password_change_spec.rb | 4 +- .../common/validation_error.html.erb_spec.rb | 1 + 30 files changed, 281 insertions(+), 126 deletions(-) rename app/assets/stylesheets/layout/{_top_shelf.sass => _warning_bar.sass} (57%) create mode 100644 app/helpers/errors_helper.rb create mode 100644 app/helpers/migrations_helper.rb create mode 100644 app/views/warning_bar/_pending_migrations.html.erb rename app/views/{browser => warning_bar}/_unsupported_browser.html.erb (92%) create mode 100644 app/views/warning_bar/_warning_bar.html.erb diff --git a/app/assets/stylesheets/content/_notifications.sass b/app/assets/stylesheets/content/_notifications.sass index 5135d014df..337031d950 100644 --- a/app/assets/stylesheets/content/_notifications.sass +++ b/app/assets/stylesheets/content/_notifications.sass @@ -305,10 +305,6 @@ $nm-upload-box-padding: rem-calc(15) rem-calc(25) ul font-size: $nm-font-size margin: 0 0 0 30px - display: inline-block - - h2, p - @extend .hidden-for-sighted .close-handler float: none @@ -356,13 +352,6 @@ $nm-upload-box-padding: rem-calc(15) rem-calc(25) .flash + .flash margin-top: 3rem -#errorExplanation - @extend %error-placeholder - top: rem-calc(68px) - - &::before - color: $nm-color-error-icon !important - a.notification-box--target-link cursor: pointer text-decoration: underline @@ -371,3 +360,15 @@ a.notification-box--target-link #errorExplanation, .notification-box--wrapper top: 4.25rem + +#errorExplanation + @extend %error-placeholder + top: rem-calc(68px) + + &::before + color: $nm-color-error-icon !important + + &.-inline + position: relative + top: 0 + left: 0 diff --git a/app/assets/stylesheets/layout/_index.sass b/app/assets/stylesheets/layout/_index.sass index ed4970a9bd..2a11f74a3f 100644 --- a/app/assets/stylesheets/layout/_index.sass +++ b/app/assets/stylesheets/layout/_index.sass @@ -31,7 +31,7 @@ @import layout/base_mobile @import layout/grid @import layout/tree_menu -@import layout/top_shelf +@import layout/warning_bar @import layout/top_menu @import layout/top_menu_mobile @import layout/breadcrumb diff --git a/app/assets/stylesheets/layout/_top_shelf.sass b/app/assets/stylesheets/layout/_warning_bar.sass similarity index 57% rename from app/assets/stylesheets/layout/_top_shelf.sass rename to app/assets/stylesheets/layout/_warning_bar.sass index bd821b020d..5126cffabe 100644 --- a/app/assets/stylesheets/layout/_top_shelf.sass +++ b/app/assets/stylesheets/layout/_warning_bar.sass @@ -1,24 +1,24 @@ -.top-shelf +.warning-bar--wrapper + position: fixed + bottom: 0 + z-index: 1001 + width: 100% + +.warning-bar--item background-color: #f4f4aa padding: 10px -.top-shelf h1 - font-size: 12px - font-weight: bold + h1 + font-size: 12px + font-weight: bold -.top-shelf p - font-size: 0.9rem - margin: 0 + p + font-size: 0.9rem + margin: 0 -.top-shelf a - font-weight: bold + a + font-weight: bold -.unsupported-browser-warning--pane - position: fixed - bottom: 0 - z-index: 1001 - width: 100% - .unsupported-browser-warning--icon cursor: pointer diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 71715ae460..a4c8dd0ff6 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -42,6 +42,7 @@ class ApplicationController < ActionController::Base include I18n include Redmine::I18n include HookHelper + include ErrorsHelper include ::OpenProject::Authentication::SessionExpiry include AdditionalUrlHelpers include OpenProjectErrorHelper @@ -113,6 +114,10 @@ class ApplicationController < ActionController::Base status: :bad_request end + rescue_from StandardError do |exception| + render_500 exception: exception + end + before_action :user_setup, :check_if_login_required, :log_requesting_user, @@ -457,73 +462,6 @@ class ApplicationController < ActionController::Base redirect_to policy.redirect_url end - def render_400(options = {}) - @project = nil - render_error({ message: :notice_bad_request, status: 400 }.merge(options)) - false - end - - def render_403(options = {}) - @project = nil - render_error({ message: :notice_not_authorized, status: 403 }.merge(options)) - false - end - - def render_404(options = {}) - render_error({ message: :notice_file_not_found, status: 404 }.merge(options)) - false - end - - def render_500(options = {}) - message = t(:notice_internal_server_error, app_title: Setting.app_title) - - if $ERROR_INFO.is_a?(ActionView::ActionViewError) - @template.instance_variable_set('@project', nil) - @template.instance_variable_set('@status', 500) - @template.instance_variable_set('@message', message) - else - @project = nil - end - - render_error({ message: message }.merge(options)) - false - end - - def render_optional_error_file(status_code) - user_setup unless User.current.id == session[:user_id] - - case status_code - when :not_found - render_404 - when :internal_server_error - render_500 - else - super - end - end - - # Renders an error response - def render_error(arg) - arg = { message: arg } unless arg.is_a?(Hash) - - @status = arg[:status] || 500 - @message = arg[:message] - - if @status >= 500 - op_handle_error "[Error #@status] #@message" - end - - @message = l(@message) if @message.is_a?(Symbol) - respond_to do |format| - format.html do - render template: 'common/error', layout: use_layout, status: @status - end - format.any do - head @status - end - end - end - # Picks which layout to use based on the request # # @return [boolean, string] name of the layout to use or false for no layout diff --git a/app/helpers/error_message_helper.rb b/app/helpers/error_message_helper.rb index 8d521dbe70..b80553e0f2 100644 --- a/app/helpers/error_message_helper.rb +++ b/app/helpers/error_message_helper.rb @@ -34,7 +34,7 @@ module ErrorMessageHelper error_messages = objects.map { |o| o.errors.full_messages }.flatten - render_error_messages_partial(error_messages, options[:object]) + render_error_messages_partial(error_messages, options) end # Will take a contract to display the errors in a rails form. @@ -51,7 +51,7 @@ module ErrorMessageHelper end end - render_error_messages_partial(error_messages, object) + render_error_messages_partial(error_messages, { object: object }) end def extract_objects_from_params(params) @@ -68,11 +68,12 @@ module ErrorMessageHelper [objects.compact, options] end - def render_error_messages_partial(messages, object) + def render_error_messages_partial(messages, options) unless messages.empty? render partial: 'common/validation_error', locals: { error_messages: messages, - object_name: object.class.model_name.human } + classes: options[:classes], + object_name: options[:object].class.model_name.human } end end end diff --git a/app/helpers/errors_helper.rb b/app/helpers/errors_helper.rb new file mode 100644 index 0000000000..68a0c76e16 --- /dev/null +++ b/app/helpers/errors_helper.rb @@ -0,0 +1,112 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2017 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2017 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See doc/COPYRIGHT.rdoc for more details. +#++ + +module ErrorsHelper + def render_400(options = {}) + @project = nil + render_error({ message: :notice_bad_request, status: 400 }.merge(options)) + false + end + + def render_403(options = {}) + @project = nil + render_error({ message: :notice_not_authorized, status: 403 }.merge(options)) + false + end + + def render_404(options = {}) + render_error({ message: :notice_file_not_found, status: 404 }.merge(options)) + false + end + + def render_500(options = {}) + message = t(:notice_internal_server_error, app_title: Setting.app_title) + + if $ERROR_INFO.is_a?(ActionView::ActionViewError) + @template.instance_variable_set('@project', nil) + @template.instance_variable_set('@status', 500) + @template.instance_variable_set('@message', message) + else + @project = nil + end + + # Append error information + if current_user.admin? + options[:message_details] = get_additional_message + end + + render_error({ message: message }.merge(options)) + false + end + + def get_additional_message + return unless OpenProject::Configuration.migration_check_on_exceptions? + + if OpenProject::Database.migrations_pending?(ensure_fresh: true) + I18n.t(:error_migrations_are_pending) + end + end + + def render_optional_error_file(status_code) + user_setup unless User.current.id == session[:user_id] + + case status_code + when :not_found + render_404 + when :internal_server_error + render_500 + else + super + end + end + + # Renders an error response + def render_error(arg) + arg = { message: arg } unless arg.is_a?(Hash) + + @status = arg[:status] || 500 + @message = arg[:message] + + if @status >= 500 + op_handle_error "[Error #@status] #@message" + end + + @message = l(@message) if @message.is_a?(Symbol) + @message_details = arg[:message_details] + respond_to do |format| + format.html do + render template: 'common/error', layout: use_layout, status: @status + end + format.any do + head @status + end + end + end +end diff --git a/app/helpers/migrations_helper.rb b/app/helpers/migrations_helper.rb new file mode 100644 index 0000000000..7ecea9d92f --- /dev/null +++ b/app/helpers/migrations_helper.rb @@ -0,0 +1,35 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2017 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2017 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See doc/COPYRIGHT.rdoc for more details. +#++ + +module MigrationsHelper + def render_pending_migrations_warning? + current_user.admin? && OpenProject::Database.migrations_pending? + end +end diff --git a/app/views/common/_error_base.html.erb b/app/views/common/_error_base.html.erb index 35ce61fe2d..5298d7f9b1 100644 --- a/app/views/common/_error_base.html.erb +++ b/app/views/common/_error_base.html.erb @@ -27,13 +27,20 @@ See docs/COPYRIGHT.rdoc for more details. ++#%> -