fix(schedule10): resolve SonarCloud findings on PR #305

All three gate failures fixed, plus the warnings worth acting on.

TRANSACTIONAL SELF-INVOCATION (6 failures). Each of the seven write methods and
checkStatus called getSchedule10 via this, and a this-call never passes through
the Spring proxy -- so that method's @transactional was silently ignored and its
reads simply joined the caller's transaction. The behaviour was correct; the
annotation was not what made it so. The assembly body is extracted into a
private, un-annotated assembleDocument(...) that every entry point calls, so the
transaction is opened by the entry point and nothing self-invokes. The class
javadoc previously admitted this subtlety in prose; the code now states it.

DUPLICATED LITERALS (2 failures). "percentageConverterErrorMsg" appeared five
times across the new Schedule 10 converter entries and is now the constant
PERCENTAGE_CONVERTER. "999.9" appeared three times as the upper bound of the
one-decimal width and distance rules and is now WIDTH_MAX.

MONSTER CLASS (warning, 21 dependencies against a limit of 20). The two exception
types added by the code review pushed the count over. Rather than suppress it,
the single-argument persist(Runnable) overload turned out to be dead: once every
one of the seven call sites carried a resource-specific message key, nothing
reached the generic form. Removing it deletes dead code and drops the
ScheduleNotSavedException dependency back to 20.

COMMENTED-OUT CODE (warning). This was the deliberate 52B demotion from the code
review -- a commented-out case label kept as a record. Reworded as prose so the
reasoning survives without a commented-out statement.

WEAK ASSERTION (warning). assertThat(keysOf(issues)).contains(
"missingRequiredFieldMsg") was satisfied by any of the eight issues in that
outcome, so it never bound the key to the Material Code Type field. The code
review had flagged the same line independently. It now asserts the key on
ballastMaterialCode specifically and pins the exact ordered set with
containsExactly. The keysOf helper became unused and is removed.

ASSERTION STYLE (warnings). 24 assertThat(map.get(k)).isEqualTo(v) forms rewritten
as assertThat(map).containsEntry(k, v). Multi-line forms and those carrying an
.as(...) description were left alone deliberately.

