From 15005b1f671cea88f9a7638a069b7d109da998b1 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 29 Oct 2019 18:32:35 +0100 Subject: [PATCH 01/14] FIX: getIssueWatchers() get only aktive suscriber --- models/issue_watch.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/issue_watch.go b/models/issue_watch.go index 2f55c6a84db86..06f6d61800886 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -65,6 +65,7 @@ func GetIssueWatchers(issueID int64) ([]*IssueWatch, error) { func getIssueWatchers(e Engine, issueID int64) (watches []*IssueWatch, err error) { err = e. Where("`issue_watch`.issue_id = ?", issueID). + And("`issue_watch`.is_watching > ?", 0). And("`user`.is_active = ?", true). And("`user`.prohibit_login = ?", false). Join("INNER", "`user`", "`user`.id = `issue_watch`.user_id"). From 783df3c298e1f35f39a56592cc6c67c0a244e409 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 29 Oct 2019 21:21:33 +0100 Subject: [PATCH 02/14] save query to work later with it or not ... --- models/issue_watch.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/models/issue_watch.go b/models/issue_watch.go index 06f6d61800886..b2e7fe5f4d4bd 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -63,6 +63,7 @@ func GetIssueWatchers(issueID int64) ([]*IssueWatch, error) { } func getIssueWatchers(e Engine, issueID int64) (watches []*IssueWatch, err error) { + // handle manual watchers err = e. Where("`issue_watch`.issue_id = ?", issueID). And("`issue_watch`.is_watching > ?", 0). @@ -70,6 +71,11 @@ func getIssueWatchers(e Engine, issueID int64) (watches []*IssueWatch, err error And("`user`.prohibit_login = ?", false). Join("INNER", "`user`", "`user`.id = `issue_watch`.user_id"). Find(&watches) + // handle default watchers (repowatchers) + // SELECT x.user_id as repowatchers FROM (SELECT watch.user_id FROM issue,watch WHERE issue.repo_id = watch.repo_id AND issue.id = 71) as x LEFT JOIN (SELECT user_id FROM issue_watch WHERE is_watching = 0) as y ON x.user_id = y.user_id WHERE y.user_id IS NULL; + // returns list of userIDs + + //watches = append(watches) return } From 9f3b25c3589ac2408d064a3e7ec76cf14f620a46 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 1 Nov 2019 05:48:31 +0100 Subject: [PATCH 03/14] fix test + add new case --- models/fixtures/issue_watch.yml | 16 ++++++++++++++++ models/issue_watch_test.go | 6 ++++++ 2 files changed, 22 insertions(+) diff --git a/models/fixtures/issue_watch.yml b/models/fixtures/issue_watch.yml index 75351eb17f883..773e1cfb3d53e 100644 --- a/models/fixtures/issue_watch.yml +++ b/models/fixtures/issue_watch.yml @@ -13,3 +13,19 @@ is_watching: false created_unix: 946684800 updated_unix: 946684800 + +- + id: 3 + user_id: 2 + issue_id: 3 + is_watching: true + created_unix: 946684800 + updated_unix: 946684800 + +- + id: 3 + user_id: 1 + issue_id: 3 + is_watching: false + created_unix: 946684800 + updated_unix: 946684800 diff --git a/models/issue_watch_test.go b/models/issue_watch_test.go index ce0d2045ca688..52e989ef39cf1 100644 --- a/models/issue_watch_test.go +++ b/models/issue_watch_test.go @@ -48,6 +48,12 @@ func TestGetIssueWatchers(t *testing.T) { iws, err = GetIssueWatchers(2) assert.NoError(t, err) + // Watcher is not watching + assert.Equal(t, 0, len(iws)) + + iws, err = GetIssueWatchers(3) + assert.NoError(t, err) + // Watcher is not watching assert.Equal(t, 1, len(iws)) iws, err = GetIssueWatchers(5) From aa31558a75432a99ba306dbd0ef81ae9d7968e9c Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 1 Nov 2019 06:20:49 +0100 Subject: [PATCH 04/14] corect tests + GetIssueWatch --- models/fixtures/issue_watch.yml | 4 ++-- models/issue_watch.go | 3 ++- models/issue_watch_test.go | 12 ++++++------ 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/models/fixtures/issue_watch.yml b/models/fixtures/issue_watch.yml index 773e1cfb3d53e..bfaa409d82581 100644 --- a/models/fixtures/issue_watch.yml +++ b/models/fixtures/issue_watch.yml @@ -17,7 +17,7 @@ - id: 3 user_id: 2 - issue_id: 3 + issue_id: 7 is_watching: true created_unix: 946684800 updated_unix: 946684800 @@ -25,7 +25,7 @@ - id: 3 user_id: 1 - issue_id: 3 + issue_id: 7 is_watching: false created_unix: 946684800 updated_unix: 946684800 diff --git a/models/issue_watch.go b/models/issue_watch.go index b2e7fe5f4d4bd..ad04a7cfb05b3 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -53,6 +53,7 @@ func getIssueWatch(e Engine, userID, issueID int64) (iw *IssueWatch, exists bool exists, err = e. Where("user_id = ?", userID). And("issue_id = ?", issueID). + And("is_watching = ?", true). Get(iw) return } @@ -66,7 +67,7 @@ func getIssueWatchers(e Engine, issueID int64) (watches []*IssueWatch, err error // handle manual watchers err = e. Where("`issue_watch`.issue_id = ?", issueID). - And("`issue_watch`.is_watching > ?", 0). + And("`issue_watch`.is_watching = ?", true). And("`user`.is_active = ?", true). And("`user`.prohibit_login = ?", false). Join("INNER", "`user`", "`user`.id = `issue_watch`.user_id"). diff --git a/models/issue_watch_test.go b/models/issue_watch_test.go index 52e989ef39cf1..2b270f49dedf2 100644 --- a/models/issue_watch_test.go +++ b/models/issue_watch_test.go @@ -30,7 +30,7 @@ func TestGetIssueWatch(t *testing.T) { assert.NoError(t, err) _, exists, err = GetIssueWatch(2, 2) - assert.Equal(t, true, exists) + assert.Equal(t, false, exists) assert.NoError(t, err) _, exists, err = GetIssueWatch(3, 1) @@ -51,12 +51,12 @@ func TestGetIssueWatchers(t *testing.T) { // Watcher is not watching assert.Equal(t, 0, len(iws)) - iws, err = GetIssueWatchers(3) - assert.NoError(t, err) - // Watcher is not watching - assert.Equal(t, 1, len(iws)) - iws, err = GetIssueWatchers(5) assert.NoError(t, err) assert.Equal(t, 0, len(iws)) + + iws, err = GetIssueWatchers(7) + assert.NoError(t, err) + // Watcher is not watching + assert.Equal(t, 1, len(iws)) } From 52cc73b14bca1b85e31b846aaddbb80821c120e8 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 4 Nov 2019 18:08:32 +0100 Subject: [PATCH 05/14] API issue_subscripton: Put/Delete require tocken --- routers/api/v1/api.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index b68717f7c8751..2ff3768c9be33 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -692,8 +692,8 @@ func RegisterRoutes(m *macaron.Macaron) { }) m.Group("/subscriptions", func() { m.Get("", bind(api.User{}), repo.GetIssueSubscribers) - m.Put("/:user", repo.AddIssueSubscription) - m.Delete("/:user", repo.DelIssueSubscription) + m.Put("/:user", reqToken(), repo.AddIssueSubscription) + m.Delete("/:user", reqToken(), repo.DelIssueSubscription) }) }) }, mustEnableIssuesOrPulls) From e607e9fcdb69b94573a7468d75e8adbc2d1df936 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 4 Nov 2019 18:35:51 +0100 Subject: [PATCH 06/14] remove redundant code --- routers/api/v1/repo/issue_subscription.go | 41 ++++------------------- 1 file changed, 6 insertions(+), 35 deletions(-) diff --git a/routers/api/v1/repo/issue_subscription.go b/routers/api/v1/repo/issue_subscription.go index 012dcda44fa85..2e3de8c744ad2 100644 --- a/routers/api/v1/repo/issue_subscription.go +++ b/routers/api/v1/repo/issue_subscription.go @@ -48,40 +48,7 @@ func AddIssueSubscription(ctx *context.APIContext) { // description: User can only subscribe itself if he is no admin // "404": // description: Issue not found - issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) - 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) + setIssueSubscription(ctx, true) } // DelIssueSubscription Unsubscribe user from issue @@ -122,6 +89,10 @@ func DelIssueSubscription(ctx *context.APIContext) { // description: User can only subscribe itself if he is no admin // "404": // 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")) if err != nil { if models.IsErrIssueNotExist(err) { @@ -150,7 +121,7 @@ func DelIssueSubscription(ctx *context.APIContext) { 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) return } From 334472d42650895158827ddfeda6fb827a4c414f Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 5 Nov 2019 23:26:17 +0100 Subject: [PATCH 07/14] swagger specify return value --- routers/api/v1/repo/issue_subscription.go | 4 ++-- templates/swagger/v1_json.tmpl | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/routers/api/v1/repo/issue_subscription.go b/routers/api/v1/repo/issue_subscription.go index 2e3de8c744ad2..5982429bbace9 100644 --- a/routers/api/v1/repo/issue_subscription.go +++ b/routers/api/v1/repo/issue_subscription.go @@ -156,8 +156,8 @@ func GetIssueSubscribers(ctx *context.APIContext, form api.User) { // format: int64 // required: true // responses: - // "201": - // "$ref": "#/responses/empty" + // "200": + // "$ref": "#/responses/UserList" // "404": // description: Issue not found issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 41e5353ea73b0..c955441d3d62c 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -3819,8 +3819,8 @@ } ], "responses": { - "201": { - "$ref": "#/responses/empty" + "200": { + "$ref": "#/responses/UserList" }, "404": { "description": "Issue not found" From 318657f9900a9a00e09d035d9a77056ddc6d74db Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 5 Nov 2019 23:51:00 +0100 Subject: [PATCH 08/14] remove unused binding --- routers/api/v1/api.go | 2 +- routers/api/v1/repo/issue_subscription.go | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 2ff3768c9be33..bd0bfb57b6980 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -691,7 +691,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/stop", reqToken(), repo.StopIssueStopwatch) }) m.Group("/subscriptions", func() { - m.Get("", bind(api.User{}), repo.GetIssueSubscribers) + m.Get("", repo.GetIssueSubscribers) m.Put("/:user", reqToken(), repo.AddIssueSubscription) m.Delete("/:user", reqToken(), repo.DelIssueSubscription) }) diff --git a/routers/api/v1/repo/issue_subscription.go b/routers/api/v1/repo/issue_subscription.go index 5982429bbace9..2c5f75f1eca07 100644 --- a/routers/api/v1/repo/issue_subscription.go +++ b/routers/api/v1/repo/issue_subscription.go @@ -7,7 +7,6 @@ package repo import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/context" - api "code.gitea.io/gitea/modules/structs" ) // AddIssueSubscription Subscribe user to issue @@ -130,7 +129,7 @@ func setIssueSubscription(ctx *context.APIContext, watch bool) { } // 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 // --- // summary: Get users who subscribed on an issue. From a06fdb4755d74f92b5d8c6ad9b8ee4bd5ae20642 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Wed, 6 Nov 2019 05:40:45 +0100 Subject: [PATCH 09/14] remove note because I'll implement this in a different way and in another PR --- models/issue_watch.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/models/issue_watch.go b/models/issue_watch.go index 4133626de6dd7..e131483a8c235 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -74,11 +74,6 @@ func getIssueWatchers(e Engine, issueID int64) (watches IssueWatchList, err erro And("`user`.prohibit_login = ?", false). Join("INNER", "`user`", "`user`.id = `issue_watch`.user_id"). Find(&watches) - // handle default watchers (repowatchers) - // SELECT x.user_id as repowatchers FROM (SELECT watch.user_id FROM issue,watch WHERE issue.repo_id = watch.repo_id AND issue.id = 71) as x LEFT JOIN (SELECT user_id FROM issue_watch WHERE is_watching = 0) as y ON x.user_id = y.user_id WHERE y.user_id IS NULL; - // returns list of userIDs - - //watches = append(watches) return } From 7e97d575f2e741b582ef14121221b2c214553819 Mon Sep 17 00:00:00 2001 From: 6543 <24977596+6543@users.noreply.github.com> Date: Fri, 15 Nov 2019 13:35:56 +0100 Subject: [PATCH 10/14] ID should be unique! --- models/fixtures/issue_watch.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/fixtures/issue_watch.yml b/models/fixtures/issue_watch.yml index bfaa409d82581..4bc3ff1b8b987 100644 --- a/models/fixtures/issue_watch.yml +++ b/models/fixtures/issue_watch.yml @@ -23,7 +23,7 @@ updated_unix: 946684800 - - id: 3 + id: 4 user_id: 1 issue_id: 7 is_watching: false From c1de540147199f2f1a8dd0d008f54af3603e2229 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 15 Nov 2019 14:01:23 +0100 Subject: [PATCH 11/14] use xorm session --- models/issue_watch.go | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/models/issue_watch.go b/models/issue_watch.go index e131483a8c235..00822f55c765e 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -21,7 +21,21 @@ type IssueWatchList []*IssueWatch // CreateOrUpdateIssueWatch set watching for a user and issue func CreateOrUpdateIssueWatch(userID, issueID int64, isWatching bool) error { - iw, exists, err := getIssueWatch(x, userID, issueID) + sess := x.NewSession() + defer sess.Close() + + if err := sess.Begin(); err != nil { + return err + } + if err := createOrUpdateIssueWatch(sess, userID, issueID, isWatching); err != nil { + return err + } + + return sess.Commit() +} + +func createOrUpdateIssueWatch(e Engine, userID, issueID int64, isWatching bool) error { + iw, exists, err := getIssueWatch(e, userID, issueID) if err != nil { return err } @@ -33,13 +47,13 @@ func CreateOrUpdateIssueWatch(userID, issueID int64, isWatching bool) error { IsWatching: isWatching, } - if _, err := x.Insert(iw); err != nil { + if _, err := e.Insert(iw); err != nil { return err } } else { iw.IsWatching = isWatching - if _, err := x.ID(iw.ID).Cols("is_watching", "updated_unix").Update(iw); err != nil { + if _, err := e.ID(iw.ID).Cols("is_watching", "updated_unix").Update(iw); err != nil { return err } } From 4facdc9bcca094e3c66799bfa83bbfd119f1c9ce Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 15 Nov 2019 14:33:13 +0100 Subject: [PATCH 12/14] Revert "use xorm session" This reverts commit c1de540147199f2f1a8dd0d008f54af3603e2229. --- models/issue_watch.go | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/models/issue_watch.go b/models/issue_watch.go index 00822f55c765e..e131483a8c235 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -21,21 +21,7 @@ type IssueWatchList []*IssueWatch // CreateOrUpdateIssueWatch set watching for a user and issue func CreateOrUpdateIssueWatch(userID, issueID int64, isWatching bool) error { - sess := x.NewSession() - defer sess.Close() - - if err := sess.Begin(); err != nil { - return err - } - if err := createOrUpdateIssueWatch(sess, userID, issueID, isWatching); err != nil { - return err - } - - return sess.Commit() -} - -func createOrUpdateIssueWatch(e Engine, userID, issueID int64, isWatching bool) error { - iw, exists, err := getIssueWatch(e, userID, issueID) + iw, exists, err := getIssueWatch(x, userID, issueID) if err != nil { return err } @@ -47,13 +33,13 @@ func createOrUpdateIssueWatch(e Engine, userID, issueID int64, isWatching bool) IsWatching: isWatching, } - if _, err := e.Insert(iw); err != nil { + if _, err := x.Insert(iw); err != nil { return err } } else { iw.IsWatching = isWatching - if _, err := e.ID(iw.ID).Cols("is_watching", "updated_unix").Update(iw); err != nil { + if _, err := x.ID(iw.ID).Cols("is_watching", "updated_unix").Update(iw); err != nil { return err } } From 0317c54618666d844aec3967993443ea798ccffe Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 15 Nov 2019 14:39:26 +0100 Subject: [PATCH 13/14] better test code * more acurate comments * use assert.False/True instead of Equal --- models/issue_watch_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/models/issue_watch_test.go b/models/issue_watch_test.go index 2b270f49dedf2..f4ed20abb89d5 100644 --- a/models/issue_watch_test.go +++ b/models/issue_watch_test.go @@ -26,15 +26,15 @@ func TestGetIssueWatch(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) _, exists, err := GetIssueWatch(9, 1) - assert.Equal(t, true, exists) + assert.True(t, exists) assert.NoError(t, err) _, exists, err = GetIssueWatch(2, 2) - assert.Equal(t, false, exists) + assert.False(t, exists) assert.NoError(t, err) _, exists, err = GetIssueWatch(3, 1) - assert.Equal(t, false, exists) + assert.False(t, exists) assert.NoError(t, err) } @@ -48,15 +48,16 @@ func TestGetIssueWatchers(t *testing.T) { iws, err = GetIssueWatchers(2) assert.NoError(t, err) - // Watcher is not watching + // Watcher is explicit not watching assert.Equal(t, 0, len(iws)) iws, err = GetIssueWatchers(5) assert.NoError(t, err) + // Issue has no Watchers assert.Equal(t, 0, len(iws)) iws, err = GetIssueWatchers(7) assert.NoError(t, err) - // Watcher is not watching + // Issue has one watcher assert.Equal(t, 1, len(iws)) } From 0ac149aeb9e963f436e7f7e66b6d5f9aa9f79bbb Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 15 Nov 2019 23:00:35 +0100 Subject: [PATCH 14/14] use more assert methodes --- models/issue_watch_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/models/issue_watch_test.go b/models/issue_watch_test.go index f4ed20abb89d5..1d0473426e8df 100644 --- a/models/issue_watch_test.go +++ b/models/issue_watch_test.go @@ -15,11 +15,11 @@ func TestCreateOrUpdateIssueWatch(t *testing.T) { assert.NoError(t, CreateOrUpdateIssueWatch(3, 1, true)) 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)) 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) { @@ -44,20 +44,20 @@ func TestGetIssueWatchers(t *testing.T) { iws, err := GetIssueWatchers(1) assert.NoError(t, err) // Watcher is inactive, thus 0 - assert.Equal(t, 0, len(iws)) + assert.Len(t, iws, 0) iws, err = GetIssueWatchers(2) assert.NoError(t, err) // Watcher is explicit not watching - assert.Equal(t, 0, len(iws)) + assert.Len(t, iws, 0) iws, err = GetIssueWatchers(5) assert.NoError(t, err) // Issue has no Watchers - assert.Equal(t, 0, len(iws)) + assert.Len(t, iws, 0) iws, err = GetIssueWatchers(7) assert.NoError(t, err) // Issue has one watcher - assert.Equal(t, 1, len(iws)) + assert.Len(t, iws, 1) }