From 45a927bb3519882ef9ae971f6c5030bc93d167b2 Mon Sep 17 00:00:00 2001 From: Bojan Stepanovski Date: Tue, 31 Mar 2026 12:15:35 -0400 Subject: [PATCH 1/3] Add invalid announcements error widget and add related test (#2568) --- apps/dashboard/app/models/announcement.rb | 39 ++++++++++++++--- .../app/views/layouts/_announcements.html.erb | 42 ++++++++++--------- .../views/shared/_announcement_error.html.erb | 8 ++++ .../announcement_parse_error_test.rb | 24 +++++++++++ 4 files changed, 89 insertions(+), 24 deletions(-) create mode 100644 apps/dashboard/app/views/shared/_announcement_error.html.erb create mode 100644 apps/dashboard/test/integration/announcement_parse_error_test.rb diff --git a/apps/dashboard/app/models/announcement.rb b/apps/dashboard/app/models/announcement.rb index 3688d23057..000f82f610 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 true 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..9c43607483 100644 --- a/apps/dashboard/app/views/layouts/_announcements.html.erb +++ b/apps/dashboard/app/views/layouts/_announcements.html.erb @@ -1,20 +1,24 @@ <% @announcements.select(&:valid?).reject(&:completed?).each do |announcement| %> - -<% end %> \ No newline at end of file + <% if announcement.parse_error? %> + <%= render partial: 'shared/announcement_error', locals: { announcement: announcement } %> + <% else %> + + <% end %> +<% end %> diff --git a/apps/dashboard/app/views/shared/_announcement_error.html.erb b/apps/dashboard/app/views/shared/_announcement_error.html.erb new file mode 100644 index 0000000000..611e4a856f --- /dev/null +++ b/apps/dashboard/app/views/shared/_announcement_error.html.erb @@ -0,0 +1,8 @@ +
+
+ Could not render announcement '<%= announcement.source || announcement.id || "unknown" %>' because of error +
+
+

<%= announcement.error_message || announcement.msg %>

+
+
diff --git a/apps/dashboard/test/integration/announcement_parse_error_test.rb b/apps/dashboard/test/integration/announcement_parse_error_test.rb new file mode 100644 index 0000000000..3d085e0ff5 --- /dev/null +++ b/apps/dashboard/test/integration/announcement_parse_error_test.rb @@ -0,0 +1,24 @@ +require 'test_helper' + +class AnnouncementParseErrorTest < ActionDispatch::IntegrationTest + test 'parsing error shows danger announcement' do + f = Tempfile.open(%w[announcement .yml]) + # write malformed YAML + f.write("::: this is not yaml: [\n") + f.close + + stub_user_configuration({ announcement_path: [f.path] }) + + begin + get '/' + assert_response :success + + assert_select 'div.alert-danger.card' do + assert_select 'div.card-header', /Could not render announcement/ + assert_select 'div.card-body', /mapping values/ + end + ensure + stub_user_configuration({}) + end + end +end \ No newline at end of file From c410475be93feebc237c242c4ca60f975c199323 Mon Sep 17 00:00:00 2001 From: bstepanovski Date: Wed, 8 Apr 2026 13:20:12 -0400 Subject: [PATCH 2/3] Update invalid announcements error widget and add related test (#2568) --- .../app/controllers/application_controller.rb | 33 +++++++++++++- apps/dashboard/app/models/announcement.rb | 1 - .../app/views/layouts/_announcements.html.erb | 40 ++++++++--------- .../views/shared/_announcement_error.html.erb | 8 ---- .../announcement_parse_error_test.rb | 24 ---------- .../integration/announcement_views_test.rb | 44 ++++++++++++++----- 6 files changed, 84 insertions(+), 66 deletions(-) delete mode 100644 apps/dashboard/app/views/shared/_announcement_error.html.erb delete mode 100644 apps/dashboard/test/integration/announcement_parse_error_test.rb 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 000f82f610..06227107b0 100644 --- a/apps/dashboard/app/models/announcement.rb +++ b/apps/dashboard/app/models/announcement.rb @@ -52,7 +52,6 @@ def button_text # Whether this is a valid announcement # @return [Boolean] whether it is valid def valid? - return true if parse_error? return false unless message return false if dismissible? && !id true diff --git a/apps/dashboard/app/views/layouts/_announcements.html.erb b/apps/dashboard/app/views/layouts/_announcements.html.erb index 9c43607483..3b1a9b6959 100644 --- a/apps/dashboard/app/views/layouts/_announcements.html.erb +++ b/apps/dashboard/app/views/layouts/_announcements.html.erb @@ -1,24 +1,20 @@ <% @announcements.select(&:valid?).reject(&:completed?).each do |announcement| %> - <% if announcement.parse_error? %> - <%= render partial: 'shared/announcement_error', locals: { announcement: announcement } %> - <% else %> - - <% end %> + <% end %> diff --git a/apps/dashboard/app/views/shared/_announcement_error.html.erb b/apps/dashboard/app/views/shared/_announcement_error.html.erb deleted file mode 100644 index 611e4a856f..0000000000 --- a/apps/dashboard/app/views/shared/_announcement_error.html.erb +++ /dev/null @@ -1,8 +0,0 @@ -
-
- Could not render announcement '<%= announcement.source || announcement.id || "unknown" %>' because of error -
-
-

<%= announcement.error_message || announcement.msg %>

-
-
diff --git a/apps/dashboard/test/integration/announcement_parse_error_test.rb b/apps/dashboard/test/integration/announcement_parse_error_test.rb deleted file mode 100644 index 3d085e0ff5..0000000000 --- a/apps/dashboard/test/integration/announcement_parse_error_test.rb +++ /dev/null @@ -1,24 +0,0 @@ -require 'test_helper' - -class AnnouncementParseErrorTest < ActionDispatch::IntegrationTest - test 'parsing error shows danger announcement' do - f = Tempfile.open(%w[announcement .yml]) - # write malformed YAML - f.write("::: this is not yaml: [\n") - f.close - - stub_user_configuration({ announcement_path: [f.path] }) - - begin - get '/' - assert_response :success - - assert_select 'div.alert-danger.card' do - assert_select 'div.card-header', /Could not render announcement/ - assert_select 'div.card-body', /mapping values/ - end - ensure - stub_user_configuration({}) - end - end -end \ No newline at end of file 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 From 6aad3721cad1647607c5af7ef6bc3882dacb5909 Mon Sep 17 00:00:00 2001 From: bstepanovski Date: Thu, 9 Apr 2026 08:53:19 -0400 Subject: [PATCH 3/3] Update tests to handle invalid announcements as error widgets --- apps/dashboard/app/models/announcement.rb | 1 + apps/dashboard/test/models/announcements_test.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/dashboard/app/models/announcement.rb b/apps/dashboard/app/models/announcement.rb index 06227107b0..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 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