Issues 53975, 53955, 53961, 53963: misc messaging issues

api/src/org/labkey/api/exp/api/SampleTypeService.java

@@ -30,6 +30,7 @@
 import org.labkey.api.gwt.client.model.GWTIndex;
 import org.labkey.api.gwt.client.model.GWTPropertyDescriptor;
 import org.labkey.api.lists.permissions.ManagePicklistsPermission;
+import org.labkey.api.ontology.Unit;
 import org.labkey.api.qc.DataState;
 import org.labkey.api.query.BatchValidationException;
 import org.labkey.api.query.ValidationException;
@@ -54,8 +55,8 @@
 
 public interface SampleTypeService
 {
-    String MISSING_COLUMN_ERROR_MESSAGE_PATTERN = "When adding or updating samples, the %s column must be provided when the %s column is.";
-    String MISSING_COLUMN_VALUE_ERROR_MESSAGE_PATTERN = "When adding or updating samples, a %s value must be provided when there is a value for %s.";
+    String MISSING_AMOUNT_ERROR_MESSAGE = "An Amount value must be provided when Units are provided.";
+    String MISSING_UNITS_ERROR_MESSAGE = "A Units value must be provided when Amounts are provided.";
     String UNPROVIDED_VALUE_ERROR_MESSAGE_PATTERN = "No %s value provided for %s %s.";
     String NEW_SAMPLE_TYPE_ALIAS_VALUE = "{{this_sample_set}}";
     String MATERIAL_INPUTS_PREFIX = "MaterialInputs/";
@@ -114,6 +115,12 @@ static void setInstance(SampleTypeService impl)
         ServiceRegistry.get().registerService(SampleTypeService.class, impl);
     }
 
