[DT-1520] DAR Expiration. #2491

otchet-broad · 2025-04-21T21:18:51Z

Addresses

https://broadworkbench.atlassian.net/browse/DT-1520

Summary

Consent had two state machines for determining if a Data Access Request was in draft form or submitted for a Data Access Committee to consider for approval. This PR aligns the code so that only one state machine exists. It is now centered on submission_date.

When submission_date is non-null, the DAR has been submitted by a RESEARCHER to a DATA ACCESS COMMITTEE for consideration. When the submission_date is null, it is considered a draft. This was the behavior in the existing code paths except for some tests that leveraged methods to short circuit that requirement.

The DataAccessRequest class now computes the fields draft, expired, and expiredAt fields when the submissionDate is set. These three values are now returned to the UI for enrichment.

~~Of critical importance is the update to DataAccessRequestDAO.findApprovedDARsByDatasetId that now requires DARs to be submitted within the last year in order to be considered "approved."~~ <- This was already completed in #2487

Also added in DataAccessRequestDAO is findSubmittedDarsByTimeRange. This method is intended to be used by a to-be-built notification component that can alert users to expiring DARs. It allows the caller to specify the date range of approved DARs so that notifications can be sent. For example, give me the list of DARs that will expire 30 days from today. Those DARs would have a submission_date during the time of 1745193600.000 to 1745279999.999

To simplify our codebase, other test only methods that were impacting draft state were removed.

Have you read CONTRIBUTING.md lately? If not, do that first.

Label PR with a Jira ticket number and include a link to the ticket
Label PR with a security risk modifier [no, low, medium, high]
PR describes scope of changes
Get a minimum of one thumbs worth of review, preferably two if enough team members are available
Get PO sign-off for all non-trivial UI or workflow changes
Verify all tests go green
Test this change deployed correctly and works on dev environment after deployment

…mpute DAR status based on database field submission_date; don't consider DARs approved after one year; demonstrate how to find expiring DARs.

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

otchet-broad · 2025-04-21T22:53:59Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

+                dd.dataset_id
+              FROM data_access_request dar
+              LEFT JOIN dar_dataset dd ON dd.reference_id = dar.reference_id
+              WHERE dar.submission_date BETWEEN SYMMETRIC to_timestamp(:begin) AND to_timestamp(:end)


Now we can support time range queries for submitted DARs.

src/main/java/org/broadinstitute/consent/http/db/DarCollectionSummaryDAO.java

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

…e payload.

rushtong

Love the removal of draft. It would be interesting (not for this PR) to see if we could do something similar for the status field in data.

src/main/java/org/broadinstitute/consent/http/db/DarCollectionDAO.java

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

s-rubenstein

Where possible, could you drop comments where the SQL text blocks are logically the same? It looks like a bunch of this is solely formatting changes but I am not sure which is functional and which is format

src/main/java/org/broadinstitute/consent/http/models/DataAccessRequest.java

otchet-broad · 2025-04-23T14:30:21Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

+          SELECT dd.dataset_id, dar.id, dar.reference_id, dar.collection_id, dar.parent_id, dar.user_id, dar.create_date, dar.sort_date, dar.submission_date, dar.update_date,
+            (regexp_replace(dar.data #>> '{}', '\\\\u0000', '', 'g'))::jsonb AS data FROM data_access_request dar
+          LEFT JOIN dar_dataset dd on dd.reference_id = dar.reference_id 
+          WHERE dar.submission_date is not null


switching from draft to submission date

Thanks for these comments, they were very helpful!

otchet-broad · 2025-04-23T14:31:00Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-          AND dar.submission_date > now() - interval '1 year'
-      """)
+  @SqlQuery(
+      """


remove dar.draft + formatting

otchet-broad · 2025-04-23T14:31:24Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-          + "  WHERE dar.draft = true "
-          + "  AND (LOWER(dar.data->>'status') != 'archived' OR dar.data->>'status' IS NULL) "
-          + "  ORDER BY dar.update_date DESC")
+      """


Remove dar.draft + formatting

otchet-broad · 2025-04-23T14:31:49Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-          + "  AND (LOWER(dar.data->>'status') != 'archived' OR dar.data->>'status' IS NULL) "
-          + "  AND dar.user_id = :userId "
-          + "  ORDER BY dar.sort_date DESC")
+      """


remove dar.draft + Formatting.

otchet-broad · 2025-04-23T14:32:27Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-          + "  AND dar.user_id = :userId "
-          + "  AND (LOWER(dar.data->>'status') != 'archived' OR dar.data->>'status' IS NULL) "
-          + "  ORDER BY dar.sort_date DESC")
+      """


Remove dar.draft + formatting

otchet-broad · 2025-04-23T14:32:50Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-          + "SET data = to_jsonb(regexp_replace(:data, '\\\\u0000', '', 'g')), user_id = :userId, sort_date = :sortDate, "
-          + "submission_date = :submissionDate, update_date = :updateDate "
-          + "WHERE reference_id = :referenceId")
+      """


formatting.

otchet-broad · 2025-04-23T14:33:01Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

   * @param referenceId String
   */
