diff --git a/backend/src/github_pm/sdlc_metrics.py b/backend/src/github_pm/sdlc_metrics.py index 054cfc1..b93fc98 100644 --- a/backend/src/github_pm/sdlc_metrics.py +++ b/backend/src/github_pm/sdlc_metrics.py @@ -441,6 +441,12 @@ def graphql_search_open_pull_requests_attention( nodes { submittedAt state + author { + __typename + ... on User { login } + ... on Bot { login } + ... on Organization { login } + } } } additions diff --git a/backend/src/github_pm/status_report_api.py b/backend/src/github_pm/status_report_api.py index a2ee89d..60282e8 100644 --- a/backend/src/github_pm/status_report_api.py +++ b/backend/src/github_pm/status_report_api.py @@ -45,10 +45,10 @@ async def get_project_status_report( Defaults: ``end_date`` = today (UTC), ``start_date`` = ``end_date`` minus 7 calendar days. Sections: merged pull requests (by merge date), pull requests opened, issues - opened (PRs excluded), recently updated PRs (open, non-draft, touched in the - window but not opened in it), reviewer and creator attention (current open PRs - by merge/review state), and PR backlog (open, non-draft PRs not updated on or - after ``start_date``, UTC calendar dates). + opened (PRs excluded), PRs needing a second approval, recently updated PRs + (open, non-draft, touched in the window but not opened in it), reviewer and + creator attention (current open PRs by merge/review state), and PR backlog + (open, non-draft PRs not updated on or after ``start_date``, UTC calendar dates). """ resolved_end = end_date if end_date is not None else _default_end_date() resolved_start = ( diff --git a/backend/src/github_pm/status_report_models.py b/backend/src/github_pm/status_report_models.py index 4d85fe3..beabb68 100644 --- a/backend/src/github_pm/status_report_models.py +++ b/backend/src/github_pm/status_report_models.py @@ -30,6 +30,12 @@ class PrBacklogItem(StatusReportItem): ) +class SecondApprovalItem(StatusReportItem): + """Ready-to-review PR row including its approving reviewer handle.""" + + reviewer_login: str = Field(description="GitHub handle of the approving reviewer") + + class ProjectStatusReportResponse(BaseModel): """Inclusive calendar window from ``start_date`` through ``end_date`` (UTC calendar dates).""" @@ -47,6 +53,13 @@ class ProjectStatusReportResponse(BaseModel): default_factory=list, description="Issues created in the window (pull requests excluded)", ) + second_approval_needed: list[SecondApprovalItem] = Field( + default_factory=list, + description=( + "Open, non-draft pull requests that are mergeable and clean, with exactly " + "one submitted approval" + ), + ) recently_updated_pull_requests: list[StatusReportItem] = Field( default_factory=list, description=( diff --git a/backend/src/github_pm/status_report_service.py b/backend/src/github_pm/status_report_service.py index 405cdb8..295d033 100644 --- a/backend/src/github_pm/status_report_service.py +++ b/backend/src/github_pm/status_report_service.py @@ -13,6 +13,7 @@ from github_pm.status_report_models import ( PrBacklogItem, ProjectStatusReportResponse, + SecondApprovalItem, StatusReportItem, ) @@ -82,6 +83,20 @@ def _backlog_item_from_gql_node(node: dict[str, Any], end_d: date) -> PrBacklogI ) +def _second_approval_item_from_gql_node( + node: dict[str, Any], review: dict[str, Any] +) -> SecondApprovalItem: + base = _item_from_gql_node(node) + author = review.get("author") + reviewer_login = author.get("login") if isinstance(author, dict) else None + return SecondApprovalItem( + number=base.number, + title=base.title, + html_url=base.html_url, + reviewer_login=str(reviewer_login or ""), + ) + + def _max_submitted_review_time(node: dict[str, Any]) -> datetime | None: """Latest ``submittedAt`` among non-pending PR reviews, or ``None`` if none.""" best: datetime | None = None @@ -98,11 +113,25 @@ def _max_submitted_review_time(node: dict[str, Any]) -> datetime | None: return best +def _submitted_approval_reviews(node: dict[str, Any]) -> list[dict[str, Any]]: + """Return submitted approval reviews on a pull request.""" + return [ + review + for review in (node.get("reviews") or {}).get("nodes") or [] + if isinstance(review, dict) and review.get("state") == "APPROVED" + ] + + def _partition_attention_open_prs( nodes: list[dict[str, Any]], -) -> tuple[list[StatusReportItem], list[StatusReportItem]]: - """Split open PR nodes into reviewer vs creator attention lists.""" +) -> tuple[ + list[StatusReportItem], + list[StatusReportItem], + list[StatusReportItem], +]: + """Split open PR nodes into reviewer, second-approval, and creator lists.""" reviewer_out: list[StatusReportItem] = [] + second_approval_out: list[StatusReportItem] = [] creator_out: list[StatusReportItem] = [] for n in nodes: if n.get("state") != "OPEN" or n.get("mergedAt"): @@ -118,18 +147,24 @@ def _partition_attention_open_prs( "BEHIND", "DIRTY", ) - if ( + is_ready_for_review = ( not is_draft and mergeable == "MERGEABLE" and merge_state not in ("BEHIND", "DIRTY") - and (last_review is None or updated > last_review) - ): + ) + approval_reviews = _submitted_approval_reviews(n) + if is_ready_for_review and len(approval_reviews) == 1: + second_approval_out.append( + _second_approval_item_from_gql_node(n, approval_reviews[0]) + ) + if is_ready_for_review and (last_review is None or updated > last_review): reviewer_out.append(_item_from_gql_node(n)) if needs_creator_branch or (last_review is not None and last_review > updated): creator_out.append(_item_from_gql_node(n)) reviewer_out.sort(key=lambda x: x.number) + second_approval_out.sort(key=lambda x: x.number) creator_out.sort(key=lambda x: x.number) - return reviewer_out, creator_out + return reviewer_out, second_approval_out, creator_out def build_project_status_report( @@ -203,8 +238,8 @@ def post_gql(payload: dict[str, Any]) -> dict[str, Any]: attention_q, filter_bot_authors=False, ) - reviewer_attention, creator_attention = _partition_attention_open_prs( - attention_nodes + reviewer_attention, second_approval, creator_attention = ( + _partition_attention_open_prs(attention_nodes) ) backlog_q = sm.open_pr_backlog_query(repo, start_date) @@ -235,6 +270,7 @@ def post_gql(payload: dict[str, Any]) -> dict[str, Any]: merged_pull_requests=merged_items, opened_pull_requests=[_item_from_gql_node(n) for n in opened_pr_filtered], opened_issues=[_item_from_gql_node(n) for n in opened_issue_filtered], + second_approval_needed=second_approval, recently_updated_pull_requests=recently_items, reviewer_attention_needed=reviewer_attention, creator_attention_needed=creator_attention, diff --git a/backend/tests/test_status_report_api.py b/backend/tests/test_status_report_api.py index fc3f86d..0e9cfbe 100644 --- a/backend/tests/test_status_report_api.py +++ b/backend/tests/test_status_report_api.py @@ -153,6 +153,7 @@ async def override_conn(): ] assert body["opened_pull_requests"][0]["number"] == 11 assert body["opened_issues"][0]["number"] == 12 + assert body["second_approval_needed"] == [] assert body["recently_updated_pull_requests"] == [] assert body["reviewer_attention_needed"] == [] assert body["creator_attention_needed"] == [] @@ -372,6 +373,61 @@ def test_attention_sections_partition_open_prs( "milestone": None, "author": {"__typename": "User", "login": "u"}, }, + { + "number": 75, + "title": "Needs second approval", + "url": "https://github.com/test/repo/pull/75", + "createdAt": "2025-01-01T00:00:00Z", + "updatedAt": "2025-04-05T12:00:00Z", + "state": "OPEN", + "isDraft": False, + "mergedAt": None, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviews": { + "nodes": [ + { + "submittedAt": "2025-04-04T10:00:00Z", + "state": "APPROVED", + "author": {"__typename": "User", "login": "reviewer"}, + } + ] + }, + "additions": 1, + "deletions": 0, + "labels": {"nodes": []}, + "milestone": None, + "author": {"__typename": "User", "login": "u"}, + }, + { + "number": 76, + "title": "Already has two approvals", + "url": "https://github.com/test/repo/pull/76", + "createdAt": "2025-01-01T00:00:00Z", + "updatedAt": "2025-04-05T12:00:00Z", + "state": "OPEN", + "isDraft": False, + "mergedAt": None, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "reviews": { + "nodes": [ + { + "submittedAt": "2025-04-03T10:00:00Z", + "state": "APPROVED", + }, + { + "submittedAt": "2025-04-04T10:00:00Z", + "state": "APPROVED", + }, + ] + }, + "additions": 1, + "deletions": 0, + "labels": {"nodes": []}, + "milestone": None, + "author": {"__typename": "User", "login": "u"}, + }, ] async def override_conn(): @@ -388,7 +444,20 @@ async def override_conn(): assert r.status_code == 200 body = r.json() - assert {p["number"] for p in body["reviewer_attention_needed"]} == {70, 71} + assert {p["number"] for p in body["reviewer_attention_needed"]} == { + 70, + 71, + 75, + 76, + } + assert body["second_approval_needed"] == [ + { + "number": 75, + "title": "Needs second approval", + "html_url": "https://github.com/test/repo/pull/75", + "reviewer_login": "reviewer", + } + ] assert {p["number"] for p in body["creator_attention_needed"]} == {72, 73, 74} def test_recently_updated_pull_requests_excludes_opened_in_window( diff --git a/frontend/src/components/ProjectStatusPanel.jsx b/frontend/src/components/ProjectStatusPanel.jsx index 62a975c..204e008 100644 --- a/frontend/src/components/ProjectStatusPanel.jsx +++ b/frontend/src/components/ProjectStatusPanel.jsx @@ -51,6 +51,10 @@ const getInitialDateRange = () => { const emptyListMessage = 'None in this period.'; +const secondApprovalReviewerSuffix = (row) => ( + ({row.reviewer_login}) +); + const backlogAgeSuffix = (row) => { const n = row.days_since_update; if (typeof n !== 'number' || Number.isNaN(n)) { @@ -168,6 +172,7 @@ const ProjectStatusPanel = () => { const [startDate, setStartDate] = useState(null); const [endDate, setEndDate] = useState(null); + const [applyRevision, setApplyRevision] = useState(0); const [draftStartDate, setDraftStartDate] = useState(initial.start); const [draftEndDate, setDraftEndDate] = useState(initial.end); const [loading, setLoading] = useState(false); @@ -193,7 +198,7 @@ const ProjectStatusPanel = () => { return; } load(startDate, endDate); - }, [startDate, endDate, load]); + }, [startDate, endDate, applyRevision, load]); useEffect(() => { if (!startDate || !endDate) { @@ -215,6 +220,7 @@ const ProjectStatusPanel = () => { } setStartDate(draftStartDate); setEndDate(draftEndDate); + setApplyRevision((revision) => revision + 1); }; const windowLabel = @@ -344,6 +350,13 @@ const ProjectStatusPanel = () => { description="Issues opened in this date range." items={report.opened_issues} /> + { html_url: 'https://github.com/o/r/issues/2', }, ]; + const secondApproval = [ + { + number: 3, + title: 'Awaiting second approval', + html_url: 'https://github.com/o/r/pull/3', + reviewer_login: 'reviewer', + }, + ]; beforeEach(() => { vi.clearAllMocks(); @@ -47,6 +55,7 @@ describe('ProjectStatusPanel', () => { merged_pull_requests: merged, opened_pull_requests: [], opened_issues: issues, + second_approval_needed: secondApproval, recently_updated_pull_requests: [], reviewer_attention_needed: [], creator_attention_needed: [], @@ -83,6 +92,86 @@ describe('ProjectStatusPanel', () => { 'https://github.com/o/r/issues/2' ); expect(screen.getByText('Track work')).toBeInTheDocument(); + + const secondApprovalLink = screen.getByRole('link', { name: '#3' }); + expect(secondApprovalLink).toHaveAttribute( + 'href', + 'https://github.com/o/r/pull/3' + ); + expect(screen.getByText('Awaiting second approval')).toBeInTheDocument(); + expect(screen.getByText('(reviewer)').tagName).toBe('STRONG'); + }); + + it('retries after a failed Apply without changing the dates', async () => { + const user = userEvent.setup(); + api.fetchProjectStatusReport + .mockRejectedValueOnce(new Error('GitHub is unavailable')) + .mockResolvedValueOnce({ + start_date: '2025-04-03', + end_date: '2025-04-10', + merged_pull_requests: merged, + opened_pull_requests: [], + opened_issues: [], + recently_updated_pull_requests: [], + reviewer_attention_needed: [], + creator_attention_needed: [], + pr_backlog: [], + }); + + render(); + + await user.click(screen.getByRole('button', { name: 'Apply' })); + expect( + await screen.findByText('GitHub is unavailable') + ).toBeInTheDocument(); + + await user.click(screen.getByRole('button', { name: 'Apply' })); + + await waitFor(() => { + expect(api.fetchProjectStatusReport).toHaveBeenCalledTimes(2); + expect(screen.getByText('Fix bug')).toBeInTheDocument(); + }); + }); + + it('re-fetches when Apply is clicked again without changing the dates', async () => { + const user = userEvent.setup(); + api.fetchProjectStatusReport + .mockResolvedValueOnce({ + start_date: '2025-04-03', + end_date: '2025-04-10', + merged_pull_requests: [], + opened_pull_requests: [], + opened_issues: [], + recently_updated_pull_requests: [], + reviewer_attention_needed: [], + creator_attention_needed: [], + pr_backlog: [], + }) + .mockResolvedValueOnce({ + start_date: '2025-04-03', + end_date: '2025-04-10', + merged_pull_requests: merged, + opened_pull_requests: [], + opened_issues: [], + recently_updated_pull_requests: [], + reviewer_attention_needed: [], + creator_attention_needed: [], + pr_backlog: [], + }); + + render(); + + await user.click(screen.getByRole('button', { name: 'Apply' })); + await waitFor(() => { + expect(api.fetchProjectStatusReport).toHaveBeenCalledTimes(1); + }); + + await user.click(screen.getByRole('button', { name: 'Apply' })); + + await waitFor(() => { + expect(api.fetchProjectStatusReport).toHaveBeenCalledTimes(2); + expect(screen.getByText('Fix bug')).toBeInTheDocument(); + }); }); it('shows days since update in PR backlog section', async () => { @@ -130,6 +219,9 @@ describe('ProjectStatusPanel', () => { expect( screen.getByRole('heading', { name: 'Recently updated PRs' }) ).toBeInTheDocument(); + expect( + screen.getByRole('heading', { name: 'Needs second approval' }) + ).toBeInTheDocument(); expect( screen.getByRole('heading', { name: 'Reviewer attention needed' }) ).toBeInTheDocument(); diff --git a/frontend/src/utils/clipboard.js b/frontend/src/utils/clipboard.js index f789fcc..4f025c7 100644 --- a/frontend/src/utils/clipboard.js +++ b/frontend/src/utils/clipboard.js @@ -40,10 +40,20 @@ function statusItemAgeSuffix(row) { return ` (${n} ${unit})`; } +/** + * @param {Record} row + * @returns {string} + */ +function statusItemReviewerSuffix(row) { + const login = + row.reviewer_login != null ? String(row.reviewer_login).trim() : ''; + return login ? ` (${login})` : ''; +} + /** * Markdown lines: linked ``#number`` then space and title (for editors that read plain text). * - * @param {Array<{ number: number, title?: string, html_url?: string, days_since_update?: number }>} items + * @param {Array<{ number: number, title?: string, html_url?: string, days_since_update?: number, reviewer_login?: string }>} items * @returns {string} */ export function formatStatusSectionClipboardMarkdown(items) { @@ -51,12 +61,13 @@ export function formatStatusSectionClipboardMarkdown(items) { (items || []) .map((row) => { const title = (row.title != null ? String(row.title) : '').trim(); + const reviewer = statusItemReviewerSuffix(row); const age = statusItemAgeSuffix(row); const url = (row.html_url != null ? String(row.html_url) : '').trim(); if (!url) { - return `#${row.number} ${title}${age}`.trim(); + return `#${row.number} ${title}${reviewer}${age}`.trim(); } - return `[#${row.number}](${url}) ${title}${age}`.trim(); + return `[#${row.number}](${url}) ${title}${reviewer}${age}`.trim(); }) .join('\n') ); @@ -65,7 +76,7 @@ export function formatStatusSectionClipboardMarkdown(items) { /** * Minimal HTML (one line per item) so paste into Word / email keeps the number as a hyperlink. * - * @param {Array<{ number: number, title?: string, html_url?: string, days_since_update?: number }>} items + * @param {Array<{ number: number, title?: string, html_url?: string, days_since_update?: number, reviewer_login?: string }>} items * @returns {string} */ export function formatStatusSectionClipboardHtml(items) { @@ -74,13 +85,17 @@ export function formatStatusSectionClipboardHtml(items) { const title = escapeHtmlText( (row.title != null ? String(row.title) : '').trim() ); + const reviewer = statusItemReviewerSuffix(row); + const reviewerHtml = reviewer + ? ` ${escapeHtmlText(reviewer.trim())}` + : ''; const age = escapeHtmlText(statusItemAgeSuffix(row)); const url = (row.html_url != null ? String(row.html_url) : '').trim(); if (!url) { - return `#${row.number} ${title}${age}`; + return `#${row.number} ${title}${reviewerHtml}${age}`; } const href = escapeHtmlAttr(url); - return `#${row.number} ${title}${age}`; + return `#${row.number} ${title}${reviewerHtml}${age}`; }) .join('
'); } @@ -99,7 +114,7 @@ export async function copyTextToClipboard(text) { /** * Copy section rows with ``#number`` hyperlinked in HTML and Markdown link syntax in plain text. * - * @param {Array<{ number: number, title?: string, html_url?: string, days_since_update?: number }>} items + * @param {Array<{ number: number, title?: string, html_url?: string, days_since_update?: number, reviewer_login?: string }>} items * @returns {Promise} */ export async function copyStatusSectionToClipboard(items) { diff --git a/frontend/src/utils/clipboard.test.js b/frontend/src/utils/clipboard.test.js index 615e4ca..120859b 100644 --- a/frontend/src/utils/clipboard.test.js +++ b/frontend/src/utils/clipboard.test.js @@ -74,6 +74,21 @@ describe('formatStatusSectionClipboardMarkdown', () => { '[#50](https://github.com/o/r/pull/50) Stale open (7 days)\n[#51](https://github.com/o/r/pull/51) One day (1 day)' ); }); + + it('appends the reviewer handle for second-approval rows', () => { + expect( + formatStatusSectionClipboardMarkdown([ + { + number: 3, + title: 'Awaiting second approval', + html_url: 'https://github.com/o/r/pull/3', + reviewer_login: 'reviewer', + }, + ]) + ).toBe( + '[#3](https://github.com/o/r/pull/3) Awaiting second approval (reviewer)' + ); + }); }); describe('formatStatusSectionClipboardHtml', () => { @@ -97,6 +112,20 @@ describe('formatStatusSectionClipboardHtml', () => { expect(html).toContain('T (3 days)'); }); + it('bolds the reviewer handle in HTML clipboard output', () => { + const html = formatStatusSectionClipboardHtml([ + { + number: 3, + title: 'Awaiting second approval', + html_url: 'https://github.com/o/r/pull/3', + reviewer_login: 'reviewer', + }, + ]); + expect(html).toContain( + 'Awaiting second approval (reviewer)' + ); + }); + it('does not end with a trailing newline', () => { const html = formatStatusSectionClipboardHtml([ { number: 1, title: 'A', html_url: 'https://github.com/o/r/pull/1' },