Fix #64: Adds support for oppia-android (#76)

* Add whitelist for oppia-android

* Remove debug statement

* Fix check

* Change to whitelist

* Address review comments

constants.js

@@ -0,0 +1,64 @@
+const openEvent = 'opened';
+const reopenEvent = 'reopened';
+const labelEvent = 'labeled';
+const synchronizeEvent = 'synchronize';
+const closeEvent = 'closed';
+const editEvent = 'edited';
+
+const claCheck = 'cla-check';
+const changelogCheck = 'changelog-check';
+// This check is required in re-open events as well to
+// prevent user from reopening the PR.
+const branchCheck = 'branch-check';
+const wipCheck = 'wip-check';
+const assigneeCheck = 'assignee-check';
+const mergeConflictCheck = 'merge-conflict-check';
+const allMergeConflictCheck = 'all-merge-conflict-check';
+
+const checksWhitelist = {
+  'oppia-android': {
+    [openEvent]: [claCheck],
+    [reopenEvent]: [],
+    [labelEvent]: [],
+    [synchronizeEvent]: [],
+    [closeEvent]: [],
+    [editEvent]: []
+  },
+  'oppia': {
+    [openEvent]: [
+      claCheck,
+      changelogCheck,
+      branchCheck,
+      wipCheck
+    ],
+    [reopenEvent]: [
+      changelogCheck,
+      branchCheck,
+      wipCheck
+    ],
+    [labelEvent]: [assigneeCheck],
+    [synchronizeEvent]: [mergeConflictCheck],
+    [closeEvent]: [allMergeConflictCheck],
+    [editEvent]: [wipCheck]
+  }
+};
+
+module.exports.openEvent = openEvent;
+module.exports.reopenEvent = reopenEvent;
+module.exports.labelEvent = labelEvent;
+module.exports.synchronizeEvent = synchronizeEvent;
+module.exports.closeEvent = closeEvent;
+module.exports.editEvent = editEvent;
+
+module.exports.claCheck = claCheck;
+module.exports.changelogCheck = changelogCheck;
+module.exports.branchCheck = branchCheck;
+module.exports.wipCheck = wipCheck;
+module.exports.assigneeCheck = assigneeCheck;
+module.exports.mergeConflictCheck = mergeConflictCheck;
+module.exports.allMergeConflictCheck = allMergeConflictCheck;
+
+module.exports.getChecksWhitelist = function() {
+  return checksWhitelist;
+};
+

index.js

@@ -4,11 +4,55 @@ const apiForSheetsModule = require('./lib/apiForSheets');
 const checkMergeConflictsModule = require('./lib/checkMergeConflicts');
 const checkPullRequestLabelsModule = require('./lib/checkPullRequestLabels');
 const checkPullRequestBranchModule = require('./lib/checkPullRequestBranch');
-const checkWIPModule = require('./lib/checkWipDraftPR');
+const checkWipModule = require('./lib/checkWipDraftPR');
+const constants = require('./constants');
 
 const whitelistedAccounts = (
   (process.env.WHITELISTED_ACCOUNTS || '').toLowerCase().split(','));
 
