User Response to Pull Request Review #121

jameesjohn · 2020-08-26T17:07:43Z

Explanation

This PR adds checks for user responding to user reviews.
This feature has been tested at jameesjohn/oppia#39.

Checklist

I have successfully deployed my own instance of Oppiabot.
- You can find instructions for doing this here.
I have manually tested all the changes made in this PR following the manual tests matrix.

This PR is stacked on #116 and new changes are from 9e51c98

…viewed

oppiabot · 2020-08-28T19:45:39Z

Hi @jameesjohn. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

…view-response

ankita240796

Hi @jameesjohn, took a pass, PTAL. Thanks!

ankita240796 · 2020-08-28T15:30:09Z

constants.js

@@ -8,7 +8,9 @@ const editEvent = 'edited';
 const issuesLabelEvent = 'issues_labeled';
 const issuesAssignedEvent = 'issues_assigned';
 const pushEvent = 'push';
+const pullRequestReviewEvent = 'pr-review';


pr_review just to be consistent.

ankita240796 · 2020-08-29T04:29:33Z

lib/checkPullRequestReview.js

@@ -287,6 +288,75 @@ const handlePullRequestReview = async (context) => {
  }
 };

+/**
+ * This function checks when a pull request gets commented on by the PR author.


when a pull request gets commented on by the PR author --> when PR author comments on the pull request

ankita240796 · 2020-08-29T04:30:23Z

lib/checkPullRequestReview.js

@@ -287,6 +288,75 @@ const handlePullRequestReview = async (context) => {
  }
 };

+/**
+ * This function checks when a pull request gets commented on by the PR author.
+ * If pr author comments on the PR asking reviewers please to take a look (ptal),


please to take a look (ptal) --> to please take a look (PTAL)

We will need to update wiki as well to have this info and how oppiabot responds.

Yes. This is contained in the doc, and I believe that will be shared with the dev community.

ankita240796 · 2020-08-29T04:30:54Z

lib/checkPullRequestReview.js

+/**
+ * This function checks when a pull request gets commented on by the PR author.
+ * If pr author comments on the PR asking reviewers please to take a look (ptal),
+ * We check if the reviewers have been assigned and if not, assign them.


We check if the reviewers have been assigned and if not, assign them --> the function checks if the reviewers have been assigned and if not, assigns them

ankita240796 · 2020-08-29T10:32:29Z

lib/checkPullRequestReview.js

+   * @type {import('probot').Octokit.IssuesGetResponse}
+   */
+  const pullRequest = context.payload.issue;
+  const isPullRequest = 'pull_request' in pullRequest;


pull_request -> constant

ankita240796 · 2020-08-29T10:35:23Z

lib/checkPullRequestReview.js

+
+  // Pause for 3 minutes in case author wants to perform
+  // the required actions.
+  await utilityModule.sleep(THREE_MINUTES);


One suggestion regarding the 3 minutes thing - we should also add this to the case when we ask author to assign the reviewers if they are the codeowners - I have noticed that the comment pops really quick even when I am about to assign the reviewer. This one to be specific: https://github.com/oppia/oppiabot/blob/master/lib/checkPullRequestLabels.js#L117. You can update it here or in a separate PR as well.

Do you think the 3 minutes wait time is needed here?
The response here should be immediate I believe.

See this for example, I already assigned a reviewer but oppiabot still leaves a comment. So, we can keep the wait time shorter but we should definitely have it. WDYT?

Okay.
I'll add it in a separate PR where I'll also be adding features for the new default changelog label.

Sounds good, thanks!

ankita240796 · 2020-08-29T10:36:25Z

lib/checkPullRequestReview.js

+      issue_number: updatedPullRequest.number,
+      assignees: unassignedUsers,
+    })
+  );


Add comment too - Hi @ .. assigning you since author requested for a review. Thanks!

I'm not sure we should still comment here since we are reacting to a comment already made by the pr author.

Hmm, makes sense. But let us unassign the author as well.

spec/checkPullRequestReviewSpec.js

ankita240796 · 2020-08-30T14:31:58Z

lib/checkPullRequestReview.js

@@ -287,6 +288,76 @@ const handlePullRequestReview = async (context) => {
  }
 };

