x1534 Improve error messages from section conf related to samples

src/main/java/uk/ac/sanger/sccp/stan/model/Labware.java

@@ -17,7 +17,7 @@
  * @author dr6
  */
 @Entity
-public class Labware {
+public class Labware implements HasIntId {
 
     /** The states a piece of labware may be in */
     public enum State {
@@ -55,6 +55,7 @@ public Labware(Integer id, String barcode, LabwareType labwareType, List<Slot> s
         setSlots(slots);
     }
 
+    @Override
     public Integer getId() {
         return this.id;
     }

src/main/java/uk/ac/sanger/sccp/stan/model/Sample.java

@@ -11,7 +11,7 @@
  * @author dr6
  */
 @Entity
-public class Sample {
+public class Sample implements HasIntId {
     @Id
     @GeneratedValue(strategy = GenerationType.IDENTITY)
     private Integer id;
@@ -31,6 +31,7 @@ public Sample(Integer id, String section, Tissue tissue, BioState bioState) {
         this.bioState = bioState;
     }
 
+    @Override
     public Integer getId() {
         return this.id;
     }

src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionValidationServiceImp.java

@@ -300,9 +300,27 @@ public void validateSections(Collection<String> problems, List<ConfirmSection> c
             }
         }
 
+        SampleDescriber sampleDescriber = makeSampleDescriber(plan);
+
         for (ConfirmSection con : cons) {
-            validateSection(problems, lw, con, plannedSampleIds, sampleMaxSection, sampleIdSections);
+            validateSection(problems, lw, con, plannedSampleIds, sampleMaxSection, sampleIdSections, sampleDescriber);
+        }
+    }
+
+    SampleDescriber makeSampleDescriber(PlanOperation plan) {
+        Map<Integer, Set<Slot>> sampleSlots = new HashMap<>();
+        Map<Integer, Sample> sampleMap = new HashMap<>();
+        for (PlanAction pa : plan.getPlanActions()) {
+            Integer sampleId = pa.getSample().getId();
+            sampleSlots.computeIfAbsent(sampleId, k -> new LinkedHashSet<>()).add(pa.getSource());
+            sampleMap.put(sampleId, pa.getSample());
         }
+        Set<Integer> labwareIds = sampleSlots.values().stream()
+                .flatMap(Collection::stream)
+                .map(Slot::getLabwareId)
+                .collect(toSet());
+        Map<Integer, Labware> lwMap =labwareRepo.findAllByIdIn(labwareIds).stream().collect(inMap(Labware::getId));
+        return new SampleDescriber(lwMap, sampleMap, sampleSlots);
     }
 
     /**
@@ -314,10 +332,11 @@ public void validateSections(Collection<String> problems, List<ConfirmSection> c
      * @param sampleMaxSection the max section already taken from each source sample id (can be null for some sample ids)
      * @param sampleIdSections a map of sample id to sections specified, which is filled in
      *                         by multiple calls to this method
+     * @param sampleDescriber helper to describe samples
      */
     public void validateSection(Collection<String> problems, Labware lw, ConfirmSection con,
                                 Map<Address, Set<Integer>> plannedSampleIds, Map<Integer, Integer> sampleMaxSection,
-                                Map<Integer, Set<String>> sampleIdSections) {
+                                Map<Integer, Set<String>> sampleIdSections, SampleDescriber sampleDescriber) {
         boolean ok = true;
         final LabwareType lt = lw.getLabwareType();
         if (nullOrEmpty(con.getDestinationAddresses())) {
@@ -366,8 +385,8 @@ public void validateSection(Collection<String> problems, Labware lw, ConfirmSect
         }
         for (Address address : con.getDestinationAddresses()) {
             if (!plannedSampleIds.getOrDefault(address, Set.of()).contains(sampleId)) {
-                addProblem(problems, "Sample id %s is not expected in address %s of labware %s.",
-                        sampleId, address, lw.getBarcode());
+                addProblem(problems, "Sample %s is not expected in address %s of labware %s.",
+                        sampleDescriber.describe(sampleId), address, lw.getBarcode());
             }
         }
 
@@ -377,14 +396,15 @@ public void validateSection(Collection<String> problems, Labware lw, ConfirmSect
             sections.add(section);
             sampleIdSections.put(sampleId, sections);
         } else if (section!=null && sections.contains(section)) {
-            addProblem(problems, "Repeated section: %s from sample id %s.", section, sampleId);
+            addProblem(problems, "Repeated section: %s from sample %s.", section, sampleDescriber.describe(sampleId));
         } else {
             sections.add(section);
         }
         Integer maxSection = sampleMaxSection.get(sampleId);
         Integer sectionInt = parseSectionInt(section);
         if (maxSection != null && sectionInt != null && sectionInt <= maxSection) {
-            addProblem(problems, "Section numbers from sample id %s must be greater than %s.", sampleId, maxSection);
+            addProblem(problems, "Section numbers (from sample %s) must be greater than %s.",
+                    sampleDescriber.describe(sampleId), maxSection);
         }
     }
 

src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/SampleDescriber.java

@@ -0,0 +1,98 @@
+package uk.ac.sanger.sccp.stan.service.operation.confirm;
+
+import uk.ac.sanger.sccp.stan.model.*;
+
+import java.util.*;
+import java.util.stream.Collectors;
+
+import static uk.ac.sanger.sccp.utils.BasicUtils.nullOrEmpty;
+
+/**
+ * Util to provide descriptions of samples used in ops.
+ * <p>Usage: <code>"You have misused sample "+describer.describe(sampleId)+"."</code>
+ * <br><code>You have misused sample 40 (section 8 of EXT12) from STAN-12 (A3).</code>
+ * <p>Descriptions are cached.
+ * @author dr6
+ */
+public class SampleDescriber {
+    private final Map<Integer, String> cache;
+    private final Map<Integer, Labware> lwMap;
+    private final Map<Integer, Sample> sampleMap;
+    private final Map<Integer, Set<Slot>> sampleSlots;
+
+    /**
+     * Creates a sample describer using the given information.
+     * @param lwMap map to get labware from labware id
+     * @param sampleMap map to get sample from sample id
+     * @param sampleSlots map from sample id to the slots the sample came from
+     */
+    public SampleDescriber(Map<Integer, Labware> lwMap, Map<Integer, Sample> sampleMap, Map<Integer, Set<Slot>> sampleSlots) {
+        this.lwMap = lwMap;
+        this.sampleMap = sampleMap;
+        this.sampleSlots = sampleSlots;
+        this.cache = new HashMap<>();
+    }
+
+    /**
+     * Gets a description of a sample, using an internal cache.
+     * @param sampleId the id of the sample to describe
+     * @return a string description of the sample
+     */
+    public String describe(Integer sampleId) {
+        return cache.computeIfAbsent(sampleId, this::makeDescription);
+    }
+
+    /**
+     * Describe some basic fields of the sample
+     */
+    String describeSampleFields(Sample sample) {
+        String xn = sample.getTissue().getExternalName();
+        if (sample.getSection()==null) {
+            return xn;
+        }
+        return "section "+sample.getSection()+" of "+xn;
+    }
+
+    /** Describe the location of the sample, if found. Otherwise returns null. */
+    String describeSampleLocations(Integer sampleId) {
+        Set<Slot> slots = sampleSlots.get(sampleId);
+        if (nullOrEmpty(slots)) {
+            return null;
+        }
+        Map<Integer, Set<Slot>> lwIdSlots = new LinkedHashMap<>();
+        for (Slot slot : slots) {
+            lwIdSlots.computeIfAbsent(slot.getLabwareId(), k -> new HashSet<>()).add(slot);
+        }
+        Set<String> locStrings = new LinkedHashSet<>();
+        for (Map.Entry<Integer, Set<Slot>> entry : lwIdSlots.entrySet()) {
+            Labware lw = lwMap.get(entry.getKey());
+            if (lw==null) {
+                continue;
+            }
+            if (lw.getLabwareType().getNumColumns()==1 && lw.getLabwareType().getNumRows()==1) {
+                locStrings.add(lw.getBarcode());
+            } else {
+                String addresses = entry.getValue().stream().map(Slot::getAddress).sorted().map(Object::toString).collect(Collectors.joining(","));
+                locStrings.add(lw.getBarcode()+" ("+addresses+")");
+            }
+        }
+        if (locStrings.isEmpty()) {
+            return null;
+        }
+        return String.join(", ", locStrings);
+    }
+
+    /** Creates a description from the sample fields and the known locations, if any */
+    String makeDescription(Integer sampleId) {
+        Sample sample = sampleMap.get(sampleId);
+        if (sample==null) {
+            return "id "+sampleId;
+        }
+        String samDesc = describeSampleFields(sample);
+        String locs = describeSampleLocations(sampleId);
+        if (locs==null) {
+            return String.format("id %s (%s)", sampleId, samDesc);
+        }
+        return String.format("id %s (%s) from %s", sampleId, samDesc, locs);
+    }
+}

src/test/java/uk/ac/sanger/sccp/stan/service/operation/confirm/TestConfirmSectionValidationService.java

@@ -437,7 +437,10 @@ public void testValidateSections() {
 
         final Set<String> problems = hashSetOf("Identifiable problem.");
 
-        doNothing().when(service).validateSection(any(), any(), any(), any(), any(), any());
+        doNothing().when(service).validateSection(any(), any(), any(), any(), any(), any(), any());
+
+        SampleDescriber mockSamDesc = mock(SampleDescriber.class);
+        doReturn(mockSamDesc).when(service).makeSampleDescriber(any());
 
         service.validateSections(problems, cons, lw1, plan, sampleIdSections);
 
@@ -447,8 +450,9 @@ public void testValidateSections() {
         Map<Address, Set<Integer>> plannedSampleIds = Map.of(A1, Set.of(sampleA.getId(), sampleB.getId()),
                 B3, Set.of(sampleB.getId()));
 
-        verify(service).validateSection(problems, lw1, cons.get(0), plannedSampleIds, sampleMaxSection, sampleIdSections);
-        verify(service).validateSection(problems, lw1, cons.get(1), plannedSampleIds, sampleMaxSection, sampleIdSections);
+
+        verify(service).validateSection(problems, lw1, cons.get(0), plannedSampleIds, sampleMaxSection, sampleIdSections, mockSamDesc);
+        verify(service).validateSection(problems, lw1, cons.get(1), plannedSampleIds, sampleMaxSection, sampleIdSections, mockSamDesc);
     }
 
     @MethodSource("validateSectionArgs")
@@ -466,12 +470,38 @@ public void testValidateSection(Labware lw, ConfirmSection con, Map<Address, Set
             }
             return true;
         });
+        SampleDescriber mockSamDesc = mock(SampleDescriber.class);
+        when(mockSamDesc.describe(any())).then(invocation -> {
+            Integer sampleId = invocation.getArgument(0);
+            return "id " + sampleId + " desc";
+        });
         Map<Integer, Set<String>> sampleIdSections = inputSampleIdSections==null ? new HashMap<>() : new HashMap<>(inputSampleIdSections);
-        service.validateSection(problems, lw, con, plannedSampleIds, sampleMaxSection, sampleIdSections);
+        service.validateSection(problems, lw, con, plannedSampleIds, sampleMaxSection, sampleIdSections, mockSamDesc);
         assertThat(problems).containsExactlyInAnyOrderElementsOf(expectedProblems);
         assertEquals(combineMaps(inputSampleIdSections, newSampleIdSections), sampleIdSections);
     }
 
+    @Test
+    void testMakeSampleDescriber() {
+        PlanOperation plan = new PlanOperation();
+        Sample[] samples = EntityFactory.makeSamples(2);
+        Labware tube = EntityFactory.makeLabware(EntityFactory.getTubeType(), samples[0]);
+        Labware plate = EntityFactory.makeLabware(EntityFactory.makeLabwareType(1,2), samples);
+        Slot[] slots = {tube.getFirstSlot(), plate.getFirstSlot(), plate.getSlots().getLast()};
+        List<PlanAction> pas = List.of(
+                new PlanAction(1, 1, slots[0], null, samples[0]),
+                new PlanAction(2, 1, slots[1], null, samples[1]),
+                new PlanAction(3, 1, slots[2], null, samples[1])
+        );
+        plan.setPlanActions(pas);
+        when(mockLwRepo.findAllByIdIn(any())).thenReturn(List.of(tube, plate));
+        SampleDescriber sd = service.makeSampleDescriber(plan);
+        assertNotNull(sd);
+        verify(mockLwRepo).findAllByIdIn(Set.of(tube.getId(), plate.getId()));
+        assertEquals(String.format("id %s (%s) from %s", samples[0].getId(), samples[0].getTissue().getExternalName(), tube.getBarcode()), sd.describe(samples[0].getId()));
+        assertEquals(String.format("id %s (%s) from %s (A1,A2)", samples[1].getId(), samples[1].getTissue().getExternalName(), plate.getBarcode()), sd.describe(samples[1].getId()));
+    }
+
     static Stream<Arguments> validateSectionArgs() {
         LabwareType lt = EntityFactory.makeLabwareType(2,3);
         Sample sampleA = EntityFactory.getSample();
@@ -508,15 +538,15 @@ static Stream<Arguments> validateSectionArgs() {
                 { lw, new ConfirmSection(A1, aid, "-4"), plannedSampleIds, sampleMaxSection, null,
                         "Bad section", null },
                 { lw, new ConfirmSection(A2, bid, "20"), plannedSampleIds, sampleMaxSection, null,
-                        "Sample id "+bid+" is not expected in address A2 of labware STAN-01.", Map.of(bid, Set.of("20"))},
+                        "Sample id "+bid+" desc is not expected in address A2 of labware STAN-01.", Map.of(bid, Set.of("20"))},
                 { lw, new ConfirmSection(B3, aid, "21"), plannedSampleIds, sampleMaxSection, null,
-                        "Sample id "+aid+" is not expected in address B3 of labware STAN-01.", Map.of(aid, Set.of("21"))},
+                        "Sample id "+aid+" desc is not expected in address B3 of labware STAN-01.", Map.of(aid, Set.of("21"))},
                 { lw, new ConfirmSection(A1, aid, "14"), plannedSampleIds, sampleMaxSection, Map.of(aid, Set.of("13","14")),
-                        "Repeated section: 14 from sample id "+aid+".", null},
+                        "Repeated section: 14 from sample id "+aid+" desc.", null},
                 { lw, new ConfirmSection(A1, aid, "8"), plannedSampleIds, sampleMaxSection, null,
-                        "Section numbers from sample id "+aid+" must be greater than 12.", Map.of(aid, Set.of("8"))},
+                        "Section numbers (from sample id "+aid+" desc) must be greater than 12.", Map.of(aid, Set.of("8"))},
                 { lw, new ConfirmSection(A1, aid, "12"), plannedSampleIds, sampleMaxSection, Map.of(aid, hashSetOf("16")),
-                        "Section numbers from sample id "+aid+" must be greater than 12.", Map.of(aid, Set.of("16","12"))},
+                        "Section numbers (from sample id "+aid+" desc) must be greater than 12.", Map.of(aid, Set.of("16","12"))},
                 { fw, new ConfirmSection(A1, aid, "20"), plannedSampleIds, sampleMaxSection, Map.of(aid, hashSetOf("18","19")),
                         "Section number not expected for fetal waste.", Map.of(aid, Set.of("18","19"))},
 

