diff --git a/api/src/org/labkey/api/query/AbstractQueryUpdateService.java b/api/src/org/labkey/api/query/AbstractQueryUpdateService.java index 19453164ec4..8bc8f200021 100644 --- a/api/src/org/labkey/api/query/AbstractQueryUpdateService.java +++ b/api/src/org/labkey/api/query/AbstractQueryUpdateService.java @@ -907,7 +907,7 @@ public List> updateRows(User user, Container container, List addAuditEvent(user, container, QueryService.AuditAction.UPDATE, configParameters, result, oldRows, providedValues); WorkflowService service = WorkflowService.get(); if (service != null && configParameters != null && configParameters.containsKey(WorkflowService.WorkflowConfigs.ActionId)) - service.onActionComplete(container, user, (Long) configParameters.get(WorkflowService.WorkflowConfigs.ActionId)); + service.onActionComplete(container, user, (Long) configParameters.get(WorkflowService.WorkflowConfigs.ActionId), (String) configParameters.get(AuditUserComment)); return result; } diff --git a/api/src/org/labkey/api/workflow/Action.java b/api/src/org/labkey/api/workflow/Action.java index 6ba2c550783..2b1dc5b4214 100644 --- a/api/src/org/labkey/api/workflow/Action.java +++ b/api/src/org/labkey/api/workflow/Action.java @@ -2,7 +2,9 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import org.apache.commons.lang3.StringUtils; +import org.jetbrains.annotations.Nullable; import org.json.JSONArray; +import org.json.JSONException; import org.json.JSONObject; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; @@ -10,6 +12,8 @@ import org.labkey.api.exp.api.ExpProtocol; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.exp.api.SampleTypeService; +import org.labkey.api.exp.query.ExpSchema; +import org.labkey.api.qc.DataState; import org.labkey.api.qc.SampleStatusService; import org.labkey.api.util.GUID; @@ -26,6 +30,7 @@ public abstract class Action extends CreatedModified public static final String ASSAY_TYPES_KEY = "assayTypes"; public static final String NUM_PER_PARENT_KEY = "numPerParent"; public static final String UPDATE_STATUS_KEY = "updateStatus"; + public static final String REMOVE_FROM_STORAGE_KEY = "removeFromStorage"; public static final String STATUS_KEY = "sampleStatus"; protected Long _rowId; protected int _ordinal; @@ -126,6 +131,88 @@ public void setInputParameters(JSONObject inputParameters) _inputParameters = inputParameters; } + /** + * Validates that the status parameters are valid, according to the following rules: + * - If updateKeyRequired is true, then STATUS_KEY and REMOVE_FROM_STORAGE_KEY may only be present if + * UPDATE_STATUS_KEY is true + * - STATUS_KEY is optional, but when present the value must be a valid status + * - REMOVE_FROM_STORAGE_KEY may only be present when STATUS_KEY is also present + * - If REMOVE_FROM_STORAGE_KEY is true, then the value for STATUS_KEY must represent a consumed state + */ + private @Nullable String validateStatus(Container container, String prefix, boolean updateKeyRequired) + { + boolean hasStatusKey = _inputParameters.has(STATUS_KEY); + boolean hasUpdateStatusKey = _inputParameters.has(UPDATE_STATUS_KEY); + boolean hasRemoveFromStorageKey = _inputParameters.has(REMOVE_FROM_STORAGE_KEY); + + boolean updateStatus; + try + { + updateStatus = hasUpdateStatusKey && _inputParameters.getBoolean(UPDATE_STATUS_KEY); + } + catch (JSONException e) + { + return prefix + UPDATE_STATUS_KEY + " must be a boolean."; + } + + boolean removeFromStorage; + try + { + removeFromStorage = hasRemoveFromStorageKey && _inputParameters.getBoolean(REMOVE_FROM_STORAGE_KEY); + } + catch (JSONException e) + { + return prefix + REMOVE_FROM_STORAGE_KEY + " must be a boolean."; + } + + // If we're expecting an UPDATE_STATUS_KEY (aliquot / derive / pool actions), then the key must be present and + // set to true in order for us to honor the STATUS_KEY and REMOVE_FROM_STORAGE_KEY. + if (updateKeyRequired && !updateStatus) + { + if (hasStatusKey) + return prefix + STATUS_KEY + " is not allowed for action of type " + _type + " when " + UPDATE_STATUS_KEY + " is false."; + + if (hasRemoveFromStorageKey) + return prefix + REMOVE_FROM_STORAGE_KEY + " is not allowed for action of type " + _type + " when " + UPDATE_STATUS_KEY + " is false."; + } + + if (removeFromStorage && !hasStatusKey) + return prefix + STATUS_KEY + " is required for action of type " + _type + " when " + REMOVE_FROM_STORAGE_KEY + " is true."; + + if (hasStatusKey && container != null) + { + long statusId; + try + { + statusId = _inputParameters.getLong(STATUS_KEY); + } + catch (JSONException e) + { + return prefix + STATUS_KEY + " must be a number."; + } + + DataState state = SampleStatusService.get().getStateForRowId(container, statusId); + + if (state == null) + return prefix + "Invalid " + STATUS_KEY + " (" + statusId + ") for container " + container.getPath() + "."; + + if (removeFromStorage && !ExpSchema.SampleStateType.Consumed.name().equals(state.getStateType())) + return prefix + STATUS_KEY + " (" + statusId + ") must represent a " + ExpSchema.SampleStateType.Consumed.name() + " state when " + REMOVE_FROM_STORAGE_KEY + " is true."; + } + + return null; + } + + private static boolean isSampleStatusKey(String key) + { + return key.equals(STATUS_KEY) || key.equals(UPDATE_STATUS_KEY) || key.equals(REMOVE_FROM_STORAGE_KEY); + } + + private boolean hasAnySampleStatusKey() + { + return _inputParameters != null && (_inputParameters.has(UPDATE_STATUS_KEY) || _inputParameters.has(STATUS_KEY) || _inputParameters.has(REMOVE_FROM_STORAGE_KEY)); + } + @JsonIgnore public List validateInputParameters(int ordinal, Container container) { @@ -169,31 +256,57 @@ public List validateInputParameters(int ordinal, Container container) } else if (_type == WorkflowService.ActionType.AliquotSamples) { + List messages = new ArrayList<>(); + if (_inputParameters == null || !_inputParameters.has(NUM_PER_PARENT_KEY)) - return List.of(prefix + NUM_PER_PARENT_KEY + " is required for action of type " + _type + "."); + messages.add(prefix + NUM_PER_PARENT_KEY + " is required for action of type " + _type + "."); else { try { int numPerParent = _inputParameters.getInt(NUM_PER_PARENT_KEY); if (numPerParent < 0) - return List.of(prefix + NUM_PER_PARENT_KEY + " cannot be negative."); + messages.add(prefix + NUM_PER_PARENT_KEY + " cannot be negative."); } catch (Exception e) { - return List.of(prefix + NUM_PER_PARENT_KEY + " must be an integer."); + messages.add(prefix + NUM_PER_PARENT_KEY + " must be an integer."); } } + + if (hasAnySampleStatusKey()) + { + String statusMessage = validateStatus(container, prefix, true); + if (statusMessage != null) messages.add(statusMessage); + } + + return messages; } else if (_type == WorkflowService.ActionType.DeriveSamples || _type == WorkflowService.ActionType.PoolSamples) { - if (_inputParameters == null || _inputParameters.isEmpty()) - return List.of(prefix + "data about sample types and sample counts per parent is required for action of type " + _type + "."); - if (_type == WorkflowService.ActionType.PoolSamples && _inputParameters.length() > 1) + String emptyMessage = prefix + "data about sample types and sample counts per parent is required for action of type " + _type + "."; + + if (_inputParameters == null) return List.of(emptyMessage); + + // We can't just check _inputParameters size because it may include sample status keys, so we extract the + // sample type IDs and validate against those. + List sampleTypeIds = new ArrayList<>(); + _inputParameters.keys().forEachRemaining(id -> { + if (isSampleStatusKey(id)) return; + sampleTypeIds.add(id); + }); + + if (sampleTypeIds.isEmpty()) + return List.of(emptyMessage); + + if (_type == WorkflowService.ActionType.PoolSamples && sampleTypeIds.size() > 1) return List.of(prefix + "only one sample type can be specified for action of type " + _type + "."); + SampleTypeService sampleTypeService = SampleTypeService.get(); Set invalidSampleTypeIds = new HashSet<>(); List invalidCounts = new ArrayList<>(); + List messages = new ArrayList<>(); - _inputParameters.keys().forEachRemaining(id -> { + for (String id : sampleTypeIds) + { try { if (sampleTypeService.getSampleType(Long.valueOf(id)) == null) @@ -204,55 +317,53 @@ else if (_type == WorkflowService.ActionType.DeriveSamples || _type == WorkflowS invalidSampleTypeIds.add(id); } Object countObj = _inputParameters.get(id); - if ((countObj instanceof String)) + if (countObj instanceof String countStr) try { - if (Integer.parseInt((String) countObj) < 0) + if (Integer.parseInt(countStr) < 0) invalidCounts.add(countObj); } catch (NumberFormatException e) { invalidCounts.add(countObj); } - else if (countObj instanceof Integer) + else if (countObj instanceof Integer count) { - if (((Integer) countObj) < 0) + if (count < 0) invalidCounts.add(countObj); } else invalidCounts.add(countObj); - }); - List messages = new ArrayList<>(); + } + if (!invalidSampleTypeIds.isEmpty()) messages.add(prefix + "invalid sample type IDs " + invalidSampleTypeIds + "."); if (!invalidCounts.isEmpty()) messages.add(prefix + "invalid sample count values " + invalidCounts + "."); + + if (hasAnySampleStatusKey()) + { + String statusMessage = validateStatus(container, prefix, true); + + if (statusMessage != null) messages.add(statusMessage); + } + return messages; } - else if (_type == WorkflowService.ActionType.RemoveFromStorage) + else if (_type == WorkflowService.ActionType.RemoveFromStorage || _type == WorkflowService.ActionType.UpdateSampleStatus) { if (_inputParameters == null || _inputParameters.isEmpty()) return Collections.emptyList(); - boolean updateStatus = _inputParameters.getBoolean(UPDATE_STATUS_KEY); - if (updateStatus && !_inputParameters.has(STATUS_KEY)) - return List.of(prefix + STATUS_KEY + " is required for action of type " + _type + " when " + UPDATE_STATUS_KEY + " is true."); - if (!updateStatus && _inputParameters.has(STATUS_KEY)) - return List.of(prefix + STATUS_KEY + " is not allowed for action of type " + _type + " when " + UPDATE_STATUS_KEY + " is false."); - if (updateStatus && container != null) - { - try - { - long statusId = _inputParameters.getLong(STATUS_KEY); - SampleStatusService sampleStatusService = SampleStatusService.get(); - if (sampleStatusService.getStateForRowId(container, statusId) == null) - return List.of(prefix + "Invalid " + STATUS_KEY + " (" + statusId + ")."); - } - catch (Exception e) - { - return List.of(prefix + "Invalid " + STATUS_KEY + "."); - } - } - } else { + + boolean updateKeyRequired = _type == WorkflowService.ActionType.RemoveFromStorage; + String statusMessage = validateStatus(container, prefix, updateKeyRequired); + + if (statusMessage != null) return List.of(statusMessage); + + return Collections.emptyList(); + } + else + { if (_inputParameters != null && !_inputParameters.isEmpty()) return List.of(prefix + "input parameters are not allowed for action of type " + _type + "."); } diff --git a/api/src/org/labkey/api/workflow/WorkflowService.java b/api/src/org/labkey/api/workflow/WorkflowService.java index b402ebaf1bb..e1a7a934947 100644 --- a/api/src/org/labkey/api/workflow/WorkflowService.java +++ b/api/src/org/labkey/api/workflow/WorkflowService.java @@ -2,6 +2,7 @@ import jakarta.servlet.http.HttpServletRequest; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import org.labkey.api.data.Container; import org.labkey.api.dataiterator.DataIteratorBuilder; import org.labkey.api.query.ValidationException; @@ -9,7 +10,6 @@ import org.labkey.api.services.ServiceRegistry; import java.util.Map; -import org.jetbrains.annotations.Nullable; public interface WorkflowService { @@ -29,7 +29,8 @@ enum ActionType MoveInStorage("input parameters", "Moved samples in storage"), CheckOut("input parameters", "Checked out samples"), CheckIn("input parameters", "Checked in samples"), - RemoveFromStorage("sample status value", "Removed samples from storage"); + RemoveFromStorage("sample status value", "Removed samples from storage"), + UpdateSampleStatus("sample status value", "Updated sample status"); private final String _inputDescription; private final String _auditMessage; @@ -65,8 +66,9 @@ static WorkflowService get() void populateConfigParams(HttpServletRequest request, Map configParameters) throws ValidationException; Map getConfigParameters(HttpServletRequest request) throws ValidationException; - void onActionComplete(@NotNull Container container, @NotNull User user, @NotNull Long actionId); + void onActionComplete(@NotNull Container container, @NotNull User user, @NotNull Long actionId, @Nullable String userAuditComment); void onActionComplete(@NotNull Container container, @NotNull User user, @NotNull Long taskId, @NotNull ActionType actionType); + boolean actionWillAddSamples(Long actionId); DataIteratorBuilder getSampleCreationDataIteratorBuilder(DataIteratorBuilder data, Container container, User user); diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java index 13774614ac3..369f0d442d6 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeUpdateServiceDI.java @@ -420,7 +420,13 @@ public DataIteratorBuilder createImportDIB(User user, Container container, DataI { if (context.getConfigParameter(WorkflowService.WorkflowConfigs.ActionId) != null) { - dib = workService.getSampleCreationDataIteratorBuilder(dib, userSchema.getContainer(), userSchema.getUser()); + Long actionId = (Long) context.getConfigParameter(WorkflowService.WorkflowConfigs.ActionId); + + if (WorkflowService.get().actionWillAddSamples(actionId)) + { + dib = workService.getSampleCreationDataIteratorBuilder(dib, userSchema.getContainer(), userSchema.getUser()); + } + dib = workService.getActionAuditDataIteratorBuilder(dib, userSchema.getContainer(), userSchema.getUser()); } }