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

* Fix close issue but time watcher still running

* refactor stopwatch codes

* Fix test

* Fix test

* Fix typo

* Fix test

models/issue_stopwatch.go

@@ -13,6 +13,26 @@ import (
 	"code.gitea.io/gitea/modules/timeutil"
 )
 
+// 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"`
@@ -35,9 +55,9 @@ func (s Stopwatch) Duration() string {
 	return SecToTime(s.Seconds())
 }
 
-func getStopwatch(e db.Engine, userID, issueID int64) (sw *Stopwatch, exists bool, err error) {
+func getStopwatch(ctx context.Context, userID, issueID int64) (sw *Stopwatch, exists bool, err error) {
 	sw = new(Stopwatch)
-	exists, err = e.
+	exists, err = db.GetEngine(ctx).
 		Where("user_id = ?", userID).
 		And("issue_id = ?", issueID).
 		Get(sw)
@@ -66,7 +86,7 @@ func CountUserStopwatches(userID int64) (int64, error) {
 
 // StopwatchExists returns true if the stopwatch exists
 func StopwatchExists(userID, issueID int64) bool {
-	_, exists, _ := getStopwatch(db.GetEngine(db.DefaultContext), userID, issueID)
+	_, exists, _ := getStopwatch(db.DefaultContext, userID, issueID)
 	return exists
 }
 
@@ -83,30 +103,43 @@ func hasUserStopwatch(e db.Engine, userID int64) (exists bool, sw *Stopwatch, er
 	return
 }
 
-// CreateOrStopIssueStopwatch will create or remove a stopwatch and will log it into issue's timeline.
+// FinishIssueStopwatchIfPossible if stopwatch exist then finish it otherwise ignore
+func FinishIssueStopwatchIfPossible(ctx context.Context, user *User, issue *Issue) error {
+	_, exists, err := getStopwatch(ctx, user.ID, issue.ID)
+	if err != nil {
+		return err
+	}
+	if !exists {
+		return nil
+	}
+	return FinishIssueStopwatch(ctx, user, issue)
+}
+
+// CreateOrStopIssueStopwatch create an issue stopwatch if it's not exist, otherwise finish it
 func CreateOrStopIssueStopwatch(user *User, issue *Issue) error {
-	ctx, committer, err := db.TxContext()
+	_, exists, err := getStopwatch(db.DefaultContext, user.ID, issue.ID)
 	if err != nil {
 		return err
 	}
-	defer committer.Close()
-	if err := createOrStopIssueStopwatch(ctx, user, issue); err != nil {
-		return err
-	}
-	return committer.Commit()
-}
-
-func createOrStopIssueStopwatch(ctx context.Context, user *User, issue *Issue) error {
-	e := db.GetEngine(ctx)
-	sw, exists, err := getStopwatch(e, user.ID, issue.ID)
-	if err != nil {
-		return err
-	}
-	if err := issue.loadRepo(e); err != nil {
-		return err
-	}
-
 	if exists {
+		return FinishIssueStopwatch(db.DefaultContext, user, issue)
+	}
+	return CreateIssueStopwatch(db.DefaultContext, user, issue)
+}
+
+// FinishIssueStopwatch if stopwatch exist then finish it otherwise return an error
+func FinishIssueStopwatch(ctx context.Context, user *User, issue *Issue) error {
+	sw, exists, err := getStopwatch(ctx, user.ID, issue.ID)
+	if err != nil {
+		return err
+	}
+	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)
 
@@ -118,7 +151,11 @@ func createOrStopIssueStopwatch(ctx context.Context, user *User, issue *Issue) e
 		Time:    timediff,
 	}
 
-		if _, err := e.Insert(tt); err != nil {
+	if err := db.Insert(ctx, tt); err != nil {
+		return err
+	}
+
+	if err := issue.loadRepo(db.GetEngine(ctx)); err != nil {
 		return err
 	}
 
@@ -132,10 +169,17 @@ func createOrStopIssueStopwatch(ctx context.Context, user *User, issue *Issue) e
 	}); err != nil {
 		return err
 	}
-		if _, err := e.Delete(sw); err != nil {
+	_, err = db.GetEngine(ctx).Delete(sw)
 	return err
 }
-	} else {
+
+// CreateIssueStopwatch creates a stopwatch if not exist, otherwise return an error
+func CreateIssueStopwatch(ctx context.Context, user *User, issue *Issue) error {
+	e := db.GetEngine(ctx)
+	if err := issue.loadRepo(e); err != nil {
+		return err
+	}
+
 	// if another stopwatch is running: stop it
 	exists, sw, err := hasUserStopwatch(e, user.ID)
 	if err != nil {
@@ -146,7 +190,8 @@ func createOrStopIssueStopwatch(ctx context.Context, user *User, issue *Issue) e
 		if err != nil {
 			return err
 		}
-			if err := createOrStopIssueStopwatch(ctx, user, issue); err != nil {
+
+		if err := FinishIssueStopwatch(ctx, user, issue); err != nil {
 			return err
 		}
 	}
@@ -161,6 +206,10 @@ func createOrStopIssueStopwatch(ctx context.Context, user *User, issue *Issue) e
 		return err
 	}
 
+	if err := issue.loadRepo(db.GetEngine(ctx)); err != nil {
+		return err
+	}
+
 	if _, err := createComment(ctx, &CreateCommentOptions{
 		Doer:  user,
 		Issue: issue,
@@ -169,7 +218,7 @@ func createOrStopIssueStopwatch(ctx context.Context, user *User, issue *Issue) e
 	}); err != nil {
 		return err
 	}
-	}
+
 	return nil
 }
 
