Update subscription messages with respect to webhook #770
Conversation
server/plugin/command.go
Outdated
| found, foundErr := p.checkIfConfiguredWebhookExists(ctx, githubClient, repo, owner) | ||
| if foundErr != nil { | ||
| if strings.Contains(foundErr.Error(), "404 Not Found") { | ||
| return errorWebhookToUser | ||
| return subOrgMsg | ||
| } | ||
| return errors.Wrap(foundErr, "failed to get the list of webhooks").Error() | ||
| } |
There was a problem hiding this comment.
In the case where the user has access to check webhooks but there isn't one, do we get an error and 404 here? I wonder if there's a way to know if the user has permissions or not
There was a problem hiding this comment.
@mickmister In the above case, we get no error and the value of found as false.
|
@hanzei added the comment for the above changes in the code. Please re-review |
hanzei
added
4: Reviews Complete
|
It looks like the screenshots changed the behavior of public vs ephemeral messages. Is this the case?
|
|
@mickmister Are you talking about the first case where the user doesn't have access to webhooks? If yes, the screenshot was missing the part for the public message. I have added a screenshot for both the cases again: |
|
@ayusht2810 The duplicated text between the public and ephemeral messages is a bit confusing. If there is an ephemeral message involved with the response, I think we should remove any duplicated text that also exists in the public post. If the entire message is duplicated, then we can just omit the ephemeral message in that case. What do you think? |
|
@mickmister Updated the messages: |
mickmister
added
3: QA Review
There was a problem hiding this comment.
This PR has been tested for the following scenarios:
- Checked the updated subscription message when user is not authorized to access webhooks
- Checked the updated subscription message when there is no webhook present
The PR was working fine for both the conditions. LGTM. Approved
Summary
Screenshots
Before
After
Before
After
Ticket Link
Fixes #715
Fixes #732
What to test?