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

feat(options): don't remind if change requested #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(options): don't remind if change requested #4

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/github.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,8 @@ github.prototype = {
return false;


if(b.state == "APPROVED")
return true;
if(b.state == "APPROVED" || b.state == "CHANGES_REQUESTED")
return true;

if(b.submitted_at < pullRequest.updated_at)
Copy link
Owner

Choose a reason for hiding this comment

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

@leizmonk this won't work .. If you requested changes you shouldn't be notified unless your changes were addressed by the author .. checking this is complicated and so line 153 makes sure that you are notified if the pull request has changed after you submitted a review and does not notify you otherwise ...

If we wish to enhance it we need to think of a way to know that these changes address the reviewer requested changes and not those of another reviewer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ahmadposten if line 153 is intended to not notify you if the pull request is in "Change requested" status and the author of the PR hasn't addressed the request, it's not working properly. Engineers on my team were getting notifications on PRs they had been requested to review even after requesting changes of the PR author.

Because there's no consistent way to guarantee that a PR author addressed the changes requested reviewer (ex: on my team people usually @ the reviewer on the review comment thread when they've pushed a change to address the reviewer's feedback, but that might not be the case on other teams), I might just close this PR.

return false;
Expand Down