Update Amounts and Units for better sorting and filtering behavior by labkey-susanh · Pull Request #6994 · LabKey/platform

labkey-susanh · 2025-09-02T23:24:02Z

Rationale

Our original implementation for the StoredAmount and Units columns added display columns over the user-provided data so the application grid displays used consistent units, but left the values as provided by users. Since we accept values in different, compatible units, this results in sorting and filtering on these columns not behaving as one would expect (e.g, samples with amounts of 2 mL and 2 L will be sorted next to each other, though displayed as 2 mL and 2000 mL in the grid).

This work update our handling of StoredAmount and Units so they always store values in the base units for sample types that have display units. This provides us with a valid basis for sorting and filtering.

We also are updating the processing of data to always require that the user provides both StoredAmount and Units when either of these values is provided.

Related Pull Requests

Changes

Upgrade script to convert existing amounts and units to base units, and drop any units that don't have values
Remove Measurement class in favor of Quantity and Units classes
Update detailed audit log to handle provided values that are converted before being stored
Update aliquot rollup logic to store values in base units
Remove recompute of rollup values when display unit is changed
Add SampleTypeAmountDisplayColumn and SampleTyepUnitDisplayColumn to display amount and units using chosen display units

(just working here, not for 24.11)

…1_fb_quantity

…s conversion more consistent in a bunch of places. ColumnRenderProperties.getConvertFn()

…fields to base units

# Conflicts: # api/src/org/labkey/api/data/ColumnRenderPropertiesImpl.java # api/src/org/labkey/api/data/DataColumn.java # api/src/org/labkey/api/data/DisplayColumn.java # api/src/org/labkey/api/ontology/KindOfQuantity.java # api/src/org/labkey/api/ontology/Quantity.java # api/src/org/labkey/api/ontology/Unit.java # experiment/resources/schemas/dbscripts/postgresql/exp-25.005-25.006.sql # experiment/resources/schemas/dbscripts/sqlserver/exp-25.005-25.006.sql

…he prepared statement anyway.

…mount display column default

Add "Stored Amount" as an import alias for StoredAmount column Add and use ImportAliasable.Helper.createImportMap() for SampleTypeUpdateServiceDI to get StoredAmount and Units column index

…unt and RawUnits when selecting from exp.material on update from file

… into fb_amountsAndUnits

…tries after upgrade

labkey-jeckels · 2025-09-12T02:54:19Z

experiment/src/org/labkey/experiment/ExperimentWarningProvider.java

