diff --git a/apps/api/internal/handler/comment.go b/apps/api/internal/handler/comment.go index 2db7ada6..28dc68e7 100644 --- a/apps/api/internal/handler/comment.go +++ b/apps/api/internal/handler/comment.go @@ -1,6 +1,7 @@ package handler import ( + "errors" "net/http" "github.com/Devlaner/devlane/api/internal/middleware" @@ -9,6 +10,14 @@ import ( "github.com/google/uuid" ) +// commentAccessNotFound reports whether err is one of the access/lookup errors +// that should surface as a 404 to the client. +func commentAccessNotFound(err error) bool { + return errors.Is(err, service.ErrCommentNotFound) || + errors.Is(err, service.ErrProjectForbidden) || + errors.Is(err, service.ErrProjectNotFound) +} + // CommentHandler serves issue comments. type CommentHandler struct { Comment *service.CommentService @@ -209,7 +218,14 @@ func (h *CommentHandler) AddReaction(c *gin.Context) { } r, err := h.Comment.AddReaction(c.Request.Context(), slug, projectID, commentID, user.ID, body.Reaction) if err != nil { - c.JSON(http.StatusConflict, gin.H{"error": err.Error()}) + switch { + case errors.Is(err, service.ErrReactionExists): + c.JSON(http.StatusConflict, gin.H{"error": "Already reacted"}) + case commentAccessNotFound(err): + c.JSON(http.StatusNotFound, gin.H{"error": "Not found"}) + default: + c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to add reaction"}) + } return } c.JSON(http.StatusCreated, r) @@ -240,6 +256,10 @@ func (h *CommentHandler) RemoveReaction(c *gin.Context) { return } if err := h.Comment.RemoveReaction(c.Request.Context(), slug, projectID, commentID, user.ID, reaction); err != nil { + if commentAccessNotFound(err) { + c.JSON(http.StatusNotFound, gin.H{"error": "Not found"}) + return + } c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to remove reaction"}) return } diff --git a/apps/api/internal/handler/comment_reaction_test.go b/apps/api/internal/handler/comment_reaction_test.go new file mode 100644 index 00000000..8a65d0cb --- /dev/null +++ b/apps/api/internal/handler/comment_reaction_test.go @@ -0,0 +1,55 @@ +package handler_test + +import ( + "net/http" + "testing" + + "github.com/Devlaner/devlane/api/internal/testutil" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +func TestComment_AddReaction_StatusCodes(t *testing.T) { + ts := testutil.NewTestServer(t) + w := testutil.SeedWorld(t, ts.DB) + issue := testutil.CreateIssue(t, ts.DB, w.Project.ID, w.Workspace.ID, w.User.ID) + comment := testutil.CreateComment(t, ts.DB, issue.ID, w.Project.ID, w.Workspace.ID, w.User.ID) + + base := "/api/workspaces/" + w.Workspace.Slug + "/projects/" + w.Project.ID.String() + + "/issues/" + issue.ID.String() + "/comments/" + comment.ID.String() + "/reactions/" + + // First reaction succeeds. + rr := ts.POST(base, map[string]any{"reaction": "👍"}, w.Session) + require.Equal(t, http.StatusCreated, rr.Code, "body=%s", rr.Body.String()) + + // Reacting again with the same emoji is a conflict, not a leaked 409-for-all. + rr2 := ts.POST(base, map[string]any{"reaction": "👍"}, w.Session) + require.Equal(t, http.StatusConflict, rr2.Code, "body=%s", rr2.Body.String()) + + // A missing comment is a 404, not a 409. + missing := "/api/workspaces/" + w.Workspace.Slug + "/projects/" + w.Project.ID.String() + + "/issues/" + issue.ID.String() + "/comments/" + uuid.NewString() + "/reactions/" + rr3 := ts.POST(missing, map[string]any{"reaction": "👍"}, w.Session) + require.Equal(t, http.StatusNotFound, rr3.Code, "body=%s", rr3.Body.String()) +} + +func TestComment_RemoveReaction_StatusCodes(t *testing.T) { + ts := testutil.NewTestServer(t) + w := testutil.SeedWorld(t, ts.DB) + issue := testutil.CreateIssue(t, ts.DB, w.Project.ID, w.Workspace.ID, w.User.ID) + comment := testutil.CreateComment(t, ts.DB, issue.ID, w.Project.ID, w.Workspace.ID, w.User.ID) + + base := "/api/workspaces/" + w.Workspace.Slug + "/projects/" + w.Project.ID.String() + + "/issues/" + issue.ID.String() + "/comments/" + comment.ID.String() + "/reactions/" + + // Add then remove succeeds. + require.Equal(t, http.StatusCreated, ts.POST(base, map[string]any{"reaction": "🎉"}, w.Session).Code) + rr := ts.DELETE(base+"%F0%9F%8E%89/", w.Session) + require.Equal(t, http.StatusNoContent, rr.Code, "body=%s", rr.Body.String()) + + // Removing from a missing comment is a 404, not a generic 500. + missing := "/api/workspaces/" + w.Workspace.Slug + "/projects/" + w.Project.ID.String() + + "/issues/" + issue.ID.String() + "/comments/" + uuid.NewString() + "/reactions/x/" + rr2 := ts.DELETE(missing, w.Session) + require.Equal(t, http.StatusNotFound, rr2.Code, "body=%s", rr2.Body.String()) +} diff --git a/apps/api/internal/service/comment.go b/apps/api/internal/service/comment.go index dc7b4268..a8c3e93e 100644 --- a/apps/api/internal/service/comment.go +++ b/apps/api/internal/service/comment.go @@ -8,6 +8,7 @@ import ( "github.com/Devlaner/devlane/api/internal/store" "github.com/Devlaner/devlane/api/internal/text" "github.com/google/uuid" + "gorm.io/gorm" ) var ErrCommentNotFound = errors.New("comment not found") @@ -188,8 +189,10 @@ func (s *CommentService) AddReaction(ctx context.Context, workspaceSlug string, WorkspaceID: c.WorkspaceID, } if err := s.reactions.Add(ctx, r); err != nil { - // Unique-constraint violation = already reacted, return existing row. - // We don't bother fetching it; caller can refetch the list. + // Unique-constraint violation = the user already reacted with this emoji. + if errors.Is(err, gorm.ErrDuplicatedKey) { + return nil, ErrReactionExists + } return nil, err } return r, nil