From 8c5b47d774837e46c97b61919bdbbe945b430825 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 9 Feb 2026 11:29:11 -0500 Subject: [PATCH] Group notification emails by board (#2506) * Group notification emails by board Notification bundle emails now group items by board instead of showing the board name redundantly for each card. Each board section has a linked header and an
separator. * Adjust board title styles and remove rem units * Sort board groups alphabetically Sort notification email board sections alphabetically by name (case-insensitive). Also, rewrite mailer tests to use Nokogiri::HTML5 for precise DOM assertions instead of regex matching. --------- Co-authored-by: Andy Smith --- app/views/layouts/mailer.html.erb | 32 ++++++--- .../bundle_mailer/notification.html.erb | 11 +-- .../bundle_mailer/notification.text.erb | 11 ++- .../notification/bundle_mailer_test.rb | 69 ++++++++++++++++--- 4 files changed, 94 insertions(+), 29 deletions(-) diff --git a/app/views/layouts/mailer.html.erb b/app/views/layouts/mailer.html.erb index 51acf3ea0..40cf6191d 100644 --- a/app/views/layouts/mailer.html.erb +++ b/app/views/layouts/mailer.html.erb @@ -9,11 +9,12 @@ } body { + color: #17233C; font-family: system-ui, sans-serif; font-size: 16px; line-height: 1.3; margin: 0; - padding: 1rem; + padding: 16px; } img { @@ -72,13 +73,13 @@ .footer { border-top: 1px solid #ccc; - margin-top: 3em; + margin-top: 32px; padding-top: 1em; } .notification { - margin-top: 1rem; - margin-bottom: 1rem; + margin-top: 16px; + margin-bottom: 16px; } .notification__author { @@ -88,12 +89,25 @@ } .notification__board { - font-size: 0.8em; - font-weight: normal; - margin-bottom: 0; + font-size: 1.4em; + font-weight: 900; + margin-bottom: 16px; margin-top: 0; - opacity: 0.66; - text-transform: uppercase; + } + + .notification__board a { + color: #17233C; + text-decoration: none; + } + + .notification__board a:hover { + text-decoration: underline; + } + + .notification__board-separator { + border: none; + border-top: 1px solid #ddd; + margin: 2em 0 1em; } .notification__details { diff --git a/app/views/mailers/notification/bundle_mailer/notification.html.erb b/app/views/mailers/notification/bundle_mailer/notification.html.erb index e1fec4895..28ca0be36 100644 --- a/app/views/mailers/notification/bundle_mailer/notification.html.erb +++ b/app/views/mailers/notification/bundle_mailer/notification.html.erb @@ -5,10 +5,13 @@ You have <%= link_to pluralize(@notifications.count, "new notification"), notifications_url %><%= " in #{ Current.account.name }" if @user.identity.accounts.many? %>.

- <% @notifications.group_by(&:card).each do |card, notifications| %> -

<%= card.board.name %>

- <%= link_to "##{ card.number } #{ card.title }", card, class: "card__title" %> - <%= render partial: "notification/bundle_mailer/notification", collection: notifications, as: :notification %> + <% @notifications.group_by { |n| n.card.board }.sort_by { |board, _| board.name.downcase }.each do |board, board_notifications| %> +
+

<%= link_to board.name, board %>

+ <% board_notifications.group_by(&:card).each do |card, notifications| %> + <%= link_to "##{ card.number } #{ card.title }", card, class: "card__title" %> + <%= render partial: "notification/bundle_mailer/notification", collection: notifications, as: :notification %> + <% end %> <% end %>

Fizzy emails you about new notifications every few hours.
diff --git a/app/views/mailers/notification/bundle_mailer/notification.text.erb b/app/views/mailers/notification/bundle_mailer/notification.text.erb index 7c5798262..4e363b89d 100644 --- a/app/views/mailers/notification/bundle_mailer/notification.text.erb +++ b/app/views/mailers/notification/bundle_mailer/notification.text.erb @@ -1,16 +1,15 @@ Notifications since <%= @bundle.starts_at.strftime("%-l%P on %A, %B %-d") %> You have <%= pluralize @notifications.count, "new notification" %><%= " in #{ Current.account.name }" if @user.identity.accounts.many? %>. -<% @notifications.group_by(&:card).each do |card, notifications| %> --------------------------------------------------------------------------------- +<% @notifications.group_by { |n| n.card.board }.sort_by { |board, _| board.name.downcase }.each do |board, board_notifications| %> +<%= board.name %> +<%= "-" * board.name.length %> -<%= card.board.name %> +<% board_notifications.group_by(&:card).each do |card, notifications| %> <%= "##{ card.number } #{ card.title }" %> - --------------------------------------------------------------------------------- - <%= render partial: "notification/bundle_mailer/notification", collection: notifications, as: :notification %> <% end %> +<% end %> -------------------------------------------------------------------------------- diff --git a/test/mailers/notification/bundle_mailer_test.rb b/test/mailers/notification/bundle_mailer_test.rb index fbc4274c9..11565dfee 100644 --- a/test/mailers/notification/bundle_mailer_test.rb +++ b/test/mailers/notification/bundle_mailer_test.rb @@ -14,11 +14,12 @@ class Notification::BundleMailerTest < ActionMailer::TestCase test "renders avatar with initials in span when avatar is not attached" do create_notification(@user) - email = Notification::BundleMailer.notification(@bundle) + html = Nokogiri::HTML5(Notification::BundleMailer.notification(@bundle).html_part.body.to_s) - assert_match /]*class="avatar"[^>]*>/, email.html_part.body.to_s - assert_match /#{@user.initials}/, email.html_part.body.to_s - assert_match /style="background-color: #[A-F0-9]{6};?"/, email.html_part.body.to_s + avatar = html.at_css("span.avatar") + assert avatar, "Expected a span.avatar element" + assert_equal @user.initials, avatar.text.strip + assert_match /background-color: #[A-F0-9]{6}/, avatar["style"] end test "renders avatar with external image URL when avatar is attached" do @@ -30,15 +31,63 @@ class Notification::BundleMailerTest < ActionMailer::TestCase create_notification(@user) - email = Notification::BundleMailer.notification(@bundle) + html = Nokogiri::HTML5(Notification::BundleMailer.notification(@bundle).html_part.body.to_s) - assert_match /]*class="avatar"[^>]*>/, email.html_part.body.to_s - assert_match /]*class="avatar"[^>]*src="[^"]*"/, email.html_part.body.to_s - assert_match /alt="#{@user.name}"/, email.html_part.body.to_s + avatar = html.at_css("img.avatar") + assert avatar, "Expected an img.avatar element" + assert avatar["src"].present? + assert_equal @user.name, avatar["alt"] + end + + test "groups notifications by board, sorted alphabetically" do + private_board = boards(:private) + private_card = Current.with(user: @user) do + private_board.cards.create!( + title: "Private card", creator: @user, status: :published, account: @user.account + ) + end + private_event = Event.create!( + creator: @user, board: private_board, eventable: private_card, + action: :card_published, account: @user.account + ) + + create_notification(@user, source: events(:logo_published)) + create_notification(@user, source: private_event) + create_notification(@user, source: events(:layout_published)) + + html = Nokogiri::HTML5(Notification::BundleMailer.notification(@bundle).html_part.body.to_s) + + board_headers = html.css(".notification__board") + assert_equal 2, board_headers.size, "Should have exactly two board headers" + assert_equal [ "Private board", "Writebook" ], board_headers.map(&:text) + end + + test "board header links to the board" do + create_notification(@user, source: events(:logo_published)) + + html = Nokogiri::HTML5(Notification::BundleMailer.notification(@bundle).html_part.body.to_s) + board = boards(:writebook) + + link = html.at_css(".notification__board a") + assert_equal "Writebook", link.text + assert_match %r{boards/#{board.id}}, link["href"] + end + + test "shows multiple cards under same board header" do + create_notification(@user, source: events(:logo_published)) + create_notification(@user, source: events(:layout_published)) + + html = Nokogiri::HTML5(Notification::BundleMailer.notification(@bundle).html_part.body.to_s) + + assert_equal 1, html.css(".notification__board").size, "Same board should only have one header" + + card_titles = html.css(".card__title").map(&:text) + assert_includes card_titles, "#1 The logo isn't big enough" + assert_includes card_titles, "#2 Layout is broken" end private - def create_notification(user) - Notification.create!(user: user, creator: user, source: events(:logo_published), created_at: 30.minutes.ago) + def create_notification(user, source: events(:logo_published)) + Notification.create!(user: user, creator: user, source: source, created_at: 30.minutes.ago) end end