Skip to content

DT-2963: Add data location URL to emails that go to Researchers#2830

Merged
rushtong merged 11 commits intodevelopfrom
gr-DT-2963-add-data-location-to-email
Mar 16, 2026
Merged

DT-2963: Add data location URL to emails that go to Researchers#2830
rushtong merged 11 commits intodevelopfrom
gr-DT-2963-add-data-location-to-email

Conversation

@rushtong
Copy link
Contributor

@rushtong rushtong commented Mar 11, 2026

Addresses

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

Summary

This adds the dataLocationUrl dataset property (if it exists) to the underlying DatasetMailDTO object that populates emails. This model is used in email sent to the following users in the following conditions:

  • Researcher: alerted when a DAR is approved via DAC or RADAR
  • Researcher: alerted when a PR is approved via DAC or RADAR
  • DAC Chairs, Members: alerted that a dataset was approved via RADAR
  • Data Custodian: alerted that a researcher has been approved

However, we are only modifying the emails that go out to researchers. The DAC and Data Custodian emails are not modified, but could be if needed in the future.

Drive-by improvements:

  1. We migrate VoteService to use a DAOContainer to populate DAO classes instead of directly injecting them one-by-one.
  2. We convert the modified html templates to ftl templates instead.
  3. Convert DatasetMailDTO to a record class.

Example Email

Screenshot 2026-03-12 at 3 46 51 PM

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

@rushtong rushtong added the WIP Work in Progress label Mar 11, 2026
@rushtong rushtong changed the title DT-2963: Add data location to emails that go to users DT-2963: Add data location to emails that go to Researchers Mar 12, 2026
@rushtong rushtong changed the title DT-2963: Add data location to emails that go to Researchers DT-2963: Add data location URL to emails that go to Researchers Mar 12, 2026
@sonarqubecloud
Copy link

@rushtong rushtong marked this pull request as ready for review March 12, 2026 19:47
@rushtong rushtong requested a review from a team as a code owner March 12, 2026 19:47
@rushtong rushtong requested review from Copilot, fboulnois and kevinmarete and removed request for a team March 12, 2026 19:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a dataset “data location URL” into the dataset model used for researcher approval emails, enabling the researcher-facing templates to display a per-dataset location link when available.

Changes:

  • Extend DatasetMailDTO to include dataLocationUrl and populate it from dataset properties in VoteService.
  • Update researcher email templates to render the dataset location column and migrate those templates from .html to .ftl.
  • Refactor VoteService construction to use DAOContainer and update affected tests accordingly.

Reviewed changes

Copilot reviewed 12 out of 12 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/VoteService.java Populates dataLocationUrl on DatasetMailDTO from dataset properties; constructor now takes DAOContainer.
src/main/java/org/broadinstitute/consent/http/models/dto/DatasetMailDTO.java Converts DTO to a record and adds dataLocationUrl field.
src/main/java/org/broadinstitute/consent/http/enumeration/EmailType.java Switches researcher-approved email templates to .ftl filenames.
src/main/java/org/broadinstitute/consent/http/ConsentModule.java Updates Guice provider wiring for the new VoteService constructor.
src/main/resources/freemarker/researcher-dar-approved.ftl Adds “Dataset Location” column and conditional link rendering.
src/main/resources/freemarker/researcher-progress-report-approved.ftl Adds “Dataset Location” column and conditional link rendering.
src/test/java/org/broadinstitute/consent/http/service/VoteServiceTest.java Updates VoteService construction to use DAOContainer; adds tests; updates RADAR notification assertions.
src/test/java/org/broadinstitute/consent/http/service/EmailServiceTest.java Updates DTO construction + minor list accessor change.
src/test/java/org/broadinstitute/consent/http/mail/message/ResearcherDarApprovedMessageTest.java Updates tests to include dataLocationUrl expectations.
src/test/java/org/broadinstitute/consent/http/mail/message/ResearcherProgressReportApprovedMessageTest.java Updates tests to include dataLocationUrl expectations.
src/test/java/org/broadinstitute/consent/http/mail/message/DataCustodianApprovalMessageTest.java Updates DTO construction to new record signature.
src/test/java/org/broadinstitute/consent/http/mail/message/DACMembersDARRADARApprovedMessageTest.java Updates DTO construction to new record signature.
Comments suppressed due to low confidence (3)

src/main/resources/freemarker/researcher-dar-approved.ftl:51

  • dataset.dataLocationUrl is interpolated directly into both the link text and the href attribute without escaping/encoding, and FreeMarker auto-escaping is not enabled in FreeMarkerTemplateHelper. This can allow HTML attribute/text injection if the stored URL contains quotes/markup. Use FreeMarker escaping (e.g., HTML-escape the attribute/text and/or URL-encode), and consider checking ?has_content instead of only ?? to avoid rendering empty links.
    src/main/resources/freemarker/researcher-progress-report-approved.ftl:51
  • dataset.dataLocationUrl is inserted into the link text and href attribute without escaping/encoding, and FreeMarker auto-escaping is not enabled in this codebase. If the dataset property contains quotes/markup, it can break the attribute context and inject HTML into the email. Escape/encode the value in the template (and consider ?has_content instead of only ?? so empty strings don’t create empty anchors).
    src/test/java/org/broadinstitute/consent/http/service/VoteServiceTest.java:1195
  • In this test setup, dataset2 is created but the subsequent setters are applied to dataset1 (e.g., dataset1.setDatasetId(2), dataset1.setAlias(2), etc.). This leaves dataset2 uninitialized (null ids / dacId), which will cause incorrect stubbing/verification and can make the test fail or pass for the wrong reason. Update the setters to initialize dataset2 instead of overwriting dataset1.
                new Date())));

    Dataset dataset2 = new Dataset();
    dataset1.setDatasetId(2);
    dataset1.setAlias(2);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@rushtong rushtong removed the WIP Work in Progress label Mar 12, 2026
Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@rushtong rushtong merged commit 90eaba8 into develop Mar 16, 2026
18 checks passed
@rushtong rushtong deleted the gr-DT-2963-add-data-location-to-email branch March 16, 2026 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants