From 0add627182388ac63fd04b94cdf912fb87fd0326 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 21 Nov 2021 17:11:48 +0800 Subject: 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 | 187 +++++++++++++-------- modules/repofiles/action.go | 219 ------------------------ modules/repofiles/action_test.go | 298 --------------------------------- routers/api/v1/repo/issue_stopwatch.go | 5 +- services/issue/commit.go | 192 +++++++++++++++++++++ services/issue/commit_test.go | 298 +++++++++++++++++++++++++++++++++ services/issue/status.go | 22 ++- services/repository/push.go | 4 +- 8 files changed, 633 insertions(+), 592 deletions(-) delete mode 100644 modules/repofiles/action.go delete mode 100644 modules/repofiles/action_test.go create mode 100644 services/issue/commit.go create mode 100644 services/issue/commit_test.go diff --git a/models/issue_stopwatch.go b/models/issue_stopwatch.go index e9c38b9165..eaec5d924e 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 { + 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 { + _, exists, err := getStopwatch(db.DefaultContext, user.ID, issue.ID) + if err != nil { return err } - return committer.Commit() + if exists { + return FinishIssueStopwatch(db.DefaultContext, user, issue) + } + return CreateIssueStopwatch(db.DefaultContext, 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) +// 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 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 := db.Insert(ctx, 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(db.GetEngine(ctx)); err != nil { + return err + } - // Create TrackedTime - tt := &TrackedTime{ - Created: time.Now(), - IssueID: issue.ID, - UserID: user.ID, - Time: timediff, - } + 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 +} - if _, err := e.Insert(tt); err != nil { - 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 _, 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 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(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 { + if err := FinishIssueStopwatch(ctx, user, issue); err != nil { return err } + } - if _, err := createComment(ctx, &CreateCommentOptions{ - Doer: user, - Issue: issue, - Repo: issue.Repo, - Type: CommentTypeStartTracking, - }); 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/modules/repofiles/action.go b/modules/repofiles/action.go deleted file mode 100644 index 0bcdb8c3a1..0000000000 --- a/modules/repofiles/action.go +++ /dev/null @@ -1,219 +0,0 @@ -// Copyright 2019 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 - -import ( - "fmt" - "html" - "net/url" - "regexp" - "strconv" - "strings" - "time" - - "code.gitea.io/gitea/models" - "code.gitea.io/gitea/modules/notification" - "code.gitea.io/gitea/modules/references" - "code.gitea.io/gitea/modules/repository" -) - -const ( - secondsByMinute = float64(time.Minute / time.Second) // seconds in a minute - secondsByHour = 60 * secondsByMinute // seconds in an hour - secondsByDay = 8 * secondsByHour // seconds in a day - secondsByWeek = 5 * secondsByDay // seconds in a week - secondsByMonth = 4 * secondsByWeek // seconds in a month -) - -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) - if len(matches) == 0 { - return 0 - } - - match := matches[0] - - var a int64 - - // months - if len(match[1]) > 0 { - mo, _ := strconv.ParseFloat(strings.Replace(match[1], ",", ".", 1), 64) - a += int64(mo * secondsByMonth) - } - - // weeks - if len(match[3]) > 0 { - w, _ := strconv.ParseFloat(strings.Replace(match[3], ",", ".", 1), 64) - a += int64(w * secondsByWeek) - } - - // days - if len(match[5]) > 0 { - d, _ := strconv.ParseFloat(strings.Replace(match[5], ",", ".", 1), 64) - a += int64(d * secondsByDay) - } - - // hours - if len(match[7]) > 0 { - h, _ := strconv.ParseFloat(strings.Replace(match[7], ",", ".", 1), 64) - a += int64(h * secondsByHour) - } - - // minutes - if len(match[9]) > 0 { - d, _ := strconv.ParseFloat(strings.Replace(match[9], ",", ".", 1), 64) - a += int64(d * secondsByMinute) - } - - return a -} - -func issueAddTime(issue *models.Issue, doer *models.User, time time.Time, timeLog string) error { - amount := timeLogToAmount(timeLog) - if amount == 0 { - return nil - } - - _, err := models.AddTime(doer, issue, amount, time) - 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. - for i := len(commits) - 1; i >= 0; i-- { - c := commits[i] - - type markKey struct { - ID int64 - Action references.XRefAction - } - - refMarked := make(map[markKey]bool) - var refRepo *models.Repository - var refIssue *models.Issue - var err error - for _, ref := range references.FindAllIssueReferences(c.Message) { - - // issue is from another repo - if len(ref.Owner) > 0 && len(ref.Name) > 0 { - refRepo, err = models.GetRepositoryFromMatch(ref.Owner, ref.Name) - if err != nil { - continue - } - } else { - refRepo = repo - } - if refIssue, err = getIssueFromRef(refRepo, ref.Index); err != nil { - return err - } - if refIssue == nil { - continue - } - - perm, err := models.GetUserRepoPermission(refRepo, doer) - if err != nil { - return err - } - - key := markKey{ID: refIssue.ID, Action: ref.Action} - if refMarked[key] { - continue - } - refMarked[key] = true - - // FIXME: this kind of condition is all over the code, it should be consolidated in a single place - canclose := perm.IsAdmin() || perm.IsOwner() || perm.CanWriteIssuesOrPulls(refIssue.IsPull) || refIssue.PosterID == doer.ID - cancomment := canclose || perm.CanReadIssuesOrPulls(refIssue.IsPull) - - // Don't proceed if the user can't comment - if !cancomment { - continue - } - - message := fmt.Sprintf(`%s`, html.EscapeString(repo.Link()), html.EscapeString(url.PathEscape(c.Sha1)), html.EscapeString(strings.SplitN(c.Message, "\n", 2)[0])) - if err = models.CreateRefComment(doer, refRepo, refIssue, message, c.Sha1); err != nil { - return err - } - - // Only issues can be closed/reopened this way, and user needs the correct permissions - if refIssue.IsPull || !canclose { - continue - } - - // Only process closing/reopening keywords - if ref.Action != references.XRefActionCloses && ref.Action != references.XRefActionReopens { - continue - } - - if !repo.CloseIssuesViaCommitInAnyBranch { - // If the issue was specified to be in a particular branch, don't allow commits in other branches to close it - if refIssue.Ref != "" { - if branchName != refIssue.Ref { - continue - } - // Otherwise, only process commits to the default branch - } else if branchName != repo.DefaultBranch { - continue - } - } - close := ref.Action == references.XRefActionCloses - if close && len(ref.TimeLog) > 0 { - if err := issueAddTime(refIssue, doer, c.Timestamp, ref.TimeLog); err != nil { - return err - } - } - if close != refIssue.IsClosed { - if err := changeIssueStatus(refRepo, refIssue, doer, close); err != nil { - return err - } - } - } - } - return nil -} diff --git a/modules/repofiles/action_test.go b/modules/repofiles/action_test.go deleted file mode 100644 index d320413dbb..0000000000 --- a/modules/repofiles/action_test.go +++ /dev/null @@ -1,298 +0,0 @@ -// Copyright 2019 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 - -import ( - "testing" - - "code.gitea.io/gitea/models" - "code.gitea.io/gitea/models/unittest" - "code.gitea.io/gitea/modules/repository" - "code.gitea.io/gitea/modules/setting" - - "github.com/stretchr/testify/assert" -) - -func TestUpdateIssuesCommit(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - pushCommits := []*repository.PushCommit{ - { - Sha1: "abcdef1", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user4@example.com", - AuthorName: "User Four", - Message: "start working on #FST-1, #1", - }, - { - Sha1: "abcdef2", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "a plain message", - }, - { - Sha1: "abcdef2", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "close #2", - }, - } - - user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) - repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository) - repo.Owner = user - - commentBean := &models.Comment{ - Type: models.CommentTypeCommitRef, - CommitSHA: "abcdef1", - PosterID: user.ID, - IssueID: 1, - } - issueBean := &models.Issue{RepoID: repo.ID, Index: 4} - - unittest.AssertNotExistsBean(t, commentBean) - unittest.AssertNotExistsBean(t, &models.Issue{RepoID: repo.ID, Index: 2}, "is_closed=1") - assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) - unittest.AssertExistsAndLoadBean(t, commentBean) - unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1") - unittest.CheckConsistencyFor(t, &models.Action{}) - - // Test that push to a non-default branch closes no issue. - pushCommits = []*repository.PushCommit{ - { - Sha1: "abcdef1", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user4@example.com", - AuthorName: "User Four", - Message: "close #1", - }, - } - repo = unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 3}).(*models.Repository) - commentBean = &models.Comment{ - Type: models.CommentTypeCommitRef, - CommitSHA: "abcdef1", - PosterID: user.ID, - IssueID: 6, - } - issueBean = &models.Issue{RepoID: repo.ID, Index: 1} - - unittest.AssertNotExistsBean(t, commentBean) - unittest.AssertNotExistsBean(t, &models.Issue{RepoID: repo.ID, Index: 1}, "is_closed=1") - assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, "non-existing-branch")) - unittest.AssertExistsAndLoadBean(t, commentBean) - unittest.AssertNotExistsBean(t, issueBean, "is_closed=1") - unittest.CheckConsistencyFor(t, &models.Action{}) - - pushCommits = []*repository.PushCommit{ - { - Sha1: "abcdef3", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "close " + setting.AppURL + repo.FullName() + "/pulls/1", - }, - } - repo = unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 3}).(*models.Repository) - commentBean = &models.Comment{ - Type: models.CommentTypeCommitRef, - CommitSHA: "abcdef3", - PosterID: user.ID, - IssueID: 6, - } - issueBean = &models.Issue{RepoID: repo.ID, Index: 1} - - unittest.AssertNotExistsBean(t, commentBean) - unittest.AssertNotExistsBean(t, &models.Issue{RepoID: repo.ID, Index: 1}, "is_closed=1") - assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) - unittest.AssertExistsAndLoadBean(t, commentBean) - unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1") - unittest.CheckConsistencyFor(t, &models.Action{}) -} - -func TestUpdateIssuesCommit_Colon(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - pushCommits := []*repository.PushCommit{ - { - Sha1: "abcdef2", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "close: #2", - }, - } - - user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) - repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository) - repo.Owner = user - - issueBean := &models.Issue{RepoID: repo.ID, Index: 4} - - unittest.AssertNotExistsBean(t, &models.Issue{RepoID: repo.ID, Index: 2}, "is_closed=1") - assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) - unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1") - unittest.CheckConsistencyFor(t, &models.Action{}) -} - -func TestUpdateIssuesCommit_Issue5957(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) - - // Test that push to a non-default branch closes an issue. - pushCommits := []*repository.PushCommit{ - { - Sha1: "abcdef1", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user4@example.com", - AuthorName: "User Four", - Message: "close #2", - }, - } - - repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository) - commentBean := &models.Comment{ - Type: models.CommentTypeCommitRef, - CommitSHA: "abcdef1", - PosterID: user.ID, - IssueID: 7, - } - - issueBean := &models.Issue{RepoID: repo.ID, Index: 2, ID: 7} - - unittest.AssertNotExistsBean(t, commentBean) - unittest.AssertNotExistsBean(t, issueBean, "is_closed=1") - assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, "non-existing-branch")) - unittest.AssertExistsAndLoadBean(t, commentBean) - unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1") - unittest.CheckConsistencyFor(t, &models.Action{}) -} - -func TestUpdateIssuesCommit_AnotherRepo(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) - - // Test that a push to default branch closes issue in another repo - // If the user also has push permissions to that repo - pushCommits := []*repository.PushCommit{ - { - Sha1: "abcdef1", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "close user2/repo1#1", - }, - } - - repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository) - commentBean := &models.Comment{ - Type: models.CommentTypeCommitRef, - CommitSHA: "abcdef1", - PosterID: user.ID, - IssueID: 1, - } - - issueBean := &models.Issue{RepoID: 1, Index: 1, ID: 1} - - unittest.AssertNotExistsBean(t, commentBean) - unittest.AssertNotExistsBean(t, issueBean, "is_closed=1") - assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) - unittest.AssertExistsAndLoadBean(t, commentBean) - unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1") - unittest.CheckConsistencyFor(t, &models.Action{}) -} - -func TestUpdateIssuesCommit_AnotherRepo_FullAddress(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) - - // Test that a push to default branch closes issue in another repo - // If the user also has push permissions to that repo - pushCommits := []*repository.PushCommit{ - { - Sha1: "abcdef1", - CommitterEmail: "user2@example.com", - CommitterName: "User Two", - AuthorEmail: "user2@example.com", - AuthorName: "User Two", - Message: "close " + setting.AppURL + "user2/repo1/issues/1", - }, - } - - repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository) - commentBean := &models.Comment{ - Type: models.CommentTypeCommitRef, - CommitSHA: "abcdef1", - PosterID: user.ID, - IssueID: 1, - } - - issueBean := &models.Issue{RepoID: 1, Index: 1, ID: 1} - - unittest.AssertNotExistsBean(t, commentBean) - unittest.AssertNotExistsBean(t, issueBean, "is_closed=1") - assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) - unittest.AssertExistsAndLoadBean(t, commentBean) - unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1") - unittest.CheckConsistencyFor(t, &models.Action{}) -} - -func TestUpdateIssuesCommit_AnotherRepoNoPermission(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 10}).(*models.User) - - // Test that a push with close reference *can not* close issue - // If the committer doesn't have push rights in that repo - pushCommits := []*repository.PushCommit{ - { - Sha1: "abcdef3", - CommitterEmail: "user10@example.com", - CommitterName: "User Ten", - AuthorEmail: "user10@example.com", - AuthorName: "User Ten", - Message: "close user3/repo3#1", - }, - { - Sha1: "abcdef4", - CommitterEmail: "user10@example.com", - CommitterName: "User Ten", - AuthorEmail: "user10@example.com", - AuthorName: "User Ten", - Message: "close " + setting.AppURL + "user3/repo3/issues/1", - }, - } - - repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 6}).(*models.Repository) - commentBean := &models.Comment{ - Type: models.CommentTypeCommitRef, - CommitSHA: "abcdef3", - PosterID: user.ID, - IssueID: 6, - } - commentBean2 := &models.Comment{ - Type: models.CommentTypeCommitRef, - CommitSHA: "abcdef4", - PosterID: user.ID, - IssueID: 6, - } - - issueBean := &models.Issue{RepoID: 3, Index: 1, ID: 6} - - unittest.AssertNotExistsBean(t, commentBean) - unittest.AssertNotExistsBean(t, commentBean2) - unittest.AssertNotExistsBean(t, issueBean, "is_closed=1") - assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) - unittest.AssertNotExistsBean(t, commentBean) - unittest.AssertNotExistsBean(t, commentBean2) - unittest.AssertNotExistsBean(t, issueBean, "is_closed=1") - unittest.CheckConsistencyFor(t, &models.Action{}) -} diff --git a/routers/api/v1/repo/issue_stopwatch.go b/routers/api/v1/repo/issue_stopwatch.go index 82a9ffe10b..ce80182511 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/services/issue/commit.go b/services/issue/commit.go new file mode 100644 index 0000000000..401084639d --- /dev/null +++ b/services/issue/commit.go @@ -0,0 +1,192 @@ +// 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 issue + +import ( + "fmt" + "html" + "net/url" + "regexp" + "strconv" + "strings" + "time" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/references" + "code.gitea.io/gitea/modules/repository" +) + +const ( + secondsByMinute = float64(time.Minute / time.Second) // seconds in a minute + secondsByHour = 60 * secondsByMinute // seconds in an hour + secondsByDay = 8 * secondsByHour // seconds in a day + secondsByWeek = 5 * secondsByDay // seconds in a week + secondsByMonth = 4 * secondsByWeek // seconds in a month +) + +var reDuration = regexp.MustCompile(`(?i)^(?:(\d+([\.,]\d+)?)(?:mo))?(?:(\d+([\.,]\d+)?)(?:w))?(?:(\d+([\.,]\d+)?)(?:d))?(?:(\d+([\.,]\d+)?)(?:h))?(?:(\d+([\.,]\d+)?)(?:m))?$`) + +// timeLogToAmount parses time log string and returns amount in seconds +func timeLogToAmount(str string) int64 { + matches := reDuration.FindAllStringSubmatch(str, -1) + if len(matches) == 0 { + return 0 + } + + match := matches[0] + + var a int64 + + // months + if len(match[1]) > 0 { + mo, _ := strconv.ParseFloat(strings.Replace(match[1], ",", ".", 1), 64) + a += int64(mo * secondsByMonth) + } + + // weeks + if len(match[3]) > 0 { + w, _ := strconv.ParseFloat(strings.Replace(match[3], ",", ".", 1), 64) + a += int64(w * secondsByWeek) + } + + // days + if len(match[5]) > 0 { + d, _ := strconv.ParseFloat(strings.Replace(match[5], ",", ".", 1), 64) + a += int64(d * secondsByDay) + } + + // hours + if len(match[7]) > 0 { + h, _ := strconv.ParseFloat(strings.Replace(match[7], ",", ".", 1), 64) + a += int64(h * secondsByHour) + } + + // minutes + if len(match[9]) > 0 { + d, _ := strconv.ParseFloat(strings.Replace(match[9], ",", ".", 1), 64) + a += int64(d * secondsByMinute) + } + + return a +} + +func issueAddTime(issue *models.Issue, doer *models.User, time time.Time, timeLog string) error { + amount := timeLogToAmount(timeLog) + if amount == 0 { + return nil + } + + _, err := models.AddTime(doer, issue, amount, time) + return err +} + +// 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 +} + +// 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. + for i := len(commits) - 1; i >= 0; i-- { + c := commits[i] + + type markKey struct { + ID int64 + Action references.XRefAction + } + + refMarked := make(map[markKey]bool) + var refRepo *models.Repository + var refIssue *models.Issue + var err error + for _, ref := range references.FindAllIssueReferences(c.Message) { + + // issue is from another repo + if len(ref.Owner) > 0 && len(ref.Name) > 0 { + refRepo, err = models.GetRepositoryFromMatch(ref.Owner, ref.Name) + if err != nil { + continue + } + } else { + refRepo = repo + } + if refIssue, err = getIssueFromRef(refRepo, ref.Index); err != nil { + return err + } + if refIssue == nil { + continue + } + + perm, err := models.GetUserRepoPermission(refRepo, doer) + if err != nil { + return err + } + + key := markKey{ID: refIssue.ID, Action: ref.Action} + if refMarked[key] { + continue + } + refMarked[key] = true + + // FIXME: this kind of condition is all over the code, it should be consolidated in a single place + canclose := perm.IsAdmin() || perm.IsOwner() || perm.CanWriteIssuesOrPulls(refIssue.IsPull) || refIssue.PosterID == doer.ID + cancomment := canclose || perm.CanReadIssuesOrPulls(refIssue.IsPull) + + // Don't proceed if the user can't comment + if !cancomment { + continue + } + + message := fmt.Sprintf(`%s`, html.EscapeString(repo.Link()), html.EscapeString(url.PathEscape(c.Sha1)), html.EscapeString(strings.SplitN(c.Message, "\n", 2)[0])) + if err = models.CreateRefComment(doer, refRepo, refIssue, message, c.Sha1); err != nil { + return err + } + + // Only issues can be closed/reopened this way, and user needs the correct permissions + if refIssue.IsPull || !canclose { + continue + } + + // Only process closing/reopening keywords + if ref.Action != references.XRefActionCloses && ref.Action != references.XRefActionReopens { + continue + } + + if !repo.CloseIssuesViaCommitInAnyBranch { + // If the issue was specified to be in a particular branch, don't allow commits in other branches to close it + if refIssue.Ref != "" { + if branchName != refIssue.Ref { + continue + } + // Otherwise, only process commits to the default branch + } else if branchName != repo.DefaultBranch { + continue + } + } + close := ref.Action == references.XRefActionCloses + if close && len(ref.TimeLog) > 0 { + if err := issueAddTime(refIssue, doer, c.Timestamp, ref.TimeLog); err != nil { + return err + } + } + if close != refIssue.IsClosed { + refIssue.Repo = refRepo + if err := ChangeStatus(refIssue, doer, close); err != nil { + return err + } + } + } + } + return nil +} diff --git a/services/issue/commit_test.go b/services/issue/commit_test.go new file mode 100644 index 0000000000..3f8c5f3b42 --- /dev/null +++ b/services/issue/commit_test.go @@ -0,0 +1,298 @@ +// Copyright 2019 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 issue + +import ( + "testing" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/repository" + "code.gitea.io/gitea/modules/setting" + + "github.com/stretchr/testify/assert" +) + +func TestUpdateIssuesCommit(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + pushCommits := []*repository.PushCommit{ + { + Sha1: "abcdef1", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user4@example.com", + AuthorName: "User Four", + Message: "start working on #FST-1, #1", + }, + { + Sha1: "abcdef2", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "a plain message", + }, + { + Sha1: "abcdef2", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "close #2", + }, + } + + user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) + repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository) + repo.Owner = user + + commentBean := &models.Comment{ + Type: models.CommentTypeCommitRef, + CommitSHA: "abcdef1", + PosterID: user.ID, + IssueID: 1, + } + issueBean := &models.Issue{RepoID: repo.ID, Index: 4} + + unittest.AssertNotExistsBean(t, commentBean) + unittest.AssertNotExistsBean(t, &models.Issue{RepoID: repo.ID, Index: 2}, "is_closed=1") + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) + unittest.AssertExistsAndLoadBean(t, commentBean) + unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1") + unittest.CheckConsistencyFor(t, &models.Action{}) + + // Test that push to a non-default branch closes no issue. + pushCommits = []*repository.PushCommit{ + { + Sha1: "abcdef1", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user4@example.com", + AuthorName: "User Four", + Message: "close #1", + }, + } + repo = unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 3}).(*models.Repository) + commentBean = &models.Comment{ + Type: models.CommentTypeCommitRef, + CommitSHA: "abcdef1", + PosterID: user.ID, + IssueID: 6, + } + issueBean = &models.Issue{RepoID: repo.ID, Index: 1} + + unittest.AssertNotExistsBean(t, commentBean) + unittest.AssertNotExistsBean(t, &models.Issue{RepoID: repo.ID, Index: 1}, "is_closed=1") + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, "non-existing-branch")) + unittest.AssertExistsAndLoadBean(t, commentBean) + unittest.AssertNotExistsBean(t, issueBean, "is_closed=1") + unittest.CheckConsistencyFor(t, &models.Action{}) + + pushCommits = []*repository.PushCommit{ + { + Sha1: "abcdef3", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "close " + setting.AppURL + repo.FullName() + "/pulls/1", + }, + } + repo = unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 3}).(*models.Repository) + commentBean = &models.Comment{ + Type: models.CommentTypeCommitRef, + CommitSHA: "abcdef3", + PosterID: user.ID, + IssueID: 6, + } + issueBean = &models.Issue{RepoID: repo.ID, Index: 1} + + unittest.AssertNotExistsBean(t, commentBean) + unittest.AssertNotExistsBean(t, &models.Issue{RepoID: repo.ID, Index: 1}, "is_closed=1") + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) + unittest.AssertExistsAndLoadBean(t, commentBean) + unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1") + unittest.CheckConsistencyFor(t, &models.Action{}) +} + +func TestUpdateIssuesCommit_Colon(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + pushCommits := []*repository.PushCommit{ + { + Sha1: "abcdef2", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "close: #2", + }, + } + + user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) + repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository) + repo.Owner = user + + issueBean := &models.Issue{RepoID: repo.ID, Index: 4} + + unittest.AssertNotExistsBean(t, &models.Issue{RepoID: repo.ID, Index: 2}, "is_closed=1") + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) + unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1") + unittest.CheckConsistencyFor(t, &models.Action{}) +} + +func TestUpdateIssuesCommit_Issue5957(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) + + // Test that push to a non-default branch closes an issue. + pushCommits := []*repository.PushCommit{ + { + Sha1: "abcdef1", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user4@example.com", + AuthorName: "User Four", + Message: "close #2", + }, + } + + repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository) + commentBean := &models.Comment{ + Type: models.CommentTypeCommitRef, + CommitSHA: "abcdef1", + PosterID: user.ID, + IssueID: 7, + } + + issueBean := &models.Issue{RepoID: repo.ID, Index: 2, ID: 7} + + unittest.AssertNotExistsBean(t, commentBean) + unittest.AssertNotExistsBean(t, issueBean, "is_closed=1") + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, "non-existing-branch")) + unittest.AssertExistsAndLoadBean(t, commentBean) + unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1") + unittest.CheckConsistencyFor(t, &models.Action{}) +} + +func TestUpdateIssuesCommit_AnotherRepo(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) + + // Test that a push to default branch closes issue in another repo + // If the user also has push permissions to that repo + pushCommits := []*repository.PushCommit{ + { + Sha1: "abcdef1", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "close user2/repo1#1", + }, + } + + repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository) + commentBean := &models.Comment{ + Type: models.CommentTypeCommitRef, + CommitSHA: "abcdef1", + PosterID: user.ID, + IssueID: 1, + } + + issueBean := &models.Issue{RepoID: 1, Index: 1, ID: 1} + + unittest.AssertNotExistsBean(t, commentBean) + unittest.AssertNotExistsBean(t, issueBean, "is_closed=1") + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) + unittest.AssertExistsAndLoadBean(t, commentBean) + unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1") + unittest.CheckConsistencyFor(t, &models.Action{}) +} + +func TestUpdateIssuesCommit_AnotherRepo_FullAddress(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User) + + // Test that a push to default branch closes issue in another repo + // If the user also has push permissions to that repo + pushCommits := []*repository.PushCommit{ + { + Sha1: "abcdef1", + CommitterEmail: "user2@example.com", + CommitterName: "User Two", + AuthorEmail: "user2@example.com", + AuthorName: "User Two", + Message: "close " + setting.AppURL + "user2/repo1/issues/1", + }, + } + + repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 2}).(*models.Repository) + commentBean := &models.Comment{ + Type: models.CommentTypeCommitRef, + CommitSHA: "abcdef1", + PosterID: user.ID, + IssueID: 1, + } + + issueBean := &models.Issue{RepoID: 1, Index: 1, ID: 1} + + unittest.AssertNotExistsBean(t, commentBean) + unittest.AssertNotExistsBean(t, issueBean, "is_closed=1") + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) + unittest.AssertExistsAndLoadBean(t, commentBean) + unittest.AssertExistsAndLoadBean(t, issueBean, "is_closed=1") + unittest.CheckConsistencyFor(t, &models.Action{}) +} + +func TestUpdateIssuesCommit_AnotherRepoNoPermission(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 10}).(*models.User) + + // Test that a push with close reference *can not* close issue + // If the committer doesn't have push rights in that repo + pushCommits := []*repository.PushCommit{ + { + Sha1: "abcdef3", + CommitterEmail: "user10@example.com", + CommitterName: "User Ten", + AuthorEmail: "user10@example.com", + AuthorName: "User Ten", + Message: "close user3/repo3#1", + }, + { + Sha1: "abcdef4", + CommitterEmail: "user10@example.com", + CommitterName: "User Ten", + AuthorEmail: "user10@example.com", + AuthorName: "User Ten", + Message: "close " + setting.AppURL + "user3/repo3/issues/1", + }, + } + + repo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 6}).(*models.Repository) + commentBean := &models.Comment{ + Type: models.CommentTypeCommitRef, + CommitSHA: "abcdef3", + PosterID: user.ID, + IssueID: 6, + } + commentBean2 := &models.Comment{ + Type: models.CommentTypeCommitRef, + CommitSHA: "abcdef4", + PosterID: user.ID, + IssueID: 6, + } + + issueBean := &models.Issue{RepoID: 3, Index: 1, ID: 6} + + unittest.AssertNotExistsBean(t, commentBean) + unittest.AssertNotExistsBean(t, commentBean2) + unittest.AssertNotExistsBean(t, issueBean, "is_closed=1") + assert.NoError(t, UpdateIssuesCommit(user, repo, pushCommits, repo.DefaultBranch)) + unittest.AssertNotExistsBean(t, commentBean) + unittest.AssertNotExistsBean(t, commentBean2) + unittest.AssertNotExistsBean(t, issueBean, "is_closed=1") + unittest.CheckConsistencyFor(t, &models.Action{}) +} diff --git a/services/issue/status.go b/services/issue/status.go index b01ce4bbb8..0a18169a27 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 a98cfb1758..97554c6490 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) } -- cgit v1.2.3