From e7985a3b9457fa5bc1b928f8a6febd04e0d2274c Mon Sep 17 00:00:00 2001 From: David Robinson <14000840+khelwood@users.noreply.github.com> Date: Mon, 24 Nov 2025 16:54:51 +0000 Subject: [PATCH 1/3] x1290 support multiple addresses in plan/confirm sectioning Add slot group entity. Remove support for creating regions in sectioning. --- .../ac/sanger/sccp/stan/model/SlotGroup.java | 102 +++++++++++ .../sanger/sccp/stan/repo/SlotGroupRepo.java | 10 ++ .../ac/sanger/sccp/stan/request/PlanData.java | 25 ++- .../stan/request/confirm/ConfirmSection.java | 46 +++-- .../sccp/stan/request/plan/PlanGroup.java | 48 ++++++ .../stan/request/plan/PlanRequestAction.java | 21 ++- .../stan/request/plan/PlanRequestLabware.java | 1 + .../sccp/stan/service/SlotGroupService.java | 27 +++ .../stan/service/SlotGroupServiceImp.java | 64 +++++++ .../confirm/ConfirmSectionServiceImp.java | 121 ++++++------- .../confirm/ConfirmSectionValidation.java | 10 +- .../ConfirmSectionValidationServiceImp.java | 96 ++++------- .../operation/plan/PlanServiceImp.java | 40 +++-- .../operation/plan/PlanValidationImp.java | 115 ++++++------- .../resources/db/changelog/changelog-3.19.xml | 38 ++++ .../db/changelog/changelog-master.xml | 1 + src/main/resources/schema.graphqls | 12 +- .../java/uk/ac/sanger/sccp/stan/Matchers.java | 17 ++ .../integrationtest/IntegrationTestUtils.java | 8 + .../TestPlanAndRecordSectionMutations.java | 49 ++---- .../TestRegisterOriginalSamplesMutation.java | 2 + .../stan/service/TestSlotGroupService.java | 69 ++++++++ .../confirm/TestConfirmSectionService.java | 162 ++++++++++-------- .../TestConfirmSectionValidationService.java | 112 +++--------- .../operation/plan/TestPlanService.java | 40 ++++- .../operation/plan/TestPlanValidation.java | 46 +++-- .../resources/graphql/confirmsection.graphql | 21 +-- .../graphql/confirmsection_simple.graphql | 2 +- src/test/resources/graphql/plan.graphql | 18 +- .../resources/graphql/plan_simple.graphql | 2 +- src/test/resources/graphql/plandata.graphql | 1 + 31 files changed, 810 insertions(+), 516 deletions(-) create mode 100644 src/main/java/uk/ac/sanger/sccp/stan/model/SlotGroup.java create mode 100644 src/main/java/uk/ac/sanger/sccp/stan/repo/SlotGroupRepo.java create mode 100644 src/main/java/uk/ac/sanger/sccp/stan/request/plan/PlanGroup.java create mode 100644 src/main/java/uk/ac/sanger/sccp/stan/service/SlotGroupService.java create mode 100644 src/main/java/uk/ac/sanger/sccp/stan/service/SlotGroupServiceImp.java create mode 100644 src/main/resources/db/changelog/changelog-3.19.xml create mode 100644 src/test/java/uk/ac/sanger/sccp/stan/service/TestSlotGroupService.java diff --git a/src/main/java/uk/ac/sanger/sccp/stan/model/SlotGroup.java b/src/main/java/uk/ac/sanger/sccp/stan/model/SlotGroup.java new file mode 100644 index 000000000..7734b4536 --- /dev/null +++ b/src/main/java/uk/ac/sanger/sccp/stan/model/SlotGroup.java @@ -0,0 +1,102 @@ +package uk.ac.sanger.sccp.stan.model; + +import javax.persistence.*; +import java.util.Objects; + +import static uk.ac.sanger.sccp.utils.BasicUtils.describe; + +/** + * A record that a slot is part of a particular group in a plan. + * @author dr6 + */ +@Entity +public class SlotGroup { + @Id + @GeneratedValue(strategy = GenerationType.IDENTITY) + private Integer id; + + private Integer groupIndex; + @ManyToOne + private Slot slot; + private Integer planId; + private Integer operationId; + + public SlotGroup() {} // required by JPA + + public SlotGroup(Integer groupIndex, Slot slot, Integer planId) { + this.groupIndex = groupIndex; + this.slot = slot; + this.planId = planId; + } + + public Integer getId() { + return this.id; + } + + public void setId(Integer id) { + this.id = id; + } + + public Integer getGroupIndex() { + return this.groupIndex; + } + + public void setGroupIndex(Integer groupIndex) { + this.groupIndex = groupIndex; + } + + public Slot getSlot() { + return this.slot; + } + + public Integer getSlotId() { + return (slot==null ? null : slot.getId()); + } + + public void setSlot(Slot slot) { + this.slot = slot; + } + + public Integer getPlanId() { + return this.planId; + } + + public void setPlanId(Integer planId) { + this.planId = planId; + } + + public Integer getOperationId() { + return this.operationId; + } + + public void setOperationId(Integer operationId) { + this.operationId = operationId; + } + + @Override + public boolean equals(Object o) { + if (o == null || getClass() != o.getClass()) return false; + SlotGroup that = (SlotGroup) o; + return (Objects.equals(this.id, that.id) + && Objects.equals(this.groupIndex, that.groupIndex) + && Objects.equals(this.slot, that.slot) + && Objects.equals(this.planId, that.planId) + && Objects.equals(this.operationId, that.operationId)); + } + + @Override + public int hashCode() { + return (id != null ? id.hashCode() : Objects.hash(groupIndex, slot, planId, operationId)); + } + + @Override + public String toString() { + return describe(this) + .add("id", id) + .add("groupIndex", groupIndex) + .add("slotId", getSlotId()) + .add("planId", planId) + .add("operationId", operationId) + .toString(); + } +} diff --git a/src/main/java/uk/ac/sanger/sccp/stan/repo/SlotGroupRepo.java b/src/main/java/uk/ac/sanger/sccp/stan/repo/SlotGroupRepo.java new file mode 100644 index 000000000..1c1fec5fe --- /dev/null +++ b/src/main/java/uk/ac/sanger/sccp/stan/repo/SlotGroupRepo.java @@ -0,0 +1,10 @@ +package uk.ac.sanger.sccp.stan.repo; + +import org.springframework.data.repository.CrudRepository; +import uk.ac.sanger.sccp.stan.model.SlotGroup; + +import java.util.List; + +public interface SlotGroupRepo extends CrudRepository { + List findByPlanId(Integer planId); +} diff --git a/src/main/java/uk/ac/sanger/sccp/stan/request/PlanData.java b/src/main/java/uk/ac/sanger/sccp/stan/request/PlanData.java index b11ff29ba..57f93387e 100644 --- a/src/main/java/uk/ac/sanger/sccp/stan/request/PlanData.java +++ b/src/main/java/uk/ac/sanger/sccp/stan/request/PlanData.java @@ -1,5 +1,6 @@ package uk.ac.sanger.sccp.stan.request; +import uk.ac.sanger.sccp.stan.model.Address; import uk.ac.sanger.sccp.stan.model.PlanOperation; import uk.ac.sanger.sccp.utils.BasicUtils; @@ -7,6 +8,7 @@ import java.util.Objects; import static uk.ac.sanger.sccp.utils.BasicUtils.newArrayList; +import static uk.ac.sanger.sccp.utils.BasicUtils.nullToEmpty; /** * The data about a previously recorded plan @@ -16,15 +18,17 @@ public class PlanData { private PlanOperation plan; private List sources; private LabwareFlagged destination; + private List> groups; - public PlanData(PlanOperation plan, Iterable sources, LabwareFlagged destination) { + public PlanData(PlanOperation plan, Iterable sources, LabwareFlagged destination, List> groups) { setPlan(plan); setSources(sources); setDestination(destination); + setGroups(groups); } public PlanData() { - this(null, null, null); + this(null, null, null, null); } public PlanOperation getPlan() { @@ -51,6 +55,18 @@ public void setDestination(LabwareFlagged destination) { this.destination = destination; } + public void setSources(List sources) { + this.sources = sources; + } + + public List> getGroups() { + return this.groups; + } + + public void setGroups(List> groups) { + this.groups = nullToEmpty(groups); + } + @Override public boolean equals(Object o) { if (this == o) return true; @@ -58,7 +74,9 @@ public boolean equals(Object o) { PlanData that = (PlanData) o; return (Objects.equals(this.plan, that.plan) && Objects.equals(this.sources, that.sources) - && Objects.equals(this.destination, that.destination)); + && Objects.equals(this.destination, that.destination) + && Objects.equals(this.groups, that.groups) + ); } @Override @@ -72,6 +90,7 @@ public String toString() { .add("plan", plan) .add("sources", sources) .add("destination", destination) + .add("groups", groups) .toString(); } } diff --git a/src/main/java/uk/ac/sanger/sccp/stan/request/confirm/ConfirmSection.java b/src/main/java/uk/ac/sanger/sccp/stan/request/confirm/ConfirmSection.java index f3e3ce8e5..6c828013c 100644 --- a/src/main/java/uk/ac/sanger/sccp/stan/request/confirm/ConfirmSection.java +++ b/src/main/java/uk/ac/sanger/sccp/stan/request/confirm/ConfirmSection.java @@ -7,40 +7,44 @@ import java.util.Objects; import static uk.ac.sanger.sccp.utils.BasicUtils.coalesce; +import static uk.ac.sanger.sccp.utils.BasicUtils.nullToEmpty; /** * The information about a particular section in a confirmed operation * @author dr6 */ public class ConfirmSection { - private Address destinationAddress; + private List
destinationAddresses = List.of(); private Integer sampleId; private Integer newSection; private List commentIds = List.of(); - private String region; private String thickness; public ConfirmSection() {} - public ConfirmSection(Address destinationAddress, Integer sampleId, Integer newSection, - List commentIds, String region) { - this.destinationAddress = destinationAddress; - this.sampleId = sampleId; - this.newSection = newSection; + public ConfirmSection(List
destinationAddresses, Integer sampleId, Integer newSection, + List commentIds) { + setDestinationAddress(destinationAddresses); + setSampleId(sampleId); + setNewSection(newSection); setCommentIds(commentIds); - this.region = region; + } + + public ConfirmSection(Address destinationAddress, Integer sampleId, Integer newSection, + List commentIds) { + this(destinationAddress==null ? null : List.of(destinationAddress), sampleId, newSection, commentIds); } public ConfirmSection(Address destinationAddress, Integer sampleId, Integer newSection) { - this(destinationAddress, sampleId, newSection, null, null); + this(destinationAddress, sampleId, newSection, null); } - public Address getDestinationAddress() { - return this.destinationAddress; + public List
getDestinationAddresses() { + return this.destinationAddresses; } - public void setDestinationAddress(Address destinationAddress) { - this.destinationAddress = destinationAddress; + public void setDestinationAddress(List
destinationAddresses) { + this.destinationAddresses = nullToEmpty(destinationAddresses); } public Integer getSampleId() { @@ -67,14 +71,6 @@ public void setCommentIds(List commentIds) { this.commentIds = coalesce(commentIds, List.of()); } - public String getRegion() { - return this.region; - } - - public void setRegion(String region) { - this.region = region; - } - public String getThickness() { return this.thickness; } @@ -88,28 +84,26 @@ public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; ConfirmSection that = (ConfirmSection) o; - return (Objects.equals(this.destinationAddress, that.destinationAddress) + return (Objects.equals(this.destinationAddresses, that.destinationAddresses) && Objects.equals(this.sampleId, that.sampleId) && Objects.equals(this.newSection, that.newSection) && Objects.equals(this.commentIds, that.commentIds) - && Objects.equals(this.region, that.region) && Objects.equals(this.thickness, that.thickness) ); } @Override public int hashCode() { - return Objects.hash(destinationAddress, sampleId, newSection); + return Objects.hash(destinationAddresses, sampleId, newSection); } @Override public String toString() { return BasicUtils.describe("ConfirmSection") - .add("destinationAddress", destinationAddress) + .add("destinationAddresses", destinationAddresses) .add("sampleId", sampleId) .add("newSection", newSection) .add("commentIds", commentIds) - .addRepr("region", region) .addRepr("thickness", thickness) .toString(); } diff --git a/src/main/java/uk/ac/sanger/sccp/stan/request/plan/PlanGroup.java b/src/main/java/uk/ac/sanger/sccp/stan/request/plan/PlanGroup.java new file mode 100644 index 000000000..a961efe07 --- /dev/null +++ b/src/main/java/uk/ac/sanger/sccp/stan/request/plan/PlanGroup.java @@ -0,0 +1,48 @@ +package uk.ac.sanger.sccp.stan.request.plan; + +import uk.ac.sanger.sccp.stan.model.Address; + +import java.util.List; +import java.util.Objects; + +import static uk.ac.sanger.sccp.utils.BasicUtils.nullToEmpty; + +/** + * @author dr6 + */ +public class PlanGroup { + private List
addresses; + + public PlanGroup() { + this(null); + } + + public PlanGroup(List
addresses) { + setAddresses(addresses); + } + + public List
getAddresses() { + return this.addresses; + } + + public void setAddresses(List
addresses) { + this.addresses = nullToEmpty(addresses); + } + + @Override + public boolean equals(Object o) { + if (o == null || getClass() != o.getClass()) return false; + PlanGroup that = (PlanGroup) o; + return Objects.equals(this.addresses, that.addresses); + } + + @Override + public int hashCode() { + return Objects.hash(addresses); + } + + @Override + public String toString() { + return this.addresses.toString(); + } +} diff --git a/src/main/java/uk/ac/sanger/sccp/stan/request/plan/PlanRequestAction.java b/src/main/java/uk/ac/sanger/sccp/stan/request/plan/PlanRequestAction.java index 21e8de86c..af9079377 100644 --- a/src/main/java/uk/ac/sanger/sccp/stan/request/plan/PlanRequestAction.java +++ b/src/main/java/uk/ac/sanger/sccp/stan/request/plan/PlanRequestAction.java @@ -3,14 +3,17 @@ import com.google.common.base.MoreObjects; import uk.ac.sanger.sccp.stan.model.Address; +import java.util.List; import java.util.Objects; +import static uk.ac.sanger.sccp.utils.BasicUtils.nullToEmpty; + /** * A request for an action (i.e. one sample moving from one slot to another slot) in a planned operation. * @author dr6 */ public class PlanRequestAction { - private Address address; + private List
addresses = List.of(); private int sampleId; private PlanRequestSource source; private String sampleThickness; @@ -18,18 +21,18 @@ public class PlanRequestAction { public PlanRequestAction() {} public PlanRequestAction(Address address, int sampleId, PlanRequestSource source, String sampleThickness) { - this.address = address; + this.addresses = address==null ? List.of() : List.of(address); this.sampleId = sampleId; this.source = source; this.sampleThickness = sampleThickness; } - public Address getAddress() { - return this.address; + public List
getAddresses() { + return this.addresses; } - public void setAddress(Address address) { - this.address = address; + public void setAddresses(List
addresses) { + this.addresses = nullToEmpty(addresses); } public int getSampleId() { @@ -62,20 +65,20 @@ public boolean equals(Object o) { if (o == null || getClass() != o.getClass()) return false; PlanRequestAction that = (PlanRequestAction) o; return (this.sampleId==that.sampleId - && Objects.equals(this.address, that.address) + && Objects.equals(this.addresses, that.addresses) && Objects.equals(this.source, that.source) && Objects.equals(this.sampleThickness, that.sampleThickness)); } @Override public int hashCode() { - return Objects.hash(address, sampleId, source); + return Objects.hash(addresses, sampleId, source); } @Override public String toString() { return MoreObjects.toStringHelper(this) - .add("address", address) + .add("addresses", addresses) .add("sampleId", sampleId) .add("source", source) .add("sampleThickness", sampleThickness) diff --git a/src/main/java/uk/ac/sanger/sccp/stan/request/plan/PlanRequestLabware.java b/src/main/java/uk/ac/sanger/sccp/stan/request/plan/PlanRequestLabware.java index ddac44a44..c5e3de7e8 100644 --- a/src/main/java/uk/ac/sanger/sccp/stan/request/plan/PlanRequestLabware.java +++ b/src/main/java/uk/ac/sanger/sccp/stan/request/plan/PlanRequestLabware.java @@ -15,6 +15,7 @@ public class PlanRequestLabware { private String lotNumber; private SlideCosting costing; private List actions; + private List groups; public PlanRequestLabware() { this(null, null, null); diff --git a/src/main/java/uk/ac/sanger/sccp/stan/service/SlotGroupService.java b/src/main/java/uk/ac/sanger/sccp/stan/service/SlotGroupService.java new file mode 100644 index 000000000..084ad0781 --- /dev/null +++ b/src/main/java/uk/ac/sanger/sccp/stan/service/SlotGroupService.java @@ -0,0 +1,27 @@ +package uk.ac.sanger.sccp.stan.service; + +import uk.ac.sanger.sccp.stan.model.*; + +import java.util.Collection; +import java.util.List; + +/** + * Service for creating and loading slot groups. + */ +public interface SlotGroupService { + /** + * Saves groups in the database. + * @param lw the labware containing the grouped slots + * @param planId the id of the plan creating the groups + * @param groups the groups (collections of addresses) + * @return the group records created in the database + */ + List saveGroups(Labware lw, Integer planId, Collection> groups); + + /** + * Loads the groups (lists of addresses) for a specified plan. + * @param planId the id of the plan + * @return a list of groups (lists of slot addresses) + */ + List> loadGroups(Integer planId); +} diff --git a/src/main/java/uk/ac/sanger/sccp/stan/service/SlotGroupServiceImp.java b/src/main/java/uk/ac/sanger/sccp/stan/service/SlotGroupServiceImp.java new file mode 100644 index 000000000..db8029168 --- /dev/null +++ b/src/main/java/uk/ac/sanger/sccp/stan/service/SlotGroupServiceImp.java @@ -0,0 +1,64 @@ +package uk.ac.sanger.sccp.stan.service; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Service; +import uk.ac.sanger.sccp.stan.model.*; +import uk.ac.sanger.sccp.stan.repo.SlotGroupRepo; + +import java.util.*; + +import static uk.ac.sanger.sccp.utils.BasicUtils.asList; + +/** + * @author dr6 + */ +@Service +public class SlotGroupServiceImp implements SlotGroupService { + private final SlotGroupRepo slotGroupRepo; + + @Autowired + public SlotGroupServiceImp(SlotGroupRepo slotGroupRepo) { + this.slotGroupRepo = slotGroupRepo; + } + + @Override + public List saveGroups(Labware lw, Integer planId, Collection> groups) { + int groupIndex = 1; + List records = new ArrayList<>(); + for (Collection
group : groups) { + for (Address address : group) { + Slot slot = lw.getSlot(address); + records.add(new SlotGroup(groupIndex, slot, planId)); + } + ++groupIndex; + } + if (records.isEmpty()) { + return List.of(); + } + return asList(slotGroupRepo.saveAll(records)); + } + + @Override + public List> loadGroups(Integer planId) { + List records = slotGroupRepo.findByPlanId(planId); + if (records.isEmpty()) { + return List.of(); + } + records = records.stream() + .sorted(Comparator.comparing(SlotGroup::getGroupIndex) + .thenComparing(g -> g.getSlot().getAddress())) + .toList(); + int currentGroupIndex = -1; + List> groups = new ArrayList<>(); + List
currentGroup = null; + for (SlotGroup record : records) { + if (currentGroup == null || record.getGroupIndex() != currentGroupIndex) { + currentGroup = new ArrayList<>(); + groups.add(currentGroup); + currentGroupIndex = record.getGroupIndex(); + } + currentGroup.add(record.getSlot().getAddress()); + } + return groups; + } +} diff --git a/src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionServiceImp.java b/src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionServiceImp.java index ca01aeb0c..d6d9cebf7 100644 --- a/src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionServiceImp.java +++ b/src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionServiceImp.java @@ -36,7 +36,6 @@ public class ConfirmSectionServiceImp implements ConfirmSectionService { private final CommentRepo commentRepo; private final OperationCommentRepo opCommentRepo; private final LabwareNoteRepo lwNoteRepo; - private final SamplePositionRepo samplePositionRepo; private final EntityManager entityManager; @@ -45,7 +44,7 @@ public ConfirmSectionServiceImp(ConfirmSectionValidationService validationServic OperationService opService, WorkService workService, LabwareRepo lwRepo, SlotRepo slotRepo, MeasurementRepo measurementRepo, SampleRepo sampleRepo, CommentRepo commentRepo, OperationCommentRepo opCommentRepo, - LabwareNoteRepo lwNoteRepo, SamplePositionRepo samplePositionRepo, + LabwareNoteRepo lwNoteRepo, EntityManager entityManager) { this.validationService = validationService; this.bioRiskService = bioRiskService; @@ -58,7 +57,6 @@ public ConfirmSectionServiceImp(ConfirmSectionValidationService validationServic this.commentRepo = commentRepo; this.opCommentRepo = opCommentRepo; this.lwNoteRepo = lwNoteRepo; - this.samplePositionRepo = samplePositionRepo; this.entityManager = entityManager; } @@ -99,7 +97,7 @@ public OperationResult perform(User user, ConfirmSectionValidation validation, C if (plan==null) { throw new IllegalArgumentException("No plan found for labware " + lw.getBarcode()); } - ConfirmLabwareResult clr = confirmLabware(user, csl, lw, plan, validation.getSlotRegions(), validation.getComments()); + ConfirmLabwareResult clr = confirmLabware(user, csl, lw, plan, validation.getComments()); var notes = plansNotes.get(plan.getId()); if (notes!=null && !notes.isEmpty()) { updateNotes(notes, clr.operation.getId(), lw.getId()); @@ -171,7 +169,7 @@ public void updateNotes(Collection notes, Integer opId, Integer lab * @return an operation (if one was created) and the labware (updated if necessary) */ public ConfirmLabwareResult confirmLabware(User user, ConfirmSectionLabware csl, Labware lw, PlanOperation plan, - UCMap slotRegions, Map commentIdMap) { + Map commentIdMap) { var secs = csl.getConfirmSections(); if (csl.isCancelled() || secs==null || secs.isEmpty()) { lw.setDiscarded(true); @@ -180,40 +178,39 @@ public ConfirmLabwareResult confirmLabware(User user, ConfirmSectionLabware csl, } final int lwId = lw.getId(); + Map sectionMap = new HashMap<>(); Set slotsToSave = new HashSet<>(); List measurements = new ArrayList<>(); - List samplePositions = new ArrayList<>(); List opComs = new ArrayList<>(); var planActionMap = getPlanActionMap(plan.getPlanActions(), lwId); List actions = new ArrayList<>(secs.size()); for (ConfirmSection sec : secs) { - PlanAction pa = planActionMap.get(new PlanActionKey(sec.getDestinationAddress(), sec.getSampleId())); - if (pa==null) { - throw new IllegalArgumentException(String.format("No plan action found matching section request: " + - "sample id %s in %s slot %s.", sec.getSampleId(), lw.getBarcode(), sec.getDestinationAddress())); - } - Slot slot = lw.getSlot(sec.getDestinationAddress()); // Use the lw object as the authoritative container for - // slot objects that we might do multiple updates on - Action action = makeAction(sec, pa, slot); - final Sample sample = action.getSample(); - slot.getSamples().add(sample); - slotsToSave.add(slot); - String thickness = thickness(sec, pa); - if (!nullOrEmpty(thickness)) { - measurements.add(new Measurement(null, "Thickness", thickness, - sample.getId(), null, slot.getId())); - } - if (!nullOrEmpty(sec.getRegion())) { - samplePositions.add(new SamplePosition(slot.getId(), sample.getId(), slotRegions.get(sec.getRegion()), null)); - } - if (!nullOrEmpty(sec.getCommentIds())) { - for (Integer commentId : sec.getCommentIds()) { - opComs.add(new OperationComment(null, commentIdMap.get(commentId), null, sample.getId(), slot.getId(), null)); + for (Address ad : sec.getDestinationAddresses()) { + PlanAction pa = planActionMap.get(ad); + if (pa == null) { + throw new IllegalArgumentException(String.format("No plan action found into %s slot %s.", + lw.getBarcode(), ad)); + } + Slot slot = lw.getSlot(ad); + final Sample sample = getSection(sectionMap, sec, pa, slot); + Action action = makeAction(sample, pa, slot); + slot.getSamples().add(sample); + slotsToSave.add(slot); + String thickness = thickness(sec, pa); + if (!nullOrEmpty(thickness)) { + measurements.add(new Measurement(null, "Thickness", thickness, + sample.getId(), null, slot.getId())); + } + if (!nullOrEmpty(sec.getCommentIds())) { + for (Integer commentId : sec.getCommentIds()) { + opComs.add(new OperationComment(null, commentIdMap.get(commentId), null, + sample.getId(), slot.getId(), null)); + } } + actions.add(action); } - actions.add(action); } slotRepo.saveAll(slotsToSave); entityManager.refresh(lw); @@ -222,10 +219,6 @@ public ConfirmLabwareResult confirmLabware(User user, ConfirmSectionLabware csl, measurements.forEach(m -> m.setOperationId(op.getId())); measurementRepo.saveAll(measurements); } - if (!samplePositions.isEmpty()) { - samplePositions.forEach(sp -> sp.setOperationId(op.getId())); - samplePositionRepo.saveAll(samplePositions); - } if (!opComs.isEmpty()) { opComs.forEach(oc -> oc.setOperationId(op.getId())); opCommentRepo.saveAll(opComs); @@ -246,45 +239,57 @@ public String thickness(ConfirmSection cs, PlanAction pa) { /** * Makes a new (unsaved) action representing the sectioning of a block into a new slot - * @param sec the request pertaining to a single section + * @param newSample the sample being put into the slot * @param pa the plan action that specified the sample being put into the new slot * @param slot the destination of the new section * @return a new unsaved action, containing the new section */ - public Action makeAction(ConfirmSection sec, PlanAction pa, Slot slot) { - Sample sample = getSection(sec, pa, slot); - return new Action(null, null, pa.getSource(), slot, sample, pa.getSample()); + public Action makeAction(Sample newSample, PlanAction pa, Slot slot) { + return new Action(null, null, pa.getSource(), slot, newSample, pa.getSample()); } /** - * Gets the required section. If it is already present in the slot (it should not be), then it is - * returned; otherwise it is created in the database and returned + * Gets the required section. If it's in the sectionMap already, it's returned. + * If it is already present in the slot (it should not be), then it is added to the sectionMap and return. + * Otherwise it is created in the database, added to the sectionMap and returned. + * The sectionMap uses tissue (id), section number and bio state in its keys. + * @param sectionMap a cache of existing sections to prevent dupes * @param sec the request pertaining to a single section * @param pa the plan action for the source sample going into this slot * @param slot the destination slot * @return the sample as specified */ - public Sample getSection(ConfirmSection sec, PlanAction pa, Slot slot) { + Sample getSection(Map sectionMap, ConfirmSection sec, PlanAction pa, Slot slot) { Sample sourceSample = pa.getSample(); + Tissue tissue = sourceSample.getTissue(); Integer section = sec.getNewSection(); BioState bs = coalesce(pa.getNewBioState(), sourceSample.getBioState()); - return slot.getSamples().stream() - .filter(sample -> sample.getTissue().getId().equals(sourceSample.getTissue().getId()) - && Objects.equals(sample.getSection(), section) - && bs.getId().equals(sample.getBioState().getId())) - .findFirst() - .orElseGet(() -> createSection(sourceSample, section, bs)); + SectionKey key = new SectionKey(tissue.getId(), section, bs); + Sample sam = sectionMap.get(key); + if (sam == null) { + sam = slot.getSamples().stream() + .filter(sample -> sample.getTissue().getId().equals(tissue.getId()) + && Objects.equals(sample.getSection(), section) + && bs.getId().equals(sample.getBioState().getId())) + .findFirst() + .orElse(null); + if (sam == null) { + sam = createSection(tissue, section, bs); + } + sectionMap.put(key, sam); + } + return sam; } /** * Creates a new section with the given section number and bio state. - * @param sourceSample the original sample (i.e. a block) + * @param tissue the tissue for the section * @param section the section number for the new sample * @param bs the biostate for the new sample * @return a new sample which has been created in the database */ - public Sample createSection(Sample sourceSample, Integer section, BioState bs) { - return sampleRepo.save(new Sample(null, section, sourceSample.getTissue(), bs)); + public Sample createSection(Tissue tissue, Integer section, BioState bs) { + return sampleRepo.save(new Sample(null, section, tissue, bs)); } /** @@ -341,17 +346,17 @@ private Stream streamOpComments(AddressCommentId ac, Map getPlanActionMap(Collection planActions, final int lwId) { - Map planActionMap = new HashMap<>(planActions.size()); + public Map getPlanActionMap(Collection planActions, final int lwId) { + Map planActionMap = new HashMap<>(planActions.size()); for (PlanAction pa : planActions) { if (pa.getDestination().getLabwareId()==lwId) { - planActionMap.putIfAbsent(new PlanActionKey(pa), pa); + planActionMap.putIfAbsent(pa.getDestination().getAddress(), pa); } } return planActionMap; @@ -385,12 +390,8 @@ public void updateSourceBlocks(Collection ops) { slotRepo.saveAll(slotsToUpdate.values()); } - /** A key used to identify a particular plan action so we can look it up. */ - public record PlanActionKey(Address address, Integer sectionId) { - PlanActionKey(PlanAction pa) { - this(pa.getDestination().getAddress(), pa.getSample().getId()); - } - } + /** Deduplication key for samples */ + record SectionKey(Integer tissueId, Integer section, BioState bs) {} /** * The result on an individual piece of labware of the confirmation request. diff --git a/src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionValidation.java b/src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionValidation.java index b81e60c1f..7cb7c46b6 100644 --- a/src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionValidation.java +++ b/src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionValidation.java @@ -13,7 +13,6 @@ public class ConfirmSectionValidation { private final Collection problems; private final UCMap labware; private final Map lwPlans; - private final UCMap slotRegions; private final Map comments; private final UCMap works; @@ -21,17 +20,15 @@ public ConfirmSectionValidation(Collection problems) { this.problems = problems; this.labware = null; this.lwPlans = null; - this.slotRegions = null; this.comments = null; this.works = null; } public ConfirmSectionValidation(UCMap labware, Map lwPlans, - UCMap slotRegions, Map comments, UCMap works) { + Map comments, UCMap works) { this.problems = List.of(); this.labware = labware; this.lwPlans = lwPlans; - this.slotRegions = slotRegions; this.comments = comments; this.works = works; } @@ -48,10 +45,6 @@ public Map getLwPlans() { return this.lwPlans; } - public UCMap getSlotRegions() { - return this.slotRegions; - } - public Map getComments() { return this.comments; } @@ -68,7 +61,6 @@ public boolean equals(Object o) { return (Objects.equals(this.problems, that.problems) && Objects.equals(this.labware, that.labware) && Objects.equals(this.lwPlans, that.lwPlans) - && Objects.equals(this.slotRegions, that.slotRegions) && Objects.equals(this.comments, that.comments) && Objects.equals(this.works, that.works) ); diff --git a/src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionValidationServiceImp.java b/src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionValidationServiceImp.java index 6bffa8ca3..6303a3b8c 100644 --- a/src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionValidationServiceImp.java +++ b/src/main/java/uk/ac/sanger/sccp/stan/service/operation/confirm/ConfirmSectionValidationServiceImp.java @@ -9,7 +9,6 @@ import uk.ac.sanger.sccp.stan.request.confirm.*; import uk.ac.sanger.sccp.stan.request.confirm.ConfirmSectionLabware.AddressCommentId; import uk.ac.sanger.sccp.stan.service.CommentValidationService; -import uk.ac.sanger.sccp.stan.service.SlotRegionService; import uk.ac.sanger.sccp.stan.service.sanitiser.Sanitiser; import uk.ac.sanger.sccp.stan.service.work.WorkService; import uk.ac.sanger.sccp.utils.BasicUtils; @@ -31,19 +30,16 @@ public class ConfirmSectionValidationServiceImp implements ConfirmSectionValidat private final LabwareRepo labwareRepo; private final PlanOperationRepo planOpRepo; private final WorkService workService; - private final SlotRegionService slotRegionService; private final CommentValidationService commentValidationService; private final Sanitiser thicknessSanitiser; @Autowired public ConfirmSectionValidationServiceImp(LabwareRepo labwareRepo, PlanOperationRepo planOpRepo, - WorkService workService, SlotRegionService slotRegionService, - CommentValidationService commentValidationService, + WorkService workService, CommentValidationService commentValidationService, @Qualifier("thicknessSanitiser") Sanitiser thicknessSanitiser) { this.labwareRepo = labwareRepo; this.planOpRepo = planOpRepo; this.workService = workService; - this.slotRegionService = slotRegionService; this.commentValidationService = commentValidationService; this.thicknessSanitiser = thicknessSanitiser; } @@ -57,8 +53,7 @@ public ConfirmSectionValidation validate(ConfirmSectionRequest request) { return new ConfirmSectionValidation(problems); } UCMap labware = validateLabware(problems, request.getLabware()); - UCMap slotRegions = validateSlotRegions(problems, request.getLabware()); - requireRegionsForMultiSampleSlots(problems, request.getLabware()); + checkRepeatedDestSlots(problems, request.getLabware()); Map plans = lookUpPlans(problems, labware.values()); validateOperations(problems, request.getLabware(), labware, plans); Map commentIdMap = validateCommentIds(problems, request.getLabware()); @@ -71,34 +66,32 @@ public ConfirmSectionValidation validate(ConfirmSectionRequest request) { if (!problems.isEmpty()) { return new ConfirmSectionValidation(problems); } - return new ConfirmSectionValidation(labware, plans, slotRegions, commentIdMap, works); + return new ConfirmSectionValidation(labware, plans, commentIdMap, works); } /** - * Checks that each section specified in a slot with multiple sections also specifies a region. + * Checks that slots aren't assigned multiple sections * @param problems receptacle for problems found * @param csls the specification of each labware */ - public void requireRegionsForMultiSampleSlots(Collection problems, Collection csls) { + public void checkRepeatedDestSlots(Collection problems, Collection csls) { for (ConfirmSectionLabware csl : csls) { if (nullOrEmpty(csl.getBarcode())) { continue; // Request is already broken, and we can't give meaningful problem messages } Map sectionCount = new HashMap<>(); - Set
addressesWithoutRegions = new HashSet<>(); for (ConfirmSection cs : csl.getConfirmSections()) { - final Address address = cs.getDestinationAddress(); - if (address != null) { - sectionCount.merge(address, 1, Integer::sum); - if (nullOrEmpty(cs.getRegion())) { - addressesWithoutRegions.add(address); + for (Address address : cs.getDestinationAddresses()) { + if (address != null) { + sectionCount.merge(address, 1, Integer::sum); } } } - for (Address address : addressesWithoutRegions) { - if (sectionCount.get(address) > 1) { - problems.add("A region must be specified for each section in slot "+address+" of "+csl.getBarcode()+"."); + for (Map.Entry entry : sectionCount.entrySet()) { + if (entry.getValue() > 1) { + problems.add(String.format("Multiple actions linked to destination %s %s.", + csl.getBarcode(), entry.getKey())); } } } @@ -155,43 +148,6 @@ public UCMap validateLabware(Collection problems, List validateSlotRegions(Collection problems, List csls) { - if (csls.stream().flatMap(csl -> csl.getConfirmSections().stream().map(ConfirmSection::getRegion)) - .allMatch(BasicUtils::nullOrEmpty)) { - return new UCMap<>(0); - } - UCMap slotRegions = slotRegionService.loadSlotRegionMap(true); - - for (ConfirmSectionLabware csl : csls) { - if (nullOrEmpty(csl.getBarcode())) { - continue; - } - Map> used = new HashMap<>(); - for (ConfirmSection cs : csl.getConfirmSections()) { - final String regionName = cs.getRegion(); - if (nullOrEmpty(regionName)) { - continue; - } - SlotRegion sr = slotRegions.get(regionName); - if (sr == null) { - problems.add("Unknown region: "+repr(regionName)); - continue; - } - final Address address = cs.getDestinationAddress(); - if (address == null) { - continue; - } - Set usedRegions = used.computeIfAbsent(address, k -> new HashSet<>()); - if (!usedRegions.add(sr)) { - problems.add(String.format("Region %s specified twice for %s in %s.", - sr.getName(), address, csl.getBarcode())); - } - } - } - return slotRegions; - } - - /** * Looks up the plans for the given labware. * Each labware should have exactly one plan, which should be a sectioning plan. @@ -360,13 +316,22 @@ public void validateSection(Collection problems, Labware lw, ConfirmSect Map> sampleIdSections) { boolean ok = true; final LabwareType lt = lw.getLabwareType(); - final Address address = con.getDestinationAddress(); - if (address == null) { + if (nullOrEmpty(con.getDestinationAddresses())) { addProblem(problems, "Section specified with no address."); ok = false; - } else if (lt.indexOf(address) < 0) { - addProblem(problems, "Invalid address %s in labware %s specified as destination.", address, lw.getBarcode()); - ok = false; + } + Set
seenAddresses = new HashSet<>(); + for (Address ad : con.getDestinationAddresses()) { + if (ad==null) { + addProblem(problems, "Null given as destination address."); + ok = false; + } else if (lt.indexOf(ad) < 0) { + addProblem(problems, "Invalid address %s in labware %s specified as destination.", ad, lw.getBarcode()); + ok = false; + } else if (!seenAddresses.add(ad)) { + addProblem(problems, "Repeated destination address %s specified for labware %s.", ad, lw.getBarcode()); + ok = false; + } } final Integer sampleId = con.getSampleId(); final Integer section = con.getNewSection(); @@ -390,10 +355,11 @@ public void validateSection(Collection problems, Labware lw, ConfirmSect if (!ok) { return; } - - 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()); + 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()); + } } Set sections = sampleIdSections.get(sampleId); diff --git a/src/main/java/uk/ac/sanger/sccp/stan/service/operation/plan/PlanServiceImp.java b/src/main/java/uk/ac/sanger/sccp/stan/service/operation/plan/PlanServiceImp.java index 2ed9fd014..c63d95c9f 100644 --- a/src/main/java/uk/ac/sanger/sccp/stan/service/operation/plan/PlanServiceImp.java +++ b/src/main/java/uk/ac/sanger/sccp/stan/service/operation/plan/PlanServiceImp.java @@ -7,8 +7,7 @@ import uk.ac.sanger.sccp.stan.request.LabwareFlagged; import uk.ac.sanger.sccp.stan.request.PlanData; import uk.ac.sanger.sccp.stan.request.plan.*; -import uk.ac.sanger.sccp.stan.service.LabwareService; -import uk.ac.sanger.sccp.stan.service.ValidationException; +import uk.ac.sanger.sccp.stan.service.*; import uk.ac.sanger.sccp.stan.service.flag.FlagLookupService; import uk.ac.sanger.sccp.utils.BasicUtils; import uk.ac.sanger.sccp.utils.UCMap; @@ -17,7 +16,6 @@ import java.util.*; import java.util.function.Predicate; -import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toSet; import static uk.ac.sanger.sccp.utils.BasicUtils.nullOrEmpty; @@ -29,6 +27,7 @@ public class PlanServiceImp implements PlanService { private final PlanValidationFactory planValidationFactory; private final LabwareService lwService; private final FlagLookupService flagLookupService; + private final SlotGroupService slotGroupService; private final PlanOperationRepo planRepo; private final PlanActionRepo planActionRepo; private final OperationTypeRepo opTypeRepo; @@ -39,13 +38,14 @@ public class PlanServiceImp implements PlanService { @Autowired public PlanServiceImp(PlanValidationFactory planValidationFactory, - LabwareService lwService, FlagLookupService flagLookupService, + LabwareService lwService, FlagLookupService flagLookupService, SlotGroupService slotGroupService, PlanOperationRepo planRepo, PlanActionRepo planActionRepo, OperationTypeRepo opTypeRepo, LabwareRepo lwRepo, LabwareTypeRepo ltRepo, LabwareNoteRepo lwNoteRepo, BioStateRepo bsRepo) { this.planValidationFactory = planValidationFactory; this.lwService = lwService; this.flagLookupService = flagLookupService; + this.slotGroupService = slotGroupService; this.planRepo = planRepo; this.planActionRepo = planActionRepo; this.opTypeRepo = opTypeRepo; @@ -96,6 +96,7 @@ public PlanResult executePlanRequest(User user, PlanRequest request) { if (!nullOrEmpty(pl.getLotNumber())) { lwNotes.add(LabwareNote.noteForPlan(lw.getId(), plan.getId(), "lot", pl.getLotNumber().toUpperCase())); } + createSlotGroups(plan.getId(), lw, pl); } if (!lwNotes.isEmpty()) { lwNoteRepo.saveAll(lwNotes); @@ -166,7 +167,7 @@ public List createActions(PlanRequestLabware requestLabware, int pla UCMap sources, Labware destination, BioState newBioState) { final List actions = new ArrayList<>(requestLabware.getActions().size()); - for (PlanRequestAction prac : sortedActions(requestLabware.getActions())) { + for (PlanRequestAction prac : requestLabware.getActions()) { Labware source = sources.get(prac.getSource().getBarcode()); Slot slot0; if (prac.getSource().getAddress()!=null) { @@ -174,16 +175,19 @@ public List createActions(PlanRequestLabware requestLabware, int pla } else { slot0 = source.getFirstSlot(); } - Slot slot1 = destination.getSlot(prac.getAddress()); Sample originalSample = slot0.getSamples().stream() .filter(sample -> sample.getId() == prac.getSampleId()) .findAny() .orElseThrow(() -> new EntityNotFoundException("Sample " + prac.getSampleId() + " not found in " + prac.getSource())); - PlanAction action = new PlanAction(null, planId, slot0, slot1, originalSample, - null, prac.getSampleThickness(), newBioState); - actions.add(action); + for (Address ad : prac.getAddresses()) { + Slot slot1 = destination.getSlot(ad); + PlanAction action = new PlanAction(null, planId, slot0, slot1, originalSample, + null, prac.getSampleThickness(), newBioState); + actions.add(action); + } } + actions.sort(Comparator.comparing(a -> a.getDestination().getAddress(), Address.COLUMN_MAJOR)); return BasicUtils.asList(planActionRepo.saveAll(actions)); } @@ -215,7 +219,8 @@ public PlanData getPlanData(String barcode, boolean loadFlags) { .map(lw -> new LabwareFlagged(lw, null)) .toList(); } - return new PlanData(plan, lfSources, lfDest); + List> groups = slotGroupService.loadGroups(plan.getId()); + return new PlanData(plan, lfSources, lfDest, groups); } public void validateLabwareForPlanData(Labware labware) { @@ -241,9 +246,16 @@ public List getSources(PlanOperation plan) { return lwRepo.findAllByIdIn(labwareIds); } - private static List sortedActions(List pracs) { - return pracs.stream() - .sorted(Comparator.comparing(PlanRequestAction::getAddress, Address.COLUMN_MAJOR)) - .collect(toList()); + /** + * Creates the slot groups for the given plan and labware + * @param planId the id of the plan record + * @param lw the destination labware + * @param prl the request for the given labware + */ + public void createSlotGroups(int planId, Labware lw, PlanRequestLabware prl) { + List> addressGroups = prl.getActions().stream() + .map(PlanRequestAction::getAddresses) + .toList(); + slotGroupService.saveGroups(lw, planId, addressGroups); } } diff --git a/src/main/java/uk/ac/sanger/sccp/stan/service/operation/plan/PlanValidationImp.java b/src/main/java/uk/ac/sanger/sccp/stan/service/operation/plan/PlanValidationImp.java index 9f76319fa..c12687c29 100644 --- a/src/main/java/uk/ac/sanger/sccp/stan/service/operation/plan/PlanValidationImp.java +++ b/src/main/java/uk/ac/sanger/sccp/stan/service/operation/plan/PlanValidationImp.java @@ -10,6 +10,7 @@ import java.util.*; import java.util.stream.Stream; +import static java.util.stream.Collectors.toSet; import static uk.ac.sanger.sccp.utils.BasicUtils.nullOrEmpty; import static uk.ac.sanger.sccp.utils.BasicUtils.pluralise; @@ -56,7 +57,7 @@ public Collection validate() { } /** - * Checks the sources. Returns a map of {@link ActionKey} to source slot for each action. + * Checks the sources. * @param opType the type of operation */ public UCMap validateSources(OperationType opType) { @@ -299,57 +300,74 @@ public void checkActions(PlanRequestLabware lw, LabwareType lt) { addProblem("No actions specified for labware %s.", lwErrorDesc(lw)); return; } - Set keys = new HashSet<>(lw.getActions().size()); + Set
seenAddresses = new HashSet<>(); for (PlanRequestAction ac : lw.getActions()) { - if (ac.getAddress()==null) { + if (nullOrEmpty(ac.getAddresses())) { addProblem("Missing destination address."); continue; } - if (lt!=null && lt.indexOf(ac.getAddress()) < 0) { - addProblem("Invalid address %s given for labware type %s.", ac.getAddress(), lt.getName()); + if (lt != null) { + Set
invalidAddresses = ac.getAddresses().stream() + .filter(ad -> lt.indexOf(ad) < 0) + .collect(toSet()); + if (!invalidAddresses.isEmpty()) { + addProblem("Invalid address given for labware type %s: %s", + lt.getName(), + invalidAddresses); + } } - ActionKey key = new ActionKey(ac); - if (key.isComplete() && !keys.add(key)) { - // We allow duplicate actions from a block, because we can create multiple sections - addProblem("Actions for labware %s contain duplicate action: %s", lwErrorDesc(lw), key); + for (Address ad : ac.getAddresses()) { + if (!seenAddresses.add(ad)) { + addProblem("Actions for labware %s contains duplicate address: %s", lwErrorDesc(lw), ad); + } } } } + /** + * Are rows laid out with only one tissue on each row? + * @param sourceLwMap look up source labware from barcode + * @param lw the destination labware + * @param lt the labware type of the destination labware + * @param rowsPerGroup number of rows expected for each tissue + * @return true if the layout is divided as expected, false otherwise + */ public boolean hasDividedLayout(UCMap sourceLwMap, PlanRequestLabware lw, LabwareType lt, int rowsPerGroup) { final int numGroups = lt.getNumRows() / rowsPerGroup; Tissue[] tissues = new Tissue[numGroups]; for (var pa : lw.getActions()) { - if (pa.getAddress()==null || pa.getSource()==null || pa.getSource().getBarcode()==null) { + if (nullOrEmpty(pa.getAddresses()) || pa.getSource()==null || pa.getSource().getBarcode()==null) { continue; } - int tissueIndex = (pa.getAddress().getRow()-1)/rowsPerGroup; - if (tissueIndex < 0 || tissueIndex >= numGroups) { - continue; // must be invalid address, which is handled elsewhere - } - Labware sourceLabware = sourceLwMap.get(pa.getSource().getBarcode()); - if (sourceLabware==null) { - continue; - } - Address sourceAddress = pa.getSource().getAddress(); - if (sourceAddress==null) { - sourceAddress = new Address(1,1); - } - Slot sourceSlot = sourceLabware.optSlot(sourceAddress).orElse(null); - if (sourceSlot==null) { - continue; - } - Sample sample = sourceSlot.getSamples().stream() - .filter(sam -> sam.getId()==pa.getSampleId()) - .findAny().orElse(null); - if (sample==null) { - continue; - } - Tissue tissue = sample.getTissue(); - if (tissues[tissueIndex]==null) { - tissues[tissueIndex] = tissue; - } else if (!tissues[tissueIndex].equals(tissue)) { - return false; + for (Address ad : pa.getAddresses()) { + int tissueIndex = (ad.getRow() - 1) / rowsPerGroup; + if (tissueIndex < 0 || tissueIndex >= numGroups) { + continue; // must be invalid address, which is handled elsewhere + } + Labware sourceLabware = sourceLwMap.get(pa.getSource().getBarcode()); + if (sourceLabware == null) { + continue; + } + Address sourceAddress = pa.getSource().getAddress(); + if (sourceAddress == null) { + sourceAddress = new Address(1, 1); + } + Slot sourceSlot = sourceLabware.optSlot(sourceAddress).orElse(null); + if (sourceSlot == null) { + continue; + } + Sample sample = sourceSlot.getSamples().stream() + .filter(sam -> sam.getId() == pa.getSampleId()) + .findAny().orElse(null); + if (sample == null) { + continue; + } + Tissue tissue = sample.getTissue(); + if (tissues[tissueIndex] == null) { + tissues[tissueIndex] = tissue; + } else if (!tissues[tissueIndex].equals(tissue)) { + return false; + } } } return true; @@ -376,27 +394,4 @@ private void addProblem(String problem) { private void addProblem(String format, Object... args) { addProblem(String.format(format, args)); } - - record ActionKey(String sourceBarcode, Address sourceAddress, int sampleId, Address destAddress) { - ActionKey(PlanRequestAction action) { - this(action.getSource().getBarcode(), action.getSource().getAddress(), - action.getSampleId(), action.getAddress()); - } - - ActionKey(String sourceBarcode, Address sourceAddress, int sampleId, Address destAddress) { - this.sourceBarcode = (sourceBarcode==null ? null : sourceBarcode.toUpperCase()); - this.sourceAddress = (sourceAddress==null ? new Address(1,1) : sourceAddress); - this.sampleId = sampleId; - this.destAddress = destAddress; - } - - boolean isComplete() { - return (this.sourceAddress!=null && this.sourceBarcode!=null && this.destAddress!=null); - } - - @Override - public String toString() { - return String.format("(address=%s, sampleId=%s, source={%s, %s})", destAddress, sampleId, sourceBarcode, sourceAddress); - } - } } diff --git a/src/main/resources/db/changelog/changelog-3.19.xml b/src/main/resources/db/changelog/changelog-3.19.xml new file mode 100644 index 000000000..b1691f719 --- /dev/null +++ b/src/main/resources/db/changelog/changelog-3.19.xml @@ -0,0 +1,38 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/main/resources/db/changelog/changelog-master.xml b/src/main/resources/db/changelog/changelog-master.xml index 0d3774340..142d0b0b4 100644 --- a/src/main/resources/db/changelog/changelog-master.xml +++ b/src/main/resources/db/changelog/changelog-master.xml @@ -42,4 +42,5 @@ + diff --git a/src/main/resources/schema.graphqls b/src/main/resources/schema.graphqls index 5c75ca79e..f9c5b9632 100644 --- a/src/main/resources/schema.graphqls +++ b/src/main/resources/schema.graphqls @@ -528,8 +528,8 @@ type PlanOperation { """A specification of an action in a plan request. Describes the action on a sample being transferred between slots.""" input PlanRequestAction { - """The intended address of the sample in the destination labware.""" - address: Address! + """The intended addresses of the sample in the destination labware.""" + addresses: [Address!]! """The id of the existing sample.""" sampleId: Int! """The thickness (if specified) of the new sample.""" @@ -597,16 +597,14 @@ input ConfirmOperationRequest { """A specification of a section to confirm in a planned sectioning operation.""" input ConfirmSection { - """The address of the destination slot for the section.""" - destinationAddress: Address! + """The addresses of the destination slots for the section.""" + destinationAddresses: [Address!]! """The original sample id of the source.""" sampleId: Int! """The section number of the new section.""" newSection: Int """The comment ids to record against this sample in this slot.""" commentIds: [Int!] - """The region of this sample in this slot, if any.""" - region: String """The thickness of the section, overriding the thickness specified in the plan.""" thickness: String } @@ -1213,6 +1211,8 @@ type PlanData { plan: PlanOperation! """The single item of destination labware for the plan.""" destination: LabwareFlagged! + """The groups of addresses specified by the plan.""" + groups: [[Address!]!]! } """A project that work can be associated with.""" diff --git a/src/test/java/uk/ac/sanger/sccp/stan/Matchers.java b/src/test/java/uk/ac/sanger/sccp/stan/Matchers.java index 50c8a2fab..d1b550361 100644 --- a/src/test/java/uk/ac/sanger/sccp/stan/Matchers.java +++ b/src/test/java/uk/ac/sanger/sccp/stan/Matchers.java @@ -201,6 +201,23 @@ public static Stubber mayAddProblem(final String problem, X returnValue) { return (problem == null ? doReturn(returnValue) : doAnswer(addProblem(problem, returnValue))); } + /** + * Like doReturn(a, b, c) for when (a,b,c) are in an iterator. + * After the iterator is exhausted, the last value will be returned + * @param xs iterator of items to return + * @return a stub for ongoing mocking + * @param return type of the mocked method + */ + public static Stubber doReturnFrom(final Iterator xs) { + final Object[] last = {null}; + return doAnswer(invocation -> { + if (xs.hasNext()) { + last[0] = xs.next(); + } + return last[0]; + }); + } + /** * A stub that will add the given problem (see {@link #addProblem}) if it is non-null; and * will return null/void diff --git a/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/IntegrationTestUtils.java b/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/IntegrationTestUtils.java index 0f02e0152..9214a9c7a 100644 --- a/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/IntegrationTestUtils.java +++ b/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/IntegrationTestUtils.java @@ -16,6 +16,8 @@ import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toMap; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.*; import static org.mockito.Mockito.*; @@ -53,6 +55,12 @@ public static List chainGetList(Object container, Object... accessors) { return chainGet(container, accessors); } + public static void assertNoErrors(Object response) { + if (chainGet(response, "errors") != null) { + fail(response.toString()); + } + } + public static Map nullableMapOf(K key1, V value1, K key2, V value2) { Map map = new HashMap<>(2); map.put(key1, value1); diff --git a/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/TestPlanAndRecordSectionMutations.java b/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/TestPlanAndRecordSectionMutations.java index aead728c4..571b6e31b 100644 --- a/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/TestPlanAndRecordSectionMutations.java +++ b/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/TestPlanAndRecordSectionMutations.java @@ -20,8 +20,7 @@ import static java.util.stream.Collectors.toList; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.*; -import static uk.ac.sanger.sccp.stan.integrationtest.IntegrationTestUtils.chainGet; -import static uk.ac.sanger.sccp.stan.integrationtest.IntegrationTestUtils.chainGetList; +import static uk.ac.sanger.sccp.stan.integrationtest.IntegrationTestUtils.*; /** * Tests the plan and record section mutations @@ -65,7 +64,7 @@ public void testPlanAndRecordSection() throws Exception { mutation = mutation.replace("55555", String.valueOf(blockSamples[0].getId())); mutation = mutation.replace("55556", String.valueOf(blockSamples[1].getId())); Map result = tester.post(mutation); - assertNull(result.get("errors")); + assertNoErrors(result); Object resultPlan = chainGet(result, "data", "plan"); List planResultLabware = chainGet(resultPlan, "labware"); assertEquals(3, planResultLabware.size()); @@ -87,8 +86,8 @@ public void testPlanAndRecordSection() throws Exception { } List> resultActions = chainGet(resultOps, 0, "planActions"); - String[] expectedPlanDestAddresses = { "A1", "A1", "B1", "B2" }; - int[] expectedPlanSampleId = {blockSamples[0].getId(), blockSamples[1].getId(), blockSamples[0].getId(), blockSamples[1].getId()}; + String[] expectedPlanDestAddresses = { "A1", "B2" }; + int[] expectedPlanSampleId = {blockSamples[0].getId(), blockSamples[1].getId()}; assertEquals(expectedPlanDestAddresses.length, resultActions.size()); for (int i = 0; i < expectedPlanDestAddresses.length; ++i) { @@ -119,7 +118,7 @@ private void testRetrievePlanData(String[] barcodes) throws Exception { .map(m -> m.get("barcode")) .collect(toList())).containsExactlyInAnyOrder(sources); List> planActionsData = chainGet(planData, "plan", "planActions"); - assertThat(planActionsData).hasSize(4); + assertThat(planActionsData).hasSize(2); for (int i = 1; i < barcodes.length; ++i) { String barcode = barcodes[i]; // fetal waste barcode @@ -134,6 +133,7 @@ private void testRetrievePlanData(String[] barcodes) throws Exception { planActionsData = chainGet(planData, "plan", "planActions"); assertThat(planActionsData).hasSize(1); } + assertEquals(List.of(List.of("A1")), planData.get("groups")); } private void testConfirm(Sample[] blockSamples, Labware[] sourceBlocks, String[] barcodes) throws Exception { @@ -148,7 +148,7 @@ private void testConfirm(Sample[] blockSamples, Labware[] sourceBlocks, String[] sbReplace(sb, "55556", String.valueOf(blockSamples[1].getId())); sbReplace(sb, "SGP4000", work.getWorkNumber()); Map result = tester.post(sb.toString()); - assertNull(result.get("errors")); + assertNoErrors(result); Object resultConfirm = chainGet(result, "data", "confirmSection"); List resultLabware = chainGet(resultConfirm, "labware"); @@ -162,13 +162,12 @@ private void testConfirm(Sample[] blockSamples, Labware[] sourceBlocks, String[] .findAny() .orElseThrow(), "samples"); - String[] expectedTissueNames = { "TISSUE1", "TISSUE1", "TISSUE2" }; - int[] expectedSecNum = { 14, 15, 15 }; + int[] expectedSecNum = { 14 }; - assertEquals(expectedTissueNames.length, a1Samples.size()); - for (int i = 0; i < expectedTissueNames.length; ++i) { + assertEquals(expectedSecNum.length, a1Samples.size()); + for (int i = 0; i < expectedSecNum.length; ++i) { Map sam = a1Samples.get(i); - assertEquals(expectedTissueNames[i], chainGet(sam, "tissue", "externalName")); + assertEquals("TISSUE1", chainGet(sam, "tissue", "externalName")); assertEquals(expectedSecNum[i], (int) sam.get("section")); assertEquals("Tissue", chainGet(sam, "bioState", "name")); } @@ -189,10 +188,10 @@ private void testConfirm(Sample[] blockSamples, Labware[] sourceBlocks, String[] assertNotNull(chainGet(resultOp, "performed")); assertEquals("Section", chainGet(resultOp, "operationType", "name")); List> actions = chainGet(resultOp, "actions"); - int[] expectedSourceLabwareIds = {sourceBlocks[0].getId(), sourceBlocks[0].getId(), sourceBlocks[1].getId(), sourceBlocks[1].getId()}; - String[] expectedDestAddress = { "A1", "A1", "A1", "B2" }; - String[] expectedActionTissues = { "TISSUE1", "TISSUE1", "TISSUE2", "TISSUE2" }; - int[] expectedActionSecNum = { 14,15,15,17 }; + int[] expectedSourceLabwareIds = {sourceBlocks[0].getId(), sourceBlocks[1].getId()}; + String[] expectedDestAddress = { "A1", "B2" }; + String[] expectedActionTissues = { "TISSUE1", "TISSUE2" }; + int[] expectedActionSecNum = { 14,17 }; assertEquals(expectedSourceLabwareIds.length, actions.size()); int destLabwareId = -1; @@ -251,12 +250,12 @@ private void testConfirm(Sample[] blockSamples, Labware[] sourceBlocks, String[] // Check that the source blocks' highest section numbers have been updated entityManager.refresh(sourceBlocks[0]); entityManager.refresh(sourceBlocks[1]); - assertEquals(15, sourceBlocks[0].getFirstSlot().getBlockHighestSection()); + assertEquals(14, sourceBlocks[0].getFirstSlot().getBlockHighestSection()); assertEquals(17, sourceBlocks[1].getFirstSlot().getBlockHighestSection()); entityManager.flush(); entityManager.refresh(work); assertThat(work.getOperationIds()).hasSize(3); - assertThat(work.getSampleSlotIds()).hasSize(6); + assertThat(work.getSampleSlotIds()).hasSize(4); List notes = lwNoteRepo.findAllByOperationIdIn(opIds); assertThat(notes).hasSize(2); @@ -269,23 +268,11 @@ private void testConfirm(Sample[] blockSamples, Labware[] sourceBlocks, String[] } assertEquals(Map.of("costing", "SGP", "lot", "1234567"), noteMap); - String samplePositionsQuery = String.format("query { samplePositions(labwareBarcode: \"%s\") { address, region, sampleId } }", barcodes[0]); - Object response = tester.post(samplePositionsQuery); - List> samplePositions = chainGet(response, "data", "samplePositions"); - assertThat(samplePositions).containsExactly( - Map.of("address", "A1", "region", "Bottom", "sampleId", sampleIds.get(0)), - Map.of("address", "A1", "region", "Top", "sampleId", sampleIds.get(1)), - Map.of("address", "A1", "region", "Middle", "sampleId", sampleIds.get(2)) - ); - Integer opId = opIds.get(0); final List opcoms = opComRepo.findAllByOperationIdIn(opIds); Integer slotId = opcoms.get(0).getSlotId(); - assertThat(opcoms).hasSize(4); + assertThat(opcoms).hasSize(2); assertOpCom(opcoms.get(0), 2, opId, sampleIds.get(0), slotId); - assertOpCom(opcoms.get(1), 1, opId, sampleIds.get(0), slotId); - assertOpCom(opcoms.get(2), 1, opId, sampleIds.get(1), slotId); - assertOpCom(opcoms.get(3), 1, opId, sampleIds.get(2), slotId); } private static void assertOpCom(OperationComment opcom, Integer commentId, Integer opId, Integer sampleId, Integer slotId) { diff --git a/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/TestRegisterOriginalSamplesMutation.java b/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/TestRegisterOriginalSamplesMutation.java index 46bd1a392..8ebfd3eeb 100644 --- a/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/TestRegisterOriginalSamplesMutation.java +++ b/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/TestRegisterOriginalSamplesMutation.java @@ -146,12 +146,14 @@ private void testSectioningBlock(Labware block, Work work) throws Exception { .replace("BARCODE0", block.getBarcode()) .replace("55555", String.valueOf(blockSampleId)); Object result = tester.post(planMutation); + assertNoErrors(result); String slideBarcode = chainGet(result, "data", "plan", "labware", 0, "barcode"); String confirmMutation = tester.readGraphQL("confirmsection_simple.graphql") .replace("BARCODE0", slideBarcode) .replace("55555", String.valueOf(blockSampleId)) .replace("SGP1", work.getWorkNumber()); result = tester.post(confirmMutation); + assertNoErrors(result); assertNotNull(chainGet(result, "data", "confirmSection", "labware", 0, "barcode")); } diff --git a/src/test/java/uk/ac/sanger/sccp/stan/service/TestSlotGroupService.java b/src/test/java/uk/ac/sanger/sccp/stan/service/TestSlotGroupService.java new file mode 100644 index 000000000..b3804034e --- /dev/null +++ b/src/test/java/uk/ac/sanger/sccp/stan/service/TestSlotGroupService.java @@ -0,0 +1,69 @@ +package uk.ac.sanger.sccp.stan.service; + +import org.junit.jupiter.api.*; +import org.mockito.*; +import uk.ac.sanger.sccp.stan.EntityFactory; +import uk.ac.sanger.sccp.stan.model.*; +import uk.ac.sanger.sccp.stan.repo.SlotGroupRepo; +import uk.ac.sanger.sccp.utils.Zip; + +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static uk.ac.sanger.sccp.stan.Matchers.returnArgument; + +/** Test {@link SlotGroupServiceImp} */ +class TestSlotGroupService { + @Mock + SlotGroupRepo mockSlotGroupRepo; + @InjectMocks + SlotGroupServiceImp service; + + private AutoCloseable mocking; + + @BeforeEach + void setup() { + mocking = MockitoAnnotations.openMocks(this); + } + + @AfterEach + void cleanup() throws Exception { + mocking.close(); + } + + @Test + void testSaveGroups() { + Integer planId = 500; + final Address A1 = new Address(1,1), A2 = new Address(1,2), A3 = new Address(1,3); + Labware lw = EntityFactory.makeEmptyLabware(EntityFactory.makeLabwareType(1,3)); + List> groups = List.of(List.of(A1, A2), List.of(A3)); + when(mockSlotGroupRepo.saveAll(any())).then(returnArgument()); + List records = service.saveGroups(lw, planId, groups); + verify(mockSlotGroupRepo).saveAll(records); + assertThat(records).containsExactly( + new SlotGroup(1, lw.getSlot(A1), planId), + new SlotGroup(1, lw.getSlot(A2), planId), + new SlotGroup(2, lw.getSlot(A3), planId) + ); + } + + @Test + void testLoadGroups() { + Integer planId = 500; + final Address A1 = new Address(1,1), A2 = new Address(1,2), A3 = new Address(1,3); + Labware lw = EntityFactory.makeEmptyLabware(EntityFactory.makeLabwareType(1,3)); + List records = List.of( + new SlotGroup(1, lw.getSlot(A1), planId), + new SlotGroup(1, lw.getSlot(A2), planId), + new SlotGroup(2, lw.getSlot(A3), planId) + ); + Zip.enumerate(records.stream()).forEach((i, r) -> r.setId(i+1)); + when(mockSlotGroupRepo.findByPlanId(planId)).thenReturn(records); + List> groups = service.loadGroups(planId); + verify(mockSlotGroupRepo).findByPlanId(planId); + assertThat(groups).containsExactly(List.of(A1, A2), List.of(A3)); + } +} \ No newline at end of file diff --git a/src/test/java/uk/ac/sanger/sccp/stan/service/operation/confirm/TestConfirmSectionService.java b/src/test/java/uk/ac/sanger/sccp/stan/service/operation/confirm/TestConfirmSectionService.java index 422a5aa73..42c56df6a 100644 --- a/src/test/java/uk/ac/sanger/sccp/stan/service/operation/confirm/TestConfirmSectionService.java +++ b/src/test/java/uk/ac/sanger/sccp/stan/service/operation/confirm/TestConfirmSectionService.java @@ -13,7 +13,7 @@ import uk.ac.sanger.sccp.stan.request.confirm.ConfirmSectionLabware.AddressCommentId; import uk.ac.sanger.sccp.stan.service.*; import uk.ac.sanger.sccp.stan.service.operation.confirm.ConfirmSectionServiceImp.ConfirmLabwareResult; -import uk.ac.sanger.sccp.stan.service.operation.confirm.ConfirmSectionServiceImp.PlanActionKey; +import uk.ac.sanger.sccp.stan.service.operation.confirm.ConfirmSectionServiceImp.SectionKey; import uk.ac.sanger.sccp.stan.service.work.WorkService; import uk.ac.sanger.sccp.utils.BasicUtils; import uk.ac.sanger.sccp.utils.UCMap; @@ -23,7 +23,6 @@ import java.util.stream.IntStream; import java.util.stream.Stream; -import static java.util.stream.Collectors.toList; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.*; @@ -43,7 +42,6 @@ public class TestConfirmSectionService { private SampleRepo mockSampleRepo; private CommentRepo mockCommentRepo; private LabwareNoteRepo mockLwNoteRepo; - private SamplePositionRepo mockSamplePositionRepo; private OperationCommentRepo mockOpCommentRepo; private EntityManager mockEntityManager; @@ -62,11 +60,10 @@ void setup() { mockCommentRepo = mock(CommentRepo.class); mockOpCommentRepo = mock(OperationCommentRepo.class); mockLwNoteRepo = mock(LabwareNoteRepo.class); - mockSamplePositionRepo = mock(SamplePositionRepo.class); mockEntityManager = mock(EntityManager.class); service = spy(new ConfirmSectionServiceImp(mockValidationService, mockBioRiskService, mockOpService, mockWorkService, mockLwRepo, mockSlotRepo, mockMeasurementRepo, mockSampleRepo, mockCommentRepo, mockOpCommentRepo, - mockLwNoteRepo, mockSamplePositionRepo, mockEntityManager)); + mockLwNoteRepo, mockEntityManager)); } private static OperationType makeOpType() { @@ -90,7 +87,7 @@ public void testConfirmOperationValid() { Labware lw = EntityFactory.getTube(); ConfirmSectionRequest request = new ConfirmSectionRequest(List.of(new ConfirmSectionLabware("STAN-01"))); ConfirmSectionValidation validation = new ConfirmSectionValidation(UCMap.from(Labware::getBarcode, lw), - Map.of(), new UCMap<>(), Map.of(), new UCMap<>()); + Map.of(), Map.of(), new UCMap<>()); doReturn(validation).when(mockValidationService).validate(request); OperationResult opResult = new OperationResult(List.of(), List.of(lw)); doReturn(opResult).when(service).perform(user, validation, request); @@ -113,7 +110,7 @@ static Stream performErrorArgs() { Arguments.of(makeValidation(lw), new ConfirmSectionRequest(List.of(new ConfirmSectionLabware(lw.getBarcode()))), "No plan found for labware "+lw.getBarcode()), - Arguments.of(new ConfirmSectionValidation(new UCMap<>(), Map.of(lw.getId(), plan), new UCMap<>(), Map.of(), new UCMap<>()), + Arguments.of(new ConfirmSectionValidation(new UCMap<>(), Map.of(lw.getId(), plan), Map.of(), new UCMap<>()), new ConfirmSectionRequest(List.of(new ConfirmSectionLabware(lw.getBarcode()))), "Invalid labware barcode: "+lw.getBarcode()) ); @@ -121,7 +118,7 @@ static Stream performErrorArgs() { private static ConfirmSectionValidation makeValidation(Labware lw) { UCMap lwMap = UCMap.from(Labware::getBarcode, lw); - return new ConfirmSectionValidation(lwMap, Map.of(), new UCMap<>(0), Map.of(), new UCMap<>(0)); + return new ConfirmSectionValidation(lwMap, Map.of(), Map.of(), new UCMap<>(0)); } private static LabwareNote planNote(Integer id, Integer lwId, Integer planId, String name, String value) { @@ -154,7 +151,6 @@ public void testPerformSuccessful() { planNote(61, lw1.getId(), 10, "note1", "value1"), planNote(62, lw1.getId(), 10, "note2", "value2") ); - UCMap regionMap = UCMap.from(SlotRegion::getName, new SlotRegion(1, "Top")); Map commentMap = Map.of(1, new Comment(1, "com", "cat")); Work work = EntityFactory.makeWork("SGP99"); UCMap works = UCMap.from(Work::getWorkNumber, work); @@ -171,8 +167,8 @@ public void testPerformSuccessful() { ConfirmSectionLabware csl2 = new ConfirmSectionLabware("STAN-02"); csl2.setWorkNumber("SGP99"); - doReturn(clr1).when(service).confirmLabware(user, csl1, lw1, plan1, regionMap, commentMap); - doReturn(clr2).when(service).confirmLabware(user, csl2, lw2, plan2, regionMap, commentMap); + doReturn(clr1).when(service).confirmLabware(user, csl1, lw1, plan1, commentMap); + doReturn(clr2).when(service).confirmLabware(user, csl2, lw2, plan2, commentMap); doNothing().when(service).recordAddressComments(any(), any(), any()); doNothing().when(service).updateSourceBlocks(any()); @@ -180,14 +176,14 @@ public void testPerformSuccessful() { ConfirmSectionRequest request = new ConfirmSectionRequest(List.of(csl1, csl2)); ConfirmSectionValidation validation = new ConfirmSectionValidation(UCMap.from(Labware::getBarcode, lw1, lw2), - Map.of(lw1.getId(), plan1, lw2.getId(), plan2), regionMap, commentMap, works); + Map.of(lw1.getId(), plan1, lw2.getId(), plan2), commentMap, works); OperationResult result = service.perform(user, validation, request); assertThat(result.getLabware()).containsExactly(lw1B, lw2B); assertThat(result.getOperations()).containsExactly(op1); - verify(service).confirmLabware(user, csl1, lw1, plan1, regionMap, commentMap); - verify(service).confirmLabware(user, csl2, lw2, plan2, regionMap, commentMap); + verify(service).confirmLabware(user, csl1, lw1, plan1, commentMap); + verify(service).confirmLabware(user, csl2, lw2, plan2, commentMap); verify(service).recordAddressComments(csl1, op1.getId(), lw1B); verify(service).recordAddressComments(csl2, null, lw2B); verify(mockBioRiskService).copyOpSampleBioRisks(result.getOperations()); @@ -238,8 +234,7 @@ public void testConfirmLabwareCancelled(boolean markedCancelled) { PlanOperation plan = new PlanOperation(); User user = EntityFactory.getUser(); Map commentMap = Map.of(1, new Comment(1, "com", "cat")); - UCMap regionMap = UCMap.from(SlotRegion::getName, new SlotRegion(1, "Top")); - assertEquals(new ConfirmLabwareResult(null, lw), service.confirmLabware(user, csl, lw, plan, regionMap, commentMap)); + assertEquals(new ConfirmLabwareResult(null, lw), service.confirmLabware(user, csl, lw, plan, commentMap)); verify(mockLwRepo).save(lw); assertTrue(lw.isDiscarded()); @@ -257,18 +252,16 @@ public void testConfirmLabwareMissingPlanAction() { Sample sample = EntityFactory.getSample(); lw.setBarcode("STAN-01"); PlanOperation plan = new PlanOperation(); - Map planActionMap = Map.of(); + Map planActionMap = Map.of(); doReturn(planActionMap).when(service).getPlanActionMap(any(), anyInt()); final Address A1 = new Address(1,1); - UCMap regionMap = UCMap.from(SlotRegion::getName, new SlotRegion(1, "Top")); Map commentMap = Map.of(1, new Comment(1, "com", "cat")); ConfirmSectionLabware csl = new ConfirmSectionLabware(lw.getBarcode(), false, List.of(new ConfirmSection(A1, sample.getId(), 12)), null, null); assertThat(assertThrows(IllegalArgumentException.class, - () -> service.confirmLabware(EntityFactory.getUser(), csl, lw, plan, regionMap, commentMap))) - .hasMessage("No plan action found matching section request: sample id "+sample.getId()+ - " in STAN-01 slot A1."); + () -> service.confirmLabware(EntityFactory.getUser(), csl, lw, plan, commentMap))) + .hasMessage("No plan action found into STAN-01 slot A1."); verifyNoInteractions(mockSlotRepo); verifyNoInteractions(mockMeasurementRepo); } @@ -289,43 +282,44 @@ public void testConfirmLabware() { final Address A1 = new Address(1,1); final Address B3 = new Address(2,3); List csecs = List.of( - new ConfirmSection(A1, sample.getId(), 10, List.of(30,31), null), - new ConfirmSection(A1, sample.getId(), 11, null, "top"), - new ConfirmSection(B3, sample.getId(), 12, null, "top") + new ConfirmSection(A1, sample.getId(), 10, List.of(30,31)), + new ConfirmSection(B3, sample.getId(), 12, null) ); csecs.getFirst().setThickness("10.5"); ConfirmSectionLabware csl = new ConfirmSectionLabware(lw1.getBarcode(), false, csecs, List.of(), null); - Map planActionMap = Stream.of( + Map planActionMap = Stream.of( new PlanAction(1, 1, source, lw1.getSlot(A1), sample), new PlanAction(2, 1, source, lw1.getSlot(B3), sample, 12, "50", null) - ).collect(BasicUtils.inMap(PlanActionKey::new, HashMap::new)); + ).collect(BasicUtils.inMap(pa -> pa.getDestination().getAddress(), HashMap::new)); plan.setPlanActions(new ArrayList<>(planActionMap.values())); doReturn(planActionMap).when(service).getPlanActionMap(any(), anyInt()); final List sections = new ArrayList<>(3); - List actions = csecs.stream().map(csec -> { + List actions = new ArrayList<>(); + for (ConfirmSection csec : csecs) { Sample section = new Sample(sample.getId()+1+sections.size(), csec.getNewSection(), sample.getTissue(), sample.getBioState()); sections.add(section); - PlanAction pa = planActionMap.get(new PlanActionKey(csec.getDestinationAddress(), csec.getSampleId())); - Action action = new Action(null, null, pa.getSource(), pa.getDestination(), section, sample); - doReturn(action).when(service).makeAction(csec, pa, pa.getDestination()); - return action; - }).collect(toList()); - + doReturn(section).when(service).createSection(eq(sample.getTissue()), eq(csec.getNewSection()), eq(sample.getBioState())); + for (Address ad : csec.getDestinationAddresses()) { + sections.add(section); + PlanAction pa = planActionMap.get(ad); + Action action = new Action(null, null, pa.getSource(), pa.getDestination(), section, sample); + doReturn(action).when(service).makeAction(any(), same(pa), same(pa.getDestination())); + actions.add(action); + } + } when(mockSlotRepo.saveAll(any())).then(Matchers.returnArgument()); Operation op = new Operation(); op.setId(66); when(mockOpService.createOperation(any(), any(), any(), any())).thenReturn(op); when(mockMeasurementRepo.saveAll(any())).then(Matchers.returnArgument()); - final SlotRegion top = new SlotRegion(1, "Top"); - UCMap regionMap = UCMap.from(SlotRegion::getName, top); final Comment com0 = new Comment(30, "com0", "cat"); final Comment com1 = new Comment(31, "com1", "cat1"); Map commentMap = Map.of(30, com0, 31, com1); User user = EntityFactory.getUser(); - ConfirmLabwareResult result = service.confirmLabware(user, csl, lw1, plan, regionMap, commentMap); + ConfirmLabwareResult result = service.confirmLabware(user, csl, lw1, plan, commentMap); assertSame(lw1, result.labware()); assertSame(op, result.operation()); @@ -335,7 +329,9 @@ public void testConfirmLabware() { verify(service).getPlanActionMap(plan.getPlanActions(), lw1.getId()); for (ConfirmSection csec : csecs) { - verify(service).makeAction(csec, planActionMap.get(new PlanActionKey(csec.getDestinationAddress(), csec.getSampleId())), lw1.getSlot(csec.getDestinationAddress())); + for (Address ad : csec.getDestinationAddresses()) { + verify(service).makeAction(any(), eq(planActionMap.get(ad)), eq(lw1.getSlot(ad))); + } } verify(mockSlotRepo).saveAll(Matchers.sameElements(List.of(lw1.getSlot(A1), lw1.getSlot(B3)), true)); verify(mockEntityManager).refresh(lw1); @@ -345,12 +341,12 @@ public void testConfirmLabware() { new Measurement(null, "Thickness", "10.5", sections.get(0).getId(), opId, lw1.getSlot(A1).getId()), new Measurement(null, "Thickness", "50", sections.get(2).getId(), opId, lw1.getSlot(B3).getId())) ); - verify(mockSamplePositionRepo).saveAll(List.of(new SamplePosition(lw1.getFirstSlot().getId(), sections.get(1).getId(), top, opId), - new SamplePosition(lw1.getSlot(B3).getId(), sections.get(2).getId(), top, opId))); verify(mockOpCommentRepo).saveAll(List.of(new OperationComment(null, com0, opId, sections.get(0).getId(), lw1.getFirstSlot().getId(), null), new OperationComment(null, com1, opId, sections.get(0).getId(), lw1.getFirstSlot().getId(), null))); for (ConfirmSection csec : csecs) { - verify(service).thickness(csec, planActionMap.get(new PlanActionKey(csec.getDestinationAddress(), csec.getSampleId()))); + for (Address ad : csec.getDestinationAddresses()) { + verify(service).thickness(csec, planActionMap.get(ad)); + } } } @@ -377,12 +373,8 @@ public void testMakeAction() { Labware lw1 = EntityFactory.makeEmptyLabware(EntityFactory.getTubeType()); Slot dest = lw1.getFirstSlot(); PlanAction pa = new PlanAction(3, 1, source, dest, sourceSample); - ConfirmSection csec = new ConfirmSection(new Address(1,1), sourceSample.getId(), section.getSection()); - doReturn(section).when(service).getSection(csec, pa, dest); - Action action = service.makeAction(csec, pa, dest); - - verify(service).getSection(csec, pa, dest); + Action action = service.makeAction(section, pa, dest); assertNull(action.getId()); assertNull(action.getOperationId()); assertSame(source, action.getSource()); @@ -391,37 +383,57 @@ public void testMakeAction() { assertSame(sourceSample, action.getSourceSample()); } - @Test - public void testGetSection() { - Tissue tissue = EntityFactory.getTissue(); - final BioState bs = EntityFactory.getBioState(); - Sample sourceSample = new Sample(100, null, tissue, bs); - Labware lw0 = EntityFactory.makeLabware(EntityFactory.getTubeType(), sourceSample); - Slot source = lw0.getFirstSlot(); - Labware lw1 = EntityFactory.makeEmptyLabware(EntityFactory.getTubeType()); - Slot dest = lw1.getFirstSlot(); - PlanAction pa = new PlanAction(3, 1, source, dest, sourceSample); - final Integer secNum = 14; - ConfirmSection csec = new ConfirmSection(new Address(1,1), sourceSample.getId(), secNum); - Sample otherSection = new Sample(101, 13, tissue, bs); - Sample newSection = new Sample(102, secNum, tissue, bs); - dest.getSamples().add(otherSection); - doReturn(newSection).when(service).createSection(sourceSample, secNum, bs); - - assertSame(newSection, service.getSection(csec, pa, dest)); - verify(service).createSection(sourceSample, secNum, bs); + @ParameterizedTest + @MethodSource("getSectionArgs") + void testGetSection(Map sectionMap, Integer sectionNum, + BioState bs0, Slot sourceSlot, Sample sourceSample, Slot destSlot, + Sample expectedSample, boolean create) { + sectionMap = new HashMap<>(sectionMap); + ConfirmSection csec = new ConfirmSection((Address) null, null, sectionNum, null); + PlanAction pa = new PlanAction(1, 1, sourceSlot, destSlot, sourceSample); + pa.setNewBioState(bs0); + if (create) { + doReturn(expectedSample).when(service).createSection(any(), any(), any()); + } + assertSame(expectedSample, service.getSection(sectionMap, csec, pa, destSlot)); + if (create) { + verify(service).createSection(pa.getSample().getTissue(), sectionNum, expectedSample.getBioState()); + } else { + verify(service, never()).createSection(any(), any(), any()); + } + assertSame(expectedSample, sectionMap.get(new SectionKey(sourceSample.getTissue().getId(), sectionNum, expectedSample.getBioState()))); + } + + static Stream getSectionArgs() { + BioState bs = EntityFactory.getBioState(); + BioState bs2 = new BioState(bs.getId()+1, "bs2"); + Sample[] sams = EntityFactory.makeSamples(3); + Sample sourceSample = sams[0]; + sourceSample.setBioState(bs2); + sams[2].setBioState(bs2); + Slot sourceSlot = EntityFactory.makeLabware(EntityFactory.getTubeType(), sourceSample).getFirstSlot(); + Slot destSlot = EntityFactory.makeEmptyLabware(EntityFactory.getTubeType()).getFirstSlot(); + Slot destSlot2 = EntityFactory.makeLabware(EntityFactory.getTubeType(), sams[2]).getFirstSlot(); + Map sectionMap = Map.of( + new SectionKey(sams[0].getTissue().getId(), 1, bs), sams[1] + ); + return Arrays.stream(new Object[][] { + { sectionMap, 1, bs, sourceSlot, sourceSample, destSlot, sams[1], false}, + { sectionMap, 1, null, sourceSlot, sourceSample, destSlot, sams[2], true}, + { sectionMap, sams[2].getSection(), null, sourceSlot, sourceSample, destSlot2, sams[2], false}, + }).map(Arguments::of); } @Test public void testCreateSection() { - Sample sourceSample = new Sample(100, null, EntityFactory.getTissue(), EntityFactory.getBioState()); + Tissue tissue = EntityFactory.getTissue(); BioState bs = new BioState(50, "Bananas"); Integer secNum = 7; - Sample createdSample = new Sample(101, secNum, sourceSample.getTissue(), bs); + Sample createdSample = new Sample(101, secNum, tissue, bs); when(mockSampleRepo.save(any())).thenReturn(createdSample); - assertSame(createdSample, service.createSection(sourceSample, secNum, bs)); - verify(mockSampleRepo).save(new Sample(null, secNum, sourceSample.getTissue(), bs)); + assertSame(createdSample, service.createSection(tissue, secNum, bs)); + verify(mockSampleRepo).save(new Sample(null, secNum, tissue, bs)); } @Test @@ -511,16 +523,14 @@ public void testGetPlanActionMap() { }; PlanAction[] pas = { new PlanAction(51, 1, source, slots[0], samples[0]), - new PlanAction(52, 1, source, slots[0], samples[1]), - new PlanAction(53, 1, source, slots[1], samples[0]), - new PlanAction(54, 1, source, slots[2], samples[1]), + new PlanAction(53, 1, source, slots[1], samples[1]), + new PlanAction(54, 1, source, slots[2], samples[0]), }; - Map map = service.getPlanActionMap(Arrays.asList(pas), 10); - assertEquals(3, map.size()); - assertEquals(pas[0], map.get(new PlanActionKey(A1, samples[0].getId()))); - assertEquals(pas[1], map.get(new PlanActionKey(A1, samples[1].getId()))); - assertEquals(pas[2], map.get(new PlanActionKey(A2, samples[0].getId()))); + Map map = service.getPlanActionMap(Arrays.asList(pas), 10); + assertEquals(2, map.size()); + assertEquals(pas[0], map.get(A1)); + assertEquals(pas[1], map.get(A2)); } /** diff --git a/src/test/java/uk/ac/sanger/sccp/stan/service/operation/confirm/TestConfirmSectionValidationService.java b/src/test/java/uk/ac/sanger/sccp/stan/service/operation/confirm/TestConfirmSectionValidationService.java index 8a3ca8837..02acb8159 100644 --- a/src/test/java/uk/ac/sanger/sccp/stan/service/operation/confirm/TestConfirmSectionValidationService.java +++ b/src/test/java/uk/ac/sanger/sccp/stan/service/operation/confirm/TestConfirmSectionValidationService.java @@ -12,7 +12,6 @@ import uk.ac.sanger.sccp.stan.request.confirm.*; import uk.ac.sanger.sccp.stan.request.confirm.ConfirmSectionLabware.AddressCommentId; import uk.ac.sanger.sccp.stan.service.CommentValidationService; -import uk.ac.sanger.sccp.stan.service.SlotRegionService; import uk.ac.sanger.sccp.stan.service.sanitiser.Sanitiser; import uk.ac.sanger.sccp.stan.service.work.WorkService; import uk.ac.sanger.sccp.utils.UCMap; @@ -20,7 +19,6 @@ import java.util.*; import java.util.function.Function; -import java.util.stream.IntStream; import java.util.stream.Stream; import static java.util.stream.Collectors.toList; @@ -46,8 +44,6 @@ public class TestConfirmSectionValidationService { @Mock WorkService mockWorkService; @Mock - SlotRegionService mockSlotRegionService; - @Mock CommentValidationService mockCommentValidationService; @Mock Sanitiser mockThicknessSanitiser; @@ -59,7 +55,7 @@ public class TestConfirmSectionValidationService { void setup() { mocking = MockitoAnnotations.openMocks(this); service = spy(new ConfirmSectionValidationServiceImp(mockLwRepo, mockPlanRepo, mockWorkService, - mockSlotRegionService, mockCommentValidationService, mockThicknessSanitiser)); + mockCommentValidationService, mockThicknessSanitiser)); opType = new OperationType(2, "Section", OperationTypeFlag.SOURCE_IS_BLOCK.bit(), EntityFactory.getBioState()); } @@ -90,15 +86,13 @@ public void testValidate(boolean valid) { PlanOperation plan = EntityFactory.makePlanForLabware(opType, List.of(), List.of()); Map planMap = Map.of(lw.getId(), plan); - UCMap regionMap = UCMap.from(SlotRegion::getName, new SlotRegion(1, "Top")); Map commentMap = Map.of(1, new Comment(1, "com", "cat")); UCMap works = UCMap.from(Work::getWorkNumber, EntityFactory.makeWork("SGP1")); mayAddProblem(valid ? null : "lw problem", lwMap).when(service).validateLabware(any(), any()); mayAddProblem(valid ? null : "plan problem", planMap).when(service).lookUpPlans(any(), any()); mayAddProblem(valid ? null : "op problem").when(service).validateOperations(any(), any(), any(), any()); - mayAddProblem(valid ? null : "region problem", regionMap).when(service).validateSlotRegions(any(), any()); - mayAddProblem(valid ? null : "missing region").when(service).requireRegionsForMultiSampleSlots(any(), any()); + mayAddProblem(valid ? null : "dupe address").when(service).checkRepeatedDestSlots(any(), any()); mayAddProblem(valid ? null : "comment problem", commentMap).when(service).validateCommentIds(any(), any()); mayAddProblem(valid ? null : "work problem", works).when(mockWorkService).validateUsableWorks(any(), any()); mayAddProblem(valid ? null : "thickness problem").when(service).sanitiseThickness(any(), any()); @@ -109,18 +103,16 @@ public void testValidate(boolean valid) { assertEquals(validation.getLwPlans(), planMap); assertEquals(validation.getLabware(), lwMap); assertEquals(validation.getComments(), commentMap); - assertEquals(validation.getSlotRegions(), regionMap); assertEquals(validation.getWorks(), works); } else { assertThat(validation.getProblems()).containsExactlyInAnyOrder( - "lw problem", "plan problem", "op problem", "region problem", "missing region", + "lw problem", "plan problem", "op problem", "dupe address", "comment problem", "work problem", "thickness problem" ); } verify(service).validateCommentIds(any(), eq(request.getLabware())); - verify(service).validateSlotRegions(any(), eq(request.getLabware())); - verify(service).requireRegionsForMultiSampleSlots(any(), eq(request.getLabware())); + verify(service).checkRepeatedDestSlots(any(), eq(request.getLabware())); verify(mockWorkService).validateUsableWorks(any(), eq(Set.of("SGP1"))); verify(service).validateLabware(any(), eq(request.getLabware())); verify(service).sanitiseThickness(any(), eq(request.getLabware())); @@ -128,6 +120,25 @@ public void testValidate(boolean valid) { verify(service).validateOperations(any(), eq(request.getLabware()), eq(lwMap), eq(planMap)); } + @Test + public void testCheckRepeatedDestSlots() { + final Address A1 = new Address(1,1), A2 = new Address(1,2), A3 = new Address(1,3); + List csls = List.of( + new ConfirmSectionLabware(null), + new ConfirmSectionLabware("STAN-1", false, List.of( + new ConfirmSection(List.of(A1, A2), null, null, null), + new ConfirmSection(A3, null, null, null) + ), null, null), + new ConfirmSectionLabware("STAN-2", false, List.of( + new ConfirmSection(List.of(A1, A2), null, null, null), + new ConfirmSection(List.of(A2, A3), null, null, null) + ), null, null) + ); + List problems = new ArrayList<>(1); + service.checkRepeatedDestSlots(problems, csls); + assertProblem(problems, "Multiple actions linked to destination STAN-2 A2."); + } + @ParameterizedTest @ValueSource(booleans={false,true}) public void testValidateCommentIds(boolean valid) { @@ -159,83 +170,6 @@ public void testValidateCommentIds(boolean valid) { assertThat(problems).containsExactlyInAnyOrderElementsOf(expectedProblems); } - @ParameterizedTest - @MethodSource("validateRegionsArgs") - public void testValidateRegions(List allRegions, List expectedRegions, - List csls, List expectedProblems) { - List problems = new ArrayList<>(expectedProblems.size()); - when(mockSlotRegionService.loadSlotRegionMap(true)).thenReturn(UCMap.from(allRegions, SlotRegion::getName)); - assertEquals(UCMap.from(expectedRegions, SlotRegion::getName), service.validateSlotRegions(problems, csls)); - assertThat(problems).containsExactlyInAnyOrderElementsOf(expectedProblems); - } - - static Stream validateRegionsArgs() { - final Address A1 = new Address(1,1); - final Address A2 = new Address(1,2); - List allRegions = List.of(new SlotRegion(1, "Top"), - new SlotRegion(2, "Bottom"), new SlotRegion(3, "Middle")); - ConfirmSectionLabware cslNoRegion = new ConfirmSectionLabware("STAN-1", false, - List.of(confirmSection(A1, null), confirmSection(A1, "")), List.of(), null); - ConfirmSectionLabware cslNoBarcode = new ConfirmSectionLabware("", false, List.of(), List.of(), null); - ConfirmSectionLabware cslRegions = new ConfirmSectionLabware("STAN-2", false, - List.of(confirmSection(A1, "Top"), confirmSection(A1, "Bottom"), - confirmSection(A2, "Top")), List.of(), null); - ConfirmSectionLabware cslUnknownRegion = new ConfirmSectionLabware("STAN-3", false, - List.of(confirmSection(A1, "Spoon")), List.of(), null); - ConfirmSectionLabware cslRepeatedRegions = new ConfirmSectionLabware("STAN-4", false, - List.of(confirmSection(A1, "Top"), confirmSection(A1, "top")), List.of(), null); - return Arrays.stream(new Object[][] { - {allRegions, List.of(), List.of(cslNoRegion, cslNoBarcode), List.of()}, - {allRegions, allRegions, List.of(cslNoRegion, cslRegions), List.of()}, - {allRegions, allRegions, List.of(cslRegions, cslUnknownRegion), List.of("Unknown region: \"Spoon\"")}, - {allRegions, allRegions, List.of(cslRepeatedRegions), List.of("Region Top specified twice for A1 in STAN-4.")}, - }).map(Arguments::of); - } - - @Test - public void testRequireRegionsForMultiSampleSlots() { - List problems = new ArrayList<>(); - final Address A1 = new Address(1,1); - final Address A2 = new Address(1,2); - final Address A3 = new Address(1,3); - final Address A4 = new Address(1,4); - ConfirmSectionLabware[] csls = IntStream.range(0, 3) - .mapToObj(i -> new ConfirmSectionLabware("STAN-"+i)) - .toArray(ConfirmSectionLabware[]::new); - csls[0].setBarcode(""); // empty barcode csl is skipped - csls[1].setConfirmSections( - List.of(makeConfirmSection(A1, null), - makeConfirmSection(A2, "Top"), - makeConfirmSection(A3, "Top"), - makeConfirmSection(A4, "Bottom") - ) - ); - csls[2].setConfirmSections( - List.of(makeConfirmSection(A1, null), - makeConfirmSection(A1, "Top"), - makeConfirmSection(A2, ""), - makeConfirmSection(A3, "Top"), - makeConfirmSection(A3, "Bottom"), - makeConfirmSection(A4, null), - makeConfirmSection(A4, null)) - ); - service.requireRegionsForMultiSampleSlots(problems, Arrays.asList(csls)); - assertThat(problems).containsExactlyInAnyOrder( - "A region must be specified for each section in slot A1 of STAN-2.", - "A region must be specified for each section in slot A4 of STAN-2." - ); - } - - private static ConfirmSection makeConfirmSection(Address address, String regionName) { - return new ConfirmSection(address, 1, 2, null, regionName); - } - - private static ConfirmSection confirmSection(Address address, String regionName) { - ConfirmSection cs = new ConfirmSection(address, 1, 1); - cs.setRegion(regionName); - return cs; - } - @MethodSource("validateLabwareArgs") @ParameterizedTest public void testValidateLabware(Object bcObj, Object lwObj, Object problemObj) { diff --git a/src/test/java/uk/ac/sanger/sccp/stan/service/operation/plan/TestPlanService.java b/src/test/java/uk/ac/sanger/sccp/stan/service/operation/plan/TestPlanService.java index 81a28eef1..6e967e3fd 100644 --- a/src/test/java/uk/ac/sanger/sccp/stan/service/operation/plan/TestPlanService.java +++ b/src/test/java/uk/ac/sanger/sccp/stan/service/operation/plan/TestPlanService.java @@ -11,10 +11,10 @@ import uk.ac.sanger.sccp.stan.request.LabwareFlagged; import uk.ac.sanger.sccp.stan.request.PlanData; import uk.ac.sanger.sccp.stan.request.plan.*; -import uk.ac.sanger.sccp.stan.service.LabwareService; -import uk.ac.sanger.sccp.stan.service.ValidationException; +import uk.ac.sanger.sccp.stan.service.*; import uk.ac.sanger.sccp.stan.service.flag.FlagLookupService; import uk.ac.sanger.sccp.utils.UCMap; +import uk.ac.sanger.sccp.utils.Zip; import java.util.*; import java.util.stream.IntStream; @@ -36,6 +36,7 @@ public class TestPlanService { private PlanValidation mockPlanValidation; private LabwareService mockLwService; private FlagLookupService mockFlagLookupService; + private SlotGroupService mockSlotGroupService; private PlanOperationRepo mockPlanRepo; private PlanActionRepo mockPlanActionRepo; @@ -53,6 +54,7 @@ void setup() { mockPlanValidation = mock(PlanValidation.class); mockLwService = mock(LabwareService.class); mockFlagLookupService = mock(FlagLookupService.class); + mockSlotGroupService = mock(SlotGroupService.class); mockPlanRepo = mock(PlanOperationRepo.class); mockPlanActionRepo = mock(PlanActionRepo.class); mockOpTypeRepo = mock(OperationTypeRepo.class); @@ -65,7 +67,8 @@ void setup() { when(mockPlanValidationFactory.createPlanValidation(any())).thenReturn(mockPlanValidation); - planService = spy(new PlanServiceImp(mockPlanValidationFactory, mockLwService, mockFlagLookupService, + planService = spy(new PlanServiceImp(mockPlanValidationFactory, + mockLwService, mockFlagLookupService, mockSlotGroupService, mockPlanRepo, mockPlanActionRepo, mockOpTypeRepo, mockLwRepo, mockLtRepo, mockLwNoteRepo, mockBsRepo)); } @@ -82,7 +85,7 @@ public void testRecordInvalidPlan() { planService.recordPlan(user, request); fail("Expected ValidationException"); } catch (ValidationException ve) { - assertEquals(ve.getMessage(), "The plan request could not be validated."); + assertEquals("The plan request could not be validated.", ve.getMessage()); // noinspection unchecked,rawtypes assertThat(ve.getProblems()).hasSameElementsAs((Iterable) problems); } @@ -141,6 +144,7 @@ public void testExecutePlanRequest_noFetalWaste(boolean useOpBs) { prlws.get(0).setCosting(SlideCosting.SGP); prlws.get(0).setLotNumber("lot1"); prlws.get(1).setCosting(SlideCosting.Faculty); + doNothing().when(planService).createSlotGroups(anyInt(), any(), any()); final PlanRequest request = new PlanRequest("Section", prlws); PlanResult result = planService.executePlanRequest(user, request); @@ -154,8 +158,10 @@ public void testExecutePlanRequest_noFetalWaste(boolean useOpBs) { verify(planService).lookUpSources(request); verify(planService).createDestinations(request); verify(planService, times(request.getLabware().size())).createActions(any(), anyInt(), same(sources), any(), any()); - verify(planService).createActions(request.getLabware().get(0), plans[0].getId(), sources, destinations.get(0), opBs); - verify(planService).createActions(request.getLabware().get(1), plans[1].getId(), sources, destinations.get(1), opBs); + for (int i = 0; i < plans.length; i++) { + verify(planService).createActions(request.getLabware().get(i), plans[i].getId(), sources, destinations.get(i), opBs); + verify(planService).createSlotGroups(plans[i].getId(), destinations.get(i), request.getLabware().get(i)); + } verify(mockLwNoteRepo).saveAll(List.of( LabwareNote.noteForPlan(destinations.get(0).getId(), plans[0].getId(), "costing", "SGP"), LabwareNote.noteForPlan(destinations.get(0).getId(), plans[0].getId(), "lot", "LOT1"), @@ -310,7 +316,7 @@ public void testCreateActions(boolean useNewBioState) { UCMap sourceMap = UCMap.from(sources, Labware::getBarcode); Labware destination = EntityFactory.makeEmptyLabware(lt); int planId = 99; - PlanRequestLabware prl = new PlanRequestLabware(lt.getName(), destination.getBarcode(), + PlanRequestLabware prl = new PlanRequestLabware(lt.getName(), destination.getBarcode(), List.of( new PlanRequestAction(A1, samples.get(0).getId(), new PlanRequestSource(sourceBarcodes.get(0), A1), null), @@ -365,7 +371,7 @@ public void testGetPlanData(int numPlansFound) { List lfSources = sources.stream().map(x -> new LabwareFlagged(x, null)).toList(); LabwareFlagged lfDest = new LabwareFlagged(lw, null); - assertEquals(new PlanData(plan, lfSources, lfDest), planService.getPlanData(barcode, false)); + assertEquals(new PlanData(plan, lfSources, lfDest, null), planService.getPlanData(barcode, false)); verify(planService).getSources(plan); } @@ -398,7 +404,7 @@ public void testGetPlanData_flags(boolean flagged) { return lws.stream().map(x -> new LabwareFlagged(x, priority)).toList(); }); - assertEquals(new PlanData(plan, lfSources, lfDest), planService.getPlanData(barcode, true)); + assertEquals(new PlanData(plan, lfSources, lfDest, null), planService.getPlanData(barcode, true)); verify(planService).getSources(plan); } @@ -453,4 +459,20 @@ public void testGetSources() { assertEquals(lwList, planService.getSources(plan)); verify(mockLwRepo).findAllByIdIn(Set.of(labware[0].getId(), labware[1].getId())); } + + @Test + public void testCreateSlotGroups() { + int planId = 100; + Labware lw = EntityFactory.getTube(); + List> addresses = List.of( + List.of(new Address(1,1), new Address(1,2)), + List.of(new Address(2,1), new Address(2,2)) + ); + List pras = List.of(new PlanRequestAction(), new PlanRequestAction()); + Zip.of(pras.stream(), addresses.stream()).forEach(PlanRequestAction::setAddresses); + PlanRequestLabware prl = new PlanRequestLabware(); + prl.setActions(pras); + planService.createSlotGroups(planId, lw, prl); + verify(mockSlotGroupService).saveGroups(lw, planId, addresses); + } } diff --git a/src/test/java/uk/ac/sanger/sccp/stan/service/operation/plan/TestPlanValidation.java b/src/test/java/uk/ac/sanger/sccp/stan/service/operation/plan/TestPlanValidation.java index fb2b18baf..525051969 100644 --- a/src/test/java/uk/ac/sanger/sccp/stan/service/operation/plan/TestPlanValidation.java +++ b/src/test/java/uk/ac/sanger/sccp/stan/service/operation/plan/TestPlanValidation.java @@ -447,48 +447,42 @@ static Stream sourcesData() { /** @see #testCheckActions */ static Stream actionsData() { LabwareType lt = EntityFactory.makeLabwareType(1,2); - final Address FIRST = new Address(1,1); - final Address SECOND = new Address(1,2); - PlanRequestSource src = new PlanRequestSource("STAN-000", FIRST); - PlanRequestSource srcAlt = new PlanRequestSource("stan-100", FIRST); - PlanRequestSource src2 = new PlanRequestSource("STAN-001", FIRST); - PlanRequestSource src3 = new PlanRequestSource("STAN-000", SECOND); + final Address A1 = new Address(1,1); + final Address A2 = new Address(1,2); + PlanRequestSource src = new PlanRequestSource("STAN-000", A1); + PlanRequestSource srcAlt = new PlanRequestSource("stan-100", A1); return Stream.of( - Arguments.of("STAN-100", List.of(new PlanRequestAction(FIRST, 4, src, null), - new PlanRequestAction(FIRST, 5, src, null), - new PlanRequestAction(SECOND, 4, src, null), - new PlanRequestAction(FIRST, 4, src2, null), - new PlanRequestAction(FIRST, 4, src3, null)), + Arguments.of("STAN-100", List.of( + new PlanRequestAction(A1, 5, src, null), + new PlanRequestAction(A2, 4, src, null)), lt, null), - Arguments.of(null, List.of(new PlanRequestAction(FIRST, 4, src, null), - new PlanRequestAction(FIRST, 5, src, null), - new PlanRequestAction(SECOND, 4, src, null), - new PlanRequestAction(FIRST, 4, src2, null), - new PlanRequestAction(FIRST, 4, src3, null)), + Arguments.of(null, List.of( + new PlanRequestAction(A1, 4, src, null), + new PlanRequestAction(A2, 4, src, null)), lt, null), Arguments.of("STAN-100", List.of(), lt, "No actions specified for labware STAN-100."), Arguments.of(null, List.of(), lt, "No actions specified for labware of type "+lt.getName()+"."), Arguments.of(null, List.of(), null, "No actions specified for labware of unspecified type."), //Duplicate actions - Arguments.of("STAN-100", List.of(new PlanRequestAction(FIRST, 4, src, null), - new PlanRequestAction(FIRST, 4, src, null), - new PlanRequestAction(SECOND, 4, srcAlt, null)), - lt, "Actions for labware STAN-100 contain duplicate action: (address=A1, sampleId=4, source={STAN-000, A1})"), + Arguments.of("STAN-100", List.of(new PlanRequestAction(A1, 4, src, null), + new PlanRequestAction(A1, 4, src, null), + new PlanRequestAction(A2, 4, srcAlt, null)), + lt, "Actions for labware STAN-100 contains duplicate address: A1"), //Duplicate actions from a non-block source without a barcode - Arguments.of(null, List.of(new PlanRequestAction(FIRST, 4, src, null), - new PlanRequestAction(FIRST, 4, src, null), - new PlanRequestAction(SECOND, 4, src, null)), - lt, "Actions for labware of type "+lt.getName()+" contain duplicate action: (address=A1, sampleId=4, source={STAN-000, A1})"), + Arguments.of(null, List.of(new PlanRequestAction(A1, 4, src, null), + new PlanRequestAction(A1, 4, src, null), + new PlanRequestAction(A2, 4, src, null)), + lt, "Actions for labware of type "+lt.getName()+" contains duplicate address: A1"), Arguments.of("STAN-100", new PlanRequestAction(null, 4, src, null), lt, "Missing destination address."), Arguments.of(null, new PlanRequestAction(null, 4, src, null), lt, "Missing destination address."), Arguments.of("STAN-100", new PlanRequestAction(new Address(2,4), 4, src, null), lt, - "Invalid address B4 given for labware type "+lt.getName()+"."), + "Invalid address given for labware type "+lt.getName()+": [B4]"), Arguments.of(null, new PlanRequestAction(new Address(4,7), 4, src, null), lt, - "Invalid address D7 given for labware type "+lt.getName()+".") + "Invalid address given for labware type "+lt.getName()+": [D7]") ); } diff --git a/src/test/resources/graphql/confirmsection.graphql b/src/test/resources/graphql/confirmsection.graphql index 678f3dc42..dab3d60a5 100644 --- a/src/test/resources/graphql/confirmsection.graphql +++ b/src/test/resources/graphql/confirmsection.graphql @@ -6,26 +6,13 @@ mutation { workNumber: "SGP4000" confirmSections: [ { - destinationAddress: "A1" + destinationAddresses: ["A1"] newSection: 14 sampleId: 55555 commentIds: [2] - region: "Bottom" } { - destinationAddress: "A1" - newSection: 15 - sampleId: 55555 - region: "Top" - } - { - destinationAddress: "A1" - newSection: 15 - sampleId: 55556 - region: "Middle" - } - { - destinationAddress: "B2" + destinationAddresses: ["B2"] newSection: 17 sampleId: 55556 } @@ -42,7 +29,7 @@ mutation { workNumber: "SGP4000" confirmSections: [ { - destinationAddress: "A1" + destinationAddresses: ["A1"] sampleId: 55555 } ] @@ -52,7 +39,7 @@ mutation { workNumber: "SGP4000" confirmSections: [ { - destinationAddress: "A1" + destinationAddresses: ["A1"] sampleId: 55556 } ] diff --git a/src/test/resources/graphql/confirmsection_simple.graphql b/src/test/resources/graphql/confirmsection_simple.graphql index 70456e04b..f92ac6eec 100644 --- a/src/test/resources/graphql/confirmsection_simple.graphql +++ b/src/test/resources/graphql/confirmsection_simple.graphql @@ -6,7 +6,7 @@ mutation { workNumber: "SGP1" confirmSections: [ { - destinationAddress: "A1" + destinationAddresses: ["A1"] newSection: 14 sampleId: 55555 } diff --git a/src/test/resources/graphql/plan.graphql b/src/test/resources/graphql/plan.graphql index 14942d13a..28d3b1e8b 100644 --- a/src/test/resources/graphql/plan.graphql +++ b/src/test/resources/graphql/plan.graphql @@ -8,22 +8,12 @@ mutation { actions:[ { source:{ barcode:"STAN-B70C" } - address: "A1" + addresses: ["A1"] sampleId: 55555 } { source: { barcode: "STAN-B70D" } - address: "A1" - sampleId: 55556 - } - { - source: { barcode: "STAN-B70C" } - address: "B1" - sampleId: 55555 - } - { - source: { barcode: "STAN-B70D" } - address: "B2" + addresses: ["B2"] sampleId: 55556 sampleThickness: "2.5" } @@ -33,7 +23,7 @@ mutation { actions:[ { source:{ barcode:"STAN-B70C" } - address: "A1" + addresses: ["A1"] sampleId: 55555 } ] @@ -42,7 +32,7 @@ mutation { actions:[ { source:{ barcode:"STAN-B70D" } - address: "A1" + addresses: ["A1"] sampleId: 55556 } ] diff --git a/src/test/resources/graphql/plan_simple.graphql b/src/test/resources/graphql/plan_simple.graphql index 02c6dbe1d..a325edf77 100644 --- a/src/test/resources/graphql/plan_simple.graphql +++ b/src/test/resources/graphql/plan_simple.graphql @@ -6,7 +6,7 @@ mutation { actions:[ { source:{ barcode:"BARCODE0" } - address: "A1" + addresses: ["A1"] sampleId: 55555 } ] diff --git a/src/test/resources/graphql/plandata.graphql b/src/test/resources/graphql/plandata.graphql index 707ef24bc..0aead596b 100644 --- a/src/test/resources/graphql/plandata.graphql +++ b/src/test/resources/graphql/plandata.graphql @@ -16,5 +16,6 @@ query { sources { barcode } + groups } } \ No newline at end of file From 1545755f8b20cbc694c32a452fe35d8f62315a81 Mon Sep 17 00:00:00 2001 From: David Robinson <14000840+khelwood@users.noreply.github.com> Date: Fri, 28 Nov 2025 15:04:29 +0000 Subject: [PATCH 2/3] x1290 Update sectioning integration test to use a slot group --- .../TestPlanAndRecordSectionMutations.java | 40 ++++++++++++++----- .../resources/graphql/confirmsection.graphql | 2 +- src/test/resources/graphql/plan.graphql | 2 +- 3 files changed, 33 insertions(+), 11 deletions(-) diff --git a/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/TestPlanAndRecordSectionMutations.java b/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/TestPlanAndRecordSectionMutations.java index 571b6e31b..f3877218a 100644 --- a/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/TestPlanAndRecordSectionMutations.java +++ b/src/test/java/uk/ac/sanger/sccp/stan/integrationtest/TestPlanAndRecordSectionMutations.java @@ -86,8 +86,8 @@ public void testPlanAndRecordSection() throws Exception { } List> resultActions = chainGet(resultOps, 0, "planActions"); - String[] expectedPlanDestAddresses = { "A1", "B2" }; - int[] expectedPlanSampleId = {blockSamples[0].getId(), blockSamples[1].getId()}; + String[] expectedPlanDestAddresses = { "A1", "A2", "B2" }; + int[] expectedPlanSampleId = {blockSamples[0].getId(), blockSamples[0].getId(), blockSamples[1].getId()}; assertEquals(expectedPlanDestAddresses.length, resultActions.size()); for (int i = 0; i < expectedPlanDestAddresses.length; ++i) { @@ -118,7 +118,8 @@ private void testRetrievePlanData(String[] barcodes) throws Exception { .map(m -> m.get("barcode")) .collect(toList())).containsExactlyInAnyOrder(sources); List> planActionsData = chainGet(planData, "plan", "planActions"); - assertThat(planActionsData).hasSize(2); + assertThat(planActionsData).hasSize(3); + assertEquals(List.of(List.of("A1", "A2"), List.of("B2")), planData.get("groups")); for (int i = 1; i < barcodes.length; ++i) { String barcode = barcodes[i]; // fetal waste barcode @@ -136,6 +137,17 @@ private void testRetrievePlanData(String[] barcodes) throws Exception { assertEquals(List.of(List.of("A1")), planData.get("groups")); } + private static List sampleIdInSlot(List slots, String address) { + for (Object slot : slots) { + if (chainGet(slot, "address").equals(address)) { + return chainGetList(slot, "samples").stream() + .map(sam -> chainGet(sam, "id")) + .toList(); + } + } + return null; + } + private void testConfirm(Sample[] blockSamples, Labware[] sourceBlocks, String[] barcodes) throws Exception { String barcode = barcodes[0]; Work work = entityCreator.createWork(null, null, null, null, null); @@ -172,6 +184,16 @@ private void testConfirm(Sample[] blockSamples, Labware[] sourceBlocks, String[] assertEquals("Tissue", chainGet(sam, "bioState", "name")); } + // A1 and A2 should contain the same sample + List a1SampleIds = sampleIdInSlot(slots, "A1"); + assertThat(a1SampleIds).hasSize(1); + List a2SampleIds = sampleIdInSlot(slots, "A2"); + assertEquals(a1SampleIds, a2SampleIds); + // B2 should contain a different sample + List b2SampleIds = sampleIdInSlot(slots, "B2"); + assertThat(b2SampleIds).hasSize(1); + assertNotEquals(a1SampleIds, b2SampleIds); + List> b2Samples = chainGetList(slots.stream() .filter(sd -> chainGet(sd, "address").equals("B2")) .findAny() @@ -188,10 +210,10 @@ private void testConfirm(Sample[] blockSamples, Labware[] sourceBlocks, String[] assertNotNull(chainGet(resultOp, "performed")); assertEquals("Section", chainGet(resultOp, "operationType", "name")); List> actions = chainGet(resultOp, "actions"); - int[] expectedSourceLabwareIds = {sourceBlocks[0].getId(), sourceBlocks[1].getId()}; - String[] expectedDestAddress = { "A1", "B2" }; - String[] expectedActionTissues = { "TISSUE1", "TISSUE2" }; - int[] expectedActionSecNum = { 14,17 }; + int[] expectedSourceLabwareIds = {sourceBlocks[0].getId(), sourceBlocks[0].getId(), sourceBlocks[1].getId()}; + String[] expectedDestAddress = { "A1", "A2", "B2" }; + String[] expectedActionTissues = { "TISSUE1", "TISSUE1", "TISSUE2" }; + int[] expectedActionSecNum = { 14,14,17 }; assertEquals(expectedSourceLabwareIds.length, actions.size()); int destLabwareId = -1; @@ -255,7 +277,7 @@ private void testConfirm(Sample[] blockSamples, Labware[] sourceBlocks, String[] entityManager.flush(); entityManager.refresh(work); assertThat(work.getOperationIds()).hasSize(3); - assertThat(work.getSampleSlotIds()).hasSize(4); + assertThat(work.getSampleSlotIds()).hasSize(5); List notes = lwNoteRepo.findAllByOperationIdIn(opIds); assertThat(notes).hasSize(2); @@ -271,7 +293,7 @@ private void testConfirm(Sample[] blockSamples, Labware[] sourceBlocks, String[] Integer opId = opIds.get(0); final List opcoms = opComRepo.findAllByOperationIdIn(opIds); Integer slotId = opcoms.get(0).getSlotId(); - assertThat(opcoms).hasSize(2); + assertThat(opcoms).hasSize(3); assertOpCom(opcoms.get(0), 2, opId, sampleIds.get(0), slotId); } diff --git a/src/test/resources/graphql/confirmsection.graphql b/src/test/resources/graphql/confirmsection.graphql index dab3d60a5..f1f1a7457 100644 --- a/src/test/resources/graphql/confirmsection.graphql +++ b/src/test/resources/graphql/confirmsection.graphql @@ -6,7 +6,7 @@ mutation { workNumber: "SGP4000" confirmSections: [ { - destinationAddresses: ["A1"] + destinationAddresses: ["A1", "A2"] newSection: 14 sampleId: 55555 commentIds: [2] diff --git a/src/test/resources/graphql/plan.graphql b/src/test/resources/graphql/plan.graphql index 28d3b1e8b..450b22a6b 100644 --- a/src/test/resources/graphql/plan.graphql +++ b/src/test/resources/graphql/plan.graphql @@ -8,7 +8,7 @@ mutation { actions:[ { source:{ barcode:"STAN-B70C" } - addresses: ["A1"] + addresses: ["A1", "A2"] sampleId: 55555 } { From a6527029710153fe8e8c33bfd521ef24228b055f Mon Sep 17 00:00:00 2001 From: David Robinson <14000840+khelwood@users.noreply.github.com> Date: Mon, 8 Dec 2025 14:38:19 +0000 Subject: [PATCH 3/3] remove extra generated setter in PlanData --- src/main/java/uk/ac/sanger/sccp/stan/request/PlanData.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/uk/ac/sanger/sccp/stan/request/PlanData.java b/src/main/java/uk/ac/sanger/sccp/stan/request/PlanData.java index 57f93387e..2d806d053 100644 --- a/src/main/java/uk/ac/sanger/sccp/stan/request/PlanData.java +++ b/src/main/java/uk/ac/sanger/sccp/stan/request/PlanData.java @@ -55,10 +55,6 @@ public void setDestination(LabwareFlagged destination) { this.destination = destination; } - public void setSources(List sources) { - this.sources = sources; - } - public List> getGroups() { return this.groups; }