feat(github-connector): exclude self-reviews from review counts (#20050)
## Summary - Adds an `isSelfReview` boolean to the consolidated `PullRequestReview` record (`true` when the reviewer is the same contributor as the PR author). - Filters `isSelfReview === true` rows out of the top-reviewers leaderboard (`top-contributors`) and per-contributor review stats (`contributor-stats`) so contributors are credited only for reviews on **other people's** PRs. - Both the live ingestion path (`fetch-prs`) and the recompute job (`recompute-pull-request-reviews`) now thread `prAuthorId` to `buildConsolidatedRow`, so re-running either is sufficient to backfill existing rows — no extra migration needed. ## Test plan - [x] `yarn test src/modules/github/pull-request-review/utils/consolidate-reviews.integration-test.ts` — 20 tests passing, including new coverage for the `isSelfReview` helper and `buildConsolidatedRow` payload. - [ ] After merge: re-run `fetch-prs` (or `recompute-pull-request-reviews`) once on a target workspace to backfill `isSelfReview` on existing `PullRequestReview` rows, then verify the top-reviewers leaderboard no longer credits self-reviews. Made with [Cursor](https://cursor.com)
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "github-connector",
|
||||
"version": "0.1.0",
|
||||
"version": "0.2.0",
|
||||
"license": "MIT",
|
||||
"engines": {
|
||||
"node": "^24.5.0",
|
||||
|
||||
+1
@@ -77,6 +77,7 @@ describe('PullRequestReview object', () => {
|
||||
expect(names).toContain('firstSubmittedAt');
|
||||
expect(names).toContain('lastSubmittedAt');
|
||||
expect(names).toContain('eventCount');
|
||||
expect(names).toContain('isSelfReview');
|
||||
expect(names).toContain('reviewer');
|
||||
expect(names).toContain('pullRequest');
|
||||
expect(names).toContain('reviewEvents');
|
||||
|
||||
+6
-1
@@ -298,7 +298,12 @@ const handler = async (
|
||||
const res = await client.query({
|
||||
pullRequestReviews: {
|
||||
__args: {
|
||||
filter: { reviewerId: { eq: contributorId } },
|
||||
filter: {
|
||||
and: [
|
||||
{ reviewerId: { eq: contributorId } },
|
||||
{ isSelfReview: { eq: false } },
|
||||
],
|
||||
},
|
||||
orderBy: [{ firstSubmittedAt: 'DescNullsLast' }],
|
||||
first: PAGE_SIZE,
|
||||
after: cursor,
|
||||
|
||||
+1
@@ -198,6 +198,7 @@ const handler = async (
|
||||
const res = await client.query({
|
||||
pullRequestReviews: {
|
||||
__args: {
|
||||
filter: { isSelfReview: { eq: false } },
|
||||
orderBy: [{ firstSubmittedAt: 'DescNullsLast' }],
|
||||
first: PAGE_SIZE,
|
||||
after: cursor,
|
||||
|
||||
+1
@@ -15,5 +15,6 @@ export async function batchUpsertConsolidatedReviews(
|
||||
eventCount: true,
|
||||
reviewerId: true,
|
||||
pullRequestId: true,
|
||||
isSelfReview: true,
|
||||
}) as Promise<PullRequestReviewRow[]>;
|
||||
}
|
||||
|
||||
+11
-2
@@ -23,7 +23,10 @@ type ReviewEventNode = {
|
||||
reviewerId: string | null;
|
||||
pullRequestId: string | null;
|
||||
reviewer: { ghLogin: string | null } | null;
|
||||
pullRequest: { githubNumber: number | null } | null;
|
||||
pullRequest: {
|
||||
githubNumber: number | null;
|
||||
authorId: string | null;
|
||||
} | null;
|
||||
};
|
||||
|
||||
type GroupContext = {
|
||||
@@ -31,6 +34,7 @@ type GroupContext = {
|
||||
reviewerId: string | null;
|
||||
prNumber: number | null;
|
||||
reviewerLogin: string | null;
|
||||
prAuthorId: string | null;
|
||||
events: { state: ReviewEventState; submittedAt: string | null }[];
|
||||
};
|
||||
|
||||
@@ -68,7 +72,7 @@ const handler = async (_event: RoutePayload<unknown>) => {
|
||||
reviewerId: true,
|
||||
pullRequestId: true,
|
||||
reviewer: { ghLogin: true },
|
||||
pullRequest: { githubNumber: true },
|
||||
pullRequest: { githubNumber: true, authorId: true },
|
||||
},
|
||||
},
|
||||
pageInfo: { hasNextPage: true, endCursor: true },
|
||||
@@ -95,6 +99,7 @@ const handler = async (_event: RoutePayload<unknown>) => {
|
||||
reviewerId: node.reviewerId,
|
||||
prNumber: node.pullRequest?.githubNumber ?? null,
|
||||
reviewerLogin: node.reviewer?.ghLogin ?? null,
|
||||
prAuthorId: node.pullRequest?.authorId ?? null,
|
||||
events: [],
|
||||
};
|
||||
groups.set(key, group);
|
||||
@@ -105,6 +110,9 @@ const handler = async (_event: RoutePayload<unknown>) => {
|
||||
if (group.prNumber === null && node.pullRequest?.githubNumber != null) {
|
||||
group.prNumber = node.pullRequest.githubNumber;
|
||||
}
|
||||
if (group.prAuthorId === null && node.pullRequest?.authorId) {
|
||||
group.prAuthorId = node.pullRequest.authorId;
|
||||
}
|
||||
group.events.push({
|
||||
state: node.state,
|
||||
submittedAt: node.submittedAt,
|
||||
@@ -123,6 +131,7 @@ const handler = async (_event: RoutePayload<unknown>) => {
|
||||
prNumber: group.prNumber,
|
||||
reviewerLogin: group.reviewerLogin,
|
||||
events: group.events,
|
||||
prAuthorId: group.prAuthorId,
|
||||
}),
|
||||
);
|
||||
}
|
||||
|
||||
+13
@@ -24,6 +24,9 @@ export const REVIEW_EVENT_COUNT_FIELD_UNIVERSAL_IDENTIFIER =
|
||||
export const PULL_REQUEST_REVIEW_CREATED_AT_FIELD_UNIVERSAL_IDENTIFIER =
|
||||
'b4d61187-1b78-5d6e-9752-79f994ff6d55';
|
||||
|
||||
export const REVIEW_IS_SELF_REVIEW_FIELD_UNIVERSAL_IDENTIFIER =
|
||||
'c1f3e8a2-9b5d-4e7f-8a6c-2d4b6f8e1a93';
|
||||
|
||||
enum ReviewState {
|
||||
APPROVED = 'APPROVED',
|
||||
CHANGES_REQUESTED = 'CHANGES_REQUESTED',
|
||||
@@ -116,5 +119,15 @@ export default defineObject({
|
||||
icon: 'IconHash',
|
||||
defaultValue: 0,
|
||||
},
|
||||
{
|
||||
universalIdentifier: REVIEW_IS_SELF_REVIEW_FIELD_UNIVERSAL_IDENTIFIER,
|
||||
name: 'isSelfReview',
|
||||
type: FieldType.BOOLEAN,
|
||||
label: 'Self review',
|
||||
description:
|
||||
'True when the reviewer is the same contributor as the PR author. Self reviews are excluded from review-count aggregations (top reviewers, contributor stats, etc.) so contributors are credited only for reviews on other people\u2019s PRs.',
|
||||
icon: 'IconUserCheck',
|
||||
defaultValue: false,
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
+1
@@ -8,4 +8,5 @@ export type PullRequestReviewRow = {
|
||||
eventCount?: number | null;
|
||||
reviewerId?: string | null;
|
||||
pullRequestId?: string | null;
|
||||
isSelfReview?: boolean | null;
|
||||
};
|
||||
|
||||
+17
@@ -12,6 +12,7 @@ export type ConsolidatedReviewUpsertInput = {
|
||||
eventCount: number;
|
||||
reviewerId: string | null;
|
||||
pullRequestId: string;
|
||||
isSelfReview: boolean;
|
||||
};
|
||||
|
||||
export type BuildConsolidatedRowParams = {
|
||||
@@ -20,8 +21,23 @@ export type BuildConsolidatedRowParams = {
|
||||
prNumber: number | null;
|
||||
reviewerLogin: string | null;
|
||||
events: ReviewEventForConsolidation[];
|
||||
/**
|
||||
* Author of the PR being reviewed. When non-null and equal to `reviewerId`
|
||||
* the consolidated row is flagged as a self-review so downstream
|
||||
* aggregations (top reviewers, contributor stats, etc.) can exclude it.
|
||||
*/
|
||||
prAuthorId?: string | null;
|
||||
};
|
||||
|
||||
export const isSelfReview = (
|
||||
reviewerId: string | null,
|
||||
prAuthorId: string | null | undefined,
|
||||
): boolean =>
|
||||
reviewerId !== null &&
|
||||
prAuthorId !== null &&
|
||||
prAuthorId !== undefined &&
|
||||
reviewerId === prAuthorId;
|
||||
|
||||
export const buildReviewKey = (
|
||||
pullRequestId: string,
|
||||
reviewerId: string | null,
|
||||
@@ -49,5 +65,6 @@ export const buildConsolidatedRow = (
|
||||
eventCount: verdict.eventCount,
|
||||
reviewerId: params.reviewerId,
|
||||
pullRequestId: params.pullRequestId,
|
||||
isSelfReview: isSelfReview(params.reviewerId, params.prAuthorId ?? null),
|
||||
};
|
||||
};
|
||||
|
||||
+66
@@ -8,6 +8,7 @@ import {
|
||||
buildConsolidatedRow,
|
||||
buildReviewKey,
|
||||
buildConsolidatedTitle,
|
||||
isSelfReview,
|
||||
} from 'src/modules/github/pull-request-review/utils/build-consolidated-row';
|
||||
|
||||
const evt = (
|
||||
@@ -140,6 +141,71 @@ describe('buildConsolidatedRow', () => {
|
||||
eventCount: 3,
|
||||
reviewerId: 'rev-2',
|
||||
pullRequestId: 'pr-1',
|
||||
isSelfReview: false,
|
||||
});
|
||||
});
|
||||
|
||||
it('flags isSelfReview when prAuthorId equals reviewerId', () => {
|
||||
const row = buildConsolidatedRow({
|
||||
pullRequestId: 'pr-1',
|
||||
reviewerId: 'alice',
|
||||
prNumber: 7,
|
||||
reviewerLogin: 'alice',
|
||||
prAuthorId: 'alice',
|
||||
events: [evt('COMMENTED', '2025-04-01T10:00:00Z')],
|
||||
});
|
||||
expect(row.isSelfReview).toBe(true);
|
||||
});
|
||||
|
||||
it('does not flag isSelfReview when prAuthorId differs from reviewerId', () => {
|
||||
const row = buildConsolidatedRow({
|
||||
pullRequestId: 'pr-1',
|
||||
reviewerId: 'alice',
|
||||
prNumber: 7,
|
||||
reviewerLogin: 'alice',
|
||||
prAuthorId: 'bob',
|
||||
events: [evt('APPROVED', '2025-04-01T10:00:00Z')],
|
||||
});
|
||||
expect(row.isSelfReview).toBe(false);
|
||||
});
|
||||
|
||||
it('does not flag isSelfReview when prAuthorId is missing', () => {
|
||||
const row = buildConsolidatedRow({
|
||||
pullRequestId: 'pr-1',
|
||||
reviewerId: 'alice',
|
||||
prNumber: 7,
|
||||
reviewerLogin: 'alice',
|
||||
events: [evt('APPROVED', '2025-04-01T10:00:00Z')],
|
||||
});
|
||||
expect(row.isSelfReview).toBe(false);
|
||||
});
|
||||
|
||||
it('does not flag isSelfReview when reviewerId is null (ghost reviewer)', () => {
|
||||
const row = buildConsolidatedRow({
|
||||
pullRequestId: 'pr-1',
|
||||
reviewerId: null,
|
||||
prNumber: 7,
|
||||
reviewerLogin: null,
|
||||
prAuthorId: null,
|
||||
events: [evt('COMMENTED', '2025-04-01T10:00:00Z')],
|
||||
});
|
||||
expect(row.isSelfReview).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('isSelfReview', () => {
|
||||
it('returns true only when both ids are present and equal', () => {
|
||||
expect(isSelfReview('a', 'a')).toBe(true);
|
||||
});
|
||||
|
||||
it('returns false when ids differ', () => {
|
||||
expect(isSelfReview('a', 'b')).toBe(false);
|
||||
});
|
||||
|
||||
it('returns false when either side is null/undefined', () => {
|
||||
expect(isSelfReview(null, 'a')).toBe(false);
|
||||
expect(isSelfReview('a', null)).toBe(false);
|
||||
expect(isSelfReview('a', undefined)).toBe(false);
|
||||
expect(isSelfReview(null, null)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
+9
@@ -110,9 +110,16 @@ const handler = async (event: RoutePayload<FetchPrsPayload>) => {
|
||||
reviewerId: string | null;
|
||||
prNumber: number;
|
||||
reviewerLogin: string | null;
|
||||
prAuthorId: string | null;
|
||||
events: { state: ReviewEventState; submittedAt: string | null }[];
|
||||
};
|
||||
|
||||
const authorIdByPullRequestId = new Map<string, string | null>();
|
||||
for (const pr of prData) {
|
||||
const id = prIdByNumber.get(pr.githubNumber);
|
||||
if (id) authorIdByPullRequestId.set(id, pr.authorId);
|
||||
}
|
||||
|
||||
let skippedReviews = 0;
|
||||
const reviewEventData: ReviewEventInput[] = [];
|
||||
const groups = new Map<GroupKey, GroupContext>();
|
||||
@@ -141,6 +148,7 @@ const handler = async (event: RoutePayload<FetchPrsPayload>) => {
|
||||
reviewerId,
|
||||
prNumber: pr.number,
|
||||
reviewerLogin: review.author?.login ?? null,
|
||||
prAuthorId: authorIdByPullRequestId.get(pullRequestId) ?? null,
|
||||
events: [],
|
||||
};
|
||||
groups.set(key, group);
|
||||
@@ -177,6 +185,7 @@ const handler = async (event: RoutePayload<FetchPrsPayload>) => {
|
||||
prNumber: group.prNumber,
|
||||
reviewerLogin: group.reviewerLogin,
|
||||
events: group.events,
|
||||
prAuthorId: group.prAuthorId,
|
||||
}),
|
||||
);
|
||||
const consolidatedRecords = await timed(
|
||||
|
||||
Reference in New Issue
Block a user