Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,13 +1,66 @@
-- Add indexes to support faster `/v6/my-reviews` queries.

CREATE EXTENSION IF NOT EXISTS pg_trgm;
CREATE SCHEMA IF NOT EXISTS skills;
CREATE SCHEMA IF NOT EXISTS reviews;

CREATE EXTENSION IF NOT EXISTS fuzzystrmatch WITH SCHEMA skills;
CREATE EXTENSION IF NOT EXISTS pg_trgm WITH SCHEMA pg_catalog;

Choose a reason for hiding this comment

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

[⚠️ design]
The pg_trgm extension is being created in the pg_catalog schema. Typically, extensions are created in the public schema or a specific schema relevant to the application. Ensure that this is intentional and that the application logic accounts for this schema placement.

CREATE EXTENSION IF NOT EXISTS pgcrypto WITH SCHEMA reviews;
CREATE EXTENSION IF NOT EXISTS "uuid-ossp" WITH SCHEMA skills;

Choose a reason for hiding this comment

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

[⚠️ design]
The uuid-ossp extension is being created in the skills schema. This is unusual as extensions are often placed in the public schema. Verify that this schema choice is deliberate and that it aligns with the application's schema management strategy.


CREATE INDEX IF NOT EXISTS "challenge_status_type_track_created_at_idx"
ON "Challenge" ("status", "typeId", "trackId", "createdAt" DESC);

DROP INDEX IF EXISTS "challenge_name_idx";

CREATE INDEX IF NOT EXISTS "challenge_name_trgm_idx"
ON "Challenge" USING gin ("name" pg_catalog.gin_trgm_ops);

DO
$$
DECLARE
challenge_phase_schema TEXT;
BEGIN
SELECT n.nspname
INTO challenge_phase_schema
FROM pg_class c
JOIN pg_namespace n ON n.oid = c.relnamespace
WHERE c.relname = 'ChallengePhase'
AND c.relkind = 'r'
LIMIT 1;

IF challenge_phase_schema IS NULL THEN
RETURN;
END IF;

IF NOT EXISTS (
SELECT 1
FROM pg_class idx
JOIN pg_namespace ns ON ns.oid = idx.relnamespace
WHERE idx.relname = 'challenge_phase_order_idx'
AND ns.nspname = challenge_phase_schema
)
AND EXISTS (
SELECT 1
FROM pg_class idx
JOIN pg_namespace ns ON ns.oid = idx.relnamespace
WHERE idx.relname = 'challenge_phase_challenge_open_end_idx'
AND ns.nspname = challenge_phase_schema
AND pg_get_indexdef(idx.oid) LIKE '%("challengeId", "isOpen", "scheduledEndDate", "actualEndDate", name)%'

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The pg_get_indexdef function is used with a LIKE clause to check index definitions. This approach can be fragile if the index definition changes in ways not accounted for by the LIKE pattern. Consider using a more robust method to verify index structure, if possible.

)
THEN
EXECUTE format(
'ALTER INDEX %I.%I RENAME TO %I',
challenge_phase_schema,
'challenge_phase_challenge_open_end_idx',
'challenge_phase_order_idx'
);
END IF;
END
$$ LANGUAGE plpgsql;

CREATE INDEX IF NOT EXISTS "challenge_phase_challenge_open_end_idx"
ON "ChallengePhase" ("challengeId", "isOpen", "scheduledEndDate", "actualEndDate");

CREATE INDEX IF NOT EXISTS "challenge_name_trgm_idx"
ON "Challenge"
USING gin ("name" pg_catalog.gin_trgm_ops);
CREATE INDEX IF NOT EXISTS "challenge_phase_order_idx"
ON "ChallengePhase" ("challengeId", "isOpen", "scheduledEndDate", "actualEndDate", "name");
30 changes: 30 additions & 0 deletions prisma/migrations/20251121140000_member_access_view/migration.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
-- View to use in performance updates (PM-2206)
DROP VIEW IF EXISTS "challenges"."MemberChallengeAccess";

Choose a reason for hiding this comment

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

[❗❗ correctness]
Dropping the view without checking if it exists first could lead to errors if the view is not present. Consider using DROP VIEW IF EXISTS to avoid potential errors.


