diff --git a/apps/dashboard/app/controllers/application_controller.rb b/apps/dashboard/app/controllers/application_controller.rb index f0a226f896..e2f3911d2c 100644 --- a/apps/dashboard/app/controllers/application_controller.rb +++ b/apps/dashboard/app/controllers/application_controller.rb @@ -77,7 +77,38 @@ def set_pinned_apps end def set_announcements - @announcements ||= Announcements.all(@user_configuration.announcement_path) + all_announcements = Announcements.all(@user_configuration.announcement_path) + + parse_errors, announcements = all_announcements.partition(&:parse_error?) + if parse_errors.any? + flash.now[:alert] = helpers.safe_join( + parse_errors.map do |a| + source = a.source.presence || a.id.presence || 'unknown' + short_name = + begin + Pathname.new(source.to_s).basename.sub_ext('').to_s + rescue StandardError + source.to_s + end + + headline = "Could not render announcement '#{short_name}' because of error" + details = [a.error_class.presence, a.error_message.presence].compact.join(': ') + details = a.message.presence || a.msg.presence || 'Error parsing announcement.' if details.blank? + + helpers.content_tag(:div) do + helpers.safe_join( + [ + helpers.content_tag(:div, headline, class: 'card-header bg-transparent'), + helpers.content_tag(:div, details.to_s, class: 'card-body py-2'), + ] + ) + end + end, + helpers.safe_join([helpers.tag.br, helpers.tag.br]) + ) + end + + @announcements ||= announcements rescue => e logger.warn "Error parsing announcements: #{e.message}" @announcements ||= [] diff --git a/apps/dashboard/app/models/announcement.rb b/apps/dashboard/app/models/announcement.rb index 3688d23057..986966febd 100644 --- a/apps/dashboard/app/models/announcement.rb +++ b/apps/dashboard/app/models/announcement.rb @@ -52,6 +52,7 @@ def button_text # Whether this is a valid announcement # @return [Boolean] whether it is valid def valid? + return false if parse_error? return false unless message return false if dismissible? && !id true @@ -75,6 +76,30 @@ def required? opts.fetch(:required, false) end + # Whether this announcement is a parse/render error + # @return [Boolean] whether it is a parse/render error + def parse_error? + opts.fetch(:parse_error, false) + end + + # Raw error message when parsing failed + # @return [String, nil] the error message if parsing failed + def error_message + opts[:error_message] + end + + # Exception class name when parsing failed + # @return [String, nil] the exception class name if parsing failed + def error_class + opts[:error_class] + end + + # Source file path for the announcement (if any) + # @return [String, nil] the source file path if it exists + def source + opts[:file] + end + private def opts @@ -86,15 +111,19 @@ def opts else {} end - @opts.symbolize_keys.compact - rescue Errno::ENOENT # File does not exist + (@opts || {}).symbolize_keys.compact + rescue Errno::ENOENT => e # File does not exist Rails.logger.warn "Announcement file not found: #{@path}" - @opts = {} + @opts = build_error_opts("Announcement file not found: #{@path}", e) rescue SyntaxError => e # Syntax errors Rails.logger.warn "Syntax error in announcement file '#{@path}': #{e.message}. Please check the file for proper syntax." - @opts = {} + @opts = build_error_opts("Syntax error parsing announcement file '#{@path}': #{e.message}", e) rescue StandardError => e # Other exceptions Rails.logger.warn "Error parsing announcement file '#{@path}': #{e.message}" - @opts = {} + @opts = build_error_opts("Error parsing announcement file '#{@path}': #{e.message}", e) + end + + def build_error_opts(message, e) + { type: :danger, message: message, parse_error: true, error_message: e.message, error_class: e.class.to_s, file: @path.to_s } end end diff --git a/apps/dashboard/app/views/layouts/_announcements.html.erb b/apps/dashboard/app/views/layouts/_announcements.html.erb index 884c0c6abe..3b1a9b6959 100644 --- a/apps/dashboard/app/views/layouts/_announcements.html.erb +++ b/apps/dashboard/app/views/layouts/_announcements.html.erb @@ -17,4 +17,4 @@ <% end %> -<% end %> \ No newline at end of file +<% end %> diff --git a/apps/dashboard/test/integration/announcement_views_test.rb b/apps/dashboard/test/integration/announcement_views_test.rb index 9b15810293..8f4b3dd4f1 100644 --- a/apps/dashboard/test/integration/announcement_views_test.rb +++ b/apps/dashboard/test/integration/announcement_views_test.rb @@ -9,9 +9,31 @@ class AnnouncementViewsTest < ActionDispatch::IntegrationTest stub_user_configuration({ announcement_path: [f.path] }) begin - get '/' - assert_response :success - assert_select 'div.announcement div.announcement-body', 'Test announcement.' + with_modified_env('OOD_ANNOUNCEMENT_PATH' => nil) do + get '/' + assert_response :success + assert_select 'div.announcement div.announcement-body', 'Test announcement.' + end + ensure + stub_user_configuration({}) + end + end + + test 'parsing error shows a danger alert' do + f = Tempfile.open(%w[announcement .yml]) + f.write("::: this is not yaml: [\n") + f.close + + stub_user_configuration({ announcement_path: [f.path] }) + + begin + with_modified_env('OOD_ANNOUNCEMENT_PATH' => nil) do + get '/' + assert_response :success + + assert_select 'div.alert.alert-danger', /Could not render announcement/ + assert_select 'div.alert.alert-danger', /mapping values/ + end ensure stub_user_configuration({}) end @@ -22,13 +44,15 @@ class AnnouncementViewsTest < ActionDispatch::IntegrationTest stub_user_configuration({ announcement_path: [file] }) begin - get '/' - assert_response :success - assert_select 'div.announcement div.announcement-body', 'This is the announcement.' - assert_select 'div.announcement .announcement-button', I18n.t('dashboard.announcements_dismissible_button') - form = css_select('div.announcement .announcement-form') - assert_equal 1, form.size - assert_equal settings_path(action: 'announcement'), form[0]['action'] + with_modified_env('OOD_ANNOUNCEMENT_PATH' => nil) do + get '/' + assert_response :success + assert_select 'div.announcement div.announcement-body', 'This is the announcement.' + assert_select 'div.announcement .announcement-button', I18n.t('dashboard.announcements_dismissible_button') + form = css_select('div.announcement .announcement-form') + assert_equal 1, form.size + assert_equal settings_path(action: 'announcement'), form[0]['action'] + end ensure stub_user_configuration({}) end diff --git a/apps/dashboard/test/models/announcements_test.rb b/apps/dashboard/test/models/announcements_test.rb index 4efe1c1057..51b1f25860 100644 --- a/apps/dashboard/test/models/announcements_test.rb +++ b/apps/dashboard/test/models/announcements_test.rb @@ -86,7 +86,7 @@ class AnnouncementsTest < ActiveSupport::TestCase Rails.logger.expects(:warn).with(regexp_matches(/Syntax error in announcement file/)) announcement = Announcements.all(f.path).first - assert_equal(:warning, announcement.type) + assert_equal(:danger, announcement.type) end test 'should create invalid announcement for invalid yaml file' do