[GH-470] allow users to specify notifications verbosity using a new render-style flag by ijansky · Pull Request #534 · mattermost/mattermost-plugin-github

ijansky · 2022-02-16T17:51:10Z

Summary

This pull request adds a --render-style flag to the subscription command to define the notification verbosity of an event rather than the full body as it can be quite lengthy and easily spam a team mattermost channel. It is a shared effort of myself and @TeodorPt and we raised this PR to get early feedback on the direction that this PR takes

Ticket Link

Fixes #470

mattermod · 2022-02-16T17:51:14Z

Hello @ijansky,

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.

codecov-commenter · 2022-02-17T08:22:11Z

Codecov Report

Merging #534 (ced9364) into master (75bcaa1) will increase coverage by 0.29%.
The diff coverage is 39.37%.

@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
+ Coverage   15.51%   15.80%   +0.29%     
==========================================
  Files          15       15              
  Lines        4151     4201      +50     
==========================================
+ Hits          644      664      +20     
- Misses       3465     3494      +29     
- Partials       42       43       +1

Impacted Files	Coverage Δ
server/plugin/command.go	`8.33% <0.00%> (-0.29%)`	⬇️
server/plugin/subscriptions.go	`9.52% <0.00%> (-0.59%)`	⬇️
server/plugin/webhook.go	`0.95% <31.81%> (+0.95%)`	⬆️
server/plugin/template.go	`95.60% <100.00%> (+0.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75bcaa1...ced9364. Read the comment docs.

hanzei

Great work 👍

This change will increase the load of the webhook handler a bit as there is additional parsing of the template, but that is fine.

ijansky · 2022-02-23T13:11:41Z

Thanks for the review @hanzei!

hanzei · 2022-02-23T13:12:09Z

Your welcome! There was nothing to complain about 😉

rinkimekari · 2022-03-02T03:43:39Z

@hanzei
I tested this fix out because it has to do with my PR #545 and I wanted to compare them. However, I can't seem to get it to work. Deploying the plugin works fine, and autocomplete for the command works perfectly, but whether or not the flag is enabled, it always shows the normal expanded message for notifications. There should definitely be tests in template_test.go for the cases when the flag is enabled.

mickmister · 2022-03-04T22:56:31Z

@ijansky Do you mind posting a screenshot of what this feature looks like?

TeodorPt · 2022-03-08T10:23:39Z

Hi @mickmister @rinkimekari, sorry for the delay, had a busy week. I have tested this locally now and it works - the way to subscribe would be /github subscriptions add [owner/repo] [features] --title-only. See screenshots below:

Long version:

Title-only:

Of course I will add unit tests for the flag enabled as well before merging. Also the "title-only" version could be displayed even more collapsed if you prefer, it's easy to change.
But we opened this PR initially as a request for comments, to get a sense of the direction and see if you would prefer this approach, since we identified 2 ways of doing it:

This approach: passing an event with config options to the template, and then letting the template decide how to present the event based on the config.
Multiple templates approach (similar to @rinkimekari 's solution): having one template for no-title/collapsed and one for full and deciding between the two.

We went for the former approach because it keeps a nice separation of rendering logic and business logic (whole rendering is the responsibility of the template itself), and it makes it easier to add additional configuration options in the future, without having to add if-else statements where the templates are used.

It seems there was duplicate work here, since there were 2 opened issues for the same issue. Let me know if you're happy for me to proceed and I'll turn this PR into something merge-worthy asap.

Thanks!

TeodorPt · 2022-03-17T08:54:00Z

@mickmister can you help me with a review when you have the chance? Thank you very much!

mickmister

Thanks @ijansky and @TeodorPt for implementing this!

This mostly looks good. I just have one request to modify how the command flags work in the command autocomplete, so that we can use multiple flags in the same command.

mickmister · 2022-03-17T18:36:52Z

+				Hint:     "(optional)",
+				Item:     "--title-only",
+			}}
+		subscriptionsAdd.AddStaticListArgument("Currently supports --exclude-org-member and --title-only", false, flags)


In the state of this PR, we are not able to use both the exclude-org-member and title-only flags simultaneously. This is because they are being treated as a single positional argument, each showing up as a single selectable option in the same argument.

Similar to what was discussed in #505 (comment), we should use "named" arguments with explicit true/false values, rather than using positional arguments that implicitly mean true.

Also, this title-only flag should be available even when config.GitHubOrg is not defined, but it's currently added in this if block which restricts to when config.GitHubOrg is defined.

mickmister · 2022-03-22T23:03:17Z

+		actual, err := renderTemplate("newPR", GetEventWithRenderConfig(
+			&github.PullRequestEvent{
+				Repo:        &repo,
+				PullRequest: &pullRequestWithMultipleLabelsAndAssignees,
+				Sender:      &user,
+			},
+			nil,
+		))


Can we add tests for all templates that support the title-only flag?

TeodorPt · 2022-04-01T12:18:09Z

Hi @mickmister, I've made several changes to the PR, PTAL again:

flags now accept a value as well, per your suggestion
taking advantage of that and the EventWithConfig approach, the flag is now not title-only anymore, but render-style, which could either be collapsed, skip-body or default
added tests

Here's the rendered output for all 3 formats in my tests:

dipak-demansol

LGTM

mattermod · 2022-05-23T01:01:18Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

TeodorPt · 2022-05-27T15:17:02Z

@mickmister ptal, this needs a maintainer approval it seems

mickmister

LGTM! Just one suggestion to namespace the flag name constants

Thanks @TeodorPt @ijansky @dipak-demansol

mickmister · 2022-05-27T15:22:42Z

 	excludeOrgMemberFlag = "exclude-org-member"
 	renderStyleFlag      = "render-style"
+	FeaturesFlag         = "features"


@TeodorPt What do you think about namespacing the constants like so:

Suggested change

excludeOrgMemberFlag = "exclude-org-member"

renderStyleFlag = "render-style"

FeaturesFlag = "features"

flagExcludeOrgMember = "exclude-org-member"

flagRenderStyle = "render-style"

flagFeatures = "features"

TeodorPt · 2022-06-23T08:15:12Z

It's been a while since updating this, been caught up in a lot of other things. I've made the changes and will do a re-test asap and ping you when it's ready (some changes to the documentation were also needed).

TeodorPt · 2022-06-25T05:44:07Z

@mickmister I finally got around to addressing your comments. I also identified some omissions in the help documentation - I've updated that as well and re-tested everything end-to-end to make sure things are still working.

I've forced-pushed due to CI tests timing out (you can check with Compare that no changes were made in the force push). However, the tests are still timing out, do you know why?

Thank you!

mickmister

LGTM!

mickmister · 2022-07-22T08:05:18Z

@DHaussermann After looking at the changes from last QA review from Dipak, there are no significant changes, so I think this is good to merge

DHaussermann · 2022-07-28T13:57:36Z

/update-branch

DHaussermann

Tested and passed

All 3 renders styles work as expected
Render styles behaves as default when not set
--render-style shows in subscriptions list when set
LGTM!

Huge thanks @ijansky for this enhancement 🎉
Sorry for the delay on this.

ijansky · 2022-07-29T19:44:06Z

@TeodorPt thank you for addressing code review comments. 👍

mattermod · 2022-08-09T01:01:26Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

mattermod added the Contributor label Feb 16, 2022

TeodorPt force-pushed the GH-470_title-only-notifications branch 3 times, most recently from ca92c97 to 6c88096 Compare February 17, 2022 08:15

ijansky force-pushed the GH-470_title-only-notifications branch from 6c88096 to 8cbf330 Compare February 17, 2022 15:15

hanzei added the 2: Dev Review Requires review by a core committer label Feb 18, 2022

hanzei requested a review from mickmister February 18, 2022 18:22

hanzei added the 3: QA Review Requires review by a QA tester label Feb 18, 2022

hanzei approved these changes Feb 23, 2022

View reviewed changes

ijansky marked this pull request as ready for review February 23, 2022 13:10

ijansky changed the title ~~[GH-470] title only notifications - WIP~~ [GH-470] title only notifications Feb 23, 2022

hanzei added this to the v2.2.0 milestone Feb 23, 2022

hanzei mentioned this pull request Feb 28, 2022

[GH-484] Add Collapsed Flag + Make Flags Accept Boolean #545

Closed

mickmister suggested changes Mar 17, 2022

View reviewed changes

mickmister reviewed Mar 22, 2022

View reviewed changes

TeodorPt force-pushed the GH-470_title-only-notifications branch 3 times, most recently from c82a29a to f5f20e3 Compare April 1, 2022 12:16

TeodorPt force-pushed the GH-470_title-only-notifications branch 3 times, most recently from a0899b1 to fea4888 Compare April 1, 2022 12:28

dipak-demansol approved these changes May 12, 2022

View reviewed changes

dipak-demansol added QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels May 12, 2022

mattermod added the Lifecycle/1:stale label May 23, 2022

mickmister approved these changes May 27, 2022

View reviewed changes

Update flag const names and documentation

f3f5c61

TeodorPt force-pushed the GH-470_title-only-notifications branch from 1c38648 to f3f5c61 Compare June 25, 2022 05:33

mickmister approved these changes Jul 22, 2022

View reviewed changes

mickmister requested a review from DHaussermann July 22, 2022 07:59

mickmister removed 2: Dev Review Requires review by a core committer Lifecycle/1:stale QA Review Done PR has been approved by QA labels Jul 22, 2022

add --verbose flag to npm install

92ee3e4

Merge branch 'master' into GH-470_title-only-notifications

ced9364

DHaussermann approved these changes Jul 29, 2022

View reviewed changes

DHaussermann added the 4: Reviews Complete All reviewers have approved the pull request label Jul 29, 2022

mattermod added the Lifecycle/1:stale label Aug 9, 2022

catalintomai approved these changes Aug 22, 2022

View reviewed changes

mickmister merged commit 4043d0b into mattermost:master Aug 22, 2022

This was referenced Aug 29, 2022

Add per-subscription setting to collapse all posts #484

Closed

Release v2.2.0 #582

Closed

Release bump v2.2.0 #583

Closed

Uh oh!

Conversation

ijansky commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Ticket Link

Uh oh!

mattermod commented Feb 16, 2022

Uh oh!

codecov-commenter commented Feb 17, 2022 • 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!

ijansky commented Feb 23, 2022

Uh oh!

hanzei commented Feb 23, 2022

Uh oh!

rinkimekari commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mickmister commented Mar 4, 2022

Uh oh!

TeodorPt commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TeodorPt commented Mar 17, 2022

Uh oh!

mickmister left a comment

Choose a reason for hiding this comment

Uh oh!

mickmister Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mickmister Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

TeodorPt commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dipak-demansol left a comment

Choose a reason for hiding this comment

Uh oh!

mattermod commented May 23, 2022

Uh oh!

TeodorPt commented May 27, 2022

Uh oh!

mickmister left a comment

Choose a reason for hiding this comment

Uh oh!

mickmister May 27, 2022

Choose a reason for hiding this comment

Uh oh!

TeodorPt commented Jun 23, 2022

Uh oh!

TeodorPt commented Jun 25, 2022

Uh oh!

mickmister left a comment

Choose a reason for hiding this comment

Uh oh!

mickmister commented Jul 22, 2022

Uh oh!

DHaussermann commented Jul 28, 2022

Uh oh!

DHaussermann left a comment

Choose a reason for hiding this comment

Uh oh!

ijansky commented Jul 29, 2022

Uh oh!

mattermod commented Aug 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ijansky commented Feb 16, 2022 •

edited

Loading

codecov-commenter commented Feb 17, 2022 •

edited

Loading

rinkimekari commented Mar 2, 2022 •

edited

Loading

TeodorPt commented Mar 8, 2022 •

edited

Loading

TeodorPt commented Apr 1, 2022 •

edited

Loading