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.

Sign up for GitHub

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

Jump to bottom

[GH-802] Added config setting for getting notification for draft PRs #804

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[GH-802] Added config setting for getting notification for draft PRs #804

wants to merge 2 commits into from

Conversation

raghavaggarwal2308
Copy link
Contributor

@raghavaggarwal2308 raghavaggarwal2308 commented Jul 15, 2024 •
edited
Loading

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:
  • Getting a single for notification regardless of the PR state (i.e. draft/ready).
  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
wiggin77
wiggin77 reviewed Jan 15, 2025
View reviewed changes
plugin.json Outdated Show resolved Hide resolved
wiggin77
wiggin77 reviewed Jan 15, 2025
View reviewed changes
plugin.json Outdated Show resolved Hide resolved
wiggin77
wiggin77 reviewed Jan 15, 2025
View reviewed changes
plugin.json Outdated Show resolved Hide resolved
wiggin77
wiggin77 approved these changes Jan 15, 2025
View reviewed changes
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
Contributor 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wiggin77 wiggin77 wiggin77 approved these changes

@ayusht2810 ayusht2810 ayusht2810 approved these changes

Assignees

@raghavaggarwal2308 raghavaggarwal2308

Labels
2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester
Projects
None yet
Milestone
v2.4.0
Development

Successfully merging this pull request may close these issues.

Make the new Draft PR behavior configurable
6 participants
@raghavaggarwal2308 @ThiefMaster @wiggin77 @ayusht2810 @hanzei @Kshitij-Katiyar