Allow studies to prevent new timepoint creation on data import. by labkey-klum · Pull Request #5075 · LabKey/platform

labkey-klum · 2023-12-21T00:25:08Z

Rationale

Studies will automatically create new timepoints when data (dataset or specimen) is introduced into the study. In limited situations clients may not want this to happen. Years ago we added a folder import check for visit based studies only to fail if the data would introduce any new visits.

This PR takes it a step farther by introducing a study wide setting to disallow the creation of new timepoints for all of the pathways that data can flow into a study:

Dataset import (bulk, single row, API)
Link to study
Folder import
Specimen import
Publish and ancillary study
REDCap or CDISC integration

The new configuration UI is under the manage visit/timepoint link off of the manage study page.

Changes

Support both visit and date based studies
Respect the configuration. StudyManager.ensureVisits provides a single choke point to allow/disallow the timepoint creation, but provided no way to report back errors. The method now accepts a parameter to fail for undefined timepoints and will return a ValidationException to indicate any errors. There were many code paths into this method that needed to thread through the parameter as well as consume the return value where appropriate.
Round trip the setting for folder export/import. When importing an export of a study that had no visit map and had the fail setting enabled, the behavior is to allow the import of the data but then set the fail flag at the end of the import. Otherwise it would be impossible to import one of these types of exports. Publish and ancillary behaves similarly.

…eation of new timepoints if the study is configured as such

…set imports, to the newer universal one

…onsistently.

github-actions · 2023-12-21T00:25:25Z

WARNING: This PR appears to have the default title generated by GitHub. Please use something more descriptive.

labkey-klum · 2023-12-21T00:29:08Z

                isFailForUndefinedVisits: <%=bean.isFailForUndefinedVisits()%>,
                isCloudRoot: <%=bean.isCloudRoot()%>,   // Remove as part of Issue #43835
-                showFailForUndefinedVisits: <%=timepointType == null || timepointType == TimepointType.VISIT%>
+                showFailForUndefinedVisits: <%=(timepointType == null || timepointType == TimepointType.VISIT) && ((study == null) || !study.isFailForUndefinedTimepoints())%>


This hides the legacy advanced folder import UI to fail for undefined visits if this is already configured at the study level.

labkey-klum · 2023-12-21T00:31:02Z

-            Study study = StudyService.get().getStudy(getContainer());
-            if (loadInfo.getRowCount() > 0 && failForUndefinedVisits && study.getTimepointType() == TimepointType.VISIT)
-                checkForUndefinedVisits(loadInfo, study);
-


This is the legacy folder import check, the new check is below in resyncStudy

labkey-klum · 2023-12-21T00:38:20Z

            {
-                result = StudyManager.getInstance().ensureVisit(this, null, sequenceNumBD, null, false);
+                result = StudyManager.getInstance().getVisit(this, null, sequenceNumBD, null);
            }


Change this to getVisit; the last parameter was saveIfNew and this was the only usage of that method. Got rid of the old method.

