Skip to content

Commit 6e86d7a

Browse files
Rylan-cgiclaude
andauthored
fix(schedules): automatic recalculation of derived figures during data entry (#291) (#344)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 700333f commit 6e86d7a

48 files changed

Lines changed: 5272 additions & 127 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

frontend/e2e/features/sch2/uc-sch2-001-report-costs/defects.md

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -107,14 +107,21 @@ at-rest state.
107107
- **Ticket:** [bcgov/nr-ilcr#291](https://github.com/bcgov/nr-ilcr/issues/291)*"[BUGFIX]: Automatic
108108
Recalculation for Schedule 1, 2, 3 and 4."* Raised app-wide, not just for Schedule 2; the behaviour
109109
difference is explained in a comment on that issue.
110-
- **Status:** OPEN — **with the dev.** The Schedule 2 dev will fix it when he gets a chance; QA confirms
111-
and closes this status line afterwards. **If recalculation-on-blur is restored, `happy-path.feature`
112-
will fail** — it currently pins the existing behaviour, so update that scenario as part of the fix
113-
rather than treating it as a regression (noted on the issue too).
114-
- **Test:** covered as the app behaves — `happy-path.feature` `@p0 @S01` asserts the at-rest figures are
115-
still shown *after* entry and *before* the Save, then the recomputed figures after it, so a change in
116-
either direction fails. No red, because this is traceable to a ratified architecture decision rather
117-
than a suspected fault.
110+
- **Status:** **RESOLVED 2026-08-21** — recalculation-on-blur restored across Schedules 1-4 on
111+
`fix/bugfix-291-auto-recalculation`. Spine AD-5 was amended: the "computed server-side, never
112+
accepted from a client" rule governs authority and persistence, and does not forbid a display-only
113+
mirror that keeps read-only cells tracking entry before Save. The mirror lives in one `derived.ts`
114+
per schedule, is never sent on a write, and is superseded by the server echo on every Save.
115+
`happy-path.feature` was updated as this entry instructed — its pre-save block now carries the
116+
recalculated figures, deliberately identical to the post-save block, so the scenario asserts
117+
mirror-vs-server agreement against a real backend rather than pinning the divergence. Trigger was
118+
on **blur**, not per keystroke, matching the legacy `f:ajax event="change"` handlers.
119+
Record: `_bmad-output/implementation-artifacts/defect-291-automatic-recalculation-schedules-1-4.md`.
120+
- **Test:** `happy-path.feature` `@p0 @S01` now asserts the RECALCULATED figures after entry and before
121+
the Save, and the same figures again after it — so it fails if the mirror and the server ever
122+
disagree, and fails again if recalculation-on-blur is removed. Frontend unit and RTL coverage sits in
123+
`components/schedule{1,2,3,4}/__tests__/derived.test.ts` and each page's `#291` tests, with
124+
expectations transcribed from the backend service tests.
118125

119126
---
120127

frontend/e2e/features/sch2/uc-sch2-001-report-costs/happy-path.feature

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -42,16 +42,20 @@ Feature: Schedule 2 — enter and save purchased log costs and log sales
4242
| Less Log Sales volume | 4 |
4343
| Less Log Sales cost | 1000 |
4444
| comments | E2E happy pathSchedule 2 |
45-
# DIVERGENCE FROM LEGACY, pinned deliberately (see defects.md DIV-1): legacy recomputed the derived
46-
# figures on each field's own `f:ajax event="change"`, so the totals moved as you typed. Every derived
47-
# value is now computed server-side and only refreshed by the save response (AD-5/AD-6), so BEFORE the
48-
# save the table still shows the at-rest figures. Asserting that here means a future change in either
49-
# direction is caught rather than silently absorbed.
45+
# DIV-1 RESOLVED (#291, 2026-08-21): legacy recomputed the derived figures on each field's own
46+
# `f:ajax event="change"`, so the totals moved as focus left a field. That behaviour is restored by a
47+
# display-only client mirror (spine AD-5 amended), so BEFORE the save the table already shows the
48+
# recalculated figures. The numbers below are deliberately IDENTICAL to the post-save block further
49+
# down: pre-save they come from the mirror, post-save from the server echo, so this scenario is now a
50+
# genuine mirror-vs-server agreement check — the AC5 "no jump on Save" guarantee, against a real
51+
# backend. The carried Wood Overhead and Total Company Logging rows must NOT move (they belong to
52+
# Schedules 3 and 1), which the unchanged rows in the post-save block still assert.
5053
Then the Schedule 2 document shows:
51-
| row | volume | cost | perUnit |
52-
| Subtotal: | 10 | 0 | 0.00 |
53-
| Net Purchased/Private Log Cost: | 10 | 0 | 0.00 |
54-
| Total Average Logging Costs: | 20 | 10 | 0.50 |
54+
| row | volume | cost | perUnit |
55+
| Subtotal: | 10 | 50,000 | 5,000.00 |
56+
| (less) Log Sales: | 4 | 1,000 | 250.00 |
57+
| Net Purchased/Private Log Cost: | 6 | 49,000 | 8,166.67 |
58+
| Total Average Logging Costs: | 16 | 49,010 | 3,063.13 |
5559
When I save Schedule 2
5660
Then I should see the message "Data saved successfully"
5761
# BR-06: subtotal, net purchased and total average are all recomputed server-side from the entered

frontend/src/components/schedule1/__tests__/Schedule1.test.tsx

Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,20 @@ const schedule1Doc = {
5454
forestMgmtAdminCost: 600000,
5555
lessSilvAdminCost: 150000,
5656
otherCosts: { volume: 8000, costSubtotal: 24000, perUnit: 3.0, count: 2 },
57+
// The server's own derived figures for THIS document, so the mirror can be compared against them
58+
// rather than against hand arithmetic in this file (code review 2026-08-21). Computed from
59+
// Schedule1Service's formulas: subtotal = 50000 line-12 + 600000 FMA + 24000 other = 674000;
60+
// total silviculture = 20000 actual - 150000 Sch3 admin (no accrued) = -130000; grand total = 544000;
61+
// its rate = 544000 / 54321 crown = 10.01; less-silv-admin rate = 150000 / 55 = 2727.27. The 143/144
62+
// and 140 rates are null because those volumes are absent from the fixture.
63+
subtotalCompanyLoggingCost: 674000,
64+
subtotalCompanyLoggingPerUnit: null,
65+
totalSilvicultureCost: -130000,
66+
totalSilviculturePerUnit: null,
67+
totalCompanyLoggingCost: 544000,
68+
totalCompanyLoggingPerUnit: 10.01,
69+
forestMgmtAdminPerUnit: null,
70+
lessSilvAdminPerUnit: 2727.27,
5771
warnings: [],
5872
}
5973

@@ -100,6 +114,210 @@ const problemBody = (status: number, detail: string) =>
100114
})
101115

