From 1649d46908c23a6798573f2837f78e555ae32445 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 1 Oct 2024 19:55:48 -0700 Subject: [PATCH 1/3] Move and merge some functions about issue --- models/issues/comment.go | 23 +----- models/issues/issue.go | 2 - models/issues/issue_test.go | 36 +++++++- models/issues/issue_update.go | 118 ++------------------------- models/issues/issue_xref.go | 10 +-- models/issues/issue_xref_test.go | 49 +++++++++-- models/issues/pull.go | 101 ----------------------- routers/api/v1/repo/issue.go | 2 +- routers/api/v1/repo/pull.go | 2 +- routers/private/hook_post_receive.go | 2 +- routers/web/repo/issue.go | 2 +- services/issue/comments.go | 9 +- services/issue/content.go | 35 +++++++- services/issue/issue.go | 41 +++++++++- services/pull/check.go | 2 +- services/pull/merge.go | 61 +++++++++++++- services/pull/pull.go | 27 +++++- 17 files changed, 261 insertions(+), 261 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index 48b8e335d48ef..f8f7029d15aa1 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -1136,33 +1136,16 @@ func UpdateCommentInvalidate(ctx context.Context, c *Comment) error { } // UpdateComment updates information of comment. -func UpdateComment(ctx context.Context, c *Comment, contentVersion int, doer *user_model.User) error { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - sess := db.GetEngine(ctx) - +func UpdateComment(ctx context.Context, c *Comment, contentVersion int) error { c.ContentVersion = contentVersion + 1 - affected, err := sess.ID(c.ID).AllCols().Where("content_version = ?", contentVersion).Update(c) + affected, err := db.GetEngine(ctx).ID(c.ID).AllCols().Where("content_version = ?", contentVersion).Update(c) if err != nil { return err } if affected == 0 { return ErrCommentAlreadyChanged } - if err := c.LoadIssue(ctx); err != nil { - return err - } - if err := c.AddCrossReferences(ctx, doer, true); err != nil { - return err - } - if err := committer.Commit(); err != nil { - return fmt.Errorf("Commit: %w", err) - } - return nil } @@ -1192,7 +1175,7 @@ func DeleteComment(ctx context.Context, comment *Comment) error { return err } - if err := comment.neuterCrossReferences(ctx); err != nil { + if err := neuterCrossReferences(ctx, comment.IssueID, comment.ID); err != nil { return err } diff --git a/models/issues/issue.go b/models/issues/issue.go index 40462ed09dfd6..fd873166983b6 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -94,8 +94,6 @@ func (err ErrIssueWasClosed) Error() string { return fmt.Sprintf("Issue [%d] %d was already closed", err.ID, err.Index) } -var ErrIssueAlreadyChanged = util.NewInvalidArgumentErrorf("the issue is already changed") - // Issue represents an issue or pull request of repository. type Issue struct { ID int64 `xorm:"pk autoincr"` diff --git a/models/issues/issue_test.go b/models/issues/issue_test.go index 1bbc0eee564fd..dc25aec7c6b2c 100644 --- a/models/issues/issue_test.go +++ b/models/issues/issue_test.go @@ -216,6 +216,40 @@ func TestIssue_loadTotalTimes(t *testing.T) { assert.Equal(t, int64(3682), ms.TotalTrackedTime) } +// createIssue creates new issue with labels for repository. +func createIssue(ctx context.Context, repo *repo_model.Repository, issue *issues_model.Issue, labelIDs []int64, uuids []string) (err error) { + ctx, committer, err := db.TxContext(ctx) + if err != nil { + return err + } + defer committer.Close() + + idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID) + if err != nil { + return fmt.Errorf("generate issue index failed: %w", err) + } + + issue.Index = idx + + if err = issues_model.NewIssueWithIndex(ctx, issue.Poster, issues_model.NewIssueOptions{ + Repo: repo, + Issue: issue, + LabelIDs: labelIDs, + Attachments: uuids, + }); err != nil { + if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) || issues_model.IsErrNewIssueInsert(err) { + return err + } + return fmt.Errorf("newIssue: %w", err) + } + + if err = committer.Commit(); err != nil { + return fmt.Errorf("Commit: %w", err) + } + + return nil +} + func testInsertIssue(t *testing.T, title, content string, expectIndex int64) *issues_model.Issue { var newIssue issues_model.Issue t.Run(title, func(t *testing.T) { @@ -229,7 +263,7 @@ func testInsertIssue(t *testing.T, title, content string, expectIndex int64) *is Title: title, Content: content, } - err := issues_model.NewIssue(db.DefaultContext, repo, &issue, nil, nil) + err := createIssue(db.DefaultContext, repo, &issue, nil, nil) assert.NoError(t, err) has, err := db.GetEngine(db.DefaultContext).ID(issue.ID).Get(&newIssue) diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index 31d76be5e0aea..f8054014163b4 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -33,7 +33,7 @@ func UpdateIssueCols(ctx context.Context, issue *Issue, cols ...string) error { return nil } -func changeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed, isMergePull bool) (*Comment, error) { +func ChangeIssuePullStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed, isMergePull bool) (*Comment, error) { // Reload the issue currentIssue, err := GetIssueByID(ctx, issue.ID) if err != nil { @@ -127,41 +127,7 @@ func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, return nil, err } - return changeIssueStatus(ctx, issue, doer, isClosed, false) -} - -// ChangeIssueTitle changes the title of this issue, as the given user. -func ChangeIssueTitle(ctx context.Context, issue *Issue, doer *user_model.User, oldTitle string) (err error) { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - - if err = UpdateIssueCols(ctx, issue, "name"); err != nil { - return fmt.Errorf("updateIssueCols: %w", err) - } - - if err = issue.LoadRepo(ctx); err != nil { - return fmt.Errorf("loadRepo: %w", err) - } - - opts := &CreateCommentOptions{ - Type: CommentTypeChangeTitle, - Doer: doer, - Repo: issue.Repo, - Issue: issue, - OldTitle: oldTitle, - NewTitle: issue.Title, - } - if _, err = CreateComment(ctx, opts); err != nil { - return fmt.Errorf("createComment: %w", err) - } - if err = issue.AddCrossReferences(ctx, doer, true); err != nil { - return err - } - - return committer.Commit() + return ChangeIssuePullStatus(ctx, issue, doer, isClosed, false) } // ChangeIssueRef changes the branch of this issue, as the given user. @@ -234,48 +200,6 @@ func UpdateIssueAttachments(ctx context.Context, issueID int64, uuids []string) return committer.Commit() } -// ChangeIssueContent changes issue content, as the given user. -func ChangeIssueContent(ctx context.Context, issue *Issue, doer *user_model.User, content string, contentVersion int) (err error) { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - - hasContentHistory, err := HasIssueContentHistory(ctx, issue.ID, 0) - if err != nil { - return fmt.Errorf("HasIssueContentHistory: %w", err) - } - if !hasContentHistory { - if err = SaveIssueContentHistory(ctx, issue.PosterID, issue.ID, 0, - issue.CreatedUnix, issue.Content, true); err != nil { - return fmt.Errorf("SaveIssueContentHistory: %w", err) - } - } - - issue.Content = content - issue.ContentVersion = contentVersion + 1 - - affected, err := db.GetEngine(ctx).ID(issue.ID).Cols("content", "content_version").Where("content_version = ?", contentVersion).Update(issue) - if err != nil { - return err - } - if affected == 0 { - return ErrIssueAlreadyChanged - } - - if err = SaveIssueContentHistory(ctx, doer.ID, issue.ID, 0, - timeutil.TimeStampNow(), issue.Content, false); err != nil { - return fmt.Errorf("SaveIssueContentHistory: %w", err) - } - - if err = issue.AddCrossReferences(ctx, doer, true); err != nil { - return fmt.Errorf("addCrossReferences: %w", err) - } - - return committer.Commit() -} - // NewIssueOptions represents the options of a new issue. type NewIssueOptions struct { Repo *repo_model.Repository @@ -365,6 +289,10 @@ func NewIssueWithIndex(ctx context.Context, doer *user_model.User, opts NewIssue return err } + if err := UpdateIssueAttachments(ctx, opts.Issue.ID, opts.Attachments); err != nil { + return err + } + if len(opts.Attachments) > 0 { attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, opts.Attachments) if err != nil { @@ -385,40 +313,6 @@ func NewIssueWithIndex(ctx context.Context, doer *user_model.User, opts NewIssue return opts.Issue.AddCrossReferences(ctx, doer, false) } -// NewIssue creates new issue with labels for repository. -func NewIssue(ctx context.Context, repo *repo_model.Repository, issue *Issue, labelIDs []int64, uuids []string) (err error) { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - - idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID) - if err != nil { - return fmt.Errorf("generate issue index failed: %w", err) - } - - issue.Index = idx - - if err = NewIssueWithIndex(ctx, issue.Poster, NewIssueOptions{ - Repo: repo, - Issue: issue, - LabelIDs: labelIDs, - Attachments: uuids, - }); err != nil { - if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) || IsErrNewIssueInsert(err) { - return err - } - return fmt.Errorf("newIssue: %w", err) - } - - if err = committer.Commit(); err != nil { - return fmt.Errorf("Commit: %w", err) - } - - return nil -} - // UpdateIssueMentions updates issue-user relations for mentioned users. func UpdateIssueMentions(ctx context.Context, issueID int64, mentions []*user_model.User) error { if len(mentions) == 0 { diff --git a/models/issues/issue_xref.go b/models/issues/issue_xref.go index e2e35859df149..448e6de1480f0 100644 --- a/models/issues/issue_xref.go +++ b/models/issues/issue_xref.go @@ -68,10 +68,10 @@ func (issue *Issue) AddCrossReferences(stdCtx context.Context, doer *user_model. OrigIssue: issue, RemoveOld: removeOld, } - return issue.createCrossReferences(stdCtx, ctx, issue.Title, issue.Content) + return createCrossReferences(stdCtx, ctx, issue.Title, issue.Content) } -func (issue *Issue) createCrossReferences(stdCtx context.Context, ctx *crossReferencesContext, plaincontent, mdcontent string) error { +func createCrossReferences(stdCtx context.Context, ctx *crossReferencesContext, plaincontent, mdcontent string) error { xreflist, err := ctx.OrigIssue.getCrossReferences(stdCtx, ctx, plaincontent, mdcontent) if err != nil { return err @@ -248,11 +248,7 @@ func (c *Comment) AddCrossReferences(stdCtx context.Context, doer *user_model.Us OrigComment: c, RemoveOld: removeOld, } - return c.Issue.createCrossReferences(stdCtx, ctx, "", c.Content) -} - -func (c *Comment) neuterCrossReferences(ctx context.Context) error { - return neuterCrossReferences(ctx, c.IssueID, c.ID) + return createCrossReferences(stdCtx, ctx, "", c.Content) } // LoadRefComment loads comment that created this reference from database diff --git a/models/issues/issue_xref_test.go b/models/issues/issue_xref_test.go index f1b1bb2a6b1b3..26ebf90de1c79 100644 --- a/models/issues/issue_xref_test.go +++ b/models/issues/issue_xref_test.go @@ -4,6 +4,7 @@ package issues_test import ( + "context" "fmt" "testing" @@ -81,15 +82,53 @@ func TestXRef_NeuterCrossReferences(t *testing.T) { assert.Equal(t, issues_model.CommentTypeIssueRef, ref.Type) assert.Equal(t, references.XRefActionNone, ref.RefAction) - d := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - i.Title = "title2, no mentions" - assert.NoError(t, issues_model.ChangeIssueTitle(db.DefaultContext, i, d, title)) - ref = unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{IssueID: itarget.ID, RefIssueID: i.ID, RefCommentID: 0}) assert.Equal(t, issues_model.CommentTypeIssueRef, ref.Type) assert.Equal(t, references.XRefActionNeutered, ref.RefAction) } +// newPullRequest creates new pull request with labels for repository. +func newPullRequest(ctx context.Context, repo *repo_model.Repository, issue *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest) (err error) { + ctx, committer, err := db.TxContext(ctx) + if err != nil { + return err + } + defer committer.Close() + + idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID) + if err != nil { + return fmt.Errorf("generate pull request index failed: %w", err) + } + + issue.Index = idx + + if err = issues_model.NewIssueWithIndex(ctx, issue.Poster, issues_model.NewIssueOptions{ + Repo: repo, + Issue: issue, + LabelIDs: labelIDs, + Attachments: uuids, + IsPull: true, + }); err != nil { + if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) || issues_model.IsErrNewIssueInsert(err) { + return err + } + return fmt.Errorf("newIssue: %w", err) + } + + pr.Index = issue.Index + pr.BaseRepo = repo + pr.IssueID = issue.ID + if err = db.Insert(ctx, pr); err != nil { + return fmt.Errorf("insert pull repo: %w", err) + } + + if err = committer.Commit(); err != nil { + return fmt.Errorf("Commit: %w", err) + } + + return nil +} + func TestXRef_ResolveCrossReferences(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) @@ -163,7 +202,7 @@ func testCreatePR(t *testing.T, repo, doer int64, title, content string) *issues d := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: doer}) i := &issues_model.Issue{RepoID: r.ID, PosterID: d.ID, Poster: d, Title: title, Content: content, IsPull: true} pr := &issues_model.PullRequest{HeadRepoID: repo, BaseRepoID: repo, HeadBranch: "head", BaseBranch: "base", Status: issues_model.PullRequestStatusMergeable} - assert.NoError(t, issues_model.NewPullRequest(db.DefaultContext, r, i, nil, nil, pr)) + assert.NoError(t, newPullRequest(db.DefaultContext, r, i, nil, nil, pr)) pr.Issue = i return pr } diff --git a/models/issues/pull.go b/models/issues/pull.go index b327ebc625c73..94e48248f9b35 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -499,107 +499,6 @@ func (pr *PullRequest) IsFromFork() bool { return pr.HeadRepoID != pr.BaseRepoID } -// SetMerged sets a pull request to merged and closes the corresponding issue -func (pr *PullRequest) SetMerged(ctx context.Context) (bool, error) { - if pr.HasMerged { - return false, fmt.Errorf("PullRequest[%d] already merged", pr.Index) - } - if pr.MergedCommitID == "" || pr.MergedUnix == 0 || pr.Merger == nil { - return false, fmt.Errorf("Unable to merge PullRequest[%d], some required fields are empty", pr.Index) - } - - pr.HasMerged = true - sess := db.GetEngine(ctx) - - if _, err := sess.Exec("UPDATE `issue` SET `repo_id` = `repo_id` WHERE `id` = ?", pr.IssueID); err != nil { - return false, err - } - - if _, err := sess.Exec("UPDATE `pull_request` SET `issue_id` = `issue_id` WHERE `id` = ?", pr.ID); err != nil { - return false, err - } - - pr.Issue = nil - if err := pr.LoadIssue(ctx); err != nil { - return false, err - } - - if tmpPr, err := GetPullRequestByID(ctx, pr.ID); err != nil { - return false, err - } else if tmpPr.HasMerged { - if pr.Issue.IsClosed { - return false, nil - } - return false, fmt.Errorf("PullRequest[%d] already merged but it's associated issue [%d] is not closed", pr.Index, pr.IssueID) - } else if pr.Issue.IsClosed { - return false, fmt.Errorf("PullRequest[%d] already closed", pr.Index) - } - - if err := pr.Issue.LoadRepo(ctx); err != nil { - return false, err - } - - if err := pr.Issue.Repo.LoadOwner(ctx); err != nil { - return false, err - } - - if _, err := changeIssueStatus(ctx, pr.Issue, pr.Merger, true, true); err != nil { - return false, fmt.Errorf("Issue.changeStatus: %w", err) - } - - // reset the conflicted files as there cannot be any if we're merged - pr.ConflictedFiles = []string{} - - // We need to save all of the data used to compute this merge as it may have already been changed by TestPatch. FIXME: need to set some state to prevent TestPatch from running whilst we are merging. - if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix, conflicted_files").Update(pr); err != nil { - return false, fmt.Errorf("Failed to update pr[%d]: %w", pr.ID, err) - } - - return true, nil -} - -// NewPullRequest creates new pull request with labels for repository. -func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - - idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID) - if err != nil { - return fmt.Errorf("generate pull request index failed: %w", err) - } - - issue.Index = idx - - if err = NewIssueWithIndex(ctx, issue.Poster, NewIssueOptions{ - Repo: repo, - Issue: issue, - LabelIDs: labelIDs, - Attachments: uuids, - IsPull: true, - }); err != nil { - if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) || IsErrNewIssueInsert(err) { - return err - } - return fmt.Errorf("newIssue: %w", err) - } - - pr.Index = issue.Index - pr.BaseRepo = repo - pr.IssueID = issue.ID - if err = db.Insert(ctx, pr); err != nil { - return fmt.Errorf("insert pull repo: %w", err) - } - - if err = committer.Commit(); err != nil { - return fmt.Errorf("Commit: %w", err) - } - - return nil -} - // ErrUserMustCollaborator represents an error that the user must be a collaborator to a given repo. type ErrUserMustCollaborator struct { UserID int64 diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index c1218440e5958..7846664f1c650 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -812,7 +812,7 @@ func EditIssue(ctx *context.APIContext) { if form.Body != nil { err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body, issue.ContentVersion) if err != nil { - if errors.Is(err, issues_model.ErrIssueAlreadyChanged) { + if errors.Is(err, issue_service.ErrIssueAlreadyChanged) { ctx.Error(http.StatusBadRequest, "ChangeContent", err) return } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 7eb4a8b8a2d4d..baf0f173a5b8d 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -630,7 +630,7 @@ func EditPullRequest(ctx *context.APIContext) { if form.Body != nil { err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body, issue.ContentVersion) if err != nil { - if errors.Is(err, issues_model.ErrIssueAlreadyChanged) { + if errors.Is(err, issue_service.ErrIssueAlreadyChanged) { ctx.Error(http.StatusBadRequest, "ChangeContent", err) return } diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 5c01216356dba..d6c3cabae4e40 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -368,7 +368,7 @@ func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.H if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { return fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err) } - if _, err := pr.SetMerged(ctx); err != nil { + if _, err := pull_service.SetMerged(ctx, pr); err != nil { return fmt.Errorf("SetMerged failed: %s/%s Error: %v", ownerName, repoName, err) } return nil diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 507b5af9d904a..98b3111d656f7 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -2286,7 +2286,7 @@ func UpdateIssueContent(ctx *context.Context) { if err := issue_service.ChangeContent(ctx, issue, ctx.Doer, ctx.Req.FormValue("content"), ctx.FormInt("content_version")); err != nil { if errors.Is(err, user_model.ErrBlockedUser) { ctx.JSONError(ctx.Tr("repo.issues.edit.blocked_user")) - } else if errors.Is(err, issues_model.ErrIssueAlreadyChanged) { + } else if errors.Is(err, issue_service.ErrIssueAlreadyChanged) { if issue.IsPull { ctx.JSONError(ctx.Tr("repo.pulls.edit.already_changed")) } else { diff --git a/services/issue/comments.go b/services/issue/comments.go index 33b5702a00c4d..027d2f961a666 100644 --- a/services/issue/comments.go +++ b/services/issue/comments.go @@ -110,7 +110,14 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, contentVersion } } - if err := issues_model.UpdateComment(ctx, c, contentVersion, doer); err != nil { + if err := issues_model.UpdateComment(ctx, c, contentVersion); err != nil { + return err + } + + if err := c.LoadIssue(ctx); err != nil { + return err + } + if err := c.AddCrossReferences(ctx, doer, true); err != nil { return err } diff --git a/services/issue/content.go b/services/issue/content.go index 6894182909275..e885c68d8c70a 100644 --- a/services/issue/content.go +++ b/services/issue/content.go @@ -5,13 +5,19 @@ package issue import ( "context" + "fmt" + "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/timeutil" + "code.gitea.io/gitea/modules/util" notify_service "code.gitea.io/gitea/services/notify" ) +var ErrIssueAlreadyChanged = util.NewInvalidArgumentErrorf("the issue is already changed") + // ChangeContent changes issue content, as the given user. func ChangeContent(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, content string, contentVersion int) error { if err := issue.LoadRepo(ctx); err != nil { @@ -26,9 +32,36 @@ func ChangeContent(ctx context.Context, issue *issues_model.Issue, doer *user_mo oldContent := issue.Content - if err := issues_model.ChangeIssueContent(ctx, issue, doer, content, contentVersion); err != nil { + hasContentHistory, err := issues_model.HasIssueContentHistory(ctx, issue.ID, 0) + if err != nil { + return fmt.Errorf("HasIssueContentHistory: %w", err) + } + if !hasContentHistory { + if err = issues_model.SaveIssueContentHistory(ctx, issue.PosterID, issue.ID, 0, + issue.CreatedUnix, issue.Content, true); err != nil { + return fmt.Errorf("SaveIssueContentHistory: %w", err) + } + } + + issue.Content = content + issue.ContentVersion = contentVersion + 1 + + affected, err := db.GetEngine(ctx).ID(issue.ID).Cols("content", "content_version").Where("content_version = ?", contentVersion).Update(issue) + if err != nil { return err } + if affected == 0 { + return ErrIssueAlreadyChanged + } + + if err = issues_model.SaveIssueContentHistory(ctx, doer.ID, issue.ID, 0, + timeutil.TimeStampNow(), issue.Content, false); err != nil { + return fmt.Errorf("SaveIssueContentHistory: %w", err) + } + + if err = issue.AddCrossReferences(ctx, doer, true); err != nil { + return fmt.Errorf("addCrossReferences: %w", err) + } notify_service.IssueChangeContent(ctx, doer, issue, oldContent) diff --git a/services/issue/issue.go b/services/issue/issue.go index 72ea66c8d98c5..c45a55dae99bb 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -33,9 +33,25 @@ func NewIssue(ctx context.Context, repo *repo_model.Repository, issue *issues_mo } if err := db.WithTx(ctx, func(ctx context.Context) error { - if err := issues_model.NewIssue(ctx, repo, issue, labelIDs, uuids); err != nil { - return err + idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID) + if err != nil { + return fmt.Errorf("generate issue index failed: %w", err) + } + + issue.Index = idx + + if err = issues_model.NewIssueWithIndex(ctx, issue.Poster, issues_model.NewIssueOptions{ + Repo: repo, + Issue: issue, + LabelIDs: labelIDs, + Attachments: uuids, + }); err != nil { + if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) || issues_model.IsErrNewIssueInsert(err) { + return err + } + return fmt.Errorf("newIssue: %w", err) } + for _, assigneeID := range assigneeIDs { if _, err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID, true); err != nil { return err @@ -86,7 +102,26 @@ func ChangeTitle(ctx context.Context, issue *issues_model.Issue, doer *user_mode } } - if err := issues_model.ChangeIssueTitle(ctx, issue, doer, oldTitle); err != nil { + if err := issues_model.UpdateIssueCols(ctx, issue, "name"); err != nil { + return fmt.Errorf("updateIssueCols: %w", err) + } + + if err := issue.LoadRepo(ctx); err != nil { + return fmt.Errorf("loadRepo: %w", err) + } + + opts := &issues_model.CreateCommentOptions{ + Type: issues_model.CommentTypeChangeTitle, + Doer: doer, + Repo: issue.Repo, + Issue: issue, + OldTitle: oldTitle, + NewTitle: issue.Title, + } + if _, err := issues_model.CreateComment(ctx, opts); err != nil { + return fmt.Errorf("createComment: %w", err) + } + if err := issue.AddCrossReferences(ctx, doer, true); err != nil { return err } diff --git a/services/pull/check.go b/services/pull/check.go index ce212f7d83bed..a3c18a6a6e08f 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -292,7 +292,7 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { pr.Merger = merger pr.MergerID = merger.ID - if merged, err := pr.SetMerged(ctx); err != nil { + if merged, err := SetMerged(ctx, pr); err != nil { log.Error("%-v setMerged : %v", pr, err) return false } else if !merged { diff --git a/services/pull/merge.go b/services/pull/merge.go index a3fbe4f627b00..9ac2353475b0e 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -490,6 +490,65 @@ func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullReques return nil } +// SetMerged sets a pull request to merged and closes the corresponding issue +func SetMerged(ctx context.Context, pr *issues_model.PullRequest) (bool, error) { + if pr.HasMerged { + return false, fmt.Errorf("PullRequest[%d] already merged", pr.Index) + } + if pr.MergedCommitID == "" || pr.MergedUnix == 0 || pr.Merger == nil { + return false, fmt.Errorf("Unable to merge PullRequest[%d], some required fields are empty", pr.Index) + } + + pr.HasMerged = true + sess := db.GetEngine(ctx) + + if _, err := sess.Exec("UPDATE `issue` SET `repo_id` = `repo_id` WHERE `id` = ?", pr.IssueID); err != nil { + return false, err + } + + if _, err := sess.Exec("UPDATE `pull_request` SET `issue_id` = `issue_id` WHERE `id` = ?", pr.ID); err != nil { + return false, err + } + + pr.Issue = nil + if err := pr.LoadIssue(ctx); err != nil { + return false, err + } + + if tmpPr, err := issues_model.GetPullRequestByID(ctx, pr.ID); err != nil { + return false, err + } else if tmpPr.HasMerged { + if pr.Issue.IsClosed { + return false, nil + } + return false, fmt.Errorf("PullRequest[%d] already merged but it's associated issue [%d] is not closed", pr.Index, pr.IssueID) + } else if pr.Issue.IsClosed { + return false, fmt.Errorf("PullRequest[%d] already closed", pr.Index) + } + + if err := pr.Issue.LoadRepo(ctx); err != nil { + return false, err + } + + if err := pr.Issue.Repo.LoadOwner(ctx); err != nil { + return false, err + } + + if _, err := issues_model.ChangeIssuePullStatus(ctx, pr.Issue, pr.Merger, true, true); err != nil { + return false, fmt.Errorf("Issue.changeStatus: %w", err) + } + + // reset the conflicted files as there cannot be any if we're merged + pr.ConflictedFiles = []string{} + + // We need to save all of the data used to compute this merge as it may have already been changed by TestPatch. FIXME: need to set some state to prevent TestPatch from running whilst we are merging. + if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix, conflicted_files").Update(pr); err != nil { + return false, fmt.Errorf("Failed to update pr[%d]: %w", pr.ID, err) + } + + return true, nil +} + // MergedManually mark pr as merged manually func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, baseGitRepo *git.Repository, commitID string) error { releaser, err := globallock.Lock(ctx, getPullWorkingLockKey(pr.ID)) @@ -543,7 +602,7 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use pr.MergerID = doer.ID var merged bool - if merged, err = pr.SetMerged(ctx); err != nil { + if merged, err = SetMerged(ctx, pr); err != nil { return err } else if !merged { return fmt.Errorf("SetMerged failed") diff --git a/services/pull/pull.go b/services/pull/pull.go index bab4e49998e15..ac6a43621c4fe 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -104,8 +104,31 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss var reviewNotifiers []*issue_service.ReviewRequestNotifier if err := db.WithTx(ctx, func(ctx context.Context) error { - if err := issues_model.NewPullRequest(ctx, repo, issue, labelIDs, uuids, pr); err != nil { - return err + idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID) + if err != nil { + return fmt.Errorf("generate pull request index failed: %w", err) + } + + issue.Index = idx + + if err = issues_model.NewIssueWithIndex(ctx, issue.Poster, issues_model.NewIssueOptions{ + Repo: repo, + Issue: issue, + LabelIDs: labelIDs, + Attachments: uuids, + IsPull: true, + }); err != nil { + if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) || issues_model.IsErrNewIssueInsert(err) { + return err + } + return fmt.Errorf("newIssue: %w", err) + } + + pr.Index = issue.Index + pr.BaseRepo = repo + pr.IssueID = issue.ID + if err = db.Insert(ctx, pr); err != nil { + return fmt.Errorf("insert pull repo: %w", err) } for _, assigneeID := range assigneeIDs { From cf6500a5bdb925355744c05a34f30a98a3d2adb2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 5 Oct 2024 13:22:08 -0700 Subject: [PATCH 2/3] Fix test --- models/issues/issue_xref_test.go | 38 ++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/models/issues/issue_xref_test.go b/models/issues/issue_xref_test.go index 26ebf90de1c79..33a0293578dcb 100644 --- a/models/issues/issue_xref_test.go +++ b/models/issues/issue_xref_test.go @@ -69,6 +69,40 @@ func TestXRef_AddCrossReferences(t *testing.T) { unittest.AssertNotExistsBean(t, &issues_model.Comment{IssueID: itarget.ID, RefIssueID: i.ID, RefCommentID: 0}) } +// changeIssueTitle changes the title of this issue, as the given user. +func changeIssueTitle(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, oldTitle string) (err error) { + ctx, committer, err := db.TxContext(ctx) + if err != nil { + return err + } + defer committer.Close() + + if err = issues_model.UpdateIssueCols(ctx, issue, "name"); err != nil { + return fmt.Errorf("updateIssueCols: %w", err) + } + + if err = issue.LoadRepo(ctx); err != nil { + return fmt.Errorf("loadRepo: %w", err) + } + + opts := &issues_model.CreateCommentOptions{ + Type: issues_model.CommentTypeChangeTitle, + Doer: doer, + Repo: issue.Repo, + Issue: issue, + OldTitle: oldTitle, + NewTitle: issue.Title, + } + if _, err = issues_model.CreateComment(ctx, opts); err != nil { + return fmt.Errorf("createComment: %w", err) + } + if err = issue.AddCrossReferences(ctx, doer, true); err != nil { + return err + } + + return committer.Commit() +} + func TestXRef_NeuterCrossReferences(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) @@ -82,6 +116,10 @@ func TestXRef_NeuterCrossReferences(t *testing.T) { assert.Equal(t, issues_model.CommentTypeIssueRef, ref.Type) assert.Equal(t, references.XRefActionNone, ref.RefAction) + d := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + i.Title = "title2, no mentions" + assert.NoError(t, changeIssueTitle(db.DefaultContext, i, d, title)) + ref = unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{IssueID: itarget.ID, RefIssueID: i.ID, RefCommentID: 0}) assert.Equal(t, issues_model.CommentTypeIssueRef, ref.Type) assert.Equal(t, references.XRefActionNeutered, ref.RefAction) From 3d3d6c4f867b2927201539d3656e7ef1881162a1 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 21 Nov 2024 12:30:36 -0800 Subject: [PATCH 3/3] Fix lint --- models/issues/comment.go | 56 +++++++++++++---------------------- models/issues/issue_update.go | 41 +++++++++---------------- routers/web/repo/issue.go | 2 +- services/pull/pull.go | 3 ++ 4 files changed, 38 insertions(+), 64 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index f8f7029d15aa1..ea883cafc1927 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -589,26 +589,27 @@ func (c *Comment) LoadAttachments(ctx context.Context) error { return nil } -// UpdateAttachments update attachments by UUIDs for the comment -func (c *Comment) UpdateAttachments(ctx context.Context, uuids []string) error { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err +// UpdateCommentAttachments update attachments by UUIDs for the comment +func UpdateCommentAttachments(ctx context.Context, c *Comment, uuids []string) error { + if len(uuids) == 0 { + return nil } - defer committer.Close() - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, uuids) - if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err) - } - for i := 0; i < len(attachments); i++ { - attachments[i].IssueID = c.IssueID - attachments[i].CommentID = c.ID - if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil { - return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err) + return db.WithTx(ctx, func(ctx context.Context) error { + attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, uuids) + if err != nil { + return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err) } - } - return committer.Commit() + for i := 0; i < len(attachments); i++ { + attachments[i].IssueID = c.IssueID + attachments[i].CommentID = c.ID + if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil { + return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err) + } + } + c.Attachments = attachments + return nil + }) } // LoadAssigneeUserAndTeam if comment.Type is CommentTypeAssignees, then load assignees @@ -875,7 +876,7 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment // Check comment type. switch opts.Type { case CommentTypeCode: - if err = updateAttachments(ctx, opts, comment); err != nil { + if err = UpdateCommentAttachments(ctx, comment, opts.Attachments); err != nil { return err } if comment.ReviewID != 0 { @@ -895,7 +896,7 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment } fallthrough case CommentTypeReview: - if err = updateAttachments(ctx, opts, comment); err != nil { + if err = UpdateCommentAttachments(ctx, comment, opts.Attachments); err != nil { return err } case CommentTypeReopen, CommentTypeClose: @@ -907,23 +908,6 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment return UpdateIssueCols(ctx, opts.Issue, "updated_unix") } -func updateAttachments(ctx context.Context, opts *CreateCommentOptions, comment *Comment) error { - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, opts.Attachments) - if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", opts.Attachments, err) - } - for i := range attachments { - attachments[i].IssueID = opts.Issue.ID - attachments[i].CommentID = comment.ID - // No assign value could be 0, so ignore AllCols(). - if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Update(attachments[i]); err != nil { - return fmt.Errorf("update attachment [%d]: %w", attachments[i].ID, err) - } - } - comment.Attachments = attachments - return nil -} - func createDeadlineComment(ctx context.Context, doer *user_model.User, issue *Issue, newDeadlineUnix timeutil.TimeStamp) (*Comment, error) { var content string var commentType CommentType diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index f8054014163b4..6d8c7c8d9070a 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -182,22 +182,22 @@ func AddDeletePRBranchComment(ctx context.Context, doer *user_model.User, repo * // UpdateIssueAttachments update attachments by UUIDs for the issue func UpdateIssueAttachments(ctx context.Context, issueID int64, uuids []string) (err error) { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, uuids) - if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err) + if len(uuids) == 0 { + return nil } - for i := 0; i < len(attachments); i++ { - attachments[i].IssueID = issueID - if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil { - return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err) + return db.WithTx(ctx, func(ctx context.Context) error { + attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, uuids) + if err != nil { + return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err) } - } - return committer.Commit() + for i := 0; i < len(attachments); i++ { + attachments[i].IssueID = issueID + if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil { + return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err) + } + } + return nil + }) } // NewIssueOptions represents the options of a new issue. @@ -293,19 +293,6 @@ func NewIssueWithIndex(ctx context.Context, doer *user_model.User, opts NewIssue return err } - if len(opts.Attachments) > 0 { - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, opts.Attachments) - if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", opts.Attachments, err) - } - - for i := 0; i < len(attachments); i++ { - attachments[i].IssueID = opts.Issue.ID - if _, err = e.ID(attachments[i].ID).Update(attachments[i]); err != nil { - return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err) - } - } - } if err = opts.Issue.LoadAttributes(ctx); err != nil { return err } diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 3f9c1b89f8798..d01e168936f3a 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -612,7 +612,7 @@ func updateAttachments(ctx *context.Context, item any, files []string) error { case *issues_model.Issue: err = issues_model.UpdateIssueAttachments(ctx, content.ID, files) case *issues_model.Comment: - err = content.UpdateAttachments(ctx, files) + err = issues_model.UpdateCommentAttachments(ctx, content, files) default: return fmt.Errorf("unknown Type: %T", content) } diff --git a/services/pull/pull.go b/services/pull/pull.go index 5c1cb41538436..29f7bd80a3add 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -235,6 +235,9 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { notify_service.IssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assigneeID]) } permDoer, err := access_model.GetUserRepoPermission(ctx, repo, issue.Poster) + if err != nil { + return err + } for _, reviewer := range opts.Reviewers { if _, err = issue_service.ReviewRequest(ctx, pr.Issue, issue.Poster, &permDoer, reviewer, true); err != nil { return err