Skip to content

Test component audit event row count and diff count checks for Sample and Source delete and move#2481

Merged
cnathe merged 12 commits intodevelopfrom
fb_auditDiffCheckDeleteMove
Jun 13, 2025
Merged

Test component audit event row count and diff count checks for Sample and Source delete and move#2481
cnathe merged 12 commits intodevelopfrom
fb_auditDiffCheckDeleteMove

Conversation

@cnathe
Copy link
Contributor

@cnathe cnathe commented Jun 10, 2025

Rationale

Expanding on recent AuditLogHelper checks for audit event row/diff counts on actions like sample/source bulk update and details update, this PR adds similar checks within the test components for deleting and moving samples and sources.

Related Pull Requests

Changes

  • ModalDialog: Added a new method getCountFromTitle(String prefix) to extract a numeric count from the dialog's title based on a given prefix. This method handles potential parsing errors gracefully by returning null if the number cannot be parsed.
  • DeleteConfirmationDialog: Modified the confirmDelete() method to include an overloaded version that accepts a skipAuditEventCheck parameter, allowing optional verification of audit events during delete confirmation.
  • DeleteConfirmationDialog: Added a static helper method verifyAuditEvents() to validate audit event counts for the last transaction, ensuring consistency between the expected and actual audit log entries. This method handles exceptions and throws a runtime error if validation fails.


var confirmPage = _confirmationSynchronizationFunction.apply(() -> this.dismiss("Yes, Delete", waitSeconds));

if (!skipAuditEventCheck && count != null && auditEventName != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried about unknowingly losing this coverage if our count or eventName calculations start failing at some point.
We should be strict when we're know we can be. Maybe make an EntityDeleteConfirmationDialog subclass that throws an error if getCountFromTitle or getAuditEventNameFromURL are null. Then switch as many places as possible to use that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agreed. I had similar thoughts. I was hoping to put the verifyAuditEvents checks into the SampleDeleteDialog and SourceDeleteDialog for this reason but there are so many usages of DeleteConfirmationDialog directly that don't use those subclasses. Maybe we can sync to discuss this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a starting point, I added a null check to the SampleDeleteDialog and SourceDeleteDialog with this commit:
https://github.com/LabKey/limsModules/pull/1461/commits/9a0712d69e990ecb8e688e86b573d6f5d022e581

@cnathe cnathe merged commit 9a5ad41 into develop Jun 13, 2025
5 of 6 checks passed
@cnathe cnathe deleted the fb_auditDiffCheckDeleteMove branch June 13, 2025 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants