Add detailed auditing of domain changes & comment ability by XingY · Pull Request #6655 · LabKey/platform

XingY · 2025-05-12T05:15:04Z

Rationale

There is currently an App only setting that "requires users provide a reason before completing certain actions", which forces user to enter an audit comment before update/delete a data in the App. Certain update and delete actions of data are wired up to accept auditUserComment but so far no enforcement is in place to ensure this comment is provided. This PR extends the actions that accept auditUserComment to updateDomain, deleteDomain and assay protocol update actions. There is a future story to enforce "required" comment.

This PR also records more details about what's updated during a domain create/update. The domain event and domain property event will now have newRecordMap and oldRecordMap columns, that record all domain or property settings during update/create. Pomain property event will now report more changes, including regex, conditional formatting, text choices, etc.

Related Pull Requests

Changes

wire up auditUserComment to updateDomain, deleteDomain and assay protocol actions and utils
wire up Domain Audit Event for dataset delete action
add old and new record map to Domain and Domain Property Events
fix up domain event comments for properties update for sample type, dataclass, assay, list and dataset
fix up domain property event comment for column datetime, number format, conditional format, regex validator, text choices update, etc.
add selenium tests to check for domain and property audit comment updates

labkey-nicka

In general, this approach of being very explicit about each change and generating the audit message snippet wherever a changes is made is fine but I don't think it is ideal. The amount of checking for changes and the ease with which future changes could miss adding these details makes this cumbersome and fragile.

Seems like there is potential for a way to have domains retain a difference so that when auditing occurs it can be computed what the auditing should record.

That said, I'm not going to hold up this work on this account.

api/src/org/labkey/api/study/Dataset.java

study/src/org/labkey/study/model/StudyManager.java

labkey-nicka · 2025-05-14T18:21:28Z

study/src/org/labkey/study/model/StudyManager.java

+        if (domain == null)
+            return;
+
+        DomainAuditProvider.DomainAuditEvent event = new DomainAuditProvider.DomainAuditEvent(study.getContainer(), String.format("The domain %s was deleted", domain.getName()));


I see that you tried to call domain.delete(user, auditUserComment). Why did you move away from that? Did you run into deletion problems / lock-up? Seems that would be much preferred to wiring up our own audit event here.

It was causing a failure on TC https://teamcity.labkey.org/buildConfiguration/LabkeyTrunk_Bvt/3508494, consistently. I cannot repro the failure locally (the problematic dataset is created in ClientAPITest), nor understand it. It might has to do with study container vs definition container vs domain container, but not sure why no repro for me.

study/src/org/labkey/study/controllers/StudyController.java

study/src/org/labkey/study/controllers/DatasetController.java

api/src/org/labkey/api/exp/list/ListDefinition.java

api/src/org/labkey/api/exp/property/DomainUtil.java

experiment/src/org/labkey/experiment/ExpDataFileListener.java

# Conflicts: # biologics/package-lock.json # biologics/package.json # inventory/package-lock.json # inventory/package.json # sampleManagement/package-lock.json # sampleManagement/package.json

XingY · 2025-05-16T23:35:16Z

In general, this approach of being very explicit about each change and generating the audit message snippet wherever a changes is made is fine but I don't think it is ideal. The amount of checking for changes and the ease with which future changes could miss adding these details makes this cumbersome and fragile.

Seems like there is potential for a way to have domains retain a difference so that when auditing occurs it can be computed what the auditing should record.

That said, I'm not going to hold up this work on this account.

The implementation has been updated to be a bit more aligned with this comment.

# Conflicts: # api/src/org/labkey/api/exp/PropertyDescriptor.java

labkey-nicka · 2025-05-20T20:50:16Z

api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java

    }

-    public static String encodeForDataMap(Container c, Map<String, ?> properties)
+    public static String encodeForDataMap(@Nullable Container c, Map<String, ?> properties)


nit: Looks like the Container parameter is not actually used by this method and can be factored out.

labkey-nicka · 2025-05-20T20:55:39Z

api/src/org/labkey/api/data/PropertyStorageSpec.java

