diff --git a/.github/extensions/external-plugin-review-board/lib/state.mjs b/.github/extensions/external-plugin-review-board/lib/state.mjs index fdd419e22..3d5db0392 100644 --- a/.github/extensions/external-plugin-review-board/lib/state.mjs +++ b/.github/extensions/external-plugin-review-board/lib/state.mjs @@ -16,6 +16,7 @@ export const COLUMNS = [ const HISTORY_LIMIT = 200; export const FIRST_PARTY_LABEL = "first party"; +const MANUAL_INTAKE_OVERRIDE_LABEL = "intake-manually-overridden"; const PENDING_INTAKE_LABELS = ["awaiting-review", "requires-submitter-fixes"]; export function isReadyForApproval(labels) { @@ -30,6 +31,7 @@ function applyFirstPartyFastTrack(item) { !item.labels?.includes(FIRST_PARTY_LABEL) || !item.labels?.includes("ready-for-review") || item.labels?.some((label) => PENDING_INTAKE_LABELS.includes(label)) || + item.labels?.includes(MANUAL_INTAKE_OVERRIDE_LABEL) || item.fastTrackDismissed || item.decision || item.review || @@ -54,6 +56,7 @@ function clearFirstPartyFastTrack(item) { if ( (item.labels?.includes(FIRST_PARTY_LABEL) && item.labels?.includes("ready-for-review") && + !item.labels?.includes(MANUAL_INTAKE_OVERRIDE_LABEL) && !item.labels?.some((label) => PENDING_INTAKE_LABELS.includes(label))) || !item.review?.fastTracked || item.reviewStatus === "queued" diff --git a/.github/extensions/external-plugin-review-board/public/app.js b/.github/extensions/external-plugin-review-board/public/app.js index c40cafa5a..b28f9d8c0 100644 --- a/.github/extensions/external-plugin-review-board/public/app.js +++ b/.github/extensions/external-plugin-review-board/public/app.js @@ -412,7 +412,7 @@ function renderDrawer() { ui.drawerFooter.hidden = Boolean(item.decision); updatePreview(); - ui.approve.disabled = bulkBusy || Boolean(item.pendingDecision); + ui.approve.disabled = bulkBusy || !item.reviewEligible || Boolean(item.pendingDecision); ui.reject.disabled ||= bulkBusy || Boolean(item.pendingDecision); renderTab(item); } @@ -584,6 +584,7 @@ function resetConfirm() { ui.approve.disabled = true; ui.reject.disabled = true; } + if (!item?.reviewEligible) ui.approve.disabled = true; } async function decide(kind) { diff --git a/.github/workflows/external-plugin-command-router.yml b/.github/workflows/external-plugin-command-router.yml index 22b42b33a..b40ded157 100644 --- a/.github/workflows/external-plugin-command-router.yml +++ b/.github/workflows/external-plugin-command-router.yml @@ -549,6 +549,23 @@ jobs: return; } + try { + await github.rest.issues.getLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + name: 'intake-manually-overridden' + }); + } catch (error) { + if (error.status !== 404) throw error; + await github.rest.issues.createLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + name: 'intake-manually-overridden', + color: 'BFD4F2', + description: 'Submission was manually moved to review without passing automated intake gates' + }); + } + await github.rest.reactions.createForIssueComment({ owner: context.repo.owner, repo: context.repo.repo, @@ -564,6 +581,7 @@ jobs: desiredLabels: new Set([ 'external-plugin', 'ready-for-review', + 'intake-manually-overridden', ...(labelNames.has('external-plugin-canvas') ? ['external-plugin-canvas'] : []) ]) }); diff --git a/.github/workflows/setup-labels.yml b/.github/workflows/setup-labels.yml index a415b1691..d529107b4 100644 --- a/.github/workflows/setup-labels.yml +++ b/.github/workflows/setup-labels.yml @@ -81,6 +81,10 @@ jobs: color: 'D93F0B', description: 'Submission has quality-gate findings that submitter must fix before maintainer review' }, + 'intake-manually-overridden': { + color: 'BFD4F2', + description: 'Submission was manually moved to review without passing automated intake gates' + }, 'approved': { color: '1D76DB', description: 'Submission was approved by a maintainer' diff --git a/eng/external-plugin-intake-state.mjs b/eng/external-plugin-intake-state.mjs index 7feac27bb..5c60e5d5a 100644 --- a/eng/external-plugin-intake-state.mjs +++ b/eng/external-plugin-intake-state.mjs @@ -27,6 +27,10 @@ export const EXTERNAL_PLUGIN_INTAKE_LABELS = Object.freeze({ color: "D93F0B", description: "Submission has quality-gate findings that submitter must fix before maintainer review", }, + "intake-manually-overridden": { + color: "BFD4F2", + description: "Submission was manually moved to review without passing automated intake gates", + }, approved: { color: "1D76DB", description: "Submission was approved by a maintainer", @@ -43,6 +47,7 @@ const EXTERNAL_PLUGIN_INTAKE_SYNC_LABELS = Object.freeze([ "awaiting-review", "ready-for-review", "requires-submitter-fixes", + "intake-manually-overridden", "rejected", ]); diff --git a/eng/external-plugin-intake-state.test.mjs b/eng/external-plugin-intake-state.test.mjs index 6abbe3eaa..b4bfc7cda 100644 --- a/eng/external-plugin-intake-state.test.mjs +++ b/eng/external-plugin-intake-state.test.mjs @@ -35,6 +35,30 @@ function mockGitHub(currentLabels) { }; } +test("a fresh intake evaluation clears the manual override label", async () => { + const github = mockGitHub([ + "external-plugin", + "ready-for-review", + "first party", + "intake-manually-overridden", + ]); + + await applyExternalPluginIntakeEvaluation({ + github, + owner: "github", + repo: "awesome-copilot", + issueNumber: 1, + evaluation: { + intakeState: "ready-for-review", + reviewSignals: { repository: { ownershipLabel: "first party" } }, + commentMarker: "", + commentBody: "Intake result", + }, + }); + + assert.deepEqual(github.labels, new Set(["external-plugin", "ready-for-review", "first party"])); +}); + for (const state of ["ready-for-review", "requires-submitter-fixes", "awaiting-review", "rejected"]) { for (const ownershipLabel of ["first party", "third party", null, undefined]) { test(`intake syncs ${ownershipLabel ?? "no ownership label"} in ${state} state`, async () => { diff --git a/eng/external-plugin-review-board-state.test.mjs b/eng/external-plugin-review-board-state.test.mjs index 8fd68e829..b93a6469e 100644 --- a/eng/external-plugin-review-board-state.test.mjs +++ b/eng/external-plugin-review-board-state.test.mjs @@ -63,6 +63,36 @@ test("first-party submissions are not fast-tracked in other pending intake state assert.equal(effectiveColumn(item), "unreviewed"); }); +test("manually overridden first-party submissions are never fast-tracked", async (t) => { + const store = await createStore(t); + const overriddenIssue = issue(1, ["first party", "intake-manually-overridden"]); + await store.syncIssues([overriddenIssue]); + + const item = store.getItem(1); + assert.equal(item.review, null); + assert.equal(item.reviewStatus, "unreviewed"); + assert.equal(effectiveColumn(item), "unreviewed"); + + await store.syncIssues([issue(1, ["first party", "intake-manually-overridden"])]); + assert.equal(store.getItem(1).review, null); + + const validationPassed = await store.syncIssues([issue(1, ["first party"])]); + assert.deepEqual(validationPassed.fastTracked, [1]); + assert.equal(store.getItem(1).review.fastTracked, true); +}); + +test("a manual override clears an existing synthetic first-party fast-track", async (t) => { + const store = await createStore(t); + await store.syncIssues([issue(1, ["first party"])]); + + const result = await store.syncIssues([issue(1, ["first party", "intake-manually-overridden"])]); + + assert.deepEqual(result.fastTracked, []); + assert.equal(store.getItem(1).review, null); + assert.equal(store.getItem(1).reviewStatus, "unreviewed"); + assert.equal(effectiveColumn(store.getItem(1)), "unreviewed"); +}); + test("approval requires ready status and no pending intake label", () => { assert.equal(isReadyForApproval(["external-plugin", "ready-for-review"]), true); assert.equal(isReadyForApproval(["external-plugin", "ready-for-review", "awaiting-review"]), false); diff --git a/eng/external-plugin-review-board-ui.test.mjs b/eng/external-plugin-review-board-ui.test.mjs new file mode 100644 index 000000000..16dfe87d4 --- /dev/null +++ b/eng/external-plugin-review-board-ui.test.mjs @@ -0,0 +1,98 @@ +import assert from "node:assert/strict"; +import { readFile } from "node:fs/promises"; +import vm from "node:vm"; +import { test } from "node:test"; + +const appSource = await readFile( + new URL("../.github/extensions/external-plugin-review-board/public/app.js", import.meta.url), + "utf8", +); + +function button() { + return { + disabled: false, + textContent: "", + classList: { remove() {} }, + }; +} + +function createContext(item, { bulkBusy = false } = {}) { + const ui = { + drawerKicker: {}, + drawerTitle: {}, + drawerMeta: {}, + drawerLink: {}, + drawerMove: { replaceChildren() {} }, + drawerRereview: {}, + tabs: [], + drawerFooter: {}, + approve: button(), + reject: button(), + }; + const context = vm.createContext({ + board: { columns: [] }, + bulkBusy, + clearTimeout() {}, + details: new Map(), + findItem: () => item, + h: (...args) => args, + hideRereview() {}, + itemName: () => "Submission", + pendingConfirm: { timer: null, kind: null }, + relativeTime: () => "now", + renderTab() {}, + selected: item.number, + ui, + updatePreview() {}, + }); + return { context, ui }; +} + +function evaluateFunction(context, source, signature, nextSignature) { + const start = source.indexOf(signature); + const end = source.indexOf(nextSignature, start); + assert.notEqual(start, -1, `Expected to find ${signature}`); + assert.notEqual(end, -1, `Expected to find ${nextSignature}`); + vm.runInContext(source.slice(start, end), context); +} + +test("Approve is disabled for submissions that are not review-eligible", () => { + const item = { number: 42, author: "contributor", labels: ["awaiting-review"], reviewEligible: false }; + const { context, ui } = createContext(item); + + evaluateFunction(context, appSource, "function renderDrawer()", "function renderTab("); + context.renderDrawer(); + + assert.equal(ui.approve.disabled, true); + assert.equal(ui.reject.disabled, false); + + evaluateFunction(context, appSource, "function resetConfirm()", "async function decide("); + context.resetConfirm(); + + assert.equal(ui.approve.disabled, true); + assert.equal(ui.reject.disabled, false); +}); + +test("Approve retains its existing eligible, pending-decision, and bulk-busy states", () => { + const eligible = { number: 42, author: "contributor", labels: [], reviewEligible: true }; + + const available = createContext(eligible); + evaluateFunction(available.context, appSource, "function renderDrawer()", "function renderTab("); + available.context.renderDrawer(); + assert.equal(available.ui.approve.disabled, false); + + const pending = createContext({ ...eligible, pendingDecision: { kind: "approve" } }); + evaluateFunction(pending.context, appSource, "function renderDrawer()", "function renderTab("); + pending.context.renderDrawer(); + assert.equal(pending.ui.approve.disabled, true); + + const busy = createContext(eligible, { bulkBusy: true }); + evaluateFunction(busy.context, appSource, "function renderDrawer()", "function renderTab("); + busy.context.renderDrawer(); + assert.equal(busy.ui.approve.disabled, true); + + const reset = createContext({ ...eligible, pendingDecision: { kind: "approve" } }); + evaluateFunction(reset.context, appSource, "function resetConfirm()", "async function decide("); + reset.context.resetConfirm(); + assert.equal(reset.ui.approve.disabled, true); +});