+    @NotNull
+    List<Unit> getSupportedUnits();
+
+    @Nullable
+    Unit getValidatedUnit(@Nullable Object rawUnits, @Nullable Unit defaultUnits, @Nullable String sampleTypeName);
+
     Map<String, ExpSampleType> getSampleTypesForRoles(Container container, ContainerFilter filter, ExpProtocol.ApplicationType type);
 
     /**

api/src/org/labkey/api/ontology/KindOfQuantity.java

@@ -20,7 +20,7 @@ public enum KindOfQuantity
         @Override
         public List<Unit> getCommonUnits()
         {
-            return List.of(Unit.L, Unit.mL, Unit.uL);
+            return List.of(Unit.kL, Unit.L, Unit.mL, Unit.uL);
         }
     },
 

api/src/org/labkey/api/ontology/Unit.java

@@ -9,7 +9,6 @@
 import org.labkey.api.data.ConversionExceptionWithMessage;
 
 import java.util.HashMap;
-import java.util.List;
 import java.util.function.Function;
 
 public enum Unit
@@ -76,8 +75,6 @@ public enum Unit
             Quantity.Mass_pg.class,
             "picogram", "picograms");
 
-    private static final String CONVERSION_EXCEPTION_MESSAGE ="Units value (%s) is not compatible with the display units (%s).";
-
     @Getter
     final @NotNull KindOfQuantity kindOfQuantity;
     @Getter
@@ -198,38 +195,6 @@ public Quantity convert(@Nullable Object value)
         return Quantity.convert(value, this);
     }
 
-    public static Unit getValidatedUnit(@Nullable Object rawUnits, @Nullable Unit defaultUnits)
-    {
-        if (rawUnits == null)
-            return null;
-        if (rawUnits instanceof Unit u)
-        {
-            if (defaultUnits == null)
-                return u;
-            else if (u.kindOfQuantity != defaultUnits.kindOfQuantity)
-                throw new ConversionExceptionWithMessage(String.format(CONVERSION_EXCEPTION_MESSAGE, rawUnits, defaultUnits));
-            else
-                return u;
-        }
-        if (!(rawUnits instanceof String rawUnitsString))
-            throw new ConversionExceptionWithMessage(String.format(CONVERSION_EXCEPTION_MESSAGE, rawUnits, defaultUnits));
-        if (!StringUtils.isBlank(rawUnitsString))
-        {
-            rawUnitsString = rawUnitsString.trim();
-
-            Unit mUnit = Unit.fromName(rawUnitsString);
-            if (mUnit == null)
-            {
-                List<Unit> commonUnits = KindOfQuantity.getSupportedUnits();
-                throw new ConversionExceptionWithMessage("Unsupported Units value (" + rawUnitsString + ").  Supported values are: " + StringUtils.join(commonUnits, ", ") + ".");
-            }
-            if (defaultUnits != null && mUnit.kindOfQuantity != defaultUnits.kindOfQuantity)
-                throw new ConversionExceptionWithMessage(String.format(CONVERSION_EXCEPTION_MESSAGE, rawUnits, defaultUnits));
-            return mUnit;
-        }
-        return null;
-    }
-
     public static class TestCase extends Assert
     {
         @Test
@@ -325,56 +290,5 @@ public void testFromName()
             assertNull(Unit.fromName(""));
         }
 
-        @Test
-        public void testGetValidatedUnit()
-        {
-            try
-            {
-                Unit.getValidatedUnit("g", Unit.mg);
-                Unit.getValidatedUnit("g ", Unit.mg);
-                Unit.getValidatedUnit(" g ", Unit.mg);
-            }
-            catch (ConversionExceptionWithMessage e)
-            {
-                fail("Compatible unit should not throw exception.");
-            }
-            try
-            {
-                assertNull(Unit.getValidatedUnit(null, Unit.unit));
-            }
-            catch (ConversionExceptionWithMessage e)
-            {
-                fail("null units should be null");
-            }
-            try
-            {
-                assertNull(Unit.getValidatedUnit("", Unit.unit));
-            }
-            catch (ConversionExceptionWithMessage e)
-            {
-                fail("empty units should be null");
-            }
-            try
-            {
-                Unit.getValidatedUnit("g", Unit.unit);
-                fail("Units that are not comparable should throw exception.");
-            }
-            catch (ConversionExceptionWithMessage ignore)
-            {
-
-            }
-
-            try
-            {
-                Unit.getValidatedUnit("nonesuch", Unit.unit);
-                fail("Invalid units should throw exception.");
-            }
-            catch (ConversionExceptionWithMessage ignore)
-            {
-
-            }
-
-        }
-
     }
 }

experiment/resources/schemas/exp.xml

@@ -223,7 +223,7 @@
           <column columnName="Name" >
               <!--
                 Resolve lists by "name" instead of "listId" as "name" will resolve cross-folder
-                where as "listId"s are only unique within a folder.
+                whereas "listId"s are only unique within a folder.
               -->
               <url>/list/grid.view?name=${Name}</url>
           </column>

experiment/src/org/labkey/experiment/ExperimentModule.java

@@ -1039,6 +1039,7 @@ public Set<Class> getIntegrationTests()
             LineageTest.class,
             OntologyManager.TestCase.class,
             PropertyServiceImpl.TestCase.class,
+            SampleTypeServiceImpl.TestCase.class,
             StorageNameGenerator.TestCase.class,
             StorageProvisionerImpl.TestCase.class,
             UniqueValueCounterTestCase.class

experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java

@@ -557,11 +557,11 @@ public void createSampleNames(@NotNull List<Map<String, Object>> maps,
         {
             // Failed to generate a name due to some part of the expression not in the row
             if (hasNameExpression())
-                throw new ExperimentException("Failed to generate name for sample on row " + e.getRowNumber(), e);
+                throw new ExperimentException("Failed to generate name for sample.", e);
             else if (hasNameAsIdCol())
-                throw new ExperimentException("SampleID or Name is required for sample on row " + e.getRowNumber(), e);
+                throw new ExperimentException("SampleID or Name is required for sample.", e);
             else
-                throw new ExperimentException("All id columns are required for sample on row " + e.getRowNumber(), e);
+                throw new ExperimentException("All id columns are required for sample.", e);
         }
     }
 
@@ -609,7 +609,7 @@ public String createSampleName(@NotNull Map<String, Object> rowMap,
         }
         catch (NameGenerator.NameGenerationException e)
         {
-            throw new ExperimentException("Failed to generate name for Sample", e);
+            throw new ExperimentException("Failed to generate name for sample.", e);
         }
     }
 

experiment/src/org/labkey/experiment/api/ExpSampleTypeTableImpl.java

@@ -64,11 +64,16 @@ public MutableColumnInfo createColumn(String alias, Column column)
             case MaterialLSIDPrefix:
             case Name:
             case LabelColor:
-            case MetricUnit:
             case AutoLinkTargetContainer:
             case AutoLinkCategory:
             case RowId:
                 return wrapColumn(alias, _rootTable.getColumn(column.toString()));
+            case MetricUnit:
+            {
+                var columnInfo = wrapColumn(alias, _rootTable.getColumn(column.toString()));
+                columnInfo.setLabel("Display Units");
+                return columnInfo;
+            }
             case Created:
                 return wrapColumn(alias, _rootTable.getColumn("Created"));
             case CreatedBy:

experiment/src/org/labkey/experiment/api/ExpSampleTypeTestCase.jsp

@@ -390,7 +390,7 @@ public void idColsSet_nameExpressionNull_hasNameProperty() throws Exception
     errors = new BatchValidationException();
     svc.insertRows(user, c, rows, errors, null, null);
     assertTrue(errors.hasErrors());
-    assertTrue(errors.getMessage().contains("SampleID or Name is required for sample on row 1"));
+    assertTrue(errors.getMessage().contains("SampleID or Name is required for sample"));
 }
 
 // idCols not null, nameExpression not null, 'name' property (not used) -- fail

experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java

@@ -23,6 +23,8 @@
 import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
+import org.junit.Assert;
+import org.junit.Test;
 import org.labkey.api.action.ApiUsageException;
 import org.labkey.api.audit.AbstractAuditHandler;
 import org.labkey.api.audit.AbstractAuditTypeProvider;
@@ -42,6 +44,7 @@
 import org.labkey.api.data.Container;
 import org.labkey.api.data.ContainerFilter;
 import org.labkey.api.data.ContainerManager;
+import org.labkey.api.data.ConversionExceptionWithMessage;
 import org.labkey.api.data.DatabaseCache;
 import org.labkey.api.data.DbSchema;
 import org.labkey.api.data.DbScope;
@@ -94,6 +97,7 @@
 import org.labkey.api.inventory.InventoryService;
 import org.labkey.api.miniprofiler.MiniProfiler;
 import org.labkey.api.miniprofiler.Timing;
+import org.labkey.api.ontology.KindOfQuantity;
 import org.labkey.api.ontology.Quantity;
 import org.labkey.api.ontology.Unit;
 import org.labkey.api.qc.DataState;
@@ -174,6 +178,16 @@ public class SampleTypeServiceImpl extends AbstractAuditHandler implements Sampl
     public static final String SAMPLE_COUNT_SEQ_NAME = "org.labkey.api.exp.api.ExpMaterial:sampleCount";
     public static final String ROOT_SAMPLE_COUNT_SEQ_NAME = "org.labkey.api.exp.api.ExpMaterial:rootSampleCount";
 
+    public static final List<Unit> SUPPORTED_UNITS = new ArrayList<>();
+    public static final String CONVERSION_EXCEPTION_MESSAGE ="Units value (%s) is not compatible with the %s display units (%s).";
+
+    static
+    {
+        SUPPORTED_UNITS.addAll(KindOfQuantity.Volume.getCommonUnits());
+        SUPPORTED_UNITS.addAll(KindOfQuantity.Mass.getCommonUnits());
+        SUPPORTED_UNITS.addAll(KindOfQuantity.Count.getCommonUnits());
+    }
+
     // columns that may appear in a row when only the sample status is updating.
     public static final Set<String> statusUpdateColumns = Set.of(
             ExpMaterialTable.Column.Modified.name().toLowerCase(),
@@ -208,6 +222,44 @@ Cache<String, SortedSet<MaterialSource>> getMaterialSourceCache()
         return materialSourceCache;
     }
 
+    @Override @NotNull
+    public List<Unit> getSupportedUnits()
+    {
+        return SUPPORTED_UNITS;
+    }
+
+    @Nullable @Override
+    public Unit getValidatedUnit(@Nullable Object rawUnits, @Nullable Unit defaultUnits, String sampleTypeName)
+    {
+        if (rawUnits == null)
+            return null;
+        if (rawUnits instanceof Unit u)
+        {
+            if (defaultUnits == null)
+                return u;
+            else if (u.getKindOfQuantity() != defaultUnits.getKindOfQuantity())
+                throw new ConversionExceptionWithMessage(String.format(CONVERSION_EXCEPTION_MESSAGE, rawUnits, sampleTypeName == null ? "" : sampleTypeName, defaultUnits));
+            else
+                return u;
+        }
+        if (!(rawUnits instanceof String rawUnitsString))
+            throw new ConversionExceptionWithMessage(String.format(CONVERSION_EXCEPTION_MESSAGE, rawUnits, sampleTypeName == null ? "" : sampleTypeName, defaultUnits));
+        if (!StringUtils.isBlank(rawUnitsString))
+        {
+            rawUnitsString = rawUnitsString.trim();
+
+            Unit mUnit = Unit.fromName(rawUnitsString);
+            List<Unit> commonUnits = getSupportedUnits();
+            if (mUnit == null || !commonUnits.contains(mUnit))
+            {
+                throw new ConversionExceptionWithMessage("Unsupported Units value (" + rawUnitsString + ").  Supported values are: " + StringUtils.join(commonUnits, ", ") + ".");
+            }
+            if (defaultUnits != null && mUnit.getKindOfQuantity() != defaultUnits.getKindOfQuantity())
+                throw new ConversionExceptionWithMessage(String.format(CONVERSION_EXCEPTION_MESSAGE, rawUnits, sampleTypeName == null ? "" : sampleTypeName, defaultUnits));
+            return mUnit;
+        }
+        return null;
+    }
 
     public void clearMaterialSourceCache(@Nullable Container c)
     {
@@ -2259,4 +2311,60 @@ public void refreshSampleTypeMaterializedView(@NotNull ExpSampleType st, SampleC
     {
         ExpMaterialTableImpl.refreshMaterializedView(st.getLSID(), reason);
     }
+
+
+    public static class TestCase extends Assert
+    {
+        @Test
+        public void testGetValidatedUnit()
+        {
+            SampleTypeService service = SampleTypeService.get();
+            try
+            {
+                service.getValidatedUnit("g", Unit.mg, "Sample Type");
+                service.getValidatedUnit("g ", Unit.mg, "Sample Type");
+                service.getValidatedUnit(" g ", Unit.mg, "Sample Type");
+            }
+            catch (ConversionExceptionWithMessage e)
+            {
+                fail("Compatible unit should not throw exception.");
+            }
+            try
+            {
+                assertNull(service.getValidatedUnit(null, Unit.unit, "Sample Type"));
+            }
+            catch (ConversionExceptionWithMessage e)
+            {
+                fail("null units should be null");
+            }
+            try
+            {
+                assertNull(service.getValidatedUnit("", Unit.unit, "Sample Type"));
+            }
+            catch (ConversionExceptionWithMessage e)
+            {
+                fail("empty units should be null");
+            }
+            try
+            {
+                service.getValidatedUnit("g", Unit.unit, "Sample Type");
+                fail("Units that are not comparable should throw exception.");
+            }
+            catch (ConversionExceptionWithMessage ignore)
+            {
+
+            }
+
+            try
+            {
+                service.getValidatedUnit("nonesuch", Unit.unit, "Sample Type");
+                fail("Invalid units should throw exception.");
+            }
+            catch (ConversionExceptionWithMessage ignore)
+            {
+
+            }
+
+        }
+    }
 }