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 c1de540147199f2f1a8dd0d008f54af3603e2229.

* better test code
* more acurate comments
* use assert.False/True instead of Equal

* use more assert methodes
release/v1.15
6543 2019-11-20 15:50:54 +01:00 committed by Lunny Xiao
parent dfd8b94923
commit 2ab8c78c30
6 changed files with 47 additions and 52 deletions

View File

@ -13,3 +13,19 @@
is_watching: false is_watching: false
created_unix: 946684800 created_unix: 946684800
updated_unix: 946684800 updated_unix: 946684800
-
id: 3
user_id: 2
issue_id: 7
is_watching: true
created_unix: 946684800
updated_unix: 946684800
-
id: 4
user_id: 1
issue_id: 7
is_watching: false
created_unix: 946684800
updated_unix: 946684800

View File

@ -56,6 +56,7 @@ func getIssueWatch(e Engine, userID, issueID int64) (iw *IssueWatch, exists bool
exists, err = e. exists, err = e.
Where("user_id = ?", userID). Where("user_id = ?", userID).
And("issue_id = ?", issueID). And("issue_id = ?", issueID).
And("is_watching = ?", true).
Get(iw) Get(iw)
return return
} }
@ -80,6 +81,7 @@ func GetIssueWatchers(issueID int64) (IssueWatchList, error) {
func getIssueWatchers(e Engine, issueID int64) (watches IssueWatchList, err error) { func getIssueWatchers(e Engine, issueID int64) (watches IssueWatchList, err error) {
err = e. err = e.
Where("`issue_watch`.issue_id = ?", issueID). Where("`issue_watch`.issue_id = ?", issueID).
And("`issue_watch`.is_watching = ?", true).
And("`user`.is_active = ?", true). And("`user`.is_active = ?", true).
And("`user`.prohibit_login = ?", false). And("`user`.prohibit_login = ?", false).
Join("INNER", "`user`", "`user`.id = `issue_watch`.user_id"). Join("INNER", "`user`", "`user`.id = `issue_watch`.user_id").

View File

@ -15,26 +15,26 @@ func TestCreateOrUpdateIssueWatch(t *testing.T) {
assert.NoError(t, CreateOrUpdateIssueWatch(3, 1, true)) assert.NoError(t, CreateOrUpdateIssueWatch(3, 1, true))
iw := AssertExistsAndLoadBean(t, &IssueWatch{UserID: 3, IssueID: 1}).(*IssueWatch) iw := AssertExistsAndLoadBean(t, &IssueWatch{UserID: 3, IssueID: 1}).(*IssueWatch)
assert.Equal(t, true, iw.IsWatching) assert.True(t, iw.IsWatching)
assert.NoError(t, CreateOrUpdateIssueWatch(1, 1, false)) assert.NoError(t, CreateOrUpdateIssueWatch(1, 1, false))
iw = AssertExistsAndLoadBean(t, &IssueWatch{UserID: 1, IssueID: 1}).(*IssueWatch) iw = AssertExistsAndLoadBean(t, &IssueWatch{UserID: 1, IssueID: 1}).(*IssueWatch)
assert.Equal(t, false, iw.IsWatching) assert.False(t, iw.IsWatching)
} }
func TestGetIssueWatch(t *testing.T) { func TestGetIssueWatch(t *testing.T) {
assert.NoError(t, PrepareTestDatabase()) assert.NoError(t, PrepareTestDatabase())
_, exists, err := GetIssueWatch(9, 1) _, exists, err := GetIssueWatch(9, 1)
assert.Equal(t, true, exists) assert.True(t, exists)
assert.NoError(t, err) assert.NoError(t, err)
_, exists, err = GetIssueWatch(2, 2) _, exists, err = GetIssueWatch(2, 2)
assert.Equal(t, true, exists) assert.False(t, exists)
assert.NoError(t, err) assert.NoError(t, err)
_, exists, err = GetIssueWatch(3, 1) _, exists, err = GetIssueWatch(3, 1)
assert.Equal(t, false, exists) assert.False(t, exists)
assert.NoError(t, err) assert.NoError(t, err)
} }
@ -44,13 +44,20 @@ func TestGetIssueWatchers(t *testing.T) {
iws, err := GetIssueWatchers(1) iws, err := GetIssueWatchers(1)
assert.NoError(t, err) assert.NoError(t, err)
// Watcher is inactive, thus 0 // Watcher is inactive, thus 0
assert.Equal(t, 0, len(iws)) assert.Len(t, iws, 0)
iws, err = GetIssueWatchers(2) iws, err = GetIssueWatchers(2)
assert.NoError(t, err) assert.NoError(t, err)
assert.Equal(t, 1, len(iws)) // Watcher is explicit not watching
assert.Len(t, iws, 0)
iws, err = GetIssueWatchers(5) iws, err = GetIssueWatchers(5)
assert.NoError(t, err) assert.NoError(t, err)
assert.Equal(t, 0, len(iws)) // Issue has no Watchers
assert.Len(t, iws, 0)
iws, err = GetIssueWatchers(7)
assert.NoError(t, err)
// Issue has one watcher
assert.Len(t, iws, 1)
} }

View File

@ -691,9 +691,9 @@ func RegisterRoutes(m *macaron.Macaron) {
m.Post("/stop", reqToken(), repo.StopIssueStopwatch) m.Post("/stop", reqToken(), repo.StopIssueStopwatch)
}) })
m.Group("/subscriptions", func() { m.Group("/subscriptions", func() {
m.Get("", bind(api.User{}), repo.GetIssueSubscribers) m.Get("", repo.GetIssueSubscribers)
m.Put("/:user", repo.AddIssueSubscription) m.Put("/:user", reqToken(), repo.AddIssueSubscription)
m.Delete("/:user", repo.DelIssueSubscription) m.Delete("/:user", reqToken(), repo.DelIssueSubscription)
}) })
}) })
}, mustEnableIssuesOrPulls) }, mustEnableIssuesOrPulls)

