Skip to content

Commit 025d483

Browse files
Rylan-cgiclaude
andcommitted
fix(schedule10): apply Story 11.2 code-review findings, resolved legacy-first
Four independent review layers over the Schedule 10 write path produced 49 findings. All patches applied; all eight decisions answered by reading docs/nr-ilcr-2.0.4 rather than by preference. TWO USER-FACING MESSAGE DEFECTS affected every rejection this story added. Required fields answered a literal "{0}: Value is required." and all 28 range constraints answered "Entered value must be between {0} and {1}", because parameterised bundle keys were wired to Bean Validation annotations, which supply no positional arguments. Legacy overrides no javax.faces.* key, so JSF fills {0} from each component's label attribute; invalidRangeErrorMsg appears in legacy in exactly one place, CheckStatus.java:196, with an args array. Both now render legacy's resolved bytes. Percentages and side slope use legacy's OWN existing keys rather than new ones. TWO PATHS WROTE NULL INTO NOT NULL COLUMNS and surfaced as opaque 500s. detailedEngineeringCostInd (@pattern treats null as valid) now defaults to "N", matching legacy's two-item dropdown with no empty option. applyBallastCoupling's missing default branch now forces material "NA", as legacy already does for its other non-crushed methods. DUPLICATE COST ROWS now take the LAST row instead of being summed. Schedule10DAO:556-600 loops the cost rows and ASSIGNS per item -- legacy never sums. Story 11.1's summing was an unrecorded deviation, and it was exactly what made a duplicated value unfixable through the API: the write path's UPDATE sets every duplicate row, and the read then re-summed them. Correcting the read needs no row deletion, so AC5 ("a cost row is never deleted by a save") still holds. ROAD-DETAIL EDITS no longer rewrite the derived moisture pair. Legacy's filterMoistureCodeLists() rebuilds the dropdown LISTS and never assigns, so an edit touching only comments must not flip a stored (F, Moist) to (SD, Moist) through a tie-break invented for this port. This also gives the BEC classification the unchanged-code exemption, so a stored row whose BEC has dropped out of the offerable set is editable again -- and findStoredClassification, previously dead code, is now its implementation. Also: byte caps on roadName and divisionName, a constructionPeriod month range, converter keys so a malformed percentage or volume no longer reports a COST error, the four dead persistence keys wired, and mill/year/category scope added to all five child UPDATE/DELETE statements so the invariant the file already asserts is actually true. THREE DECISIONS NEEDED NO CODE CHANGE -- an unmapped TFL blocking an edit, HALF_UP rounding, and token-free deletes are all faithful legacy behaviour. A fourth proved the STORY wrong rather than the code: legacy zeroes the four stabilizing dimensions for method N only, keeps them for D, and never zeroes ttTransfer. Pinned deviation (e) is amended to match legacy; deviation (l)'s TFL key counts are corrected to 21 vs 20, differing only by the unstorable 52B, which is now demoted to a comment as Schedule 6 had already done. TEST CORRECTIONS, several of them tests that could not fail: a vacuous intValue() assertion on a 0.3 input, three of the four "zeroed" dimensions never asserted, isNotNull() on NOT NULL columns, and a Set-collapse that let containsExactly pass with a constraint deleted. Added the missing coverage: Check Status emission order, both unreachable-rule labels asserted positively, ballast method D, three requireOffered negatives, the road-detail 409 path, year/category/foreign-mill delete scoping, UPDATE_* re-stamping across all three tables, and AC8's three untested road-detail endpoints. Schedule10CopyIT and Schedule10DeleteIT no longer share a (mill, year) -- they passed only because "Copy" sorts before "Delete". ONE CLAIM WAS REMOVED RATHER THAN SATISFIED: Schedule10WriteAuthorizationIT asserted that a controller unit test pins the EDIT action by name. No such test exists, and because both shipped groups hold both actions, changing mayEdit() to ask for VIEW_SCHEDULE would pass the entire suite. Recorded in deferred-work.md instead of left as a claim the tests do not support. Verification: 1150 unit + 879 integration tests, 0 failures, BUILD SUCCESS; 0 checkstyle violations in schedule10 (1234 pre-existing elsewhere untouched). Schedule 10 owns 259 tests across 17 classes, up from 243 before the review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent ff6f0dc commit 025d483

24 files changed

Lines changed: 1243 additions & 177 deletions

