Skip to content

Commit 36eb3c4

Browse files
lunnyGiteaBot
lunny
and
GiteaBot
authored
Add transaction when creating pull request created dirty data (#26259)
Fix #26129 Replace #26258 This PR will introduce a transaction on creating pull request so that if some step failed, it will rollback totally. And there will be no dirty pull request exist. --------- Co-authored-by: Giteabot <[email protected]>
1 parent a85a862 commit 36eb3c4

File tree

8 files changed

+121
-90
lines changed

8 files changed

+121
-90
lines changed

‎models/issues/pull.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -533,13 +533,12 @@ func (pr *PullRequest) SetMerged(ctx context.Context) (bool, error) {
533533
}
534534

535535
// NewPullRequest creates new pull request with labels for repository.
536-
func NewPullRequest(outerCtx context.Context, repo *repo_model.Repository, issue *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) {
537-
ctx, committer, err := db.TxContext(outerCtx)
536+
func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) {
537+
ctx, committer, err := db.TxContext(ctx)
538538
if err != nil {
539539
return err
540540
}
541541
defer committer.Close()
542-
ctx.WithContext(outerCtx)
543542

544543
idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID)
545544
if err != nil {
@@ -948,14 +947,14 @@ func PullRequestCodeOwnersReview(ctx context.Context, pull *Issue, pr *PullReque
948947

949948
for _, u := range uniqUsers {
950949
if u.ID != pull.Poster.ID {
951-
if _, err := AddReviewRequest(pull, u, pull.Poster); err != nil {
950+
if _, err := AddReviewRequest(ctx, pull, u, pull.Poster); err != nil {
952951
log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err)
953952
return err
954953
}
955954
}
956955
}
957956
for _, t := range uniqTeams {
958-
if _, err := AddTeamReviewRequest(pull, t, pull.Poster); err != nil {
957+
if _, err := AddTeamReviewRequest(ctx, pull, t, pull.Poster); err != nil {
959958
log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err)
960959
return err
961960
}

‎models/issues/pull_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,14 +88,14 @@ func TestLoadRequestedReviewers(t *testing.T) {
8888
user1, err := user_model.GetUserByID(db.DefaultContext, 1)
8989
assert.NoError(t, err)
9090

91-
comment, err := issues_model.AddReviewRequest(issue, user1, &user_model.User{})
91+
comment, err := issues_model.AddReviewRequest(db.DefaultContext, issue, user1, &user_model.User{})
9292
assert.NoError(t, err)
9393
assert.NotNil(t, comment)
9494

9595
assert.NoError(t, pull.LoadRequestedReviewers(db.DefaultContext))
9696
assert.Len(t, pull.RequestedReviewers, 1)
9797

98-
comment, err = issues_model.RemoveReviewRequest(issue, user1, &user_model.User{})
98+
comment, err = issues_model.RemoveReviewRequest(db.DefaultContext, issue, user1, &user_model.User{})
9999
assert.NoError(t, err)
100100
assert.NotNil(t, comment)
101101

‎models/issues/review.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -560,8 +560,8 @@ func InsertReviews(reviews []*Review) error {
560560
}
561561

562562
// AddReviewRequest add a review request from one reviewer
563-
func AddReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment, error) {
564-
ctx, committer, err := db.TxContext(db.DefaultContext)
563+
func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) {
564+
ctx, committer, err := db.TxContext(ctx)
565565
if err != nil {
566566
return nil, err
567567
}
@@ -615,8 +615,8 @@ func AddReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment,
615615
}
616616

617617
// RemoveReviewRequest remove a review request from one reviewer
618-
func RemoveReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment, error) {
619-
ctx, committer, err := db.TxContext(db.DefaultContext)
618+
func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) {
619+
ctx, committer, err := db.TxContext(ctx)
620620
if err != nil {
621621
return nil, err
622622
}
@@ -676,8 +676,8 @@ func restoreLatestOfficialReview(ctx context.Context, issueID, reviewerID int64)
676676
}
677677

678678
// AddTeamReviewRequest add a review request from one team
679-
func AddTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) {
680-
ctx, committer, err := db.TxContext(db.DefaultContext)
679+
func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) {
680+
ctx, committer, err := db.TxContext(ctx)
681681
if err != nil {
682682
return nil, err
683683
}
@@ -735,8 +735,8 @@ func AddTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_
735735
}
736736

737737
// RemoveTeamReviewRequest remove a review request from one team
738-
func RemoveTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) {
739-
ctx, committer, err := db.TxContext(db.DefaultContext)
738+
func RemoveTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) {
739+
ctx, committer, err := db.TxContext(ctx)
740740
if err != nil {
741741
return nil, err
742742
}

‎routers/web/repo/issue.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2290,7 +2290,7 @@ func UpdateIssueAssignee(ctx *context.Context) {
22902290
return
22912291
}
22922292

2293-
_, _, err = issue_service.ToggleAssignee(ctx, issue, ctx.Doer, assigneeID)
2293+
_, _, err = issue_service.ToggleAssigneeWithNotify(ctx, issue, ctx.Doer, assigneeID)
22942294
if err != nil {
22952295
ctx.ServerError("ToggleAssignee", err)
22962296
return

‎services/issue/assignee.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doe
3333

3434
if !found {
3535
// This function also does comments and hooks, which is why we call it separately instead of directly removing the assignees here
36-
if _, _, err := ToggleAssignee(ctx, issue, doer, assignee.ID); err != nil {
36+
if _, _, err := ToggleAssigneeWithNotify(ctx, issue, doer, assignee.ID); err != nil {
3737
return err
3838
}
3939
}
@@ -42,8 +42,8 @@ func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doe
4242
return nil
4343
}
4444

45-
// ToggleAssignee changes a user between assigned and not assigned for this issue, and make issue comment for it.
46-
func ToggleAssignee(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (removed bool, comment *issues_model.Comment, err error) {
45+
// ToggleAssigneeWithNoNotify changes a user between assigned and not assigned for this issue, and make issue comment for it.
46+
func ToggleAssigneeWithNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (removed bool, comment *issues_model.Comment, err error) {
4747
removed, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID)
4848
if err != nil {
4949
return false, nil, err
@@ -62,9 +62,9 @@ func ToggleAssignee(ctx context.Context, issue *issues_model.Issue, doer *user_m
6262
// ReviewRequest add or remove a review request from a user for this PR, and make comment for it.
6363
func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) {
6464
if isAdd {
65-
comment, err = issues_model.AddReviewRequest(issue, reviewer, doer)
65+
comment, err = issues_model.AddReviewRequest(ctx, issue, reviewer, doer)
6666
} else {
67-
comment, err = issues_model.RemoveReviewRequest(issue, reviewer, doer)
67+
comment, err = issues_model.RemoveReviewRequest(ctx, issue, reviewer, doer)
6868
}
6969

7070
if err != nil {
@@ -229,9 +229,9 @@ func IsValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team,
229229
// TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it.
230230
func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *organization.Team, isAdd bool) (comment *issues_model.Comment, err error) {
231231
if isAdd {
232-
comment, err = issues_model.AddTeamReviewRequest(issue, reviewer, doer)
232+
comment, err = issues_model.AddTeamReviewRequest(ctx, issue, reviewer, doer)
233233
} else {
234-
comment, err = issues_model.RemoveTeamReviewRequest(issue, reviewer, doer)
234+
comment, err = issues_model.RemoveTeamReviewRequest(ctx, issue, reviewer, doer)
235235
}
236236

237237
if err != nil {

‎services/issue/issue.go

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ func NewIssue(ctx context.Context, repo *repo_model.Repository, issue *issues_mo
2727
}
2828

2929
for _, assigneeID := range assigneeIDs {
30-
if err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID); err != nil {
30+
if _, err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID, true); err != nil {
3131
return err
3232
}
3333
}
@@ -128,7 +128,7 @@ func UpdateAssignees(ctx context.Context, issue *issues_model.Issue, oneAssignee
128128
// has access to the repo.
129129
for _, assignee := range allNewAssignees {
130130
// Extra method to prevent double adding (which would result in removing)
131-
err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID)
131+
_, err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID, true)
132132
if err != nil {
133133
return err
134134
}
@@ -173,36 +173,36 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi
173173

174174
// AddAssigneeIfNotAssigned adds an assignee only if he isn't already assigned to the issue.
175175
// Also checks for access of assigned user
176-
func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (err error) {
176+
func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64, notify bool) (comment *issues_model.Comment, err error) {
177177
assignee, err := user_model.GetUserByID(ctx, assigneeID)
178178
if err != nil {
179-
return err
179+
return nil, err
180180
}
181181

182182
// Check if the user is already assigned
183183
isAssigned, err := issues_model.IsUserAssignedToIssue(ctx, issue, assignee)
184184
if err != nil {
185-
return err
185+
return nil, err
186186
}
187187
if isAssigned {
188188
// nothing to to
189-
return nil
189+
return nil, nil
190190
}
191191

192192
valid, err := access_model.CanBeAssigned(ctx, assignee, issue.Repo, issue.IsPull)
193193
if err != nil {
194-
return err
194+
return nil, err
195195
}
196196
if !valid {
197-
return repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name}
197+
return nil, repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name}
198198
}
199199

200-
_, _, err = ToggleAssignee(ctx, issue, doer, assigneeID)
201-
if err != nil {
202-
return err
200+
if notify {
201+
_, comment, err = ToggleAssigneeWithNotify(ctx, issue, doer, assigneeID)
202+
return comment, err
203203
}
204-
205-
return nil
204+
_, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID)
205+
return comment, err
206206
}
207207

208208
// GetRefEndNamesAndURLs retrieves the ref end names (e.g. refs/heads/branch-name -> branch-name)

‎services/pull/patch.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,14 +62,19 @@ func TestPatch(pr *issues_model.PullRequest) error {
6262
ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("TestPatch: %s", pr))
6363
defer finished()
6464

65-
// Clone base repo.
6665
prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
6766
if err != nil {
68-
log.Error("createTemporaryRepoForPR %-v: %v", pr, err)
67+
if !git_model.IsErrBranchNotExist(err) {
68+
log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err)
69+
}
6970
return err
7071
}
7172
defer cancel()
7273

74+
return testPatch(ctx, prCtx, pr)
75+
}
76+
77+
func testPatch(ctx context.Context, prCtx *prContext, pr *issues_model.PullRequest) error {
7378
gitRepo, err := git.OpenRepository(ctx, prCtx.tmpBasePath)
7479
if err != nil {
7580
return fmt.Errorf("OpenRepository: %w", err)

0 commit comments

Comments
 (0)