Skip to content

Further fixes for /v6/my-reviews for big accounts #141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 2, 2025
Merged

Further fixes for /v6/my-reviews for big accounts #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 121 additions & 0 deletions prisma/migrations/20251102001629_add_review_pending_summary/migration.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
-- CreateTable
CREATE TABLE "review_pending_summary" (
"resourceId" TEXT NOT NULL,
"pendingAppealCount" INTEGER NOT NULL,
"updatedAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,

CONSTRAINT "review_pending_summary_pkey" PRIMARY KEY ("resourceId")
);

-- CreateIndex
CREATE INDEX "review_pending_summary_updated_at_idx" ON "review_pending_summary"("updatedAt");

BEGIN;

-- Prime the summary table
INSERT INTO reviews.review_pending_summary ("resourceId", "pendingAppealCount", "updatedAt")
SELECT
rv."resourceId",
COUNT(*) AS "pendingAppealCount",
now()
FROM reviews.review rv
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The use of now() for updatedAt in the INSERT INTO statement could lead to inconsistencies if the transaction is long-running. Consider using CURRENT_TIMESTAMP to ensure consistency with the default value of the column.

JOIN reviews."reviewItem" ri
ON ri."reviewId" = rv.id
JOIN reviews."reviewItemComment" ric
ON ric."reviewItemId" = ri.id
JOIN reviews.appeal ap
ON ap."reviewItemCommentId" = ric.id
LEFT JOIN reviews."appealResponse" apr
ON apr."appealId" = ap.id
AND apr."resourceId" = rv."resourceId"
WHERE apr.id IS NULL
GROUP BY rv."resourceId"
ON CONFLICT ("resourceId")
DO UPDATE SET
"pendingAppealCount" = EXCLUDED."pendingAppealCount",
"updatedAt" = now();
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
Using now() here could lead to inconsistencies if the transaction is long-running. Consider using CURRENT_TIMESTAMP to ensure consistency with the default value of the column.


-- Helper to recompute a single resource
CREATE OR REPLACE FUNCTION reviews.update_review_pending_summary_for_resource(p_resource_id text)
RETURNS void
LANGUAGE plpgsql
AS $$
DECLARE
pending_count integer;
BEGIN
SELECT COUNT(*)
INTO pending_count
FROM reviews.review rv
JOIN reviews."reviewItem" ri
ON ri."reviewId" = rv.id
JOIN reviews."reviewItemComment" ric
ON ric."reviewItemId" = ri.id
JOIN reviews.appeal ap
ON ap."reviewItemCommentId" = ric.id
LEFT JOIN reviews."appealResponse" apr
ON apr."appealId" = ap.id
AND apr."resourceId" = rv."resourceId"
WHERE rv."resourceId" = p_resource_id
AND apr.id IS NULL;

IF pending_count > 0 THEN
INSERT INTO reviews.review_pending_summary ("resourceId", "pendingAppealCount", "updatedAt")
VALUES (p_resource_id, pending_count, now())
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The use of now() for updatedAt in the VALUES clause could lead to inconsistencies if the transaction is long-running. Consider using CURRENT_TIMESTAMP to ensure consistency with the default value of the column.

ON CONFLICT ("resourceId")
DO UPDATE SET
"pendingAppealCount" = EXCLUDED."pendingAppealCount",
"updatedAt" = now();
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
Using now() here could lead to inconsistencies if the transaction is long-running. Consider using CURRENT_TIMESTAMP to ensure consistency with the default value of the column.

ELSE
DELETE FROM reviews.review_pending_summary
WHERE "resourceId" = p_resource_id;
END IF;
END;
$$;

-- Triggers for the appeals table
CREATE OR REPLACE FUNCTION reviews.handle_appeal_change()
RETURNS trigger
LANGUAGE plpgsql
AS $$
BEGIN
PERFORM reviews.update_review_pending_summary_for_resource(NEW."resourceId");
RETURN NEW;
END;
$$;

DROP TRIGGER IF EXISTS appeal_pending_maintainer
ON reviews.appeal;

CREATE TRIGGER appeal_pending_maintainer
AFTER INSERT OR UPDATE OR DELETE ON reviews.appeal
FOR EACH ROW
EXECUTE FUNCTION reviews.handle_appeal_change();

-- Triggers for appeal responses
CREATE OR REPLACE FUNCTION reviews.handle_appeal_response_change()
RETURNS trigger
LANGUAGE plpgsql
AS $$
DECLARE
target_resource text;
BEGIN
IF TG_OP IN ('INSERT','UPDATE') THEN
target_resource := NEW."resourceId";
ELSE
target_resource := OLD."resourceId";
END IF;

