[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>

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),

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)
+}
+
 // __________      .__  .__ __________                                     __
 // \______   \__ __|  | |  |\______   \ ____  ________ __   ____   _______/  |_
 //  |     ___/  |  \  | |  | |       _// __ \/ ____/  |  \_/ __ \ /  ___/\   __\

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

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})
 }

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

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(),
 		})

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"`
 }

routers/api/v1/swagger/options.go

@@ -123,4 +123,7 @@ type swaggerParameterBodies struct {
 
 	// in:body
 	RepoTopicOptions api.RepoTopicOptions
+
+	// in:body
+	EditReactionOption api.EditReactionOption
 }