-  @SqlUpdate("DELETE FROM data_access_request WHERE reference_id = :referenceId")
+  @SqlUpdate(


otchet-broad · 2025-04-23T14:33:19Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-      "UPDATE data_access_request dar "
-          + "SET data=jsonb_set((regexp_replace(dar.data #>> '{}', '\\\\u0000', '', 'g'))::jsonb, '{status}', '\"Canceled\"') "
-          + "WHERE reference_id IN (<referenceIds>)")
+      """


Formatting.

otchet-broad · 2025-04-23T14:33:59Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

   * @param collectionId Integer
   */
-  @SqlUpdate("DELETE FROM data_access_request WHERE collection_id = :collectionId")
+  @SqlUpdate(


Formatting.

otchet-broad · 2025-04-23T14:34:34Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-          + "VALUES (:referenceId, :userId, :createDate, :sortDate, "
-          + ":submissionDate, :updateDate, to_jsonb(:data), true)")
+      """
+          INSERT INTO data_access_request


remove draft.

src/main/java/org/broadinstitute/consent/http/models/DataAccessRequest.java

otchet-broad · 2025-04-23T14:43:59Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-          + "(collection_id, reference_id, user_id, create_date, sort_date, submission_date, update_date, data) "
-          + "VALUES (:collectionId, :referenceId, :userId, :createDate, :sortDate, " +
-          ":submissionDate, :updateDate, to_jsonb(:data))")
+      """


Formatting.

otchet-broad · 2025-04-23T14:44:17Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-  void updateDraftForCollection(@Bind("collectionId") Integer collectionId,
+      """
+          UPDATE data_access_request
+            SET  submission_date = now(), collection_id = :collectionId


remove the use of draft

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

pshapiro4broad · 2025-04-23T14:47:00Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

+              LEFT JOIN dar_dataset dd ON dd.reference_id = dar.reference_id
+              WHERE dar.submission_date BETWEEN SYMMETRIC :begin AND :end
+          """)
+  List<DataAccessRequest> findSubmittedDarsByTimeRange(@Bind("begin") Timestamp begin,


Why is this API being added? I don't see it used anywhere in this PR other than tests.

Before when I asked about SYMMETRIC, my question was really about how do we know that this needs to support out-of-order queries? Without an example of how it will be used in code, it's hard to tell.

The ticket includes an AC of "Mechanism to find expired DARs"
This is an example of a "Mechanism to find expired DARs"

If the only need is to find expired, could it be done with a query that has a single bound? Adding the service method that uses this API (e.g. DataAccessRequestService.findExpiredDars()) would help me understand how this API works to meet the AC.

Is the idea that, instead of keeping track of which DARs we've notified users of, we instead find only those that expired in a specific time range? That could work but we'd need to be very precise about when the check is done, to avoid missing some DARs or notifying twice for others.

Exactly. As discussed at standup this morning, we have a ticket to notify DAR submitters when their DAR will expire in 30 days (from today). We'll know the starting time and ending time of the query range. That's the use case I was specifically thinking about.
Do others exist? Sure.
Are they in other tickets? Yes.
Do I expect this to change? Maybe. It's just an example.

otchet-broad · 2025-04-23T14:51:18Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-          + "FROM data_access_request "
-          + "WHERE (LOWER(data->>'status') != 'archived' "
-          + "OR data->>'status' IS NULL)")
+      """


otchet-broad · 2025-04-23T14:51:37Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-          + " SET data = jsonb_set ((data #>> '{}')::jsonb, '{status}', '\"Archived\"', true) "
-          + " WHERE reference_id IN (<referenceIds>)")
+      """
+         UPDATE data_access_request


otchet-broad · 2025-04-23T14:51:47Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-          + "VALUES (:referenceId, :datasetId) "
-          + "ON CONFLICT DO NOTHING")
+      """
+          INSERT INTO dar_dataset (reference_id, dataset_id)


Formatting.

otchet-broad · 2025-04-23T14:51:59Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-          + "VALUES (:referenceId, :datasetId) "
-          + "ON CONFLICT DO NOTHING")
+      """
+          INSERT INTO dar_dataset (reference_id, dataset_id)


otchet-broad · 2025-04-23T14:52:20Z

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java

-  @SqlUpdate("DELETE FROM dar_dataset WHERE reference_id = :referenceId")
+  @SqlUpdate(
+      """
+          DELETE FROM dar_dataset WHERE reference_id = :referenceId


Formatting.

otchet-broad · 2025-04-23T14:53:29Z

src/main/java/org/broadinstitute/consent/http/models/DarCollection.java

      "dar.id AS dar_id, dar.reference_id AS dar_reference_id, dar.collection_id AS dar_collection_id, "
          +
-          "dar.parent_id AS dar_parent_id, dar.draft AS dar_draft, dar.user_id AS dar_userId, " +
+          "dar.parent_id AS dar_parent_id, dar.user_id AS dar_userId, " +


remove draft

otchet-broad · 2025-04-23T14:53:50Z

src/main/java/org/broadinstitute/consent/http/models/DarCollection.java

    return datasets;
  }

