Skip to content
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

Delaying Draft Pull Request Notifications until Marked as 'Ready for Review' #682

Merged
merged 5 commits into from
Aug 17, 2023

Conversation

abdulsmapara
Copy link
Contributor

@abdulsmapara abdulsmapara commented Jun 11, 2023

Summary

Delaying Draft Pull Request Notifications until Marked as 'Ready for Review'

Requirement addressed:
  • When a draft PR is submitted, an on-line notification gets posted.
  • When a draft PR is marked as ready-for-review, a "standard" PR notification with the full body gets posted.
Description

When draft PR created, following message is posted, without PR description/body =>
image

When PR marked ready for review, following message is posted, which includes PR description/body =>
image

When PR directly created with ready for review, following message is posted, which includes PR description/body =>
image

Ticket Link

#680

@mattermost-build
Copy link
Contributor

Hello @abdulsmapara,

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.

@abdulsmapara abdulsmapara marked this pull request as ready for review June 17, 2023 07:02
@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Jun 20, 2023
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution 🎉

server/plugin/template.go Outdated Show resolved Hide resolved
server/plugin/template.go Outdated Show resolved Hide resolved
server/plugin/template.go Outdated Show resolved Hide resolved
@hanzei hanzei added the 3: QA Review Requires review by a QA tester label Jun 20, 2023
@abdulsmapara abdulsmapara requested a review from hanzei June 21, 2023 18:12
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work 👍

server/plugin/webhook.go Show resolved Hide resolved
@hanzei hanzei requested a review from crspeller June 23, 2023 13:58
@crspeller crspeller removed the 2: Dev Review Requires review by a core committer label Jun 27, 2023
@mattermost-build
Copy link
Contributor

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!

@abdulsmapara
Copy link
Contributor Author

@DHaussermann, requesting you to please review the PR.
cc: @hanzei

@abdulsmapara
Copy link
Contributor Author

@DHaussermann, any updates on the PR review ?
cc: @hanzei, @crspeller

@DHaussermann
Copy link

Sorry @abdulsmapara for the delay. I will follow-up to find a testing resource for this PR.

@abdulsmapara
Copy link
Contributor Author

no problem @DHaussermann
Thanks for the update.

@abdulsmapara
Copy link
Contributor Author

Any update @DHaussermann ?

cc: @hanzei ,@crspeller

@crspeller crspeller requested review from lindalumitchell and removed request for DHaussermann August 8, 2023 14:40
@crspeller
Copy link
Member

crspeller commented Aug 8, 2023

@lindalumitchell Do you have some time to test this PR?

@lindalumitchell
Copy link

@crspeller yes, I should be able to; I know Dylan is out next week as well. I'm unsure how to build a test version of the plugin though; can you help?

@hanzei
Copy link
Contributor

hanzei commented Aug 14, 2023

/update-branch

@hanzei
Copy link
Contributor

hanzei commented Aug 14, 2023

@lindalumitchell You can download the bundle bundle at https://github.com/mattermost/mattermost-plugin-github/actions/runs/5855120323?pr=682.

Copy link

@lindalumitchell lindalumitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the further delay on this review; I hadn't tested this area before. But I got it all set up and tested and it looks great! Thanks very much for the contribution!

Verified as described that the simple notification posts on draft PR creation, and full notification posts when marked Ready for Review. Also smoke tested around other actions and notifications with all behaving as expected and no issues found. 👍

@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed Lifecycle/1:stale 3: QA Review Requires review by a QA tester labels Aug 17, 2023
@hanzei hanzei added this to the v2.2.0 milestone Aug 17, 2023
@hanzei hanzei merged commit 896ccfd into mattermost:master Aug 17, 2023
9 checks passed
@hanzei
Copy link
Contributor

hanzei commented Aug 17, 2023

Thanks @abdulsmapara 🚀

@ThiefMaster
Copy link

Could this please be made configurable?

I'd much rather have it show the full message when the PR is opened, and nothing at all when it's changed from/to draft (like it was before this PR).

In particular, it's spammy right now when someone opens the PR, then converts it to draft (because they forgot), and then back to a regular one shortly after (that results in a total of two "full" messages and one small).

I think this change would be more bearable if the plugin kept track of whether a PR was in "ready" state before so the bull message is only sent once. But even then I'd rather have a setting to disable the whole functionality, because depending on the project, Draft PR just means that there's still some stuff left for discussion, but that people still want to be properly informed about the PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants