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

Oppiabot should alert the author to address every review comment. #234

Open
SAEb-ai opened this issue Mar 28, 2021 · 6 comments
Open

Oppiabot should alert the author to address every review comment. #234

SAEb-ai opened this issue Mar 28, 2021 · 6 comments

Comments

@SAEb-ai
Copy link
Contributor

SAEb-ai commented Mar 28, 2021

Oppiabot should ping the author to address every review comment if the author mistakenly forgets to address it by leaving the keyword "Done" and after all review comments are addressed then only oppiabot should assign the reviewer . This will help the reviewer to review the PR properly.
For ref: See (oppia/oppia#11967 (comment))

But yep could this be achieved by oppiabot @jameesjohn @vojtechjelinek PTAL. If yes, does it sound good? If yes please do assign this to me. I know I have two issues already assigned to me but I can make a PR for them within 2-3 days.

@jameesjohn
Copy link
Contributor

This seems a bit complex since we would need to ensure that the comment actually needs to be addressed, and is not just a mere comment from the reviewer.

Can you prepare a design document for this?

What do you think @vojtechjelinek?

@SAEb-ai
Copy link
Contributor Author

SAEb-ai commented Mar 28, 2021

@jameesjohn I can prepare a design doc. But first I think that I have to get the bot running on my local repo which I couldn't currently. I have texted you also on hangouts about the problem. Could you please help me there?That problem is blocking me to introduce more stuffs here in order to improve oppiabot.

@jameesjohn
Copy link
Contributor

Yeah, sure

@vojtechjelinek
Copy link
Contributor

I don't think it is a priority for now.

@seanlip
Copy link
Member

seanlip commented Jan 6, 2023

I think Oppiabot could at least leave a note saying that "these comments weren't addressed" and link to those comments. I think this is still worth doing since I've seen some authors miss comment replies, especially in longer PRs.

@BenHenning
Copy link
Member

FWIW this has bit me a bunch as a reviewer in recent reviews as it happens almost always for the first couple PRs from a new contributor. On Oppia Android, we have the "require all conversations to be addressed" requirement enabled which I think complements a feature like this quite well.

I think the desirable flow would be:

  • Reviewer performs a review pass & sends PR back to author
  • Author responds to some or none of the comments, sends PR back
  • Oppiabot detects that some comments weren't replied to and automatically sends the PR back to the author*
  • Author addresses the comments in some way (either by saying 'Done', asking for help, or indicating they will address that comment after the next review pass--Oppiabot would provide guidance on these choices in its reply)
  • Reviewer confirms addressed comments have been addressed & resolves them
  • Once all comments are addressed & the PR is approved, it can be merged**

* I'd expect the logic here to be that Oppiabot only expects the PR author to have a comment dated after the most recent review pass from that comment's original author. Reviews are available through the API: https://docs.github.com/en/rest/pulls/reviews?apiVersion=2022-11-28#list-reviews-for-a-pull-request.

** This one is a bit trickier, but we really need an enforcement mechanism for comment threads that are marked as resolved by non-authors. This is probably a separate feature request, but I'd expect Oppiabot to reopen or at least warn on any comment threads that were closed by a non-author. This is indirectly supported by the API per: https://stackoverflow.com/questions/55713929, https://docs.github.com/en/rest/guides/working-with-comments?apiVersion=2022-11-28, and https://docs.github.com/en/rest/pulls/comments?apiVersion=2022-11-28.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

No branches or pull requests

5 participants