diff --git a/modules/structs/repo_file.go b/modules/structs/repo_file.go index 4729bde491..59665062b7 100644 --- a/modules/structs/repo_file.go +++ b/modules/structs/repo_file.go @@ -24,13 +24,6 @@ type FileOptions struct { Signoff bool `json:"signoff"` } -type FileOptionsWithSHA struct { - FileOptions - // the blob ID (SHA) for the file that already exists, it is required for changing existing files - // required: true - SHA string `json:"sha" binding:"Required"` -} - func (f *FileOptions) GetFileOptions() *FileOptions { return f } @@ -41,7 +34,7 @@ type FileOptionsInterface interface { var _ FileOptionsInterface = (*FileOptions)(nil) -// CreateFileOptions options for creating files +// CreateFileOptions options for creating a file // Note: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used) type CreateFileOptions struct { FileOptions @@ -50,16 +43,21 @@ type CreateFileOptions struct { ContentBase64 string `json:"content"` } -// DeleteFileOptions options for deleting files (used for other File structs below) +// DeleteFileOptions options for deleting a file // Note: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used) type DeleteFileOptions struct { - FileOptionsWithSHA + FileOptions + // the blob ID (SHA) for the file to delete + // required: true + SHA string `json:"sha" binding:"Required"` } -// UpdateFileOptions options for updating files +// UpdateFileOptions options for updating or creating a file // Note: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used) type UpdateFileOptions struct { - FileOptionsWithSHA + FileOptions + // the blob ID (SHA) for the file that already exists to update, or leave it empty to create a new file + SHA string `json:"sha"` // content must be base64 encoded // required: true ContentBase64 string `json:"content"` diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index 2b6348c2fb..ba98263819 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -525,7 +525,7 @@ func CreateFile(ctx *context.APIContext) { func UpdateFile(ctx *context.APIContext) { // swagger:operation PUT /repos/{owner}/{repo}/contents/{filepath} repository repoUpdateFile // --- - // summary: Update a file in a repository + // summary: Update a file in a repository if SHA is set, or create the file if SHA is not set // consumes: // - application/json // produces: @@ -554,6 +554,8 @@ func UpdateFile(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/FileResponse" + // "201": + // "$ref": "#/responses/FileResponse" // "403": // "$ref": "#/responses/error" // "404": @@ -572,8 +574,9 @@ func UpdateFile(ctx *context.APIContext) { ctx.APIError(http.StatusUnprocessableEntity, err) return } + willCreate := apiOpts.SHA == "" opts.Files = append(opts.Files, &files_service.ChangeRepoFile{ - Operation: "update", + Operation: util.Iif(willCreate, "create", "update"), ContentReader: contentReader, SHA: apiOpts.SHA, FromTreePath: apiOpts.FromPath, @@ -587,7 +590,7 @@ func UpdateFile(ctx *context.APIContext) { handleChangeRepoFilesError(ctx, err) } else { fileResponse := files_service.GetFileResponseFromFilesResponse(filesResponse, 0) - ctx.JSON(http.StatusOK, fileResponse) + ctx.JSON(util.Iif(willCreate, http.StatusCreated, http.StatusOK), fileResponse) } } diff --git a/services/pull/comment.go b/services/pull/comment.go index ca156d3d92..f24e8128e9 100644 --- a/services/pull/comment.go +++ b/services/pull/comment.go @@ -69,6 +69,10 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss if err != nil { return nil, err } + // It maybe an empty pull request. Only non-empty pull request need to create push comment + if len(data.CommitIDs) == 0 { + return nil, nil + } } dataJSON, err := json.Marshal(data) diff --git a/services/pull/pull.go b/services/pull/pull.go index a519a1032f..a8468b5bf3 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -155,6 +155,20 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { issue_service.ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifiers) + // Request reviews, these should be requested before other notifications because they will add request reviews record + // on database + permDoer, err := access_model.GetUserRepoPermission(ctx, repo, issue.Poster) + for _, reviewer := range opts.Reviewers { + if _, err = issue_service.ReviewRequest(ctx, pr.Issue, issue.Poster, &permDoer, reviewer, true); err != nil { + return err + } + } + for _, teamReviewer := range opts.TeamReviewers { + if _, err = issue_service.TeamReviewRequest(ctx, pr.Issue, issue.Poster, teamReviewer, true); err != nil { + return err + } + } + mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, issue.Poster, issue.Content) if err != nil { return err @@ -173,17 +187,7 @@ 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) - for _, reviewer := range opts.Reviewers { - if _, err = issue_service.ReviewRequest(ctx, pr.Issue, issue.Poster, &permDoer, reviewer, true); err != nil { - return err - } - } - for _, teamReviewer := range opts.TeamReviewers { - if _, err = issue_service.TeamReviewRequest(ctx, pr.Issue, issue.Poster, teamReviewer, true); err != nil { - return err - } - } + return nil } diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 966aff12f8..0cefa6795f 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -7634,7 +7634,7 @@ "tags": [ "repository" ], - "summary": "Update a file in a repository", + "summary": "Update a file in a repository if SHA is set, or create the file if SHA is not set", "operationId": "repoUpdateFile", "parameters": [ { @@ -7671,6 +7671,9 @@ "200": { "$ref": "#/responses/FileResponse" }, + "201": { + "$ref": "#/responses/FileResponse" + }, "403": { "$ref": "#/responses/error" }, @@ -22886,7 +22889,7 @@ "x-go-package": "code.gitea.io/gitea/modules/structs" }, "CreateFileOptions": { - "description": "CreateFileOptions options for creating files\nNote: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)", + "description": "CreateFileOptions options for creating a file\nNote: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)", "type": "object", "required": [ "content" @@ -23904,7 +23907,7 @@ "x-go-package": "code.gitea.io/gitea/modules/structs" }, "DeleteFileOptions": { - "description": "DeleteFileOptions options for deleting files (used for other File structs below)\nNote: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)", + "description": "DeleteFileOptions options for deleting a file\nNote: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)", "type": "object", "required": [ "sha" @@ -23940,7 +23943,7 @@ "x-go-name": "NewBranchName" }, "sha": { - "description": "the blob ID (SHA) for the file that already exists, it is required for changing existing files", + "description": "the blob ID (SHA) for the file to delete", "type": "string", "x-go-name": "SHA" }, @@ -28700,10 +28703,9 @@ "x-go-package": "code.gitea.io/gitea/modules/structs" }, "UpdateFileOptions": { - "description": "UpdateFileOptions options for updating files\nNote: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)", + "description": "UpdateFileOptions options for updating or creating a file\nNote: `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used)", "type": "object", "required": [ - "sha", "content" ], "properties": { @@ -28747,7 +28749,7 @@ "x-go-name": "NewBranchName" }, "sha": { - "description": "the blob ID (SHA) for the file that already exists, it is required for changing existing files", + "description": "the blob ID (SHA) for the file that already exists to update, or leave it empty to create a new file", "type": "string", "x-go-name": "SHA" }, diff --git a/tests/integration/api_repo_file_delete_test.go b/tests/integration/api_repo_file_delete_test.go index 9dd47f93e6..59e2131618 100644 --- a/tests/integration/api_repo_file_delete_test.go +++ b/tests/integration/api_repo_file_delete_test.go @@ -20,21 +20,19 @@ import ( func getDeleteFileOptions() *api.DeleteFileOptions { return &api.DeleteFileOptions{ - FileOptionsWithSHA: api.FileOptionsWithSHA{ - FileOptions: api.FileOptions{ - BranchName: "master", - NewBranchName: "master", - Message: "Removing the file new/file.txt", - Author: api.Identity{ - Name: "John Doe", - Email: "johndoe@example.com", - }, - Committer: api.Identity{ - Name: "Jane Doe", - Email: "janedoe@example.com", - }, + SHA: "103ff9234cefeee5ec5361d22b49fbb04d385885", + FileOptions: api.FileOptions{ + BranchName: "master", + NewBranchName: "master", + Message: "Removing the file new/file.txt", + Author: api.Identity{ + Name: "John Doe", + Email: "johndoe@example.com", + }, + Committer: api.Identity{ + Name: "Jane Doe", + Email: "janedoe@example.com", }, - SHA: "103ff9234cefeee5ec5361d22b49fbb04d385885", }, } } diff --git a/tests/integration/api_repo_file_update_test.go b/tests/integration/api_repo_file_update_test.go index 9a56711da6..6e6aae389f 100644 --- a/tests/integration/api_repo_file_update_test.go +++ b/tests/integration/api_repo_file_update_test.go @@ -28,21 +28,19 @@ func getUpdateFileOptions() *api.UpdateFileOptions { content := "This is updated text" contentEncoded := base64.StdEncoding.EncodeToString([]byte(content)) return &api.UpdateFileOptions{ - FileOptionsWithSHA: api.FileOptionsWithSHA{ - FileOptions: api.FileOptions{ - BranchName: "master", - NewBranchName: "master", - Message: "My update of new/file.txt", - Author: api.Identity{ - Name: "John Doe", - Email: "johndoe@example.com", - }, - Committer: api.Identity{ - Name: "Anne Doe", - Email: "annedoe@example.com", - }, + SHA: "103ff9234cefeee5ec5361d22b49fbb04d385885", + FileOptions: api.FileOptions{ + BranchName: "master", + NewBranchName: "master", + Message: "My update of new/file.txt", + Author: api.Identity{ + Name: "John Doe", + Email: "johndoe@example.com", + }, + Committer: api.Identity{ + Name: "Anne Doe", + Email: "annedoe@example.com", }, - SHA: "103ff9234cefeee5ec5361d22b49fbb04d385885", }, ContentBase64: contentEncoded, } @@ -180,6 +178,15 @@ func TestAPIUpdateFile(t *testing.T) { assert.Equal(t, expectedDownloadURL, *fileResponse.Content.DownloadURL) assert.Equal(t, updateFileOptions.Message+"\n", fileResponse.Commit.Message) + // Test updating a file without SHA (should create the file) + updateFileOptions = getUpdateFileOptions() + updateFileOptions.SHA = "" + req = NewRequestWithJSON(t, "PUT", "/api/v1/repos/user2/repo1/contents/update-create.txt", &updateFileOptions).AddTokenAuth(token2) + resp = MakeRequest(t, req, http.StatusCreated) + DecodeJSON(t, resp, &fileResponse) + assert.Equal(t, "08bd14b2e2852529157324de9c226b3364e76136", fileResponse.Content.SHA) + assert.Equal(t, setting.AppURL+"user2/repo1/raw/branch/master/update-create.txt", *fileResponse.Content.DownloadURL) + // Test updating a file and renaming it updateFileOptions = getUpdateFileOptions() updateFileOptions.BranchName = repo1.DefaultBranch diff --git a/tests/integration/pull_compare_test.go b/tests/integration/pull_compare_test.go index 01989e8f89..fc921d6268 100644 --- a/tests/integration/pull_compare_test.go +++ b/tests/integration/pull_compare_test.go @@ -102,7 +102,15 @@ func TestPullCompare_EnableAllowEditsFromMaintainer(t *testing.T) { // user4 creates a new branch and a PR testEditFileToNewBranch(t, user4Session, "user4", forkedRepoName, "master", "user4/update-readme", "README.md", "Hello, World\n(Edited by user4)\n") - resp := testPullCreateDirectly(t, user4Session, repo3.OwnerName, repo3.Name, "master", "user4", forkedRepoName, "user4/update-readme", "PR for user4 forked repo3") + resp := testPullCreateDirectly(t, user4Session, createPullRequestOptions{ + BaseRepoOwner: repo3.OwnerName, + BaseRepoName: repo3.Name, + BaseBranch: "master", + HeadRepoOwner: "user4", + HeadRepoName: forkedRepoName, + HeadBranch: "user4/update-readme", + Title: "PR for user4 forked repo3", + }) prURL := test.RedirectURL(resp) // user2 (admin of repo3) goes to the PR files page diff --git a/tests/integration/pull_create_test.go b/tests/integration/pull_create_test.go index d44c19b0b7..d9811d000f 100644 --- a/tests/integration/pull_create_test.go +++ b/tests/integration/pull_create_test.go @@ -60,26 +60,50 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo string, toSel return resp } -func testPullCreateDirectly(t *testing.T, session *TestSession, baseRepoOwner, baseRepoName, baseBranch, headRepoOwner, headRepoName, headBranch, title string) *httptest.ResponseRecorder { - headCompare := headBranch - if headRepoOwner != "" { - if headRepoName != "" { - headCompare = fmt.Sprintf("%s/%s:%s", headRepoOwner, headRepoName, headBranch) +type createPullRequestOptions struct { + BaseRepoOwner string + BaseRepoName string + BaseBranch string + HeadRepoOwner string + HeadRepoName string + HeadBranch string + Title string + ReviewerIDs string // comma-separated list of user IDs +} + +func (opts createPullRequestOptions) IsValid() bool { + return opts.BaseRepoOwner != "" && opts.BaseRepoName != "" && opts.BaseBranch != "" && + opts.HeadBranch != "" && opts.Title != "" +} + +func testPullCreateDirectly(t *testing.T, session *TestSession, opts createPullRequestOptions) *httptest.ResponseRecorder { + if !opts.IsValid() { + t.Fatal("Invalid pull request options") + } + + headCompare := opts.HeadBranch + if opts.HeadRepoOwner != "" { + if opts.HeadRepoName != "" { + headCompare = fmt.Sprintf("%s/%s:%s", opts.HeadRepoOwner, opts.HeadRepoName, opts.HeadBranch) } else { - headCompare = fmt.Sprintf("%s:%s", headRepoOwner, headBranch) + headCompare = fmt.Sprintf("%s:%s", opts.HeadRepoOwner, opts.HeadBranch) } } - req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/compare/%s...%s", baseRepoOwner, baseRepoName, baseBranch, headCompare)) + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/compare/%s...%s", opts.BaseRepoOwner, opts.BaseRepoName, opts.BaseBranch, headCompare)) resp := session.MakeRequest(t, req, http.StatusOK) // Submit the form for creating the pull htmlDoc := NewHTMLParser(t, resp.Body) link, exists := htmlDoc.doc.Find("form.ui.form").Attr("action") assert.True(t, exists, "The template has changed") - req = NewRequestWithValues(t, "POST", link, map[string]string{ + params := map[string]string{ "_csrf": htmlDoc.GetCSRF(), - "title": title, - }) + "title": opts.Title, + } + if opts.ReviewerIDs != "" { + params["reviewer_ids"] = opts.ReviewerIDs + } + req = NewRequestWithValues(t, "POST", link, params) resp = session.MakeRequest(t, req, http.StatusOK) return resp } @@ -246,7 +270,15 @@ func TestPullCreatePrFromBaseToFork(t *testing.T) { testEditFile(t, sessionBase, "user2", "repo1", "master", "README.md", "Hello, World (Edited)\n") // Create a PR - resp := testPullCreateDirectly(t, sessionFork, "user1", "repo1", "master", "user2", "repo1", "master", "This is a pull title") + resp := testPullCreateDirectly(t, sessionFork, createPullRequestOptions{ + BaseRepoOwner: "user1", + BaseRepoName: "repo1", + BaseBranch: "master", + HeadRepoOwner: "user2", + HeadRepoName: "repo1", + HeadBranch: "master", + Title: "This is a pull title", + }) // check the redirected URL url := test.RedirectURL(resp) assert.Regexp(t, "^/user1/repo1/pulls/[0-9]*$", url) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 67dd023a8a..5b13b7187f 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -184,13 +184,29 @@ func TestPullView_CodeOwner(t *testing.T) { session := loginUser(t, "user5") // create a pull request on the forked repository, code reviewers should not be mentioned - testPullCreateDirectly(t, session, "user5", "test_codeowner", forkedRepo.DefaultBranch, "", "", "codeowner-basebranch-forked", "Test Pull Request on Forked Repository") + testPullCreateDirectly(t, session, createPullRequestOptions{ + BaseRepoOwner: "user5", + BaseRepoName: "test_codeowner", + BaseBranch: forkedRepo.DefaultBranch, + HeadRepoOwner: "", + HeadRepoName: "", + HeadBranch: "codeowner-basebranch-forked", + Title: "Test Pull Request on Forked Repository", + }) pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"}) unittest.AssertNotExistsBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) // create a pull request to base repository, code reviewers should be mentioned - testPullCreateDirectly(t, session, repo.OwnerName, repo.Name, repo.DefaultBranch, forkedRepo.OwnerName, forkedRepo.Name, "codeowner-basebranch-forked", "Test Pull Request3") + testPullCreateDirectly(t, session, createPullRequestOptions{ + BaseRepoOwner: repo.OwnerName, + BaseRepoName: repo.Name, + BaseBranch: repo.DefaultBranch, + HeadRepoOwner: forkedRepo.OwnerName, + HeadRepoName: forkedRepo.Name, + HeadBranch: "codeowner-basebranch-forked", + Title: "Test Pull Request3", + }) pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"}) unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8}) diff --git a/tests/integration/repo_webhook_test.go b/tests/integration/repo_webhook_test.go index 49f6853abc..91539f2429 100644 --- a/tests/integration/repo_webhook_test.go +++ b/tests/integration/repo_webhook_test.go @@ -15,6 +15,7 @@ import ( "time" auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" @@ -681,15 +682,30 @@ func Test_WebhookPullRequest(t *testing.T) { }, http.StatusOK) defer provider.Close() + testCtx := NewAPITestContext(t, "user2", "repo1", auth_model.AccessTokenScopeAll) + // add user4 as collabrator so that it can be a reviewer + doAPIAddCollaborator(testCtx, "user4", perm.AccessModeWrite)(t) + // 1. create a new webhook with special webhook for repo1 - session := loginUser(t, "user2") + sessionUser2 := loginUser(t, "user2") + sessionUser4 := loginUser(t, "user4") - testAPICreateWebhookForRepo(t, session, "user2", "repo1", provider.URL(), "pull_request") + // ignore the possible review_requested event to keep the test deterministic + testAPICreateWebhookForRepo(t, sessionUser2, "user2", "repo1", provider.URL(), "pull_request_only") - testAPICreateBranch(t, session, "user2", "repo1", "master", "master2", http.StatusCreated) + testAPICreateBranch(t, sessionUser2, "user2", "repo1", "master", "master2", http.StatusCreated) // 2. trigger the webhook repo1 := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 1}) - testCreatePullToDefaultBranch(t, session, repo1, repo1, "master2", "first pull request") + testPullCreateDirectly(t, sessionUser4, createPullRequestOptions{ + BaseRepoOwner: repo1.OwnerName, + BaseRepoName: repo1.Name, + BaseBranch: repo1.DefaultBranch, + HeadRepoOwner: "", + HeadRepoName: "", + HeadBranch: "master2", + Title: "first pull request", + ReviewerIDs: "2", // add user2 as reviewer + }) // 3. validate the webhook is triggered assert.Equal(t, "pull_request", triggeredEvent) @@ -701,6 +717,8 @@ func Test_WebhookPullRequest(t *testing.T) { assert.Equal(t, 0, *payloads[0].PullRequest.Additions) assert.Equal(t, 0, *payloads[0].PullRequest.ChangedFiles) assert.Equal(t, 0, *payloads[0].PullRequest.Deletions) + assert.Len(t, payloads[0].PullRequest.RequestedReviewers, 1) + assert.Equal(t, int64(2), payloads[0].PullRequest.RequestedReviewers[0].ID) }) }