@@ -188,7 +237,7 @@ func CancelStopwatch(user *User, issue *Issue) error {
 
 func cancelStopwatch(ctx context.Context, user *User, issue *Issue) error {
 	e := db.GetEngine(ctx)
-	sw, exists, err := getStopwatch(e, user.ID, issue.ID)
+	sw, exists, err := getStopwatch(ctx, user.ID, issue.ID)
 	if err != nil {
 		return err
 	}
@@ -202,6 +251,10 @@ func cancelStopwatch(ctx context.Context, user *User, issue *Issue) error {
 			return err
 		}
 
+		if err := issue.loadRepo(db.GetEngine(ctx)); err != nil {
+			return err
+		}
+
 		if _, err := createComment(ctx, &CreateCommentOptions{
 			Doer:  user,
 			Issue: issue,

routers/api/v1/repo/issue_stopwatch.go

@@ -9,6 +9,7 @@ import (
 	"net/http"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/convert"
 	"code.gitea.io/gitea/routers/api/v1/utils"
@@ -55,7 +56,7 @@ func StartIssueStopwatch(ctx *context.APIContext) {
 		return
 	}
 
-	if err := models.CreateOrStopIssueStopwatch(ctx.User, issue); err != nil {
+	if err := models.CreateIssueStopwatch(db.DefaultContext, ctx.User, issue); err != nil {
 		ctx.Error(http.StatusInternalServerError, "CreateOrStopIssueStopwatch", err)
 		return
 	}
@@ -104,7 +105,7 @@ func StopIssueStopwatch(ctx *context.APIContext) {
 		return
 	}
 
-	if err := models.CreateOrStopIssueStopwatch(ctx.User, issue); err != nil {
+	if err := models.FinishIssueStopwatch(db.DefaultContext, ctx.User, issue); err != nil {
 		ctx.Error(http.StatusInternalServerError, "CreateOrStopIssueStopwatch", err)
 		return
 	}

services/issue/commit.go

@@ -1,8 +1,8 @@
-// Copyright 2019 The Gitea Authors. All rights reserved.
+// Copyright 2021 The Gitea Authors. All rights reserved.
 // 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"
@@ -14,7 +14,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"
 )
@@ -29,19 +28,6 @@ const (
 
 var reDuration = regexp.MustCompile(`(?i)^(?:(\d+([\.,]\d+)?)(?:mo))?(?:(\d+([\.,]\d+)?)(?:w))?(?:(\d+([\.,]\d+)?)(?:d))?(?:(\d+([\.,]\d+)?)(?:h))?(?:(\d+([\.,]\d+)?)(?:m))?$`)
 
-// getIssueFromRef returns the issue referenced by a ref. Returns a nil *Issue
-// if the provided ref references a non-existent issue.
-func getIssueFromRef(repo *models.Repository, index int64) (*models.Issue, error) {
-	issue, err := models.GetIssueByIndex(repo.ID, index)
-	if err != nil {
-		if models.IsErrIssueNotExist(err) {
-			return nil, nil
-		}
-		return nil, err
-	}
-	return issue, nil
-}
-
 // timeLogToAmount parses time log string and returns amount in seconds
 func timeLogToAmount(str string) int64 {
 	matches := reDuration.FindAllStringSubmatch(str, -1)
@@ -96,31 +82,17 @@ 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 {
+// getIssueFromRef returns the issue referenced by a ref. Returns a nil *Issue
-	stopTimerIfAvailable := func(doer *models.User, issue *models.Issue) error {
+// if the provided ref references a non-existent issue.
-
+func getIssueFromRef(repo *models.Repository, index int64) (*models.Issue, error) {
-		if models.StopwatchExists(doer.ID, issue.ID) {
+	issue, err := models.GetIssueByIndex(repo.ID, index)
-			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.IsErrIssueNotExist(err) {
-		if models.IsErrDependenciesLeft(err) {
+			return nil, nil
-			return stopTimerIfAvailable(doer, issue)
 		}
-		return err
+		return nil, err
 	}
-
+	return issue, nil
-	notification.NotifyIssueChangeStatus(doer, issue, comment, closed)
-
-	return stopTimerIfAvailable(doer, issue)
 }
 
 // UpdateIssuesCommit checks if issues are manipulated by commit message.
@@ -209,7 +181,8 @@ func UpdateIssuesCommit(doer *models.User, repo *models.Repository, commits []*r
 				}
 			}
 			if close != refIssue.IsClosed {
-				if err := changeIssueStatus(refRepo, refIssue, doer, close); err != nil {
+				refIssue.Repo = refRepo
+				if err := ChangeStatus(refIssue, doer, close); err != nil {
 					return err
 				}
 			}

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"

services/issue/status.go

@@ -6,16 +6,30 @@ package issue
 
 import (
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/modules/notification"
 )
 
 // ChangeStatus changes issue status to open or closed.
-func ChangeStatus(issue *models.Issue, doer *models.User, isClosed bool) (err error) {
+func ChangeStatus(issue *models.Issue, doer *models.User, closed bool) error {
-	comment, err := issue.ChangeStatus(doer, isClosed)
+	comment, err := issue.ChangeStatus(doer, closed)
 	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 closed {
+				return models.FinishIssueStopwatchIfPossible(db.DefaultContext, doer, issue)
+			}
+		}
+		return err
 	}
 
-	notification.NotifyIssueChangeStatus(doer, issue, comment, isClosed)
+	if closed {
+		if err := models.FinishIssueStopwatchIfPossible(db.DefaultContext, doer, issue); err != nil {
+			return err
+		}
+	}
+
+	notification.NotifyIssueChangeStatus(doer, issue, comment, closed)
+
 	return nil
 }

services/repository/push.go

@@ -19,10 +19,10 @@ 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"
 	"code.gitea.io/gitea/modules/timeutil"
+	issue_service "code.gitea.io/gitea/services/issue"
 	pull_service "code.gitea.io/gitea/services/pull"
 )
 
@@ -198,7 +198,7 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
 				commits := repo_module.GitToPushCommits(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)
 				}