[GH-719] Pushed commits events: add option to show Author instead of committer by cpatulea · Pull Request #729 · mattermost/mattermost-plugin-github

cpatulea · 2024-01-10T09:47:01Z

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.

Go to plugin settings and set "Pushed commits events: show Author instead of Committer:" to true.
Create a git commit with committer != author. For example, "git am" a patch from someone else, or "git commit --author 'Test author test@example.com'" (example: https://github.com/FOULAB/eigma-test/commit/65ed77834f659a0498c5d02cc319e38bd12c50f3)
Push commit to GitHub
Observe the pushed commits notification.

Ticket Link

Fixes #719

…nstead of Committer.

mattermost-build · 2024-01-10T09:47:04Z

Hello @cpatulea,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

Co-authored-by: Raghav Aggarwal <raghav.aggarwal@brightscout.com>

raghavaggarwal2308

LGTM 👍🏽

cpatulea · 2024-01-17T10:35:17Z

Hi @hanzei would you be able to approve for merge, or reassign to someone else if needed? Thanks in advance.

codecov-commenter · 2024-01-23T12:00:46Z

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (5aa2450) 15.79% compared to head (5e0a5f2) 15.81%.
Report is 1 commits behind head on master.

❗ Current head 5e0a5f2 differs from pull request most recent head fe1ed95. Consider uploading reports for the commit fe1ed95 to get more accurate results

Files	Patch %	Lines
server/plugin/template.go	44.44%	4 Missing and 1 partial ⚠️
server/plugin/webhook.go	0.00%	1 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

hanzei

Thanks for the contribution @cpatulea 👍

I would love to see a test for the changes before merging it.

hanzei · 2024-01-23T12:04:26Z

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


Nit: Is the with needed?

Suggested change

[` + "`{{.GetID | substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{with . | commitAuthor}}{{.GetName}}{{end}}

[` + "`{{.GetID | substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{commitAuthor .}}{{.GetName}}{{end}}

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.

I think it should work with

Suggested change

[` + "`{{.GetID | substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{with . | commitAuthor}}{{.GetName}}{{end}}

[` + "`{{.GetID | substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{commitAuthor .}}{{.GetName}}{{end}}

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

cpatulea · 2024-01-23T15:08:54Z

I would love to see a test for the changes before merging it.

Done, PTAL.

hanzei

Thanks for adding a test 👍

hanzei · 2024-01-23T15:50:56Z

@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>

cpatulea · 2024-01-23T15:52:58Z

@cpatulea I've left two nit pick, but the change LGTM. Please let me know if you want to address them.

Yep, I have addressed them.

hanzei

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)

cpatulea · 2024-01-25T17:34:39Z

server/plugin/template.go:112: File is not gofmt-ed with -s (gofmt)

Done.

hanzei

Nice work 👍

My comment in #729 (comment) is non-blocking.

mickmister

Thanks for this contribution @cpatulea! LGTM, just one comment for discussion

cpatulea · 2024-02-04T15:19:41Z

Hey @AayushChaudhary0001 would you be able to take a look when you have a second?

AayushChaudhary0001 · 2024-02-05T07:24:24Z

@cpatulea Yes, I will review it mostly by this week.

AayushChaudhary0001

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

mickmister · 2024-02-08T19:15:34Z

Thanks @cpatulea!

[mattermostGH-719] Pushed commits events: add option to show Author i…

249cffc

…nstead of Committer.

cpatulea requested a review from hanzei as a code owner January 10, 2024 09:47

mattermost-build added the Contributor label Jan 10, 2024

hanzei requested a review from raghavaggarwal2308 January 10, 2024 09:48

hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jan 10, 2024

raghavaggarwal2308 suggested changes Jan 12, 2024

View reviewed changes

Comment thread plugin.json Outdated

Comment thread server/plugin/configuration.go Outdated

cpatulea and others added 3 commits January 12, 2024 12:57

Update plugin.json

0f208dd

Co-authored-by: Raghav Aggarwal <raghav.aggarwal@brightscout.com>

Update server/plugin/configuration.go

b5e071d

Co-authored-by: Raghav Aggarwal <raghav.aggarwal@brightscout.com>

Rename configuration field to ShowAuthorInCommitNotification.

5e0a5f2

cpatulea requested a review from raghavaggarwal2308 January 12, 2024 11:02

raghavaggarwal2308 approved these changes Jan 12, 2024

View reviewed changes

hanzei requested changes Jan 23, 2024

View reviewed changes

cpatulea added 3 commits January 23, 2024 14:44

Settings: use exact name 'Pushes'.

c2bb06b

Add unit test.

2cee047

Fix formatting (gofmt -w).

9b4a850

cpatulea requested a review from hanzei January 23, 2024 15:09

hanzei approved these changes Jan 23, 2024

View reviewed changes

Comment thread server/plugin/template_test.go Outdated

Comment thread server/plugin/template.go Outdated

hanzei requested a review from AayushChaudhary0001 January 23, 2024 15:51

hanzei removed the 2: Dev Review Requires review by a core committer label Jan 23, 2024

cpatulea and others added 2 commits January 23, 2024 15:51

Update server/plugin/template_test.go - t.Cleanup

d085287

Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>

Update server/plugin/template.go - branch style

fe1ed95

Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>

hanzei requested changes Jan 24, 2024

View reviewed changes

Fix formatting (gofmt -s -w).

f7bdacd

cpatulea requested a review from hanzei January 25, 2024 17:34

hanzei approved these changes Jan 26, 2024

View reviewed changes

mickmister approved these changes Jan 26, 2024

View reviewed changes

Comment thread server/plugin/webhook.go

AayushChaudhary0001 approved these changes Feb 7, 2024

View reviewed changes

mickmister added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Feb 8, 2024

mickmister merged commit 87db4b0 into mattermost:master Feb 8, 2024

This was referenced Feb 8, 2024

Bump version to 2.2.0. #709

Merged

Bump prepackage Github plugin version to 2.2.0 mattermost/mattermost#26218

Merged

raghavaggarwal2308 mentioned this pull request Aug 26, 2024

Showing invalid name when a draft PR is marked as ready #813

Closed

	[` + "`{{.GetID \| substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{with . \| commitAuthor}}{{.GetName}}{{end}}
	[` + "`{{.GetID \| substr 0 6}}`" + `]({{.GetURL}}) {{.GetMessage}} - {{commitAuthor .}}{{.GetName}}{{end}}

Uh oh!

Conversation

cpatulea commented Jan 10, 2024

Summary

Screenshot

What to test?

Ticket Link

Uh oh!

mattermost-build commented Jan 10, 2024

Uh oh!

Uh oh!

Uh oh!

raghavaggarwal2308 left a comment

Choose a reason for hiding this comment

Uh oh!

cpatulea commented Jan 17, 2024

Uh oh!

codecov-commenter commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hanzei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hanzei Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

cpatulea Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

hanzei Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

cpatulea Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

cpatulea commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hanzei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hanzei commented Jan 23, 2024

Uh oh!

cpatulea commented Jan 23, 2024

Uh oh!

hanzei left a comment

Choose a reason for hiding this comment

Uh oh!

cpatulea commented Jan 25, 2024

Uh oh!

hanzei left a comment

Choose a reason for hiding this comment

Uh oh!

mickmister left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cpatulea commented Feb 4, 2024

Uh oh!

AayushChaudhary0001 commented Feb 5, 2024

Uh oh!

AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

Uh oh!

mickmister commented Feb 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-commenter commented Jan 23, 2024 •

edited

Loading

cpatulea commented Jan 23, 2024 •

edited

Loading