From 0f8a6119ae0495ca446422ab8843a634da3d0d3e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 15 Nov 2021 13:04:40 +0800 Subject: [PATCH 1/6] Fix close issue but time watcher still running --- .../action.go => services/issue/commit.go | 51 +++++-------------- .../issue/commit_test.go | 2 +- services/issue/status.go | 27 ++++++++-- services/repository/push.go | 4 +- 4 files changed, 38 insertions(+), 46 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/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 0bcdb8c3a1ac9..401084639d630 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 d320413dbbc45..3f8c5f3b42e50 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 b01ce4bbb86ed..d7482e689a1b3 100644 --- a/services/issue/status.go +++ b/services/issue/status.go @@ -10,12 +10,31 @@ import ( ) // 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 { + 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 + } + + 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) { + return stopTimerIfAvailable(doer, issue) + } + return err } - notification.NotifyIssueChangeStatus(doer, issue, comment, isClosed) + if err := stopTimerIfAvailable(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 a98cfb1758377..97554c64900ca 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) } From e0d082fc981708dc3f7f3ad25054cc4a46e5fe60 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 20 Nov 2021 21:34:18 +0800 Subject: [PATCH 2/6] refactor stopwatch codes --- models/issue_stopwatch.go | 175 +++++++++++++++++++++++--------------- services/issue/status.go | 21 ++--- 2 files changed, 114 insertions(+), 82 deletions(-) diff --git a/models/issue_stopwatch.go b/models/issue_stopwatch.go index e9c38b9165be6..03e5b142b724a 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 is not 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 is already exist[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,110 @@ 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 err := issue.loadRepo(e); 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) + return FinishIssueStopwatch(db.DefaultContext, user, issue) + } + return CreateIssueStopwatch(db.DefaultContext, user, issue) +} - // Create TrackedTime - tt := &TrackedTime{ - Created: time.Now(), - IssueID: issue.ID, +// 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, - Time: timediff, + IssueID: issue.ID, } + } - if _, err := e.Insert(tt); err != nil { - return err - } + // Create tracked time out of the time difference between start date and actual date + timediff := time.Now().Unix() - int64(sw.CreatedUnix) - 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) - 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 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 := 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 +} - // Create stopwatch - sw = &Stopwatch{ +// 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, _, err := hasUserStopwatch(e, user.ID) + if err != nil { + return err + } + if exists { + return ErrIssueStopwatchAlreadyExist{ UserID: user.ID, IssueID: issue.ID, } + } - if err := db.Insert(ctx, sw); err != nil { - return err - } + // Create stopwatch + var sw = &Stopwatch{ + UserID: user.ID, + IssueID: issue.ID, + } - if _, err := createComment(ctx, &CreateCommentOptions{ - Doer: user, - Issue: issue, - Repo: issue.Repo, - Type: CommentTypeStartTracking, - }); err != nil { - return err - } + 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 { + return err + } + return nil } @@ -188,7 +225,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 } diff --git a/services/issue/status.go b/services/issue/status.go index d7482e689a1b3..0a18169a27abd 100644 --- a/services/issue/status.go +++ b/services/issue/status.go @@ -6,32 +6,27 @@ 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, 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 - } - 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) + if closed { + return models.FinishIssueStopwatchIfPossible(db.DefaultContext, doer, issue) + } } return err } - if err := stopTimerIfAvailable(doer, issue); err != nil { - return err + if closed { + if err := models.FinishIssueStopwatchIfPossible(db.DefaultContext, doer, issue); err != nil { + return err + } } notification.NotifyIssueChangeStatus(doer, issue, comment, closed) From d5066552ad1e553456761310e34428c037df261a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 21 Nov 2021 11:10:48 +0800 Subject: [PATCH 3/6] Fix test --- models/issue_stopwatch.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/models/issue_stopwatch.go b/models/issue_stopwatch.go index 03e5b142b724a..b219eed982a36 100644 --- a/models/issue_stopwatch.go +++ b/models/issue_stopwatch.go @@ -155,6 +155,10 @@ func FinishIssueStopwatch(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, @@ -198,6 +202,10 @@ func CreateIssueStopwatch(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, @@ -239,6 +247,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, From 90c5d4394904b1865a6f60cadca3fd80e084b8fb Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 21 Nov 2021 13:07:09 +0800 Subject: [PATCH 4/6] Fix test --- routers/api/v1/repo/issue_stopwatch.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/routers/api/v1/repo/issue_stopwatch.go b/routers/api/v1/repo/issue_stopwatch.go index 82a9ffe10bb73..ce8018251108f 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 } From 632daec8205e8608583ec27a5d490de6a9410e01 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 21 Nov 2021 13:08:18 +0800 Subject: [PATCH 5/6] Fix typo --- models/issue_stopwatch.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/issue_stopwatch.go b/models/issue_stopwatch.go index b219eed982a36..597a69a251285 100644 --- a/models/issue_stopwatch.go +++ b/models/issue_stopwatch.go @@ -20,7 +20,7 @@ type ErrIssueStopwatchNotExist struct { } func (err ErrIssueStopwatchNotExist) Error() string { - return fmt.Sprintf("issue stopwatch is not exist[uid: %d, issue_id: %d", err.UserID, err.IssueID) + 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 @@ -30,7 +30,7 @@ type ErrIssueStopwatchAlreadyExist struct { } func (err ErrIssueStopwatchAlreadyExist) Error() string { - return fmt.Sprintf("issue stopwatch is already exist[uid: %d, issue_id: %d", err.UserID, err.IssueID) + return fmt.Sprintf("issue stopwatch already exists[uid: %d, issue_id: %d", err.UserID, err.IssueID) } // Stopwatch represents a stopwatch for time tracking. From 35cc99c52406e46678f8e5280557f1d97f1edc74 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 21 Nov 2021 16:01:23 +0800 Subject: [PATCH 6/6] Fix test --- models/issue_stopwatch.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/models/issue_stopwatch.go b/models/issue_stopwatch.go index 597a69a251285..eaec5d924e8c9 100644 --- a/models/issue_stopwatch.go +++ b/models/issue_stopwatch.go @@ -181,19 +181,23 @@ func CreateIssueStopwatch(ctx context.Context, user *User, issue *Issue) error { } // if another stopwatch is running: stop it - exists, _, err := hasUserStopwatch(e, user.ID) + exists, sw, err := hasUserStopwatch(e, user.ID) if err != nil { return err } if exists { - return ErrIssueStopwatchAlreadyExist{ - UserID: user.ID, - IssueID: issue.ID, + issue, err := getIssueByID(e, sw.IssueID) + if err != nil { + return err + } + + if err := FinishIssueStopwatch(ctx, user, issue); err != nil { + return err } } // Create stopwatch - var sw = &Stopwatch{ + sw = &Stopwatch{ UserID: user.ID, IssueID: issue.ID, }