Skip to content

Fixes #7152 - Allow create/update/delete message to be empty, use default message #7324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 29, 2019
Merged

Fixes #7152 - Allow create/update/delete message to be empty, use default message #7324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60b373b
Fixes #7152 - Allow create/update/delete message to be empty, use def…
richmahn Jun 28, 2019
524462f
Linting fix
richmahn Jun 28, 2019
bfc9282
Fix to delete integration tests
richmahn Jun 28, 2019
fcc67ba
Merge branch 'master' into fix-7152-api-create-update-file-message-sh…
zeripath Jun 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion integrations/api_repo_file_create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

@lunny lunny Jun 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richmahn I mean we can use a translated message here. This will not block this PR.

Message: ctx.Tr("my language message")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why worry about translating a message in the integration test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you were just showing that as an example, but with GitHub there is no translation done with commit messages. But right, not the issue addressed by this PR, as it is just giving the default editor.repo.add, editor.repo.update, editor.repo.delete messages if message is empty.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @richmahn I don't think we should be trying to translate messages in the integration tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richmahn Oh. it's a test file. Ignore my words above.

Copy link
Contributor Author

@richmahn richmahn Jun 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a message should never be translated, but as the user intended. Only when it is auto-generated should it be translated. There may be a problem to as what if for some reason they actually wanted to write something like "template" or "home" which would get translated to "Template" or "Home" from our locale_en-US.ini file... while not a big change, it would boggle them why the message is getting uppercased.

Author: api.Identity{
Name: "John Doe",
Email: "[email protected]",
Expand Down Expand Up @@ -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()
Expand Down
16 changes: 15 additions & 1 deletion integrations/api_repo_file_delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: "[email protected]",
Expand Down Expand Up @@ -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++
Expand Down
36 changes: 33 additions & 3 deletions integrations/api_repo_file_update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: "[email protected]",
},
Committer: api.Identity{
Name: "Jane Doe",
Email: "[email protected]",
},
},
SHA: "103ff9234cefeee5ec5361d22b49fbb04d385885",
},
Content: contentEncoded,
}
}

Expand Down Expand Up @@ -67,7 +82,7 @@ func getExpectedFileResponseForUpdate(commitID, treePath string) *api.FileRespon
Email: "[email protected]",
},
},
Message: "Updates README.md\n",
Message: "My update of README.md\n",
},
Verification: &api.PayloadCommitVerification{
Verified: false,
Expand Down Expand Up @@ -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()
Expand All @@ -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)
Expand Down
44 changes: 23 additions & 21 deletions integrations/repofiles_delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: "[email protected]",
},
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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
})
}

Expand Down
5 changes: 5 additions & 0 deletions modules/repofiles/delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion modules/structs/repo_file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions routers/api/v1/repo/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down