Skip to content

Commit 24243ad

Browse files
committed
refactor(fria): simplify code from review findings
## Backend - Bulk INSERT for rights initialization (10 queries -> 1) - Parallelize rights upserts with Promise.all (10 sequential -> concurrent) - Extract recomputeAndPersistScore helper, replacing 3 copy-pasted blocks across addRiskItem, updateRiskItem, deleteRiskItem - Parallelize independent queries in updateFria and updateFriaRights with Promise.all - Remove double-read of rights in updateFriaRights (reuse result from score computation instead of re-fetching after commit) - Use FriaStatus and FriaRiskLevel enums instead of raw strings ## Frontend - Fix stale version history: refetch when currentVersion changes instead of caching permanently after first load
1 parent 2ecfb33 commit 24243ad

File tree

3 files changed

+61
-65
lines changed

3 files changed

+61
-65
lines changed

Clients/src/presentation/pages/ProjectView/Fria/FriaVersionHistory.tsx

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,6 @@ const FriaVersionHistory = ({ friaId, currentVersion, inline = false }: FriaVers
199199

200200
useEffect(() => {
201201
if (!inline && !panelOpen) return;
202-
if (versions.length > 0) return;
203202

204203
const fetchVersions = async () => {
205204
setIsLoading(true);
@@ -215,7 +214,7 @@ const FriaVersionHistory = ({ friaId, currentVersion, inline = false }: FriaVers
215214
};
216215

217216
fetchVersions();
218-
}, [inline, panelOpen, friaId, versions.length]);
217+
}, [inline, panelOpen, friaId, currentVersion]);
219218

