[DT-1583] Make DAR and progress report creation transactional

[kevinmarete]

Addresses

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

Summary

• Make DAR v2 and progress report database writes atomic using a shared JDBI transaction handle.
• Roll back collection, DAR, dataset/DAA relationship, snapshot, and SO-approval changes when persistence fails.
• Delete newly uploaded progress-report documents from GCS when validation or persistence fails.
• Treat post-commit closeout notification failures as non-fatal to avoid deleting documents referenced by committed records.

---

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

[Copilot]

Pull request overview

This PR updates Data Access Request (DAR) and Progress Report creation flows to execute multi-step DAO writes inside a single JDBI transaction, improving atomicity and rollback behavior. It also adds/updates unit tests to reflect the new transactional behavior and ensures uploaded Progress Report documents are cleaned up if creation fails.

Changes:

• Introduces DataAccessRequestServiceDAO.inTransaction(...) to run workflows with handle-attached DAOs.
• Wraps createDataAccessRequest and createProgressReport persistence steps in a single transaction and updates helper methods to accept transactional DAO instances.
• Adds tests for transaction rollback semantics and for compensating deletion of uploaded Progress Report documents on failure.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/main/java/org/broadinstitute/consent/http/service/DataAccessRequestService.java	Moves DAR/progress report multi-step persistence into a single transaction and wires helper methods to use transactional DAOs.
src/main/java/org/broadinstitute/consent/http/service/dao/DataAccessRequestServiceDAO.java	Adds a transaction helper that attaches participating DAOs to the same JDBI handle.
src/main/java/org/broadinstitute/consent/http/resources/DataAccessRequestResource.java	Adds compensating rollback to delete newly uploaded progress report documents if creation fails.
src/test/java/org/broadinstitute/consent/http/service/DataAccessRequestServiceTest.java	Updates service-layer unit tests to mock the new transactional DAO execution path.
src/test/java/org/broadinstitute/consent/http/service/dao/DataAccessRequestServiceDAOTest.java	Adds an integration-style test asserting DB rollback when a transaction step fails.
src/test/java/org/broadinstitute/consent/http/resources/DataAccessRequestResourceTest.java	Adds a test verifying uploaded documents are deleted when progress report creation fails.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/main/java/org/broadinstitute/consent/http/service/DataAccessRequestService.java:385

• The closeout-notification failure is intended to be non-fatal, but this catch only handles TemplateException/IOException. If emailService/sendGrid/emailDAO throws an unchecked exception (e.g., RuntimeException from SendGridAPI or DB insert), the request will still fail after the DB transaction has committed, which can re-trigger the resource-layer document rollback you’re trying to avoid.

      } catch (TemplateException | IOException e) {
        // Persistence has already committed, so a notification failure must not make the caller
        // treat creation as failed and compensate by deleting the progress report documents.
        logException("Unable to send submitted closeout message for " + referenceId, e);
      }

[rushtong]

This looks good, thank you for pulling these operations together.

[otchet-broad]

This is a great first step. I wasn't sure if the documents submitted with the payload and count incrementer are also supposed to be part of the same transaction.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[kevinmarete]

Addressed the suppressed feedback about unchecked closeout-notification failures.

The post-commit notification block now catches unchecked exceptions in addition to the declared template and I/O failures. This prevents a notification failure after persistence has committed from propagating to the resource and incorrectly triggering GCS document cleanup.

Added createCloseoutProgressReportIgnoresUncheckedNotificationFailure to verify that the progress report is still returned when notification delivery throws a runtime exception.