Skip to content

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

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 2 commits into from
Nov 2, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cef951a
Further fixes for /v6/my-reviews for big accounts
jmgasper Nov 2, 2025
14ea642
Merge pull request #141 from topcoder-platform/performance
jmgasper Nov 2, 2025
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 now() function is used multiple times in the SQL statements. Consider using a single variable to store the current timestamp to ensure consistency across the transaction, especially if the transaction takes a non-trivial amount of time to execute.

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();

-- 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())
ON CONFLICT ("resourceId")
DO UPDATE SET
"pendingAppealCount" = EXCLUDED."pendingAppealCount",
"updatedAt" = now();
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();

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]
There is no newline at the end of the file. While this is a minor 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.

COMMIT;
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 explicitly define its length in the database schema. This can help prevent potential issues with varying string lengths across different database systems.

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]
The type change from boolean | null to boolean for hasPendingAppealResponses may introduce issues if the previous logic depended on distinguishing between false and null. Ensure that the rest of the codebase correctly handles this change.

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 always non-null. If pendingAppealCount can be null, this logic may not behave as expected. Consider verifying the nullability of pendingAppealCount.

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