102116
describe('Schedule1 editable page', () => {
117+
// ---- Defect #291: derived figures track data entry, on blur, before Save. -----------------------
118+
119+
/** A cost-table row's cells as text: [label, volume, cost, $/m³]. */
120+
const rowCells = (label: string | RegExp) => {
121+
const tr = screen.getByText(label).closest('tr')
122+
if (!tr) throw new Error(`no row for "${String(label)}"`)
123+
return within(tr)
124+
.getAllByRole('cell')
125+
.map((cell) => cell.textContent)
126+
}
127+
/** The read-only $/m³ cell of a cost-table row (index 3). */
128+
const rate = (label: string | RegExp) => rowCells(label)[3]
129+
/** The read-only cost cell (index 2). */
130+
const costOf = (label: string | RegExp) => rowCells(label)[2]
131+
132+
const SUBTOTAL = 'Subtotal Company Logging Cost (no Silviculture)'
133+
const GRAND_TOTAL = 'Total Company Logging Costs (Including total Silviculture Cost)'
134+
135+
test('typing alone moves nothing; blurring a logging cost recalculates the whole chain (#291)', async () => {
136+
server.use(http.get(URL, () => HttpResponse.json(schedule1Doc)))
137+
render(<Schedule1 />)
138+
const user = userEvent.setup()
139+
140+
const cost = await screen.findByLabelText('Standing Tree to Loaded Truck cost')
141+
// Seeded: 50000/1000 = 50.00. Subtotal = 50000 + 600000 FMA + 24000 other = 674000.
142+
expect(rate('Standing Tree to Loaded Truck')).toBe('50.00')
143+
expect(costOf(SUBTOTAL)).toBe('674,000')
144+
145+
await user.clear(cost)
146+
await user.type(cost, '100000')
147+
expect(rate('Standing Tree to Loaded Truck')).toBe('50.00') // not per keystroke
148+
expect(costOf(SUBTOTAL)).toBe('674,000')
149+
150+
await user.tab()
151+
expect(rate('Standing Tree to Loaded Truck')).toBe('100.00') // 100000/1000
152+
expect(costOf(SUBTOTAL)).toBe('724,000') // 100000 + 600000 + 24000
153+
// Grand total = subtotal + total silviculture (20000 − 150000 = −130000) = 594000,
154+
// over the Sch 3 crown volume 54321 -> 10.93.
155+
expect(costOf(GRAND_TOTAL)).toBe('594,000')
156+
expect(rate(GRAND_TOTAL)).toBe('10.93')
157+
})
158+
159+
test("a volume blur recalculates that row's $/m³ only (#291)", async () => {
160+
server.use(http.get(URL, () => HttpResponse.json(schedule1Doc)))
161+
render(<Schedule1 />)
162+
const user = userEvent.setup()
163+
164+
const volume = await screen.findByLabelText('Standing Tree to Loaded Truck volume')
165+
await user.clear(volume)
166+
await user.type(volume, '2000')
167+
await user.tab()
168+
169+
expect(rate('Standing Tree to Loaded Truck')).toBe('25.00') // 50000/2000
170+
// A volume is not part of any cost total, so the subtotal is unchanged.
171+
expect(costOf(SUBTOTAL)).toBe('674,000')
172+
})
173+
174+
test("139's RATE is mirrored while its COST stays the Schedule 3 pull (#291)", async () => {
175+
// Row 139's two halves come from different places, and the page treats them differently on
176+
// purpose: the cost is pulled from Schedule 3 and nothing on this page feeds it, but the VOLUME is
177+
// user-entered -- so `deriveSchedule1` computes a rate for 139 (derived.ts:98) even though it
178+
// computes no cost for it. The mirror therefore supersedes 139's rate and must NOT touch its cost.
179+
//
180+
// Written after a SonarQube refactor of a three-deep ternary (2026-08-21) exposed that this
181+
// opposite-precedence rule was pinned by no test: inverting it left all 268 green.
182+
server.use(http.get(URL, () => HttpResponse.json(schedule1Doc)))
183+
render(<Schedule1 />)
184+
const user = userEvent.setup()
185+
186+
const label = 'Less Silviculture Admin Costs'
187+
// Served state: 150,000 pulled from Schedule 3 over the fixture's volume of 55.
188+
expect(await screen.findByLabelText(`${label} volume`)).toHaveValue('55')
189+
expect(rate(label)).toBe('2,727.27') // 150,000 / 55
190+
191+
const volume = screen.getByLabelText(`${label} volume`)
192+
await user.clear(volume)
193+
await user.type(volume, '60000')
194+
await user.tab()
195+
196+
// The rate moved off the served figure -- the mirror owns it.
197+
expect(rate(label)).toBe('2.50') // 150,000 / 60,000
198+
// ...while the cost is still the Schedule 3 pull, untouched by the mirror.
199+
expect(costOf(label)).toBe('150,000')
200+
})
201+
202+
test('the Other Costs $/m³ tracks the volume entered on this page (#291)', async () => {
203+
server.use(http.get(URL, () => HttpResponse.json(schedule1Doc)))
204+
render(<Schedule1 />)
205+
const user = userEvent.setup()
206+
207+
const volume = await screen.findByLabelText('Subtotal Other Costs volume')
208+
// Seeded: subtotal 24000 over volume 8000 = 3.00.
209+
expect(rate(/^Subtotal Other Costs\(2\):$/)).toBe('3.00')
210+
211+
await user.clear(volume)
212+
await user.type(volume, '6000')
213+
await user.tab()
214+
expect(rate(/^Subtotal Other Costs\(2\):$/)).toBe('4.00') // 24000/6000
215+
216+
// Clearing it blanks the rate rather than dividing by zero.
217+
await user.clear(screen.getByLabelText('Subtotal Other Costs volume'))
218+
await user.tab()
219+
expect(rate(/^Subtotal Other Costs\(2\):$/)).toBe('—')
220+
})
221+
222+
test('Total Silviculture keeps legacy null semantics as costs are entered (#291)', async () => {
223+
// The pre-fill fixture has every volume set and every cost blank, with no Sch 3 admin pull, so
224+
// Total Silviculture must read blank — not a negative admin cost.
225+
server.use(http.get(URL, () => HttpResponse.json(prefillDoc)))
226+
render(<Schedule1 />)
227+
const user = userEvent.setup()
228+
229+
const label = 'Total Silviculture (As per Financial Statements)'
230+
expect(await screen.findByText(label)).toBeInTheDocument()
231+
expect(costOf(label)).toBe('—')
232+
233+
// Entering an Accrued cost alone is enough to produce a total (addition needs one operand).
234+
const accrued = screen.getByLabelText('Accrued less Actual $ Spent cost')
235+
await user.clear(accrued)
236+
await user.type(accrued, '50000')
237+
await user.tab()
238+
expect(costOf(label)).toBe('50,000')
239+
})
240+
241+
test('on load the mirror reproduces the served figures exactly (#291 AC5)', async () => {
242+
// A direct mirror-vs-server comparison with no edit involved: the fixture now carries the figures
243+
// Schedule1Service computes for it, so a mirror that rounds or propagates nulls differently fails
244+
// here. Schedule 1 is the page with the two easiest-to-conflate rules, and before the code review
245+
// its fixture carried no derived fields at all — so every assertion was self-referential.
246+
server.use(http.get(URL, () => HttpResponse.json(schedule1Doc)))
247+
render(<Schedule1 />)
248+
await screen.findByText('Standing Tree to Loaded Truck')
249+
250+
expect(costOf(SUBTOTAL)).toBe('674,000')
251+
expect(costOf(GRAND_TOTAL)).toBe('544,000')
252+
expect(rate(GRAND_TOTAL)).toBe('10.01')
253+
expect(rate('Standing Tree to Loaded Truck')).toBe('50.00')
254+
expect(rate('Less Silviculture Admin Costs')).toBe('2,727.27') // 150000 / 55
255+
expect(costOf('Total Silviculture (As per Financial Statements)')).toBe('-130,000')
256+
expect(rate(/^Subtotal Other Costs\(2\):$/)).toBe('3.00')
257+
})
258+
259+
test('the mirror equals the SERVER figures, before and after Save (#291 AC5)', async () => {
260+
// Asserted against the echo's own derived fields, not against a snapshot of the pre-Save render:
261+
// an editable page always renders the mirror, so comparing render-to-render compared the mirror
262+
// with itself and passed even with a wrong echo (code review 2026-08-21).
263+
server.use(
264+
http.get(URL, () => HttpResponse.json(schedule1Doc)),
265+
http.put(URL, () =>
266+
HttpResponse.json({
267+
...schedule1Doc,
268+
revisionCount: 4,
269+
lineItems: [{ costItemCode: 12, volume: 1000, cost: 100000, perUnit: 100.0 }],
270+
subtotalCompanyLoggingCost: 724000,
271+
totalSilvicultureCost: -130000,
272+
totalCompanyLoggingCost: 594000,
273+
totalCompanyLoggingPerUnit: 10.93,
274+
message: { key: 'dataSavedSuccesfullyInfoMsg', text: 'Data saved successfully' },
275+
}),
276+
),
277+
)
278+
render(<Schedule1 />)
279+
const user = userEvent.setup()
280+
281+
const cost = await screen.findByLabelText('Standing Tree to Loaded Truck cost')
282+
await user.clear(cost)
283+
await user.type(cost, '100000')
284+
await user.tab()
285+
286+
// The mirror must already agree with what the server will send: 100000 + 600000 + 24000 = 724000,
287+
// grand total 724000 - 130000 = 594000, rate 594000 / 54321 = 10.93.
288+
expect(costOf(SUBTOTAL)).toBe('724,000')
289+
expect(costOf(GRAND_TOTAL)).toBe('594,000')
290+
expect(rate(GRAND_TOTAL)).toBe('10.93')
291+
292+
await user.click(screen.getAllByRole('button', { name: /^save$/i })[0])
293+
expect(await screen.findByText('Data saved successfully')).toBeInTheDocument()
294+
295+
expect(costOf(SUBTOTAL)).toBe('724,000')
296+
expect(costOf(GRAND_TOTAL)).toBe('594,000')
297+
expect(rate(GRAND_TOTAL)).toBe('10.93')
298+
})
299+
300+
test('view mode renders the document figures as-is — no client recomputation (#291 AC7)', async () => {
301+
// A stored subtotal that deliberately disagrees with the line items: a recomputing view would show
302+
// 674,000 instead of the server's own figure.
303+
server.use(
304+
http.get(URL, () =>
305+
HttpResponse.json({
306+
...schedule1Doc,
307+
trackStatus: 'S',
308+
editable: false,
309+
subtotalCompanyLoggingCost: 999999,
310+
subtotalCompanyLoggingPerUnit: 111.11,
311+
}),
312+
),
313+
)
314+
render(<Schedule1 />)
315+
316+
expect(await screen.findByText(SUBTOTAL)).toBeInTheDocument()
317+
expect(costOf(SUBTOTAL)).toBe('999,999')
318+
expect(rate(SUBTOTAL)).toBe('111.11')
319+
})
320+
103321
test('editable:true renders an editable form; perUnit stays read-only (AC1)', async () => {
104322
server.use(http.get(URL, () => HttpResponse.json(schedule1Doc)))
105323
render(<Schedule1 />)

0 commit comments

Comments
 (0)