mattermost / mattermost-plugin-github

GitHub plugin for Mattermost
Apache License 2.0
156 stars 146 forks source link

Update subscription messages with respect to webhook #770

Closed ayusht2810 closed 3 weeks ago

ayusht2810 commented 3 months ago

Summary

Screenshots

Ticket Link

Fixes https://github.com/mattermost/mattermost-plugin-github/issues/715 Fixes https://github.com/mattermost/mattermost-plugin-github/issues/732

What to test?

ayusht2810 commented 3 months ago

@hanzei added the comment for the above changes in the code. Please re-review

mickmister commented 2 months ago

It looks like the screenshots changed the behavior of public vs ephemeral messages. Is this the case?

image

ayusht2810 commented 2 months ago

@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: Case 1: image Case 2: image

mickmister commented 2 months ago

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

ayusht2810 commented 2 months ago

@mickmister Updated the messages: image Let me know if anything else needs to be changed here.