Merge pull request #45 from fox-forks/hyperupcall-bug-fixes-2

Improve error message and merge heuristic

Author: orta

index.js

@@ -17,7 +17,7 @@ async function run() {
   // Merge if they say they have access
   if (context.eventName === "issue_comment" || context.eventName === "pull_request_review") {
     const bodyLower = getPayloadBody().toLowerCase();
-    if (bodyLower.includes("lgtm") && !bodyLower.includes("lgtm but")) {
+    if (hasValidLgtmSubstring(bodyLower)) {
       new Actor().mergeIfHasAccess();
     } else if (bodyLower.includes("@github-actions close")) {
       new Actor().closePROrIssueIfInCodeowners();
@@ -170,6 +170,12 @@ class Actor {
 
     const { octokit, thisRepo, issue, sender } = this;
 
+    // Don't try merge if mergability is not yet known
+    if (prInfo.data.mergeable === null) {
+      await octokit.issues.createComment({ ...thisRepo, issue_number: issue.number, body: `Sorry @${sender}, this PR is still running background checks to compute mergeability. They'll need to complete before this can be merged.` });
+      return
+    }
+
     // Don't try merge unmergable stuff
     if (!prInfo.data.mergeable) {
       await octokit.issues.createComment({ ...thisRepo, issue_number: issue.number, body: `Sorry @${sender}, this PR has merge conflicts. They'll need to be fixed before this can be merged.` });
@@ -267,6 +273,27 @@ function getFilesNotOwnedByCodeOwner(owner, files, cwd) {
   return contents.includes("@" + login.toLowerCase() + " ") || contents.includes("@" + login.toLowerCase() + "\n")
 }
 
+/**
+ *
+ * @param {string} bodyLower
+ */
+function hasValidLgtmSubstring(bodyLower) {
+  if (bodyLower.includes("lgtm")) {
+    if (bodyLower.includes("lgtm but")) return false
+    if (bodyLower.includes("lgtm, but")) return false
+
+    const charBefore = bodyLower.charAt(bodyLower.indexOf("lgtm") - 1)
+    const charAfter = bodyLower.charAt(bodyLower.indexOf("lgtm") + "lgtm".length)
+    if (charBefore === "\"" || charAfter === "\"") return false
+    if (charBefore === "'" || charAfter === "'") return false
+    if (charBefore === "`" || charAfter === "`") return false
+
+    return true
+  }
+
+  return false
+}
+
 
 /**
  *
@@ -343,7 +370,8 @@ async function createOrAddLabel(octokit, repoDeets, labelConfig) {
 module.exports = {
   getFilesNotOwnedByCodeOwner,
   findCodeOwnersForChangedFiles,
-  githubLoginIsInCodeowners
+  githubLoginIsInCodeowners,
+  hasValidLgtmSubstring
 }
 
 // @ts-ignore

index.test.js

@@ -1,4 +1,4 @@
-const { getFilesNotOwnedByCodeOwner, findCodeOwnersForChangedFiles, githubLoginIsInCodeowners } = require(".");
+const { getFilesNotOwnedByCodeOwner, findCodeOwnersForChangedFiles, githubLoginIsInCodeowners, hasValidLgtmSubstring } = require(".");
 
 test("determine who owns a set of files", () => {
   const noFiles = findCodeOwnersForChangedFiles(["root-codeowners/one.two.js"], "./test-code-owners-repo");
@@ -53,3 +53,30 @@ describe(githubLoginIsInCodeowners, () => {
     expect(noOrt).toEqual(false);
   });
 })  
+
+describe(hasValidLgtmSubstring, () => {
+  test("allows lgtm", () => {
+    const isValidSubstring = hasValidLgtmSubstring("this lgtm!");
+    expect(isValidSubstring).toEqual(true);
+  });
+  test("denies lgtm but", () => {
+    const isValidSubstring = hasValidLgtmSubstring("this lgtm but");
+    expect(isValidSubstring).toEqual(false);
+  });
+  test("denies lgtm but", () => {
+    const isValidSubstring = hasValidLgtmSubstring("this lgtm, but");
+    expect(isValidSubstring).toEqual(false);
+  });
+  test("denies lgtm in double quotes", () => {
+    const isValidSubstring = hasValidLgtmSubstring("\"lgtm\"");
+    expect(isValidSubstring).toEqual(false);
+  });
+  test("denies lgtm in single quotes", () => {
+    const isValidSubstring = hasValidLgtmSubstring("'lgtm");
+    expect(isValidSubstring).toEqual(false);
+  });
+  test("denies lgtm in inline code blocks", () => {
+    const isValidSubstring = hasValidLgtmSubstring("lgtm`");
+    expect(isValidSubstring).toEqual(false);
+  });
+})