[GH-470] allow users to specify notifications verbosity using a new render-style flag

[ijansky]

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 https://github.com/mattermost/mattermost-plugin-github/issues/470

[mattermod]

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]

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%)	:arrow_down:
server/plugin/subscriptions.go	9.52% <0.00%> (-0.59%)	:arrow_down:
server/plugin/webhook.go	0.95% <31.81%> (+0.95%)	:arrow_up:
server/plugin/template.go	95.60% <100.00%> (+0.21%)	:arrow_up:

---

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.

[ijansky]

Thanks for the review @hanzei!

[hanzei]

Your welcome! There was nothing to complain about :wink:

[rinkimekari]

@hanzei I tested this fix out because it has to do with my PR https://github.com/mattermost/mattermost-plugin-github/pull/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]

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

[TeodorPt]

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:

1. 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.
2. 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]

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

[TeodorPt]

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:

[mickmister]

@TeodorPt Please avoid force-pushing, as GitHub makes it difficult to keep track of changes over time when this happens. Personally, I like deterministically seeing the diff between reviews. Unfortunately when this occurs I have to re-review the whole PR since there is no way of me knowing that the previous commits do not have other changes in them.

[TeodorPt]

@TeodorPt Please avoid force-pushing, as GitHub makes it difficult to keep track of changes over time when this happens. Personally, I like deterministically seeing the diff between reviews. Unfortunately when this occurs I have to re-review the whole PR since there is no way of me knowing that the previous commits do not have other changes in them.

I agree, I didn't change anything though. My email alias was wrong and that's why I forced pushed.

[mickmister]

@TeodorPt Also, if you'd like to have me re-review after changes have been addressed, please explicitly re-request review via GitHub's request feature

[mickmister]

there is no way of me knowing that the previous commits do not have other changes in them.

I agree, I didn't change anything though.

@TeodorPt This is mainly a concern for security. Being an open-source project, we have to be absolutely sure what changes are being introduced, which means we need to review every line. I'm sure you are trustworthy, but I need to verify the changes nonetheless.

[TeodorPt]

there is no way of me knowing that the previous commits do not have other changes in them.

I agree, I didn't change anything though.

@TeodorPt This is mainly a concern for security. Being an open-source project, we have to be absolutely sure what changes are being introduced, which means we need to review every line. I'm sure you are trustworthy, but I need to verify the changes nonetheless.

From that angle I have to agree. Thanks for the perspective :) please re-review then and I will avoid force-pushing any further.

[mickmister]

@dipak-demansol This is ready for QA review

[mattermod]

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!

[mickmister]

@dipak-demansol Please prioritize this review if you can

[TeodorPt]

So you're saying flags don't appear in the auto-complete if no event is specified? I think this was pre-existing with the other flag as well, but I'll take a look to see how we can make it appear.

[dipak-demansol]

So you're saying flags don't appear in the auto-complete if no event is specified? I think this was pre-existing with the other flag as well, but I'll take a look to see how we can make it appear.

Yes, flags is not appeared in the auto-complete if no event is specified.

[mickmister]

Let's move the features argument to a flag argument instead of a positional, to properly treat it as an optional argument among the other optional arguments. features should be the first listed flag in the list for discoverability.

cc @TeodorPt @dipak-demansol

[TeodorPt]

@mickmister @dipak-demansol ptal again, now features is a flag and needs to be added by something like --features pulls,issues

[mattermod]

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]

@mickmister ptal, this needs a maintainer approval it seems

[TeodorPt]

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]

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

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

/update-branch

[ijansky]

@TeodorPt thank you for addressing code review comments. 👍

[mattermod]

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!