PERFORM reviews.update_review_pending_summary_for_resource(target_resource);
RETURN COALESCE(NEW, OLD);
END;
$$;

DROP TRIGGER IF EXISTS appeal_response_pending_maintainer
ON reviews."appealResponse";

CREATE TRIGGER appeal_response_pending_maintainer
AFTER INSERT OR UPDATE OR DELETE ON reviews."appealResponse"
FOR EACH ROW
EXECUTE FUNCTION reviews.handle_appeal_response_change();

COMMIT;
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 style]
The file does not end with a newline character. While this is not a critical issue, it is generally a good practice to include a newline at the end of files to avoid potential issues with some tools and to adhere to POSIX standards.

9 changes: 9 additions & 0 deletions prisma/schema.prisma
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,15 @@ model appealResponse {
@@index([appealId, resourceId], map: "appeal_response_appeal_resource_idx") // Supports lookups for pending appeal responses by appeal and resource
}

model reviewPendingSummary {
resourceId String @id
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 maintainability]
Consider adding a @db.VarChar annotation to resourceId to specify the maximum length for consistency with other string fields in the schema.

pendingAppealCount Int
updatedAt DateTime @default(now())

@@index([updatedAt], map: "review_pending_summary_updated_at_idx")
@@map("review_pending_summary")
}

model challengeResult {
challengeId String
userId String
Expand Down
145 changes: 94 additions & 51 deletions src/api/my-review/myReview.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ interface ChallengeSummaryRow {
status: string;
hasIncompleteReviews: boolean | null;
incompletePhaseName: string | null;
hasPendingAppealResponses: boolean | null;
hasPendingAppealResponses: boolean;
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ correctness]
Changing hasPendingAppealResponses from boolean | null to boolean in the ChallengeSummaryRow interface may lead to issues if there are existing code paths that rely on the null value to represent a specific state. Ensure that all usages of this property are updated accordingly to handle the absence of a pending appeal response correctly.

isAppealsResponsePhaseOpen: boolean | null;
appealsResponsePhaseName: string | null;
}
Expand Down Expand Up @@ -156,6 +156,7 @@ export class MyReviewService {
whereFragments.push(Prisma.sql`c.status = 'ACTIVE'`);
}

const cteFragments: Prisma.Sql[] = [];
const baseJoins: Prisma.Sql[] = [];
const countJoins: Prisma.Sql[] = [];
const countExtras: Prisma.Sql[] = [];
Expand All @@ -168,25 +169,64 @@ export class MyReviewService {
);
}

cteFragments.push(
Prisma.sql`
member_resources AS (
SELECT
r.id,
r."challengeId",
r."roleId"
FROM resources."Resource" r
WHERE r."memberId" = ${normalizedUserId}
)
`,
);
cteFragments.push(
Prisma.sql`
appeals_phase AS (
SELECT DISTINCT ON (p."challengeId")
p."challengeId",
TRUE AS "isAppealsResponsePhaseOpen",
p.name AS "appealsResponsePhaseName"
FROM challenges."ChallengePhase" p
WHERE p."challengeId" IN (
SELECT DISTINCT mr."challengeId"
FROM member_resources mr
)
AND LOWER(p.name) IN ('appeals response', 'iterative appeals response')
AND p."isOpen" IS TRUE
ORDER BY
p."challengeId",
p."scheduledEndDate" DESC NULLS LAST,
p."actualEndDate" DESC NULLS LAST,
p.name ASC
)
`,
);

baseJoins.push(
Prisma.sql`
LEFT JOIN resources."Resource" r
JOIN member_resources r
ON r."challengeId" = c.id
AND r."memberId" = ${normalizedUserId}
`,
);
baseJoins.push(
Prisma.sql`
LEFT JOIN resources."ResourceRole" rr
ON rr.id = r."roleId"
`,
);

