From 9600c27085df3f895b2128f1b77aa0ce0b57e7f2 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 31 Dec 2019 09:21:21 +0100 Subject: [PATCH] [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 * fix test PART1 * extend FindReactionsOptions with UserID option * catch error on test * fix test PART2 * format ... Co-authored-by: zeripath Co-authored-by: techknowlogick --- integrations/api_issue_reaction_test.go | 18 ++++----- models/error.go | 15 ++++++++ models/issue_reaction.go | 34 ++++++++++++++--- models/issue_reaction_test.go | 13 +++---- modules/structs/issue_reaction.go | 4 +- routers/api/v1/repo/issue_reaction.go | 50 ++++++++++++++----------- routers/api/v1/swagger/issue.go | 23 ++++-------- routers/api/v1/swagger/options.go | 3 ++ templates/swagger/v1_json.tmpl | 38 +++++++++---------- 9 files changed, 119 insertions(+), 79 deletions(-) diff --git a/integrations/api_issue_reaction_test.go b/integrations/api_issue_reaction_test.go index c474f7bad..1906b8d09 100644 --- 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 396d7594c..f0d5699aa 100644 --- 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 6896eeeaf..d421ab44e 100644 --- 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 1189b389e..723a6be53 100644 --- 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 9d7174005..56408313e 100644 --- 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 4b06bb987..bbc767cc9 100644 --- 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 68c0a9a38..b12ea0096 100644 --- 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 74a475e27..83cbb3a74 100644 --- 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 77def7aea..f31b37cc4 100644 --- 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": {