-
Notifications
You must be signed in to change notification settings - Fork 154
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
GIthub Discussions and Discussion Comments Webhooks #808
GIthub Discussions and Discussion Comments Webhooks #808
Conversation
Hello @elewis787, 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. Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project. Please help complete the Mattermost contribution license agreement? This is a standard procedure for many open source projects. Please let us know if you have any questions. We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team. |
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.
Good work on the PR! No specific complaints but there are a few typos around.
Can you get some screenshots and add those to the PR summary?
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Co-authored-by: Felipe Martin <[email protected]>
Thanks @fmartingr. Apologies for the spelling errors! I have attached screenshots and a demo org/repository. |
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.
Thanks for the submission! I will request someone else's reviews to have more eyes on this and make sure it adheres to the plugin guidelines.
@elewis787 Can you check the lint errors? |
I pushed a fix for the indentation error in the const block. I'm looking into if I can retrigger the ci lint check to verify I caught all of them. I am not seeing any other changes locally. |
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.
@elewis787 Great work on this PR. Just added a few code refactoring comments
@fmartingr There are already four other people who review the PR. If you don't mind, I prefer not to review the PR. Is that fine with you? |
@elewis787 Yes, I think we can revert that |
I have push a reverted package-lock.json for the e2e tests. |
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.
Great work @elewis787 👍
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
Anything I can do to help push this forward? |
Hey @elewis787 , |
Thank you! No rush. Please reach out if there is anything I can do to help. |
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.
This PR has been tested and looks good. All the notifications regarding the new discussion creation and discussion comments are received on Mattermost. Approved.
Note:- Please add the change regarding the events(discussions
, discussion comments
) to be enabled in Github webhook for notifications of discussions and discussion comments in the readme file.
@elewis787 Can you create a PR in the MM docs https://github.com/mattermost/docs repo for the documentation changes? I think the webhook configurations needed to be updated for this PR to work. |
Absolutely! I'll work on a write up. |
@elewis787 Thanks! Please let us know when the PR is created. We can merge this PR after that |
@raghavaggarwal2308 here are the minimal doc updates mattermost/docs#7371. Please let me know if more details are needed. |
@elewis787 Awesome work on this PR. Thank you for your contribution. |
@raghavaggarwal2308 Thanks for all of your help with the reviews and direction! I am excited to see this merged. |
Summary
This pr adds basic support for GitHub discussions and discussion comment events.
Ticket Link
If this pull request addresses a Help Wanted ticket #480
Notes
I would love to hear thoughts on the template for both discussions and discussion comments. I did not add additional style or attempt to pull out a lot of information but I am happy to update the template as needed.
I also struggled with the playwrite test. If helpful, I am happy to further debug them and ensure they are working. I tested this manually using smee.io for webhook forwarding to a local deployment. I replayed the webhooks while testing.
test where done by integrated https://github.com/orgs/elewis787-org/discussions
I have attached a few screenshots to help demonstrate the functionality