Periodic Checks #119

jameesjohn · 2020-08-25T15:57:11Z

Explanation

Setup periodic checks to be carried out by oppiabot.

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.
I have not manually tested all the new additions here since I was not able to create the scenario. However, other features have been tested at Update README.md jameesjohn/comment-on-pr#8, sample jameesjohn/comment-on-pr#11, Testing CLA check in Probot upgrade jameesjohn/comment-on-pr#14.

…dic-checks

ankita240796

Took a first pass, PTAL @jameesjohn!

ankita240796 · 2020-08-25T21:46:30Z

index.js

            break;
        }
      }
    }
  }
-}
+};


index.js

ankita240796 · 2020-08-25T21:48:11Z

lib/utils.js

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


Change to constant.

ankita240796 · 2020-08-25T21:48:14Z

lib/utils.js

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


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

lib/utils.js

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


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

lib/periodicChecks.js

+ * @param {import('probot').Context} context
+ * @param {import('probot').Octokit.PullsGetResponse} pullRequest
+ */
+const handleApproval = async (context, pullRequest) => {


I am a bit confused - what is the purpose here - don't we have a similar method in #116 as well?

Similar, yes. But that in #116, the function is called after a pull request gets approved.
This is function is called when oppiabot goes through all pull requests and finds that one has been approved but has no assignee.

ankita240796 · 2020-08-25T21:55:06Z

lib/periodicChecks.js

+ * 5. Ping appropriate assignee.
+ * @param {import('probot').Context} context
+ */
+const ensurePullRequestIsAssigned = async (context) => {


Can we do one other thing here - log the assignees/reviewers who haven't responded within 24 hours of being assigned?

I don't think I understand what you mean by logging the assignees

ankita240796 · 2020-08-25T21:55:21Z

lib/periodicChecks.js

+ * This function ensures that a pull request is assigned.
+ * 1. Fetch all pull requests.
+ * 2. Filter out those without assignees.
+ * 3. Find out appropriate assignee.


How do we find out - explain.

ankita240796 · 2020-08-25T21:56:38Z

lib/periodicChecks.js

+      await pingAndAssignReviewers(context, pullRequestData);
+    } else if (hasChangesRequested) {
+      console.log('PULL REQUEST HAS CHANGES REQUESTED...');
+      await assignPrAuthor(context, pullRequestData);


Why not pingAndAssignAuthor? Pass the comment to the function if it is different for different usecases.

ankita240796 · 2020-08-25T21:57:09Z

lib/periodicChecks.js

+      console.log('CONFUSED STATE, ASK FOR HELP FROM WELFARE TEAM...');
+      context.github.issues.createComment(
+        context.repo({
+          issue_number: pullRequestData.number,
+          body:
+            'Hi @' +
+            teamLeads.welfareTeam +
+            ', this pull request needs some assistance, PTAL. Thanks!',


Hmm, how is this decided, I did not see this in the pull request review doc.

While implementing this, I noticed that there would be an empty else case, although that is highly improbable, I decided to add it.
I also updated the review doc with this change.

Let us ping core-maintainers here.

ankita240796

A few high level comments, PTAL. Thanks @jameesjohn

index.js

lib/periodicChecks.js

ankita240796 · 2020-08-28T15:11:49Z

lib/periodicChecks.js

+const pingAndAssignReviewers = async (context, pullRequest) => {
+  const reviewers = pullRequest.requested_reviewers.map(
+    (reviewer) => reviewer.login
+  );
+
+  context.github.issues.addAssignees(
+    context.repo({
+      issue_number: pullRequest.number,
+      assignees: reviewers,
+    })
+  );
+
+  context.github.issues.createComment(
+    context.repo({
+      issue_number: pullRequest.number,
+      body:
+        'Assigning @' +
+        reviewers.join(', @') +
+        ' for codeowner reviews. Thanks!',
+    })
+  );
+};
+
+/**
+ * This function assigns the PR author to a pull request.
+ * @param {import('probot').Context} context
+ * @param {import('probot').Octokit.PullsGetResponse} pullRequest
+ * @param {string} comment
+ */
+const pingAndAssignPrAuthor = async (context, pullRequest, comment) => {
+  context.github.issues.addAssignees(
+    context.repo({
+      issue_number: pullRequest.number,
+      assignees: [pullRequest.user.login],
+    })
+  );
+  await context.github.issues.createComment(
+    context.repo({
+      issue_number: pullRequest.number,
+      body: comment,
+    })
+  );
+};


My one concern with all this is aren't we doing similar tasks in #116, shouldn't we reuse the common functions?

Yes, we are doing something similar there.
We can't reuse the function since #116 has not been merged.

This function is still valuable after merging #116.

ankita240796 · 2020-08-28T15:15:31Z

lib/periodicChecks.js

+ * @param {import('probot').Context} context
+ * @param {import('probot').Octokit.PullsGetResponse} pullRequest
+ */
+const handleApprovedPR = async (context, pullRequest) => {


This is exactly similar to the function in pr reviewed module - why not import that and use the function directly?

Modified to use that in the pr reviewed module.

lib/periodicChecks.js

ankita240796

Another note - we also plan to have a periodic check to ensure that all issues have a project added to them, are you planning to that in a separate PR?

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

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!

…dic-checks

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

lib/checkPullRequestReview.js

+    // 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 if it is created without
+    //  changelog label.


Remove extra space before changelog. Also, have we matched the comments with the ones in this section: https://docs.google.com/document/d/1m-7dZWGw77hZ6GOnjBu4p1XGXxyBPY2n2pzf-76RuTY/edit?ts=5f240b85#heading=h.66ts492p306d

lib/checkPullRequestReview.js

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

lib/periodicChecks.js

+      console.log('CONFUSED STATE, ASK FOR HELP FROM WELFARE TEAM...');
+      context.github.issues.createComment(
+        context.repo({
+          issue_number: pullRequestData.number,
+          body:
+            'Hi @' +
+            teamLeads.welfareTeam +
+            ', this pull request needs some assistance, PTAL. Thanks!',


Let us ping core-maintainers here.

lib/periodicChecks.js

jameesjohn · 2020-08-31T23:31:01Z

The ensureIssueHasProjects feature has been tested at jameesjohn/comment-on-pr#35, jameesjohn/comment-on-pr#34, jameesjohn/comment-on-pr#29, jameesjohn/comment-on-pr#10.

ankita240796 · 2020-09-01T08:43:44Z

index.js

+              context
+            );
+            break;
+          case constants.periodicCheck:


ensurePullRequestIsAssigned -> ensureAllPullRequestsAreAssigned

ensureIssueHasProjects -> ensureAllIssuesHaveProjects

ankita240796 · 2020-09-01T08:44:04Z

index.js

+    // interval: 24 * 60 * 60 * 1000, // 1 day
+    interval: 60 * 0.5 * 1000


lib/periodicChecks.js

ankita240796 · 2020-09-01T08:48:44Z

lib/periodicChecks.js

+    // Add Todo triage label if not already added in a previous iteration.
+    if (!labels.includes(TODO_TRIAGE_LABEL)) {
+      context.github.issues.addLabels(
+        context.repo({
+          issue_number: issue.number,
+          labels: [TODO_TRIAGE_LABEL],
+        })
+      );


You can remove this as discussed.

The approach looks good otherwise!

oppiabot · 2020-09-01T11:12:48Z

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!

…dic-checks

ankita240796 · 2020-09-01T12:22:43Z

lib/checkPullRequestReview.js

@@ -16,6 +16,7 @@
 * @fileoverview File to handle checks when a PR gets reviewed.
 */
 const utilityModule = require('./utils');
+const { pingAndAssignReviewers } = require('./utils');


You can directly use utilityModule. pingAndAssignReviewers. It is already imported.

ankita240796 · 2020-09-01T12:23:44Z

lib/utils.js

+ * @param {import('probot').Octokit.PullsGetResponse} pullRequest
+ * @param {string[]} reviewers
+ */
+const pingAndAssignReviewers = async (context, pullRequest, reviewers) => {


Why not here itself? Would prevent any extra follow ups since this is the last PR for your M3.

ankita240796 · 2020-09-01T12:54:29Z

lib/periodicChecks.js

+ *  - If pull request has pending reviews     -- Remaining reviewers.
+ *  - If pull request has changes requested   -- PR author.
+ *  - If pull request has been approved       -- Any member of the Organisation.
+ *  - If none of the above cases              -- Welfare team lead.


Onboarding team lead

ankita240796 · 2020-09-01T12:54:44Z

lib/periodicChecks.js

+ *  - If pull request has a merge conflict    -- PR author.
+ *  - If pull request has pending reviews     -- Remaining reviewers.
+ *  - If pull request has changes requested   -- PR author.
+ *  - If pull request has been approved       -- Any member of the Organisation.


Project owner, right?

Project owner or PR author.

ankita240796 · 2020-09-01T12:55:08Z

lib/periodicChecks.js

+        pullRequestData
+      );
+    } else {
+      console.log('CONFUSED STATE, ASK FOR HELP FROM WELFARE TEAM...');


ONBOARDING TEAM

ankita240796 · 2020-09-01T12:55:28Z

lib/periodicChecks.js

+ * This function ensures that all issue are linked to a project.
+ * 1. Fetch all open issues.
+ * 2. Fetch all project cards.
+ * 3. Filter open issues by the ones that have the triage label.


Update comment - we are not using triage label.

ankita240796 · 2020-09-01T12:56:32Z

lib/periodicChecks.js

+    const labels = issue.labels.map((label) => label.name);
+    if (labels.includes(TODO_TRIAGE_LABEL)) {
+      // If label has already been added we do nothing since we have already
+      // pinged the core maintainers.
+      return;
+    }
+    // Add Todo triage label.
+    context.github.issues.addLabels(
+      context.repo({
+        issue_number: issue.number,
+        labels: [TODO_TRIAGE_LABEL],
+      })
+    );


We should not do any of this - just check for project and ping. The label is for project leads to assign a priority to the issue.

Noted.
Removed.

ankita240796 · 2020-09-01T12:57:13Z

lib/utils.js

+
+  // Pull requests are also returned from the api, hence we need to filter
+  // them out.
+  return openIssues.filter((issue) => 'pull_request' in issue === false);


pull_request -> constant.

ankita240796 · 2020-09-01T12:57:46Z

lib/utils.js

+ * @param {import('probot').Context} context
+ */
+const getAllProjectCards = async (context) => {
+  const { data: allProjects } = await context.github.projects.listForRepo(


Please make sure line lengths are correct, throughout the PR.

Yes.
This has 75 chars.

ankita240796

Final comments - thanks for doing this so patiently @jameesjohn!

ankita240796 · 2020-09-01T20:59:25Z

lib/periodicChecks.js

+      );
+    } else if (hasPendingReviews(pullRequestData)) {
+      console.log('PULL REQUEST HAS PENDING REVIEWS...');
+      const reviewers = pullRequest.requested_reviewers.map(


Just one nit - can we skip assigning users who are already assigned?

All the checks here are running when no one is assigned.

Ah ok, yes missed that.

ankita240796 · 2020-09-01T20:59:52Z

lib/periodicChecks.js

+      );
+    } else if (hasChangesRequested) {
+      console.log('PULL REQUEST HAS CHANGES REQUESTED...');
+      const review = await getLastReviewOfType(


Wasn't this function renamed to getLastReviewOfSpecificType?

Yes. Apparently the refactor functionality in VSCode played a trick here.
I'll update it manually.

ankita240796 · 2020-09-01T21:00:13Z

lib/periodicChecks.js

+ * 4. Get Remaining issues
+ * 5. Ping core maintainers


Full stop in end.

ankita240796 · 2020-09-01T21:01:40Z

spec/periodicChecksSpec.js

+        });
+      });
+
+      it('should assign project owner', () => {


should not assign author case.

ankita240796

LGTM, Thanks @jameesjohn!

ankita240796 · 2020-09-02T05:00:23Z

lib/periodicChecks.js

+      );
+    } else if (hasPendingReviews(pullRequestData)) {
+      console.log('PULL REQUEST HAS PENDING REVIEWS...');
+      const reviewers = pullRequest.requested_reviewers.map(


Ah ok, yes missed that.

jameesjohn added 6 commits August 21, 2020 22:29

setup for periodic checks

935cacd

continue working on periodic checks

fd72e13

complete work on periodic checks

22f25f7

Minor modifications after testing

873efdc

complete tests

b9e5591

Merge branch 'master' of https://github.com/oppia/oppiabot into perio…

fcfbaa2

…dic-checks

jameesjohn requested a review from ankita240796 August 25, 2020 15:57

jameesjohn assigned ankita240796 Aug 25, 2020

ankita240796 suggested changes Aug 25, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Aug 25, 2020

address review comments

8a9570d

jameesjohn assigned ankita240796 and unassigned jameesjohn Aug 27, 2020

jameesjohn added 2 commits August 27, 2020 22:16

modify doc string

85804e4

minor changes

9fa82c6

ankita240796 suggested changes Aug 28, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Aug 28, 2020

ankita240796 suggested changes Aug 28, 2020

View reviewed changes

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

jameesjohn added 2 commits August 28, 2020 22:06

Merge branch 'master' of https://github.com/oppia/oppiabot into perio…

8b60ac2

…dic-checks

address review comments

7186149

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

ankita240796 suggested changes Aug 30, 2020

View reviewed changes

address review comments

2732f61

jameesjohn requested a review from ankita240796 August 30, 2020 21:31

jameesjohn assigned ankita240796 and unassigned jameesjohn Aug 30, 2020

ankita240796 suggested changes Sep 1, 2020

View reviewed changes

address review comments

213bc8f

jameesjohn assigned ankita240796 and unassigned jameesjohn Sep 1, 2020

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

oppiabot bot assigned jameesjohn Sep 1, 2020

Merge branch 'master' of https://github.com/oppia/oppiabot into perio…

5ef90d0

…dic-checks

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

jameesjohn assigned ankita240796 and unassigned ankita240796 and jameesjohn Sep 1, 2020

ankita240796 suggested changes Sep 1, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Sep 1, 2020

address review comments

afde037

jameesjohn assigned ankita240796 and unassigned jameesjohn Sep 1, 2020

jameesjohn requested a review from ankita240796 September 1, 2020 15:41

ankita240796 suggested changes Sep 1, 2020

View reviewed changes

ankita240796 assigned jameesjohn and unassigned ankita240796 Sep 1, 2020

address review comments

7cab048

jameesjohn assigned ankita240796 and unassigned jameesjohn Sep 1, 2020

ankita240796 approved these changes Sep 2, 2020

View reviewed changes

ankita240796 removed their assignment Sep 2, 2020

ankita240796 merged commit 7f3794c into oppia:master Sep 2, 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

		// interval: 24 * 60 * 60 * 1000, // 1 day
		interval: 60 * 0.5 * 1000

Periodic Checks #119

Periodic Checks #119

Conversation

jameesjohn commented Aug 25, 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

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

ankita240796 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Aug 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameesjohn commented Aug 31, 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

oppiabot bot commented Sep 1, 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

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

ankita240796 left a comment

Choose a reason for hiding this comment