Skip to content

Commit bb262dc

Browse files
Rylan-cgiclaude
andcommitted
fix(schedule10): address the PR #305 review comments
All three "should fix" items from SScholefield's review. 1. DEAD persist(Runnable) OVERLOAD -- already gone. Removed during the SonarCloud pass for the same reason the review gives: once all seven call sites carried a resource-specific message key, the generic overload was unreachable. The ScheduleNotSavedException import went with it. 2. IDENTICAL if/else BODIES at the duplicate-cost branch. Real, and mine: when the read path changed from summing duplicates to last-row-wins, both branches became the same put. Collapsed to a conditional LOG.warn followed by one put. The containsKey check stays -- a present-but-null entry is a real cost row, so get() != null would miss a duplicate. Now in Schedule10DocumentAssembler. 3. THE "22 KEYS" CLAIM in InvalidTflNumberException. Real, and worse than the review could see from the diff. Recounting showed BOTH tables now hold 20 identical live TFL keys -- so the "21 versus 20" correction went stale inside the very commit that demoted 52B. That is a hard-coded count drifting for the third time in this story. So rather than write a fourth number, both javadocs now state the PROPERTY the code actually depends on: the two tables accept and reject the same values, and here a single table answers both the accept question and the derive question. 52B keeps a sentence because it is the one real difference in the two tables' history. Each note says explicitly why no count is quoted, so the next reader does not helpfully add one back. Verification: 1151 unit tests, 0 failures, including the six cost-map guards that cover the collapsed branch; 0 checkstyle violations in schedule10. Not addressed here, by agreement: the ROAD_CONSTRUCTION_REPORT_SEQ pre-deploy item is a process blocker rather than a code change, and is being linked from the PR description instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 66d01c4 commit bb262dc

3 files changed

Lines changed: 26 additions & 21 deletions

File tree

backend/src/main/java/ca/bc/gov/nrs/ilcr/schedule10/InvalidTflNumberException.java

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,16 @@
88
* missing-leading-zero aliases have been applied. Maps to 400 with legacy's verbatim validator
99
* text.
1010
*
11-
* <p>Because the accept set and the Road-Group-derivable set are the same 22 keys, a TFL that
12-
* passes this check always derives a Road Group — so the "unmapped TFL saves with a blank Road
13-
* Group" state is unreachable through a write, in this application and in legacy alike. It exists
14-
* only in stored data that predates or bypassed the screen.
11+
* <p>The accept set and the Road-Group-derivable set are the SAME set, because a single table
12+
* answers both questions — {@code RoadGroup10Lookup.rg10ByTflNumberCode}. So a TFL that passes this
13+
* check always derives a Road Group, and the "unmapped TFL saves with a blank Road Group" state is
14+
* unreachable through a write. It exists only in stored data that predates or bypassed the screen.
15+
*
16+
* <p>An earlier version of this note put a number on that set — "the same 22 keys" — which was
17+
* wrong and then drifted again when {@code "52B"} was demoted. One table answering both questions
18+
* is the fact this class rests on, and it holds whatever the table's contents are; see {@link
19+
* RoadGroup10Lookup#canonicalTfl} for why the count is deliberately not quoted (code review
20+
* 2026-08-19).
1521
*/
1622
public class InvalidTflNumberException extends BusinessException {
1723

backend/src/main/java/ca/bc/gov/nrs/ilcr/schedule10/RoadGroup10Lookup.java

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -95,21 +95,20 @@ static String rmgFor(String tsaNumber, String tsbNumberCode, String tflNumberCod
9595
*
9696
* <p><strong>On which table validates.</strong> The legacy validator calls Schedule <em>6</em>'s
9797
* lookup even for this screen, which reads like a defect. Validating here against Schedule 10's
98-
* own table is behaviourally equivalent, but NOT because the tables are identical — an earlier
99-
* version of this note claimed "an identical set of 22 keys", and that was wrong on both counts
100-
* (corrected at code review 2026-08-18). The accurate position, from counting the {@code case}
101-
* labels of both methods:
98+
* own table is behaviourally equivalent, because <strong>the two accept and reject exactly the
99+
* same values</strong>: they agree on every key either one holds, and the only code they ever
100+
* disagreed on was {@code "52B"}, which neither offers now.
102101
*
103-
* <ul>
104-
* <li>this table holds <strong>21</strong> live keys; {@code schedule6.RoadGroupLookup} holds
105-
* <strong>20</strong>;
106-
* <li>the sets are identical on all 20 shared keys and differ only by {@code "52B"}, which
107-
* Schedule 6 demoted to a comment because {@code TFL_NUMBER_CODE} is {@code VARCHAR2(2)} —
108-
* a 3-character TFL is unstorable, and {@code ConstructionPageRequest.tflNumberCode}
109-
* carries {@code @Size(max = 2)} to match;
110-
* <li>so for every input that can physically reach either method, the two accept and reject
111-
* exactly the same values, and validating against this table matches legacy.
112-
* </ul>
102+
* <p>{@code "52B"} is worth a sentence because it is the one real difference in the two tables'
103+
* history. Legacy kept it live here and Schedule 6 demoted it to a comment, on the grounds that
104+
* {@code TFL_NUMBER_CODE} is {@code VARCHAR2(2)} on both sides and {@code
105+
* ConstructionPageRequest.tflNumberCode} carries {@code @Size(max = 2)} — so a 3-character TFL
106+
* can never reach either switch. This schedule followed at code review 2026-08-18.
107+
*
108+
* <p>Deliberately stated as a PROPERTY rather than a key count. Two earlier revisions of this
109+
* note cited a number — first "an identical set of 22 keys", then "21 versus 20" — and both went
110+
* stale, the second within the same change that demoted {@code 52B} (flagged at review
111+
* 2026-08-19). The property is what the write path depends on; the count is trivia that rots.
113112
*
114113
* <p>Only the returned Road Group values differ between the tables, which is what this class
115114
* exists to keep separate. The cross-wiring is still worth reporting upstream: the tables are

backend/src/main/java/ca/bc/gov/nrs/ilcr/schedule10/Schedule10DocumentAssembler.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -145,14 +145,14 @@ Schedule10Response assemble(long millId, int year, String trackStatus, boolean e
145145
costsByDetail.computeIfAbsent(row.roadDetailId(), key -> new LinkedHashMap<>());
146146
// containsKey, not get() != null — a present-but-null entry is a real cost row that must be
147147
// recognised as a duplicate when a second row for the same item arrives.
148+
// containsKey, not get() != null — a present-but-null entry is a real cost row, and a second
149+
// row for the same item must still be recognised as a duplicate.
148150
if (byItem.containsKey(row.costItemId())) {
149151
LOG.warn("Schedule 10 road detail {} has MORE THAN ONE cost row for item {} — the last row"
150152
+ " wins, as in legacy (mill {}, year {})",
151153
row.roadDetailId(), row.costItemId(), millId, year);
152-
byItem.put(row.costItemId(), row.cost());
153-
} else {
154-
byItem.put(row.costItemId(), row.cost());
155154
}
155+
byItem.put(row.costItemId(), row.cost());
156156
}
157157

158158
// Two distinct sets: what the dropdown may OFFER (xref-gated) and what this document must be

0 commit comments

Comments
 (0)