Fix close issue but time watcher still running (#17761)

* Fix bug

* Update models/issue_stopwatch.go

Co-authored-by: zeripath <art27@cantab.net>

Co-authored-by: zeripath <art27@cantab.net>

6 files changed, 151 insertions, 94 deletions

diff --git a/models/issue_stopwatch.go b/models/issue_stopwatch.go
index 8cdad94fd4..4b53b48111 100644
--- a/models/issue_stopwatch.go
+++ b/models/issue_stopwatch.go
@@ -13,6 +13,26 @@ import (
 	"xorm.io/xorm"
 )
 
+// ErrIssueStopwatchNotExist represents an error that stopwatch is not exist
+type ErrIssueStopwatchNotExist struct {
+	UserID  int64
+	IssueID int64
+}
+
+func (err ErrIssueStopwatchNotExist) Error() string {
+	return fmt.Sprintf("issue stopwatch doesn't exist[uid: %d, issue_id: %d", err.UserID, err.IssueID)
+}
+
+// ErrIssueStopwatchAlreadyExist represents an error that stopwatch is already exist
+type ErrIssueStopwatchAlreadyExist struct {
+	UserID  int64
+	IssueID int64
+}
+
+func (err ErrIssueStopwatchAlreadyExist) Error() string {
+	return fmt.Sprintf("issue stopwatch already exists[uid: %d, issue_id: %d", err.UserID, err.IssueID)
+}
+
 // Stopwatch represents a stopwatch for time tracking.
 type Stopwatch struct {
 	ID          int64              `xorm:"pk autoincr"`
@@ -74,91 +94,141 @@ func hasUserStopwatch(e Engine, userID int64) (exists bool, sw *Stopwatch, err e
 	return
 }
 
+// FinishIssueStopwatchIfPossible if stopwatch exist then finish it otherwise ignore
+func FinishIssueStopwatchIfPossible(user *User, issue *Issue) error {
+	_, exists, err := getStopwatch(x, user.ID, issue.ID)
+	if err != nil {
+		return err
+	}
+	if !exists {
+		return nil
+	}
+	return FinishIssueStopwatch(user, issue)
+}
+
 // CreateOrStopIssueStopwatch will create or remove a stopwatch and will log it into issue's timeline.
 func CreateOrStopIssueStopwatch(user *User, issue *Issue) error {
+	_, exists, err := getStopwatch(x, user.ID, issue.ID)
+	if err != nil {
+		return err
+	}
+	if exists {
+		return FinishIssueStopwatch(user, issue)
+	}
+	return CreateIssueStopwatch(user, issue)
+}
+
+// FinishIssueStopwatch if stopwatch exist then finish it otherwise return an error
+func FinishIssueStopwatch(user *User, issue *Issue) error {
 	sess := x.NewSession()
 	defer sess.Close()
 	if err := sess.Begin(); err != nil {
 		return err
 	}
-	if err := createOrStopIssueStopwatch(sess, user, issue); err != nil {
+	if err := finishIssueStopwatch(sess, user, issue); err != nil {
 		return err
 	}
 	return sess.Commit()
 }
 
-func createOrStopIssueStopwatch(e *xorm.Session, user *User, issue *Issue) error {
+func finishIssueStopwatch(e *xorm.Session, user *User, issue *Issue) error {
 	sw, exists, err := getStopwatch(e, user.ID, issue.ID)
 	if err != nil {
 		return err
 	}
-	if err := issue.loadRepo(e); err != nil {
+	if !exists {
+		return ErrIssueStopwatchNotExist{
+			UserID:  user.ID,
+			IssueID: issue.ID,
+		}
+	}
+
+	// Create tracked time out of the time difference between start date and actual date
+	timediff := time.Now().Unix() - int64(sw.CreatedUnix)
+
+	// Create TrackedTime
+	tt := &TrackedTime{
+		Created: time.Now(),
+		IssueID: issue.ID,
+		UserID:  user.ID,
+		Time:    timediff,
+	}
+
+	if _, err := e.Insert(tt); err != nil {
 		return err
 	}
 
-	if exists {
-		// Create tracked time out of the time difference between start date and actual date
-		timediff := time.Now().Unix() - int64(sw.CreatedUnix)
+	if err := issue.loadRepo(e); err != nil {
+		return err
+	}
+	if _, err := createComment(e, &CreateCommentOptions{
+		Doer:    user,
+		Issue:   issue,
+		Repo:    issue.Repo,
+		Content: SecToTime(timediff),
+		Type:    CommentTypeStopTracking,
+		TimeID:  tt.ID,
+	}); err != nil {
+		return err
+	}
+	_, err = e.Delete(sw)
+	return err
+}
 
-		// Create TrackedTime
-		tt := &TrackedTime{
-			Created: time.Now(),
-			IssueID: issue.ID,
-			UserID:  user.ID,
-			Time:    timediff,
-		}
+// CreateIssueStopwatch creates a stopwatch if not exist, otherwise return an error
+func CreateIssueStopwatch(user *User, issue *Issue) error {
+	sess := x.NewSession()
+	defer sess.Close()
+	if err := sess.Begin(); err != nil {
+		return err
+	}
+	if err := createIssueStopwatch(sess, user, issue); err != nil {
+		return err
+	}
+	return sess.Commit()
+}
 
-		if _, err := e.Insert(tt); err != nil {
-			return err
-		}
+func createIssueStopwatch(e *xorm.Session, user *User, issue *Issue) error {
+	if err := issue.loadRepo(e); err != nil {
+		return err
+	}
 
-		if _, err := createComment(e, &CreateCommentOptions{
-			Doer:    user,
-			Issue:   issue,
-			Repo:    issue.Repo,
-			Content: SecToTime(timediff),
-			Type:    CommentTypeStopTracking,
-			TimeID:  tt.ID,
-		}); err != nil {
-			return err
-		}
-		if _, err := e.Delete(sw); err != nil {
-			return err
-		}
-	} else {
-		// if another stopwatch is running: stop it
-		exists, sw, err := hasUserStopwatch(e, user.ID)
+	// if another stopwatch is running: stop it
+	exists, sw, err := hasUserStopwatch(e, user.ID)
+	if err != nil {
+		return err
+	}
+	if exists {
+		issue, err := getIssueByID(e, sw.IssueID)
 		if err != nil {
 			return err
 		}
-		if exists {
-			issue, err := getIssueByID(e, sw.IssueID)
-			if err != nil {
-				return err
-			}
-			if err := createOrStopIssueStopwatch(e, user, issue); err != nil {
-				return err
-			}
+		if err := finishIssueStopwatch(e, user, issue); err != nil {
+			return err
 		}
+	}
 
-		// Create stopwatch
-		sw = &Stopwatch{
-			UserID:  user.ID,
-			IssueID: issue.ID,
-		}
+	// Create stopwatch
+	sw = &Stopwatch{
+		UserID:  user.ID,
+		IssueID: issue.ID,
+	}
 
-		if _, err := e.Insert(sw); err != nil {
-			return err
-		}
+	if _, err := e.Insert(sw); err != nil {
+		return err
+	}
 
-		if _, err := createComment(e, &CreateCommentOptions{
-			Doer:  user,
-			Issue: issue,
-			Repo:  issue.Repo,
-			Type:  CommentTypeStartTracking,
-		}); err != nil {
-			return err
-		}
+	if err := issue.loadRepo(e); err != nil {
+		return err
+	}
+
+	if _, err := createComment(e, &CreateCommentOptions{
+		Doer:  user,
+		Issue: issue,
+		Repo:  issue.Repo,
+		Type:  CommentTypeStartTracking,
+	}); err != nil {
+		return err
 	}
 	return nil
 }
diff --git a/routers/api/v1/repo/issue_stopwatch.go b/routers/api/v1/repo/issue_stopwatch.go
index a4a2261b9a..2ab6457d7f 100644
--- a/routers/api/v1/repo/issue_stopwatch.go
+++ b/routers/api/v1/repo/issue_stopwatch.go
@@ -55,8 +55,8 @@ func StartIssueStopwatch(ctx *context.APIContext) {
 		return
 	}
 
-	if err := models.CreateOrStopIssueStopwatch(ctx.User, issue); err != nil {
-		ctx.Error(http.StatusInternalServerError, "CreateOrStopIssueStopwatch", err)
+	if err := models.CreateIssueStopwatch(ctx.User, issue); err != nil {
+		ctx.Error(http.StatusInternalServerError, "CreateIssueStopwatch", err)
 		return
 	}
 
@@ -104,8 +104,8 @@ func StopIssueStopwatch(ctx *context.APIContext) {
 		return
 	}
 
-	if err := models.CreateOrStopIssueStopwatch(ctx.User, issue); err != nil {
-		ctx.Error(http.StatusInternalServerError, "CreateOrStopIssueStopwatch", err)
+	if err := models.FinishIssueStopwatch(ctx.User, issue); err != nil {
+		ctx.Error(http.StatusInternalServerError, "FinishIssueStopwatch", err)
 		return
 	}
 
diff --git a/modules/repofiles/action.go b/services/issue/commit.go
index d7e3ff4525..0a74d08949 100644
--- a/modules/repofiles/action.go
+++ b/services/issue/commit.go
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a MIT-style
 // license that can be found in the LICENSE file.
 
-package repofiles
+package issue
 
 import (
 	"fmt"
@@ -13,7 +13,6 @@ import (
 	"time"
 
 	"code.gitea.io/gitea/models"
-	"code.gitea.io/gitea/modules/notification"
 	"code.gitea.io/gitea/modules/references"
 	"code.gitea.io/gitea/modules/repository"
 )
@@ -95,33 +94,6 @@ func issueAddTime(issue *models.Issue, doer *models.User, time time.Time, timeLo
 	return err
 }
 
-func changeIssueStatus(repo *models.Repository, issue *models.Issue, doer *models.User, closed bool) error {
-	stopTimerIfAvailable := func(doer *models.User, issue *models.Issue) error {
-
-		if models.StopwatchExists(doer.ID, issue.ID) {
-			if err := models.CreateOrStopIssueStopwatch(doer, issue); err != nil {
-				return err
-			}
-		}
-
-		return nil
-	}
-
-	issue.Repo = repo
-	comment, err := issue.ChangeStatus(doer, closed)
-	if err != nil {
-		// Don't return an error when dependencies are open as this would let the push fail
-		if models.IsErrDependenciesLeft(err) {
-			return stopTimerIfAvailable(doer, issue)
-		}
-		return err
-	}
-
-	notification.NotifyIssueChangeStatus(doer, issue, comment, closed)
-
-	return stopTimerIfAvailable(doer, issue)
-}
-
 // UpdateIssuesCommit checks if issues are manipulated by commit message.
 func UpdateIssuesCommit(doer *models.User, repo *models.Repository, commits []*repository.PushCommit, branchName string) error {
 	// Commits are appended in the reverse order.
@@ -208,7 +180,10 @@ func UpdateIssuesCommit(doer *models.User, repo *models.Repository, commits []*r
 				}
 			}
 			if close != refIssue.IsClosed {
-				if err := changeIssueStatus(refRepo, refIssue, doer, close); err != nil {
+				if refIssue.Repo != nil {
+					refIssue.Repo = refRepo
+				}
+				if err := ChangeStatus(refIssue, doer, close); err != nil {
 					return err
 				}
 			}
diff --git a/modules/repofiles/action_test.go b/services/issue/commit_test.go
index 97632df68f..f3502d3e0a 100644
--- a/modules/repofiles/action_test.go
+++ b/services/issue/commit_test.go
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a MIT-style
 // license that can be found in the LICENSE file.
 
-package repofiles
+package issue
 
 import (
 	"testing"
diff --git a/services/issue/status.go b/services/issue/status.go
index b01ce4bbb8..35f67bf928 100644
--- a/services/issue/status.go
+++ b/services/issue/status.go
@@ -13,7 +13,19 @@ import (
 func ChangeStatus(issue *models.Issue, doer *models.User, isClosed bool) (err error) {
 	comment, err := issue.ChangeStatus(doer, isClosed)
 	if err != nil {
-		return
+		// Don't return an error when dependencies are open as this would let the push fail
+		if models.IsErrDependenciesLeft(err) {
+			if isClosed {
+				return models.FinishIssueStopwatchIfPossible(doer, issue)
+			}
+		}
+		return err
+	}
+
+	if isClosed {
+		if err := models.FinishIssueStopwatchIfPossible(doer, issue); err != nil {
+			return err
+		}
 	}
 
 	notification.NotifyIssueChangeStatus(doer, issue, comment, isClosed)
diff --git a/services/repository/push.go b/services/repository/push.go
index 03e292757a..dde621e1da 100644
--- a/services/repository/push.go
+++ b/services/repository/push.go
@@ -16,9 +16,9 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/notification"
 	"code.gitea.io/gitea/modules/queue"
-	"code.gitea.io/gitea/modules/repofiles"
 	repo_module "code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/setting"
+	issue_service "code.gitea.io/gitea/services/issue"
 	pull_service "code.gitea.io/gitea/services/pull"
 )
 
@@ -194,7 +194,7 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
 				commits := repo_module.ListToPushCommits(l)
 				commits.HeadCommit = repo_module.CommitToPushCommit(newCommit)
 
-				if err := repofiles.UpdateIssuesCommit(pusher, repo, commits.Commits, refName); err != nil {
+				if err := issue_service.UpdateIssuesCommit(pusher, repo, commits.Commits, refName); err != nil {
 					log.Error("updateIssuesCommit: %v", err)
 				}