+async function runChecks(context, checkEvent) {
+  const repoName = context.repo().repo.toLowerCase();
+  const checksWhitelist = constants.getChecksWhitelist();
+  if (checksWhitelist.hasOwnProperty(repoName)) {
+    const checks = checksWhitelist[repoName];
+    if (checks.hasOwnProperty(checkEvent)) {
+      const checkList = checks[checkEvent];
+      for (var i = 0; i < checkList.length; i++) {
+        switch (checkList[i]) {
+          case constants.claCheck:
+            await apiForSheetsModule.checkClaStatus(context);
+            break;
+          case constants.changelogCheck:
+            await checkPullRequestLabelsModule.checkChangelogLabel(context);
+            break;
+          case constants.branchCheck:
+            await checkPullRequestBranchModule.checkBranch(context);
+            break;
+          case constants.wipCheck:
+            await checkWipModule.checkWIP(context);
+            break;
+          case constants.assigneeCheck:
+            await checkPullRequestLabelsModule.checkAssignee(context);
+            break;
+          case constants.mergeConflictCheck:
+            await checkMergeConflictsModule.checkMergeConflictsInPullRequest(
+              context, context.payload.pull_request);
+            break;
+          case constants.allMergeConflictCheck:
+            await (
+              checkMergeConflictsModule.checkMergeConflictsInAllPullRequests(
+                context));
+            break;
+        }
+      }
+    }
+  }
+}
+
+function checkWhitelistedAccounts(context) {
+  return whitelistedAccounts.includes(context.repo().owner.toLowerCase());
+}
+
 /**
  * This is the main entrypoint to the Probot app
  * @param {import('probot').Application} oppiabot
@@ -26,55 +70,48 @@ module.exports = (oppiabot) => {
     // repository on which the bot has been installed.
     // This condition checks whether the owner account is included in
     // the whitelisted accounts.
-    if (whitelistedAccounts.includes(context.repo().owner.toLowerCase())) {
-      await apiForSheetsModule.checkClaStatus(context);
-      await checkPullRequestLabelsModule.checkChangelogLabel(context);
-      await checkPullRequestBranchModule.checkBranch(context);
-      await checkWIPModule.checkWIP(context);
+    if (checkWhitelistedAccounts(context)) {
+      await runChecks(context, constants.openEvent);
     }
   });
 
   oppiabot.on('pull_request.reopened', async context => {
-    if (whitelistedAccounts.includes(context.repo().owner.toLowerCase())) {
-      await checkPullRequestLabelsModule.checkChangelogLabel(context);
-      // Prevent user from reopening the PR.
-      await checkPullRequestBranchModule.checkBranch(context);
-      await checkWIPModule.checkWIP(context);
+    if (checkWhitelistedAccounts(context)) {
+      await runChecks(context, constants.reopenEvent);
     }
   });
 
   oppiabot.on('pull_request.labeled', async context => {
-    if (whitelistedAccounts.includes(context.repo().owner.toLowerCase())) {
-      await checkPullRequestLabelsModule.checkAssignee(context);
+     if (checkWhitelistedAccounts(context)) {
+      await runChecks(context, constants.labelEvent);
     }
   });
 
   oppiabot.on('pull_request.synchronize', async context => {
-    if (whitelistedAccounts.includes(context.repo().owner.toLowerCase())) {
+    if (checkWhitelistedAccounts(context)) {
       // eslint-disable-next-line no-console
       console.log(' PR SYNC EVENT TRIGGERED..');
-      await checkMergeConflictsModule.checkMergeConflictsInPullRequest(
-        context, context.payload.pull_request);
+      await runChecks(context, constants.synchronizeEvent);
     }
   });
 
   oppiabot.on('pull_request.closed', async context => {
-    if (whitelistedAccounts.includes(context.repo().owner.toLowerCase()) &&
+     if (
+      checkWhitelistedAccounts(context) &&
       context.payload.pull_request.merged === true) {
       // eslint-disable-next-line no-console
       console.log(' A PR HAS BEEN MERGED..');
-      await checkMergeConflictsModule.checkMergeConflictsInAllPullRequests(
-        context);
+      await runChecks(context, constants.closeEvent);
     }
   });
 
   oppiabot.on('pull_request.edited', async context => {
     if (
-      whitelistedAccounts.includes(context.repo().owner.toLowerCase()) &&
+      checkWhitelistedAccounts(context) &&
       context.payload.pull_request.state === 'open') {
       // eslint-disable-next-line no-console
       console.log('A PR HAS BEEN EDITED...');
-      await checkWIPModule.checkWIP(context);
+      await runChecks(context, constants.editEvent);
     }
   });
 };

spec/apiForSheetsSpec.js

@@ -2,7 +2,11 @@ require('dotenv').config();
 const { createProbot } = require('probot');
 // The plugin refers to the actual app in index.js.
 const oppiaBot = require('../index');
+const constants = require('../constants');
 const apiForSheetsModule = require('../lib/apiForSheets');
+const checkPullRequestLabelsModule = require('../lib/checkPullRequestLabels');
+const checkPullRequestBranchModule = require('../lib/checkPullRequestBranch');
+const checkWipModule = require('../lib/checkWipDraftPR');
 const scheduler = require('../lib/scheduler');
 const pullRequestpayload = JSON.parse(
   JSON.stringify(require('../fixtures/pullRequestPayload.json'))
@@ -47,7 +51,8 @@ describe('Api For Sheets Module', () => {
             body:
               'Hi! @tester7777. Welcome to Oppia! ' +
               'Please could you follow the instructions' +
-              '<a href="https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#setting-things-up">here</a>' +
+              '<a href="https://github.com/oppia/oppia/wiki/' +
+              'Contributing-code-to-Oppia#setting-things-up">here</a>' +
               "to get started ? You'll need to do this before" +
               'we can accept your PR. Thanks!',
           },
@@ -114,6 +119,9 @@ describe('Api For Sheets Module', () => {
       spyOn(apiForSheetsModule, 'checkClaStatus').and.callThrough();
       spyOn(apiForSheetsModule, 'authorize').and.callThrough({});
       spyOn(apiForSheetsModule, 'checkClaSheet').and.callThrough();
+      spyOn(checkPullRequestLabelsModule, 'checkChangelogLabel').and.callThrough();
+      spyOn(checkPullRequestBranchModule, 'checkBranch').and.callThrough();
+      spyOn(checkWipModule, 'checkWIP').and.callThrough();
       spyOn(google, 'sheets').and.returnValue({
         spreadsheets: {
           values: {
@@ -129,6 +137,13 @@ describe('Api For Sheets Module', () => {
       done();
     });
 
+    it('should call other checks', () => {
+      expect(
+        checkPullRequestLabelsModule.checkChangelogLabel).toHaveBeenCalled();
+      expect(checkPullRequestBranchModule.checkBranch).toHaveBeenCalled();
+      expect(checkWipModule.checkWIP).toHaveBeenCalled();
+    });
+
     it('should be called for the given payload', () => {
       expect(apiForSheetsModule.checkClaStatus).toHaveBeenCalled();
     });
@@ -305,4 +320,85 @@ describe('Api For Sheets Module', () => {
       });
     });
   });
+
+  describe('checks are run per the whitelist', () => {
+    beforeEach(function (done) {
+      spyOn(apiForSheetsModule, 'checkClaStatus').and.callThrough();
+      spyOn(apiForSheetsModule, 'authorize').and.callThrough({});
+      spyOn(apiForSheetsModule, 'checkClaSheet').and.callThrough();
+      spyOn(checkPullRequestLabelsModule, 'checkChangelogLabel').and.callThrough();
+      spyOn(checkPullRequestBranchModule, 'checkBranch').and.callThrough();
+      spyOn(checkWipModule, 'checkWIP').and.callThrough();
+      spyOn(google, 'sheets').and.returnValue({
+        spreadsheets: {
+          values: {
+            get: jasmine.createSpy('get').and.callFake(async (obj, cb) => {
+              await cb(null, {
+                data : {values: [['test']]},
+              });
+            }),
+          },
+        },
+      });
+      spyOn(constants, 'getChecksWhitelist').and.returnValue({
+        'oppia': {
+          'opened': ['cla-check']
+        }
+      });
+      robot.receive(pullRequestpayload);
+      done();
+    });
+
+    it('should not call non whitelisted checks', () => {
+      expect(
+        checkPullRequestLabelsModule.checkChangelogLabel).not.toHaveBeenCalled();
+      expect(checkPullRequestBranchModule.checkBranch).not.toHaveBeenCalled();
+      expect(checkWipModule.checkWIP).not.toHaveBeenCalled();
+    });
+
+    it('should be called for the given payload', () => {
+      expect(apiForSheetsModule.checkClaStatus).toHaveBeenCalled();
+    });
+
+    it('should be called once for the given payload', () => {
+      expect(apiForSheetsModule.checkClaStatus.calls.count()).toEqual(1);
+    });
+
+    it('should be called with one argument for the given payload', () => {
+      expect(apiForSheetsModule.checkClaStatus.calls.argsFor(0).length).toEqual(
+        1
+      );
+    });
+
+    it('should be called with the correct username for the given payload', () => {
+      const context = apiForSheetsModule.checkClaStatus.calls.argsFor(0)[0];
+      expect(context.payload.pull_request.user.login).toEqual('testuser7777');
+    });
+
+    it('should call authorize', () => {
+      expect(apiForSheetsModule.authorize).toHaveBeenCalled();
+    });
+
+    it('should call authorize once', () => {
+      expect(apiForSheetsModule.authorize.calls.count()).toEqual(1);
+    });
+
+    it('should call authorize with one argument', () => {
+      expect(apiForSheetsModule.authorize.calls.argsFor(0).length).toEqual(1);
+    });
+
+    it('should call checkClaSheet', () => {
+      expect(apiForSheetsModule.checkClaSheet).toHaveBeenCalled();
+    });
+
+    it('should call checkClaSheet once', () => {
+      expect(apiForSheetsModule.checkClaSheet.calls.count()).toEqual(1);
+    });
+
+    it('should call checkClaSheet with one argument', () => {
+      expect(apiForSheetsModule.checkClaSheet.calls.argsFor(0).length).toEqual(
+        1
+      );
+    });
+  });
 });

spec/setup.js

@@ -32,7 +32,7 @@ const envPath = path.join(__dirname, '..', '.env');
 const envExamplePath = path.join(__dirname, '..', '.env.example');
 let envData = '';
 
-const setWhitelistedAccount = () => {
+const setEnvVariables = () => {
   // Load env.
   let data = '';
   if (fs.existsSync(envPath)) {
@@ -139,7 +139,7 @@ const runTest = () => {
 
   return new Promise((resolve, reject) => {
     exec(
-      nycPath + ' "' + jasminePath + '"',
+      nycPath + ' ' + jasminePath,
       (error, stdout, stderr) => {
         console.log(stdout);
         if (error) {
@@ -160,7 +160,7 @@ const revertEnv = () => {
   fs.writeFileSync(envPath, envData);
 };
 
-setWhitelistedAccount();
+setEnvVariables();
 runTest().then(
   () => {
     revertEnv();