diff --git a/app/controllers/cards/readings_controller.rb b/app/controllers/cards/readings_controller.rb index 95acc7511..1fc675b84 100644 --- a/app/controllers/cards/readings_controller.rb +++ b/app/controllers/cards/readings_controller.rb @@ -2,12 +2,12 @@ class Cards::ReadingsController < ApplicationController include CardScoped def create - @notifications = @card.read_by(Current.user) + @card.read_by(Current.user) record_board_access end def destroy - @notifications = @card.unread_by(Current.user) + @card.unread_by(Current.user) record_board_access end diff --git a/app/controllers/notifications/trays_controller.rb b/app/controllers/notifications/trays_controller.rb index c2bac662a..0adc0e17b 100644 --- a/app/controllers/notifications/trays_controller.rb +++ b/app/controllers/notifications/trays_controller.rb @@ -1,6 +1,6 @@ class Notifications::TraysController < ApplicationController def show - @notifications = Current.user.notifications.preloaded.unread.ordered.limit(100) + @notifications = Current.user.notifications.unread.preloaded.ordered.limit(100) # Invalidate on the whole set instead of the unread set since the max updated at in the unread set # can stay the same when reading old notifications. diff --git a/app/helpers/notifications_helper.rb b/app/helpers/notifications_helper.rb index 355f60550..f376d15c1 100644 --- a/app/helpers/notifications_helper.rb +++ b/app/helpers/notifications_helper.rb @@ -29,9 +29,7 @@ module NotificationsHelper def notification_tag(notification, &) tag.div id: dom_id(notification), class: "tray__item tray__item--notification", data: { navigable_list_target: "item", - notifications_tray_target: "notification", - card_id: notification.card.id, - timestamp: notification.created_at.to_i + card_id: notification.card.id } do link_to(notification, class: [ "card card--notification", { "card--closed": notification.card.closed? }, { "unread": !notification.read? } ], @@ -58,7 +56,7 @@ module NotificationsHelper data: { action: "form#submit:stop badge#update:stop", form_target: "submit" }, form: { data: { controller: "form" } } do concat(icon_tag("remove")) - concat(tag.span("1", class: "badge-count", data: { group_count: "" })) + concat(tag.span(notification.unread_count, class: "badge-count")) if notification.unread_count > 1 end end end diff --git a/app/javascript/controllers/notifications_tray_controller.js b/app/javascript/controllers/notifications_tray_controller.js deleted file mode 100644 index 12077425b..000000000 --- a/app/javascript/controllers/notifications_tray_controller.js +++ /dev/null @@ -1,89 +0,0 @@ -import { Controller } from "@hotwired/stimulus" - -export default class extends Controller { - static targets = [ "notification", "hiddenNotifications" ] - static classes = [ "grouped" ] - - connect() { - this.group() - } - - group() { - const notificationsByCardId = this.#groupNotificationsByCardId() - - for (const cardId in notificationsByCardId) { - const notifications = notificationsByCardId[cardId] - if (notifications.length > 1) { - this.#renderGroup(notifications) - } - } - - this.grouped = true - } - - notificationTargetConnected(notification) { - if (this.grouped && notification.parentElement !== this.hiddenNotificationsTarget) { - this.#groupNotification(notification) - } - } - - #groupNotificationsByCardId() { - const notificationsByCardId = {} - - this.notificationTargets.forEach(notification => { - const cardId = notification.dataset.cardId - notificationsByCardId[cardId] ||= [] - notificationsByCardId[cardId].push(notification) - }) - - return notificationsByCardId - } - - #groupNotification(notification) { - const groupedNotifications = this.#groupedNotificationsFor(notification) - - if (groupedNotifications.length > 1) { - this.#renderGroup(groupedNotifications) - } - } - - #groupedNotificationsFor(notification) { - const cardId = notification.dataset.cardId - return this.notificationTargets - .filter(notification => notification.dataset.cardId === cardId) - } - - #renderGroup(groupedNotifications) { - groupedNotifications.sort((notification_1, notification_2) => parseInt(notification_1.dataset.timestamp) - parseInt(notification_2.dataset.timestamp)) - - this.#showAsGrouped(groupedNotifications[0], groupedNotifications.length) - groupedNotifications.slice(1).forEach(notification => this.#hideInGroup(notification)) - } - - #showAsGrouped(notification, size) { - notification.classList.add(this.groupedClass) - this.#showGroupedNotification(notification) - this.#setGroupCount(notification, size) - } - - #hideInGroup(notification) { - this.#hideGroupedNotification(notification) - this.#setGroupCount(notification, "") - } - - // We use a hidden container instead of hiding the notifications directly so that the CSS that sort the - // tray element indexes doesn't get messed up with the child positions changing. - #showGroupedNotification(notification) { - if (notification.parentElement === this.hiddenNotificationsTarget) { - this.hiddenNotificationsTarget.parentElement.insertBefore(notification, this.hiddenNotificationsTarget) - } - } - - #hideGroupedNotification(notification) { - this.hiddenNotificationsTarget.appendChild(notification) - } - - #setGroupCount(notification, count) { - notification.querySelector("[data-group-count]").textContent = count - } -} diff --git a/app/models/card/readable.rb b/app/models/card/readable.rb index 9c272dd75..0168f00a9 100644 --- a/app/models/card/readable.rb +++ b/app/models/card/readable.rb @@ -2,15 +2,11 @@ module Card::Readable extend ActiveSupport::Concern def read_by(user) - notifications_for(user).tap do |notifications| - notifications.each(&:read) - end + user.notifications.find_by(card: self)&.read end def unread_by(user) - all_notifications_for(user).tap do |notifications| - notifications.each(&:unread) - end + user.notifications.find_by(card: self)&.unread end def remove_inaccessible_notifications @@ -25,18 +21,6 @@ module Card::Readable Card::RemoveInaccessibleNotificationsJob.perform_later(self) end - def notifications_for(user) - scope = user.notifications.unread - scope.where(source: event_notification_sources) - .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/models/concerns/push_notifiable.rb b/app/models/concerns/push_notifiable.rb index e497bc3fe..e7bfdc56a 100644 --- a/app/models/concerns/push_notifiable.rb +++ b/app/models/concerns/push_notifiable.rb @@ -3,6 +3,7 @@ module PushNotifiable included do after_create_commit :push_notification_later + after_update_commit :push_notification_later, if: :source_id_previously_changed? end private diff --git a/app/models/notification.rb b/app/models/notification.rb index 545f6ab75..3d7df5885 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -5,32 +5,39 @@ class Notification < ApplicationRecord belongs_to :user belongs_to :creator, class_name: "User" belongs_to :source, polymorphic: true + belongs_to :card scope :unread, -> { where(read_at: nil) } scope :read, -> { where.not(read_at: nil) } - scope :ordered, -> { order(read_at: :desc, created_at: :desc) } + scope :ordered, -> { order(read_at: :desc, updated_at: :desc) } + scope :preloaded, -> { preload(:card, :creator, :account, source: [ :board, :creator, { eventable: [ :closure, :board, :assignments ] } ]) } - after_create_commit :broadcast_unread - after_destroy_commit :broadcast_read + before_validation :set_card after_create :bundle + after_update :bundle, if: :source_id_previously_changed? - scope :preloaded, -> { preload(:creator, :account, source: [ :board, :creator, { eventable: [ :closure, :board, :assignments ] } ]) } + after_create_commit -> { broadcast_prepend_later_to user, :notifications, target: "notifications" } + after_update_commit -> { broadcast_update } + after_destroy_commit -> { broadcast_remove_to user, :notifications } delegate :notifiable_target, to: :source - delegate :card, to: :source - def self.read_all - all.each { |notification| notification.read } + class << self + def read_all + all.each(&:read) + end + + def unread_all + all.each(&:unread) + end end def read - update!(read_at: Time.current) - broadcast_read + update!(read_at: Time.current, unread_count: 0) end def unread - update!(read_at: nil) - broadcast_unread + update!(read_at: nil, unread_count: 1) end def read? @@ -38,15 +45,19 @@ class Notification < ApplicationRecord end private - def broadcast_unread - broadcast_prepend_later_to user, :notifications, target: "notifications" - end - - def broadcast_read - broadcast_remove_to user, :notifications + def set_card + self.card = source.card end def bundle user.bundle(self) if user.settings.bundling_emails? end + + def broadcast_update + if read? + broadcast_remove_to(user, :notifications) + else + broadcast_prepend_later_to(user, :notifications, target: "notifications") + end + end end diff --git a/app/models/notification/bundle.rb b/app/models/notification/bundle.rb index fb57a4fd0..1eadb3c9e 100644 --- a/app/models/notification/bundle.rb +++ b/app/models/notification/bundle.rb @@ -5,7 +5,7 @@ class Notification::Bundle < ApplicationRecord enum :status, %i[ pending processing delivered ] scope :due, -> { pending.where("ends_at <= ?", Time.current) } - scope :containing, ->(notification) { where("starts_at <= ? AND ends_at > ?", notification.created_at, notification.created_at) } + scope :containing, ->(notification) { where("starts_at <= ? AND ends_at > ?", notification.updated_at, notification.updated_at) } scope :overlapping_with, ->(other_bundle) do where( "(starts_at <= ? AND ends_at >= ?) OR (starts_at <= ? AND ends_at >= ?) OR (starts_at >= ? AND ends_at <= ?)", @@ -33,7 +33,7 @@ class Notification::Bundle < ApplicationRecord end def notifications - user.notifications.where(created_at: window).unread + user.notifications.where(updated_at: window).unread end def deliver diff --git a/app/models/notifier.rb b/app/models/notifier.rb index 24c1b3b4e..b50fc6d26 100644 --- a/app/models/notifier.rb +++ b/app/models/notifier.rb @@ -16,7 +16,13 @@ class Notifier if should_notify? # Processing recipients in order avoids deadlocks if notifications overlap. recipients.sort_by(&:id).map do |recipient| - Notification.create! user: recipient, source: source, creator: creator + notification = Notification.find_or_initialize_by(user: recipient, card: source.card) + notification.source = source + notification.creator = creator + notification.read_at = nil + notification.unread_count = (notification.unread_count || 0) + 1 + notification.save! + notification end end end diff --git a/app/models/user/notifiable.rb b/app/models/user/notifiable.rb index f6788aaac..7598f0e0f 100644 --- a/app/models/user/notifiable.rb +++ b/app/models/user/notifiable.rb @@ -25,12 +25,12 @@ module User::Notifiable def expand_pending_bundle_for(notification) pending = notification_bundles.pending.last - if pending.present? && notification.created_at < pending.starts_at - pending.update!(starts_at: notification.created_at) # expand the window to include this notification + if pending.present? && notification.updated_at < pending.starts_at + pending.update!(starts_at: notification.updated_at) # expand the window to include this notification end end def create_bundle_for(notification) - notification_bundles.create!(starts_at: notification.created_at) + notification_bundles.create!(starts_at: notification.updated_at) end end diff --git a/app/views/cards/readings/create.turbo_stream.erb b/app/views/cards/readings/create.turbo_stream.erb deleted file mode 100644 index 5d7fe7801..000000000 --- a/app/views/cards/readings/create.turbo_stream.erb +++ /dev/null @@ -1,3 +0,0 @@ -<% @notifications.each do |notification| %> - <%= turbo_stream.remove notification %> -<% end %> diff --git a/app/views/notifications/_notification.html.erb b/app/views/notifications/_notification.html.erb index 02f547c4e..6bed2aa56 100644 --- a/app/views/notifications/_notification.html.erb +++ b/app/views/notifications/_notification.html.erb @@ -2,7 +2,7 @@ <%# Helper Dependency Updated: avatar_image_tag 2025-12-15 %> <%= notification_tag notification do %> <%= render "notifications/notification/header", notification: notification do %> - <%= notification_toggle_read_button(notification, url: card_reading_path(notification.card)) %> + <%= 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.json.jbuilder b/app/views/notifications/_notification.json.jbuilder index ba27c5425..d2fadfa1b 100644 --- a/app/views/notifications/_notification.json.jbuilder +++ b/app/views/notifications/_notification.json.jbuilder @@ -1,5 +1,5 @@ json.cache! notification do - json.(notification, :id) + json.(notification, :id, :unread_count) json.read notification.read? json.read_at notification.read_at&.utc json.created_at notification.created_at.utc diff --git a/app/views/notifications/_tray.html.erb b/app/views/notifications/_tray.html.erb index 1f5aae469..74d492049 100644 --- a/app/views/notifications/_tray.html.erb +++ b/app/views/notifications/_tray.html.erb @@ -8,9 +8,8 @@ data-navigable-list-reverse-navigation-value="true" data-action="keydown->navigable-list#navigate dialog:show@document->navigable-list#reset turbo-visit->navigable-list#reset keydown.esc->dialog#close:stop click@document->dialog#closeOnClickOutside"> <%= turbo_frame_tag "notifications", src: tray_notifications_path, refresh: "morph", data: { - controller: "notifications-tray frame-reloader", - notifications_tray_grouped_class: "notification--grouped", - action: "turbo:frame-render->notifications-tray#group focus@window->frame-reloader#reload" } %> + controller: "frame-reloader", + action: "focus@window->frame-reloader#reload" } %>
diff --git a/app/views/notifications/index.json.jbuilder b/app/views/notifications/index.json.jbuilder index 660bbb673..e8d9a93ed 100644 --- a/app/views/notifications/index.json.jbuilder +++ b/app/views/notifications/index.json.jbuilder @@ -1 +1 @@ -json.array! (@unread || []) + @page.records, partial: "notifications/notification", as: :notification +json.array! (@unread || []) + @page.records, partial: "notifications/notification", as: :notification, cached: true diff --git a/app/views/notifications/index/_notification.html.erb b/app/views/notifications/index/_notification.html.erb deleted file mode 100644 index 90f3d45eb..000000000 --- a/app/views/notifications/index/_notification.html.erb +++ /dev/null @@ -1,6 +0,0 @@ -<%= 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/index/_read_notifications.html.erb b/app/views/notifications/index/_read_notifications.html.erb index 04c2e3c48..c73d1d3c2 100644 --- a/app/views/notifications/index/_read_notifications.html.erb +++ b/app/views/notifications/index/_read_notifications.html.erb @@ -3,7 +3,7 @@

