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

diff --git a/models/fixtures/issue_watch.yml b/models/fixtures/issue_watch.yml
index 75351eb17f883d3cb12371dc08dbad4638e0baf0..4bc3ff1b8b98723e0b82c081f0a5a61b09db2c09 100644 (file)
--- 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 3d7d48a1252403d1425bd3b07c5b8927e31efa7f..e42e371a1fae5905bad25572675dba59ec7efc4e 100644 (file)
--- 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 ce0d2045ca6883275f7ad546b8ebb798fdbbd52b..1d0473426e8df057391f2b71a11b09937fc80372 100644 (file)
--- 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 ec6ec191d74eb806df75ecf35780380bb1d7b939..47c9c95c7fe00cd6b40c63f9d56dcd8afcd2ae41 100644 (file)
--- 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 012dcda44fa8546f3cdb28dd8a7ead2ba6b4f6be..2c5f75f1eca07ff6325d988b645b2b57b78af9ef 100644 (file)
--- 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 4427c28747366758ec02deda5c224920bf71da16..338ab104e9e94be8273b026a41f4963461c52ed0 100644 (file)
--- 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"