[GH-484] Add Collapsed Flag + Make Flags Accept Boolean by rinkimekari · Pull Request #545 · mattermost/mattermost-plugin-github

rinkimekari · 2022-02-26T03:38:48Z

This addresses #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 · 2022-02-26T03:38:52Z

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 · 2022-02-28T09:31:41Z

@rinkimekari That you very much for the constitution 👍 Could you please outline what the difference between your PR and #534 is?

rinkimekari · 2022-03-02T02:04:45Z

@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 · 2022-03-02T04:14:03Z

@hanzei #484 and #470 seem to be duplicate tickets, which this PR and the title-only PR #534 fix respectively.

#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?

mickmister

Thanks for the PR @rinkimekari 🎉

I have a few requests regarding the parsing of the command's provided arguments, and some requests for wording changes. Otherwise the PR is looking great 👍

mickmister · 2022-03-02T03:35:28Z

  - The following flags are supported:
-     - `--exclude-org-member`: events triggered by organization members will not be delivered. It will be locked to the organization provided in the plugin configuration and it will only work for users whose membership is public. Note that organization members and collaborators are not the same.
+     - `--exclude-org-member [true/false]`: events triggered by organization members will not be delivered. It will be locked to the organization provided in the plugin configuration and it will only work for users whose membership is public. Note that organization members and collaborators are not the same.
+     - `--collapsed [true/false]`: All notifications about events from the plugin will be collapsed and save space if set to true. The default is false, which shows large, informational messages for notifications, taking up more space.


Suggested change

- `--collapsed [true/false]`: All notifications about events from the plugin will be collapsed and save space if set to true. The default is false, which shows large, informational messages for notifications, taking up more space.

- `--collapsed [true/false]`: All notifications about events from the plugin will be collapsed, if set to `true`. The default is `false`, which shows large, informational messages, including the issue's description, for notifications related issue creations and PR creations. Using the `--collapsed` flag allows the posts to take up less space and as a result, cause ;less the noise in the subscription's channel.

@cwarnermm Can you please take a look at the wording here?

A few typos, but I fixed those and it'll be in my next commit.

mickmister · 2022-03-02T03:39:40Z

+	excludeOrgMemberFlag      = "exclude-org-member"
+	collapseNotificationsFlag = "collapsed"


We should probably namespace these with a prefix:

Suggested change

excludeOrgMemberFlag = "exclude-org-member"

collapseNotificationsFlag = "collapsed"

commandFlagExcludeOrgMember = "exclude-org-member"

commandFlagCollapsed = "collapsed"

mickmister · 2022-03-02T03:41:40Z

+func validFlagsString() string {
+	flags := ""
+	for s, i := range validFlags {
+		if i {
+			flags += s + ", "
+		}
+	}
+
+	return strings.TrimSuffix(flags, ", ")
+}


There are other commands that use flags, such as the /github mute command. So I suppose if we want this function, it should probably be made general purpose, by accepting a map[string]bool.

Good idea. I'll change it.

One thought. What are those flags and where are they? They should also be handled the same way as these flags right?

mickmister · 2022-03-02T03:55:09Z

+				if err != "" {
+					return err
+				}
+				i++


This implicit increment at the end of the for loop seems like a bit of an anti-pattern. I would expect the loop's i variable to be incremented exclusively by the for loop definition.

We can instead split the parameters into groups like so:

arguments := parameters[1:] numGroups := (len(arguments) + 1) / 2 for i := 1; i < numGroups; i++ { if len(arguments) < i * 2 { return "Please use the correct format for flags. For example: `--collapsed true`. Valid flags include " + validFlagsString() } start := (i - 1) * 2 name, value := arguments[start], arguments[start + 1] }

mickmister · 2022-03-02T03:56:39Z

+		HelpText: "Expand notifications",
+	},
+	}
+	subscriptionsAdd.AddNamedStaticListArgument("collapsed", "Whether or not to collapse event notifications. The default is expanded.", false, collapsedFlagItems)


Suggested change

subscriptionsAdd.AddNamedStaticListArgument("collapsed", "Whether or not to collapse event notifications. The default is expanded.", false, collapsedFlagItems)

subscriptionsAdd.AddNamedStaticListArgument("collapsed", "Issue creation and PR creation posts will be collapsed, like posts for other event types. The default is false.", false, collapsedFlagItems)

mickmister · 2022-03-02T04:20:49Z

 		UserId:  p.BotUserID,
 		Type:    "custom_git_issue",
-		Message: renderedMessage,
+		Message: "",


We don't need to place this here, as Go will use the empty string by default. If this field was a pointer, it would be possible to be nil, but in this case it's guaranteed to be a valid string, defaulting to the empty string.

Another spot above also uses this pattern and should be changed to let the struct implicitly set its Message

rinkimekari · 2022-03-02T04:31:34Z

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

Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com>

codecov-commenter · 2022-03-02T04:36:14Z

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%)`	⬇️
server/plugin/subscriptions.go	`9.42% <0.00%> (-0.69%)`	⬇️
server/plugin/webhook.go	`0.00% <0.00%> (ø)`
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 99372e2...1609ae2. Read the comment docs.

Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com>

mickmister · 2022-08-29T16:52:28Z

This PR has some overlap with #534 which has been merged, so I'm going to close this one. Thanks for your work on this PR @rinkimekari 👍

rinkimekari · 2022-08-29T20:36:44Z

👍

Add Collapsed Flag

f7b9d60

rinkimekari requested a review from hanzei as a code owner February 26, 2022 03:38

mattermod added the Contributor label Feb 26, 2022

Fix Test

5f42cc7

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

hanzei removed their request for review March 1, 2022 13:47

mickmister self-requested a review March 2, 2022 03:34

rinkimekari mentioned this pull request Mar 2, 2022

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

Merged

mickmister suggested changes Mar 2, 2022

View reviewed changes

Use actionLabeled

4076b8c

Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com>

rinkimekari and others added 3 commits March 1, 2022 23:37

Fix typo

98c9dc1

Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com>

More specific error

2986f34

Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com>

More specific error

1609ae2

Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com>

mattermod added the Lifecycle/1:stale label Mar 19, 2022

mickmister closed this Aug 29, 2022

		excludeOrgMemberFlag = "exclude-org-member"
		collapseNotificationsFlag = "collapsed"

	subscriptionsAdd.AddNamedStaticListArgument("collapsed", "Whether or not to collapse event notifications. The default is expanded.", false, collapsedFlagItems)
	subscriptionsAdd.AddNamedStaticListArgument("collapsed", "Issue creation and PR creation posts will be collapsed, like posts for other event types. The default is false.", false, collapsedFlagItems)

Uh oh!

Conversation

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

Uh oh!

Uh oh!

mattermod commented Feb 26, 2022

Uh oh!

hanzei commented Feb 28, 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 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mickmister left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rinkimekari commented Mar 2, 2022

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

mickmister commented Aug 29, 2022

Uh oh!

rinkimekari commented Aug 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rinkimekari commented Feb 26, 2022 •

edited

Loading

rinkimekari commented Mar 2, 2022 •

edited

Loading

mickmister commented Mar 2, 2022 •

edited

Loading

codecov-commenter commented Mar 2, 2022 •

edited

Loading