-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GH-378]:Fixed issue #378 'Check for webhook while subscribing' #664
Conversation
…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
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 ReportPatch coverage has no change and project coverage change:
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
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and the check & message changes seem to work properly
server/plugin/command.go
Outdated
@@ -25,6 +26,11 @@ const ( | |||
featureStars = "stars" | |||
) | |||
|
|||
const ( | |||
ErrorNoWebhookFound = "\nNo webhook was found for this repository or organization. To create one, enter the following slash command `/github setup webhook`" | |||
GithubListOptionsPerPageValue = 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a const
for perPage
anywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no there is no constant for this in the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1/5 let's use a generic perPage
here and in the rest of the code.
server/plugin/command.go
Outdated
// Breaking from the loop if the repo or org is not found | ||
if strings.Contains(err.Error(), "404 Not Found") { | ||
isWebhook = true | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking here de-facto means returning, right? Why is the 404 case special?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one suggestion to rename a function. Otherwise LGTM 👍
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
/update-branch |
We don't have permissions to update this PR, please contact the submitter to apply the update. |
server/plugin/command.go
Outdated
@@ -25,6 +26,11 @@ const ( | |||
featureStars = "stars" | |||
) | |||
|
|||
const ( | |||
ErrorNoWebhookFound = "\nNo webhook was found for this repository or organization. To create one, enter the following slash command `/github setup webhook`" | |||
GithubListOptionsPerPageValue = 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1/5 let's use a generic perPage
here and in the rest of the code.
@Kshitij-Katiyar There is no need to re-request a review. That can be done once there is new code to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 🎉
@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. |
@matthewbirtch Do you have any thoughts on the screenshot and comment mentioned above? |
@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 |
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