From 002b597c1f87cd5c69d32053a62f57c08d48d3ee Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Sat, 29 Jun 2019 11:19:24 -0400 Subject: [PATCH] Fixes #7152 - Allow create/update/delete message to be empty, use default message (#7324) * Fixes #7152 - Allow create/update/delete message to be empty, use default message * Linting fix * Fix to delete integration tests --- integrations/api_repo_file_create_test.go | 15 +++++++- integrations/api_repo_file_delete_test.go | 16 ++++++++- integrations/api_repo_file_update_test.go | 36 +++++++++++++++++-- integrations/repofiles_delete_test.go | 44 ++++++++++++----------- modules/repofiles/delete.go | 5 +++ modules/structs/repo_file.go | 2 +- routers/api/v1/repo/file.go | 13 +++++++ 7 files changed, 104 insertions(+), 27 deletions(-) diff --git a/integrations/api_repo_file_create_test.go b/integrations/api_repo_file_create_test.go index 973ed9dfa5..b00583c191 100644 --- a/integrations/api_repo_file_create_test.go +++ b/integrations/api_repo_file_create_test.go @@ -28,7 +28,7 @@ func getCreateFileOptions() api.CreateFileOptions { FileOptions: api.FileOptions{ BranchName: "master", NewBranchName: "master", - Message: "Creates new/file.txt", + Message: "Making this new file new/file.txt", Author: api.Identity{ Name: "John Doe", Email: "johndoe@example.com", @@ -150,6 +150,19 @@ func TestAPICreateFile(t *testing.T) { assert.EqualValues(t, expectedSHA, fileResponse.Content.SHA) assert.EqualValues(t, expectedHTMLURL, fileResponse.Content.HTMLURL) assert.EqualValues(t, expectedDownloadURL, fileResponse.Content.DownloadURL) + assert.EqualValues(t, createFileOptions.Message+"\n", fileResponse.Commit.Message) + + // Test creating a file without a message + createFileOptions = getCreateFileOptions() + createFileOptions.Message = "" + fileID++ + treePath = fmt.Sprintf("new/file%d.txt", fileID) + url = fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s?token=%s", user2.Name, repo1.Name, treePath, token2) + req = NewRequestWithJSON(t, "POST", url, &createFileOptions) + resp = session.MakeRequest(t, req, http.StatusCreated) + DecodeJSON(t, resp, &fileResponse) + expectedMessage := "Add '" + treePath + "'\n" + assert.EqualValues(t, expectedMessage, fileResponse.Commit.Message) // Test trying to create a file that already exists, should fail createFileOptions = getCreateFileOptions() diff --git a/integrations/api_repo_file_delete_test.go b/integrations/api_repo_file_delete_test.go index 2f5f9028a8..59b3ff91b6 100644 --- a/integrations/api_repo_file_delete_test.go +++ b/integrations/api_repo_file_delete_test.go @@ -23,7 +23,7 @@ func getDeleteFileOptions() *api.DeleteFileOptions { FileOptions: api.FileOptions{ BranchName: "master", NewBranchName: "master", - Message: "Updates new/file.txt", + Message: "Removing the file new/file.txt", Author: api.Identity{ Name: "John Doe", Email: "johndoe@example.com", @@ -89,6 +89,20 @@ func TestAPIDeleteFile(t *testing.T) { DecodeJSON(t, resp, &fileResponse) assert.NotNil(t, fileResponse) assert.Nil(t, fileResponse.Content) + assert.EqualValues(t, deleteFileOptions.Message+"\n", fileResponse.Commit.Message) + + // Test deleting file without a message + fileID++ + treePath = fmt.Sprintf("delete/file%d.txt", fileID) + createFile(user2, repo1, treePath) + deleteFileOptions = getDeleteFileOptions() + deleteFileOptions.Message = "" + url = fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s?token=%s", user2.Name, repo1.Name, treePath, token2) + req = NewRequestWithJSON(t, "DELETE", url, &deleteFileOptions) + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &fileResponse) + expectedMessage := "Delete '" + treePath + "'\n" + assert.EqualValues(t, expectedMessage, fileResponse.Commit.Message) // Test deleting a file with the wrong SHA fileID++ diff --git a/integrations/api_repo_file_update_test.go b/integrations/api_repo_file_update_test.go index 90fecf59d0..17fa2adb26 100644 --- a/integrations/api_repo_file_update_test.go +++ b/integrations/api_repo_file_update_test.go @@ -25,8 +25,23 @@ func getUpdateFileOptions() *api.UpdateFileOptions { content := "This is updated text" contentEncoded := base64.StdEncoding.EncodeToString([]byte(content)) return &api.UpdateFileOptions{ - DeleteFileOptions: *getDeleteFileOptions(), - Content: contentEncoded, + DeleteFileOptions: api.DeleteFileOptions{ + 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: "Jane Doe", + Email: "janedoe@example.com", + }, + }, + SHA: "103ff9234cefeee5ec5361d22b49fbb04d385885", + }, + Content: contentEncoded, } } @@ -67,7 +82,7 @@ func getExpectedFileResponseForUpdate(commitID, treePath string) *api.FileRespon Email: "johndoe@example.com", }, }, - Message: "Updates README.md\n", + Message: "My update of README.md\n", }, Verification: &api.PayloadCommitVerification{ Verified: false, @@ -140,6 +155,7 @@ func TestAPIUpdateFile(t *testing.T) { assert.EqualValues(t, expectedSHA, fileResponse.Content.SHA) assert.EqualValues(t, expectedHTMLURL, fileResponse.Content.HTMLURL) assert.EqualValues(t, expectedDownloadURL, fileResponse.Content.DownloadURL) + assert.EqualValues(t, updateFileOptions.Message+"\n", fileResponse.Commit.Message) // Test updating a file and renaming it updateFileOptions = getUpdateFileOptions() @@ -160,6 +176,20 @@ func TestAPIUpdateFile(t *testing.T) { assert.EqualValues(t, expectedHTMLURL, fileResponse.Content.HTMLURL) assert.EqualValues(t, expectedDownloadURL, fileResponse.Content.DownloadURL) + // Test updating a file without a message + updateFileOptions = getUpdateFileOptions() + updateFileOptions.Message = "" + updateFileOptions.BranchName = repo1.DefaultBranch + fileID++ + treePath = fmt.Sprintf("update/file%d.txt", fileID) + createFile(user2, repo1, treePath) + url = fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s?token=%s", user2.Name, repo1.Name, treePath, token2) + req = NewRequestWithJSON(t, "PUT", url, &updateFileOptions) + resp = session.MakeRequest(t, req, http.StatusOK) + DecodeJSON(t, resp, &fileResponse) + expectedMessage := "Update '" + treePath + "'\n" + assert.EqualValues(t, expectedMessage, fileResponse.Commit.Message) + // Test updating a file with the wrong SHA fileID++ treePath = fmt.Sprintf("update/file%d.txt", fileID) diff --git a/integrations/repofiles_delete_test.go b/integrations/repofiles_delete_test.go index c3bb18ec9c..f4cb4510be 100644 --- a/integrations/repofiles_delete_test.go +++ b/integrations/repofiles_delete_test.go @@ -24,40 +24,32 @@ func getDeleteRepoFileOptions(repo *models.Repository) *repofiles.DeleteRepoFile TreePath: "README.md", Message: "Deletes README.md", SHA: "4b4851ad51df6a7d9f25c979345979eaeb5b349f", - Author: nil, - Committer: nil, + Author: &repofiles.IdentityOptions{ + Name: "Bob Smith", + Email: "bob@smith.com", + }, + Committer: nil, } } func getExpectedDeleteFileResponse(u *url.URL) *api.FileResponse { + // Just returns fields that don't change, i.e. fields with commit SHAs and dates can't be determined return &api.FileResponse{ Content: nil, Commit: &api.FileCommitResponse{ - CommitMeta: api.CommitMeta{ - URL: u.String() + "api/v1/repos/user2/repo1/git/commits/65f1bf27bc3bf70f64657658635e66094edbcb4d", - SHA: "65f1bf27bc3bf70f64657658635e66094edbcb4d", - }, - HTMLURL: u.String() + "user2/repo1/commit/65f1bf27bc3bf70f64657658635e66094edbcb4d", Author: &api.CommitUser{ Identity: api.Identity{ - Name: "user1", - Email: "address1@example.com", + Name: "Bob Smith", + Email: "bob@smith.com", }, - Date: "2017-03-19T20:47:59Z", }, Committer: &api.CommitUser{ Identity: api.Identity{ - Name: "Ethan Koenig", - Email: "ethantkoenig@gmail.com", + Name: "Bob Smith", + Email: "bob@smith.com", }, - Date: "2017-03-19T20:47:59Z", - }, - Parents: []*api.CommitMeta{}, - Message: "Initial commit\n", - Tree: &api.CommitMeta{ - URL: u.String() + "api/v1/repos/user2/repo1/git/trees/2a2f1d4670728a2e10049e345bd7a276468beab6", - SHA: "2a2f1d4670728a2e10049e345bd7a276468beab6", }, + Message: "Deletes README.md\n", }, Verification: &api.PayloadCommitVerification{ Verified: false, @@ -89,7 +81,12 @@ func testDeleteRepoFile(t *testing.T, u *url.URL) { fileResponse, err := repofiles.DeleteRepoFile(repo, doer, opts) assert.Nil(t, err) expectedFileResponse := getExpectedDeleteFileResponse(u) - assert.EqualValues(t, expectedFileResponse, fileResponse) + assert.NotNil(t, fileResponse) + assert.Nil(t, fileResponse.Content) + assert.EqualValues(t, expectedFileResponse.Commit.Message, fileResponse.Commit.Message) + assert.EqualValues(t, expectedFileResponse.Commit.Author.Identity, fileResponse.Commit.Author.Identity) + assert.EqualValues(t, expectedFileResponse.Commit.Committer.Identity, fileResponse.Commit.Committer.Identity) + assert.EqualValues(t, expectedFileResponse.Verification, fileResponse.Verification) }) t.Run("Verify README.md has been deleted", func(t *testing.T) { @@ -124,7 +121,12 @@ func testDeleteRepoFileWithoutBranchNames(t *testing.T, u *url.URL) { fileResponse, err := repofiles.DeleteRepoFile(repo, doer, opts) assert.Nil(t, err) expectedFileResponse := getExpectedDeleteFileResponse(u) - assert.EqualValues(t, expectedFileResponse, fileResponse) + assert.NotNil(t, fileResponse) + assert.Nil(t, fileResponse.Content) + assert.EqualValues(t, expectedFileResponse.Commit.Message, fileResponse.Commit.Message) + assert.EqualValues(t, expectedFileResponse.Commit.Author.Identity, fileResponse.Commit.Author.Identity) + assert.EqualValues(t, expectedFileResponse.Commit.Committer.Identity, fileResponse.Commit.Committer.Identity) + assert.EqualValues(t, expectedFileResponse.Verification, fileResponse.Verification) }) } diff --git a/modules/repofiles/delete.go b/modules/repofiles/delete.go index 91910fa860..3d9b06b1c1 100644 --- a/modules/repofiles/delete.go +++ b/modules/repofiles/delete.go @@ -198,6 +198,11 @@ func DeleteRepoFile(repo *models.Repository, doer *models.User, opts *DeleteRepo return nil, fmt.Errorf("PushUpdate: %v", err) } + commit, err = t.GetCommit(commitHash) + if err != nil { + return nil, err + } + file, err := GetFileResponseFromCommit(repo, commit, opts.NewBranch, treePath) if err != nil { return nil, err diff --git a/modules/structs/repo_file.go b/modules/structs/repo_file.go index c447d26724..e5be9ce108 100644 --- a/modules/structs/repo_file.go +++ b/modules/structs/repo_file.go @@ -8,7 +8,7 @@ package structs // FileOptions options for all file APIs type FileOptions struct { // message (optional) for the commit of this file. if not supplied, a default message will be used - Message string `json:"message" binding:"Required"` + Message string `json:"message"` // branch (optional) to base this file from. if not given, the default branch is used BranchName string `json:"branch"` // new_branch (optional) will make a new branch from `branch` before creating the file diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index 20f80f37f4..d510756283 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -204,6 +204,11 @@ func CreateFile(ctx *context.APIContext, apiOpts api.CreateFileOptions) { Email: apiOpts.Author.Email, }, } + + if opts.Message == "" { + opts.Message = ctx.Tr("repo.editor.add", opts.TreePath) + } + if fileResponse, err := createOrUpdateFile(ctx, opts); err != nil { ctx.Error(http.StatusInternalServerError, "CreateFile", err) } else { @@ -264,6 +269,10 @@ func UpdateFile(ctx *context.APIContext, apiOpts api.UpdateFileOptions) { }, } + if opts.Message == "" { + opts.Message = ctx.Tr("repo.editor.update", opts.TreePath) + } + if fileResponse, err := createOrUpdateFile(ctx, opts); err != nil { ctx.Error(http.StatusInternalServerError, "UpdateFile", err) } else { @@ -346,6 +355,10 @@ func DeleteFile(ctx *context.APIContext, apiOpts api.DeleteFileOptions) { }, } + if opts.Message == "" { + opts.Message = ctx.Tr("repo.editor.delete", opts.TreePath) + } + if fileResponse, err := repofiles.DeleteRepoFile(ctx.Repo.Repository, ctx.User, opts); err != nil { ctx.Error(http.StatusInternalServerError, "DeleteFile", err) } else {