From 0add627182388ac63fd04b94cdf912fb87fd0326 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 21 Nov 2021 17:11:48 +0800 Subject: [PATCH] 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 | 193 +++++++++++------- routers/api/v1/repo/issue_stopwatch.go | 5 +- .../action.go => services/issue/commit.go | 51 ++--- .../issue/commit_test.go | 2 +- services/issue/status.go | 22 +- services/repository/push.go | 4 +- 6 files changed, 159 insertions(+), 118 deletions(-) rename modules/repofiles/action.go => services/issue/commit.go (84%) rename modules/repofiles/action_test.go => services/issue/commit_test.go (99%) diff --git a/models/issue_stopwatch.go b/models/issue_stopwatch.go index e9c38b916..eaec5d924 100644 --- a/models/issue_stopwatch.go +++ b/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,93 +103,122 @@ 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. -func CreateOrStopIssueStopwatch(user *User, issue *Issue) error { - ctx, committer, err := db.TxContext() +// 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 } - defer committer.Close() - if err := createOrStopIssueStopwatch(ctx, user, issue); err != nil { - return err + if !exists { + return nil } - return committer.Commit() + return FinishIssueStopwatch(ctx, user, issue) } -func createOrStopIssueStopwatch(ctx context.Context, user *User, issue *Issue) error { - e := db.GetEngine(ctx) - sw, exists, err := getStopwatch(e, user.ID, issue.ID) +// CreateOrStopIssueStopwatch create an issue stopwatch if it's not exist, otherwise finish it +func CreateOrStopIssueStopwatch(user *User, issue *Issue) error { + _, exists, err := getStopwatch(db.DefaultContext, user.ID, issue.ID) if 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) + + // Create TrackedTime + tt := &TrackedTime{ + Created: time.Now(), + IssueID: issue.ID, + UserID: user.ID, + Time: timediff, + } + + if err := db.Insert(ctx, tt); err != nil { + return err + } + + if err := issue.loadRepo(db.GetEngine(ctx)); err != nil { + return err + } + + if _, err := createComment(ctx, &CreateCommentOptions{ + Doer: user, + Issue: issue, + Repo: issue.Repo, + Content: SecToTime(timediff), + Type: CommentTypeStopTracking, + TimeID: tt.ID, + }); err != nil { + return err + } + _, err = db.GetEngine(ctx).Delete(sw) + return err +} + +// 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 { + 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) - - // 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 _, err := createComment(ctx, &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) + 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(ctx, user, issue); err != nil { - return err - } - } - // Create stopwatch - sw = &Stopwatch{ - UserID: user.ID, - IssueID: issue.ID, - } - - if err := db.Insert(ctx, sw); err != nil { - return err - } - - if _, err := createComment(ctx, &CreateCommentOptions{ - Doer: user, - Issue: issue, - Repo: issue.Repo, - Type: CommentTypeStartTracking, - }); err != nil { + if err := FinishIssueStopwatch(ctx, user, issue); err != nil { return err } } + + // Create stopwatch + sw = &Stopwatch{ + UserID: user.ID, + IssueID: issue.ID, + } + + if err := db.Insert(ctx, sw); err != nil { + return err + } + + if err := issue.loadRepo(db.GetEngine(ctx)); err != nil { + return err + } + + if _, err := createComment(ctx, &CreateCommentOptions{ + Doer: user, + Issue: issue, + Repo: issue.Repo, + Type: CommentTypeStartTracking, + }); 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, diff --git a/routers/api/v1/repo/issue_stopwatch.go b/routers/api/v1/repo/issue_stopwatch.go index 82a9ffe10..ce8018251 100644 --- a/routers/api/v1/repo/issue_stopwatch.go +++ b/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 } diff --git a/modules/repofiles/action.go b/services/issue/commit.go similarity index 84% rename from modules/repofiles/action.go rename to services/issue/commit.go index 0bcdb8c3a..401084639 100644 --- a/modules/repofiles/action.go +++ b/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 { - 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) +// 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 { - // Don't return an error when dependencies are open as this would let the push fail - if models.IsErrDependenciesLeft(err) { - return stopTimerIfAvailable(doer, issue) + if models.IsErrIssueNotExist(err) { + return nil, nil } - return err + return nil, err } - - notification.NotifyIssueChangeStatus(doer, issue, comment, closed) - - return stopTimerIfAvailable(doer, issue) + return issue, nil } // 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 } } diff --git a/modules/repofiles/action_test.go b/services/issue/commit_test.go similarity index 99% rename from modules/repofiles/action_test.go rename to services/issue/commit_test.go index d320413db..3f8c5f3b4 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 b01ce4bbb..0a18169a2 100644 --- a/services/issue/status.go +++ b/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) { - comment, err := issue.ChangeStatus(doer, isClosed) +func ChangeStatus(issue *models.Issue, doer *models.User, closed bool) error { + 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 } diff --git a/services/repository/push.go b/services/repository/push.go index a98cfb175..97554c649 100644 --- a/services/repository/push.go +++ b/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) }