labkey-klum · 2023-12-21T00:41:22Z

                {
-                    List<Double> undefinedSequenceNums = StudyManager.getInstance().getUndefinedSequenceNumsForDataset(_datasetDefinition.getContainer(), _datasetDefinition.getDatasetId());
-                    if (!undefinedSequenceNums.isEmpty())
+                    // check for undefined visits and don't commit the data if there are errors


Swap out the legacy check for the newer check.

labkey-klum · 2023-12-21T00:43:59Z

-                        <labkey:input type="radio" name="timepointType" id="continuousTimepointType" value="<%=TimepointType.CONTINUOUS%>" checked="<%=(form.getTimepointType() == TimepointType.CONTINUOUS)%>" onChange="document.getElementById('defaultDurationRow').style.display = document.getElementById('dateTimepointType').checked ? 'table-row' : 'none'; document.getElementById('defaultDateRow').style.display = document.getElementById('continuousTimepointType').checked ? 'none' : 'table-row';" /> Continuous
+                        <labkey:input type="radio" formGroup="false" name="timepointType" id="dateTimepointType" value="<%=TimepointType.DATE%>" checked="<%=(form.getTimepointType() == TimepointType.DATE)%>" onChange="document.getElementById('defaultDurationRow').style.display = document.getElementById('dateTimepointType').checked ? 'table-row' : 'none'; document.getElementById('defaultDateRow').style.display = document.getElementById('continuousTimepointType').checked ? 'none' : 'table-row';" /> Dates &nbsp;&nbsp;
+                        <labkey:input type="radio" formGroup="false" name="timepointType" value="<%=TimepointType.VISIT%>" checked="<%=(form.getTimepointType() == TimepointType.VISIT || form.getTimepointType() == null)%>" onChange="document.getElementById('defaultDurationRow').style.display = document.getElementById('dateTimepointType').checked ? 'table-row' : 'none'; document.getElementById('defaultDateRow').style.display = document.getElementById('continuousTimepointType').checked ? 'none' : 'table-row';" /> Assigned Visits  &nbsp;&nbsp;
+                        <labkey:input type="radio" formGroup="false" name="timepointType" id="continuousTimepointType" value="<%=TimepointType.CONTINUOUS%>" checked="<%=(form.getTimepointType() == TimepointType.CONTINUOUS)%>" onChange="document.getElementById('defaultDurationRow').style.display = document.getElementById('dateTimepointType').checked ? 'table-row' : 'none'; document.getElementById('defaultDateRow').style.display = document.getElementById('continuousTimepointType').checked ? 'none' : 'table-row';" /> Continuous


Unrelated to this ticket but the create study page layout was off.

FYI: I fixed this in a PR earlier today 😄 https://github.com/LabKey/platform/pull/5071/files#diff-d865ecd1f5c16055018efd9c61efa77f323780055e97efe390e52411199d6c33

Great, I'll merge develop into my branch to pick up your change.

labkey-klum · 2023-12-21T00:45:32Z

     * @param potentiallyDeletedParticipants optionally, the specific participants that may have been removed from the
     * study. If null, all participants will be checked to see if they are still in the study.
     * @param participantVisitResyncRequired If true, will force an update of the ParticipantVisit mapping for this study
-     * @param forceUpdateCohorts If true, forces updateParticipantCohorts() (specimen import case)


forceUpdateCohorts was never used.

labkey-klum · 2023-12-21T00:47:19Z

        clickAndWait(Locator.linkWithText(CALC_COL_QUERY_SNAPSHOT));
-        waitForSnapshotUpdate("-2");
+        waitForSnapshotUpdate("999151515");



Test was failing because the QuerySnapshot was being deleted before the snapshot had a chance to update itself.

cnathe · 2023-12-22T14:24:29Z

-            if (target.getDefaultTimepointDuration() < 1)
-                errors.reject(ERROR_MSG, "Default timepoint duration must be a positive number.");
+            StudyImpl study = getStudy();
+            if (study.getTimepointType() == TimepointType.DATE)


curious why we are checking the timepoint type for the study and the target props form object (one here and one a few lines below). would they really ever differ?

Good question, both JSP's (manageVisits.jsp, manageTimepoints.jsp) now use the same controller endpoint. I had to make it a little more fault tolerant of the values getting posted. Perhaps I should make it clearer to the endpoint what type of information is being posted.

cnathe · 2023-12-22T14:27:09Z

+
+                // after the data has been imported, configure the new study setting for undefined timepoints
+                if (sourceStudy.isFailForUndefinedTimepoints())
+                    mutableStudy.setFailForUndefinedTimepoints(true);


good call on needing to apply this after the visit map is created for the new study (as mentioned in the PR message).

labkey-klum added 11 commits December 14, 2023 13:53

layout fix

e38b186

placeholder upgrade scripts

6725e4e

Merge branch 'develop' into fb_issue_49069

28fd730

refactor methods which call ensureVisits to support preventing the cr…

f004f0f

…eation of new timepoints if the study is configured as such

Round trip undefined visits setting.

d1ca647

don't leak a server exception

695f178

consolidate previous checks for undefined visits in specimen and data…

d88e329

…set imports, to the newer universal one

Improve description

24903b3

Merge branch 'develop' into fb_issue_49069

ffde407

automated test fixes

474265f

Support for date based studies, handle errors in folder import more c…

74a339d

…onsistently.

labkey-klum commented Dec 21, 2023

View reviewed changes

clarify javadoc

1aea680

labkey-klum commented Dec 21, 2023

View reviewed changes

labkey-klum changed the title ~~Fb issue 49069~~ Allow studies to prevent new timepoint creation on data import. Dec 21, 2023

labkey-klum linked an issue Dec 21, 2023 that may be closed by this pull request

Issue 49069: Ability to require that visits exist before entering data for them #5050

Closed

30 tasks

labkey-klum requested review from cnathe and labkey-matthewb December 21, 2023 01:03

merge from develop

14b79b7

cnathe approved these changes Dec 22, 2023

View reviewed changes

labkey-tchad and others added 3 commits December 27, 2023 09:04

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

117ae63

Fix pluralization in error message

d9a3ea6

Bug fixes from manual testing

368beb8

labkey-klum merged commit 9031621 into develop Dec 29, 2023

labkey-klum deleted the fb_issue_49069 branch December 29, 2023 21:13

labkey-klum mentioned this pull request Jan 4, 2024

fixes for automated test failures #5100

Merged

Conversation

labkey-klum commented Dec 21, 2023

Rationale

Changes

Uh oh!

github-actions Bot commented Dec 21, 2023

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants