From 2ab8c78c30f6e29206b07fe15b0aec6c21df7005 Mon Sep 17 00:00:00 2001 From: 6543 <24977596+6543@users.noreply.github.com> Date: Wed, 20 Nov 2019 15:50:54 +0100 Subject: [PATCH] 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 --- models/fixtures/issue_watch.yml | 16 ++++++++ models/issue_watch.go | 2 + models/issue_watch_test.go | 23 +++++++---- routers/api/v1/api.go | 6 +-- routers/api/v1/repo/issue_subscription.go | 48 +++++------------------ templates/swagger/v1_json.tmpl | 4 +- 6 files changed, 47 insertions(+), 52 deletions(-) diff --git a/models/fixtures/issue_watch.yml b/models/fixtures/issue_watch.yml index 75351eb17f..4bc3ff1b8b 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: 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 diff --git a/models/issue_watch.go b/models/issue_watch.go index 3d7d48a125..e42e371a1f 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -56,6 +56,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 } @@ -80,6 +81,7 @@ func GetIssueWatchers(issueID int64) (IssueWatchList, error) { func getIssueWatchers(e Engine, issueID int64) (watches IssueWatchList, err error) { err = e. Where("`issue_watch`.issue_id = ?", issueID). + 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 ce0d2045ca..1d0473426e 100644 --- a/models/issue_watch_test.go +++ b/models/issue_watch_test.go @@ -15,26 +15,26 @@ 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) { 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, true, 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) } @@ -44,13 +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) - assert.Equal(t, 1, len(iws)) + // Watcher is explicit not watching + assert.Len(t, iws, 0) iws, err = GetIssueWatchers(5) 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) } diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index ec6ec191d7..47c9c95c7f 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -691,9 +691,9 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/stop", reqToken(), repo.StopIssueStopwatch) }) m.Group("/subscriptions", func() { - m.Get("", bind(api.User{}), repo.GetIssueSubscribers) - m.Put("/:user", repo.AddIssueSubscription) - m.Delete("/:user", repo.DelIssueSubscription) + m.Get("", repo.GetIssueSubscribers) + m.Put("/:user", reqToken(), repo.AddIssueSubscription) + m.Delete("/:user", reqToken(), repo.DelIssueSubscription) }) }) }, mustEnableIssuesOrPulls) diff --git a/routers/api/v1/repo/issue_subscription.go b/routers/api/v1/repo/issue_subscription.go index 012dcda44f..2c5f75f1ec 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 @@ -48,40 +47,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 +88,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 +120,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 } @@ -159,7 +129,7 @@ func DelIssueSubscription(ctx *context.APIContext) { } // 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. @@ -185,8 +155,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 4427c28747..338ab104e9 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -3832,8 +3832,8 @@ } ], "responses": { - "201": { - "$ref": "#/responses/empty" + "200": { + "$ref": "#/responses/UserList" }, "404": { "description": "Issue not found"