DT-3047: Save DAA IDs when submitting a DAR or PR#2850
Conversation
rushtong
requested review from
Copilot,
kevinmarete and
otchet-broad
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
Adds persistence of Data Access Agreement (DAA) IDs into the stored JSON “data package” for Data Access Requests (DARs) and Progress Reports (PRs), so downstream workflows can reference the DAAs that applied at submission time.
Changes:
- Add
daaIdstoDataAccessRequestDataand populate it during DAR submission and PR submission. - Introduce
DaaDAO.findDaaIdsByDatasetIds(...)to resolve DAAs from dataset IDs via DAC associations. - Extend unit/integration tests to cover the new DAO query and (partially) the new service behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/broadinstitute/consent/http/service/DataAccessRequestService.java | Sets daaIds on DAR/PR submission by querying DAAs for relevant dataset IDs. |
| src/main/java/org/broadinstitute/consent/http/models/DataAccessRequestData.java | Adds List<Integer> daaIds with getter/setter for JSON persistence. |
| src/main/java/org/broadinstitute/consent/http/db/DaaDAO.java | Adds SQL query to fetch DAA IDs for a list of dataset IDs. |
| src/main/java/org/broadinstitute/consent/http/db/DAOContainer.java | Wires DaaDAO into the shared DAO container. |
| src/main/java/org/broadinstitute/consent/http/ConsentModule.java | Provides DaaDAO into DAOContainer for DI construction of services. |
| src/test/java/org/broadinstitute/consent/http/service/DataAccessRequestServiceTest.java | Updates service tests to include DaaDAO wiring and verify DAA lookup on DAR submission. |
| src/test/java/org/broadinstitute/consent/http/models/DataAccessRequestDataTest.java | Adds extensive getter/setter coverage including daaIds. |
| src/test/java/org/broadinstitute/consent/http/db/DaaDAOTest.java | Adds integration tests for findDaaIdsByDatasetIds behavior across association scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/org/broadinstitute/consent/http/service/DataAccessRequestService.java
Outdated
Show resolved
Hide resolved
src/test/java/org/broadinstitute/consent/http/service/DataAccessRequestServiceTest.java
Show resolved
Hide resolved
src/test/java/org/broadinstitute/consent/http/models/DataAccessRequestDataTest.java
Show resolved
Hide resolved
src/main/java/org/broadinstitute/consent/http/models/DataAccessRequestData.java
Outdated
Show resolved
Hide resolved
| List<Integer> datasetIds = dataAccessRequest.getDatasetIds(); | ||
|
|
||
| // Save the agreed upon DAAs at the time of DAR submission | ||
| List<Integer> daaIds = daaDAO.findDaaIdsByDatasetIds(datasetIds); |
There was a problem hiding this comment.
Could we add the DAA IDs that were agreed to to the DataAccessRequestData object and store those values?
We have other tickets to verify the value in DataAccessRequestData contains the full list needed. I expect that we'll call something in DataAccessRequestService on submission (not draft) that verifies the list contains the needed elements or we'll reject the creation.
There was a problem hiding this comment.
When a DAR comes into the service layer, there are no DAA id values in the DataAccessRequestData. The UI does not populate that value in the POST request. A different approach would be to populate the values in the DatasetService.enforceDAARestrictions method, but that would be overloading (in the colloquial sense) that method's intention.
There was a problem hiding this comment.
I think we should add the DAAs to the DataAccessRequestData object so we know what IDs were loaded when the client submitted the DARs. If we just add them at submission time, how can we be sure they were actually reviewed by the client?
There was a problem hiding this comment.
Per conversation, we're removing the explicit set values and relying on the UI to populate correct values.
|
|
||
| // Save the agreed upon DAAs at the time of PR submission | ||
| DataAccessRequestData darData = progressReport.getData(); | ||
| List<Integer> daaIds = daaDAO.findDaaIdsByDatasetIds(new ArrayList<>(progressReportDatasetIds)); |
There was a problem hiding this comment.
Same here. Why not just save the DAAs provided in DataAccessRequestData?
|
Thanks for the updates! Looks good! |
|
Addresses
https://broadworkbench.atlassian.net/browse/DT-3047
Summary
This PR adds the DAA IDs for the datasets in a DAR or PR when the entity is saved. These ids are stored in the DAR's json data package and can be referred to in other workflows.
Have you read CONTRIBUTING.md lately? If not, do that first.