From eea9afb6395bbfee44abc4e1f0fc23164ce04239 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Fri, 14 Nov 2025 12:58:24 +0100 Subject: [PATCH] Fix: dismiss grouped notifications by card from the notifications tray This also splits the templates into two: index and tray, since the URL now changes. --- app/controllers/cards/readings_controller.rb | 5 +++ app/helpers/notifications_helper.rb | 6 +-- app/models/card/readable.rb | 14 ++++++- .../notifications/_notification.html.erb | 18 ++------- app/views/notifications/index.html.erb | 4 +- .../index/_notification.html.erb | 6 +++ .../notifications/notification/_body.html.erb | 9 +++++ .../notification/_header.html.erb | 26 +++++++------ config/routes.rb | 1 + .../cards/readings_controller_test.rb | 38 +++++++++++++++++++ test/models/card/readable_test.rb | 35 +++++++++++++++++ 11 files changed, 130 insertions(+), 32 deletions(-) create mode 100644 app/views/notifications/index/_notification.html.erb create mode 100644 app/views/notifications/notification/_body.html.erb diff --git a/app/controllers/cards/readings_controller.rb b/app/controllers/cards/readings_controller.rb index e6a728829..887b5dd3d 100644 --- a/app/controllers/cards/readings_controller.rb +++ b/app/controllers/cards/readings_controller.rb @@ -8,6 +8,11 @@ class Cards::ReadingsController < ApplicationController record_board_access end + def destroy + @notifications = @card.unread_by(Current.user) + record_board_access + end + private def record_board_access @card.board.accessed_by(Current.user) diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index 29348493a..f09153cff 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -34,9 +34,9 @@ module NotificationsHelper end end - def notification_toggle_read_button(notification) + def notification_toggle_read_button(notification, url:) if notification.read? - button_to notification_reading_path(notification), + button_to url, method: :delete, class: "card__notification-unread-indicator btn btn--circle borderless", title: "Mark as unread", @@ -45,7 +45,7 @@ module NotificationsHelper concat(icon_tag("unseen")) end else - button_to notification_reading_path(notification), + button_to url, class: "card__notification-unread-indicator btn btn--circle borderless", title: "Mark as read", data: { action: "form#submit:stop badge#update:stop", form_target: "submit" }, diff --git a/app/models/card/readable.rb b/app/models/card/readable.rb index 06c811715..903db2d71 100644 --- a/app/models/card/readable.rb +++ b/app/models/card/readable.rb @@ -7,9 +7,15 @@ module Card::Readable end end + def unread_by(user) + all_notifications_for(user).tap do |notifications| + notifications.each(&:unread) + end + end + def remove_inaccessible_notifications User.find_each do |user| - notifications_for(user).destroy_all unless accessible_to?(user) + all_notifications_for(user).destroy_all unless accessible_to?(user) end end @@ -24,6 +30,12 @@ module Card::Readable .or(scope.where(source: mention_notification_sources)) end + def all_notifications_for(user) + scope = user.notifications + scope.where(source: event_notification_sources) + .or(scope.where(source: mention_notification_sources)) + end + def event_notification_sources events.or(comment_creation_events) end diff --git a/app/views/notifications/_notification.html.erb b/app/views/notifications/_notification.html.erb index 40d413c2b..1596d8529 100644 --- a/app/views/notifications/_notification.html.erb +++ b/app/views/notifications/_notification.html.erb @@ -1,18 +1,8 @@ <% cache notification do %> <%= notification_tag notification do %> - -
- <%= render "notifications/notification/header", notification: notification %> -
- -
-
- <%= avatar_image_tag notification.creator %> -
- -

- <%= render "notifications/notification/#{notification.source_type.underscore}/body", notification: notification %> -

-
+ <%= render "notifications/notification/header", notification: notification do %> + <%= notification_toggle_read_button(notification, url: card_reading_path(notification.card)) %> + <% end %> + <%= render "notifications/notification/body", notification: notification %> <% end %> <% end %> diff --git a/app/views/notifications/index.html.erb b/app/views/notifications/index.html.erb index 2fa4e0078..eae50830b 100644 --- a/app/views/notifications/index.html.erb +++ b/app/views/notifications/index.html.erb @@ -26,7 +26,7 @@ <% end %>
- <%= render partial: "notifications/notification", collection: @unread, cached: true %> + <%= render partial: "notifications/index/notification", collection: @unread, cached: true %>
@@ -34,7 +34,7 @@

Previously seen

