Skip to content

Conversation

@raghavaggarwal2308
Copy link

@raghavaggarwal2308 raghavaggarwal2308 commented Jul 15, 2024

Summary

  • Added config setting for getting notification for draft PRs
  • Add config setting to make the changes from this PR configurable.

Screenshot:

image

What to test:

  1. When setting is disabled:
  • No notification for draft pull request.
  • Detailed notification for normal pull request
  1. When setting is enable:
  • Getting a notification with less details when a draft PR is created.
  • Getting a notification with complete details when a draft PR is marked as ready for review.
  • Getting a notification with complete details for a normal PR.

How to test:

  1. Connect you github account.
  2. Create subscription for "pulls" in desired channel
  3. Trigger the desired event from github.

Ticket Link

Fixes #802

@raghavaggarwal2308 raghavaggarwal2308 self-assigned this Jul 15, 2024
@raghavaggarwal2308 raghavaggarwal2308 added this to the v2.4.0 milestone Jul 15, 2024
@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Jul 31, 2024
@raghavaggarwal2308 raghavaggarwal2308 added the 3: QA Review Requires review by a QA tester label Aug 29, 2024
Copy link
Member

@wiggin77 wiggin77 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Just some small wording changes requested.

@ThiefMaster
Copy link

The PR does not really match the issue nor the description... the whole issue was about being able to get notified with full details when the PR is opened regardless of draft state. But currently your setting gives you only the choice between:

  • no notifications at all for draft PRs
  • short notification for draft PRs + full notification for normal Prs and when marked as ready

It does not cover the case of people using draft PRs because there are open discussion points etc, but otherwise already being relevant!

@raghavaggarwal2308
Copy link
Author

@ThiefMaster Can you please clarify what exactly do we need here?

  1. Do you want us to get a single notification when a PR (draft/normal) is created and not notification after that?
  2. What is the config setting that we need? How should it behave in the on/off state?
    cc: @wiggin77

@AayushChaudhary0001
Copy link

While testing this PR following issue is found:

  • When the notification setting for draft PRs is set to "False", notifications are still being triggered including all PR details which is not correct.

@Kshitij-Katiyar
Copy link
Contributor

@AayushChaudhary0001 I have fixed the issues reported. Please review

@AayushChaudhary0001
Copy link

I found a scenario while retesting in which no notifications are triggered when plugin config setting for draft PR is disabled and a draft PR is marked for review with complete details.

image

no notifications received:-

image

@Kshitij-Katiyar
Copy link
Contributor

I found a scenario while retesting in which no notifications are triggered when plugin config setting for draft PR is disabled and a draft PR is marked for review with complete details.

image

no notifications received:-

image

@wiggin77 I think we should get a notification whenever a PR is marked readyForReview, no matter the config setting.
What is your opinion on this?

@wiggin77
Copy link
Member

@wiggin77 I think we should get a notification whenever a PR is marked readyForReview, no matter the config setting.
What is your opinion on this?

Yes, I agree.

@Kshitij-Katiyar
Copy link
Contributor

@AayushChaudhary0001 Please re-test this PR

Copy link

@AayushChaudhary0001 AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

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

The PR is tested and code changes are working fine on adding config settings for notifications for draft PRs.
LGTM, Approved.

@AayushChaudhary0001 AayushChaudhary0001 merged commit fbb3836 into master May 19, 2025
11 checks passed
@AayushChaudhary0001 AayushChaudhary0001 deleted the MM-802 branch May 19, 2025 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make the new Draft PR behavior configurable

8 participants