DO $$
BEGIN
IF EXISTS (
SELECT 1
FROM information_schema.tables
WHERE table_schema = 'resources'
AND table_name = 'Resource'
) THEN
EXECUTE format(
'CREATE VIEW %I.%I AS
SELECT DISTINCT r."challengeId", r."memberId"

Choose a reason for hiding this comment

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

[⚠️ performance]
Using SELECT DISTINCT can be resource-intensive. Ensure that this is necessary for the use case, as it may impact performance.

FROM resources."Resource" r
WHERE r."challengeId" IS NOT NULL

Choose a reason for hiding this comment

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

[⚠️ performance]
The condition r."challengeId" IS NOT NULL AND r."memberId" IS NOT NULL is used to filter out null values. Ensure that these columns are indexed if they are frequently queried, to improve performance.

AND r."memberId" IS NOT NULL',
current_schema(), 'MemberChallengeAccess'
);
ELSE
EXECUTE format(
'CREATE VIEW %I.%I AS
SELECT CAST(NULL AS TEXT) AS "challengeId",
CAST(NULL AS TEXT) AS "memberId"
WHERE FALSE',
current_schema(), 'MemberChallengeAccess'
);
END IF;
END;
$$;
15 changes: 15 additions & 0 deletions prisma/schema.prisma
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ model Challenge {
terms ChallengeTerm[]
skills ChallengeSkill[]
auditLogs AuditLog[]
memberAccesses MemberChallengeAccess[]

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The addition of memberAccesses in the Challenge model introduces a new relation to MemberChallengeAccess. Ensure that the MemberChallengeAccess model is correctly populated and maintained, as any orphaned records could lead to inconsistencies. Consider adding cascading delete or update rules if appropriate.


// Relation to ChallengeType (FK: typeId)
type ChallengeType @relation(fields: [typeId], references: [id])
Expand Down Expand Up @@ -161,6 +162,20 @@ model Challenge {
@@index([projectId, status])
}

//////////////////////////////////////////
// MemberChallengeAccess view – member/challenge pairs from resources schema
//////////////////////////////////////////

model MemberChallengeAccess {
challengeId String
memberId String

challenge Challenge @relation(fields: [challengeId], references: [id])

@@id([challengeId, memberId])

Choose a reason for hiding this comment

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

[❗❗ correctness]
The composite primary key @@id([challengeId, memberId]) in MemberChallengeAccess ensures uniqueness of member/challenge pairs. Verify that this constraint aligns with the intended data model and that no existing data violates this constraint.

@@map("MemberChallengeAccess")
}

//////////////////////////////////////////
// ChallengeType model
//////////////////////////////////////////
Expand Down
27 changes: 0 additions & 27 deletions src/common/helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -976,32 +976,6 @@ function calculateChallengeEndDate(challenge, data) {
// return result.toDate()
}

/**
* Lists challenge ids that given member has access to.
* @param {Number} memberId the member id
* @returns {Promise<Array>} an array of challenge ids represents challenges that given member has access to.
*/
async function listChallengesByMember(memberId) {
const token = await m2mHelper.getM2MToken();
let allIds = [];

try {
const result = await axios.get(`${config.RESOURCES_API_URL}/${memberId}/challenges`, {
headers: { Authorization: `Bearer ${token}` },
params: {
useScroll: true,
},
});

allIds = result.data || [];
} catch (e) {
// only log the error but don't throw it, so the following logic can still be executed.
logger.debug(`Failed to get challenges that accessible to the memberId ${memberId}`, e);
}

return allIds;
}

/**
* Lists resources that given member has in the given challenge.
* @param {Number} memberId the member id
Expand Down Expand Up @@ -1544,7 +1518,6 @@ module.exports = {
dedupeChallengeTerms,
postBusEvent,
calculateChallengeEndDate,
listChallengesByMember,
listResourcesByMemberAndChallenge,
getProjectDefaultTerms,
validateChallengeTerms,
Expand Down
15 changes: 14 additions & 1 deletion src/common/phase-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ class ChallengePhaseHelper {
timelineTemplateId
);
const { phaseDefinitionMap } = await this.getPhaseDefinitionsAndMap();
const challengePhaseIds = new Set(_.map(challengePhases, "phaseId"));

Choose a reason for hiding this comment

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

[⚠️ performance]
Consider using _.keyBy instead of _.map followed by new Set to directly create a set of phase IDs. This could improve performance by reducing the number of iterations over challengePhases.


// Ensure deterministic processing order based on the timeline template sequence
// DB returns phases ordered by dates, which can cause "fixedStartDate" logic below
Expand All @@ -107,9 +108,18 @@ class ChallengePhaseHelper {
const phaseFromTemplate = timelineTemplateMap.get(phase.phaseId);
const phaseDefinition = phaseDefinitionMap.get(phase.phaseId);
const newPhase = _.find(newPhases, (p) => p.phaseId === phase.phaseId);
const templatePredecessor = _.get(phaseFromTemplate, "predecessor");
// Prefer template predecessor only when that phase exists on the challenge, otherwise keep the stored link.
const resolvedPredecessor = _.isNil(phaseFromTemplate)
? phase.predecessor
: _.isNil(templatePredecessor)
? null
: challengePhaseIds.has(templatePredecessor)
? templatePredecessor
: phase.predecessor;
const updatedPhase = {
...phase,
predecessor: phaseFromTemplate && phaseFromTemplate.predecessor,
predecessor: resolvedPredecessor,
description: phaseDefinition.description,
};
if (updatedPhase.name === "Post-Mortem") {
Expand Down Expand Up @@ -157,6 +167,9 @@ class ChallengePhaseHelper {
const predecessorPhase = _.find(updatedPhases, {
phaseId: phase.predecessor,
});
if (_.isNil(predecessorPhase)) {
continue;

Choose a reason for hiding this comment

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

[⚠️ correctness]
The continue statement here skips processing phases without a valid predecessor. Ensure that this behavior is intentional and that skipping these phases won't lead to incorrect scheduling or logic errors.

}
if (phase.name === "Iterative Review") {
if (!iterativeReviewSet) {
if (_.isNil(phase.actualStartDate)) {
Expand Down
56 changes: 33 additions & 23 deletions src/services/ChallengeService.js
Original file line number Diff line number Diff line change
Expand Up @@ -938,23 +938,28 @@ async function searchChallenges(currentUser, criteria) {
});
}

let memberChallengeIds;
let currentUserChallengeIds;
let currentUserChallengeIdSet;
const requestedMemberId = !_.isNil(criteria.memberId)
? _.toString(criteria.memberId)
: null;
const currentUserMemberId =
currentUser && !_hasAdminRole && !_.get(currentUser, "isMachine", false)
? _.toString(currentUser.userId)
: null;
const memberIdForTaskFilter = requestedMemberId || currentUserMemberId;

// FIXME: This is wrong!
// if (!_.isUndefined(currentUser) && currentUser.handle) {
// accessQuery.push({ match_phrase: { createdBy: currentUser.handle } })
// }

if (criteria.memberId) {
memberChallengeIds = await helper.listChallengesByMember(criteria.memberId);
if (requestedMemberId) {
prismaFilter.where.AND.push({
id: { in: memberChallengeIds },
memberAccesses: {
some: {
memberId: requestedMemberId,
},
},
});
} else if (currentUser && !_hasAdminRole && !_.get(currentUser, "isMachine", false)) {
currentUserChallengeIds = await helper.listChallengesByMember(currentUser.userId);
memberChallengeIds = currentUserChallengeIds;
}

// FIXME: Tech Debt
Expand Down Expand Up @@ -991,9 +996,11 @@ async function searchChallenges(currentUser, criteria) {
}
} else if (excludeTasks) {
const taskFilter = [];
if (_.get(memberChallengeIds, "length", 0) > 0) {
if (memberIdForTaskFilter) {
taskFilter.push({
id: { in: memberChallengeIds },
memberAccesses: {
some: { memberId: memberIdForTaskFilter },
},
});
}
taskFilter.push({
Expand All @@ -1016,12 +1023,22 @@ async function searchChallenges(currentUser, criteria) {
const sortFilter = {};
sortFilter[sortByProp] = sortOrderProp;

const challengeInclude = currentUserMemberId
? {
...includeReturnFields,
memberAccesses: {
where: { memberId: currentUserMemberId },
select: { memberId: true },
},
}
: includeReturnFields;

const prismaQuery = {
...prismaFilter,
take: perPage,
skip: (page - 1) * perPage,
orderBy: [sortFilter],
include: includeReturnFields,
include: challengeInclude,
};

try {
Expand Down Expand Up @@ -1079,20 +1096,13 @@ async function searchChallenges(currentUser, criteria) {
if (!currentUser.isMachine && !_hasAdminRole) {
result.forEach((challenge) => {
_.unset(challenge, "billing");
});

if (!currentUserChallengeIds) {
currentUserChallengeIds = await helper.listChallengesByMember(currentUser.userId);
}

const accessibleIds = currentUserChallengeIds || [];
currentUserChallengeIdSet = currentUserChallengeIdSet || new Set(accessibleIds);

result.forEach((challenge) => {
if (!currentUserChallengeIdSet.has(challenge.id)) {
if (!_.get(challenge, "memberAccesses.length")) {
_.unset(challenge, "privateDescription");
}
_.unset(challenge, "memberAccesses");
});
} else {
result.forEach((challenge) => _.unset(challenge, "memberAccesses"));
}
} else {
result.forEach((challenge) => {
Expand Down