[GH-378]:Fixed issue #378 'Check for webhook while subscribing' by Kshitij-Katiyar · Pull Request #664 · mattermost/mattermost-plugin-github

Kshitij-Katiyar · 2023-04-04T10:59:12Z

Summary

When a user creates a subscription to a repo the plugin is now checking the repo (or parent Organization for Enterprise) to see if a webhook is in place to deliver the events.
If no webhook is found on the GitHub side, an ephemeral post is sent to remind the user that a webhook must be set up..
Issue #378

…bing' (#28) * [MI-2935]:Fixed issue mattermost#378 'Check for webhook while subscribing' * [MI-2935]:Fixed self review comments * [MI-2935]:Updated the message * [MI-2935]:Fixed review comments * [MI-2935]:Fixed lint errors * [MI-2935]:Fixed review comments

mattermost-build · 2023-04-04T10:59:21Z

Hello @Kshitij-Katiyar,

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 · 2023-04-04T11:10:40Z

Codecov Report

Patch coverage has no change and project coverage change: -0.23 ⚠️

Comparison is base (757c9c1) 15.54% compared to head (a963985) 15.32%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #664      +/-   ##
==========================================
- Coverage   15.54%   15.32%   -0.23%     
==========================================
  Files          15       15              
  Lines        5473     5548      +75     
==========================================
- Hits          851      850       -1     
- Misses       4579     4656      +77     
+ Partials       43       42       -1

Impacted Files	Coverage Δ
server/plugin/command.go	`7.50% <0.00%> (-0.65%)`	⬇️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

m1lt0n

Tested locally and the check & message changes seem to work properly

hanzei · 2023-04-11T21:34:55Z


