-
Notifications
You must be signed in to change notification settings - Fork 148
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.
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
[GH-719] Pushed commits events: add option to show Author instead of committer #729
Conversation
…nstead of Committer.
Co-authored-by: Raghav Aggarwal <raghav.aggarwal@brightscout.com>
Co-authored-by: Raghav Aggarwal <raghav.aggarwal@brightscout.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏽
Hi @hanzei would you be able to approve for merge, or reassign to someone else if needed? Thanks in advance. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #729 +/- ##
==========================================
+ Coverage 15.79% 15.81% +0.01%
==========================================
Files 15 15
Lines 5767 5779 +12
==========================================
+ Hits 911 914 +3
- Misses 4814 4822 +8
- Partials 42 43 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @cpatulea 👍
I would love to see a test for the changes before merging it.
@@ -273,7 +283,7 @@ Assignees: {{range $i, $el := .Assignees -}} {{- if $i}}, {{end}}{{template "use | |||
template.Must(masterTemplate.New("pushedCommits").Funcs(funcMap).Parse(` | |||
{{template "user" .GetSender}} {{if .GetForced}}force-{{end}}pushed [{{len .Commits}} new commit{{if ne (len .Commits) 1}}s{{end}}]({{.GetCompare}}) to [\[{{.GetRepo.GetFullName}}:{{.GetRef | trimRef}}\]]({{.GetRepo.GetHTMLURL}}/tree/{{.GetRef | trimRef}}): | |||
{{range .Commits -}} | |||
[` + "`{{.GetID | substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{.GetCommitter.GetName}} | |||
[` + "`{{.GetID | substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{with . | commitAuthor}}{{.GetName}}{{end}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Is the with
needed?
[` + "`{{.GetID | substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{with . | commitAuthor}}{{.GetName}}{{end}} | |
[` + "`{{.GetID | substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{commitAuthor .}}{{.GetName}}{{end}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm new to Go template language and couldn't get it working without with
.
My attempt with:
[` + "`{{.GetID | substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{commitAuthor . | .GetName}}
gives this runtime error:
=== RUN TestPushedCommitsTemplate/single_commit,_with_'Show_Author_in_commit_notifications'
template_test.go:785:
Error Trace: /Users/catalinp/src/mattermost-plugin-github/server/plugin/template_test.go:785
Error: Received unexpected error:
template: pushedCommits:4:78: executing "pushedCommits" at <.GetName>: can't evaluate field GetName in type *github.HeadCommit
Could not execute template named pushedCommits
github.com/mattermost/mattermost-plugin-github/server/plugin.renderTemplate
/Users/catalinp/src/mattermost-plugin-github/server/plugin/template.go:459
github.com/mattermost/mattermost-plugin-github/server/plugin.TestPushedCommitsTemplate.func5
/Users/catalinp/src/mattermost-plugin-github/server/plugin/template_test.go:765
testing.tRunner
/opt/homebrew/Cellar/go/1.21.1/libexec/src/testing/testing.go:1595
runtime.goexit
/opt/homebrew/Cellar/go/1.21.1/libexec/src/runtime/asm_arm64.s:1197
Test: TestPushedCommitsTemplate/single_commit,_with_'Show_Author_in_commit_notifications'
Please let me know if you're aware of a better approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should work with
[` + "`{{.GetID | substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{with . | commitAuthor}}{{.GetName}}{{end}} | |
[` + "`{{.GetID | substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{commitAuthor .}}{{.GetName}}{{end}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, that is not valid template syntax due to imbalanced end
:
$ go test -v ./server/... -run 'TestPushedCommitsTemplate.*'
? github.com/mattermost/mattermost-plugin-github/server [no test files]
? github.com/mattermost/mattermost-plugin-github/server/testutils [no test files]
? github.com/mattermost/mattermost-plugin-github/server/plugin/graphql [no test files]
panic: template: pushedCommits:5: unexpected {{end}}
goroutine 1 [running]:
text/template.Must(...)
/opt/homebrew/Cellar/go/1.21.6/libexec/src/text/template/helper.go:26
github.com/mattermost/mattermost-plugin-github/server/plugin.init.0()
/Users/catalinp/src/mattermost-plugin-github/server/plugin/template.go:283 +0x4498
FAIL github.com/mattermost/mattermost-plugin-github/server/plugin 0.493s
FAIL
Done, PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding a test 👍
@cpatulea I've left two nit pick, but the change LGTM. Please let me know if you want to address them. |
Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>
Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>
Yep, I have addressed them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is still complaining about a linter error:
Running golangci-lint
golangci-lint run ./...
server/plugin/template.go:112: File is not `gofmt`-ed with `-s` (gofmt)
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍
My comment in #729 (comment) is non-blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution @cpatulea! LGTM, just one comment for discussion
Hey @AayushChaudhary0001 would you be able to take a look when you have a second? |
@cpatulea Yes, I will review it mostly by this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has been tested for the following scenarios:-
- Commit shown by author name
- Commit shown by commiter name
The PR was working fine for both the above conditions, LGTM. Approved
Thanks @cpatulea! |
Summary
Pushed commits events: add option to show Author instead of Committer.
Screenshot
What to test?
Before starting, some Mattermost channel should be subscribed to pushed commits notifications for a repo.
Ticket Link
Fixes #719