+public class ExperimentWarningProvider implements WarningProvider
+{
+    @Override
+    public void addDynamicWarnings(@NotNull Warnings warnings, @Nullable ViewContext context, boolean showAllWarnings)


I believe we'll call this method on every LKS page load for admins, so we should cache the message to avoid requerying (likely very large) audit table when there is a mismatch.

One idea is to store an additional ExperimentModule.ACTUAL_AUDIT_COUNT_PROP in ExperimentModule.AMOUNT_AND_UNIT_UPGRADE_PROP property map.

Storing the count as a class member seems to work.

experiment/src/org/labkey/experiment/ExperimentWarningProvider.java

XingY · 2025-09-13T01:37:28Z

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

+        if (rawUnits == null)
+            return null;
+        if (rawUnits instanceof Unit u)
+            if (defaultUnits == null)


Can we add the outer braces to the block of if/else if/else ? It's confusing to read and error prone.

Sure. Done.

XingY · 2025-09-13T01:59:12Z

experiment/src/org/labkey/experiment/ExperimentWarningProvider.java

+public class ExperimentWarningProvider implements WarningProvider
+{
+    @Override
+    public void addDynamicWarnings(@NotNull Warnings warnings, @Nullable ViewContext context, boolean showAllWarnings)


One idea is to store an additional ExperimentModule.ACTUAL_AUDIT_COUNT_PROP in ExperimentModule.AMOUNT_AND_UNIT_UPGRADE_PROP property map.

XingY · 2025-09-13T02:01:47Z

experiment/src/org/labkey/experiment/ExperimentUpgradeCode.java

+        else if (materialUnit != null && !materialUnit.isCompatible(baseUnit))
+        {
+            LOG.info("{} '{}' for sample '{}' is not compatible with the base unit '{}'. No conversion done.", isAliquot ? "Aliquot unit" : "Unit", materialUnit.name(), sampleMap.get(Name.name()), baseUnit);
+            sampleCounts.put("invalidUnits", sampleCounts.getOrDefault("invalidUnits", 0) + 1);


Is it time to change those log level to DEBUG?

XingY · 2025-09-13T02:03:34Z

experiment/src/org/labkey/experiment/ExperimentUpgradeCode.java

+            {
+                PropertyManager.WritablePropertyMap props = PropertyManager.getWritableProperties(AMOUNT_AND_UNIT_UPGRADE_PROP, true);
+                props.put(AUDIT_COUNT_PROP, auditCount.toString());
+                props.put(TRANSACTION_ID_PROP, transactionId == null ? null : transactionId.toString());


If transactionId is null, do we still want to set property map?

It can't really be null unless my read of the logic is incorrect. This null check just to appease the IntelliJ warnings.

XingY · 2025-09-13T02:12:04Z

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

-        }
-
-        if (!StringUtils.isEmpty(totalDisplayUnitStr))
+        Unit totalUnit = null;


I feel we are not taking advantage of the fact now all aliquots are now stored in the same base "units", if sample type has a unit set. Rollup should now be much simpler (and faster) for sample types with unit, potentially could be done with a simple sql script.

Yeah, I see what you're saying, but I'm going to not make this change now. We can make this optimization in a separate, smaller PR so we can concentrate on assuring no regressions with the rollup vs. the unit conversion code.

Can you open an issue for this so we don't forget?

https://www.labkey.org/home/Developer/issues/issues-details.view?issueId=53932

labkey-matthewb and others added 30 commits December 20, 2024 15:23

quantity type prototype

c298fdc

(just working here, not for 24.11)

imports

37f9cb9

Merge remote-tracking branch 'origin/develop' into fb_quantity

60b2e5e

Merge remote-tracking branch 'origin/release24.11-SNAPSHOT' into 24.1…

127b97e

…1_fb_quantity

merge patch

0bea798

merge patch

345c146

java 17

e37d898

merge forward

4e19852

update expected result

0327635

build problem

c344188

I wasn't really planning on commiting this for a while. But this make…

a09337c

…s conversion more consistent in a bunch of places. ColumnRenderProperties.getConvertFn()

Merge branch 'develop' into fb_quantity

cedae1c

fixup after merge from develop

f2ed404

Merge branch 'develop' into fb_quantity

cecd9ac

Add QUANTITY_COLUMN_SUFFIX_TESTING experimental feature flag

eafbf25

Merge remote-tracking branch 'origin/develop' into fb_quantity

c65b82b

Merge remote-tracking branch 'origin/develop' into fb_quantity

cc7c908

Add "unit" unit to reflect current usage and some minor helper methods

7f1e3df

Quantity.java unit tests

c28adfb

Set Unit enum base to "unit" and "ml"

ae9d244

fixup Quantity testFormat test case

8f2d333

Unit.java unit tests and set ml base unit value as 1 instead of liter

dbe0f43

Upgrade script to convert StoredAmount and Units and related aliquot …

c625828

…fields to base units

comment update

dca876c

Add null check in isCompatible

75c08e5

compute aliquot total rollup in base units instead of display units

ad85c7b

Move to next schema version

a9c4f5d

Add Nullable and NotNull annotations and getValidatedUnit method

49ec409

Use Quantity Unit instead of Measurement.Unit

0a8c67b

cnathe mentioned this pull request Sep 9, 2025

Update Amounts and Units for better sorting and filtering behavior LabKey/testAutomation#2678

Merged

labkey-susanh and others added 13 commits September 9, 2025 15:44

Revert change to use a List in the eventQueue since we aren't using t…

4e64f4a

…he prepared statement anyway.

Set default format for RawAmount column if none set, to match StoredA…

96dade4

…mount display column default

Merge branch 'develop' into fb_amountsAndUnits

c7421c6

Remove RawAmount and RawUnits labels

ae33a83

Add "Stored Amount" as an import alias for StoredAmount column Add and use ImportAliasable.Helper.createImportMap() for SampleTypeUpdateServiceDI to get StoredAmount and Units column index

rename ImportAliasable createImportMap to createImportSet

9c2c0d0

remove Unit.count from getCommonUnits()

d4d5f1a

SampleTypeUpdateServiceDI update to fix warning for requesting RawAmo…

38fbf77

…unt and RawUnits when selecting from exp.material on update from file

Clear cache after doing the update

4bcaedf

Add space before units

27cfd0e

Update user display for upgrade audit event

5c5c23f

Merge branch 'fb_amountsAndUnits' of https://github.com/LabKey/platform…

c52712d

… into fb_amountsAndUnits

Merge remote-tracking branch 'origin/develop' into fb_amountsAndUnits

4aa09bb

Add a warning message if we don't see the expected number of audit en…

28919d5

…tries after upgrade

labkey-jeckels reviewed Sep 12, 2025

View reviewed changes

labkey-susanh added 4 commits September 12, 2025 06:30

Constants and caching

a98e517

Merge remote-tracking branch 'origin/develop' into fb_amountsAndUnits

1e96b71

Add recommendation text

aca2ff8

Allow the property deletion to happen even from a GET page

f11af1e

XingY reviewed Sep 13, 2025

View reviewed changes

cnathe and others added 3 commits September 13, 2025 08:26

Merge branch 'develop' into fb_amountsAndUnits

58ed496

switch to debug logging

83dfcaf

braces for better readability

3dec37b

labkey-susanh requested a review from XingY September 15, 2025 15:46

XingY approved these changes Sep 15, 2025

View reviewed changes

cnathe merged commit 420129c into develop Sep 15, 2025
16 of 17 checks passed

cnathe deleted the fb_amountsAndUnits branch September 15, 2025 18:54

labkey-susanh mentioned this pull request Sep 16, 2025

Prevent showing audit count warning if module startup is still in progress #7043

Merged

1 task

cnathe mentioned this pull request Sep 16, 2025

Test updates for errors where amount and units are now both required when one is provided LabKey/testAutomation#2692

Merged

cnathe mentioned this pull request Oct 2, 2025

GridPanelTest update to expect Units field in filter dialog LabKey/testAutomation#2722

Merged

Conversation

labkey-susanh commented Sep 2, 2025 • edited by cnathe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Related Pull Requests

Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

labkey-susanh commented Sep 2, 2025 •

edited by cnathe

Loading