Fix the missing users in the subscriptions endpoint.

models/fixtures/comment.yml

@@ -9,15 +9,15 @@
 -
   id: 2
   type: 0 # comment
-  poster_id: 3 # user not watching (see watch.yml)
+  poster_id: 3 # the user not explicitly not watching (see issue_watch.yml)
   issue_id: 1 # in repo_id 1
   content: "good work!"
   created_unix: 946684811
   updated_unix: 946684811
 -
   id: 3
   type: 0 # comment
-  poster_id: 5 # user not watching (see watch.yml)
+  poster_id: 5 # the user not explicitly not watching (see issue_watch.yml)
   issue_id: 1 # in repo_id 1
   content: "meh..."
   created_unix: 946684812

models/issues/issue_subscriber.go

@@ -0,0 +1,103 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package issues
+
+import (
+	"context"
+
+	"code.gitea.io/gitea/models/db"
+	repo_model "code.gitea.io/gitea/models/repo"
+	user_model "code.gitea.io/gitea/models/user"
+
+	"xorm.io/builder"
+)
+
+// CheckIssueSubscriber check if a user is subscribing an issue
+// it takes participants and repo watch into account
+func CheckIssueSubscriber(user *user_model.User, issue *Issue) (bool, error) {
+	iw, exist, err := GetIssueWatch(db.DefaultContext, user.ID, issue.ID)
+	if err != nil {
+		return false, err
+	}
+	if exist {
+		return iw.IsWatching, nil
+	}
+	w, err := repo_model.GetWatch(db.DefaultContext, user.ID, issue.RepoID)
+	if err != nil {
+		return false, err
+	}
+	return repo_model.IsWatchMode(w.Mode) || IsUserParticipantsOfIssue(user, issue), nil
+}
+
+// GetIssueSubscribers returns subscribers of a given issue
+func GetIssueSubscribers(ctx context.Context, issue *Issue, listOptions db.ListOptions) (user_model.UserList, error) {
+	subscribeUserIds := builder.Select("`issue_watch`.user_id").
+		From("issue_watch").
+		Where(builder.Eq{"`issue_watch`.issue_id": issue.ID, "`issue_watch`.is_watching": true})
+
+	unsubscribeUserIds := builder.Select("`issue_watch`.user_id").
+		From("issue_watch").
+		Where(builder.Eq{"`issue_watch`.issue_id": issue.ID, "`issue_watch`.is_watching": false})
+
+	participantsUserIds := builder.Select("`comment`.poster_id").
+		From("comment").
+		Where(builder.Eq{"`comment`.issue_id": issue.ID}).
+		And(builder.In("`comment`.type", CommentTypeComment, CommentTypeCode, CommentTypeReview))
+
+	repoSubscribeUserIds := builder.Select("`watch`.user_id").
+		From("watch").
+		Where(builder.Eq{"`watch`.repo_id": issue.RepoID}).
+		And(builder.NotIn("`watch`.mode", repo_model.WatchModeDont, repo_model.WatchModeNone))
+
+	sess := db.GetEngine(ctx).Table("user").
+		Where(builder.Or(
+			builder.In("id", subscribeUserIds),
+			builder.In("id", repoSubscribeUserIds),
+			builder.In("id", participantsUserIds),
+			builder.In("id", issue.PosterID),
+		).
+			And(builder.NotIn("id", unsubscribeUserIds)).
+			And(builder.Eq{"`user`.is_active": true, "`user`.prohibit_login": false}))
+
+	if listOptions.Page != 0 {
+		sess = db.SetSessionPagination(sess, &listOptions)
+		users := make(user_model.UserList, 0, listOptions.PageSize)
+		return users, sess.Find(&users)
+	}
+	users := make(user_model.UserList, 0, 8)
+	return users, sess.Find(&users)
+}
+
+// CountIssueSubscribers count subscribers of a given issue
+func CountIssueSubscribers(ctx context.Context, issue *Issue) (int64, error) {
+	subscribeUserIds := builder.Select("`issue_watch`.user_id").
+		From("issue_watch").
+		Where(builder.Eq{"`issue_watch`.issue_id": issue.ID, "`issue_watch`.is_watching": true})
+
+	unsubscribeUserIds := builder.Select("`issue_watch`.user_id").
+		From("issue_watch").
+		Where(builder.Eq{"`issue_watch`.issue_id": issue.ID, "`issue_watch`.is_watching": false})
+
+	participantsUserIds := builder.Select("`comment`.poster_id").
+		From("comment").
+		Where(builder.Eq{"`comment`.issue_id": issue.ID}).
+		And(builder.In("`comment`.type", CommentTypeComment, CommentTypeCode, CommentTypeReview))
+
+	repoSubscribeUserIds := builder.Select("`watch`.user_id").
+		From("watch").
+		Where(builder.Eq{"`watch`.repo_id": issue.RepoID}).
+		And(builder.NotIn("`watch`.mode", repo_model.WatchModeDont, repo_model.WatchModeNone))
+
+	sess := db.GetEngine(ctx).Table("user").
+		Where(builder.Or(
+			builder.In("id", subscribeUserIds),
+			builder.In("id", repoSubscribeUserIds),
+			builder.In("id", participantsUserIds),
+			builder.In("id", issue.PosterID),
+		).
+			And(builder.NotIn("id", unsubscribeUserIds)).
+			And(builder.Eq{"`user`.is_active": true, "`user`.prohibit_login": false}))
+
+	return sess.Count(new(user_model.User))
+}

