From a50026e2f30897904704895362da0fb12c7e5b26 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Fri, 3 May 2024 15:11:51 +0900 Subject: [PATCH] Fix no edit history after editing issue's title and content (#30814) Fix #30807 reuse functions in services --- models/issues/issue_update.go | 56 ----------------------------- modules/structs/pull.go | 2 +- routers/api/v1/repo/issue.go | 36 +++++++++---------- routers/api/v1/repo/pull.go | 37 +++++++++---------- tests/integration/api_issue_test.go | 4 +++ tests/integration/api_pull_test.go | 26 +++++++++----- 6 files changed, 56 insertions(+), 105 deletions(-) diff --git a/models/issues/issue_update.go b/models/issues/issue_update.go index ef96e1ee50..147b7eb3b9 100644 --- a/models/issues/issue_update.go +++ b/models/issues/issue_update.go @@ -429,62 +429,6 @@ func UpdateIssueMentions(ctx context.Context, issueID int64, mentions []*user_mo return nil } -// UpdateIssueByAPI updates all allowed fields of given issue. -// If the issue status is changed a statusChangeComment is returned -// similarly if the title is changed the titleChanged bool is set to true -func UpdateIssueByAPI(ctx context.Context, issue *Issue, doer *user_model.User) (statusChangeComment *Comment, titleChanged bool, err error) { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return nil, false, err - } - defer committer.Close() - - if err := issue.LoadRepo(ctx); err != nil { - return nil, false, fmt.Errorf("loadRepo: %w", err) - } - - // Reload the issue - currentIssue, err := GetIssueByID(ctx, issue.ID) - if err != nil { - return nil, false, err - } - - if _, err := db.GetEngine(ctx).ID(issue.ID).Cols( - "name", "content", "milestone_id", "priority", - "deadline_unix", "updated_unix", "is_locked"). - Update(issue); err != nil { - return nil, false, err - } - - titleChanged = currentIssue.Title != issue.Title - if titleChanged { - opts := &CreateCommentOptions{ - Type: CommentTypeChangeTitle, - Doer: doer, - Repo: issue.Repo, - Issue: issue, - OldTitle: currentIssue.Title, - NewTitle: issue.Title, - } - _, err := CreateComment(ctx, opts) - if err != nil { - return nil, false, fmt.Errorf("createComment: %w", err) - } - } - - if currentIssue.IsClosed != issue.IsClosed { - statusChangeComment, err = doChangeIssueStatus(ctx, issue, doer, false) - if err != nil { - return nil, false, err - } - } - - if err := issue.AddCrossReferences(ctx, doer, true); err != nil { - return nil, false, err - } - return statusChangeComment, titleChanged, committer.Commit() -} - // UpdateIssueDeadline updates an issue deadline and adds comments. Setting a deadline to 0 means deleting it. func UpdateIssueDeadline(ctx context.Context, issue *Issue, deadlineUnix timeutil.TimeStamp, doer *user_model.User) (err error) { // if the deadline hasn't changed do nothing diff --git a/modules/structs/pull.go b/modules/structs/pull.go index 05a8d59633..b04def52b8 100644 --- a/modules/structs/pull.go +++ b/modules/structs/pull.go @@ -85,7 +85,7 @@ type CreatePullRequestOption struct { // EditPullRequestOption options when modify pull request type EditPullRequestOption struct { Title string `json:"title"` - Body string `json:"body"` + Body *string `json:"body"` Base string `json:"base"` Assignee string `json:"assignee"` Assignees []string `json:"assignees"` diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index dfe6d31f74..b91fbc33bf 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -29,7 +29,6 @@ import ( "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" issue_service "code.gitea.io/gitea/services/issue" - notify_service "code.gitea.io/gitea/services/notify" ) // SearchIssues searches for issues across the repositories that the user has access to @@ -803,12 +802,19 @@ func EditIssue(ctx *context.APIContext) { return } - oldTitle := issue.Title if len(form.Title) > 0 { - issue.Title = form.Title + err = issue_service.ChangeTitle(ctx, issue, ctx.Doer, form.Title) + if err != nil { + ctx.Error(http.StatusInternalServerError, "ChangeTitle", err) + return + } } if form.Body != nil { - issue.Content = *form.Body + err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body) + if err != nil { + ctx.Error(http.StatusInternalServerError, "ChangeContent", err) + return + } } if form.Ref != nil { err = issue_service.ChangeIssueRef(ctx, issue, ctx.Doer, *form.Ref) @@ -880,24 +886,14 @@ func EditIssue(ctx *context.APIContext) { return } } - issue.IsClosed = api.StateClosed == api.StateType(*form.State) - } - statusChangeComment, titleChanged, err := issues_model.UpdateIssueByAPI(ctx, issue, ctx.Doer) - if err != nil { - if issues_model.IsErrDependenciesLeft(err) { - ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies") + if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", api.StateClosed == api.StateType(*form.State)); err != nil { + if issues_model.IsErrDependenciesLeft(err) { + ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this issue because it still has open dependencies") + return + } + ctx.Error(http.StatusInternalServerError, "ChangeStatus", err) return } - ctx.Error(http.StatusInternalServerError, "UpdateIssueByAPI", err) - return - } - - if titleChanged { - notify_service.IssueChangeTitle(ctx, ctx.Doer, issue, oldTitle) - } - - if statusChangeComment != nil { - notify_service.IssueChangeStatus(ctx, ctx.Doer, "", issue, statusChangeComment, issue.IsClosed) } // Refetch from database to assign some automatic values diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 4129f94ac3..8bd4ddf64b 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -602,12 +602,19 @@ func EditPullRequest(ctx *context.APIContext) { return } - oldTitle := issue.Title if len(form.Title) > 0 { - issue.Title = form.Title + err = issue_service.ChangeTitle(ctx, issue, ctx.Doer, form.Title) + if err != nil { + ctx.Error(http.StatusInternalServerError, "ChangeTitle", err) + return + } } - if len(form.Body) > 0 { - issue.Content = form.Body + if form.Body != nil { + err = issue_service.ChangeContent(ctx, issue, ctx.Doer, *form.Body) + if err != nil { + ctx.Error(http.StatusInternalServerError, "ChangeContent", err) + return + } } // Update or remove deadline if set @@ -686,24 +693,14 @@ func EditPullRequest(ctx *context.APIContext) { ctx.Error(http.StatusPreconditionFailed, "MergedPRState", "cannot change state of this pull request, it was already merged") return } - issue.IsClosed = api.StateClosed == api.StateType(*form.State) - } - statusChangeComment, titleChanged, err := issues_model.UpdateIssueByAPI(ctx, issue, ctx.Doer) - if err != nil { - if issues_model.IsErrDependenciesLeft(err) { - ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this pull request because it still has open dependencies") + if err := issue_service.ChangeStatus(ctx, issue, ctx.Doer, "", api.StateClosed == api.StateType(*form.State)); err != nil { + if issues_model.IsErrDependenciesLeft(err) { + ctx.Error(http.StatusPreconditionFailed, "DependenciesLeft", "cannot close this pull request because it still has open dependencies") + return + } + ctx.Error(http.StatusInternalServerError, "ChangeStatus", err) return } - ctx.Error(http.StatusInternalServerError, "UpdateIssueByAPI", err) - return - } - - if titleChanged { - notify_service.IssueChangeTitle(ctx, ctx.Doer, issue, oldTitle) - } - - if statusChangeComment != nil { - notify_service.IssueChangeStatus(ctx, ctx.Doer, "", issue, statusChangeComment, issue.IsClosed) } // change pull target branch diff --git a/tests/integration/api_issue_test.go b/tests/integration/api_issue_test.go index 17b4e5bd71..8bfb6fabe2 100644 --- a/tests/integration/api_issue_test.go +++ b/tests/integration/api_issue_test.go @@ -194,6 +194,10 @@ func TestAPIEditIssue(t *testing.T) { issueAfter := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 10}) repoAfter := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issueBefore.RepoID}) + // check comment history + unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{IssueID: issueAfter.ID, OldTitle: issueBefore.Title, NewTitle: title}) + unittest.AssertExistsAndLoadBean(t, &issues_model.ContentHistory{IssueID: issueAfter.ID, ContentText: body, IsFirstCreated: false}) + // check deleted user assert.Equal(t, int64(500), issueAfter.PosterID) assert.NoError(t, issueAfter.LoadAttributes(db.DefaultContext)) diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index bb479caf89..9bf0d3d745 100644 --- a/tests/integration/api_pull_test.go +++ b/tests/integration/api_pull_test.go @@ -223,23 +223,33 @@ func TestAPIEditPull(t *testing.T) { session := loginUser(t, owner10.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + title := "create a success pr" req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &api.CreatePullRequestOption{ Head: "develop", Base: "master", - Title: "create a success pr", + Title: title, }).AddTokenAuth(token) - pull := new(api.PullRequest) + apiPull := new(api.PullRequest) resp := MakeRequest(t, req, http.StatusCreated) - DecodeJSON(t, resp, pull) - assert.EqualValues(t, "master", pull.Base.Name) + DecodeJSON(t, resp, apiPull) + assert.EqualValues(t, "master", apiPull.Base.Name) - req = NewRequestWithJSON(t, http.MethodPatch, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d", owner10.Name, repo10.Name, pull.Index), &api.EditPullRequestOption{ + newTitle := "edit a this pr" + newBody := "edited body" + req = NewRequestWithJSON(t, http.MethodPatch, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d", owner10.Name, repo10.Name, apiPull.Index), &api.EditPullRequestOption{ Base: "feature/1", - Title: "edit a this pr", + Title: newTitle, + Body: &newBody, }).AddTokenAuth(token) resp = MakeRequest(t, req, http.StatusCreated) - DecodeJSON(t, resp, pull) - assert.EqualValues(t, "feature/1", pull.Base.Name) + DecodeJSON(t, resp, apiPull) + assert.EqualValues(t, "feature/1", apiPull.Base.Name) + // check comment history + pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: apiPull.ID}) + err := pull.LoadIssue(db.DefaultContext) + assert.NoError(t, err) + unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{IssueID: pull.Issue.ID, OldTitle: title, NewTitle: newTitle}) + unittest.AssertExistsAndLoadBean(t, &issues_model.ContentHistory{IssueID: pull.Issue.ID, ContentText: newBody, IsFirstCreated: false}) req = NewRequestWithJSON(t, http.MethodPatch, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d", owner10.Name, repo10.Name, pull.Index), &api.EditPullRequestOption{ Base: "not-exist",