View File

@ -7,7 +7,6 @@ package repo
import ( import (
"code.gitea.io/gitea/models" "code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/context"
api "code.gitea.io/gitea/modules/structs"
) )
// AddIssueSubscription Subscribe user to issue // AddIssueSubscription Subscribe user to issue
@ -48,40 +47,7 @@ func AddIssueSubscription(ctx *context.APIContext) {
// description: User can only subscribe itself if he is no admin // description: User can only subscribe itself if he is no admin
// "404": // "404":
// description: Issue not found // description: Issue not found
issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) setIssueSubscription(ctx, true)
if err != nil {
if models.IsErrIssueNotExist(err) {
ctx.NotFound()
} else {
ctx.Error(500, "GetIssueByIndex", err)
}
return
}
user, err := models.GetUserByName(ctx.Params(":user"))
if err != nil {
if models.IsErrUserNotExist(err) {
ctx.NotFound()
} else {
ctx.Error(500, "GetUserByName", err)
}
return
}
//only admin and user for itself can change subscription
if user.ID != ctx.User.ID && !ctx.User.IsAdmin {
ctx.Error(403, "User", nil)
return
}
if err := models.CreateOrUpdateIssueWatch(user.ID, issue.ID, true); err != nil {
ctx.Error(500, "CreateOrUpdateIssueWatch", err)
return
}
ctx.Status(201)
} }
// DelIssueSubscription Unsubscribe user from issue // DelIssueSubscription Unsubscribe user from issue
@ -122,6 +88,10 @@ func DelIssueSubscription(ctx *context.APIContext) {
// description: User can only subscribe itself if he is no admin // description: User can only subscribe itself if he is no admin
// "404": // "404":
// description: Issue not found // description: Issue not found
setIssueSubscription(ctx, false)
}
func setIssueSubscription(ctx *context.APIContext, watch bool) {
issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))
if err != nil { if err != nil {
if models.IsErrIssueNotExist(err) { if models.IsErrIssueNotExist(err) {
@ -150,7 +120,7 @@ func DelIssueSubscription(ctx *context.APIContext) {
return return
} }
if err := models.CreateOrUpdateIssueWatch(user.ID, issue.ID, false); err != nil { if err := models.CreateOrUpdateIssueWatch(user.ID, issue.ID, watch); err != nil {
ctx.Error(500, "CreateOrUpdateIssueWatch", err) ctx.Error(500, "CreateOrUpdateIssueWatch", err)
return return
} }
@ -159,7 +129,7 @@ func DelIssueSubscription(ctx *context.APIContext) {
} }
// GetIssueSubscribers return subscribers of an issue // GetIssueSubscribers return subscribers of an issue
func GetIssueSubscribers(ctx *context.APIContext, form api.User) { func GetIssueSubscribers(ctx *context.APIContext) {
// swagger:operation GET /repos/{owner}/{repo}/issues/{index}/subscriptions issue issueSubscriptions // swagger:operation GET /repos/{owner}/{repo}/issues/{index}/subscriptions issue issueSubscriptions
// --- // ---
// summary: Get users who subscribed on an issue. // summary: Get users who subscribed on an issue.
@ -185,8 +155,8 @@ func GetIssueSubscribers(ctx *context.APIContext, form api.User) {
// format: int64 // format: int64
// required: true // required: true
// responses: // responses:
// "201": // "200":
// "$ref": "#/responses/empty" // "$ref": "#/responses/UserList"
// "404": // "404":
// description: Issue not found // description: Issue not found
issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index"))

View File

@ -3832,8 +3832,8 @@
} }
], ],
"responses": { "responses": {
"201": { "200": {
"$ref": "#/responses/empty" "$ref": "#/responses/UserList"
}, },
"404": { "404": {
"description": "Issue not found" "description": "Issue not found"