Also rebased onto main, which had moved twice: the second move bumped checkstyle
from 13.9.0 to 13.10.0 and touched backend/. That exposed a hole in the local
verification -- the checkstyle gate had been run with -o (offline), so after the
version bump it failed to resolve the plugin and never actually ran. Re-run
online against 13.10.0: 0 violations in schedule10 (1235 pre-existing elsewhere,
up from 1234 through main's own new code).

Verification: 1150 unit + 879 integration tests, 0 failures.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: Rylan-cgi

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

@@ -320,6 +320,9 @@ private static String converterKeyForField(String causeMessage) {
    * collection
    * hops ({@code CulvertSaveAllRequest["culverts"]->…->CulvertRequest["spanSize"]}).
    */
+  /** Shared by all five Schedule 10 material percentages, which fail identically. */
+  private static final String PERCENTAGE_CONVERTER = "percentageConverterErrorMsg";
+
   private static final Map<String, String> CONVERTER_KEYS_BY_TARGET = Map.ofEntries(
       Map.entry("CulvertRequest[\"spanSize\"]", "culvertSpanConverterErrorMsg"),
       Map.entry("CulvertRequest[\"riseSize\"]", "culvertRiseConverterErrorMsg"),
@@ -330,11 +333,11 @@ private static String converterKeyForField(String causeMessage) {
       Map.entry("RoadDetailRequest[\"sideSlopePct\"]", "sideSlopePercentageConverterErrorMsg"),
       Map.entry("RoadDetailRequest[\"endHaulVolume\"]", "volumeConverterErrorMsg"),
       Map.entry("RoadDetailRequest[\"overlandVolume\"]", "volumeConverterErrorMsg"),
-      Map.entry("MaterialCompositionRequest[\"solidRockPct\"]", "percentageConverterErrorMsg"),
-      Map.entry("MaterialCompositionRequest[\"rippableRockPct\"]", "percentageConverterErrorMsg"),
-      Map.entry("MaterialCompositionRequest[\"coarsePct\"]", "percentageConverterErrorMsg"),
-      Map.entry("MaterialCompositionRequest[\"finePct\"]", "percentageConverterErrorMsg"),
-      Map.entry("MaterialCompositionRequest[\"organicPct\"]", "percentageConverterErrorMsg"));
+      Map.entry("MaterialCompositionRequest[\"solidRockPct\"]", PERCENTAGE_CONVERTER),
+      Map.entry("MaterialCompositionRequest[\"rippableRockPct\"]", PERCENTAGE_CONVERTER),
+      Map.entry("MaterialCompositionRequest[\"coarsePct\"]", PERCENTAGE_CONVERTER),
+      Map.entry("MaterialCompositionRequest[\"finePct\"]", PERCENTAGE_CONVERTER),
+      Map.entry("MaterialCompositionRequest[\"organicPct\"]", PERCENTAGE_CONVERTER));
 
   /**
    * Handles authorization denials from method security ({@code @PreAuthorize}). Without this

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

@@ -369,11 +369,11 @@ private static String rg10ByTflNumberCode(String tflNumberCode) {
       case "03", "23", "33", "55", "56":
         roadGroup = "11";
         break;
-      // "52B" is legacy-live but unreachable, and is demoted to a comment here for the reason
-      // schedule6.RoadGroupLookup already records: TFL_NUMBER_CODE is VARCHAR2(2) on both sides,
-      // and ConstructionPageRequest.tflNumberCode carries @Size(max = 2), so a 3-character TFL
-      // never reaches this switch on read or on save. As a live case it read as an accepted value
-      // that the request contract rejects (code review 2026-08-18). case "52B": roadGroup = "5";
+      // Legacy maps the three-character TFL "52B" to this same road group. It is deliberately NOT
+      // reproduced: TFL_NUMBER_CODE is VARCHAR2(2) on both sides and the request constrains the
+      // field to two characters, so such a value cannot reach this switch on read or on save.
+      // Including it would read as an accepted value the request contract rejects.
+      // schedule6.RoadGroupLookup dropped it earlier for the same reason (code review 2026-08-18).
       case "05":
         roadGroup = "5";
         break;

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

@@ -91,6 +91,8 @@ final class Schedule10CheckStatus {
   private static final String FMT_MONEY_NARROW = "###,###";
 
   private static final BigDecimal PCT_MAX = new BigDecimal("100");
+  /** Shared upper bound for the three one-decimal width/distance rules. */
+  private static final BigDecimal WIDTH_MAX = new BigDecimal("999.9");
   private static final BigDecimal SEVEN_DIGITS = new BigDecimal("9999999");
   private static final BigDecimal EIGHT_DIGITS = new BigDecimal("99999999");
   private static final BigDecimal ZERO = BigDecimal.ZERO;
@@ -216,7 +218,7 @@ static DetailOutcome evaluateRoadDetail(
     requireRange(issues, "subGradeLength", prefix + " Sub-Grade: Length (km)",
         field(subGrade, SubGrade::length), ZERO, FMT_3DP, PCT_MAX, false);
     requireRange(issues, "subGradeSurfaceWidth", prefix + " Sub-Grade: Surface Width (m)",
-        field(subGrade, SubGrade::surfaceWidth), ZERO, FMT_1DP, new BigDecimal("999.9"), false);
+        field(subGrade, SubGrade::surfaceWidth), ZERO, FMT_1DP, WIDTH_MAX, false);
     requireRange(issues, "subGradeActualCost", prefix + " Sub-Grade: Actual Cost ($)",
         field(subGrade, SubGrade::actualCost), ZERO, FMT_MONEY, SEVEN_DIGITS, false);
     requireRange(issues, "subGradeTtTransfer", prefix + " Sub-Grade: TtT Transfer ($)",
@@ -255,14 +257,14 @@ static DetailOutcome evaluateRoadDetail(
         field(stabilizing, Stabilizing::length), ZERO, FMT_3DP, new BigDecimal("999.999"), crushed);
     requireRange(issues, "stabilizingSurfaceWidth",
         prefix + " Additional Stabilizing: Surface Width (m)",
-        field(stabilizing, Stabilizing::surfaceWidth), ZERO, FMT_1DP, new BigDecimal("999.9"),
+        field(stabilizing, Stabilizing::surfaceWidth), ZERO, FMT_1DP, WIDTH_MAX,
         crushed);
     requireRange(issues, "stabilizingDepth", prefix + " Additional Stabilizing: Depth (m)",
         field(stabilizing, Stabilizing::depth), ZERO, FMT_2DP_SMALL, new BigDecimal("99.9"),
         crushed);
     requireRange(issues, "stabilizingDistanceToSource",
         prefix + " Additional Stabilizing: Distance to Source (km)",
-        field(stabilizing, Stabilizing::distanceToSource), ZERO, FMT_1DP, new BigDecimal("999.9"),
+        field(stabilizing, Stabilizing::distanceToSource), ZERO, FMT_1DP, WIDTH_MAX,
         crushed);
 
     if (!crushed) {

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

@@ -7,7 +7,6 @@
 
 import ca.bc.gov.nrs.ilcr.dto.base.CodeDescriptionDto;
 import ca.bc.gov.nrs.ilcr.schedule1.ScheduleNotEditableException;
-import ca.bc.gov.nrs.ilcr.schedule1.ScheduleNotSavedException;
 import ca.bc.gov.nrs.ilcr.schedule1.StaleRevisionException;
 import ca.bc.gov.nrs.ilcr.schedule10.Schedule10Repository.BecClassificationRow;
 import ca.bc.gov.nrs.ilcr.schedule10.Schedule10Repository.CodeRow;
@@ -50,14 +49,17 @@
  * lookup, two BEC queries and five code-list queries; the three-query property is about the nested
  * body, not the request as a whole.
  *
- * <p>{@link #getSchedule10} runs in one read-only transaction so those queries observe a single
- * consistent snapshot. Without it a concurrent write between the page and detail reads yields a
- * silently torn document — details belonging to a page that is not in the result are dropped with
- * no error. Note the scope of that guarantee: the seven write methods call {@code getSchedule10} to
- * build their response, and Spring's default {@code REQUIRED} propagation makes it JOIN the
- * caller's read-write transaction, so {@code readOnly} does not apply on those paths. They still
- * get a single consistent snapshot — their own transaction — but calling the read read-only there
- * would be wrong to claim (corrected at code review 2026-08-18).
+ * <p>Assembly always runs inside a transaction so those queries observe a single consistent
+ * snapshot. Without one, a concurrent write between the page and detail reads yields a silently
+ * torn document — details belonging to a page not in the result are dropped with no error.
+ *
+ * <p>The transaction is opened by the ENTRY POINT, not by the assembly: {@link #getSchedule10} and
+ * {@link #checkStatus} open a read-only one, and each write method opens a read-write one, then all
+ * of them call the un-annotated {@code assembleDocument}. An earlier revision had the write methods
+ * call {@code getSchedule10} directly, which never passes through the Spring proxy — so its
+ * {@code @Transactional} was silently ignored and the reads simply joined the caller's transaction.
+ * The behaviour was correct; the annotation just was not what made it so. Restructured at code
+ * review follow-up 2026-08-18 so the code states it rather than depending on a proxy subtlety.
  *
  * <p>Derivation rules that matter:
  * <ul>
@@ -140,6 +142,25 @@ public Schedule10Service(Schedule10Repository repository) {
    */
   @Transactional(readOnly = true)
   public Schedule10Response getSchedule10(long millId, int year, boolean callerMayEdit) {
+    return assembleDocument(millId, year, callerMayEdit);
+  }
+
+  /**
+   * Assembles the document with NO transaction annotation of its own.
+   *
+   * <p>This exists so the seven write methods and {@link #checkStatus} can build their response
+   * without self-invoking {@link #getSchedule10}. A {@code this.}-call never passes through the
+   * Spring proxy, so the inner {@code @Transactional} was silently ignored — the reads simply
+   * joined the caller's transaction, which is correct behaviour but not what the annotation
+   * appeared to promise. Extracting the body states that directly rather than relying on a
+   * proxy subtlety, and
+   * removes the self-invocation Sonar flags (code review follow-up 2026-08-18).
+   *
+   * <p>Every caller is already inside a transaction: {@code getSchedule10} and {@code checkStatus}
+   * open a read-only one, and each write method opens a read-write one. Nothing calls this
+   * unwrapped, so the single-snapshot guarantee is unchanged.
+   */
+  private Schedule10Response assembleDocument(long millId, int year, boolean callerMayEdit) {
     List<RoadConstructionReportEntity> pageRows = repository.findPages(millId, year);
     List<RoadConstructionReportDetailEntity> detailRows = repository.findRoadDetails(millId, year);
     List<CostLineRow> costRows = repository.findCostLines(millId, year);
@@ -513,7 +534,7 @@ public Schedule10Response addPage(
     int pageId = repository.nextPageId();
     persist(() -> repository.insertPage(
         toPageEntity(pageId, millId, year, request, location), millId, year, user), PAGE_NOT_SAVED);
-    return getSchedule10(millId, year, callerMayEdit);
+    return assembleDocument(millId, year, callerMayEdit);
   }
 
   /**
@@ -549,7 +570,7 @@ public Schedule10Response updatePage(
         throw new StaleRevisionException();
       }
     }, PAGE_NOT_SAVED);
-    return getSchedule10(millId, year, callerMayEdit);
+    return assembleDocument(millId, year, callerMayEdit);
   }
 
   /**
@@ -581,7 +602,7 @@ public Schedule10Response copyPage(
         source.constructionDivisionName(), source.ilcrForestRegionCode(), source.tsbNumberCode(),
         source.tsaNumber(), source.tflNumberCode(), 0);
     persist(() -> repository.insertPage(copy, millId, year, user), PAGE_NOT_SAVED);
-    return getSchedule10(millId, year, callerMayEdit);
+    return assembleDocument(millId, year, callerMayEdit);
   }
 
   /**
@@ -611,7 +632,7 @@ public Schedule10Response deletePage(
         throw new ConstructionPageNotFoundException();
       }
     }, PAGE_NOT_DELETED);
-    return getSchedule10(millId, year, callerMayEdit);
+    return assembleDocument(millId, year, callerMayEdit);
   }
 
   /**
@@ -642,7 +663,7 @@ public Schedule10Response addRoadDetail(
           moisture.asmCode(), user);
       writeCostLines(roadDetailId, coupled, user, millId, year);
     }, DETAIL_NOT_SAVED);
-    return getSchedule10(millId, year, callerMayEdit);
+    return assembleDocument(millId, year, callerMayEdit);
   }
 
   /**
@@ -681,7 +702,7 @@ public Schedule10Response updateRoadDetail(
       }
       writeCostLines(roadDetailId, coupled, user, millId, year);
     }, DETAIL_NOT_SAVED);
-    return getSchedule10(millId, year, callerMayEdit);
+    return assembleDocument(millId, year, callerMayEdit);
   }
 
   /**
@@ -705,7 +726,7 @@ public Schedule10Response deleteRoadDetail(
         throw new RoadDetailNotFoundException();
       }
     }, DETAIL_NOT_DELETED);
-    return getSchedule10(millId, year, callerMayEdit);
+    return assembleDocument(millId, year, callerMayEdit);
   }
 
   /**
@@ -726,7 +747,7 @@ public Schedule10Response deleteRoadDetail(
   public Schedule10CheckStatus.Outcome checkStatus(long millId, int year) {
     // callerMayEdit is irrelevant to the rules, and passing false keeps this read from implying any
     // edit authority in the document it evaluates.
-    return Schedule10CheckStatus.evaluate(getSchedule10(millId, year, false));
+    return Schedule10CheckStatus.evaluate(assembleDocument(millId, year, false));
   }
 
   // -----------------------------------------------------------------------------------------------
@@ -1079,26 +1100,15 @@ private void cost(
   }
 
   /**
-   * Runs a write, translating a data-access failure into the house save error.
+   * Runs a write, translating a data-access failure into the resource-specific legacy save error.
    *
    * <p>Only the exception TYPE is logged. Never the values: a rejected write would otherwise put
    * cost, volume or comment content into the log, which the data-sensitivity rules forbid.
-   */
-  private void persist(Runnable write) {
-    try {
-      write.run();
-    } catch (DataAccessException ex) {
-      LOG.warn("Schedule 10 write failed [{}]", ex.getClass().getSimpleName());
-      throw new ScheduleNotSavedException();
-    }
-  }
-
-  /**
-   * Runs a write, translating a data-access failure into the resource-specific legacy save error.
    *
-   * <p>Same contract as {@link #persist(Runnable)} — type-only logging, never values — but names
-   * the resource that failed, which is what § Validation rules directs the write path to do
-   * wherever the failing resource is known (code review 2026-08-18: the four keys were dead).
+   * <p>Every write names the resource that failed, which is what § Validation rules directs the
+   * write path to do wherever that resource is known (code review 2026-08-18: the four keys were
+   * declared but dead). A generic no-key overload existed alongside this one until all seven call
+   * sites carried a key, at which point it was unreachable and was removed.
    *
    * @param write the write to run
    * @param messageKey one of the {@link Schedule10PersistenceException} constants

backend/src/test/java/ca/bc/gov/nrs/ilcr/schedule10/Schedule10CheckStatusTest.java

@@ -76,10 +76,6 @@ private static ConstructionPage page(List<RoadDetail> details) {
         details.size(), 0, details);
   }
 
-  private static List<String> keysOf(List<Issue> issues) {
-    return issues.stream().map(Issue::messageKey).toList();
-  }
-
   private static Issue issueFor(List<Issue> issues, String field) {
     return issues.stream()
         .filter(issue -> field.equals(issue.field()))
@@ -286,11 +282,18 @@ void crushedRequiresEverything() {
       DetailOutcome outcome =
           Schedule10CheckStatus.evaluateRoadDetail(detail, PAGE_LABEL, ALLOWABLE);
 
-      assertThat(outcome.issues()).extracting(Issue::field).contains(
+      // containsExactly, not contains: the sub-grade and material figures are all clean here, so the
+      // gated stabilizing block is the WHOLE outcome, and its order is contractual.
+      assertThat(outcome.issues()).extracting(Issue::field).containsExactly(
           "stabilizingLength", "stabilizingSurfaceWidth", "stabilizingDepth",
           "stabilizingDistanceToSource", "ballastMaterialCode", "stabilizingActualCost",
           "stabilizingTtTransfer", "stabilizingOtherTransfer");
-      assertThat(keysOf(outcome.issues())).contains("missingRequiredFieldMsg");
+      // The key is bound to the FIELD. The previous form asserted only that
+      // "missingRequiredFieldMsg" appeared somewhere in the outcome, which any of the eight issues
+      // satisfied — so it never pinned the material-type rule to its own message (flagged by both the
+      // code review and Sonar, 2026-08-18).
+      assertThat(issueFor(outcome.issues(), "ballastMaterialCode").messageKey())
+          .isEqualTo("missingRequiredFieldMsg");
     }
 
     @Test

backend/src/test/java/ca/bc/gov/nrs/ilcr/schedule10/Schedule10CopyIT.java

@@ -77,11 +77,11 @@ void copyCarriesHeaderOnly() throws Exception {
     var stored = jdbc.queryForMap(
         "SELECT ENTRY_USERID, UPDATE_USERID, TSB_NUMBER_CODE, TFL_NUMBER_CODE, REVISION_COUNT"
             + " FROM THE.ROAD_CONSTRUCTION_REPRT WHERE ROAD_CONSTRUCTION_REPRT_ID = ?", copyId);
-    assertThat(stored.get("ENTRY_USERID")).isEqualTo("dev-submitter");
-    assertThat(stored.get("UPDATE_USERID")).isEqualTo("dev-submitter");
+    assertThat(stored).containsEntry("ENTRY_USERID", "dev-submitter");
+    assertThat(stored).containsEntry("UPDATE_USERID", "dev-submitter");
     assertThat(((Number) stored.get("REVISION_COUNT")).intValue()).isZero();
     // The location legs are carried across verbatim — the supply block was asserted nowhere before.
-    assertThat(stored.get("TSB_NUMBER_CODE")).isEqualTo("01A");
+    assertThat(stored).containsEntry("TSB_NUMBER_CODE", "01A");
     assertThat(stored.get("TFL_NUMBER_CODE")).isNull();
   }
 

backend/src/test/java/ca/bc/gov/nrs/ilcr/schedule10/Schedule10PageWriteIT.java

@@ -97,13 +97,13 @@ void createsTsaLocatedPage() throws Exception {
         "SELECT ILCR_CATEGORY_ID, REVISION_COUNT, ENTRY_USERID, ENTRY_TIMESTAMP, UPDATE_USERID,"
             + " UPDATE_TIMESTAMP, CONSTRUCTION_DATE FROM THE.ROAD_CONSTRUCTION_REPRT"
             + " WHERE ROAD_CONSTRUCTION_REPRT_ID = ?", pageId);
-    assertThat(stored.get("ILCR_CATEGORY_ID")).isEqualTo("10");
-    assertThat(stored.get("ENTRY_USERID")).isEqualTo("dev-submitter");
-    assertThat(stored.get("UPDATE_USERID")).isEqualTo("dev-submitter");
+    assertThat(stored).containsEntry("ILCR_CATEGORY_ID", "10");
+    assertThat(stored).containsEntry("ENTRY_USERID", "dev-submitter");
+    assertThat(stored).containsEntry("UPDATE_USERID", "dev-submitter");
     assertThat(((Number) stored.get("REVISION_COUNT")).intValue()).isZero();
     // A fresh insert stamps both from the same SYSDATE, so they are equal — and both sit in the
     // present, which a defaulted or absent value would not.
-    assertThat(stored.get("ENTRY_TIMESTAMP")).isEqualTo(stored.get("UPDATE_TIMESTAMP"));
+    assertThat(stored).containsEntry("ENTRY_TIMESTAMP", stored.get("UPDATE_TIMESTAMP"));
     assertThat((Timestamp) stored.get("ENTRY_TIMESTAMP"))
         .isAfter(Timestamp.valueOf("2020-01-01 00:00:00"));
     // Legacy never writes this column and every real delivery page holds NULL.
@@ -133,7 +133,7 @@ void tflPageClearsSupplyBlockCounterpart() throws Exception {
             + " WHERE ROAD_CONSTRUCTION_REPRT_ID = ?", page.get("pageId").asInt());
     assertThat(stored.get("TSA_NUMBER")).isNull();
     assertThat(stored.get("TSB_NUMBER_CODE")).isNull();
-    assertThat(stored.get("TFL_NUMBER_CODE")).isEqualTo("08");
+    assertThat(stored).containsEntry("TFL_NUMBER_CODE", "08");
   }
 
   @Test
@@ -194,8 +194,8 @@ void editBumpsRevisionAndRestampsUpdateColumns() throws Exception {
         "SELECT ENTRY_USERID, UPDATE_USERID FROM THE.ROAD_CONSTRUCTION_REPRT"
             + " WHERE ROAD_CONSTRUCTION_REPRT_ID = 8956");
     // ENTRY_* must survive an update untouched; only UPDATE_* is restamped.
-    assertThat(stored.get("ENTRY_USERID")).isEqualTo(entryBefore);
-    assertThat(stored.get("UPDATE_USERID")).isEqualTo("dev-submitter");
+    assertThat(stored).containsEntry("ENTRY_USERID", entryBefore);
+    assertThat(stored).containsEntry("UPDATE_USERID", "dev-submitter");
   }
 
   @Test