work on PR comments

coder · defelmnq · Mar 17, 2025 · Mar 12, 2025 · Mar 12, 2025 · Mar 12, 2025
commit d72d1f260099609e4dbc5e2f563c176b1c196b32
diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go
diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json
diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -3296,6 +3296,7 @@ func (q *FakeQuerier) GetFilteredInboxNotificationsByUserID(_ context.Context, a
 	defer q.mutex.RUnlock()
 
 	notifications := make([]database.InboxNotification, 0)
+	// TODO : after using go version >= 1.23 , we can change this one to https://pkg.go.dev/slices#Backward
 	for idx := len(q.inboxNotifications) - 1; idx >= 0; idx-- {
 		notification := q.inboxNotifications[idx]
 

diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/notificationsinbox.sql b/coderd/database/queries/notificationsinbox.sql
@@ -21,7 +21,7 @@ SELECT * FROM inbox_notifications WHERE
 -- param limit_opt: The limit of notifications to fetch. If the limit is not specified, it defaults to 25
 SELECT * FROM inbox_notifications WHERE
 	user_id = @user_id AND
-	(template_id = ANY(@templates::UUID[]) OR @templates IS NULL) AND
+	(@templates::UUID[] IS NULL OR template_id = ANY(@templates::UUID[])) AND
 	(@targets::UUID[] IS NULL OR targets @> @targets::UUID[]) AND
 	(@read_status::inbox_notification_read_status = 'all' OR (@read_status::inbox_notification_read_status = 'unread' AND read_at IS NULL) OR (@read_status::inbox_notification_read_status = 'read' AND read_at IS NOT NULL)) AND
 	(@created_at_opt::TIMESTAMPTZ = '0001-01-01 00:00:00Z' OR created_at < @created_at_opt::TIMESTAMPTZ)

diff --git a/coderd/inboxnotifications.go b/coderd/inboxnotifications.go
@@ -140,7 +140,7 @@ func (api *API) watchInboxNotifications(rw http.ResponseWriter, r *http.Request)
 	go httpapi.Heartbeat(ctx, conn)
 	defer conn.Close(websocket.StatusNormalClosure, "connection closed")
 
-	notificationCh := make(chan codersdk.InboxNotification, 1)
+	notificationCh := make(chan codersdk.InboxNotification, 10)
 
 	closeInboxNotificationsSubscriber, err := api.Pubsub.SubscribeWithErr(pubsub.InboxNotificationForOwnerEventChannel(apikey.UserID),
 		pubsub.HandleInboxNotificationEvent(
@@ -150,6 +150,10 @@ func (api *API) watchInboxNotifications(rw http.ResponseWriter, r *http.Request)
 					return
 				}
 
+				// HandleInboxNotificationEvent cb receives all the inbox notifications - without any filters excepted the user_id.
+				// Based on query parameters defined above and filters defined by the client - we then filter out the
+				// notifications we do not want to forward and discard it.
+
 				// filter out notifications that don't match the targets
 				if len(targets) > 0 {
 					for _, target := range targets {
@@ -179,7 +183,11 @@ func (api *API) watchInboxNotifications(rw http.ResponseWriter, r *http.Request)
 					}
 				}
 
-				notificationCh <- payload.InboxNotification
+				// keep a safe guard in case of latency to push notifications through websocket
+				select {
+				case notificationCh <- payload.InboxNotification:
+				default:
+				}
 			},
 		))
 	if err != nil {
@@ -306,7 +314,7 @@ func (api *API) listInboxNotifications(rw http.ResponseWriter, r *http.Request)
 // @Produce json
 // @Tags Notifications
 // @Param id path string true "id of the notification"
-// @Success 201 {object} codersdk.Response
+// @Success 200 {object} codersdk.Response
 // @Router /notifications/inbox/{id}/read-status [put]
 func (api *API) updateInboxNotificationReadStatus(rw http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()

diff --git a/coderd/inboxnotifications_test.go b/coderd/inboxnotifications_test.go
@@ -19,6 +19,10 @@ import (
 	"github.com/coder/coder/v2/testutil"
 )
 
+const (
+	inboxNotificationsPageSize = 25
+)
+
 func TestInboxNotifications_List(t *testing.T) {
 	t.Parallel()
 
@@ -33,7 +37,8 @@ func TestInboxNotifications_List(t *testing.T) {
 		t.Parallel()
 
 		client, _, _ := coderdtest.NewWithAPI(t, &coderdtest.Options{})
-		_ = coderdtest.CreateFirstUser(t, client)
+		firstUser := coderdtest.CreateFirstUser(t, client)
+		client, _ = coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID)
 
 		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
 		defer cancel()
@@ -51,6 +56,7 @@ func TestInboxNotifications_List(t *testing.T) {
 
 		client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{})
 		firstUser := coderdtest.CreateFirstUser(t, client)
+		client, member := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID)
 
 		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
 		defer cancel()
@@ -64,7 +70,7 @@ func TestInboxNotifications_List(t *testing.T) {
 		for i := range 40 {
 			dbgen.NotificationInbox(t, api.Database, database.InsertInboxNotificationParams{
 				ID:         uuid.New(),
-				UserID:     firstUser.UserID,
+				UserID:     member.ID,
 				TemplateID: notifications.TemplateWorkspaceOutOfMemory,
 				Title:      fmt.Sprintf("Notification %d", i),
 				Actions:    json.RawMessage("[]"),
@@ -77,12 +83,12 @@ func TestInboxNotifications_List(t *testing.T) {
 		require.NoError(t, err)
 		require.NotNil(t, notifs)
 		require.Equal(t, 40, notifs.UnreadCount)
-		require.Len(t, notifs.Notifications, 25)
+		require.Len(t, notifs.Notifications, inboxNotificationsPageSize)
 
 		require.Equal(t, "Notification 39", notifs.Notifications[0].Title)
 
 		notifs, err = client.ListInboxNotifications(ctx, codersdk.ListInboxNotificationsRequest{
-			StartingBefore: notifs.Notifications[24].ID,
+			StartingBefore: notifs.Notifications[inboxNotificationsPageSize-1].ID,
 		})
 		require.NoError(t, err)
 		require.NotNil(t, notifs)
@@ -97,6 +103,7 @@ func TestInboxNotifications_List(t *testing.T) {
 
 		client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{})
 		firstUser := coderdtest.CreateFirstUser(t, client)
+		client, member := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID)
 
 		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
 		defer cancel()
@@ -110,7 +117,7 @@ func TestInboxNotifications_List(t *testing.T) {
 		for i := range 10 {
 			dbgen.NotificationInbox(t, api.Database, database.InsertInboxNotificationParams{
 				ID:     uuid.New(),
-				UserID: firstUser.UserID,
+				UserID: member.ID,
 				TemplateID: func() uuid.UUID {
 					if i%2 == 0 {
 						return notifications.TemplateWorkspaceOutOfMemory
@@ -141,6 +148,7 @@ func TestInboxNotifications_List(t *testing.T) {
 
 		client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{})
 		firstUser := coderdtest.CreateFirstUser(t, client)
+		client, member := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID)
 
 		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
 		defer cancel()
@@ -156,7 +164,7 @@ func TestInboxNotifications_List(t *testing.T) {
 		for i := range 10 {
 			dbgen.NotificationInbox(t, api.Database, database.InsertInboxNotificationParams{
 				ID:         uuid.New(),
-				UserID:     firstUser.UserID,
+				UserID:     member.ID,
 				TemplateID: notifications.TemplateWorkspaceOutOfMemory,
 				Targets: func() []uuid.UUID {
 					if i%2 == 0 {
@@ -188,6 +196,7 @@ func TestInboxNotifications_List(t *testing.T) {
 
 		client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{})
 		firstUser := coderdtest.CreateFirstUser(t, client)
+		client, member := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID)
 
 		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
 		defer cancel()
@@ -203,7 +212,7 @@ func TestInboxNotifications_List(t *testing.T) {
 		for i := range 10 {
 			dbgen.NotificationInbox(t, api.Database, database.InsertInboxNotificationParams{
 				ID:     uuid.New(),
-				UserID: firstUser.UserID,
+				UserID: member.ID,
 				TemplateID: func() uuid.UUID {
 					if i < 5 {
 						return notifications.TemplateWorkspaceOutOfMemory
@@ -250,6 +259,7 @@ func TestInboxNotifications_ReadStatus(t *testing.T) {
 
 	client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{})
 	firstUser := coderdtest.CreateFirstUser(t, client)
+	client, member := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID)
 
 	ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
 	defer cancel()
@@ -263,7 +273,7 @@ func TestInboxNotifications_ReadStatus(t *testing.T) {
 	for i := range 20 {
 		dbgen.NotificationInbox(t, api.Database, database.InsertInboxNotificationParams{
 			ID:         uuid.New(),
-			UserID:     firstUser.UserID,
+			UserID:     member.ID,
 			TemplateID: notifications.TemplateWorkspaceOutOfMemory,
 			Title:      fmt.Sprintf("Notification %d", i),
 			Actions:    json.RawMessage("[]"),

diff --git a/coderd/notifications/manager.go b/coderd/notifications/manager.go
@@ -76,7 +76,7 @@ func WithTestClock(clock quartz.Clock) ManagerOption {
 //
 // helpers is a map of template helpers which are used to customize notification messages to use global settings like
 // access URL etc.
-func NewManager(cfg codersdk.NotificationsConfig, store Store, ps pubsub.Pubsub, helpers template.FuncMap, metrics *Metrics, log slog.Logger, opts ...ManagerOption) (*Manager, error) { // TODO(dannyk): add the ability to use multiple notification methods.
+func NewManager(cfg codersdk.NotificationsConfig, store Store, ps pubsub.Pubsub, helpers template.FuncMap, metrics *Metrics, log slog.Logger, opts ...ManagerOption) (*Manager, error) {
 	var method database.NotificationMethod
 	if err := method.Scan(cfg.Method.String()); err != nil {
 		return nil, xerrors.Errorf("notification method %q is invalid", cfg.Method)

diff --git a/coderd/pubsub/inboxnotification.go b/coderd/pubsub/inboxnotification.go
@@ -39,6 +39,5 @@ type InboxNotificationEvent struct {
 type InboxNotificationEventKind string
 
 const (
-	InboxNotificationEventKindNew  InboxNotificationEventKind = "new"
-	InboxNotificationEventKindRead InboxNotificationEventKind = "read"
+	InboxNotificationEventKindNew InboxNotificationEventKind = "new"
 )
diff --git a/coderd/util/uuid/uuid.go b/coderd/util/uuid/uuid.go
@@ -0,0 +1,17 @@
+package uuid
+
+import (
+	"strings"
+
+	"github.com/google/uuid"
+)
+
+func FromSliceToString(uuids []uuid.UUID, separator string) string {
+	uuidStrings := make([]string, 0, len(uuids))
+
+	for _, u := range uuids {
+		uuidStrings = append(uuidStrings, u.String())
+	}
+
+	return strings.Join(uuidStrings, separator)
+}
diff --git a/codersdk/inboxnotification.go b/codersdk/inboxnotification.go
@@ -8,6 +8,8 @@ import (
 	"time"
 
 	"github.com/google/uuid"
+
+	utiluuid "github.com/coder/coder/v2/coderd/util/uuid"
 )
 
 type InboxNotification struct {
@@ -33,18 +35,6 @@ type GetInboxNotificationResponse struct {
 	UnreadCount  int               `json:"unread_count"`
 }
 
-func uuidSliceToString(s []uuid.UUID) string {
-	resp := ""
-	for idx, v := range s {
-		resp += v.String()
-		if idx < len(s)-1 {
-			resp += ","
-		}
-	}
-
-	return resp
-}
-
 type ListInboxNotificationsRequest struct {
 	Targets        []uuid.UUID
 	Templates      []uuid.UUID
@@ -60,10 +50,10 @@ type ListInboxNotificationsResponse struct {
 func ListInboxNotificationsRequestToQueryParams(req ListInboxNotificationsRequest) []RequestOption {
 	var opts []RequestOption
 	if len(req.Targets) > 0 {
-		opts = append(opts, WithQueryParam("targets", uuidSliceToString(req.Targets)))
+		opts = append(opts, WithQueryParam("targets", utiluuid.FromSliceToString(req.Targets, ",")))
 	}
 	if len(req.Templates) > 0 {
-		opts = append(opts, WithQueryParam("templates", uuidSliceToString(req.Templates)))
+		opts = append(opts, WithQueryParam("templates", utiluuid.FromSliceToString(req.Templates, ",")))
 	}
 	if req.ReadStatus != "" {
 		opts = append(opts, WithQueryParam("read_status", req.ReadStatus))

diff --git a/docs/reference/api/notifications.md b/docs/reference/api/notifications.md