Skip to content

Commit 2ab8c78

Browse files
6543lunny
6543
authored and
lunny
committed
Refactor Issues Subscription (#8738)
* FIX: getIssueWatchers() get only aktive suscriber * save query to work later with it or not ... * fix test + add new case * corect tests + GetIssueWatch * API issue_subscripton: Put/Delete require tocken * remove redundant code * swagger specify return value * remove unused binding * remove note because I'll implement this in a different way and in another PR * ID should be unique! * use xorm session * Revert "use xorm session" This reverts commit c1de540. * better test code * more acurate comments * use assert.False/True instead of Equal * use more assert methodes
1 parent dfd8b94 commit 2ab8c78

File tree

6 files changed

+47
-52
lines changed

6 files changed

+47
-52
lines changed

‎models/fixtures/issue_watch.yml

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,19 @@
1313
is_watching: false
1414
created_unix: 946684800
1515
updated_unix: 946684800
16+
17+
-
18+
id: 3
19+
user_id: 2
20+
issue_id: 7
21+
is_watching: true
22+
created_unix: 946684800
23+
updated_unix: 946684800
24+
25+
-
26+
id: 4
27+
user_id: 1
28+
issue_id: 7
29+
is_watching: false
30+
created_unix: 946684800
31+
updated_unix: 946684800

‎models/issue_watch.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ func getIssueWatch(e Engine, userID, issueID int64) (iw *IssueWatch, exists bool
5656
exists, err = e.
5757
Where("user_id = ?", userID).
5858
And("issue_id = ?", issueID).
59+
And("is_watching = ?", true).
5960
Get(iw)
6061
return
6162
}
@@ -80,6 +81,7 @@ func GetIssueWatchers(issueID int64) (IssueWatchList, error) {
8081
func getIssueWatchers(e Engine, issueID int64) (watches IssueWatchList, err error) {
8182
err = e.
8283
Where("`issue_watch`.issue_id = ?", issueID).
84+
And("`issue_watch`.is_watching = ?", true).
8385
And("`user`.is_active = ?", true).
8486
And("`user`.prohibit_login = ?", false).
8587
Join("INNER", "`user`", "`user`.id = `issue_watch`.user_id").

‎models/issue_watch_test.go

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,26 +15,26 @@ func TestCreateOrUpdateIssueWatch(t *testing.T) {
1515

1616
assert.NoError(t, CreateOrUpdateIssueWatch(3, 1, true))
1717
iw := AssertExistsAndLoadBean(t, &IssueWatch{UserID: 3, IssueID: 1}).(*IssueWatch)
18-
assert.Equal(t, true, iw.IsWatching)
18+
assert.True(t, iw.IsWatching)
1919

2020
assert.NoError(t, CreateOrUpdateIssueWatch(1, 1, false))
2121
iw = AssertExistsAndLoadBean(t, &IssueWatch{UserID: 1, IssueID: 1}).(*IssueWatch)
22-
assert.Equal(t, false, iw.IsWatching)
22+
assert.False(t, iw.IsWatching)
2323
}
2424

2525
func TestGetIssueWatch(t *testing.T) {
2626
assert.NoError(t, PrepareTestDatabase())
2727

2828
_, exists, err := GetIssueWatch(9, 1)
29-
assert.Equal(t, true, exists)
29+
assert.True(t, exists)
3030
assert.NoError(t, err)
3131

3232
_, exists, err = GetIssueWatch(2, 2)
33-
assert.Equal(t, true, exists)
33+
assert.False(t, exists)
3434
assert.NoError(t, err)
3535

3636
_, exists, err = GetIssueWatch(3, 1)
37-
assert.Equal(t, false, exists)
37+
assert.False(t, exists)
3838
assert.NoError(t, err)
3939
}
4040

@@ -44,13 +44,20 @@ func TestGetIssueWatchers(t *testing.T) {
4444
iws, err := GetIssueWatchers(1)
4545
assert.NoError(t, err)
4646
// Watcher is inactive, thus 0
47-
assert.Equal(t, 0, len(iws))
47+
assert.Len(t, iws, 0)
4848

4949
iws, err = GetIssueWatchers(2)
5050
assert.NoError(t, err)
51-
assert.Equal(t, 1, len(iws))
51+
// Watcher is explicit not watching
52+
assert.Len(t, iws, 0)
5253

5354
iws, err = GetIssueWatchers(5)
5455
assert.NoError(t, err)
55-
assert.Equal(t, 0, len(iws))
56+
// Issue has no Watchers
57+
assert.Len(t, iws, 0)
58+
59+
iws, err = GetIssueWatchers(7)
60+
assert.NoError(t, err)
61+
// Issue has one watcher
62+
assert.Len(t, iws, 1)
5663
}

‎routers/api/v1/api.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -691,9 +691,9 @@ func RegisterRoutes(m *macaron.Macaron) {
691691
m.Post("/stop", reqToken(), repo.StopIssueStopwatch)
692692
})
693693
m.Group("/subscriptions", func() {
694-
m.Get("", bind(api.User{}), repo.GetIssueSubscribers)
695-
m.Put("/:user", repo.AddIssueSubscription)
696-
m.Delete("/:user", repo.DelIssueSubscription)
694+
m.Get("", repo.GetIssueSubscribers)
695+
m.Put("/:user", reqToken(), repo.AddIssueSubscription)
696+
m.Delete("/:user", reqToken(), repo.DelIssueSubscription)
697697
})
698698
})
699699
}, mustEnableIssuesOrPulls)

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