models/issues/issue_subscriber_test.go

@@ -0,0 +1,69 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package issues_test
+
+import (
+	"testing"
+
+	"code.gitea.io/gitea/models/db"
+	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/models/unittest"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGetIssueWatchers(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+
+	issueList := issues_model.IssueList{
+		unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1}), // repo 1
+		unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}), // repo 1
+		unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 5}), // repo 1
+		unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 7}), // repo 2
+	}
+
+	iws, err := issues_model.GetIssueSubscribers(db.DefaultContext, issueList[0], db.ListOptions{})
+	assert.NoError(t, err)
+	// +isIssueWatching[]
+	// -noIssueWatching[]
+	// +participates[2,3,5]
+	// +poster[1]
+	// +repoWatch[1,4,9,11]
+	// -inactive[3,9]
+	// => [1,4,5,11]
+	assert.Len(t, iws, 4)
+
+	iws, err = issues_model.GetIssueSubscribers(db.DefaultContext, issueList[1], db.ListOptions{})
+	assert.NoError(t, err)
+	// +isIssueWatching[]
+	// -noIssueWatching[2]
+	// +participates[1]
+	// +poster[1]
+	// +repoWatch[1,4,9,11]
+	// -inactive[3,9]
+	// => [1,4,11]
+	assert.Len(t, iws, 3)
+
+	iws, err = issues_model.GetIssueSubscribers(db.DefaultContext, issueList[2], db.ListOptions{})
+	assert.NoError(t, err)
+	// +isIssueWatching[]
+	// -noIssueWatching[]
+	// +participates[]
+	// +poster[2]
+	// +repoWatch[1,4,9,11]
+	// -inactive[3,9]
+	// => [1,2,4,11]
+	assert.Len(t, iws, 4)
+
+	iws, err = issues_model.GetIssueSubscribers(db.DefaultContext, issueList[3], db.ListOptions{})
+	assert.NoError(t, err)
+	// +isIssueWatching[]
+	// -noIssueWatching[]
+	// +participates[]
+	// +poster[2]
+	// +repoWatch[]
+	// -inactive[3,9]
+	// => [2]
+	assert.Len(t, iws, 1)
+}

models/issues/issue_watch.go