+/**
+ * This function checks when PR author comments on the pull request.


checks when -> handles the case when a

ankita240796 · 2020-08-30T14:32:19Z

lib/checkPullRequestReview.js

@@ -287,6 +288,76 @@ const handlePullRequestReview = async (context) => {
  }
 };

+/**
+ * This function checks when PR author comments on the pull request.
+ * If pr author comments on the PR asking reviewers please to take a look (PTAL),


please to take a look (PTAL) -> to please take a look (PTAL)

ankita240796 · 2020-08-30T14:32:44Z

lib/checkPullRequestReview.js

+/**
+ * This function checks when PR author comments on the pull request.
+ * If pr author comments on the PR asking reviewers please to take a look (PTAL),
+ * The function checks if the reviewers have been assigned and if not, assigns them.


Line too long - break.

ankita240796 · 2020-08-30T14:33:14Z

lib/checkPullRequestReview.js

+
+  const usersInComment = utilityModule.getUsernamesFromText(comment.body);
+  // Check if assignment was carried out by author.
+  const assignees = updatedPullRequest.assignees.map((assignee) => assignee.login);


Break if line too long.

ankita240796 · 2020-08-30T14:33:26Z

lib/checkPullRequestReview.js

+  const unassignedUsers = usersInComment.filter((user) =>
+    !assignees.includes(user)
+  );
+  console.log({ unassignedUsers });


We don't need this.

ankita240796 · 2020-08-30T14:34:00Z

lib/checkPullRequestReview.js

+      issue_number: updatedPullRequest.number,
+      assignees: unassignedUsers,
+    })
+  );


Hmm, makes sense. But let us unassign the author as well.

ankita240796 · 2020-08-30T14:34:37Z

spec/checkPullRequestReviewSpec.js

+          });
+        });
+
+        it('should assign remaining reviewer', () => {


reviewer -> reviewers

spec/checkPullRequestReviewSpec.js

ankita240796 · 2020-08-30T14:36:02Z

spec/checkPullRequestReviewSpec.js

+    });
+
+    describe(
+      'when the comment is from an issue and not a pull request', () => {


ankita240796 · 2020-08-31T01:52:51Z

lib/checkPullRequestReview.js

+
+  // Pause for 3 minutes in case author wants to perform
+  // the required actions.
+  await utilityModule.sleep(THREE_MINUTES);


See this for example, I already assigned a reviewer but oppiabot still leaves a comment. So, we can keep the wait time shorter but we should definitely have it. WDYT?

ankita240796 · 2020-08-31T01:55:53Z

lib/checkPullRequestReview.js

+    (assignee) => assignee.login === updatedPullRequest.user.login
+  );
+  if (authorIsAssigned) {
+    context.github.issues.removeAssignees(


Add a comment too - we have unassigned you since a review was requested - please make sure all comments are addressed.

Comment should be something like unassigning @author since a re-review was requested - please make sure all comments are addressed.

ankita240796 · 2020-08-31T01:56:21Z

spec/checkPullRequestReviewSpec.js

    });
+
+    describe(
+      'when author asks reviewer to ptal and does not assign them and author is assigned', () => {


Break line - please check the complete file.

ankita240796

Just a few more nits, PTAL. Thanks @jameesjohn!

ankita240796 · 2020-08-31T12:29:27Z

lib/checkPullRequestReview.js

+
+  // Pause for 3 minutes in case author wants to perform
+  // the required actions.
+  await utilityModule.sleep(THREE_MINUTES);


Sounds good, thanks!

ankita240796 · 2020-08-31T12:29:45Z

lib/checkPullRequestReview.js

+    (assignee) => assignee.login === updatedPullRequest.user.login
+  );
+  if (authorIsAssigned) {
+    context.github.issues.removeAssignees(


ankita240796 · 2020-08-31T12:30:55Z

lib/checkPullRequestReview.js

+      context.repo({
+        issue_number: updatedPullRequest.number,
+        body:
+          'Unassigning @author since a re-review was requested ' +


This should be @{updatedPullRequest.user.login}.

`Unassigning @{author} since a re-review was requested. @{author} Please make sure you have addressed all the review comments. Thanks!'

Done.
Thanks

ankita240796

LGTM, Thanks @jameesjohn!

jameesjohn added 16 commits August 18, 2020 12:00

initial work here

4c0681b

complete pr review check

22546d9

Write tests for pull request review

f946925

add license text

79fbf8d

Merge branch 'master' of https://github.com/oppia/oppiabot into pr-re…

85104e1

…viewed

address review comments

4e76ebb

address review comments

8a9ab85

address review comments

2e73f9a

Address review comments.

e444f95

Merge branch 'master' of https://github.com/oppia/oppiabot into pr-re…

1c023d7

…viewed

merge

84c84ce

remove unnecessary files

d376fc1

address review comments

df10f49

address review comments

15934ff

address review comments

020588d

complete work on user response to review

9e51c98

jameesjohn requested a review from ankita240796 August 26, 2020 17:08

jameesjohn assigned ankita240796 Aug 26, 2020

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Aug 28, 2020

oppiabot bot assigned jameesjohn Aug 28, 2020

Merge branch 'master' of https://github.com/oppia/oppiabot into pr-re…

5ed5579

…view-response

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Aug 28, 2020

jameesjohn removed their assignment Aug 28, 2020

ankita240796 suggested changes Aug 29, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Aug 29, 2020

address review comments

3e9bba5

ankita240796 suggested changes Aug 30, 2020

View reviewed changes

address review comments

c530d60

ankita240796 suggested changes Aug 31, 2020

View reviewed changes

jameesjohn assigned ankita240796 and unassigned ankita240796 Aug 31, 2020

address review comments

62aed72

jameesjohn assigned ankita240796 and unassigned jameesjohn Aug 31, 2020

ankita240796 suggested changes Aug 31, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Aug 31, 2020

address review comments

3f98531

jameesjohn assigned ankita240796 and unassigned jameesjohn Aug 31, 2020

ankita240796 approved these changes Aug 31, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Aug 31, 2020

ankita240796 merged commit 6164c43 into oppia:master Aug 31, 2020

ankita240796 unassigned jameesjohn Aug 31, 2020

ankita240796 mentioned this pull request Sep 3, 2020

Assign the code owners for review to a PR once the project owner adds the “Need code owners approval” label #35

Closed

User Response to Pull Request Review #121

User Response to Pull Request Review #121

Conversation

jameesjohn commented Aug 26, 2020

Explanation

Checklist

oppiabot bot commented Aug 28, 2020

ankita240796 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankita240796 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankita240796 left a comment

Choose a reason for hiding this comment