diff --git a/README.md b/README.md index ddf4bc44..0453b584 100644 --- a/README.md +++ b/README.md @@ -49,12 +49,17 @@ the pull is considered ready. Conversely, if a pull only touches test code, you might put only `qa_req 0` on it to say that it doesn't need to be QAed, since it should break tests if there's anything wrong with it. -When you CR a pull, add a comment to it with `CR :emoji:` in it. +When you CR a pull, you can either approve it on GitHub (using GitHub's native +**Approve** review action) or add a comment containing `CR :emoji:`. (`emoji` is simply a word or words between colons; we often use GitHub emoji, -which follow this format.) This comment is considered a _signoff_. Pulldasher -will update the pull's display to indicate that one of the required CRs is -completed. Similarly, when you QA a pull, add a comment containing -`QA :emoji:`, and the number of QA signoffs will increase. +which follow this format.) A GitHub approval and a `CR :emoji:` comment are both +considered _signoffs_. Pulldasher will update the pull's display to indicate +that one of the required CRs is completed. Similarly, when you QA a pull, add a +comment containing `QA :emoji:`, and the number of QA signoffs will increase. + +To honor GitHub approvals, enable the **Pull request reviews** webhook event on +tracked repositories. Set `useGithubApprovalForCr: false` in `config.js` to +disable this and require `CR :emoji:` comments (legacy behavior). ## Feature Details diff --git a/config.example.js b/config.example.js index e54d33ed..97789c12 100644 --- a/config.example.js +++ b/config.example.js @@ -40,8 +40,8 @@ module.exports = { // containing the secret from `hook_secret` below. // - Content type should be `application/x-www-form-urlencoded` // - Choose to be sent individual events, and then check the Issue comments, - // Issues, Pull requests, Pull request review comments, Pushes, and - // Statuses boxes + // Issues, Pull requests, Pull request reviews, Pull request review + // comments, Pushes, and Statuses boxes github: { // Get this from the GitHub application setup page. clientId: "your github application client id", @@ -63,6 +63,10 @@ module.exports = { secret: "secret for signing session cookies", }, + // When true (default), GitHub PR reviews with state APPROVED count as CR + // signoffs without requiring a CR :emoji: tag in the review body. + useGithubApprovalForCr: true, + // List of repositories for pulldasher to watch. repos: [ // Just listing the repo as a string indicates pulldasher should consider diff --git a/models/signature.js b/models/signature.js index d3e58867..25b11673 100644 --- a/models/signature.js +++ b/models/signature.js @@ -54,9 +54,32 @@ Signature.parseReview = function parseReview(review, repoFullName, pullNumber) { user: review.user, created_at: review.submitted_at, id: review.id, + state: review.state, }; - return parseSignatures(reviewData); + const signatures = parseSignatures(reviewData); + + // A GitHub "Approve" review counts as a CR signoff even without a + // `CR :emoji:` tag in the body. Skip if the body already produced one. + if ( + useGithubApprovalForCr() && + review.state === "APPROVED" && + !signatures.some((sig) => sig.data.type === "CR") + ) { + signatures.push( + new Signature({ + repo: repoFullName, + number: pullNumber, + user: review.user, + type: "CR", + created_at: review.submitted_at, + active: true, + comment_id: review.id, + }) + ); + } + + return signatures; }; function parseSignatures(data) { @@ -114,7 +137,11 @@ Signature.compare = function (a, b) { }; function hasTag(body, tag) { - return tag.regex.test(body); + return tag.regex.test(body || ""); +} + +function useGithubApprovalForCr() { + return config.useGithubApprovalForCr !== false; } export default Signature; diff --git a/package.json b/package.json index 5d881028..97c1d378 100644 --- a/package.json +++ b/package.json @@ -66,6 +66,7 @@ "frontend:watch": "webpack --watch --config ./frontend/webpack.dev.config.js", "postinstall": "npm run build", "build": "webpack --config ./frontend/webpack.prod.config.js", + "test": "node --test test/*.test.js", "precommit": "lint-staged" }, "lint-staged": { diff --git a/test/signature.test.js b/test/signature.test.js new file mode 100644 index 00000000..19e33308 --- /dev/null +++ b/test/signature.test.js @@ -0,0 +1,62 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +process.env.CONFIG_PATH = "../test/test-config.js"; + +const Signature = (await import("../models/signature.js")).default; + +const reviewUser = { id: 1, login: "reviewer" }; +const repo = "owner/repo"; +const number = 42; + +test("parseReview creates CR signature from APPROVED review with empty body", () => { + const signatures = Signature.parseReview( + { + id: 100, + body: "", + state: "APPROVED", + user: reviewUser, + submitted_at: "2024-01-01T00:00:00Z", + }, + repo, + number + ); + + assert.equal(signatures.length, 1); + assert.equal(signatures[0].data.type, "CR"); + assert.equal(signatures[0].data.comment_id, 100); + assert.equal(signatures[0].data.user.login, "reviewer"); +}); + +test("parseReview keeps single CR signature when body already contains CR tag", () => { + const signatures = Signature.parseReview( + { + id: 101, + body: "CR :thumbsup:", + state: "APPROVED", + user: reviewUser, + submitted_at: "2024-01-01T00:00:00Z", + }, + repo, + number + ); + + assert.equal(signatures.length, 1); + assert.equal(signatures[0].data.type, "CR"); +}); + +test("parseReview ignores CHANGES_REQUESTED reviews", () => { + const signatures = Signature.parseReview( + { + id: 102, + body: "Please fix this", + state: "CHANGES_REQUESTED", + user: reviewUser, + submitted_at: "2024-01-01T00:00:00Z", + }, + repo, + number + ); + + assert.equal(signatures.length, 0); +}); diff --git a/test/test-config.js b/test/test-config.js new file mode 100644 index 00000000..9e30e615 --- /dev/null +++ b/test/test-config.js @@ -0,0 +1,19 @@ +var emoji = + "(\u00a9|\u00ae|[\u2000-\u3300]|\ud83c[\ud000-\udfff]|\ud83d[\ud000-\udfff]|\ud83e[\ud000-\udfff])"; +var emojiText = ":[^\n:]+:"; +var signature = "(" + emojiText + "|" + emoji + ")"; + +export default { + useGithubApprovalForCr: true, + repos: ["owner/repo"], + tags: [ + { + name: "CR", + regex: new RegExp("\\bCR " + signature, "i"), + }, + { + name: "QA", + regex: new RegExp("\\bQA " + signature, "i"), + }, + ], +};