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

[rinkimekari]

This addresses https://github.com/mattermost/mattermost-plugin-github/issues/484, which requests a feature that allows for collapsing large notification messages. Examples: /github subscriptions add owner/repo pulls,issues --collapsed true /github subscriptions add owner/repo pulls,issues --exclude-org-members true

When set to true, this flag makes posts that normally use larger text for titles into smaller, inline posts with a less detailed summary of the repo, pr/issue name, label name if applicable, and who the action was committed by. Of course, when set to false, the default behavior is used. Autocomplete behavior for all flags has been added and adjusted to complete possible parameters as well.

To allow for the addition of parameters such as booleans to flags, the behavior for the --exclude-org-member flag has changed as well. However, it is also the same logic - you just put true or false after it to do what you want.

Thanks for reviewing!

[mattermod]

Hello @rinkimekari,

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.

[hanzei]

@rinkimekari That you very much for the constitution :+1: Could you please outline what the difference between your PR and https://github.com/mattermost/mattermost-plugin-github/pull/534 is?

[rinkimekari]

@hanzei Mine includes more information than just the title, while still staying in one line. It looks like this:

Obviously the info changes based on the event. Another thing is that my pull request makes it easier for future flags to implement arguments. Also another difference is that I implemented parsing for the flags so it tells the user if they used an invalid flag and helps them use it correctly.

With some adjustments to fit the new flag parsing I made, the title-only flag can also be added as an option for those who want the least amount of screen real estate used. They wouldn't conflict with each other, though there should probably be a check to see if both are enabled and respond accordingly.

PS: I was going to try out the title-only flag using the submitted branch for further comparison, but I can't seem to get it to work. Is it working for you?

~~PPS: I looked through the code for the title-only flag and it doesn't seem to act based on each subscription, but rather globally. I may have looked at it wrong, but this may want to be looked at.~~

Edit for clarity: I can deploy the plugin and autocomplete for the flag works perfectly, but when using the flag, it still shows the normal large messages. I'll put my issues in the PR so the submitter can view them.

Edit again: I read some more code and I was wrong about it not acting based on subscriptions. My bad.

[mickmister]

@hanzei https://github.com/mattermost/mattermost-plugin-github/issues/484 and https://github.com/mattermost/mattermost-plugin-github/issues/470 seem to be duplicate tickets, which this PR and the title-only PR https://github.com/mattermost/mattermost-plugin-github/pull/534 fix respectively.

https://github.com/mattermost/mattermost-plugin-github/issues/484, the issue linked in this PR, was claimed over a month ago by the author of this PR. It seems we have duplicate work submitted between these PRs. The other PR states it is "title-only", but reading the code of the PR, it looks like it indeed contains the PR author's name etc. So it seems we have have duplicate PRs here.

How should we handle this case when we have duplicate tickets with one of them claimed but not submitted before the other ticket was implemented, but was not claimed?

[rinkimekari]

After reviewing the code some more, @mickmister is correct in that it shows more than just the title, and it still keeps the larger font. That being said, it might be a good idea to create a title-only flag that shows only the title in normal sized text with what happened, e.g. "New PR: mattermost/mattermost-plugin-github#545" Let me know if you want to see this implemented

[codecov-commenter]

Codecov Report

Merging #545 (1609ae2) into master (99372e2) will decrease coverage by 0.04%. The diff coverage is 9.02%.

@@            Coverage Diff             @@
##           master     #545      +/-   ##
==========================================
- Coverage   15.57%   15.52%   -0.05%     
==========================================
  Files          15       15              
  Lines        4136     4233      +97     
==========================================
+ Hits          644      657      +13     
- Misses       3450     3534      +84     
  Partials       42       42              

Impacted Files	Coverage Δ
server/plugin/command.go	8.19% <0.00%> (-0.43%)	:arrow_down:
server/plugin/subscriptions.go	9.42% <0.00%> (-0.69%)	:arrow_down:
server/plugin/webhook.go	0.00% <0.00%> (ø)
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 99372e2...1609ae2. Read the comment docs.

[mickmister]

This PR has some overlap with https://github.com/mattermost/mattermost-plugin-github/pull/534 which has been merged, so I'm going to close this one. Thanks for your work on this PR @rinkimekari :+1:

[rinkimekari]

👍