+  public void setDatasets(Set<Dataset> datasets) {


Formatting.

pshapiro4broad

Some possible changes; also I would like to avoid storing computed values if that's possible here. But looks OK overall.

src/main/java/org/broadinstitute/consent/http/models/DataAccessRequest.java

src/main/java/org/broadinstitute/consent/http/models/DarCollectionSummary.java

src/main/resources/assets/schemas/DataAccessRequest.yaml

src/test/java/org/broadinstitute/consent/http/db/DataAccessRequestDAOTest.java

…ests; updated swagger based on this change.

otchet-broad added 5 commits April 21, 2025 16:37

Refactor to remove database property draft on data_access_request; co…

5184bfb

…mpute DAR status based on database field submission_date; don't consider DARs approved after one year; demonstrate how to find expiring DARs.

Merge branch 'develop' into otchet-dt-1520-add-dar-expired-status

eed3c8f

Add database index for submission_date.

fd35724

Reformat SQL to make semgrep happy.

0e2719e

More SQL formatting.

98fbe16

otchet-broad commented Apr 21, 2025

View reviewed changes

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java Outdated Show resolved Hide resolved

otchet-broad commented Apr 21, 2025

View reviewed changes

otchet-broad added 3 commits April 21, 2025 18:55

Fix comment.

51b943e

Fix flaky test.

cb83c7f

Adjusting other query ranges for tests.

e366aa7

otchet-broad marked this pull request as ready for review April 21, 2025 23:40

otchet-broad requested a review from a team as a code owner April 21, 2025 23:40

otchet-broad requested review from rushtong and s-rubenstein and removed request for a team April 21, 2025 23:40

rushtong reviewed Apr 22, 2025

View reviewed changes

src/main/java/org/broadinstitute/consent/http/db/DarCollectionSummaryDAO.java Outdated Show resolved Hide resolved

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java Outdated Show resolved Hide resolved

Add expired and expiresAt indicators, update swagger to match respons…

f39b9fd

…e payload.

rushtong approved these changes Apr 22, 2025

View reviewed changes

Consolidate duplicate SQL date range in queries.

a1a6f69

pshapiro4broad reviewed Apr 22, 2025

View reviewed changes

src/main/java/org/broadinstitute/consent/http/db/DarCollectionDAO.java Show resolved Hide resolved

pshapiro4broad reviewed Apr 22, 2025

View reviewed changes

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java Outdated Show resolved Hide resolved

pshapiro4broad reviewed Apr 22, 2025

View reviewed changes

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java Outdated Show resolved Hide resolved

otchet-broad added 2 commits April 22, 2025 10:06

Reformatting SQL with indentation.

8c7a751

PR feedback: Request to use a Java object instead of double.

e4c42bf

otchet-broad requested review from fboulnois, pshapiro4broad and rjohanek April 22, 2025 16:18

otchet-broad mentioned this pull request Apr 23, 2025

[DT-1545] Block submitted dars from being modified. #2495

Merged

s-rubenstein reviewed Apr 23, 2025

View reviewed changes

pshapiro4broad reviewed Apr 23, 2025

View reviewed changes

src/main/java/org/broadinstitute/consent/http/models/DataAccessRequest.java Outdated Show resolved Hide resolved

otchet-broad commented Apr 23, 2025

View reviewed changes

pshapiro4broad reviewed Apr 23, 2025

View reviewed changes

src/main/java/org/broadinstitute/consent/http/models/DataAccessRequest.java Outdated Show resolved Hide resolved

pshapiro4broad reviewed Apr 23, 2025

View reviewed changes

src/main/java/org/broadinstitute/consent/http/models/DataAccessRequest.java Outdated Show resolved Hide resolved

s-rubenstein approved these changes Apr 23, 2025

View reviewed changes

otchet-broad commented Apr 23, 2025

View reviewed changes

pshapiro4broad reviewed Apr 23, 2025

View reviewed changes

src/main/java/org/broadinstitute/consent/http/db/DataAccessRequestDAO.java Show resolved Hide resolved

pshapiro4broad reviewed Apr 23, 2025

View reviewed changes

otchet-broad commented Apr 23, 2025

View reviewed changes

pshapiro4broad approved these changes Apr 23, 2025

View reviewed changes

otchet-broad added 3 commits April 23, 2025 12:33

Use TimeUnit.

d733948

Isoleated refactoring to use Timestamp for expiresAt value; updated t…

bbfbe71

…ests; updated swagger based on this change.

Refactor date manipulations in tests to make it clearer.

c2da143

otchet-broad merged commit beb8e10 into develop Apr 23, 2025
14 checks passed

otchet-broad deleted the otchet-dt-1520-add-dar-expired-status branch April 23, 2025 19:05

[DT-1520] DAR Expiration. #2491

[DT-1520] DAR Expiration. #2491

Uh oh!

Conversation

otchet-broad commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Addresses

Summary

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rushtong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

s-rubenstein left a comment

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!

otchet-broad Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

pshapiro4broad Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pshapiro4broad left a comment

otchet-broad commented Apr 21, 2025 •

edited

Loading

otchet-broad Apr 23, 2025 •

edited

Loading

pshapiro4broad Apr 23, 2025 •

edited

Loading