+const (
+	ErrorNoWebhookFound           = "\nNo webhook was found for this repository or organization. To create one, enter the following slash command `/github setup webhook`"
+	GithubListOptionsPerPageValue = 50


Do we have a const for perPage anywhere else?

no there is no constant for this in the code

1/5 let's use a generic perPage here and in the rest of the code.

hanzei · 2023-04-11T21:39:31Z

+			// Breaking from the loop if the repo or org is not found
+			if strings.Contains(err.Error(), "404 Not Found") {
+				isWebhook = true
+				break


Breaking here de-facto means returning, right? Why is the 404 case special?

404 case is special because we are checking whether there is an org or repo present with the name we put during subscribing if not we don't need any additional messages.

…#29) * [MI-2998]:Fixed review comments of PR mattermost#664 on github plugin * [MI-2998]:Fixed self review comments

mickmister

Just one suggestion to rename a function. Otherwise LGTM 👍

mattermost-build · 2023-05-07T01:01:53Z

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!

hanzei · 2023-05-09T19:50:00Z

/update-branch

mattermost-build · 2023-05-09T19:50:02Z

We don't have permissions to update this PR, please contact the submitter to apply the update.

hanzei · 2023-05-09T19:50:41Z


+const (
+	ErrorNoWebhookFound           = "\nNo webhook was found for this repository or organization. To create one, enter the following slash command `/github setup webhook`"
+	GithubListOptionsPerPageValue = 50


1/5 let's use a generic perPage here and in the rest of the code.

hanzei · 2023-08-08T12:25:18Z

@Kshitij-Katiyar There is no need to re-request a review. That can be done once there is new code to review.

hanzei

Awesome 🎉

DHaussermann · 2023-09-25T13:12:44Z

@Kshitij-Katiyar I did not have time to pull this into my work queue.

I'll keep myself asa reviewer for now. Please advise if this can be covered by a QA resource on the Brightscout side.

AayushChaudhary0001 · 2023-11-21T11:31:21Z

The PR has been tested and the actual result can be enhanced as the ephemeral post is getting merged with the warning message for the private repository.

Enhancement : A separate title for the ephemeral post can be added.

mickmister · 2023-11-22T21:22:37Z

@matthewbirtch Do you have any thoughts on the screenshot and comment mentioned above?

mickmister · 2023-11-22T21:25:01Z

@AayushChaudhary0001 I see, the "No webhook was found" is blended into the same paragraph as the "private repository" warning. Maybe we should put the webhook message above, or have a "Note:" before the webhook message

matthewbirtch · 2023-11-22T21:30:07Z

@AayushChaudhary0001 I see, the "No webhook was found" is blended into the same paragraph as the "private repository" warning. Maybe we should put the webhook message above, or have a "Note:" before the webhook message

I agree that we shouldn't blend this note in with the warning paragraph.

I would also say that it doesn't quite feel right that we say "Successfully subscribed to {repo}", and then have a warning and a note about no webhook being found.

Maybe we consider adding the note to the first paragraph so it reads:

"Successfully subscribed to {repo}, but no webhook was found for this repository or organization. To create one, enter /github setup webhook"

Kshitij-Katiyar requested a review from hanzei as a code owner April 4, 2023 10:59

Kshitij-Katiyar requested a review from m1lt0n April 4, 2023 10:59

mattermost-build added the Contributor label Apr 4, 2023

m1lt0n approved these changes Apr 4, 2023

View reviewed changes

hanzei added the 2: Dev Review Requires review by a core committer label Apr 11, 2023

hanzei linked an issue Apr 11, 2023 that may be closed by this pull request

GitHub - Check for an existing webhook when subscribing to a repo #378

Closed

hanzei requested changes Apr 11, 2023

View reviewed changes

Kshitij-Katiyar added a commit to Brightscout/mattermost-plugin-github that referenced this pull request Apr 13, 2023

[MI-2998]:Fixed review comments of PR mattermost#664 on github plugin

992307a

Kshitij-Katiyar mentioned this pull request Apr 13, 2023

[MI-2998]:Fixed review comments of PR #664 on github plugin Brightscout/mattermost-plugin-github#29

Merged

[MI-2998]:Fixed review comments of PR mattermost#664 on github plugin (…

24d33e2

…#29) * [MI-2998]:Fixed review comments of PR mattermost#664 on github plugin * [MI-2998]:Fixed self review comments

Kshitij-Katiyar requested a review from hanzei April 17, 2023 12:29

hanzei requested review from mickmister and removed request for hanzei April 23, 2023 20:17

mickmister approved these changes Apr 25, 2023

View reviewed changes

Comment thread server/plugin/command.go Outdated

mickmister added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Apr 25, 2023

[MM-378]:Updated function name

a963985

Kshitij-Katiyar requested a review from hanzei April 26, 2023 07:44

mattermost-build added the Lifecycle/1:stale label May 7, 2023

hanzei requested review from DHaussermann and removed request for hanzei May 9, 2023 19:50

hanzei requested changes May 9, 2023

View reviewed changes

hanzei removed the request for review from DHaussermann May 9, 2023 19:53

hanzei added the 2: Dev Review Requires review by a core committer label May 9, 2023

Kshitij-Katiyar requested a review from hanzei July 7, 2023 09:40

mickmister approved these changes Jul 11, 2023

View reviewed changes

hanzei reviewed Jul 20, 2023

View reviewed changes

Comment thread server/plugin/command.go Outdated

Comment thread server/plugin/command.go Outdated

Kshitij-Katiyar requested a review from hanzei July 21, 2023 10:26

hanzei removed their request for review August 8, 2023 11:23

Kshitij-Katiyar requested a review from hanzei August 8, 2023 12:13

hanzei removed their request for review August 8, 2023 12:24

[MM-378]:Fixed review comments

d4b58a7

Kshitij-Katiyar requested a review from hanzei August 9, 2023 10:20

hanzei approved these changes Aug 9, 2023

View reviewed changes

hanzei removed 2: Dev Review Requires review by a core committer Awaiting Submitter Action Blocked on the author Lifecycle/1:stale labels Aug 9, 2023

hanzei requested a review from DHaussermann August 9, 2023 12:16

hanzei requested a review from AayushChaudhary0001 September 29, 2023 07:35

Kshitij-Katiyar added this to the v2.3.0 milestone Oct 5, 2023

hanzei removed the request for review from DHaussermann October 16, 2023 20:38

hanzei mentioned this pull request Nov 9, 2023

Bump version to 2.2.0. #709

Merged

avas27JTG modified the milestones: v2.3.0, v2.2.0 Nov 13, 2023

mickmister added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Nov 22, 2023

mickmister merged commit 1a1f668 into mattermost:master Nov 22, 2023

mickmister mentioned this pull request Nov 28, 2023

Clarify subscribe message when there is no webhook found #715

Closed

Uh oh!

Conversation

Kshitij-Katiyar commented Apr 4, 2023

Uh oh!

mattermost-build commented Apr 4, 2023

Uh oh!

codecov-commenter commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

m1lt0n left a comment

Choose a reason for hiding this comment

Uh oh!

hanzei Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

Kshitij-Katiyar Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

hanzei May 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hanzei Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

Kshitij-Katiyar Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mickmister left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattermost-build commented May 7, 2023

Uh oh!

hanzei commented May 9, 2023

Uh oh!

mattermost-build commented May 9, 2023

Uh oh!

hanzei May 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hanzei commented Aug 8, 2023

Uh oh!

hanzei left a comment

Choose a reason for hiding this comment

Uh oh!

DHaussermann commented Sep 25, 2023

Uh oh!

AayushChaudhary0001 commented Nov 21, 2023

Uh oh!

mickmister commented Nov 22, 2023

Uh oh!

mickmister commented Nov 22, 2023

Uh oh!

matthewbirtch commented Nov 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

codecov-commenter commented Apr 4, 2023 •

edited

Loading