feat: add notifications inbox db #16599

defelmnq · 2025-02-18T10:11:03Z

This PR is linked to the following issue.

The objective is to create the DB layer and migration for the new Coder Inbox.

fix rbac roles

coderd/database/queries/notificationsinbox.sql

coderd/database/migrations/000296_notifications_inbox.up.sql

dannykopping · 2025-02-21T08:05:47Z

coderd/database/migrations/000296_notifications_inbox.up.sql

+	title			TEXT 						NOT NULL,
+	content 		TEXT 						NOT NULL,
+	icon 			TEXT 						NOT NULL,
+	actions 		JSONB 						NOT NULL,


Given that you have title and content, I assume you're going to be accepting the fully rendered content (probably Markdown) of the notification in these fields. That means that CTAs will already be included in content, I think. What role will actions play here?

The idea was to generate the title, content and actions using a Dispatcher method similar to SMTP.
the payload field contains everything to have these three elements.

The reason why I had in mind to keep the actions separated from the body is that , based on the client (VSCode for example) we will not be able to handle the actions the same way. If we integrate the actions in the body using markdown it will not be compatible with everything.

So we just return labels and associated URLs and each client can handle it their way.

wdyt ?

If we integrate the actions in the body using markdown it will not be compatible with everything.

I think this is how it's already done, so we may need to change some things around later.
For now let's go with this until we reach any blockers.

coderd/database/migrations/000296_notifications_inbox.up.sql

dannykopping · 2025-02-21T08:07:24Z

coderd/rbac/roles_test.go