rowExtras.push(Prisma.sql`r."challengeId" IS NOT NULL`);
countExtras.push(
countJoins.push(
Prisma.sql`
EXISTS (
SELECT 1
FROM resources."Resource" r
WHERE r."challengeId" = c.id
AND r."memberId" = ${normalizedUserId}
)
JOIN member_resources r
ON r."challengeId" = c.id
`,
);
countJoins.push(
Prisma.sql`
LEFT JOIN resources."ResourceRole" rr
ON rr.id = r."roleId"
`,
);
} else {
Expand Down Expand Up @@ -237,10 +277,8 @@ export class MyReviewService {
0
)::bigint AS "completedReviews"
FROM reviews.review rv
INNER JOIN resources."Resource" rr
ON rr.id = rv."resourceId"
WHERE rr."challengeId" = c.id
) rp ON TRUE
WHERE rv."resourceId" = r.id
) rp ON r.id IS NOT NULL
`,
Prisma.sql`
LEFT JOIN LATERAL (
Expand Down Expand Up @@ -284,41 +322,40 @@ export class MyReviewService {
) deliverable_reviews ON r.id IS NOT NULL
`,
Prisma.sql`
LEFT JOIN LATERAL (
SELECT
EXISTS (
SELECT 1
FROM reviews.review rv_pending
JOIN reviews."reviewItem" ri
ON ri."reviewId" = rv_pending.id
JOIN reviews."reviewItemComment" ric
ON ric."reviewItemId" = ri.id
JOIN reviews.appeal ap
ON ap."reviewItemCommentId" = ric.id
LEFT JOIN reviews."appealResponse" apr
ON apr."appealId" = ap.id
AND apr."resourceId" = r.id
WHERE rv_pending."resourceId" = r.id
AND apr.id IS NULL
) AS "hasPendingAppealResponses"
) pending_appeals ON r.id IS NOT NULL
`,
Prisma.sql`
LEFT JOIN LATERAL (
SELECT
TRUE AS "isAppealsResponsePhaseOpen",
p.name AS "appealsResponsePhaseName"
FROM challenges."ChallengePhase" p
WHERE p."challengeId" = c.id
AND LOWER(p.name) IN ('appeals response', 'iterative appeals response')
AND p."isOpen" IS TRUE
ORDER BY
p."scheduledEndDate" DESC NULLS LAST,
p."actualEndDate" DESC NULLS LAST,
p.name ASC
LIMIT 1
) appeals_response_phase ON TRUE
LEFT JOIN reviews.review_pending_summary pending_appeals
ON pending_appeals."resourceId" = r.id
`,
];

if (!adminUser) {
metricJoins.push(
Prisma.sql`
LEFT JOIN appeals_phase appeals_response_phase
ON appeals_response_phase."challengeId" = c.id
`,
);
} else {
metricJoins.push(
Prisma.sql`
LEFT JOIN LATERAL (
SELECT
TRUE AS "isAppealsResponsePhaseOpen",
p.name AS "appealsResponsePhaseName"
FROM challenges."ChallengePhase" p
WHERE p."challengeId" = c.id
AND LOWER(p.name) IN ('appeals response', 'iterative appeals response')
AND p."isOpen" IS TRUE
ORDER BY
p."scheduledEndDate" DESC NULLS LAST,
p."actualEndDate" DESC NULLS LAST,
p.name ASC
LIMIT 1
) appeals_response_phase ON TRUE
`,
);
}

metricJoins.push(
Prisma.sql`
LEFT JOIN LATERAL (
SELECT
Expand All @@ -329,7 +366,7 @@ export class MyReviewService {
LIMIT 1
) cr ON TRUE
`,
];
);

const joinClause = joinSqlFragments(
[...baseJoins, ...metricJoins],
Expand Down Expand Up @@ -442,7 +479,12 @@ export class MyReviewService {
: fallbackOrderFragments;
const orderClause = joinSqlFragments(orderFragments, Prisma.sql`, `);

const withClause = cteFragments.length
? Prisma.sql`WITH ${joinSqlFragments(cteFragments, Prisma.sql`, `)} `
: Prisma.sql``;

const countQuery = Prisma.sql`
${withClause}
SELECT COUNT(*) AS "total"
FROM challenges."Challenge" c
${countJoinClause}
Expand Down Expand Up @@ -475,6 +517,7 @@ export class MyReviewService {
}

const rowQuery = Prisma.sql`
${withClause}
SELECT
c.id AS "challengeId",
c.name AS "challengeName",
Expand All @@ -491,7 +534,7 @@ export class MyReviewService {
cw.winners AS "winners",
deliverable_reviews."hasIncompleteReviews" AS "hasIncompleteReviews",
deliverable_reviews."incompletePhaseName" AS "incompletePhaseName",
pending_appeals."hasPendingAppealResponses" AS "hasPendingAppealResponses",
COALESCE(pending_appeals."pendingAppealCount" > 0, FALSE) AS "hasPendingAppealResponses",
Copy link

@github-actions github-actions bot Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The use of COALESCE(pending_appeals."pendingAppealCount" > 0, FALSE) assumes that pendingAppealCount is never NULL. If there are scenarios where pendingAppealCount could be NULL, this logic might not behave as expected. Consider verifying that pendingAppealCount is always initialized or handle the NULL case explicitly.

appeals_response_phase."isAppealsResponsePhaseOpen" AS "isAppealsResponsePhaseOpen",
appeals_response_phase."appealsResponsePhaseName" AS "appealsResponsePhaseName",
c.status AS "status"
Expand Down