@@ -7,16 +7,14 @@ import (
 	"context"
 
 	"code.gitea.io/gitea/models/db"
-	repo_model "code.gitea.io/gitea/models/repo"
-	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/timeutil"
 )
 
 // IssueWatch is connection request for receiving issue notification.
 type IssueWatch struct {
 	ID          int64              `xorm:"pk autoincr"`
-	UserID      int64              `xorm:"UNIQUE(watch) NOT NULL"`
 	IssueID     int64              `xorm:"UNIQUE(watch) NOT NULL"`
+	UserID      int64              `xorm:"UNIQUE(watch) NOT NULL"`
 	IsWatching  bool               `xorm:"NOT NULL"`
 	CreatedUnix timeutil.TimeStamp `xorm:"created NOT NULL"`
 	UpdatedUnix timeutil.TimeStamp `xorm:"updated NOT NULL"`
@@ -67,23 +65,6 @@ func GetIssueWatch(ctx context.Context, userID, issueID int64) (iw *IssueWatch,
 	return iw, exists, err
 }
 
-// CheckIssueWatch check if an user is watching an issue
-// it takes participants and repo watch into account
-func CheckIssueWatch(user *user_model.User, issue *Issue) (bool, error) {
-	iw, exist, err := GetIssueWatch(db.DefaultContext, user.ID, issue.ID)
-	if err != nil {
-		return false, err
-	}
-	if exist {
-		return iw.IsWatching, nil
-	}
-	w, err := repo_model.GetWatch(db.DefaultContext, user.ID, issue.RepoID)
-	if err != nil {
-		return false, err
-	}
-	return repo_model.IsWatchMode(w.Mode) || IsUserParticipantsOfIssue(user, issue), nil
-}
-
 // GetIssueWatchersIDs returns IDs of subscribers or explicit unsubscribers to a given issue id
 // but avoids joining with `user` for performance reasons
 // User permissions must be verified elsewhere if required
@@ -96,34 +77,6 @@ func GetIssueWatchersIDs(ctx context.Context, issueID int64, watching bool) ([]i
 		Find(&ids)
 }
 
-// GetIssueWatchers returns watchers/unwatchers of a given issue
-func GetIssueWatchers(ctx context.Context, issueID int64, listOptions db.ListOptions) (IssueWatchList, error) {
-	sess := db.GetEngine(ctx).
-		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")
-
-	if listOptions.Page != 0 {
-		sess = db.SetSessionPagination(sess, &listOptions)
-		watches := make([]*IssueWatch, 0, listOptions.PageSize)
-		return watches, sess.Find(&watches)
-	}
-	watches := make([]*IssueWatch, 0, 8)
-	return watches, sess.Find(&watches)
-}
-
-// CountIssueWatchers count watchers/unwatchers of a given issue
-func CountIssueWatchers(ctx context.Context, issueID int64) (int64, error) {
-	return db.GetEngine(ctx).
-		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").Count(new(IssueWatch))
-}
-
 // RemoveIssueWatchersByRepoID remove issue watchers by repoID
 func RemoveIssueWatchersByRepoID(ctx context.Context, userID, repoID int64) error {
 	_, err := db.GetEngine(ctx).

models/issues/issue_watch_test.go

@@ -41,27 +41,3 @@ func TestGetIssueWatch(t *testing.T) {
 	assert.False(t, exists)
 	assert.NoError(t, err)
 }
-
-func TestGetIssueWatchers(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-
-	iws, err := issues_model.GetIssueWatchers(db.DefaultContext, 1, db.ListOptions{})
-	assert.NoError(t, err)
-	// Watcher is inactive, thus 0
-	assert.Len(t, iws, 0)
-
-	iws, err = issues_model.GetIssueWatchers(db.DefaultContext, 2, db.ListOptions{})
-	assert.NoError(t, err)
-	// Watcher is explicit not watching
-	assert.Len(t, iws, 0)
-
-	iws, err = issues_model.GetIssueWatchers(db.DefaultContext, 5, db.ListOptions{})
-	assert.NoError(t, err)
-	// Issue has no Watchers
-	assert.Len(t, iws, 0)
-
-	iws, err = issues_model.GetIssueWatchers(db.DefaultContext, 7, db.ListOptions{})
-	assert.NoError(t, err)
-	// Issue has one watcher
-	assert.Len(t, iws, 1)
-}

models/migrations/migrations.go

@@ -528,6 +528,8 @@ var migrations = []Migration{
 	NewMigration("Add Version to ActionRun table", v1_21.AddVersionToActionRunTable),
 	// v273 -> v274
 	NewMigration("Add Action Schedule Table", v1_21.AddActionScheduleTable),
+	// v274 -> v275
+	NewMigration("Adjust index order of issue_watch table ", v1_21.AdjustIssueWatchIndexOrder),
 }
 
 // GetCurrentDBVersion returns the current db version

models/migrations/v1_21/v274.go

@@ -0,0 +1,23 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_21 //nolint
+import (
+	"code.gitea.io/gitea/modules/timeutil"
+
+	"xorm.io/xorm"
+)
+
+func AdjustIssueWatchIndexOrder(x *xorm.Engine) error {
+	type IssueWatch struct {
+		ID          int64              `xorm:"pk autoincr"`
+		UserID      int64              `xorm:"UNIQUE(watch) NOT NULL"`
+		IssueID     int64              `xorm:"UNIQUE(watch) NOT NULL"`
+		IsWatching  bool               `xorm:"NOT NULL"`
+		CreatedUnix timeutil.TimeStamp `xorm:"created NOT NULL"`
+		UpdatedUnix timeutil.TimeStamp `xorm:"updated NOT NULL"`
+	}
+	// Drop the old index :(user_id,issue_id)
+	// Then automatically created new index => (issue_id,user_id)
+	return x.DropIndexes(new(IssueWatch))
+}

routers/api/v1/repo/issue_subscription.go

@@ -132,7 +132,7 @@ func setIssueSubscription(ctx *context.APIContext, watch bool) {
 		return
 	}
 
-	current, err := issues_model.CheckIssueWatch(user, issue)
+	current, err := issues_model.CheckIssueSubscriber(user, issue)
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "CheckIssueWatch", err)
 		return
@@ -196,14 +196,14 @@ func CheckIssueSubscription(ctx *context.APIContext) {
 		return
 	}
 
-	watching, err := issues_model.CheckIssueWatch(ctx.Doer, issue)
+	subscribed, err := issues_model.CheckIssueSubscriber(ctx.Doer, issue)
 	if err != nil {
 		ctx.InternalServerError(err)
 		return
 	}
 	ctx.JSON(http.StatusOK, api.WatchInfo{
-		Subscribed:    watching,
-		Ignored:       !watching,
+		Subscribed:    subscribed,
+		Ignored:       !subscribed,
 		Reason:        nil,
 		CreatedAt:     issue.CreatedUnix.AsTime(),
 		URL:           issue.APIURL() + "/subscriptions",
@@ -262,30 +262,20 @@ func GetIssueSubscribers(ctx *context.APIContext) {
 		return
 	}
 
-	iwl, err := issues_model.GetIssueWatchers(ctx, issue.ID, utils.GetListOptions(ctx))
+	users, err := issues_model.GetIssueSubscribers(ctx, issue, utils.GetListOptions(ctx))
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "GetIssueWatchers", err)
+		ctx.Error(http.StatusInternalServerError, "GetIssueSubscribers", err)
 		return
 	}
 
-	userIDs := make([]int64, 0, len(iwl))
-	for _, iw := range iwl {
-		userIDs = append(userIDs, iw.UserID)
-	}
-
-	users, err := user_model.GetUsersByIDs(userIDs)
-	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "GetUsersByIDs", err)
-		return
-	}
 	apiUsers := make([]*api.User, 0, len(users))
 	for _, v := range users {
 		apiUsers = append(apiUsers, convert.ToUser(ctx, v, ctx.Doer))
 	}
 
-	count, err := issues_model.CountIssueWatchers(ctx, issue.ID)
+	count, err := issues_model.CountIssueSubscribers(ctx, issue)
 	if err != nil {
-		ctx.Error(http.StatusInternalServerError, "CountIssueWatchers", err)
+		ctx.Error(http.StatusInternalServerError, "CountIssueSubscribers", err)
 		return
 	}