Added collapsed style for issue labeled subscriptions mattermost/matt… #948
Conversation
|
Hello @JOAO-Ethan, 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. |
marianunez
added
2: Dev Review
There was a problem hiding this comment.
Pull request overview
This PR adds a collapsed notification style for GitHub issue labeled events. When the collapsed style is configured, issue labeled notifications display in a more compact format instead of the default expanded format with title and multiple lines.
Key Changes:
- Modified the
issueLabelledtemplate to support both collapsed and expanded notification styles
Comments suppressed due to low confidence (2)
server/plugin/template.go:1
- There are two spaces between the closing backtick and 'by'. This should be reduced to one space for consistent spacing.
// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved.
server/plugin/template.go:1
- The word 'issue-labeled' uses a hyphen while line 311 uses 'labeled' without 'issue-' prefix. Consider using consistent terminology across both style variants (either 'labeled' or 'issue-labeled' in both places) for better maintainability.
// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nevyangelova
left a comment
nevyangelova
left a comment
There was a problem hiding this comment.
Thanks @JOAO-Ethan looks good to me. Note for QA:
- Create a subscription with --render-style collapsed
/github subscribe owner/repo issues --render-style collapsed - Label an issue in GitHub
- Verify the notification in Mattermost is a single line like:
owner/repo issue #123 labeled bug by @username.
- Then repeat with expanded style
/github subscribe owner/repo issues --render-style expanded - Verify the notification shows heading format:
Issue Title
owner/repo#123
#issue-labeled enhancement by @username.
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
Hello! Apologies in the delay providing feedback on your PR, and great job in making sure we can have the collapsed style also present in these notifications :)
I think we'll just want the TestIssueLabelledTemplate test in template_test.go edited to also include a test case for when we are passing the collapsed config, there are other tests in the same file that you can reference to accomplish that
Thank you for your contribution and let me know if you have any questions 🙌
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
Superb! Thank you for following up so quickly 🥳
Everything looks great now
|
@JOAO-Ethan Ahh seems there are some lingering issues with the tests, would you please look into them and tag me again once done for a final check? 🙏 Thank you! |
|
@avasconcelos114 I think it's good ? Tbh I don't really understand why sometimes it wont accept the newline characters ^^' |
|
Hope it's good now |
server/plugin/template.go
Outdated
| template.Must(masterTemplate.New("issueLabelled").Funcs(funcMap).Parse(` | ||
| {{ if eq .Config.Style "collapsed" -}} | ||
| {{template "repo" .Event.GetRepo}} issue {{template "issue" .Event.GetIssue}} labeled ` + "`{{.Event.GetLabel.GetName}}`" + ` by {{template "user" .Event.GetSender}}. | ||
| {{- else }} |
There was a problem hiding this comment.
I ran a quick check in my local environment (you can run make test to run the same tests that are running in the pipeline prior to pushing your changes) and the only bit you're missing for the template and tests to work properly is a hyphen in this section
| {{- else }} | |
| {{- else -}} |
There was a problem hiding this comment.
it's fixed ! my tests pass in my local env
There was a problem hiding this comment.
Lovely! Now I see everything's running perfectly :)
|
@ogi-m this should be now ready to be tested following the steps Nevy posted earlier |
5 participants
Summary
Added a collapsed style for labeled issue notifications.
Ticket Link
Fixes #740