Previously seen

- <%= render partial: "notifications/index/notification", collection: page.records, cached: true %> + <%= render partial: "notifications/notification", collection: page.records, cached: true %>
<% end %> diff --git a/app/views/notifications/index/_unread_notifications.html.erb b/app/views/notifications/index/_unread_notifications.html.erb index 4890a4822..fed57dbb9 100644 --- a/app/views/notifications/index/_unread_notifications.html.erb +++ b/app/views/notifications/index/_unread_notifications.html.erb @@ -11,12 +11,12 @@ <% end %>
- <%= render partial: "notifications/index/notification", collection: unread, cached: true %> + <%= render partial: "notifications/notification", collection: unread, cached: true %>
<% if unread.any? %> <% total_unread_count = Current.user.notifications.unread.count %> - <% if Current.user.notifications.unread.count > NotificationsController::MAX_UNREAD_NOTIFICATIONS %> + <% if total_unread_count > NotificationsController::MAX_UNREAD_NOTIFICATIONS %>
Showing the <%= NotificationsController::MAX_UNREAD_NOTIFICATIONS %> most recent (<%= total_unread_count - NotificationsController::MAX_UNREAD_NOTIFICATIONS %> are hidden)
diff --git a/app/views/notifications/trays/show.html.erb b/app/views/notifications/trays/show.html.erb index 278d97a4d..f61c52d51 100644 --- a/app/views/notifications/trays/show.html.erb +++ b/app/views/notifications/trays/show.html.erb @@ -1,4 +1,3 @@ <%= turbo_frame_tag "notifications" do %> <%= render partial: "notifications/notification", collection: @notifications, cached: true %> - <% end %> diff --git a/db/migrate/20260209165805_notifications_data_migration.rb b/db/migrate/20260209165805_notifications_data_migration.rb new file mode 100644 index 000000000..d1522f780 --- /dev/null +++ b/db/migrate/20260209165805_notifications_data_migration.rb @@ -0,0 +1,84 @@ +class NotificationsDataMigration < ActiveRecord::Migration[8.2] + BATCH_SIZE = 10_000 + + class Notification < ActiveRecord::Base + self.table_name = "notifications" + end + + def change + reversible do |dir| + dir.up do + populate_card_id + collapse_duplicates + end + end + + change_column_null :notifications, :card_id, false + add_index :notifications, [ :user_id, :card_id ], unique: true + end + + private + def populate_card_id + execute(<<~SQL) + UPDATE notifications + SET card_id = ( + SELECT CASE events.eventable_type + WHEN 'Card' THEN events.eventable_id + WHEN 'Comment' THEN (SELECT comments.card_id FROM comments WHERE comments.id = events.eventable_id) + END + FROM events + WHERE events.id = notifications.source_id + ) + WHERE notifications.card_id IS NULL + AND notifications.source_type = 'Event' + SQL + + execute(<<~SQL) + UPDATE notifications + SET card_id = ( + SELECT CASE mentions.source_type + WHEN 'Card' THEN mentions.source_id + WHEN 'Comment' THEN (SELECT comments.card_id FROM comments WHERE comments.id = mentions.source_id) + END + FROM mentions + WHERE mentions.id = notifications.source_id + ) + WHERE notifications.card_id IS NULL + AND notifications.source_type = 'Mention' + SQL + end + + def collapse_duplicates + loop do + duplicates = Notification.find_by_sql(<<~SQL) + SELECT user_id, card_id, + MAX(id) AS keep_id, + COUNT(*) AS total, + SUM(CASE WHEN read_at IS NULL THEN 1 ELSE 0 END) AS unread_total + FROM notifications + WHERE card_id IS NOT NULL + GROUP BY user_id, card_id + HAVING COUNT(*) > 1 + LIMIT #{BATCH_SIZE} + SQL + + break if duplicates.empty? + + duplicates.each do |row| + Notification.where(user_id: row.user_id, card_id: row.card_id) + .where.not(id: row.keep_id) + .delete_all + + Notification.where(id: row.keep_id) + .update_all(unread_count: row.unread_total.to_i) + end + end + + # Set unread_count for remaining non-collapsed notifications + execute(<<~SQL) + UPDATE notifications + SET unread_count = CASE WHEN read_at IS NULL THEN 1 ELSE 0 END + WHERE unread_count = 0 AND card_id IS NOT NULL + SQL + end +end diff --git a/db/schema.rb b/db/schema.rb index ddfdb3b60..db1a2283d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -385,7 +385,7 @@ ActiveRecord::Schema[8.2].define(version: 2026_02_11_122517) do create_table "notifications", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.uuid "account_id", null: false - t.uuid "card_id" + t.uuid "card_id", null: false t.datetime "created_at", null: false t.uuid "creator_id" t.datetime "read_at" @@ -397,6 +397,7 @@ ActiveRecord::Schema[8.2].define(version: 2026_02_11_122517) do t.index ["account_id"], name: "index_notifications_on_account_id" t.index ["creator_id"], name: "index_notifications_on_creator_id" t.index ["source_type", "source_id"], name: "index_notifications_on_source" + t.index ["user_id", "card_id"], name: "index_notifications_on_user_id_and_card_id", unique: true t.index ["user_id", "read_at", "created_at"], name: "index_notifications_on_user_id_and_read_at_and_created_at", order: { read_at: :desc, created_at: :desc } t.index ["user_id"], name: "index_notifications_on_user_id" end diff --git a/db/schema_sqlite.rb b/db/schema_sqlite.rb index 5fa6d05b7..3486ca9d1 100644 --- a/db/schema_sqlite.rb +++ b/db/schema_sqlite.rb @@ -385,7 +385,7 @@ ActiveRecord::Schema[8.2].define(version: 2026_02_11_122517) do create_table "notifications", id: :uuid, force: :cascade do |t| t.uuid "account_id", null: false - t.uuid "card_id" + t.uuid "card_id", null: false t.datetime "created_at", null: false t.uuid "creator_id" t.datetime "read_at" @@ -397,6 +397,7 @@ ActiveRecord::Schema[8.2].define(version: 2026_02_11_122517) do t.index ["account_id"], name: "index_notifications_on_account_id" t.index ["creator_id"], name: "index_notifications_on_creator_id" t.index ["source_type", "source_id"], name: "index_notifications_on_source" + t.index ["user_id", "card_id"], name: "index_notifications_on_user_id_and_card_id", unique: true t.index ["user_id", "read_at", "created_at"], name: "index_notifications_on_user_id_and_read_at_and_created_at", order: { read_at: :desc, created_at: :desc } t.index ["user_id"], name: "index_notifications_on_user_id" end diff --git a/test/controllers/cards/readings_controller_test.rb b/test/controllers/cards/readings_controller_test.rb index b7f7474c7..0c9c9aeed 100644 --- a/test/controllers/cards/readings_controller_test.rb +++ b/test/controllers/cards/readings_controller_test.rb @@ -8,7 +8,7 @@ class Cards::ReadingsControllerTest < ActionDispatch::IntegrationTest test "create" do freeze_time - assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: false, to: true do + assert_changes -> { notifications(:logo_assignment_kevin).reload.read? }, from: false, to: true do assert_changes -> { accesses(:writebook_kevin).reload.accessed_at }, from: nil, to: Time.current do post card_reading_url(cards(:logo)), as: :turbo_stream end @@ -17,31 +17,20 @@ class Cards::ReadingsControllerTest < ActionDispatch::IntegrationTest assert_response :success end - test "read one notification on card visit" do - assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: false, to: true do + test "read notification on card visit" do + assert_changes -> { notifications(:logo_assignment_kevin).reload.read? }, from: false, to: true do post card_reading_path(cards(:logo)), as: :turbo_stream end assert_response :success end - test "read multiple notifications on card visit" do - assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: false, to: true do - assert_changes -> { notifications(:logo_assignment_kevin).reload.read? }, from: false, to: true do - post card_reading_path(cards(:logo)), as: :turbo_stream - end - end - - assert_response :success - 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 -> { notifications(:logo_assignment_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 @@ -50,26 +39,13 @@ class Cards::ReadingsControllerTest < ActionDispatch::IntegrationTest assert_response :success end - test "unread one notification on destroy" do - notifications(:logo_published_kevin).read + test "unread notification on destroy" do + 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 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/controllers/notifications/bulk_readings_controller_test.rb b/test/controllers/notifications/bulk_readings_controller_test.rb index 3512b7b43..f889dfaac 100644 --- a/test/controllers/notifications/bulk_readings_controller_test.rb +++ b/test/controllers/notifications/bulk_readings_controller_test.rb @@ -6,7 +6,7 @@ class Notifications::BulkReadingsControllerTest < ActionDispatch::IntegrationTes end test "create marks all notifications as read" do - assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: false, to: true do + assert_changes -> { notifications(:logo_assignment_kevin).reload.read? }, from: false, to: true do assert_changes -> { notifications(:layout_commented_kevin).reload.read? }, from: false, to: true do post bulk_reading_path end @@ -24,7 +24,7 @@ class Notifications::BulkReadingsControllerTest < ActionDispatch::IntegrationTes end test "create as JSON" do - assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: false, to: true do + assert_changes -> { notifications(:logo_assignment_kevin).reload.read? }, from: false, to: true do assert_changes -> { notifications(:layout_commented_kevin).reload.read? }, from: false, to: true do post bulk_reading_path, as: :json end diff --git a/test/controllers/notifications/readings_controller_test.rb b/test/controllers/notifications/readings_controller_test.rb index 3f2a690a9..fca29a35b 100644 --- a/test/controllers/notifications/readings_controller_test.rb +++ b/test/controllers/notifications/readings_controller_test.rb @@ -3,38 +3,37 @@ require "test_helper" class Notifications::ReadingsControllerTest < ActionDispatch::IntegrationTest setup do sign_in_as :kevin + @notification = notifications(:logo_assignment_kevin) end test "create" do - assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: false, to: true do - post notification_reading_path(notifications(:logo_published_kevin), format: :turbo_stream) + assert_changes -> { @notification.reload.read? }, from: false, to: true do + post notification_reading_path(@notification, format: :turbo_stream) assert_response :success end end test "destroy" do - notification = notifications(:logo_published_kevin) - notification.read # Mark as read first + @notification.read - assert_changes -> { notification.reload.read? }, from: true, to: false do - delete notification_reading_path(notification, format: :turbo_stream) + assert_changes -> { @notification.reload.read? }, from: true, to: false do + delete notification_reading_path(@notification, format: :turbo_stream) assert_response :success end end test "create as JSON" do - assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: false, to: true do - post notification_reading_path(notifications(:logo_published_kevin)), as: :json + assert_changes -> { @notification.reload.read? }, from: false, to: true do + post notification_reading_path(@notification), as: :json assert_response :no_content end end test "destroy as JSON" do - notification = notifications(:logo_published_kevin) - notification.read + @notification.read - assert_changes -> { notification.reload.read? }, from: true, to: false do - delete notification_reading_path(notification), as: :json + assert_changes -> { @notification.reload.read? }, from: true, to: false do + delete notification_reading_path(@notification), as: :json assert_response :no_content end end diff --git a/test/controllers/notifications_controller_test.rb b/test/controllers/notifications_controller_test.rb index e4e7bec38..0d0565f24 100644 --- a/test/controllers/notifications_controller_test.rb +++ b/test/controllers/notifications_controller_test.rb @@ -10,18 +10,17 @@ class NotificationsControllerTest < ActionDispatch::IntegrationTest assert_response :success assert_kind_of Array, @response.parsed_body - assert @response.parsed_body.any? { |n| n["id"] == notifications(:logo_published_kevin).id } + assert @response.parsed_body.any? { |n| n["id"] == notifications(:logo_assignment_kevin).id } end test "index as JSON includes notification attributes" do get notifications_path, as: :json - notification = @response.parsed_body.find { |n| n["id"] == notifications(:logo_published_kevin).id } + notification = @response.parsed_body.find { |n| n["id"] == notifications(:logo_assignment_kevin).id } - assert_not_nil notification["title"] - assert_not_nil notification["body"] assert_not_nil notification["created_at"] assert_not_nil notification["card"] assert_not_nil notification["creator"] + assert_not_nil notification["unread_count"] end end diff --git a/test/fixtures/notifications.yml b/test/fixtures/notifications.yml index 9fe7da317..b7ae167cd 100644 --- a/test/fixtures/notifications.yml +++ b/test/fixtures/notifications.yml @@ -1,15 +1,9 @@ -logo_published_kevin: - id: <%= ActiveRecord::FixtureSet.identify("logo_published_kevin", :uuid) %> - user: kevin_uuid - source: logo_published_uuid (Event) - created_at: <%= 1.week.ago %> - creator: david_uuid - account: 37s_uuid - logo_assignment_kevin: id: <%= ActiveRecord::FixtureSet.identify("logo_assignment_kevin", :uuid) %> user: kevin_uuid source: logo_assignment_km_uuid (Event) + card: logo_uuid + unread_count: 2 created_at: <%= 1.week.ago %> creator: david_uuid account: 37s_uuid @@ -18,22 +12,18 @@ layout_commented_kevin: id: <%= ActiveRecord::FixtureSet.identify("layout_commented_kevin", :uuid) %> user: kevin_uuid source: layout_commented_uuid (Event) + card: layout_uuid + unread_count: 1 created_at: <%= 1.week.ago %> creator: david_uuid account: 37s_uuid -logo_card_david_mention_by_jz: - id: <%= ActiveRecord::FixtureSet.identify("logo_card_david_mention_by_jz_notif", :uuid) %> - user: david_uuid - source: logo_card_david_mention_by_jz_uuid (Mention) - created_at: <%= 1.week.ago %> - creator: david_uuid - account: 37s_uuid - -logo_comment_david_mention_by_jz: - id: <%= ActiveRecord::FixtureSet.identify("logo_comment_david_mention_by_jz_notif", :uuid) %> +logo_mentioned_david: + id: <%= ActiveRecord::FixtureSet.identify("logo_mentioned_david", :uuid) %> user: david_uuid source: logo_comment_david_mention_by_jz_uuid (Mention) + card: logo_uuid + unread_count: 2 created_at: <%= 1.week.ago %> creator: david_uuid account: 37s_uuid diff --git a/test/mailers/notification/bundle_mailer_test.rb b/test/mailers/notification/bundle_mailer_test.rb index 11565dfee..36f9b1e60 100644 --- a/test/mailers/notification/bundle_mailer_test.rb +++ b/test/mailers/notification/bundle_mailer_test.rb @@ -3,6 +3,7 @@ require "test_helper" class Notification::BundleMailerTest < ActionMailer::TestCase setup do @user = users(:david) + @user.notifications.destroy_all @bundle = Notification::Bundle.create!( user: @user, diff --git a/test/models/card/readable_test.rb b/test/models/card/readable_test.rb index 4adb8747e..dd86ee997 100644 --- a/test/models/card/readable_test.rb +++ b/test/models/card/readable_test.rb @@ -1,60 +1,41 @@ require "test_helper" class Card::ReadableTest < ActiveSupport::TestCase - test "read clears events notifications" do - assert_changes -> { notifications(:logo_published_kevin).reload.read? }, from: false, to: true do - assert_changes -> { notifications(:logo_assignment_kevin).reload.read? }, from: false, to: true do - cards(:logo).read_by(users(:kevin)) - end + test "read marks notification as read" do + assert_changes -> { notifications(:logo_assignment_kevin).reload.read? }, from: false, to: true do + cards(:logo).read_by(users(:kevin)) end end - test "read clear mentions in the description" do - assert_changes -> { notifications(:logo_card_david_mention_by_jz).reload.read? }, from: false, to: true do + test "read marks mention notification as read" do + assert_changes -> { notifications(:logo_mentioned_david).reload.read? }, from: false, to: true do cards(:logo).read_by(users(:david)) end end - test "read clear mentions in comments" do - assert_changes -> { notifications(:logo_comment_david_mention_by_jz).reload.read? }, from: false, to: true do - cards(:logo).read_by(users(:david)) - end - end - - test "read clears notifications from the comments" do + test "read marks comment notification as read" do assert_changes -> { notifications(:layout_commented_kevin).reload.read? }, from: false, to: true do cards(:layout).read_by(users(:kevin)) end end - test "unread marks events notifications as unread" do - notifications(:logo_published_kevin).read + test "unread marks notification as unread" do 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 + assert_changes -> { notifications(:logo_assignment_kevin).reload.read? }, from: true, to: false do + cards(:logo).unread_by(users(:kevin)) end end - test "unread marks mentions in the description as unread" do - notifications(:logo_card_david_mention_by_jz).read + test "unread marks mention notification as unread" do + notifications(:logo_mentioned_david).read - assert_changes -> { notifications(:logo_card_david_mention_by_jz).reload.read? }, from: true, to: false do + assert_changes -> { notifications(:logo_mentioned_david).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 + test "unread marks comment notification as unread" do notifications(:layout_commented_kevin).read assert_changes -> { notifications(:layout_commented_kevin).reload.read? }, from: true, to: false do @@ -68,8 +49,8 @@ class Card::ReadableTest < ActiveSupport::TestCase david = users(:david) assert card.accessible_to?(kevin) - kevin_notifications = [ notifications(:logo_published_kevin), notifications(:logo_assignment_kevin) ] - david_notifications = [ notifications(:logo_card_david_mention_by_jz), notifications(:logo_comment_david_mention_by_jz) ] + kevin_notification = notifications(:logo_assignment_kevin) + david_notification = notifications(:logo_mentioned_david) # Kevin loses access card.board.accesses.find_by(user: kevin).destroy @@ -78,14 +59,10 @@ class Card::ReadableTest < ActiveSupport::TestCase card.remove_inaccessible_notifications - # Kevin's notifications removed - kevin_notifications.each do |notification| - assert_not Notification.exists?(notification.id) - end + # Kevin's notification removed + assert_not Notification.exists?(kevin_notification.id) - # David's notifications preserved - david_notifications.each do |notification| - assert Notification.exists?(notification.id) - end + # David's notification preserved + assert Notification.exists?(david_notification.id) end end diff --git a/test/models/notification/bundle_test.rb b/test/models/notification/bundle_test.rb index 2a8b242af..66dd8dd77 100644 --- a/test/models/notification/bundle_test.rb +++ b/test/models/notification/bundle_test.rb @@ -5,6 +5,7 @@ class Notification::BundleTest < ActiveSupport::TestCase setup do @user = users(:david) + @user.notifications.destroy_all @user.settings.bundle_email_every_few_hours! end @@ -25,7 +26,7 @@ class Notification::BundleTest < ActiveSupport::TestCase end end - test "notifications are bundled withing the aggregation period" do + test "notifications are bundled within the aggregation period" do @user.notification_bundles.destroy_all notification_1 = assert_difference -> { @user.notification_bundles.pending.count }, 1 do @@ -34,12 +35,12 @@ class Notification::BundleTest < ActiveSupport::TestCase travel_to 3.hours.from_now notification_2 = assert_no_difference -> { @user.notification_bundles.count } do - @user.notifications.create!(source: events(:logo_published), creator: @user) + @user.notifications.create!(source: events(:layout_published), creator: @user) end travel_to 3.days.from_now notification_3 = assert_difference -> { @user.notification_bundles.pending.count }, 1 do - @user.notifications.create!(source: events(:logo_published), creator: @user) + @user.notifications.create!(source: events(:text_published), creator: @user) end assert_equal 2, @user.notification_bundles.count @@ -150,10 +151,10 @@ class Notification::BundleTest < ActiveSupport::TestCase test "out-of-order notification bundling should still work" do first_notification = @user.notifications.create!(source: events(:logo_published), creator: @user) - second_notification = @user.notifications.create!(source: events(:logo_published), creator: @user) + second_notification = @user.notifications.create!(source: events(:layout_commented), creator: @user) @user.notification_bundles.destroy_all - assert first_notification.created_at < second_notification.created_at + assert first_notification.updated_at <= second_notification.updated_at @user.bundle(second_notification) @user.bundle(first_notification) diff --git a/test/models/notification_pusher_test.rb b/test/models/notification_pusher_test.rb index 21b9e202b..53380effe 100644 --- a/test/models/notification_pusher_test.rb +++ b/test/models/notification_pusher_test.rb @@ -3,10 +3,7 @@ require "test_helper" class NotificationPusherTest < ActiveSupport::TestCase setup do @user = users(:david) - @notification = @user.notifications.create!( - source: events(:logo_published), - creator: users(:jason) - ) + @notification = notifications(:logo_mentioned_david) @pusher = NotificationPusher.new(@notification) @user.push_subscriptions.create!( diff --git a/test/models/notification_test.rb b/test/models/notification_test.rb index 9c8ebbe12..8d9809ba1 100644 --- a/test/models/notification_test.rb +++ b/test/models/notification_test.rb @@ -1,49 +1,67 @@ require "test_helper" class NotificationTest < ActiveSupport::TestCase - test "unread marks notification as unread" do - notification = notifications(:logo_published_kevin) - notification.read # Mark as read first - - assert_changes -> { notification.reload.read? }, from: true, to: false do - notification.unread - end - end - - test "unread broadcasts to notifications" do - notification = notifications(:logo_published_kevin) - notification.read # Mark as read first - - assert_turbo_stream_broadcasts([ notification.user, :notifications ], count: 1) do - perform_enqueued_jobs do - notification.unread - end - end - end - test "read marks notification as read" do - notification = notifications(:logo_published_kevin) - # Ensure it starts as unread - notification.update!(read_at: nil) + notification = notifications(:logo_assignment_kevin) + notification.update!(read_at: nil, unread_count: 2) assert_changes -> { notification.reload.read? }, from: false, to: true do notification.read end + + assert_equal 0, notification.unread_count end - test "read broadcasts to notifications" do - notification = notifications(:logo_published_kevin) - # Ensure it starts as unread - notification.update!(read_at: nil) + test "unread marks notification as unread" do + notification = notifications(:logo_assignment_kevin) + notification.read + + assert_changes -> { notification.reload.read? }, from: true, to: false do + notification.unread + end + + assert_equal 1, notification.unread_count + end + + test "read_all marks all notifications and resets unread counts" do + kevin = users(:kevin) + + kevin.notifications.unread.read_all + + assert kevin.notifications.reload.all?(&:read?) + assert kevin.notifications.reload.all? { |n| n.unread_count == 0 } + end + + test "unread_count tracks notification count per card" do + notification = notifications(:logo_assignment_kevin) + assert_equal 2, notification.unread_count + end + + test "broadcasting on create prepends" do + kevin = users(:kevin) + layout = cards(:layout) + + notifications(:layout_commented_kevin).destroy + + perform_enqueued_jobs do + Notification.create!(user: kevin, source: events(:layout_commented), creator: users(:david)) + end + + assert_turbo_stream_broadcasts [ kevin, :notifications ] + end + + test "broadcasting on update when read removes" do + notification = notifications(:layout_commented_kevin) assert_turbo_stream_broadcasts([ notification.user, :notifications ], count: 1) do - notification.read + perform_enqueued_jobs do + notification.read + end end end - test "deleting notification broadcasts its removal" do - notification = notifications(:logo_published_kevin) - notification.update!(read_at: nil) + test "broadcasting on destroy removes" do + notification = notifications(:logo_assignment_kevin) assert_turbo_stream_broadcasts([ notification.user, :notifications ], count: 1) do notification.destroy diff --git a/test/models/notifier/event_notifier_test.rb b/test/models/notifier/event_notifier_test.rb index 82942b96c..6393ddb99 100644 --- a/test/models/notifier/event_notifier_test.rb +++ b/test/models/notifier/event_notifier_test.rb @@ -71,18 +71,18 @@ class Notifier::EventNotifierTest < ActiveSupport::TestCase test "create notifications on publish for mentionees" do users(:kevin).mentioned_by(users(:david), at: cards(:logo)) - assert_difference -> { users(:kevin).notifications.count }, +1 do - Notifier.for(events(:logo_published)).notify - end + notifications = Notifier.for(events(:logo_published)).notify + + assert_includes notifications.map(&:user), users(:kevin) end - test "don'create notifications on publish for mentionees that are not watching" do + test "create notifications on publish for mentionees that are not watching" do users(:kevin).mentioned_by(users(:david), at: cards(:logo)) cards(:logo).unwatch_by(users(:kevin)) - assert_difference -> { users(:kevin).notifications.count }, +1 do - Notifier.for(events(:logo_published)).notify - end + notifications = Notifier.for(events(:logo_published)).notify + + assert_includes notifications.map(&:user), users(:kevin) end test "don't create notifications on comment for mentionees" do diff --git a/test/models/user/notifiable_test.rb b/test/models/user/notifiable_test.rb index c7bc4fabc..aabfdda2d 100644 --- a/test/models/user/notifiable_test.rb +++ b/test/models/user/notifiable_test.rb @@ -3,6 +3,7 @@ require "test_helper" class User::NotifiableTest < ActiveSupport::TestCase setup do @user = users(:david) + @user.notifications.destroy_all @user.settings.bundle_email_every_few_hours! end @@ -12,7 +13,7 @@ class User::NotifiableTest < ActiveSupport::TestCase end bundle = @user.notification_bundles.last - assert_equal notification.created_at, bundle.starts_at + assert_equal notification.updated_at, bundle.starts_at assert bundle.pending? end @@ -20,7 +21,7 @@ class User::NotifiableTest < ActiveSupport::TestCase @user.notifications.create!(source: events(:logo_published), creator: @user) assert_no_difference -> { @user.notification_bundles.count } do - @user.notifications.create!(source: events(:logo_published), creator: @user) + @user.notifications.create!(source: events(:layout_published), creator: @user) end end end diff --git a/test/system/smoke_test.rb b/test/system/smoke_test.rb index c307c50d2..31e8f723f 100644 --- a/test/system/smoke_test.rb +++ b/test/system/smoke_test.rb @@ -67,13 +67,13 @@ class SmokeTest < ApplicationSystemTestCase test "dismissing notifications" do sign_in_as(users(:david)) - notif = notifications(:logo_card_david_mention_by_jz) + notification = notifications(:logo_mentioned_david) - assert_selector "div##{dom_id(notif)}" + assert_selector "div##{dom_id(notification)}" - within_window(open_new_window) { visit card_url(notif.card) } + within_window(open_new_window) { visit card_url(notification.card) } - assert_no_selector "div##{dom_id(notif)}" + assert_no_selector "div##{dom_id(notification)}" end test "dragging card to a new column" do