Skip to content

Commit 53424d2

Browse files
tofikwesttofikwest
authored
Merge branch 'main' into tofik/cs-189-auditor-view-permission-gating
2 parents c4a4da5 + 3fc78a8 commit 53424d2

7 files changed

Lines changed: 327 additions & 7 deletions

File tree

‎apps/api/src/policies/policies.service.spec.ts‎

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,205 @@ describe('PoliciesService', () => {
242242
});
243243
});
244244

245+
describe('acceptChanges', () => {
246+
const buildPendingPolicy = (overrides: Record<string, unknown> = {}) => ({
247+
id: 'pol_1',
248+
organizationId: 'org_abc',
249+
pendingVersionId: 'ver_1',
250+
approverId: 'mem_approver',
251+
frequency: 'yearly',
252+
...overrides,
253+
});
254+
255+
const mockTransactionTx = () => {
256+
db.$transaction.mockImplementation(
257+
async (callback: (tx: unknown) => Promise<unknown>) => {
258+
const tx = {
259+
policyVersion: { update: db.policyVersion.update },
260+
policy: { update: db.policy.update },
261+
};
262+
return callback(tx);
263+
},
264+
);
265+
};
266+
267+
it('publishes the pending version on a successful approve', async () => {
268+
const pendingVersion = {
269+
id: 'ver_1',
270+
version: 2,
271+
content: [{ type: 'paragraph' }],
272+
};
273+
db.policy.findUnique.mockResolvedValueOnce(buildPendingPolicy());
274+
db.policyVersion.findUnique.mockResolvedValueOnce(pendingVersion);
275+
db.member.findFirst.mockResolvedValueOnce({ id: 'mem_caller' });
276+
mockTransactionTx();
277+
278+
const result = await service.acceptChanges(
279+
'pol_1',
280+
'org_abc',
281+
{ approverId: 'mem_approver' },
282+
'usr_caller',
283+
);
284+
285+
expect(result).toEqual({ versionId: 'ver_1', version: 2 });
286+
expect(db.policyVersion.update).toHaveBeenCalledWith({
287+
where: { id: 'ver_1' },
288+
data: { publishedById: 'mem_caller' },
289+
});
290+
const policyUpdateArg = db.policy.update.mock.calls[0][0];
291+
expect(policyUpdateArg.data.status).toBe('published');
292+
expect(policyUpdateArg.data.currentVersionId).toBe('ver_1');
293+
expect(policyUpdateArg.data.pendingVersionId).toBeNull();
294+
expect(policyUpdateArg.data.approverId).toBeNull();
295+
expect(policyUpdateArg.data.signedBy).toEqual([]);
296+
});
297+
298+
it('succeeds when called via session impersonation — caller userId differs from approverId', async () => {
299+
// Simulates an admin impersonating the assigned approver:
300+
// the impersonated session's userId belongs to the approver, but
301+
// the authorization check only requires the body-supplied approverId
302+
// to match policy.approverId — which it does.
303+
const pendingVersion = {
304+
id: 'ver_1',
305+
version: 2,
306+
content: [],
307+
};
308+
db.policy.findUnique.mockResolvedValueOnce(buildPendingPolicy());
309+
db.policyVersion.findUnique.mockResolvedValueOnce(pendingVersion);
310+
db.member.findFirst.mockResolvedValueOnce({ id: 'mem_impersonated' });
311+
mockTransactionTx();
312+
313+
const result = await service.acceptChanges(
314+
'pol_1',
315+
'org_abc',
316+
{ approverId: 'mem_approver' },
317+
'usr_impersonated',
318+
);
319+
320+
expect(result).toEqual({ versionId: 'ver_1', version: 2 });
321+
expect(db.policyVersion.update).toHaveBeenCalledWith({
322+
where: { id: 'ver_1' },
323+
data: { publishedById: 'mem_impersonated' },
324+
});
325+
});
326+
327+
it('rejects when the body approverId does not match the assigned approver', async () => {
328+
db.policy.findUnique.mockResolvedValueOnce(buildPendingPolicy());
329+
330+
await expect(
331+
service.acceptChanges('pol_1', 'org_abc', { approverId: 'mem_wrong' }),
332+
).rejects.toThrow(/only the assigned approver/i);
333+
334+
expect(db.$transaction).not.toHaveBeenCalled();
335+
});
336+
337+
it('self-heals stale approverId when no pending version exists', async () => {
338+
const orgId = 'org_abc';
339+
const approverId = 'mem_approver';
340+
const stalePolicy = {
341+
id: 'pol_1',
342+
organizationId: orgId,
343+
pendingVersionId: null,
344+
approverId,
345+
};
346+
db.policy.findUnique.mockResolvedValueOnce(stalePolicy);
347+
db.policy.update.mockResolvedValueOnce({ ...stalePolicy, approverId: null });
348+
349+
await expect(
350+
service.acceptChanges('pol_1', orgId, { approverId }),
351+
).rejects.toThrow(/no pending changes/i);
352+
353+
expect(db.policy.update).toHaveBeenCalledWith({
354+
where: { id: 'pol_1' },
355+
data: { approverId: null },
356+
});
357+
expect(db.$transaction).not.toHaveBeenCalled();
358+
});
359+
360+
it('throws without mutating when the policy has no approval state at all', async () => {
361+
const orgId = 'org_abc';
362+
const cleanPolicy = {
363+
id: 'pol_1',
364+
organizationId: orgId,
365+
pendingVersionId: null,
366+
approverId: null,
367+
};
368+
db.policy.findUnique.mockResolvedValueOnce(cleanPolicy);
369+
370+
await expect(
371+
service.acceptChanges('pol_1', orgId, { approverId: 'mem_x' }),
372+
).rejects.toThrow(/no pending version/i);
373+
374+
expect(db.policy.update).not.toHaveBeenCalled();
375+
});
376+
});
377+
378+
describe('denyChanges', () => {
379+
it('reverts to draft on a successful deny when never published', async () => {
380+
db.policy.findUnique.mockResolvedValueOnce({
381+
id: 'pol_1',
382+
organizationId: 'org_abc',
383+
pendingVersionId: 'ver_1',
384+
approverId: 'mem_approver',
385+
lastPublishedAt: null,
386+
});
387+
db.policy.update.mockResolvedValueOnce({});
388+
389+
const result = await service.denyChanges('pol_1', 'org_abc', {
390+
approverId: 'mem_approver',
391+
});
392+
393+
expect(result).toEqual({ status: 'draft' });
394+
expect(db.policy.update).toHaveBeenCalledWith({
395+
where: { id: 'pol_1' },
396+
data: {
397+
status: 'draft',
398+
pendingVersionId: null,
399+
approverId: null,
400+
},
401+
});
402+
});
403+
404+
it('reverts to published on a successful deny when previously published', async () => {
405+
db.policy.findUnique.mockResolvedValueOnce({
406+
id: 'pol_1',
407+
organizationId: 'org_abc',
408+
pendingVersionId: 'ver_2',
409+
approverId: 'mem_approver',
410+
lastPublishedAt: new Date('2026-01-01'),
411+
});
412+
db.policy.update.mockResolvedValueOnce({});
413+
414+
const result = await service.denyChanges('pol_1', 'org_abc', {
415+
approverId: 'mem_approver',
416+
});
417+
418+
expect(result).toEqual({ status: 'published' });
419+
});
420+
421+
it('self-heals stale approverId when no pending version exists', async () => {
422+
const orgId = 'org_abc';
423+
const approverId = 'mem_approver';
424+
const stalePolicy = {
425+
id: 'pol_1',
426+
organizationId: orgId,
427+
pendingVersionId: null,
428+
approverId,
429+
};
430+
db.policy.findUnique.mockResolvedValueOnce(stalePolicy);
431+
db.policy.update.mockResolvedValueOnce({ ...stalePolicy, approverId: null });
432+
433+
await expect(
434+
service.denyChanges('pol_1', orgId, { approverId }),
435+
).rejects.toThrow(/no pending changes/i);
436+
437+
expect(db.policy.update).toHaveBeenCalledWith({
438+
where: { id: 'pol_1' },
439+
data: { approverId: null },
440+
});
441+
});
442+
});
443+
245444
describe('createVersion', () => {
246445
const organizationId = 'org_123';
247446
const policyId = 'pol_1';

‎apps/api/src/policies/policies.service.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1029,6 +1029,15 @@ export class PoliciesService {
10291029
}
10301030

10311031
if (!policy.pendingVersionId) {
1032+
if (policy.approverId) {
1033+
await db.policy.update({
1034+
where: { id: policyId },
1035+
data: { approverId: null },
1036+
});
1037+
throw new BadRequestException(
1038+
'This policy has no pending changes to approve. The stale approval request has been cleared — please ask the policy owner to re-submit if a new approval is needed.',
1039+
);
1040+
}
10321041
throw new BadRequestException('No pending version to approve');
10331042
}
10341043

@@ -1090,6 +1099,15 @@ export class PoliciesService {
10901099
}
10911100

10921101
if (!policy.pendingVersionId) {
1102+
if (policy.approverId) {
1103+
await db.policy.update({
1104+
where: { id: policyId },
1105+
data: { approverId: null },
1106+
});
1107+
throw new BadRequestException(
1108+
'This policy has no pending changes to deny. The stale approval request has been cleared — please ask the policy owner to re-submit if a new approval is needed.',
1109+
);
1110+
}
10931111
throw new BadRequestException('No pending version to deny');
10941112
}
10951113

‎apps/api/src/trigger/policies/update-policy-helpers.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -275,11 +275,18 @@ export async function updatePolicyInDatabase(
275275
}
276276

277277
await db.$transaction(async (tx) => {
278-
// Clear version references first to avoid FK constraint issues during deletion
278+
// Clear version references first to avoid FK constraint issues during deletion.
279+
// Clear approverId alongside pendingVersionId so the two fields never diverge
280+
// — any lingering approverId without a pending version produces the inconsistent
281+
// state behind CS-254/260/261 ("No pending version to approve").
279282
if (policy.versions.length > 0) {
280283
await tx.policy.update({
281284
where: { id: policyId },
282-
data: { currentVersionId: null, pendingVersionId: null },
285+
data: {
286+
currentVersionId: null,
287+
pendingVersionId: null,
288+
approverId: null,
289+
},
283290
});
284291
await tx.policyVersion.deleteMany({ where: { policyId } });
285292
}

‎apps/app/src/app/(app)/[orgId]/policies/[policyId]/components/PolicyAlerts.test.tsx‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ describe('PolicyAlerts', () => {
128128
const pendingPolicy = {
129129
...basePolicy,
130130
approverId: 'other-member',
131+
pendingVersionId: 'ver-1',
131132
approver: {
132133
id: 'other-member',
133134
user: { name: 'Other User', email: 'other@test.com' },
@@ -138,6 +139,22 @@ describe('PolicyAlerts', () => {
138139
);
139140
expect(screen.getByText('Pending approval')).toBeInTheDocument();
140141
});
142+
143+
it('does not render pending approval notice when pendingVersionId is null (stale approverId)', () => {
144+
const stalePolicy = {
145+
...basePolicy,
146+
approverId: 'other-member',
147+
pendingVersionId: null,
148+
approver: {
149+
id: 'other-member',
150+
user: { name: 'Other User', email: 'other@test.com' },
151+
},
152+
};
153+
const { container } = render(
154+
<PolicyAlerts policy={stalePolicy} isPendingApproval={true} />,
155+
);
156+
expect(container.innerHTML).toBe('');
157+
});
141158
});
142159

143160
describe('auditor permissions (no update)', () => {
@@ -163,6 +180,7 @@ describe('PolicyAlerts', () => {
163180
const pendingPolicy = {
164181
...basePolicy,
165182
approverId: 'other-member',
183+
pendingVersionId: 'ver-1',
166184
approver: {
167185
id: 'other-member',
168186
user: { name: 'Other User', email: 'other@test.com' },

‎apps/app/src/app/(app)/[orgId]/policies/[policyId]/components/PolicyPageTabs.test.tsx‎

Lines changed: 76 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,19 @@ vi.mock('../hooks/useAuditLogs', () => ({
6868

6969
// Mock child components to isolate testing
7070
vi.mock('./PolicyAlerts', () => ({
71-
PolicyAlerts: ({ policy }: { policy: unknown }) => (
72-
<div data-testid="policy-alerts">{policy ? 'alerts' : 'no-alerts'}</div>
71+
PolicyAlerts: ({
72+
policy,
73+
isPendingApproval,
74+
}: {
75+
policy: unknown;
76+
isPendingApproval: boolean;
77+
}) => (
78+
<div
79+
data-testid="policy-alerts"
80+
data-pending={String(isPendingApproval)}
81+
>
82+
{policy ? 'alerts' : 'no-alerts'}
83+
</div>
7384
),
7485
}));
7586

@@ -239,4 +250,67 @@ describe('PolicyPageTabs', () => {
239250
).not.toBeInTheDocument();
240251
});
241252
});
253+
254+
describe('isPendingApproval derivation', () => {
255+
beforeEach(() => {
256+
setMockPermissions(ADMIN_PERMISSIONS);
257+
});
258+
259+
it('is true only when both approverId and pendingVersionId are set', () => {
260+
const policy = {
261+
...basePolicy,
262+
approverId: 'mem-1',
263+
pendingVersionId: 'ver-1',
264+
};
265+
render(
266+
<PolicyPageTabs
267+
{...defaultProps}
268+
policy={policy}
269+
isPendingApproval={true}
270+
/>,
271+
);
272+
expect(screen.getByTestId('policy-alerts')).toHaveAttribute(
273+
'data-pending',
274+
'true',
275+
);
276+
});
277+
278+
it('is false when approverId is set but pendingVersionId is null (inconsistent state)', () => {
279+
const stalePolicy = {
280+
...basePolicy,
281+
approverId: 'mem-1',
282+
pendingVersionId: null,
283+
};
284+
render(
285+
<PolicyPageTabs
286+
{...defaultProps}
287+
policy={stalePolicy}
288+
isPendingApproval={true}
289+
/>,
290+
);
291+
expect(screen.getByTestId('policy-alerts')).toHaveAttribute(
292+
'data-pending',
293+
'false',
294+
);
295+
});
296+
297+
it('is false when pendingVersionId is set but approverId is null', () => {
298+
const policy = {
299+
...basePolicy,
300+
approverId: null,
301+
pendingVersionId: 'ver-1',
302+
};
303+
render(
304+
<PolicyPageTabs
305+
{...defaultProps}
306+
policy={policy}
307+
isPendingApproval={false}
308+
/>,
309+
);
310+
expect(screen.getByTestId('policy-alerts')).toHaveAttribute(
311+
'data-pending',
312+
'false',
313+
);
314+
});
315+
});
242316
});

0 commit comments

Comments
 (0)