From f59f72410bf4d5f0363eb8fa351206a5d12acb15 Mon Sep 17 00:00:00 2001 From: Ankita Saxena Date: Sat, 6 Jun 2020 11:45:42 +0530 Subject: [PATCH] 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 | 64 ++++++++++++++++++++++++++ index.js | 79 +++++++++++++++++++++++--------- spec/apiForSheetsSpec.js | 98 +++++++++++++++++++++++++++++++++++++++- spec/setup.js | 6 +-- 4 files changed, 222 insertions(+), 25 deletions(-) create mode 100644 constants.js diff --git a/constants.js b/constants.js new file mode 100644 index 00000000..cdcb5314 --- /dev/null +++ b/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; +}; + diff --git a/index.js b/index.js index 85f46aee..667b5720 100644 --- a/index.js +++ b/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); } }); }; diff --git a/spec/apiForSheetsSpec.js b/spec/apiForSheetsSpec.js index 55835de0..53f8eba6 100644 --- a/spec/apiForSheetsSpec.js +++ b/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' + - 'here' + + 'here' + "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 + ); + }); + }); }); diff --git a/spec/setup.js b/spec/setup.js index c9dc0a57..2896a1d2 100644 --- a/spec/setup.js +++ b/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();