src/test/java/uk/ac/sanger/sccp/stan/service/operation/confirm/TestSampleDescriber.java

@@ -0,0 +1,80 @@
+package uk.ac.sanger.sccp.stan.service.operation.confirm;
+
+import org.junit.jupiter.api.Test;
+import uk.ac.sanger.sccp.stan.EntityFactory;
+import uk.ac.sanger.sccp.stan.model.*;
+import uk.ac.sanger.sccp.utils.Zip;
+
+import java.util.*;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.mockito.Mockito.*;
+import static uk.ac.sanger.sccp.utils.BasicUtils.inMap;
+
+/** Test {@link SampleDescriber} */
+class TestSampleDescriber {
+
+    static <E extends HasIntId> Map<Integer, E> makeIdMap(E[] items) {
+        return Arrays.stream(items).collect(inMap(E::getId));
+    }
+
+    @Test
+    void testDescribe() {
+        SampleDescriber sd = spy(new SampleDescriber(null, null, null));
+        final String desc = "sample 10 description";
+        doReturn(desc).when(sd).makeDescription(10);
+        assertEquals(desc, sd.describe(10));
+        assertEquals(desc, sd.describe(10));
+        verify(sd).makeDescription(10);
+    }
+
+    @Test
+    void testDescribeSampleFields() {
+        Sample[] samples = EntityFactory.makeSamples(2);
+        SampleDescriber sd = new SampleDescriber(null, makeIdMap(samples), null);
+        samples[0].setSection("15");
+        String xn = samples[0].getTissue().getExternalName();
+        assertEquals("section 15 of "+xn, sd.describeSampleFields(samples[0]));
+        assertEquals(xn, sd.describeSampleFields(samples[1]));
+    }
+
+    @Test
+    void testDescribeSampleLocations() {
+        Sample[] samples = EntityFactory.makeSamples(3);
+        Labware tube1 = EntityFactory.makeTube(samples[0]);
+        Labware tube2 = EntityFactory.makeTube(samples[1]);
+        Labware plate = EntityFactory.makeLabware(EntityFactory.makeLabwareType(1,2), samples[1], samples[1]);
+
+        Map<Integer, Sample> sampleMap = makeIdMap(samples);
+        Map<Integer, Labware> lwMap = makeIdMap(new Labware[]{tube1, tube2, plate});
+        Map<Integer, Set<Slot>> sampleSlots = Map.of(
+                samples[0].getId(), Set.of(tube1.getFirstSlot()),
+                samples[1].getId(), new LinkedHashSet<>(List.of(tube2.getFirstSlot(), plate.getFirstSlot(), plate.getSlots().getLast()))
+        );
+        SampleDescriber sd = new SampleDescriber(lwMap, sampleMap, sampleSlots);
+        assertEquals(tube1.getBarcode(), sd.describeSampleLocations(samples[0].getId()));
+        assertEquals(String.format("%s, %s (A1,A2)", tube2.getBarcode(), plate.getBarcode()), sd.describeSampleLocations(samples[1].getId()));
+        assertNull(sd.describeSampleLocations(samples[2].getId()));
+    }
+
+    @Test
+    void testMakeDescription() {
+        Sample[] samples = EntityFactory.makeSamples(3);
+        Integer[] sampleIds = Arrays.stream(samples).map(Sample::getId).toArray(Integer[]::new);
+        SampleDescriber sd = spy(new SampleDescriber(null, makeIdMap(samples), null));
+        String[] fieldDescs = {"EXT1", "section 4 of EXT2", "EXT3"};
+        Zip.of(Arrays.stream(samples), Arrays.stream(fieldDescs)).forEach((sam, desc) -> doReturn(desc).when(sd).describeSampleFields(sam));
+        String[] locDescs = {"STAN-1", "STAN-2 (A1), STAN-3", null};
+        Zip.of(Arrays.stream(sampleIds), Arrays.stream(locDescs)).forEach((id, desc) -> doReturn(desc).when(sd).describeSampleLocations(id));
+
+        String[] expected = {String.format("id %s (EXT1) from STAN-1", sampleIds[0]),
+                String.format("id %s (section 4 of EXT2) from STAN-2 (A1), STAN-3", sampleIds[1]),
+                String.format("id %s (EXT3)", sampleIds[2]),
+        };
+        Zip.of(Arrays.stream(sampleIds), Arrays.stream(expected))
+                .forEach((id, desc) -> assertEquals(desc, sd.makeDescription(id)));
+        Integer lastId = sampleIds[2]+1;
+        assertEquals("id "+lastId, sd.makeDescription(lastId));
+    }
+}