Fix check order in pr-bot

Only check user permissions if a command is detected to avoid adding 'sorry, not allowed' comments in response to comments that aren't commands
microsoft · May 17, 2022 · 8366ffa · 8366ffa
1 parent 8fe7231
commit 8366ffa
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 32 deletions.
diff --git a/.github/scripts/build.js b/.github/scripts/build.js
@@ -17,19 +17,6 @@ async function getCommandFromComment({ core, context, github }) {
   const runId = context.runId;
   const prAuthorUsername = context.payload.issue.user.login;
 
-  // only allow actions for users with write access
-  if (!await userHasWriteAccessToRepo({ core, github }, commentUsername, repoOwner, repoName)) {
-    core.notice("Command: none - user doesn't have write permission]");
-    await github.rest.issues.createComment({
-      owner: repoOwner,
-      repo: repoName,
-      issue_number: prNumber,
-      body: `Sorry, @${commentUsername}, only users with write access to the repo can run pr-bot commands.`
-    });
-    logAndSetOutput(core, "command", "none");
-    return "none";
-  }
-
   // Determine PR SHA etc
   const ciGitRef = getRefForPr(prNumber);
   logAndSetOutput(core, "ciGitRef", ciGitRef);
@@ -65,7 +52,20 @@ async function getCommandFromComment({ core, context, github }) {
   let command = "none";
   const trimmedFirstLine = commentFirstLine.trim();
   if (trimmedFirstLine[0] === "/") {
-    const parts = trimmedFirstLine.split(' ').filter(p=>p !== '');
+    // only allow actions for users with write access
+    if (!await userHasWriteAccessToRepo({ core, github }, commentUsername, repoOwner, repoName)) {
+      core.notice("Command: none - user doesn't have write permission]");
+      await github.rest.issues.createComment({
+        owner: repoOwner,
+        repo: repoName,
+        issue_number: prNumber,
+        body: `Sorry, @${commentUsername}, only users with write access to the repo can run pr-bot commands.`
+      });
+      logAndSetOutput(core, "command", "none");
+      return "none";
+    }
+
+    const parts = trimmedFirstLine.split(' ').filter(p => p !== '');
     const commandText = parts[0];
     switch (commandText) {
       case "/test":

diff --git a/.github/scripts/build.test.js b/.github/scripts/build.test.js
@@ -53,27 +53,49 @@ describe('getCommandFromComment', () => {
   }
 
   describe('with non-contributor', () => {
-    test(`for '/test' should return 'none'`, async () => {
-      const context = createCommentContext({
-        username: 'non-contributor',
-        body: '/test',
+    describe(`for '/test`, () => {
+      test(`should return 'none'`, async () => {
+        const context = createCommentContext({
+          username: 'non-contributor',
+          body: '/test',
+        });
+        const command = await getCommandFromComment({ core, context, github });
+        expect(outputFor(mockCoreSetOutput, 'command')).toBe('none');
       });
-      const command = await getCommandFromComment({ core, context, github });
-      expect(outputFor(mockCoreSetOutput, 'command')).toBe('none');
-    });
 
-    test(`should add a comment indicating that the user cannot run commands`, async () => {
-      const context = createCommentContext({
-        username: 'non-contributor',
-        body: '/test',
-        pullRequestNumber: PR_NUMBER.UPSTREAM_NON_DOCS_CHANGES,
+      test(`should add a comment indicating that the user cannot run commands`, async () => {
+        const context = createCommentContext({
+          username: 'non-contributor',
+          body: '/test',
+          pullRequestNumber: PR_NUMBER.UPSTREAM_NON_DOCS_CHANGES,
+        });
+        await getCommandFromComment({ core, context, github });
+        expect(mockGithubRestIssuesCreateComment).toHaveComment({
+          owner: 'someOwner',
+          repo: 'someRepo',
+          issue_number: PR_NUMBER.UPSTREAM_NON_DOCS_CHANGES,
+          bodyMatcher: /Sorry, @non-contributor, only users with write access to the repo can run pr-bot commands./
+        });
       });
-      await getCommandFromComment({ core, context, github });
-      expect(mockGithubRestIssuesCreateComment).toHaveComment({
-        owner: 'someOwner',
-        repo: 'someRepo',
-        issue_number: PR_NUMBER.UPSTREAM_NON_DOCS_CHANGES,
-        bodyMatcher: /Sorry, @non-contributor, only users with write access to the repo can run pr-bot commands./
+    });
+    describe(`for 'non-command`, () => {
+      test(`should return 'none'`, async () => {
+        const context = createCommentContext({
+          username: 'non-contributor',
+          body: 'non-command',
+        });
+        const command = await getCommandFromComment({ core, context, github });
+        expect(outputFor(mockCoreSetOutput, 'command')).toBe('none');
+      });
+
+      test(`should not add a comment`, async () => {
+        const context = createCommentContext({
+          username: 'non-contributor',
+          body: 'non-command',
+          pullRequestNumber: PR_NUMBER.UPSTREAM_NON_DOCS_CHANGES,
+        });
+        await getCommandFromComment({ core, context, github });
+        expect(mockGithubRestIssuesCreateComment).not.toHaveBeenCalled();
       });
     });