Merge branch 'main' into webhook-bark

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"`

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)
 	}
 }
 

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)

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
 }
 

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"
         },

tests/integration/api_repo_file_delete_test.go

@@ -20,21 +20,19 @@ import (
 
 func getDeleteFileOptions() *api.DeleteFileOptions {
 	return &api.DeleteFileOptions{
-		FileOptionsWithSHA: api.FileOptionsWithSHA{
+		SHA: "103ff9234cefeee5ec5361d22b49fbb04d385885",
-			FileOptions: api.FileOptions{
+		FileOptions: api.FileOptions{
-				BranchName:    "master",
+			BranchName:    "master",
-				NewBranchName: "master",
+			NewBranchName: "master",
-				Message:       "Removing the file new/file.txt",
+			Message:       "Removing the file new/file.txt",
-				Author: api.Identity{
+			Author: api.Identity{
-					Name:  "John Doe",
+				Name:  "John Doe",
-					Email: "johndoe@example.com",
+				Email: "johndoe@example.com",
-				},
+			},
-				Committer: api.Identity{
+			Committer: api.Identity{
-					Name:  "Jane Doe",
+				Name:  "Jane Doe",
-					Email: "janedoe@example.com",
+				Email: "janedoe@example.com",
-				},
 			},
-			SHA: "103ff9234cefeee5ec5361d22b49fbb04d385885",
 		},
 	}
 }

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{
+		SHA: "103ff9234cefeee5ec5361d22b49fbb04d385885",
-			FileOptions: api.FileOptions{
+		FileOptions: api.FileOptions{
-				BranchName:    "master",
+			BranchName:    "master",
-				NewBranchName: "master",
+			NewBranchName: "master",
-				Message:       "My update of new/file.txt",
+			Message:       "My update of new/file.txt",
-				Author: api.Identity{
+			Author: api.Identity{
-					Name:  "John Doe",
+				Name:  "John Doe",
-					Email: "johndoe@example.com",
+				Email: "johndoe@example.com",
-				},
+			},
-				Committer: api.Identity{
+			Committer: api.Identity{
-					Name:  "Anne Doe",
+				Name:  "Anne Doe",
-					Email: "annedoe@example.com",
+				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

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

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 {
+type createPullRequestOptions struct {
-	headCompare := headBranch
+	BaseRepoOwner string
-	if headRepoOwner != "" {
+	BaseRepoName  string
-		if headRepoName != "" {
+	BaseBranch    string
-			headCompare = fmt.Sprintf("%s/%s:%s", headRepoOwner, headRepoName, headBranch)
+	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)

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})

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)
 	})
 }