220219
const handleRowToggle = (id: number) => {
221220
setExpandedRow((prev) => (prev === id ? null : id));

Servers/controllers/fria.ctrl.ts

Lines changed: 16 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { Request, Response } from "express";
22
import { sequelize } from "../database/db";
33
import { QueryTypes } from "sequelize";
4+
import { FriaStatus } from "../domain.layer/enums/fria-status.enum";
45
import { STATUS_CODE } from "../utils/statusCode.utils";
56
import { logProcessing, logSuccess, logFailure } from "../utils/logger/logHelper";
67
import { appendToAuditLedger } from "../utils/auditLedger.utils";
@@ -29,6 +30,7 @@ import {
2930
getFriaSnapshotsQuery,
3031
getFriaSnapshotByVersionQuery,
3132
computeFriaScore,
33+
recomputeAndPersistScore,
3234
} from "../utils/fria.utils";
3335

3436
const FILE_NAME = "fria.ctrl.ts";
@@ -188,9 +190,11 @@ export async function updateFria(req: Request, res: Response): Promise<Response>
188190

189191
const transaction = await sequelize.transaction();
190192
try {
191-
// Fetch rights and risk items for score computation
192-
const rights = await getFriaRightsQuery(existing.id!, organizationId);
193-
const riskItems = await getFriaRiskItemsQuery(existing.id!, organizationId);
193+
// Fetch rights and risk items in parallel for score computation
194+
const [rights, riskItems] = await Promise.all([
195+
getFriaRightsQuery(existing.id!, organizationId),
196+
getFriaRiskItemsQuery(existing.id!, organizationId),
197+
]);
194198

195199
// Merge user data with fresh scores in a single UPDATE
196200
const merged = { ...existing, ...data };
@@ -280,16 +284,16 @@ export async function updateFriaRights(req: Request, res: Response): Promise<Res
280284
try {
281285
await bulkUpsertFriaRightsQuery(friaId, rights, organizationId, transaction);
282286

283-
const updatedRights = await getFriaRightsQuery(friaId, organizationId);
284-
const riskItems = await getFriaRiskItemsQuery(friaId, organizationId);
287+
const [updatedRights, riskItems] = await Promise.all([
288+
getFriaRightsQuery(friaId, organizationId),
289+
getFriaRiskItemsQuery(friaId, organizationId),
290+
]);
285291
const scores = computeFriaScore(fria, updatedRights, riskItems);
286292

287293
await updateFriaQuery(friaId, scores, organizationId, userId, transaction);
288294

289295
await transaction.commit();
290296

291-
const result = await getFriaRightsQuery(friaId, organizationId);
292-
293297
try {
294298
await appendToAuditLedger({
295299
organizationId,
@@ -311,7 +315,7 @@ export async function updateFriaRights(req: Request, res: Response): Promise<Res
311315
organizationId,
312316
});
313317

314-
return res.status(200).json(STATUS_CODE[200](result));
318+
return res.status(200).json(STATUS_CODE[200](updatedRights));
315319
} catch (error) {
316320
await transaction.rollback();
317321
throw error;
@@ -393,14 +397,7 @@ export async function addRiskItem(req: Request, res: Response): Promise<Response
393397

394398
const item = await addFriaRiskItemQuery(friaId, data, organizationId);
395399

396-
// Recompute score
397-
const fria = await getFriaByIdQuery(friaId, organizationId);
398-
if (fria) {
399-
const rights = await getFriaRightsQuery(friaId, organizationId);
400-
const riskItems = await getFriaRiskItemsQuery(friaId, organizationId);
401-
const scores = computeFriaScore(fria, rights, riskItems);
402-
await updateFriaQuery(friaId, scores, organizationId, userId);
403-
}
400+
await recomputeAndPersistScore(friaId, organizationId, userId);
404401

405402
try {
406403
await appendToAuditLedger({
@@ -466,14 +463,7 @@ export async function updateRiskItem(req: Request, res: Response): Promise<Respo
466463
return res.status(404).json(STATUS_CODE[404]("Risk item not found"));
467464
}
468465

469-
// Recompute score
470-
const fria = await getFriaByIdQuery(friaId, organizationId);
471-
if (fria) {
472-
const rights = await getFriaRightsQuery(friaId, organizationId);
473-
const riskItems = await getFriaRiskItemsQuery(friaId, organizationId);
474-
const scores = computeFriaScore(fria, rights, riskItems);
475-
await updateFriaQuery(friaId, scores, organizationId, userId);
476-
}
466+
await recomputeAndPersistScore(friaId, organizationId, userId);
477467

478468
await logSuccess({
479469
eventType: "Update",
@@ -524,14 +514,7 @@ export async function deleteRiskItem(req: Request, res: Response): Promise<Respo
524514

525515
await deleteFriaRiskItemQuery(itemId, organizationId, undefined, friaId);
526516

527-
// Recompute score
528-
const fria = await getFriaByIdQuery(friaId, organizationId);
529-
if (fria) {
530-
const rights = await getFriaRightsQuery(friaId, organizationId);
531-
const riskItems = await getFriaRiskItemsQuery(friaId, organizationId);
532-
const scores = computeFriaScore(fria, rights, riskItems);
533-
await updateFriaQuery(friaId, scores, organizationId, userId);
534-
}
517+
await recomputeAndPersistScore(friaId, organizationId, userId);
535518

536519
try {
537520
await appendToAuditLedger({
@@ -758,7 +741,7 @@ export async function submitFria(req: Request, res: Response): Promise<Response>
758741

759742
await updateFriaQuery(
760743
friaId,
761-
{ status: "submitted", version: fria.version + 1 },
744+
{ status: FriaStatus.SUBMITTED, version: fria.version + 1 },
762745
organizationId,
763746
userId,
764747
transaction

Servers/utils/fria.utils.ts

Lines changed: 44 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { sequelize } from "../database/db";
22
import { QueryTypes, Transaction } from "sequelize";
3+
import { FriaRiskLevel } from "../domain.layer/enums/fria-status.enum";
34
import {
45
IFriaAssessmentJSON,
56
IFriaRight,
@@ -191,28 +192,24 @@ export const initializeFriaRightsQuery = async (
191192
organizationId: number,
192193
transaction?: Transaction
193194
) => {
194-
const results = [];
195-
for (const r of DEFAULT_RIGHTS) {
196-
const query = `
197-
INSERT INTO fria_rights (organization_id, fria_id, right_key, right_title, charter_ref, flagged, severity, confidence)
198-
VALUES (:organizationId, :friaId, :rightKey, :rightTitle, :charterRef, FALSE, 0, 0)
199-
ON CONFLICT (fria_id, right_key) DO NOTHING
200-
RETURNING *
201-
`;
202-
const result = await sequelize.query(query, {
203-
type: QueryTypes.INSERT,
204-
replacements: {
205-
organizationId,
206-
friaId,
207-
rightKey: r.right_key,
208-
rightTitle: r.right_title,
209-
charterRef: r.charter_ref,
210-
},
211-
transaction,
212-
});
213-
results.push(result);
214-
}
215-
return results;
195+
const valuesClauses = DEFAULT_RIGHTS.map(
196+
(_, i) => `(:orgId, :friaId, :rk${i}, :rt${i}, :cr${i}, FALSE, 0, 0)`
197+
).join(", ");
198+
199+
const replacements: Record<string, unknown> = { orgId: organizationId, friaId };
200+
DEFAULT_RIGHTS.forEach((r, i) => {
201+
replacements[`rk${i}`] = r.right_key;
202+
replacements[`rt${i}`] = r.right_title;
203+
replacements[`cr${i}`] = r.charter_ref;
204+
});
205+
206+
return sequelize.query(
207+
`INSERT INTO fria_rights (organization_id, fria_id, right_key, right_title, charter_ref, flagged, severity, confidence)
208+
VALUES ${valuesClauses}
209+
ON CONFLICT (fria_id, right_key) DO NOTHING
210+
RETURNING *`,
211+
{ type: QueryTypes.INSERT, replacements, transaction }
212+
);
216213
};
217214

218215
export const upsertFriaRightQuery = async (
@@ -270,11 +267,9 @@ export const bulkUpsertFriaRightsQuery = async (
270267
organizationId: number,
271268
transaction?: Transaction
272269
) => {
273-
const results = [];
274-
for (const right of rightsArray) {
275-
const result = await upsertFriaRightQuery(friaId, right, organizationId, transaction);
276-
results.push(result);
277-
}
270+
const results = await Promise.all(
271+
rightsArray.map((right) => upsertFriaRightQuery(friaId, right, organizationId, transaction))
272+
);
278273
return results;
279274
};
280275

@@ -568,9 +563,9 @@ export const computeFriaScore = (
568563
riskScore = Math.min(riskScore, 100);
569564

570565
// Determine level
571-
let riskLevel = "Low";
572-
if (riskScore >= 60) riskLevel = "High";
573-
else if (riskScore >= 30) riskLevel = "Medium";
566+
let riskLevel = FriaRiskLevel.LOW as string;
567+
if (riskScore >= 60) riskLevel = FriaRiskLevel.HIGH;
568+
else if (riskScore >= 30) riskLevel = FriaRiskLevel.MEDIUM;
574569

575570
// Compute completion percentage
576571
const sectionFields = [
@@ -615,3 +610,22 @@ export const computeFriaScore = (
615610
rights_flagged: rightsFlagged,
616611
};
617612
};
613+
614+
/**
615+
* Fetches current FRIA data, recomputes scores, and persists them.
616+
* Used after mutations to rights/risk items to keep scores in sync.
617+
*/
618+
export const recomputeAndPersistScore = async (
619+
friaId: number,
620+
organizationId: number,
621+
userId: number
622+
) => {
623+
const [fria, rights, riskItems] = await Promise.all([
624+
getFriaByIdQuery(friaId, organizationId),
625+
getFriaRightsQuery(friaId, organizationId),
626+
getFriaRiskItemsQuery(friaId, organizationId),
627+
]);
628+
if (!fria) return;
629+
const scores = computeFriaScore(fria, rights, riskItems);
630+
await updateFriaQuery(friaId, scores, organizationId, userId);
631+
};

0 commit comments

Comments
 (0)