Lines changed: 9 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ package repo
77
import (
88
"code.gitea.io/gitea/models"
99
"code.gitea.io/gitea/modules/context"
10-
api "code.gitea.io/gitea/modules/structs"
1110
)
1211

1312
// AddIssueSubscription Subscribe user to issue
@@ -48,40 +47,7 @@ func AddIssueSubscription(ctx *context.APIContext) {
4847
// description: User can only subscribe itself if he is no admin
4948
// "404":
5049
// description: Issue not found
51-
issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
52-
if err != nil {
53-
if models.IsErrIssueNotExist(err) {
54-
ctx.NotFound()
55-
} else {
56-
ctx.Error(500, "GetIssueByIndex", err)
57-
}
58-
59-
return
60-
}
61-
62-
user, err := models.GetUserByName(ctx.Params(":user"))
63-
if err != nil {
64-
if models.IsErrUserNotExist(err) {
65-
ctx.NotFound()
66-
} else {
67-
ctx.Error(500, "GetUserByName", err)
68-
}
69-
70-
return
71-
}
72-
73-
//only admin and user for itself can change subscription
74-
if user.ID != ctx.User.ID && !ctx.User.IsAdmin {
75-
ctx.Error(403, "User", nil)
76-
return
77-
}
78-
79-
if err := models.CreateOrUpdateIssueWatch(user.ID, issue.ID, true); err != nil {
80-
ctx.Error(500, "CreateOrUpdateIssueWatch", err)
81-
return
82-
}
83-
84-
ctx.Status(201)
50+
setIssueSubscription(ctx, true)
8551
}
8652

8753
// DelIssueSubscription Unsubscribe user from issue
@@ -122,6 +88,10 @@ func DelIssueSubscription(ctx *context.APIContext) {
12288
// description: User can only subscribe itself if he is no admin
12389
// "404":
12490
// description: Issue not found
91+
setIssueSubscription(ctx, false)
92+
}
93+
94+
func setIssueSubscription(ctx *context.APIContext, watch bool) {
12595
issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
12696
if err != nil {
12797
if models.IsErrIssueNotExist(err) {
@@ -150,7 +120,7 @@ func DelIssueSubscription(ctx *context.APIContext) {
150120
return
151121
}
152122

153-
if err := models.CreateOrUpdateIssueWatch(user.ID, issue.ID, false); err != nil {
123+
if err := models.CreateOrUpdateIssueWatch(user.ID, issue.ID, watch); err != nil {
154124
ctx.Error(500, "CreateOrUpdateIssueWatch", err)
155125
return
156126
}
@@ -159,7 +129,7 @@ func DelIssueSubscription(ctx *context.APIContext) {
159129
}
160130

161131
// GetIssueSubscribers return subscribers of an issue
162-
func GetIssueSubscribers(ctx *context.APIContext, form api.User) {
132+
func GetIssueSubscribers(ctx *context.APIContext) {
163133
// swagger:operation GET /repos/{owner}/{repo}/issues/{index}/subscriptions issue issueSubscriptions
164134
// ---
165135
// summary: Get users who subscribed on an issue.
@@ -185,8 +155,8 @@ func GetIssueSubscribers(ctx *context.APIContext, form api.User) {
185155
// format: int64
186156
// required: true
187157
// responses:
188-
// "201":
189-
// "$ref": "#/responses/empty"
158+
// "200":
159+
// "$ref": "#/responses/UserList"
190160
// "404":
191161
// description: Issue not found
192162
issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))

‎templates/swagger/v1_json.tmpl

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3832,8 +3832,8 @@
38323832
}
38333833
],
38343834
"responses": {
3835-
"201": {
3836-
"$ref": "#/responses/empty"
3835+
"200": {
3836+
"$ref": "#/responses/UserList"
38373837
},
38383838
"404": {
38393839
"description": "Issue not found"

0 commit comments

Comments
 (0)