Implement PR reviewed checks #116

jameesjohn · 2020-08-20T09:58:50Z

Explanation

This PR implements the checks that are to be done when a PR gets reviewed.

Reviewer reviews and approves
- Bot unassigns reviewer and assigns next reviewer(s)(if any).
- If all reviewers have approved, bot checks if the user has merging rights and assigns user if that is the case or otherwise assigns one of the reviewers.
Reviewer reviews and requests changes
- Bot unassigns reviewer and assigns the user

This functionality has been tested at jameesjohn/comment-on-pr#8 (review)

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.

…viewed

ankita240796

Hi @jameesjohn, thanks for the PR! This looks great, took a pass at the main module, will be checking the specs in some time. PTAL, Thanks!

ankita240796 · 2020-08-21T14:50:05Z

lib/utils.js

@@ -17,6 +17,9 @@
 */

 const DATASTORE_LABEL = 'PR: Affects datastore layer';
+const LABELS_EXCLUDED_FROM_CODEOWNER_ASSIGNMENT = [


Add a brief comment to explain why we excluded them.

ankita240796 · 2020-08-21T14:50:23Z

lib/utils.js

+ * @param {String} changelogLabel
+ */
+const getProjectOwnerFromLabel = (changelogLabel) => {
+  const labelSubstrings = changelogLabel.split('@');


@ -> Constant

ankita240796 · 2020-08-21T14:51:02Z

lib/checkPullRequestReview.js

+const handleChangesRequested = async (context) => {
+  // Pause for 3 minutes in case reviewer wants to perform
+  // the required actions.
+  await utilityModule.sleep(60 * 1000 * 3);


Declare 3 minutes as constant.

ankita240796 · 2020-08-21T14:51:35Z

lib/checkPullRequestReview.js

+const LGTM_LABEL = 'PR: LGTM';
+
+/**
+ * This functions handles a changes requested review.


Explain what the function does to handle the changes requested review.

ankita240796 · 2020-08-21T14:52:03Z

lib/checkPullRequestReview.js

+   */
+  const review = context.payload.review;
+  // Fetch the pull request incase there has been any changes since
+  // the review was made.


What changes, do you mean in the 3 minutes we pause?

ankita240796 · 2020-08-21T15:00:05Z

lib/checkPullRequestReview.js

+    );
+  }
+
+  // Check if pull request has been approved by all reviewers.


Add a comment as to why we can't directly check from the pull request payload that it is approved by all reviewers.

ankita240796 · 2020-08-21T15:00:54Z

lib/checkPullRequestReview.js

+      username: pullRequest.user.login,
+    });
+
+    if (membershipCheckResponse.status === 204) {


204 -> constant

ankita240796 · 2020-08-21T15:01:18Z

lib/checkPullRequestReview.js

+    if (membershipCheckResponse.status === 204) {
+      // Assign author.
+      await context.github.issues.addAssignees(
+        context.repo({


We should create comment here too asking author to merge the PR.

ankita240796 · 2020-08-21T15:01:33Z

lib/checkPullRequestReview.js

+   * @type {import('probot').Octokit.PullsGetReviewResponse} review
+   */
+  const review = context.payload.review;
+  if (review.state === CHANGES_REQUESTED) {


Make the states as constants.

They are set as constants.

Ah sorry, my bad.

ankita240796 · 2020-08-21T15:02:46Z

lib/checkPullRequestReview.js

+    await context.github.issues.addLabels(
+      context.repo({
+        issue_number: pullRequest.number,
+        labels: [LGTM_LABEL],


One other thing - there are cases some times when the reviewer approves the PR but requests a minor change along with it (As I do sometimes on your PR :P). Is there any way to check that?

No, we may not be able to check this accurately.

Hmm, let us do one thing then - Add a note in comment for whoever is the final assignee to merge the PR: Please ensure that no comments are pending from the author's end.

ankita240796

Thanks @jameesjohn, took a full pass, PTAL.

lib/checkPullRequestReview.js

ankita240796 · 2020-08-22T15:31:43Z

lib/checkPullRequestReview.js

+    await context.github.issues.addLabels(
+      context.repo({
+        issue_number: pullRequest.number,
+        labels: [LGTM_LABEL],


Hmm, let us do one thing then - Add a note in comment for whoever is the final assignee to merge the PR: Please ensure that no comments are pending from the author's end.

ankita240796 · 2020-08-22T15:31:57Z

lib/checkPullRequestReview.js

+          body:
+            'Hi @' +
+            reviewer +
+            ', this PR is ready to be merged, PTAL. Thanks!',


Please make sure you merge the PR since the author does not have merging rights.

I'm not really sure about this considering the author/reviewer might need to wait for CI checks to be complete before merge.

ankita240796 · 2020-08-22T15:32:01Z

lib/checkPullRequestReview.js

+          body:
+            'Hi @' +
+            pullRequest.user.login +
+            ', this PR is ready to be merged, PTAL. Thanks!',


Please make sure you merge the PR.

I'm not really sure about this considering the author/reviewer might need to wait for CI checks to be complete before merge.

ankita240796 · 2020-08-22T15:34:08Z

spec/checkPullRequestReviewSpec.js

+        await robot.receive(payloadData);
+      });
+
+      it('should check review', () => {


check type of review

ankita240796 · 2020-08-22T15:40:20Z

spec/checkPullRequestReviewSpec.js

+        });
+      });
+
+      it('should ping remaining reviewers', () => {


One more - should not assign pr author.

ankita240796 · 2020-08-22T15:40:39Z

spec/checkPullRequestReviewSpec.js

+        expect(github.issues.addAssignees).not.toHaveBeenCalled();
+      });
+
+      it('should not ping remaining reviewers', () => {


ankita240796 · 2020-08-22T15:40:54Z

spec/checkPullRequestReviewSpec.js

+      });
+
+      it('should not ping remaining reviewers', () => {
+        expect(github.issues.createComment).not.toHaveBeenCalledTimes(2);


Why 2 times?

only one comment should be added (that of the PR author).

ankita240796 · 2020-08-22T15:41:23Z

spec/checkPullRequestReviewSpec.js

+        });
+      });
+
+      it('should ping remaining reviewers', () => {


Should not assign author case.

ankita240796 · 2020-08-22T15:43:59Z

spec/checkPullRequestReviewSpec.js

+      });
+    });
+
+    describe('when reviewer already unassigns themselves', () => {


Is the description correct?

I believe so. I can modify it to when reviewer is not assigned.

ankita240796 · 2020-08-22T22:35:02Z

lib/checkPullRequestReview.js

+          issue_number: pullRequest.number,
+          body:
+            'Hi @' +
+            reviewer +


...this PR is ready to be merged. We are assigning you since the author does not have merging rights. Please make sure...

ankita240796 · 2020-08-22T22:35:27Z

lib/checkPullRequestReview.js

+   * @type {import('probot').Octokit.PullsGetReviewResponse} review
+   */
+  const review = context.payload.review;
+  if (review.state === CHANGES_REQUESTED) {


Ah sorry, my bad.

ankita240796 · 2020-08-22T22:39:59Z

spec/checkPullRequestReviewSpec.js

+      });
+    });
+
+    describe(' when reviewer is not assigned.', () => {


Remove extra space after starting quote.

Also, shouldn't this be: when all reviewers have approved and none are assigned?

Yep. Updated.

ankita240796 · 2020-08-22T22:42:34Z

spec/checkPullRequestReviewSpec.js

+    });
+
+    describe(
+      'when reviewer is last reviewer and already adds the lgtm label', () => {


One more case - LGTM label is not added, but one of the reviewers is already assigned - are we handling that?

If the LGTM label is not added, we add the label. If one of the reviewers is already assigned, I'll update the code to not assign any other person.

oppiabot · 2020-08-24T11:43:37Z

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!

ankita240796 · 2020-08-25T14:34:11Z

lib/utils.js

+ */
+const getChangelogLabelFromPullRequest = (pullRequest) => {
+  const label = pullRequest.labels.find((label) =>
+    label.name.toUpperCase().startsWith('PR CHANGELOG')


Make PR CHANGELOG a constant.

ankita240796 · 2020-08-25T14:37:42Z

lib/checkPullRequestLabels.js

- * @param {import('probot').Octokit.PullsGetResponse} pullRequest
- * @param {String} changelogLabel
- */
-const canAssignProjectOwner = (pullRequest, changelogLabel) => {


Why are we moving this to utils?

I think this was done before I decided to assign the last reviewer. I'll move it back here.

ankita240796 · 2020-08-25T14:38:30Z

lib/utils.js

+ * @param {import('probot').Octokit.PullsGetResponse} pullRequest
+ * @param {String} changelogLabel
+ */
+const canAssignProjectOwner = (pullRequest, changelogLabel) => {


Is this function being used anywhere else other than check pull req label module?

No, Removed.

ankita240796 · 2020-08-25T14:39:34Z

lib/checkPullRequestReview.js

+      );
+    }
+  } else {
+    // The pull request has been approved, and we need to add the LGTM label.


Since the pull request has been approved, we add the LGTM label.

ankita240796 · 2020-08-25T14:43:13Z

lib/checkPullRequestReview.js

+    // The last reviewer got unassigned, but the pull request assignees list
+    // does not reflect that since we did not refetch the pull request data,
+    // hence if any other person is assigned to the pull request, they are
+    // likely the one to merge it.
+    console.log({assignees})


We can remove the console.log, I think.

Also, we should add a comment before returning, asking the assignee for merging.

And why not just check membership of the assignees instead of comparing them to reviewer?

We can remove the console.log, I think.

Removed.

And why not just check membership of the assignees instead of comparing them to reviewer?

I believe all reviewers should have merging rights, right?

Also, we should add a comment before returning, asking the assignee for merging.

Comment added.

ankita240796 · 2020-08-25T14:43:28Z

lib/checkPullRequestReview.js

+  );
+  const pullRequest = pullRequestResponse.data;
+
+  if (pullRequest.state === 'closed') {


closed -> constant.

ankita240796 · 2020-08-25T14:44:52Z

spec/checkPullRequestReviewSpec.js

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


One more case - not assign PR author.

ankita240796 · 2020-08-25T14:45:40Z

spec/checkPullRequestReviewSpec.js

+    });
+
+    describe('when all reviewers have approved and none are assigned.', () => {
+      // Note that when the reviewer is the last reviewer, the list of


Can you add an example - comment isn't that clear.

I'm not sure how the example to add since this can only be noticed from the payload. I've modified the comment though.

ankita240796 · 2020-08-25T14:46:02Z

spec/checkPullRequestReviewSpec.js

+      });
+    });
+
+    describe('when all reviewers have approved and none are assigned.', () => {


and pr author cannot merge

ankita240796

Final comments, Thanks @jameesjohn!

ankita240796 · 2020-08-25T21:58:19Z

lib/checkPullRequestReview.js

+  const searchResult = await context.github.search.issuesAndPullRequests(
+    context.repo({
+      q:
+        'repo:' +


Make this into constant.

ankita240796 · 2020-08-25T21:58:24Z

lib/checkPullRequestReview.js

+      q:
+        'repo:' +
+        context.payload.repository.full_name +
+        ' review:approved ' +


ankita240796 · 2020-08-25T21:59:24Z

lib/checkPullRequestReview.js

+    })
+  );
+  const pullRequest = pullRequestResponse.data;
+  const CLOSED_STATE = 'closed';


Make it a top level constant.

ankita240796 · 2020-08-25T22:04:50Z

lib/checkPullRequestReview.js

+          body:
+            'Hi @' +
+            assignees[0] +
+            ', this PR is ready to be merged. We are assigning you since ' +


assigning -> pinging

ankita240796 · 2020-08-27T06:17:23Z

lib/checkPullRequestReview.js

+    const labels = pullRequest.labels.map((label) => label.name);
+    if (labels.includes(LGTM_LABEL)) {
+      // Do nothing and end execution if label has already been added.
+      return;


What if label is added but no one is assigned?

Modified to continue check.

ankita240796 · 2020-08-27T06:19:40Z

lib/checkPullRequestReview.js

+    // The last reviewer got unassigned, but the pull request assignees list
+    // does not reflect that since we did not refetch the pull request data,
+    // hence if any other person is assigned to the pull request, they are
+    // likely the one to merge it.
+
+    if (assignees.some((user) => user !== reviewer)) {
+      const remainingAssignee = assignees.find(user => user !== reviewer);
+      await context.github.issues.createComment(
+        context.repo({
+          issue_number: pullRequest.number,
+          body:
+            'Hi @' +
+            remainingAssignee +
+            ', this PR is ready to be merged. We are pinging you since ' +
+            'you are the remaining assingee on this PR. Please make sure ' +
+            "there are no pending comments from the author's end before " +
+            'merge. Thanks!',
+        })
+      );
+      return;


This is a bit confusing - can we be sure that the assignee has merging rights?

Why not do this instead - get list of assignees and check if any one of them has merging rights.

If yes - leave a comment asking them to do the merge.
If no - assign the reviewer.

This is simpler and more readable as well.

Following discussions offline, we resolved to only assign the project owner if the author does not have merging rights since we are sure that all pull requests will have a changelog label.

ankita240796

Minor comments, no major concerns now. PTAL, Thanks @jameesjohn!

ankita240796 · 2020-08-28T14:37:31Z

lib/checkPullRequestReview.js

+ * 5. Assign other reviewers if any.
+ * 5. If reviewer was the last reviewer, check if pull request has been
+ *    approved, and add the LGTM Label.
+ * 6. If pull request has been approved, assign PR author or last reviewer


has been approved -> has LGTM label

ankita240796 · 2020-08-28T14:52:52Z

lib/checkPullRequestReview.js

+    } else {
+      // Assign project owner to merge the pull request.
+      // A pull request will always have a changelog label because we are
+      // assigning the PR author to the pull request on creation if created


assigning the PR author to the pull request on creation if created without a changelog label --> assigning the PR author to the pull request if it is created without changelog label

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

spec/checkPullRequestReviewSpec.js

+        });
+      });
+
+      it('should ping one of the reviewers to merge', () => {


one of the reviewers -> project owner

ankita240796 · 2020-08-28T14:58:58Z

spec/checkPullRequestReviewSpec.js

+    });
+
+    describe(
+      'when reviewer is the last reviewer and pr author can not merge', () => {


Why is this different from previous case?

It was part of the previous implementation.
Removed.

ankita240796

LGTM, Thanks @jameesjohn!

jameesjohn added 5 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

jameesjohn requested a review from ankita240796 August 20, 2020 09:59

jameesjohn assigned ankita240796 Aug 20, 2020

ankita240796 suggested changes Aug 21, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Aug 21, 2020

jameesjohn added 2 commits August 22, 2020 15:25

address review comments

4e76ebb

address review comments

8a9ab85

jameesjohn assigned ankita240796 and unassigned jameesjohn Aug 22, 2020

ankita240796 suggested changes Aug 22, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Aug 22, 2020

address review comments

2e73f9a

jameesjohn assigned ankita240796 and unassigned jameesjohn Aug 22, 2020

jameesjohn requested a review from ankita240796 August 22, 2020 19:02

ankita240796 suggested changes Aug 22, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Aug 22, 2020

Address review comments.

e444f95

jameesjohn assigned ankita240796 and unassigned jameesjohn Aug 24, 2020

jameesjohn requested a review from ankita240796 August 24, 2020 10:51

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

ankita240796 suggested changes Aug 25, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Aug 25, 2020

address review comments

df10f49

jameesjohn requested a review from ankita240796 August 25, 2020 16:26

jameesjohn assigned ankita240796 and unassigned jameesjohn Aug 25, 2020

address review comments

15934ff

ankita240796 mentioned this pull request Aug 25, 2020

Periodic Checks #119

Merged

2 tasks

ankita240796 suggested changes Aug 25, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Aug 25, 2020

address review comments

020588d

jameesjohn assigned ankita240796 and unassigned jameesjohn Aug 26, 2020

jameesjohn mentioned this pull request Aug 26, 2020

User Response to Pull Request Review #121

Merged

2 tasks

ankita240796 suggested changes Aug 27, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Aug 27, 2020

address review comments

8bd213b

jameesjohn requested a review from ankita240796 August 27, 2020 21:02

jameesjohn assigned ankita240796 and unassigned jameesjohn Aug 27, 2020

ankita240796 suggested changes Aug 28, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Aug 28, 2020

address review comments

26aa079

ankita240796 approved these changes Aug 28, 2020

View reviewed changes

jameesjohn merged commit f8057de into oppia:master Aug 28, 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

Implement PR reviewed checks #116

Implement PR reviewed checks #116

Conversation

jameesjohn commented Aug 20, 2020

Explanation

Checklist

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

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

oppiabot bot commented Aug 24, 2020

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

jameesjohn Aug 25, 2020 • edited Loading

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

jameesjohn Aug 25, 2020 •

edited

Loading