backend/src/main/java/ca/bc/gov/nrs/ilcr/exception/GlobalExceptionHandler.java

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -320,10 +320,21 @@ private static String converterKeyForField(String causeMessage) {
320320
* collection
321321
* hops ({@code CulvertSaveAllRequest["culverts"]->…->CulvertRequest["spanSize"]}).
322322
*/
323-
private static final Map<String, String> CONVERTER_KEYS_BY_TARGET = Map.of(
324-
"CulvertRequest[\"spanSize\"]", "culvertSpanConverterErrorMsg",
325-
"CulvertRequest[\"riseSize\"]", "culvertRiseConverterErrorMsg",
326-
"CulvertRequest[\"culvertPieceCount\"]", "culvertPieceCountConverterErrorMsg");
323+
private static final Map<String, String> CONVERTER_KEYS_BY_TARGET = Map.ofEntries(
324+
Map.entry("CulvertRequest[\"spanSize\"]", "culvertSpanConverterErrorMsg"),
325+
Map.entry("CulvertRequest[\"riseSize\"]", "culvertRiseConverterErrorMsg"),
326+
Map.entry("CulvertRequest[\"culvertPieceCount\"]", "culvertPieceCountConverterErrorMsg"),
327+
// Schedule 10 (code review 2026-08-18). Without these, every malformed Integer on a road
328+
// detail — a percentage or a haul volume — fell through to the type default and told the
329+
// reporter their COST was invalid.
330+
Map.entry("RoadDetailRequest[\"sideSlopePct\"]", "sideSlopePercentageConverterErrorMsg"),
331+
Map.entry("RoadDetailRequest[\"endHaulVolume\"]", "volumeConverterErrorMsg"),
332+
Map.entry("RoadDetailRequest[\"overlandVolume\"]", "volumeConverterErrorMsg"),
333+
Map.entry("MaterialCompositionRequest[\"solidRockPct\"]", "percentageConverterErrorMsg"),
334+
Map.entry("MaterialCompositionRequest[\"rippableRockPct\"]", "percentageConverterErrorMsg"),
335+
Map.entry("MaterialCompositionRequest[\"coarsePct\"]", "percentageConverterErrorMsg"),
336+
Map.entry("MaterialCompositionRequest[\"finePct\"]", "percentageConverterErrorMsg"),
337+
Map.entry("MaterialCompositionRequest[\"organicPct\"]", "percentageConverterErrorMsg"));
327338

328339
/**
329340
* Handles authorization denials from method security ({@code @PreAuthorize}). Without this
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
package ca.bc.gov.nrs.ilcr.schedule10;
2+
3+
import ca.bc.gov.nrs.ilcr.exception.BusinessException;
4+
import org.springframework.http.HttpStatus;
5+
6+
/**
7+
* Raised when the additional-stabilizing material type is missing while the ballast method requires
8+
* it.
9+
*
10+
* <p>Legacy makes this field conditionally required — {@code pageDtlASType} carries {@code
11+
* required="#{...typeMandatory}"}, and {@code typeMandatory} is true only for ballast method {@code
12+
* "C"} ({@code RoadConstructionReportDetailType:1129}). Because the requiredness depends on a
13+
* sibling field, Bean Validation on a single record component cannot express it, so the check lives
14+
* in the service and arrives here.
15+
*
16+
* <p>Maps to 400 {@code materialCodeTypeRequiredErrorMsg} — the resolved form of legacy's JSF
17+
* required template with {@code {0}} filled from that component's {@code label="Material Code
18+
* Type"}. It replaces {@code invalidCodeValueErrorMsg} ({@code A valid value must be selected from
19+
* the list.}), used here until code review 2026-08-18: a pick-from-the-list message for a
20+
* missing-required condition.
21+
*/
22+
public class MaterialCodeTypeRequiredException extends BusinessException {
23+
24+
public MaterialCodeTypeRequiredException() {
25+
super(HttpStatus.BAD_REQUEST, "materialCodeTypeRequiredErrorMsg");
26+
}
27+
}

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

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -94,12 +94,27 @@ static String rmgFor(String tsaNumber, String tsbNumberCode, String tflNumberCod
9494
* hold; this returns the canonical form so the stored value is the one that resolves.
9595
*
9696
* <p><strong>On which table validates.</strong> The legacy validator calls Schedule <em>6</em>'s
97-
* lookup even for this screen. That reads like a defect, but the two tables accept an identical
98-
* set of 22 keys — verified by diffing the {@code case} labels of both methods — so validating
99-
* here against Schedule 10's own table accepts and rejects exactly what legacy does. Only the
100-
* returned Road Group values differ between the tables, which is what this class exists to keep
101-
* separate. The cross-wiring is still worth reporting upstream: the tables are maintained
102-
* independently, so a future edit to either would silently split validation from derivation.
97+
* 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:
102+
*
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>
113+
*
114+
* <p>Only the returned Road Group values differ between the tables, which is what this class
115+
* exists to keep separate. The cross-wiring is still worth reporting upstream: the tables are
116+
* maintained independently, so a future edit to either would silently split validation from
117+
* derivation.
103118
*
104119
* @param tflNumberCode the entered TFL number, possibly missing a leading zero
105120
* @return the canonical TFL to store, or {@code null} when the value is not a valid TFL
@@ -354,7 +369,12 @@ private static String rg10ByTflNumberCode(String tflNumberCode) {
354369
case "03", "23", "33", "55", "56":
355370
roadGroup = "11";
356371
break;
357-
case "05", "52B":
372+
// "52B" is legacy-live but unreachable, and is demoted to a comment here for the reason
373+
// schedule6.RoadGroupLookup already records: TFL_NUMBER_CODE is VARCHAR2(2) on both sides,
374+
// and ConstructionPageRequest.tflNumberCode carries @Size(max = 2), so a 3-character TFL
375+
// never reaches this switch on read or on save. As a live case it read as an accepted value
376+
// that the request contract rejects (code review 2026-08-18). case "52B": roadGroup = "5";
377+
case "05":
358378
roadGroup = "5";
359379
break;
360380
case "30", "52", "53":
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
package ca.bc.gov.nrs.ilcr.schedule10;
2+
3+
import ca.bc.gov.nrs.ilcr.exception.BusinessException;
4+
import org.springframework.http.HttpStatus;
5+
6+
/**
7+
* Raised when a Schedule 10 write fails at the persistence layer, naming the resource that failed.
8+
*
9+
* <p>Maps to 500, exactly as {@code ScheduleNotSavedException} does, but carries one of the four
10+
* resource-specific legacy keys instead of the generic {@code scheduleNotSavedErrorMsg}: §
11+
* Validation rules directs the write path to use them "where the failing resource is known", which
12+
* is what resolves the UC's otherwise-unattached ERR-004. Those four keys were declared but
13+
* unreferenced until code review 2026-08-18, so every failure — page insert, detail insert, cost
14+
* upsert, cascade delete — flattened into one message the reporter could not act on.
15+
*
16+
* <p>The {@code @Transactional} write boundary rolls back before this surfaces.
17+
*/
18+
public class Schedule10PersistenceException extends BusinessException {
19+
20+
/** The page could not be saved. */
21+
public static final String PAGE_NOT_SAVED = "roadConstructionReportNotSavedErrorMsg";
22+
23+
/** The road detail could not be saved. */
24+
public static final String DETAIL_NOT_SAVED = "roadConstructionReportDetailNotSavedErrorMsg";
25+
26+
/** The page could not be deleted. */
27+
public static final String PAGE_NOT_DELETED = "roadConstructionReportNotDeletedErrorMsg";
28+
29+
/** The road detail could not be deleted. */
30+
public static final String DETAIL_NOT_DELETED = "roadConstructionReportDetailNotDeletedErrorMsg";
31+
32+
/**
33+
* Wraps a persistence failure against a named resource.
34+
*
35+
* @param messageKey one of the four constants on this class
36+
*/
37+
public Schedule10PersistenceException(String messageKey) {
38+
super(HttpStatus.INTERNAL_SERVER_ERROR, messageKey);
39+
}
40+
}

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

Lines changed: 68 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -655,9 +655,12 @@ int countRoadDetail(
655655
* @param costItemId the legacy cost-item ordinal
656656
* @param cost the amount, or {@code null} to clear it in place
657657
* @param user the actor stamped into the audit columns
658+
* @param millId the mill the owning page must belong to
659+
* @param year the reporting year the owning page must belong to
658660
*/
659-
default void upsertCostLine(int roadDetailId, int costItemId, Integer cost, String user) {
660-
if (updateCostLine(roadDetailId, costItemId, cost, user) == 0) {
661+
default void upsertCostLine(
662+
int roadDetailId, int costItemId, Integer cost, String user, long millId, int year) {
663+
if (updateCostLine(roadDetailId, costItemId, cost, user, millId, year) == 0) {
661664
insertCostLine(nextCostDetailId(), roadDetailId, costItemId, cost, user);
662665
}
663666
}
@@ -673,10 +676,19 @@ default void upsertCostLine(int roadDetailId, int costItemId, Integer cost, Stri
673676
UPDATE_TIMESTAMP = SYSTIMESTAMP
674677
WHERE ROAD_CONSTRUCTION_REPRT_DTL_ID = :roadDetailId
675678
AND ILCR_REPORT_COST_ITEM_ID = :costItemId
679+
AND EXISTS (SELECT 1
680+
FROM THE.ROAD_CONSTRUCTION_REPRT_DTL d
681+
JOIN THE.ROAD_CONSTRUCTION_REPRT r
682+
ON r.ROAD_CONSTRUCTION_REPRT_ID = d.ROAD_CONSTRUCTION_REPRT_ID
683+
WHERE d.ROAD_CONSTRUCTION_REPRT_DTL_ID = :roadDetailId
684+
AND r.ILCR_MILL_ID = :millId
685+
AND r.REPORT_YEAR = :year
686+
AND r.ILCR_CATEGORY_ID = '10')
676687
""")
677688
int updateCostLine(
678689
@Param("roadDetailId") int roadDetailId, @Param("costItemId") int costItemId,
679-
@Param("cost") Integer cost, @Param("user") String user);
690+
@Param("cost") Integer cost, @Param("user") String user, @Param("millId") long millId,
691+
@Param("year") int year);
680692

681693
/**
682694
* Insert half of {@link #upsertCostLine}. Schedule 10 cost rows carry a NULL {@code
@@ -708,8 +720,18 @@ void insertCostLine(
708720
@Query("""
709721
DELETE FROM THE.ILCR_COST_REPORT_DETAIL
710722
WHERE ROAD_CONSTRUCTION_REPRT_DTL_ID = :roadDetailId
723+
AND EXISTS (SELECT 1
724+
FROM THE.ROAD_CONSTRUCTION_REPRT_DTL d
725+
JOIN THE.ROAD_CONSTRUCTION_REPRT r
726+
ON r.ROAD_CONSTRUCTION_REPRT_ID = d.ROAD_CONSTRUCTION_REPRT_ID
727+
WHERE d.ROAD_CONSTRUCTION_REPRT_DTL_ID = :roadDetailId
728+
AND r.ILCR_MILL_ID = :millId
729+
AND r.REPORT_YEAR = :year
730+
AND r.ILCR_CATEGORY_ID = '10')
711731
""")
712-
int deleteCostsForRoadDetail(@Param("roadDetailId") int roadDetailId);
732+
int deleteCostsForRoadDetail(
733+
@Param("roadDetailId") int roadDetailId, @Param("millId") long millId,
734+
@Param("year") int year);
713735

714736
/**
715737
* Delete every cost line belonging to any road detail of one page — the first step of the page
@@ -721,26 +743,47 @@ void insertCostLine(
721743
WHERE ROAD_CONSTRUCTION_REPRT_DTL_ID IN (
722744
SELECT d.ROAD_CONSTRUCTION_REPRT_DTL_ID
723745
FROM THE.ROAD_CONSTRUCTION_REPRT_DTL d
724-
WHERE d.ROAD_CONSTRUCTION_REPRT_ID = :pageId)
746+
JOIN THE.ROAD_CONSTRUCTION_REPRT r
747+
ON r.ROAD_CONSTRUCTION_REPRT_ID = d.ROAD_CONSTRUCTION_REPRT_ID
748+
WHERE d.ROAD_CONSTRUCTION_REPRT_ID = :pageId
749+
AND r.ILCR_MILL_ID = :millId
750+
AND r.REPORT_YEAR = :year
751+
AND r.ILCR_CATEGORY_ID = '10')
725752
""")
726-
int deleteCostsForPage(@Param("pageId") int pageId);
753+
int deleteCostsForPage(
754+
@Param("pageId") int pageId, @Param("millId") long millId, @Param("year") int year);
727755

728756
/** Delete one road detail, scoped to its parent page. Its cost lines must already be gone. */
729757
@Modifying
730758
@Query("""
731759
DELETE FROM THE.ROAD_CONSTRUCTION_REPRT_DTL
732760
WHERE ROAD_CONSTRUCTION_REPRT_DTL_ID = :roadDetailId
733761
AND ROAD_CONSTRUCTION_REPRT_ID = :pageId
762+
AND EXISTS (SELECT 1
763+
FROM THE.ROAD_CONSTRUCTION_REPRT r
764+
WHERE r.ROAD_CONSTRUCTION_REPRT_ID = :pageId
765+
AND r.ILCR_MILL_ID = :millId
766+
AND r.REPORT_YEAR = :year
767+
AND r.ILCR_CATEGORY_ID = '10')
734768
""")
735-
int deleteRoadDetail(@Param("roadDetailId") int roadDetailId, @Param("pageId") int pageId);
769+
int deleteRoadDetail(
770+
@Param("roadDetailId") int roadDetailId, @Param("pageId") int pageId,
771+
@Param("millId") long millId, @Param("year") int year);
736772

737773
/** Delete every road detail of one page — the second step of the page cascade. */
738774
@Modifying
739775
@Query("""
740776
DELETE FROM THE.ROAD_CONSTRUCTION_REPRT_DTL
741777
WHERE ROAD_CONSTRUCTION_REPRT_ID = :pageId
778+
AND EXISTS (SELECT 1
779+
FROM THE.ROAD_CONSTRUCTION_REPRT r
780+
WHERE r.ROAD_CONSTRUCTION_REPRT_ID = :pageId
781+
AND r.ILCR_MILL_ID = :millId
782+
AND r.REPORT_YEAR = :year
783+
AND r.ILCR_CATEGORY_ID = '10')
742784
""")
743-
int deleteRoadDetailsForPage(@Param("pageId") int pageId);
785+
int deleteRoadDetailsForPage(
786+
@Param("pageId") int pageId, @Param("millId") long millId, @Param("year") int year);
744787

745788
/**
746789
* Delete one page, scoped to mill/year/category. Runs LAST in the cascade, after its cost lines
@@ -768,20 +811,31 @@ int deletePage(
768811
d.ILCR_ROAD_BALLAST_METHOD_CODE AS ballast_method_code,
769812
d.ILCR_ROAD_BALLAST_MATERL_CODE AS ballast_material_code,
770813
d.REL_SOIL_MOIST_RGM_CLS_CODE AS rsmr_class_code,
771-
d.BECBIOGEO_CATALOGUE_ID AS bec_id
814+
d.BECBIOGEO_CATALOGUE_ID AS bec_id,
815+
d.RELATIVE_SOIL_MOISTUR_RGM_CODE AS asm_code,
816+
d.ILCR_SOIL_MOISTURE_CODE AS soil_moisture_code
772817
FROM THE.ROAD_CONSTRUCTION_REPRT_DTL d
773818
WHERE d.ROAD_CONSTRUCTION_REPRT_DTL_ID = :roadDetailId
774819
""")
775820
Optional<StoredClassification> findStoredClassification(@Param("roadDetailId") int roadDetailId);
776821

777822
/**
778-
* The classification codes a road detail already carries.
779-
*
780-
* <p>Used only for the exemption that lets an unchanged code survive its own expiry: a code that
781-
* has since been retired must not permanently block re-saving a row that already holds it.
823+
* The classification codes a road detail already carries, plus the moisture pair derived from
824+
* them.
825+
*
826+
* <p>Read on every edit so an UNCHANGED classification keeps the moisture pair already stored.
827+
* Legacy's {@code filterMoistureCodeLists()} ({@code Schedule10MB:665-689}) rebuilds only the two
828+
* dropdown LISTS — it never assigns to the detail — so the stored ASM and soil-moisture codes
829+
* change in legacy only when the user picks new ones. Re-deriving unconditionally would rewrite
830+
* two NOT NULL columns the legacy print reports consume during an edit that touched neither input
831+
* (code review 2026-08-18).
832+
*
833+
* <p>This also delivers the unchanged-code exemption for the BEC classification: a stored id that
834+
* has since dropped out of the offerable xref-gated set would otherwise make its road detail
835+
* permanently unsaveable, because the derivation rejects a zero-candidate pair.
782836
*/
783837
record StoredClassification(
784838
String roadLifetimeCode, String ballastMethodCode, String ballastMaterialCode,
785-
String rsmrClassCode, Integer becId) {
839+
String rsmrClassCode, Integer becId, String asmCode, String soilMoistureCode) {
786840
}
787841
}

0 commit comments

Comments
 (0)