+			},
+			Resource: rbac.ResourceInboxNotification.WithID(uuid.New()).InOrg(orgID).WithOwner(currentUser.String()),
+			AuthorizeMap: map[bool][]hasAuthSubjects{
+				true:  {owner, orgMemberMe, orgAdmin},


Why does orgMemberMe have privileges to policy.ActionCreate inbox notifications?

From my RBAC understanding orgMemberMe is the current user.
Associated to this part on the which we use the currentUser id.

In the definition of the ResourceInboxNotification, I assign the currentUser to InboxNotification using the WithOwner(currentUser.String()) method.

TL;DR - orgMemberMe is the owner of the resource in the test - and the owner of the resource should be able to access it.

I think we might want memberMe in here as well?

@defelmnq I understand why the current user should be able to read their own messages, but why should they be allowed to create new messages? That should be a system privilege.

dannykopping · 2025-02-21T08:10:48Z

coderd/database/queries/notificationsinbox.sql

+-- name: FetchUnreadInboxNotificationsByUserID :many
+SELECT * FROM inbox_notifications WHERE user_id = $1 AND read_at IS NULL ORDER BY created_at DESC;
+
+-- name: FetchInboxNotificationsByUserID :many
+SELECT * FROM inbox_notifications WHERE user_id = $1 ORDER BY created_at DESC;


These queries are identical except for read_at IS NULL; rather parameterise FetchInboxNotificationsByUserID so you can explicitly filter for read/unread, or neither.

FetchUnreadInboxNotificationsByUserIDFilteredByTemplatesAndTargets could be refactored out according to the same pattern, too.

As I tried to iterate here and from what I found on the internet - I can't find a way to achieve that with sqlc.

sqlc, with narg, allow to have null parameters and optional values, with default values if we want - here the objective is a bit different as we want to completely change the query and the WHERE clause based on if a parameter is present or not.

If you have more idea, I could check.

You can alternatively just write a custom query yourself (see coderd/database/modelqueries.go) and not need to deal with sqlc's idiosyncracies.

defelmnq · 2025-02-21T06:55:42Z

coderd/database/migrations/000296_notifications_inbox.up.sql

+	id 				UUID 						PRIMARY KEY,
+	user_id 		UUID 						NOT NULL REFERENCES users(id) ON DELETE CASCADE,
+	template_id 	UUID 						NOT NULL REFERENCES notification_templates(id) ON DELETE CASCADE,
+	targets 		UUID[],


Targets do not reference a specific table.

Targets represent another entity which is connected to the notification - this field will be used to filter and search.

defelmnq · 2025-02-21T06:56:13Z

coderd/database/migrations/000296_notifications_inbox.up.sql

@@ -0,0 +1,15 @@
+CREATE TABLE inbox_notifications (
+	id 				UUID 						PRIMARY KEY,
+	user_id 		UUID 						NOT NULL REFERENCES users(id) ON DELETE CASCADE,


references the user that receives the notification.

coderd/database/queries/notificationsinbox.sql

defelmnq · 2025-02-21T12:05:50Z

coderd/rbac/roles_test.go

+			},
+			Resource: rbac.ResourceInboxNotification.WithID(uuid.New()).InOrg(orgID).WithOwner(currentUser.String()),
+			AuthorizeMap: map[bool][]hasAuthSubjects{
+				true:  {owner, orgMemberMe, orgAdmin},


From my RBAC understanding orgMemberMe is the current user.
Associated to this part on the which we use the currentUser id.

In the definition of the ResourceInboxNotification, I assign the currentUser to InboxNotification using the WithOwner(currentUser.String()) method.

TL;DR - orgMemberMe is the owner of the resource in the test - and the owner of the resource should be able to access it.

defelmnq · 2025-02-24T07:04:16Z

Update on the PR with some changes :

Naming for the DB methods has been changed to fit with the existing ones.
Added a new method CountUnreadNotificationsByUserID which was missing
Added pagination to list methods

About the pagination - because of the way notifications can be pushed (in real-time , potentially a lot of notifs quickly) - I decided to paginate using IDs instead of an offset which would be hard to maintain.

johnstcn

No blocking comments, some suggestions below.

johnstcn · 2025-02-24T11:36:37Z

coderd/rbac/roles_test.go

+			},
+			Resource: rbac.ResourceInboxNotification.WithID(uuid.New()).InOrg(orgID).WithOwner(currentUser.String()),
+			AuthorizeMap: map[bool][]hasAuthSubjects{
+				true:  {owner, orgMemberMe, orgAdmin},


I think we might want memberMe in here as well?

johnstcn · 2025-02-24T11:39:58Z

coderd/database/queries/notificationsinbox.sql

+-- name: CountUnreadInboxNotificationsByUserID :one
+SELECT COUNT(*) FROM inbox_notifications WHERE user_id = $1 AND read_at IS NULL;


Where do we plan to use this query? If we're just displaying the unread notifications count in the UI, would it suffice to just call len(GetUnreadInboxNotificationsByUserID()? My assumption is that the UI will be fetching unread notifications anyway.

This query will be called in two cases :

An endpoint to get the number of unread notifications (without any notifications - just the count)

Whenever you want to list the number of notifications (unread or not, filtered or not) - we'll include the unread_count

johnstcn · 2025-02-24T11:41:42Z

coderd/database/queries/notificationsinbox.sql

+-- name: GetUnreadInboxNotificationsByUserIDFilteredByTemplatesAndTargets :many
+-- param user_id: The user ID
+-- param templates: The template IDs to filter by - the template_id = ANY(@templates::UUID[]) condition checks if the template_id is in the @templates array
+-- param targets: The target IDs to filter by - the targets @> COALESCE(@targets, ARRAY[]::UUID[]) condition checks if the targets array (from the DB) contains all the elements in the @targets array


As a caller, it's not really clear to me what kind of UUID a target should be without looking at example usages. Could this be reworded slightly to be clearer?

johnstcn · 2025-02-24T11:43:11Z

coderd/database/migrations/000296_notifications_inbox.up.sql

Obligatory reminder to double-check migration number before merging!

coderd/database/types.go

coderd/database/queries/notificationsinbox.sql

dannykopping · 2025-02-25T07:05:31Z

coderd/database/types.go

+type InboxNotificationReadStatus string
+
+const (
+	InboxNotificationReadStatusRead   InboxNotificationReadStatus = "READ"


Rather create a SQL type for this to get automatic codegen
Nit: make this lowercase to match other types' values

dannykopping · 2025-02-25T07:06:28Z

coderd/database/queries/notificationsinbox.sql

+	ORDER BY created_at DESC
+	LIMIT (COALESCE(NULLIF(@limit_opt :: INT, 0), 25));
+
+-- name: GetInboxNotificationsByUserIDFilteredByTemplatesAndTargets :many


Nit: for future-proofing, rather drop the specificity here in case we add more filter options later:

Suggested change

-- name: GetInboxNotificationsByUserIDFilteredByTemplatesAndTargets :many

-- name: GetFilteredInboxNotificationsByUserID :many

Renamed ✅

dannykopping · 2025-02-25T07:11:41Z

coderd/rbac/roles_test.go

+			},
+			Resource: rbac.ResourceInboxNotification.WithID(uuid.New()).InOrg(orgID).WithOwner(currentUser.String()),
+			AuthorizeMap: map[bool][]hasAuthSubjects{
+				true:  {owner, orgMemberMe, orgAdmin},


@defelmnq I understand why the current user should be able to read their own messages, but why should they be allowed to create new messages? That should be a system privilege.

dannykopping

LGTM 👍

defelmnq self-assigned this Feb 18, 2025

defelmnq added 7 commits February 20, 2025 11:02

work on db queries

90c7b64

work on dbauthz

e205049

work on dbauthz

ef91969

fmt and lint

72e6de4

change queries to match targets

9edb384

test dbauthz

dc74ac4

move notification migration

6c9e41f

defelmnq force-pushed the notif-inbox/int-334 branch from acc1f08 to 6c9e41f Compare February 20, 2025 11:03

defelmnq added 4 commits February 20, 2025 11:12

make gen

1f18eca

Merge remote-tracking branch 'origin/main' into notif-inbox/int-334

d02609f

rename inbox notifications

3bb9c57

rename inbox notifications

27c1592

fix rbac roles

defelmnq force-pushed the notif-inbox/int-334 branch from e035bf9 to 27c1592 Compare February 21, 2025 05:41

defelmnq added 4 commits February 21, 2025 06:31

Improve authz

08c55c3

add references in db

b441448

fix test icon

1814c58

improve foreign keys

9f46f51

defelmnq requested review from mafredri, dannykopping and DanielleMaywood February 21, 2025 07:00

defelmnq marked this pull request as ready for review February 21, 2025 07:01

defelmnq added 3 commits February 21, 2025 07:10

add back querier file

ddc65e6

improve uuid in seeding

d1ce11e

improve dbauthz testing

1f2fa24

dannykopping reviewed Feb 21, 2025

View reviewed changes

defelmnq added 2 commits February 21, 2025 11:33

format migration

791ed76

format migration

e141354

defelmnq commented Feb 21, 2025

View reviewed changes

format migration

a14723b

defelmnq added 9 commits February 21, 2025 14:41

rename functions

e413ab9

add back index

452583e

fix query name missing

5aa54e1

regen file

da56e8f

add count queries

30b5ac4

improve queries to add pagination

8230ef3

fix dbauthz tests

783bfe0

fix sqlc queries

c5aa917

improve dbauthz testing

da50947

defelmnq requested review from dannykopping and johnstcn February 24, 2025 07:04

johnstcn approved these changes Feb 24, 2025

View reviewed changes

defelmnq added 5 commits February 24, 2025 12:10

improve sql queries

c1da7f8

add type for read status

f03976c

iterate on testing dbauthz

94763e8

change zero value of timestamp

0c5d322

add returns check in tests

66d6fd6

defelmnq commented Feb 25, 2025

View reviewed changes

coderd/database/types.go Outdated Show resolved Hide resolved

defelmnq added 3 commits February 25, 2025 06:48

Merge remote-tracking branch 'origin/main' into notif-inbox/int-334

c45dfce

rename migration

b54dbef

improve queries comments

e2e895b

dannykopping reviewed Feb 25, 2025

View reviewed changes

cange type for read status

e6abcbd

dannykopping approved these changes Feb 25, 2025

View reviewed changes

merge

f691cf5

defelmnq merged commit c074f77 into main Mar 3, 2025
33 of 35 checks passed

defelmnq deleted the notif-inbox/int-334 branch March 3, 2025 09:12

github-actions bot locked and limited conversation to collaborators Mar 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add notifications inbox db #16599

feat: add notifications inbox db #16599

defelmnq commented Feb 18, 2025

dannykopping Feb 21, 2025

defelmnq Feb 21, 2025 •

edited

Loading

dannykopping Feb 25, 2025

dannykopping Feb 21, 2025

defelmnq Feb 21, 2025

johnstcn Feb 24, 2025

dannykopping Feb 25, 2025

dannykopping Feb 21, 2025

defelmnq Feb 24, 2025

johnstcn Feb 24, 2025

defelmnq Feb 21, 2025

defelmnq Feb 21, 2025

defelmnq Feb 21, 2025

defelmnq commented Feb 24, 2025

johnstcn left a comment

johnstcn Feb 24, 2025

johnstcn Feb 24, 2025

defelmnq Feb 24, 2025

johnstcn Feb 24, 2025

johnstcn Feb 24, 2025

dannykopping Feb 25, 2025

dannykopping Feb 25, 2025

defelmnq Feb 25, 2025

dannykopping Feb 25, 2025

dannykopping left a comment

		-- name: CountUnreadInboxNotificationsByUserID :one
		SELECT COUNT(*) FROM inbox_notifications WHERE user_id = $1 AND read_at IS NULL;

	-- name: GetInboxNotificationsByUserIDFilteredByTemplatesAndTargets :many
	-- name: GetFilteredInboxNotificationsByUserID :many

feat: add notifications inbox db #16599

feat: add notifications inbox db #16599

Conversation

defelmnq commented Feb 18, 2025

Choose a reason for hiding this comment

defelmnq Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defelmnq commented Feb 24, 2025

johnstcn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

defelmnq Feb 21, 2025 •

edited

Loading