bterkuile/fizzy
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #2512 from basecamp/server-side-notification-grouping

Browse Source 
Stack notifications in all views
This commit is contained in:
Stanko Krtalić
2026-02-12 10:33:12 +01:00
committed by GitHub
parent 446c75bb5c 36ee253a1a
commit 61144667fc
35 changed files with 265 additions and 320 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/controllers/cards/readings_controller.rb
+2 -2
View File
@@ -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
app/controllers/notifications/trays_controller.rb
+1 -1
View File
@@ -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.
app/helpers/notifications_helper.rb
+2 -4
View File
@@ -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
app/javascript/controllers/notifications_tray_controller.js
-89
View File
@@ -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
}
}
app/models/card/readable.rb
+2 -18
View File
@@ -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
app/models/concerns/push_notifiable.rb
+1
View File
@@ -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
app/models/notification.rb
+28 -17
View File
@@ -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
app/models/notification/bundle.rb
+2 -2
View File
@@ -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
app/models/notifier.rb
+7 -1
View File
@@ -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
app/models/user/notifiable.rb
+3 -3
View File
@@ -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
app/views/cards/readings/create.turbo_stream.erb
-3
View File
@@ -1,3 +0,0 @@
<% @notifications.each do |notification| %>
<%= turbo_stream.remove notification %>
<% end %>
app/views/notifications/_notification.html.erb
+1 -1
View File
@@ -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 %>
app/views/notifications/_notification.json.jbuilder
+1 -1
View File
@@ -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
app/views/notifications/_tray.html.erb
+2 -3
View File
@@ -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" } %>
<div class="tray__item tray__item--hat txt-x-small gap-half">
<div data-navigable-list-target="item" class="full-width">
app/views/notifications/index.json.jbuilder
+1 -1
View File
@@ -1 +1 @@
json.array! (@unread || []) + @page.records, partial: "notifications/notification", as: :notification
json.array! (@unread || []) + @page.records, partial: "notifications/notification", as: :notification, cached: true
app/views/notifications/index/_notification.html.erb
-6
View File
@@ -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 %>
app/views/notifications/index/_read_notifications.html.erb
+1 -1
View File
@@ -3,7 +3,7 @@
<h2 class="txt-medium margin-block-start-double margin-block-end-half txt-uppercase translucent">Previously seen</h2>
<div id="notifications_list_read" contents>
<%= render partial: "notifications/index/notification", collection: page.records, cached: true %>
<%= render partial: "notifications/notification", collection: page.records, cached: true %>
</div>
</section>
<% end %>
app/views/notifications/index/_unread_notifications.html.erb
+2 -2
View File
@@ -11,12 +11,12 @@
<% end %>
<div id="notifications_list" contents>
<%= render partial: "notifications/index/notification", collection: unread, cached: true %>
<%= render partial: "notifications/notification", collection: unread, cached: true %>
</div>
<% 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 %>
<div class="fill-highlight txt-x-small border-radius pad-block-half pad-inline">
Showing the <%= NotificationsController::MAX_UNREAD_NOTIFICATIONS %> most recent (<%= total_unread_count - NotificationsController::MAX_UNREAD_NOTIFICATIONS %> are hidden)
</div>
app/views/notifications/trays/show.html.erb
-1
View File
@@ -1,4 +1,3 @@
<%= turbo_frame_tag "notifications" do %>
<%= render partial: "notifications/notification", collection: @notifications, cached: true %>
<div hidden data-notifications-tray-target="hiddenNotifications"></div>
<% end %>
db/migrate/20260209165805_notifications_data_migration.rb
+84
View File
@@ -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
db/schema.rb
Generated
+2 -1
View File
@@ -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
db/schema_sqlite.rb
+2 -1
View File
@@ -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
test/controllers/cards/readings_controller_test.rb
+7 -31
View File
@@ -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
test/controllers/notifications/bulk_readings_controller_test.rb
+2 -2
View File
@@ -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
test/controllers/notifications/readings_controller_test.rb
+11 -12
View File
@@ -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
test/controllers/notifications_controller_test.rb
+3 -4
View File
@@ -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
test/fixtures/notifications.yml
Vendored
+8 -18
View File
@@ -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
test/mailers/notification/bundle_mailer_test.rb
+1
View File
@@ -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,
test/models/card/readable_test.rb
+19 -42
View File
@@ -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
test/models/notification/bundle_test.rb
+6 -5
View File
@@ -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)
test/models/notification_pusher_test.rb
+1 -4
View File
@@ -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!(
test/models/notification_test.rb
+49 -31
View File
@@ -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
test/models/notifier/event_notifier_test.rb
+7 -7
View File
@@ -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
test/models/user/notifiable_test.rb
+3 -2
View File
@@ -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
test/system/smoke_test.rb
+4 -4
View File
@@ -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