- <%= render partial: "notifications/notification", collection: @page.records, cached: true %> + <%= render partial: "notifications/index/notification", collection: @page.records, cached: true %>
diff --git a/app/views/notifications/index/_notification.html.erb b/app/views/notifications/index/_notification.html.erb new file mode 100644 index 000000000..90f3d45eb --- /dev/null +++ b/app/views/notifications/index/_notification.html.erb @@ -0,0 +1,6 @@ +<%= notification_tag notification do %> + <%= render "notifications/notification/header", notification: notification do %> + <%= notification_toggle_read_button(notification, url: notification_reading_path(notification)) %> + <% end %> + <%= render "notifications/notification/body", notification: notification %> +<% end %> diff --git a/app/views/notifications/notification/_body.html.erb b/app/views/notifications/notification/_body.html.erb new file mode 100644 index 000000000..21652d10a --- /dev/null +++ b/app/views/notifications/notification/_body.html.erb @@ -0,0 +1,9 @@ +
+
+ <%= avatar_image_tag notification.creator %> +
+ +

+ <%= render "notifications/notification/#{notification.source_type.underscore}/body", notification: notification %> +

+
diff --git a/app/views/notifications/notification/_header.html.erb b/app/views/notifications/notification/_header.html.erb index 8b78fa3ea..de3432354 100644 --- a/app/views/notifications/notification/_header.html.erb +++ b/app/views/notifications/notification/_header.html.erb @@ -1,21 +1,23 @@ -
+
+
Card number <%= notification.card.id %> - + <%= notification.card.board.name %> -
+
-
- <%= notification.creator.familiar_name %> -
+
+ <%= notification.creator.familiar_name %> +
-
- <%= local_datetime_tag(notification.created_at, style: :timeordate) %> -
+
+ <%= local_datetime_tag(notification.created_at, style: :timeordate) %> +
-
- <%= notification_toggle_read_button(notification) %> -
+
+ <%= yield %> +
+ diff --git a/config/routes.rb b/config/routes.rb index 640e82843..154430254 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -77,6 +77,7 @@ Rails.application.routes.draw do resource :reading resource :triage resource :watch + resource :reading resources :assignments resources :steps diff --git a/test/controllers/cards/readings_controller_test.rb b/test/controllers/cards/readings_controller_test.rb index 86d4f0482..fc0e83c78 100644 --- a/test/controllers/cards/readings_controller_test.rb +++ b/test/controllers/cards/readings_controller_test.rb @@ -40,4 +40,42 @@ class Cards::ReadingsControllerTest < ActionDispatch::IntegrationTest assert_equal "false", response.headers["X-Writer-Affinity"] end + + test "destroy" do + freeze_time + + notifications(:logo_published_kevin).read + notifications(:logo_assignment_kevin).read + + assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: true, to: false do + assert_changes -> { accesses(:writebook_kevin).reload.accessed_at }, to: Time.current do + delete card_reading_url(cards(:logo)), as: :turbo_stream + end + end + + assert_response :success + end + + test "unread one notification on destroy" do + notifications(:logo_published_kevin).read + + assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: true, to: false do + delete card_reading_path(cards(:logo)), as: :turbo_stream + end + + assert_response :success + end + + test "unread multiple notifications on destroy" do + notifications(:logo_published_kevin).read + notifications(:logo_assignment_kevin).read + + assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: true, to: false do + assert_changes -> { notifications(:logo_assignment_kevin).reload.read? }, from: true, to: false do + delete card_reading_path(cards(:logo)), as: :turbo_stream + end + end + + assert_response :success + end end diff --git a/test/models/card/readable_test.rb b/test/models/card/readable_test.rb index 565a31e2b..4adb8747e 100644 --- a/test/models/card/readable_test.rb +++ b/test/models/card/readable_test.rb @@ -27,6 +27,41 @@ class Card::ReadableTest < ActiveSupport::TestCase end end + test "unread marks events notifications as unread" do + notifications(:logo_published_kevin).read + notifications(:logo_assignment_kevin).read + + assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: true, to: false do + assert_changes -> { notifications(:logo_assignment_kevin).reload.read? }, from: true, to: false do + cards(:logo).unread_by(users(:kevin)) + end + end + end + + test "unread marks mentions in the description as unread" do + notifications(:logo_card_david_mention_by_jz).read + + assert_changes -> { notifications(:logo_card_david_mention_by_jz).reload.read? }, from: true, to: false do + cards(:logo).unread_by(users(:david)) + end + end + + test "unread marks mentions in comments as unread" do + notifications(:logo_comment_david_mention_by_jz).read + + assert_changes -> { notifications(:logo_comment_david_mention_by_jz).reload.read? }, from: true, to: false do + cards(:logo).unread_by(users(:david)) + end + end + + test "unread marks notifications from the comments as unread" do + notifications(:layout_commented_kevin).read + + assert_changes -> { notifications(:layout_commented_kevin).reload.read? }, from: true, to: false do + cards(:layout).unread_by(users(:kevin)) + end + end + test "remove inaccessible notifications" do card = cards(:logo) kevin = users(:kevin)