[DT-3063, DT-3065] Store relationship between DARs/Progress Reports and DAAs, Flag DARs/PRs for SO approval when user not pre authorized for DAAs #2859
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,10 @@ | |
|
|
||
| import java.time.Instant; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import org.broadinstitute.consent.http.db.mapper.DaaAuditMapper; | ||
| import org.broadinstitute.consent.http.db.mapper.DaaDatasetReducer; | ||
| import org.broadinstitute.consent.http.db.mapper.DaaMapper; | ||
| import org.broadinstitute.consent.http.db.mapper.DataAccessAgreementReducer; | ||
| import org.broadinstitute.consent.http.db.mapper.FileStorageObjectMapper; | ||
|
|
@@ -15,6 +17,7 @@ | |
| import org.jdbi.v3.sqlobject.customizer.Bind; | ||
| import org.jdbi.v3.sqlobject.customizer.BindList; | ||
| import org.jdbi.v3.sqlobject.customizer.BindList.EmptyHandling; | ||
| import org.jdbi.v3.sqlobject.statement.SqlBatch; | ||
| import org.jdbi.v3.sqlobject.statement.SqlQuery; | ||
| import org.jdbi.v3.sqlobject.statement.SqlUpdate; | ||
| import org.jdbi.v3.sqlobject.statement.UseRowReducer; | ||
|
|
@@ -299,4 +302,26 @@ WHERE dataset.dataset_id IN (<datasetIds>) | |
| Set<Integer> findDaaIdsByDatasetIds( | ||
| @BindList(value = "datasetIds", onEmpty = EmptyHandling.NULL_STRING) | ||
| List<Integer> datasetIds); | ||
|
|
||
| @SqlQuery( | ||
| """ | ||
| SELECT daa.daa_id, dataset.dataset_id | ||
| FROM data_access_agreement daa | ||
| INNER JOIN dac_daa ON daa.daa_id = dac_daa.daa_id | ||
| INNER JOIN dac ON dac.dac_id = dac_daa.dac_id | ||
| INNER JOIN dataset ON dataset.dac_id = dac.dac_id | ||
| WHERE dataset.dataset_id IN (<datasetIds>) | ||
| GROUP BY daa.daa_id, dataset.dataset_id | ||
| ORDER BY daa.daa_id | ||
| """) | ||
| @UseRowReducer(DaaDatasetReducer.class) | ||
| Map<Integer, Set<Integer>> mapDaaIdsToDatasetIds( | ||
| @BindList(value = "datasetIds", onEmpty = EmptyHandling.NULL_STRING) Set<Integer> datasetIds); | ||
|
|
||
| @SqlBatch( | ||
| """ | ||
| INSERT INTO dar_daa (dar_id, daa_id) VALUES (:darId, :daaId) | ||
| ON CONFLICT DO NOTHING | ||
| """) | ||
| void insertDarDAARelationship(@Bind("darId") Integer darId, @Bind("daaId") Set<Integer> daaIds); | ||
|
Contributor
There was a problem hiding this comment. Copilot caught this for me - I was unaware of this detail. See current jdbi doc for reference.
Contributor
Author
There was a problem hiding this comment. I want to spend some time looking at this because I think I wrote a test that ends up proving this works. See
Contributor
Author
There was a problem hiding this comment. I've updated the method and added the |
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| package org.broadinstitute.consent.http.db.mapper; | ||
|
|
||
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
| import java.util.Map; | ||
| import java.util.Map.Entry; | ||
| import java.util.Set; | ||
| import java.util.stream.Stream; | ||
| import org.jdbi.v3.core.result.RowReducer; | ||
| import org.jdbi.v3.core.result.RowView; | ||
|
|
||
| public class DaaDatasetReducer | ||
| implements RowReducer<Map<Integer, Set<Integer>>, Map.Entry<Integer, Set<Integer>>> { | ||
|
|
||
| @Override | ||
| public Map<Integer, Set<Integer>> container() { | ||
| return new HashMap<>(); | ||
| } | ||
|
|
||
| @Override | ||
| public void accumulate(Map<Integer, Set<Integer>> container, RowView rowView) { | ||
| int daaId = rowView.getColumn("daa_id", Integer.class); | ||
| Integer datasetId = rowView.getColumn("dataset_id", Integer.class); | ||
| container.computeIfAbsent(daaId, k -> new HashSet<>()).add(datasetId); | ||
| } | ||
|
|
||
| @Override | ||
| public Stream<Entry<Integer, Set<Integer>>> stream(Map<Integer, Set<Integer>> container) { | ||
|
Contributor
There was a problem hiding this comment. This looks super useful 👍🏽 |
||
| return container.entrySet().stream(); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,9 +22,11 @@ | |
| import java.util.Objects; | ||
| import java.util.Set; | ||
| import java.util.UUID; | ||
| import java.util.stream.Collectors; | ||
| import org.apache.commons.validator.routines.EmailValidator; | ||
| import org.broadinstitute.consent.http.configurations.ConsentConfiguration; | ||
| import org.broadinstitute.consent.http.db.DAOContainer; | ||
| import org.broadinstitute.consent.http.db.DaaDAO; | ||
| import org.broadinstitute.consent.http.db.DarCollectionDAO; | ||
| import org.broadinstitute.consent.http.db.DataAccessRequestDAO; | ||
| import org.broadinstitute.consent.http.db.DatasetDAO; | ||
|
|
@@ -74,6 +76,7 @@ public class DataAccessRequestService implements ConsentLogger { | |
| private static final String LAB_STAFF = "Lab staff"; | ||
| private final CounterService counterService; | ||
| private final DataAccessRequestDAO dataAccessRequestDAO; | ||
| private final DaaDAO daaDAO; | ||
| private final DarCollectionDAO darCollectionDAO; | ||
| private final ElectionDAO electionDAO; | ||
| private final InstitutionService institutionService; | ||
|
|
@@ -109,6 +112,7 @@ public DataAccessRequestService( | |
| this.matchDAO = container.getMatchDAO(); | ||
| this.voteDAO = container.getVoteDAO(); | ||
| this.userDAO = container.getUserDAO(); | ||
| this.daaDAO = container.getDaaDAO(); | ||
| this.dacService = dacService; | ||
| this.dataAccessRequestServiceDAO = dataAccessRequestServiceDAO; | ||
| this.ruleService = ruleService; | ||
|
|
@@ -243,18 +247,20 @@ public DataAccessRequest createDataAccessRequest( | |
| referenceId, user.getUserId(), now, now, darData, user.getEraCommonsId()); | ||
| } else { | ||
| referenceId = UUID.randomUUID().toString(); | ||
| Integer darId = | ||
| dataAccessRequestDAO.insertDataAccessRequest( | ||
| collectionId, | ||
| referenceId, | ||
| user.getUserId(), | ||
| now, | ||
| now, | ||
| now, | ||
| darData, | ||
| user.getEraCommonsId()); | ||
|
Contributor
There was a problem hiding this comment. It would be nice if these were both in a single transaction. I have some
Contributor
Author
There was a problem hiding this comment. I might go with a slightly different approach to see how we like it, but it will be in one transaction.
Contributor
Author
There was a problem hiding this comment. Coming back to this one. It looks like there were already multiple database updates in this method that should effectively unwind if there's an error here, but because of the number of elements involved, I'm wondering if we should make a ticket for this and come back to it later. Would that be acceptable? There are also other methods in this class (and related services) that need to be linked as well, hence my hesitation to add this to the scope of this ticket.
Contributor
There was a problem hiding this comment. Sure thing - it would be nice to tighten this up separately
Contributor
Author
There was a problem hiding this comment. DT-3176 created. |
||
| daaDAO.insertDarDAARelationship(darId, dataAccessRequest.data.getDaaIds()); | ||
| } | ||
| syncDataAccessRequestDatasets(datasetIds, referenceId); | ||
| boolean requiresSOApproval = flagIfSOApprovalIsNeeded(user, datasetIds, referenceId); | ||
| if (!requiresSOApproval) { | ||
| ruleService.triggerDACRuleSettings(user, datasetIds, referenceId, request); | ||
| } | ||
|
|
@@ -287,18 +293,23 @@ public DataAccessRequest createProgressReport( | |
| "Progress report can only be created for approved datasets in the parent DAR"); | ||
| } | ||
| try { | ||
| Integer id = | ||
| dataAccessRequestDAO.insertProgressReport( | ||
| progressReport.getParentId(), | ||
| progressReport.getCollectionId(), | ||
| referenceId, | ||
| user.getUserId(), | ||
| progressReport.getData(), | ||
| user.getEraCommonsId()); | ||
| if (!progressReport.getIsCloseoutProgressReport()) { | ||
| daaDAO.insertDarDAARelationship(id, progressReport.getData().getDaaIds()); | ||
| } | ||
| } catch (JdbiException e) { | ||
| throw new BadRequestException( | ||
| "Unable to create progress report for Data Access Request " + parentDar.getReferenceId()); | ||
| } | ||
| boolean userIsPreAuthedForDaas = | ||
| isUserPreAuthorizedForAllDaas(user, progressReport.getDatasetIds()); | ||
| if (progressReport.getIsCloseoutProgressReport()) { | ||
| try { | ||
| User signingOfficialUser = | ||
|
|
@@ -312,11 +323,15 @@ public DataAccessRequest createProgressReport( | |
| } catch (TemplateException | IOException e) { | ||
| throw new InternalServerErrorException(e); | ||
| } | ||
| } else if (!userIsPreAuthedForDaas) { | ||
| dataAccessRequestDAO.updateRequiresSOApproval(true, referenceId); | ||
| } | ||
|
|
||
| syncDataAccessRequestDatasets(progressReportDatasetIds, referenceId); | ||
|
|
||
| if (!progressReport.getIsCloseoutProgressReport() | ||
| && !progressReport.getHasDMI() | ||
| && userIsPreAuthedForDaas) { | ||
| ruleService.triggerDACRuleSettings(user, progressReportDatasetIds, referenceId, request); | ||
| } | ||
|
|
||
|
|
@@ -392,7 +407,7 @@ public void validateProgressReport( | |
| throw new BadRequestException( | ||
| "Cannot create a progress report for a draft Data Access Request"); | ||
| } | ||
| if (progressReport.getDatasetIds().isEmpty()) { | ||
| throw new BadRequestException("At least one dataset is required"); | ||
| } | ||
| if (!Set.copyOf(parentDar.getDatasetIds()).containsAll(progressReport.getDatasetIds())) { | ||
|
|
@@ -421,6 +436,9 @@ public void validateProgressReport( | |
| "The selected signing official in the closeout was not found."); | ||
| } | ||
| } | ||
| if (!progressReport.getIsCloseoutProgressReport() && !progressReport.getHasDMI()) { | ||
| hasAcknowledgedRequiredDaas(progressReport); | ||
| } | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
|
|
@@ -438,6 +456,29 @@ protected void validateCommonDarAndProgressReportElements(User user, DataAccessR | |
| userService.validateActiveERACredentials(user); | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| protected void hasAcknowledgedRequiredDaas(DataAccessRequest dar) { | ||
| if (dar.getDatasetIds().isEmpty()) { | ||
| throw new BadRequestException("At least one dataset is required"); | ||
| } | ||
|
|
||
| Set<Integer> requiredDaas = daaDAO.findDaaIdsByDatasetIds(dar.getDatasetIds()); | ||
|
|
||
| if (!(requiredDaas.containsAll(dar.getData().getDaaIds()) | ||
|
otchet-broad marked this conversation as resolved.
|
||
| && requiredDaas.size() == dar.getData().getDaaIds().size())) { | ||
| throw new BadRequestException("All of the DAAs required were not acknowledged."); | ||
| } | ||
| } | ||
|
|
||
| private boolean isUserPreAuthorizedForAllDaas(User user, List<Integer> datasetIds) { | ||
| Set<Integer> datasetDaas = | ||
| daaDAO.findDaaIdsByDatasetIds(datasetIds).stream().collect(Collectors.toSet()); | ||
|
|
||
| Set<Integer> userDaas = user.getLibraryCard().getDaaIds().stream().collect(Collectors.toSet()); | ||
|
Contributor
There was a problem hiding this comment. Any potential for NPEs here? Current code paths look clean but there are likely exception cases w're missing. Might be nice to log potential data errors here.
Contributor
There was a problem hiding this comment. Sonar is flagging both of these set constructors as simplify-able with a |
||
|
|
||
| return userDaas.containsAll(datasetDaas); | ||
| } | ||
|
|
||
| public void validateDar(User user, DataAccessRequest dar) { | ||
| validateCommonDarAndProgressReportElements(user, dar); | ||
|
|
||
|
|
@@ -450,6 +491,7 @@ public void validateDar(User user, DataAccessRequest dar) { | |
| validateNoKeyPersonnelDuplicates(dar.getData()); | ||
| validatePersonnelInstitutionAndLibraryCardRequirements(user, dar.getData()); | ||
| validateCountryOfOperation(dar.getData(), false); | ||
| hasAcknowledgedRequiredDaas(dar); | ||
| } | ||
|
|
||
| protected void validateCountryOfOperation(DataAccessRequestData darData, boolean skipPI) { | ||
|
|
@@ -736,11 +778,13 @@ public List<Election> findOpenElectionsByReferenceId(String referenceId) { | |
| return electionDAO.findOpenElectionsByReferenceIds(List.of(referenceId)); | ||
| } | ||
|
|
||
| private boolean flagIfSOApprovalIsNeeded( | ||
| User user, List<Integer> datasetIds, String referenceId) { | ||
| if (!datasetDAO | ||
| .filterDatasetIdsByAutomationRuleType( | ||
| datasetIds, DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL.name()) | ||
| .isEmpty() | ||
| || !isUserPreAuthorizedForAllDaas(user, datasetIds)) { | ||
| dataAccessRequestDAO.updateRequiresSOApproval(true, referenceId); | ||
| return true; | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.