+
+        public String toStringVal()
+        {
+            if (columnNames == null)


nit: Should this check for empty as well? Seems a bit incongruent to return when null, but then join and display the unique flag setting when columnNames is empty.

labkey-nicka · 2025-05-20T20:56:34Z

api/src/org/labkey/api/assay/AbstractAssayProvider.java


    @Override
-    public ValidationException setValidationAndAnalysisScripts(ExpProtocol protocol, @NotNull List<AnalysisScript> scripts) throws ExperimentException
+    public Pair<ValidationException, Pair<String, String>> setValidationAndAnalysisScripts(ExpProtocol protocol, @NotNull List<AnalysisScript> scripts) throws ExperimentException


nit: A typed record would provide more context than type Pair<String, String> about what these strings are.

labkey-nicka · 2025-05-20T21:16:00Z

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

+
+    public Map<String, Object> getAuditRecordMap(@Nullable String validatorStr, @Nullable String conditionalFormatStr)
+    {
+        Map<String, Object> map = new LinkedHashMap<>();


Should this include any of the following:

getContainer()

getPropertyId()

getDatabaseDefaultValue()

labkey-nicka · 2025-05-20T21:23:37Z

api/src/org/labkey/api/exp/property/Domain.java

-    void save(User user, boolean auditComment) throws ChangePropertyDescriptorException;
-    void save(User user, @Nullable String allowAddBaseProperty) throws ChangePropertyDescriptorException;
+    void save(User user, @Nullable Map<String, Object> newRecordMap) throws ChangePropertyDescriptorException;
+    void save(User user, @Nullable String auditComment, @Nullable String auditUserComment, @Nullable Map<String, Object> oldRecordMap, @Nullable Map<String, Object> newRecordMap) throws ChangePropertyDescriptorException;


nit: A comment here about the difference between auditComment and auditUserComment could be useful.

labkey-nicka · 2025-05-20T21:26:28Z

api/src/org/labkey/api/exp/property/DomainPropertyAuditProvider.java

+
+        appendValueMapColumns(table);
+
+        DetailsURL url = DetailsURL.fromString("audit-detailedAuditChanges.view?auditRowId=${rowId}&auditEventType=" + EVENT_NAME);


Consider consolidating these URL generation logic:

labkey-nicka · 2025-05-20T21:30:54Z

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

    }
+
+    @Override
+    public Map<String, Object> getAuditRecordMap(AssayProvider provider, Container container)


Container parameter is unused

labkey-nicka · 2025-05-20T21:40:11Z

api/gwtsrc/org/labkey/api/gwt/client/model/GWTIndex.java

+    public static List<String> toStringVals(List<GWTIndex> indices, Set<PropertyStorageSpec.Index> excludeBaseIndices)
+    {
+        if (indices == null || indices.isEmpty())
+            return null;


nit: Can just return Collections.emptyList() then callers do not need to worry about null values (and could be annotated @NotNull.

labkey-nicka · 2025-05-20T21:42:32Z

experiment/src/org/labkey/experiment/api/property/DomainPropertyImpl.java

        return super.toString() + _pd.getPropertyURI();
    }

+    public Map<String, Object> getAuditRecordMap(@Nullable String validatorStr, @Nullable String conditionalFormatStr)


Since DomainPropertyImpl wraps a PropertyDescriptor should this somehow utilize the audit record of the underlying property descriptor and then extend with whatever additional properties are desired?

labkey-nicka · 2025-05-20T21:45:20Z

In general, this approach of being very explicit about each change and generating the audit message snippet wherever a changes is made is fine but I don't think it is ideal. The amount of checking for changes and the ease with which future changes could miss adding these details makes this cumbersome and fragile.
Seems like there is potential for a way to have domains retain a difference so that when auditing occurs it can be computed what the auditing should record.
That said, I'm not going to hold up this work on this account.

The implementation has been updated to be a bit more aligned with this comment.

Much improved!

…d tests

… Pattern and can be hit quickly with unicode characters

# Conflicts: # core/package-lock.json # core/package.json # experiment/package-lock.json # experiment/package.json

# Conflicts: # biologics/package-lock.json # biologics/package.json # inventory/package-lock.json # inventory/package.json # sampleManagement/package-lock.json # sampleManagement/package.json

# Conflicts: # biologics/package-lock.json # biologics/package.json # inventory/package-lock.json # inventory/package.json # sampleManagement/package-lock.json # sampleManagement/package.json Merge branch 'develop' into fb_domainAudit # Conflicts: # core/package-lock.json # core/package.json # experiment/package-lock.json # experiment/package.json

XingY added 3 commits May 8, 2025 09:48

Support auditUserComment for domain update/delete

c4c68d4

Add detailed auditing of domain changes & comment ability

11b5243

Merge branch 'develop' into fb_domainAudit

9894ab1

XingY added 4 commits May 11, 2025 22:32

clean

6257385

Merge branch 'develop' into fb_domainAudit

724904e

switch to use domain.delete for study dataset delete

4842438

fix regex/validator, add selenium tests

ca526b9

XingY mentioned this pull request May 13, 2025

Selenium tests for domain audit details LabKey/testAutomation#2445

Merged

XingY added 6 commits May 13, 2025 09:18

fix dataset delete

0dd74aa

fix dataset delete

b255592

update tests

850e993

fix folder exclusion audit

987dbcd

fix audit detail for field format change

43cc174

selenium tests

cf08a84

labkey-nicka self-requested a review May 14, 2025 17:01

labkey-nicka assigned XingY May 14, 2025

labkey-nicka reviewed May 14, 2025

View reviewed changes

XingY added 8 commits May 14, 2025 13:56

audit unique indices update

24db680

code review changes

95af4ec

switch to use old and new record map for domain event details

2f33703

populate new record map during domain creation

485de76

clean

782f5ab

clean

8c721f2

Merge branch 'develop' into fb_domainAudit

51b623a

Merge branch 'develop' into fb_domainAudit

39237ce

# Conflicts: # biologics/package-lock.json # biologics/package.json # inventory/package-lock.json # inventory/package.json # sampleManagement/package-lock.json # sampleManagement/package.json

XingY requested a review from labkey-nicka May 16, 2025 23:35

XingY and others added 4 commits May 19, 2025 00:17

fixes and update tests

59d7f37

Merge branch 'develop' into fb_domainAudit

dcc884a

update tests

301f215

Merge branch 'develop' into fb_domainAudit

c28d300

# Conflicts: # api/src/org/labkey/api/exp/PropertyDescriptor.java

labkey-nicka approved these changes May 20, 2025

View reviewed changes

XingY added 10 commits May 21, 2025 00:10

support calculated field audit changes, genId update audit, update/ad…

b2bd762

…d tests

revert isCalculatedColumn getter

606bdd7

Fix text -> flag field type conversion audit

a5b72b6

add more tests

ec5aef7

fix more tests

423c00f

Issue 53108: Max length of Aliquot Naming Pattern is less than Naming…

1ffc59d

… Pattern and can be hit quickly with unicode characters

code review changes

92ac24f

refactor

c26f270

Merge branch 'develop' into fb_domainAudit

fcea799

# Conflicts: # core/package-lock.json # core/package.json # experiment/package-lock.json # experiment/package.json

Merge branch 'develop' into fb_domainAudit

a44b1ce

# Conflicts: # biologics/package-lock.json # biologics/package.json # inventory/package-lock.json # inventory/package.json # sampleManagement/package-lock.json # sampleManagement/package.json

XingY mentioned this pull request May 23, 2025

Remove container argument from AbstractAuditTypeProvider.encodeForDataMap LabKey/wnprc-modules#827

Merged

XingY added 4 commits May 23, 2025 12:47

Merge branch 'develop' into fb_domainAudit

e1ed9e3

Merge branch 'develop' into fb_domainAudit

410a9c9

publish

7de2e20

XingY merged commit 6af0a05 into develop May 24, 2025
6 of 13 checks passed

XingY deleted the fb_domainAudit branch May 24, 2025 01:11

XingY mentioned this pull request May 28, 2025

Fix detailedAuditChanges action link #6710

Merged


		appendValueMapColumns(table);

		DetailsURL url = DetailsURL.fromString("audit-detailedAuditChanges.view?auditRowId=${rowId}&auditEventType=" + EVENT_NAME);

Conversation

XingY commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Related Pull Requests

Changes

Uh oh!

labkey-nicka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

XingY commented May 16, 2025

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!

labkey-nicka commented May 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

XingY commented May 12, 2025 •

edited

Loading