Maintains a deduplication hash for eachnotification_message entry on insert/update to prevent identical notifications from being enqueued on the same day.

I plumbed incoder/quartz to help manipulate time to make theTestNotificationDuplicates simple and readable.

github-actionsbot assigneddannykopping

Aug 5, 2024

dannykopping force-pushed thedk/notif-dedupe branch frome571901 to43ea55eCompare

August 5, 2024 18:10

dannykopping marked this pull request as ready for review

August 5, 2024 18:25

dannykopping requested review fromjohnstcn andmafredri

August 5, 2024 18:26

dannykopping commented

Aug 5, 2024

View reviewed changes

coderd/database/queries/notifications.sql

		@@ -13,14 +13,15 @@ WHERE nt.id = @notification_template_id
		AND u.id = @user_id;

		-- name: EnqueueNotificationMessage :exec
		INSERT INTO notification_messages (id, notification_template_id, user_id, method, payload, targets, created_by)
		INSERT INTO notification_messages (id, notification_template_id, user_id, method, payload, targets, created_by, created_at)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I addedcreated_at to be passed in to make the dedupe hash testable by using Quartz to advance the clock.

feat: add notification deduplication trigger

675aa8f

Signed-off-by: Danny Kopping <danny@coder.com>

dannykopping force-pushed thedk/notif-dedupe branch from43ea55e to675aa8fCompare

August 5, 2024 18:28

johnstcn reviewed

Aug 5, 2024

View reviewed changes

coderd/database/migrations/000239_notifications_dedupe.up.sql OutdatedShow resolvedHide resolved

coderd/database/models.goShow resolvedHide resolved

coderd/notifications/enqueuer.goShow resolvedHide resolved

mafredri approved these changes

Aug 6, 2024

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Flagged some stuff but looks pretty straight-forward to me, approved.

coderd/database/migrations/000239_notifications_dedupe.up.sql OutdatedShow resolvedHide resolved

coderd/notifications/enqueuer.go OutdatedShow resolvedHide resolved

coderd/notifications/manager_test.goShow resolvedHide resolved

github-actionsbot added the staleThis issue is like stale bread. label

Aug 14, 2024

github-actionsbot closed this

Aug 18, 2024

dannykopping reopened this

Aug 19, 2024

github-actionsbot removed the staleThis issue is like stale bread. label

Aug 20, 2024

johnstcn approved these changes

Aug 21, 2024

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Approving to unblock; previous comments still hold.

dannykopping added2 commits

August 21, 2024 08:34

Merge branch 'main' ofhttps://github.com/coder/coderinto dk/notif-d…

e9b2f3b

…edupe

Review comments

6fc765b

Signed-off-by: Danny Kopping <danny@coder.com>

mafredri approved these changes

Aug 21, 2024

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Still LGTM 👍🏻

coderd/notifications/metrics_test.go

		@@ -13,6 +13,8 @@ import (
		"github.com/stretchr/testify/assert"
		"github.com/stretchr/testify/require"

		"github.com/coder/quartz"

		"github.com/coder/serpent"

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Another instance of import grouping, this is the job of the tooling tbh (and it's failing), so up to you if you want to fix it 😄. (There are actually a few more of these too but I'll leave those uncommented.)