[API] Fix 9544 | return 200 when reaction already exist (#9550)

* add ErrReactionAlreadyExist

* extend CreateReaction

* reaction already exist = 200

* extend FindReactionsOptions

* refactor swagger options/definitions

* fix swagger-validate

* Update models/error.go

Co-Authored-By: zeripath <art27@cantab.net>
* fix test PART1

* extend FindReactionsOptions with UserID option

* catch error on test

* fix test PART2

* format ...

Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: techknowlogick <matti@mdranta.net>

diff --git a/integrations/api_issue_reaction_test.go b/integrations/api_issue_reaction_test.go
index c474f7bad2541488af9b98e85ff31a677b547d4f..1906b8d09082c48e1e811f759fd808cd5c569439 100644 (file)
--- a/integrations/api_issue_reaction_test.go
+++ b/integrations/api_issue_reaction_test.go
@@ -47,7 +47,7 @@ func TestAPIIssuesReactions(t *testing.T) {
                Reaction: "rocket",
        })
        resp = session.MakeRequest(t, req, http.StatusCreated)
-       var apiNewReaction api.ReactionResponse
+       var apiNewReaction api.Reaction
        DecodeJSON(t, resp, &apiNewReaction)
 
        //Add existing reaction
@@ -56,10 +56,10 @@ func TestAPIIssuesReactions(t *testing.T) {
        //Get end result of reaction list of issue #1
        req = NewRequestf(t, "GET", urlStr)
        resp = session.MakeRequest(t, req, http.StatusOK)
-       var apiReactions []*api.ReactionResponse
+       var apiReactions []*api.Reaction
        DecodeJSON(t, resp, &apiReactions)
-       expectResponse := make(map[int]api.ReactionResponse)
-       expectResponse[0] = api.ReactionResponse{
+       expectResponse := make(map[int]api.Reaction)
+       expectResponse[0] = api.Reaction{
                User:     user2.APIFormat(),
                Reaction: "eyes",
                Created:  time.Unix(1573248003, 0),
@@ -107,7 +107,7 @@ func TestAPICommentReactions(t *testing.T) {
                Reaction: "+1",
        })
        resp = session.MakeRequest(t, req, http.StatusCreated)
-       var apiNewReaction api.ReactionResponse
+       var apiNewReaction api.Reaction
        DecodeJSON(t, resp, &apiNewReaction)
 
        //Add existing reaction
@@ -116,15 +116,15 @@ func TestAPICommentReactions(t *testing.T) {
        //Get end result of reaction list of issue #1
        req = NewRequestf(t, "GET", urlStr)
        resp = session.MakeRequest(t, req, http.StatusOK)
-       var apiReactions []*api.ReactionResponse
+       var apiReactions []*api.Reaction
        DecodeJSON(t, resp, &apiReactions)
-       expectResponse := make(map[int]api.ReactionResponse)
-       expectResponse[0] = api.ReactionResponse{
+       expectResponse := make(map[int]api.Reaction)
+       expectResponse[0] = api.Reaction{
                User:     user2.APIFormat(),
                Reaction: "laugh",
                Created:  time.Unix(1573248004, 0),
        }
-       expectResponse[1] = api.ReactionResponse{
+       expectResponse[1] = api.Reaction{
                User:     user1.APIFormat(),
                Reaction: "laugh",
                Created:  time.Unix(1573248005, 0),

diff --git a/models/error.go b/models/error.go
index 396d7594c8e93569a4fcdcc2f17fd4e0b75d6106..f0d5699aad6b86a8ec26587bddac6f4ca68ae8d5 100644 (file)
--- a/models/error.go
+++ b/models/error.go
@@ -1201,6 +1201,21 @@ func (err ErrForbiddenIssueReaction) Error() string {
        return fmt.Sprintf("'%s' is not an allowed reaction", err.Reaction)
 }
 
+// ErrReactionAlreadyExist is used when a existing reaction was try to created
+type ErrReactionAlreadyExist struct {
+       Reaction string
+}
+
+// IsErrReactionAlreadyExist checks if an error is a ErrReactionAlreadyExist.
+func IsErrReactionAlreadyExist(err error) bool {
+       _, ok := err.(ErrReactionAlreadyExist)
+       return ok
+}
+
+func (err ErrReactionAlreadyExist) Error() string {
+       return fmt.Sprintf("reaction '%s' already exists", err.Reaction)
+}
+
 // __________      .__  .__ __________                                     __
 // \______   \__ __|  | |  |\______   \ ____  ________ __   ____   _______/  |_
 //  |     ___/  |  \  | |  | |       _// __ \/ ____/  |  \_/ __ \ /  ___/\   __\

diff --git a/models/issue_reaction.go b/models/issue_reaction.go
index 6896eeeafc6c463899dd77b557ddea874e08dffb..d421ab44e92d4fdf187b1583428a4e5fff15dc8c 100644 (file)
--- a/models/issue_reaction.go
+++ b/models/issue_reaction.go
@@ -30,6 +30,8 @@ type Reaction struct {
 type FindReactionsOptions struct {
        IssueID   int64
        CommentID int64
+       UserID    int64
+       Reaction  string
 }
 
 func (opts *FindReactionsOptions) toConds() builder.Cond {
@@ -46,6 +48,12 @@ func (opts *FindReactionsOptions) toConds() builder.Cond {
        } else if opts.CommentID == -1 {
                cond = cond.And(builder.Eq{"reaction.comment_id": 0})
        }
+       if opts.UserID > 0 {
+               cond = cond.And(builder.Eq{"reaction.user_id": opts.UserID})
+       }
+       if opts.Reaction != "" {
+               cond = cond.And(builder.Eq{"reaction.type": opts.Reaction})
+       }
 
        return cond
 }
@@ -80,9 +88,25 @@ func createReaction(e *xorm.Session, opts *ReactionOptions) (*Reaction, error) {
                UserID:  opts.Doer.ID,
                IssueID: opts.Issue.ID,
        }
+       findOpts := FindReactionsOptions{
+               IssueID:   opts.Issue.ID,
+               CommentID: -1, // reaction to issue only
+               Reaction:  opts.Type,
+               UserID:    opts.Doer.ID,
+       }
        if opts.Comment != nil {
                reaction.CommentID = opts.Comment.ID
+               findOpts.CommentID = opts.Comment.ID
        }
+
+       existingR, err := findReactions(e, findOpts)
+       if err != nil {
+               return nil, err
+       }
+       if len(existingR) > 0 {
+               return existingR[0], ErrReactionAlreadyExist{Reaction: opts.Type}
+       }
+
        if _, err := e.Insert(reaction); err != nil {
                return nil, err
        }
@@ -99,23 +123,23 @@ type ReactionOptions struct {
 }
 
 // CreateReaction creates reaction for issue or comment.
-func CreateReaction(opts *ReactionOptions) (reaction *Reaction, err error) {
+func CreateReaction(opts *ReactionOptions) (*Reaction, error) {
        if !setting.UI.ReactionsMap[opts.Type] {
                return nil, ErrForbiddenIssueReaction{opts.Type}
        }
 
        sess := x.NewSession()
        defer sess.Close()
-       if err = sess.Begin(); err != nil {
+       if err := sess.Begin(); err != nil {
                return nil, err
        }
 
-       reaction, err = createReaction(sess, opts)
+       reaction, err := createReaction(sess, opts)
        if err != nil {
-               return nil, err
+               return reaction, err
        }
 
-       if err = sess.Commit(); err != nil {
+       if err := sess.Commit(); err != nil {
                return nil, err
        }
        return reaction, nil

diff --git a/models/issue_reaction_test.go b/models/issue_reaction_test.go
index 1189b389e9a9ce38d5584dc1d73e77a976ee860d..723a6be5368484a80bf078a4fdb6d4cd738646d2 100644 (file)
--- a/models/issue_reaction_test.go
+++ b/models/issue_reaction_test.go
@@ -50,9 +50,10 @@ func TestIssueAddDuplicateReaction(t *testing.T) {
                Type:  "heart",
        })
        assert.Error(t, err)
-       assert.Nil(t, reaction)
+       assert.Equal(t, ErrReactionAlreadyExist{Reaction: "heart"}, err)
 
-       AssertExistsAndLoadBean(t, &Reaction{Type: "heart", UserID: user1.ID, IssueID: issue1.ID})
+       existingR := AssertExistsAndLoadBean(t, &Reaction{Type: "heart", UserID: user1.ID, IssueID: issue1.ID}).(*Reaction)
+       assert.Equal(t, existingR.ID, reaction.ID)
 }
 
 func TestIssueDeleteReaction(t *testing.T) {
@@ -129,7 +130,6 @@ func TestIssueCommentDeleteReaction(t *testing.T) {
        user2 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
        user3 := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User)
        user4 := AssertExistsAndLoadBean(t, &User{ID: 4}).(*User)
-       ghost := NewGhostUser()
 
        issue1 := AssertExistsAndLoadBean(t, &Issue{ID: 1}).(*Issue)
 
@@ -139,14 +139,13 @@ func TestIssueCommentDeleteReaction(t *testing.T) {
        addReaction(t, user2, issue1, comment1, "heart")
        addReaction(t, user3, issue1, comment1, "heart")
        addReaction(t, user4, issue1, comment1, "+1")
-       addReaction(t, ghost, issue1, comment1, "heart")
 
        err := comment1.LoadReactions()
        assert.NoError(t, err)
-       assert.Len(t, comment1.Reactions, 5)
+       assert.Len(t, comment1.Reactions, 4)
 
        reactions := comment1.Reactions.GroupByType()
-       assert.Len(t, reactions["heart"], 4)
+       assert.Len(t, reactions["heart"], 3)
        assert.Len(t, reactions["+1"], 1)
 }
 
@@ -160,7 +159,7 @@ func TestIssueCommentReactionCount(t *testing.T) {
        comment1 := AssertExistsAndLoadBean(t, &Comment{ID: 1}).(*Comment)
 
        addReaction(t, user1, issue1, comment1, "heart")
-       DeleteCommentReaction(user1, issue1, comment1, "heart")
+       assert.NoError(t, DeleteCommentReaction(user1, issue1, comment1, "heart"))
 
        AssertNotExistsBean(t, &Reaction{Type: "heart", UserID: user1.ID, IssueID: issue1.ID, CommentID: comment1.ID})
 }

diff --git a/modules/structs/issue_reaction.go b/modules/structs/issue_reaction.go
index 9d717400520014b5383274ccc12beab285abd700..56408313ee4d3eaa0f2dc0e2a1e94ec996ca8780 100644 (file)
--- a/modules/structs/issue_reaction.go
+++ b/modules/structs/issue_reaction.go
@@ -13,8 +13,8 @@ type EditReactionOption struct {
        Reaction string `json:"content"`
 }
 
-// ReactionResponse contain one reaction
-type ReactionResponse struct {
+// Reaction contain one reaction
+type Reaction struct {
        User     *User  `json:"user"`
        Reaction string `json:"content"`
        // swagger:strfmt date-time

diff --git a/routers/api/v1/repo/issue_reaction.go b/routers/api/v1/repo/issue_reaction.go
index 4b06bb987c9fc8d8643d4f34ba2980e84daed008..bbc767cc99ad4221c297b39d0ddf057ec2901385 100644 (file)
--- a/routers/api/v1/repo/issue_reaction.go
+++ b/routers/api/v1/repo/issue_reaction.go
@@ -41,7 +41,7 @@ func GetIssueCommentReactions(ctx *context.APIContext) {
        //   required: true
        // responses:
        //   "200":
-       //     "$ref": "#/responses/ReactionResponseList"
+       //     "$ref": "#/responses/ReactionList"
        //   "403":
        //     "$ref": "#/responses/forbidden"
 
@@ -71,9 +71,9 @@ func GetIssueCommentReactions(ctx *context.APIContext) {
                return
        }
 
-       var result []api.ReactionResponse
+       var result []api.Reaction
        for _, r := range reactions {
-               result = append(result, api.ReactionResponse{
+               result = append(result, api.Reaction{
                        User:     r.User.APIFormat(),
                        Reaction: r.Type,
                        Created:  r.CreatedUnix.AsTime(),
@@ -114,8 +114,10 @@ func PostIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOpti
        //   schema:
        //     "$ref": "#/definitions/EditReactionOption"
        // responses:
+       //   "200":
+       //     "$ref": "#/responses/Reaction"
        //   "201":
-       //     "$ref": "#/responses/ReactionResponse"
+       //     "$ref": "#/responses/Reaction"
        //   "403":
        //     "$ref": "#/responses/forbidden"
 
@@ -188,19 +190,20 @@ func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOp
                if err != nil {
                        if models.IsErrForbiddenIssueReaction(err) {
                                ctx.Error(http.StatusForbidden, err.Error(), err)
+                       } else if models.IsErrReactionAlreadyExist(err) {
+                               ctx.JSON(http.StatusOK, api.Reaction{
+                                       User:     ctx.User.APIFormat(),
+                                       Reaction: reaction.Type,
+                                       Created:  reaction.CreatedUnix.AsTime(),
+                               })
                        } else {
                                ctx.Error(http.StatusInternalServerError, "CreateCommentReaction", err)
                        }
                        return
                }
-               _, err = reaction.LoadUser()
-               if err != nil {
-                       ctx.Error(http.StatusInternalServerError, "Reaction.LoadUser()", err)
-                       return
-               }
 
-               ctx.JSON(http.StatusCreated, api.ReactionResponse{
-                       User:     reaction.User.APIFormat(),
+               ctx.JSON(http.StatusCreated, api.Reaction{
+                       User:     ctx.User.APIFormat(),
                        Reaction: reaction.Type,
                        Created:  reaction.CreatedUnix.AsTime(),
                })
@@ -244,7 +247,7 @@ func GetIssueReactions(ctx *context.APIContext) {
        //   required: true
        // responses:
        //   "200":
-       //     "$ref": "#/responses/ReactionResponseList"
+       //     "$ref": "#/responses/ReactionList"
        //   "403":
        //     "$ref": "#/responses/forbidden"
 
@@ -274,9 +277,9 @@ func GetIssueReactions(ctx *context.APIContext) {
                return
        }
 
-       var result []api.ReactionResponse
+       var result []api.Reaction
        for _, r := range reactions {
-               result = append(result, api.ReactionResponse{
+               result = append(result, api.Reaction{
                        User:     r.User.APIFormat(),
                        Reaction: r.Type,
                        Created:  r.CreatedUnix.AsTime(),
@@ -317,8 +320,10 @@ func PostIssueReaction(ctx *context.APIContext, form api.EditReactionOption) {
        //   schema:
        //     "$ref": "#/definitions/EditReactionOption"
        // responses:
+       //   "200":
+       //     "$ref": "#/responses/Reaction"
        //   "201":
-       //     "$ref": "#/responses/ReactionResponse"
+       //     "$ref": "#/responses/Reaction"
        //   "403":
        //     "$ref": "#/responses/forbidden"
 
@@ -386,19 +391,20 @@ func changeIssueReaction(ctx *context.APIContext, form api.EditReactionOption, i
                if err != nil {
                        if models.IsErrForbiddenIssueReaction(err) {
                                ctx.Error(http.StatusForbidden, err.Error(), err)
+                       } else if models.IsErrReactionAlreadyExist(err) {
+                               ctx.JSON(http.StatusOK, api.Reaction{
+                                       User:     ctx.User.APIFormat(),
+                                       Reaction: reaction.Type,
+                                       Created:  reaction.CreatedUnix.AsTime(),
+                               })
                        } else {
                                ctx.Error(http.StatusInternalServerError, "CreateCommentReaction", err)
                        }
                        return
                }
-               _, err = reaction.LoadUser()
-               if err != nil {
-                       ctx.Error(http.StatusInternalServerError, "Reaction.LoadUser()", err)
-                       return
-               }
 
-               ctx.JSON(http.StatusCreated, api.ReactionResponse{
-                       User:     reaction.User.APIFormat(),
+               ctx.JSON(http.StatusCreated, api.Reaction{
+                       User:     ctx.User.APIFormat(),
                        Reaction: reaction.Type,
                        Created:  reaction.CreatedUnix.AsTime(),
                })

diff --git a/routers/api/v1/swagger/issue.go b/routers/api/v1/swagger/issue.go
index 68c0a9a38d694eb643bc02e9b7ed6f29f8a901a9..b12ea0096a4114cb486aef3b7044b8b499f05b78 100644 (file)
--- a/routers/api/v1/swagger/issue.go
+++ b/routers/api/v1/swagger/issue.go
@@ -99,23 +99,16 @@ type swaggerResponseStopWatchList struct {
        Body []api.StopWatch `json:"body"`
 }
 
-// EditReactionOption
-// swagger:response EditReactionOption
-type swaggerEditReactionOption struct {
+// Reaction
+// swagger:response Reaction
+type swaggerReaction struct {
        // in:body
-       Body api.EditReactionOption `json:"body"`
+       Body api.Reaction `json:"body"`
 }
 
-// ReactionResponse
-// swagger:response ReactionResponse
-type swaggerReactionResponse struct {
+// ReactionList
+// swagger:response ReactionList
+type swaggerReactionList struct {
        // in:body
-       Body api.ReactionResponse `json:"body"`
-}
-
-// ReactionResponseList
-// swagger:response ReactionResponseList
-type swaggerReactionResponseList struct {
-       // in:body
-       Body []api.ReactionResponse `json:"body"`
+       Body []api.Reaction `json:"body"`
 }

diff --git a/routers/api/v1/swagger/options.go b/routers/api/v1/swagger/options.go
index 74a475e2756663a99ae6c04e77060e00a89fe567..83cbb3a74d9dd3f2d634acdcdbc8b44e279bfeab 100644 (file)
--- a/routers/api/v1/swagger/options.go
+++ b/routers/api/v1/swagger/options.go
@@ -123,4 +123,7 @@ type swaggerParameterBodies struct {
 
        // in:body
        RepoTopicOptions api.RepoTopicOptions
+
+       // in:body
+       EditReactionOption api.EditReactionOption
 }

diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl
index 77def7aeab08aa32711fef9a52810b9edaf9a92f..f31b37cc4527dedee25b91536f9adcee3836c957 100644 (file)
--- a/templates/swagger/v1_json.tmpl
+++ b/templates/swagger/v1_json.tmpl
@@ -3130,7 +3130,7 @@
         ],
         "responses": {
           "200": {
-            "$ref": "#/responses/ReactionResponseList"
+            "$ref": "#/responses/ReactionList"
           },
           "403": {
             "$ref": "#/responses/forbidden"
@@ -3181,8 +3181,11 @@
           }
         ],
         "responses": {
+          "200": {
+            "$ref": "#/responses/Reaction"
+          },
           "201": {
-            "$ref": "#/responses/ReactionResponse"
+            "$ref": "#/responses/Reaction"
           },
           "403": {
             "$ref": "#/responses/forbidden"
@@ -3896,7 +3899,7 @@
         ],
         "responses": {
           "200": {
-            "$ref": "#/responses/ReactionResponseList"
+            "$ref": "#/responses/ReactionList"
           },
           "403": {
             "$ref": "#/responses/forbidden"
@@ -3947,8 +3950,11 @@
           }
         ],
         "responses": {
+          "200": {
+            "$ref": "#/responses/Reaction"
+          },
           "201": {
-            "$ref": "#/responses/ReactionResponse"
+            "$ref": "#/responses/Reaction"
           },
           "403": {
             "$ref": "#/responses/forbidden"
@@ -10822,8 +10828,8 @@
       },
       "x-go-package": "code.gitea.io/gitea/modules/structs"
     },
-    "ReactionResponse": {
-      "description": "ReactionResponse contain one reaction",
+    "Reaction": {
+      "description": "Reaction contain one reaction",
       "type": "object",
       "properties": {
         "content": {
@@ -11735,12 +11741,6 @@
         }
       }
     },
-    "EditReactionOption": {
-      "description": "EditReactionOption",
-      "schema": {
-        "$ref": "#/definitions/EditReactionOption"
-      }
-    },
     "EmailList": {
       "description": "EmailList",
       "schema": {
@@ -11927,18 +11927,18 @@
         }
       }
     },
-    "ReactionResponse": {
-      "description": "ReactionResponse",
+    "Reaction": {
+      "description": "Reaction",
       "schema": {
-        "$ref": "#/definitions/ReactionResponse"
+        "$ref": "#/definitions/Reaction"
       }
     },
-    "ReactionResponseList": {
-      "description": "ReactionResponseList",
+    "ReactionList": {
+      "description": "ReactionList",
       "schema": {
         "type": "array",
         "items": {
-          "$ref": "#/definitions/ReactionResponse"
+          "$ref": "#/definitions/Reaction"
         }
       }
     },
@@ -12164,7 +12164,7 @@
     "parameterBodies": {
       "description": "parameterBodies",
       "schema": {
-        "$ref": "#/definitions/RepoTopicOptions"
+        "$ref": "#/definitions/EditReactionOption"
       }
     },
     "redirect": {