Skip to content

Commit 9600c27

Browse files
6543zeripathtechknowlogick
committed
[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 <[email protected]> * fix test PART1 * extend FindReactionsOptions with UserID option * catch error on test * fix test PART2 * format ... Co-authored-by: zeripath <[email protected]> Co-authored-by: techknowlogick <[email protected]>
1 parent 655aea1 commit 9600c27

File tree

9 files changed

+119
-79
lines changed

9 files changed

+119
-79
lines changed

‎integrations/api_issue_reaction_test.go

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ func TestAPIIssuesReactions(t *testing.T) {
4747
Reaction: "rocket",
4848
})
4949
resp = session.MakeRequest(t, req, http.StatusCreated)
50-
var apiNewReaction api.ReactionResponse
50+
var apiNewReaction api.Reaction
5151
DecodeJSON(t, resp, &apiNewReaction)
5252

5353
//Add existing reaction
@@ -56,10 +56,10 @@ func TestAPIIssuesReactions(t *testing.T) {
5656
//Get end result of reaction list of issue #1
5757
req = NewRequestf(t, "GET", urlStr)
5858
resp = session.MakeRequest(t, req, http.StatusOK)
59-
var apiReactions []*api.ReactionResponse
59+
var apiReactions []*api.Reaction
6060
DecodeJSON(t, resp, &apiReactions)
61-
expectResponse := make(map[int]api.ReactionResponse)
62-
expectResponse[0] = api.ReactionResponse{
61+
expectResponse := make(map[int]api.Reaction)
62+
expectResponse[0] = api.Reaction{
6363
User: user2.APIFormat(),
6464
Reaction: "eyes",
6565
Created: time.Unix(1573248003, 0),
@@ -107,7 +107,7 @@ func TestAPICommentReactions(t *testing.T) {
107107
Reaction: "+1",
108108
})
109109
resp = session.MakeRequest(t, req, http.StatusCreated)
110-
var apiNewReaction api.ReactionResponse
110+
var apiNewReaction api.Reaction
111111
DecodeJSON(t, resp, &apiNewReaction)
112112

113113
//Add existing reaction
@@ -116,15 +116,15 @@ func TestAPICommentReactions(t *testing.T) {
116116
//Get end result of reaction list of issue #1
117117
req = NewRequestf(t, "GET", urlStr)
118118
resp = session.MakeRequest(t, req, http.StatusOK)
119-
var apiReactions []*api.ReactionResponse
119+
var apiReactions []*api.Reaction
120120
DecodeJSON(t, resp, &apiReactions)
121-
expectResponse := make(map[int]api.ReactionResponse)
122-
expectResponse[0] = api.ReactionResponse{
121+
expectResponse := make(map[int]api.Reaction)
122+
expectResponse[0] = api.Reaction{
123123
User: user2.APIFormat(),
124124
Reaction: "laugh",
125125
Created: time.Unix(1573248004, 0),
126126
}
127-
expectResponse[1] = api.ReactionResponse{
127+
expectResponse[1] = api.Reaction{
128128
User: user1.APIFormat(),
129129
Reaction: "laugh",
130130
Created: time.Unix(1573248005, 0),

‎models/error.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1201,6 +1201,21 @@ func (err ErrForbiddenIssueReaction) Error() string {
12011201
return fmt.Sprintf("'%s' is not an allowed reaction", err.Reaction)
12021202
}
12031203

1204+
// ErrReactionAlreadyExist is used when a existing reaction was try to created
1205+
type ErrReactionAlreadyExist struct {
1206+
Reaction string
1207+
}
1208+
1209+
// IsErrReactionAlreadyExist checks if an error is a ErrReactionAlreadyExist.
1210+
func IsErrReactionAlreadyExist(err error) bool {
1211+
_, ok := err.(ErrReactionAlreadyExist)
1212+
return ok
1213+
}
1214+
1215+
func (err ErrReactionAlreadyExist) Error() string {
1216+
return fmt.Sprintf("reaction '%s' already exists", err.Reaction)
1217+
}
1218+
12041219
// __________ .__ .__ __________ __
12051220
// \______ \__ __| | | |\______ \ ____ ________ __ ____ _______/ |_
12061221
// | ___/ | \ | | | | _// __ \/ ____/ | \_/ __ \ / ___/\ __\

‎models/issue_reaction.go

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ type Reaction struct {
3030
type FindReactionsOptions struct {
3131
IssueID int64
3232
CommentID int64
33+
UserID int64
34+
Reaction string
3335
}
3436

3537
func (opts *FindReactionsOptions) toConds() builder.Cond {
@@ -46,6 +48,12 @@ func (opts *FindReactionsOptions) toConds() builder.Cond {
4648
} else if opts.CommentID == -1 {
4749
cond = cond.And(builder.Eq{"reaction.comment_id": 0})
4850
}
51+
if opts.UserID > 0 {
52+
cond = cond.And(builder.Eq{"reaction.user_id": opts.UserID})
53+
}
54+
if opts.Reaction != "" {
55+
cond = cond.And(builder.Eq{"reaction.type": opts.Reaction})
56+
}
4957

5058
return cond
5159
}
@@ -80,9 +88,25 @@ func createReaction(e *xorm.Session, opts *ReactionOptions) (*Reaction, error) {
8088
UserID: opts.Doer.ID,
8189
IssueID: opts.Issue.ID,
8290
}
91+
findOpts := FindReactionsOptions{
92+
IssueID: opts.Issue.ID,
93+
CommentID: -1, // reaction to issue only
94+
Reaction: opts.Type,
95+
UserID: opts.Doer.ID,
96+
}
8397
if opts.Comment != nil {
8498
reaction.CommentID = opts.Comment.ID
99+
findOpts.CommentID = opts.Comment.ID
85100
}
101+
102+
existingR, err := findReactions(e, findOpts)
103+
if err != nil {
104+
return nil, err
105+
}
106+
if len(existingR) > 0 {
107+
return existingR[0], ErrReactionAlreadyExist{Reaction: opts.Type}
108+
}
109+
86110
if _, err := e.Insert(reaction); err != nil {
87111
return nil, err
88112
}
@@ -99,23 +123,23 @@ type ReactionOptions struct {
99123
}
100124

101125
// CreateReaction creates reaction for issue or comment.
102-
func CreateReaction(opts *ReactionOptions) (reaction *Reaction, err error) {
126+
func CreateReaction(opts *ReactionOptions) (*Reaction, error) {
103127
if !setting.UI.ReactionsMap[opts.Type] {
104128
return nil, ErrForbiddenIssueReaction{opts.Type}
105129
}
106130

107131
sess := x.NewSession()
108132
defer sess.Close()
109-
if err = sess.Begin(); err != nil {
133+
if err := sess.Begin(); err != nil {
110134
return nil, err
111135
}
112136

113-
reaction, err = createReaction(sess, opts)
137+
reaction, err := createReaction(sess, opts)
114138
if err != nil {
115-
return nil, err
139+
return reaction, err
116140
}
117141

118-
if err = sess.Commit(); err != nil {
142+
if err := sess.Commit(); err != nil {
119143
return nil, err
120144
}
121145
return reaction, nil

‎models/issue_reaction_test.go

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,10 @@ func TestIssueAddDuplicateReaction(t *testing.T) {
5050
Type: "heart",
5151
})
5252
assert.Error(t, err)
53-
assert.Nil(t, reaction)
53+
assert.Equal(t, ErrReactionAlreadyExist{Reaction: "heart"}, err)
5454

55-
AssertExistsAndLoadBean(t, &Reaction{Type: "heart", UserID: user1.ID, IssueID: issue1.ID})
55+
existingR := AssertExistsAndLoadBean(t, &Reaction{Type: "heart", UserID: user1.ID, IssueID: issue1.ID}).(*Reaction)
56+
assert.Equal(t, existingR.ID, reaction.ID)
5657
}
5758

5859
func TestIssueDeleteReaction(t *testing.T) {
@@ -129,7 +130,6 @@ func TestIssueCommentDeleteReaction(t *testing.T) {
129130
user2 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
130131
user3 := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User)
131132
user4 := AssertExistsAndLoadBean(t, &User{ID: 4}).(*User)
132-
ghost := NewGhostUser()
133133

134134
issue1 := AssertExistsAndLoadBean(t, &Issue{ID: 1}).(*Issue)
135135

@@ -139,14 +139,13 @@ func TestIssueCommentDeleteReaction(t *testing.T) {
139139
addReaction(t, user2, issue1, comment1, "heart")
140140
addReaction(t, user3, issue1, comment1, "heart")
141141
addReaction(t, user4, issue1, comment1, "+1")
142-
addReaction(t, ghost, issue1, comment1, "heart")
143142

144143
err := comment1.LoadReactions()
145144
assert.NoError(t, err)
146-
assert.Len(t, comment1.Reactions, 5)
145+
assert.Len(t, comment1.Reactions, 4)
147146

148147
reactions := comment1.Reactions.GroupByType()
149-
assert.Len(t, reactions["heart"], 4)
148+
assert.Len(t, reactions["heart"], 3)
150149
assert.Len(t, reactions["+1"], 1)
151150
}
152151

@@ -160,7 +159,7 @@ func TestIssueCommentReactionCount(t *testing.T) {
160159
comment1 := AssertExistsAndLoadBean(t, &Comment{ID: 1}).(*Comment)
161160

162161
addReaction(t, user1, issue1, comment1, "heart")
163-
DeleteCommentReaction(user1, issue1, comment1, "heart")
162+
assert.NoError(t, DeleteCommentReaction(user1, issue1, comment1, "heart"))
164163

165164
AssertNotExistsBean(t, &Reaction{Type: "heart", UserID: user1.ID, IssueID: issue1.ID, CommentID: comment1.ID})
166165
}

‎modules/structs/issue_reaction.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ type EditReactionOption struct {
1313
Reaction string `json:"content"`
1414
}
1515

16-
// ReactionResponse contain one reaction
17-
type ReactionResponse struct {
16+
// Reaction contain one reaction
17+
type Reaction struct {
1818
User *User `json:"user"`
1919
Reaction string `json:"content"`
2020
// swagger:strfmt date-time

‎routers/api/v1/repo/issue_reaction.go

Lines changed: 28 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ func GetIssueCommentReactions(ctx *context.APIContext) {
4141
// required: true
4242
// responses:
4343
// "200":
44-
// "$ref": "#/responses/ReactionResponseList"
44+
// "$ref": "#/responses/ReactionList"
4545
// "403":
4646
// "$ref": "#/responses/forbidden"
4747

@@ -71,9 +71,9 @@ func GetIssueCommentReactions(ctx *context.APIContext) {
7171
return
7272
}
7373

74-
var result []api.ReactionResponse
74+
var result []api.Reaction
7575
for _, r := range reactions {
76-
result = append(result, api.ReactionResponse{
76+
result = append(result, api.Reaction{
7777
User: r.User.APIFormat(),
7878
Reaction: r.Type,
7979
Created: r.CreatedUnix.AsTime(),
@@ -114,8 +114,10 @@ func PostIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOpti
114114
// schema:
115115
// "$ref": "#/definitions/EditReactionOption"
116116
// responses:
117+
// "200":
118+
// "$ref": "#/responses/Reaction"
117119
// "201":
118-
// "$ref": "#/responses/ReactionResponse"
120+
// "$ref": "#/responses/Reaction"
119121
// "403":
120122
// "$ref": "#/responses/forbidden"
121123

@@ -188,19 +190,20 @@ func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOp
188190
if err != nil {
189191
if models.IsErrForbiddenIssueReaction(err) {
190192
ctx.Error(http.StatusForbidden, err.Error(), err)
193+
} else if models.IsErrReactionAlreadyExist(err) {
194+
ctx.JSON(http.StatusOK, api.Reaction{
195+
User: ctx.User.APIFormat(),
196+
Reaction: reaction.Type,
197+
Created: reaction.CreatedUnix.AsTime(),
198+
})
191199
} else {
192200
ctx.Error(http.StatusInternalServerError, "CreateCommentReaction", err)
193201
}
194202
return
195203
}
196-
_, err = reaction.LoadUser()
197-
if err != nil {
198-
ctx.Error(http.StatusInternalServerError, "Reaction.LoadUser()", err)
199-
return
200-
}
201204

202-
ctx.JSON(http.StatusCreated, api.ReactionResponse{
203-
User: reaction.User.APIFormat(),
205+
ctx.JSON(http.StatusCreated, api.Reaction{
206+
User: ctx.User.APIFormat(),
204207
Reaction: reaction.Type,
205208
Created: reaction.CreatedUnix.AsTime(),
206209
})
@@ -244,7 +247,7 @@ func GetIssueReactions(ctx *context.APIContext) {
244247
// required: true
245248
// responses:
246249
// "200":
247-
// "$ref": "#/responses/ReactionResponseList"
250+
// "$ref": "#/responses/ReactionList"
248251
// "403":
249252
// "$ref": "#/responses/forbidden"
250253

@@ -274,9 +277,9 @@ func GetIssueReactions(ctx *context.APIContext) {
274277
return
275278
}
276279

277-
var result []api.ReactionResponse
280+
var result []api.Reaction
278281
for _, r := range reactions {
279-
result = append(result, api.ReactionResponse{
282+
result = append(result, api.Reaction{
280283
User: r.User.APIFormat(),
281284
Reaction: r.Type,
282285
Created: r.CreatedUnix.AsTime(),
@@ -317,8 +320,10 @@ func PostIssueReaction(ctx *context.APIContext, form api.EditReactionOption) {
317320
// schema:
318321
// "$ref": "#/definitions/EditReactionOption"
319322
// responses:
323+
// "200":
324+
// "$ref": "#/responses/Reaction"
320325
// "201":
321-
// "$ref": "#/responses/ReactionResponse"
326+
// "$ref": "#/responses/Reaction"
322327
// "403":
323328
// "$ref": "#/responses/forbidden"
324329

@@ -386,19 +391,20 @@ func changeIssueReaction(ctx *context.APIContext, form api.EditReactionOption, i
386391
if err != nil {
387392
if models.IsErrForbiddenIssueReaction(err) {
388393
ctx.Error(http.StatusForbidden, err.Error(), err)
394+
} else if models.IsErrReactionAlreadyExist(err) {
395+
ctx.JSON(http.StatusOK, api.Reaction{
396+
User: ctx.User.APIFormat(),
397+
Reaction: reaction.Type,
398+
Created: reaction.CreatedUnix.AsTime(),
399+
})
389400
} else {
390401
ctx.Error(http.StatusInternalServerError, "CreateCommentReaction", err)
391402
}
392403
return
393404
}
394-
_, err = reaction.LoadUser()
395-
if err != nil {
396-
ctx.Error(http.StatusInternalServerError, "Reaction.LoadUser()", err)
397-
return
398-
}
399405

400-
ctx.JSON(http.StatusCreated, api.ReactionResponse{
401-
User: reaction.User.APIFormat(),
406+
ctx.JSON(http.StatusCreated, api.Reaction{
407+
User: ctx.User.APIFormat(),
402408
Reaction: reaction.Type,
403409
Created: reaction.CreatedUnix.AsTime(),
404410
})

‎routers/api/v1/swagger/issue.go

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -99,23 +99,16 @@ type swaggerResponseStopWatchList struct {
9999
Body []api.StopWatch `json:"body"`
100100
}
101101

102-
// EditReactionOption
103-
// swagger:response EditReactionOption
104-
type swaggerEditReactionOption struct {
102+
// Reaction
103+
// swagger:response Reaction
104+
type swaggerReaction struct {
105105
// in:body
106-
Body api.EditReactionOption `json:"body"`
106+
Body api.Reaction `json:"body"`
107107
}
108108

109-
// ReactionResponse
110-
// swagger:response ReactionResponse
111-
type swaggerReactionResponse struct {
109+
// ReactionList
110+
// swagger:response ReactionList
111+
type swaggerReactionList struct {
112112
// in:body
113-
Body api.ReactionResponse `json:"body"`
114-
}
115-
116-
// ReactionResponseList
117-
// swagger:response ReactionResponseList
118-
type swaggerReactionResponseList struct {
119-
// in:body
120-
Body []api.ReactionResponse `json:"body"`
113+
Body []api.Reaction `json:"body"`
121114
}

‎routers/api/v1/swagger/options.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,4 +123,7 @@ type swaggerParameterBodies struct {
123123

124124
// in:body
125125
RepoTopicOptions api.RepoTopicOptions
126+
127+
// in:body
128+
EditReactionOption api.EditReactionOption
126129
}

0 commit comments

Comments
 (0)