Skip to content

Conversation

@tvandort
Copy link
Contributor

@tvandort tvandort commented Nov 7, 2025

Do not include identities submitted as combined policy requests in generic erasure emails.

Ticket ENG-1945

Description Of Changes

Do not include submitted Privacy Request Identities in generic erasure emails if the policy has more than one type.

Code Changes

  • No longer run the generic erasure email integration on policies with more than 1 policy type.

Steps to Confirm

  1. Configure another DSR Policy with two rules. One rule should be consent and the other should be erasure.
  2. Configure a generic DSR integration.
  3. Submit a DSR as an erasure and ensure it gets to the awaiting email send state.
  4. Submit a DSR with the new policy that is both consent and erasure. Ensure that it never enters the awaiting email send state.

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@vercel
Copy link

vercel bot commented Nov 7, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Nov 12, 2025 7:12pm
fides-privacy-center Ignored Ignored Nov 12, 2025 7:12pm

@codecov
Copy link

codecov bot commented Nov 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.32%. Comparing base (ce27594) to head (d872de1).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6938   +/-   ##
=======================================
  Coverage   87.32%   87.32%           
=======================================
  Files         525      525           
  Lines       34418    34421    +3     
  Branches     3960     3960           
=======================================
+ Hits        30055    30058    +3     
  Misses       3500     3500           
  Partials      863      863           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tvandort tvandort marked this pull request as ready for review November 10, 2025 21:12
@tvandort tvandort requested a review from a team as a code owner November 10, 2025 21:12
@tvandort tvandort requested review from vcruces and removed request for a team November 10, 2025 21:12
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 10, 2025

Greptile Overview

Greptile Summary

This PR prevents generic erasure emails from being sent for privacy requests submitted under policies with multiple action types (e.g., consent + erasure). The change addresses a specific issue where combined policy requests (used for "Do Not Sell" features) were incorrectly including user identities in generic erasure email notifications.

Key Changes:

  • Added get_all_action_types() method to Policy model to retrieve all action types from policy rules
  • Modified needs_email() in BaseErasureEmailConnector to skip email sending when policies have more than one action type
  • Updated CHANGELOG.md to document the fix

Notes:

  • The implementation is straightforward and achieves the stated goal
  • Test coverage for the new multiple action types logic appears to be missing from the existing test suite

Confidence Score: 4/5

  • This PR is safe to merge with minor test coverage considerations
  • The implementation is simple and correct - checking the count of action types before sending erasure emails is the right approach. The new get_all_action_types() method follows existing patterns in the codebase. However, the score is 4 instead of 5 because the new logic lacks explicit test coverage to validate the multiple action types scenario
  • src/fides/api/service/connectors/base_erasure_email_connector.py - verify test coverage exists for the multiple action types logic

Important Files Changed

File Analysis

Filename Score Overview
src/fides/api/models/policy.py 5/5 Added get_all_action_types() method to return set of all action types from policy rules
src/fides/api/service/connectors/base_erasure_email_connector.py 3/5 Modified needs_email() to skip erasure emails for policies with multiple action types - missing test coverage for new logic

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +63 to +65
# do not send erasure emails if there is more than 1 action type for the request.
# we use consent + erasure types to facilitate Do Not Sell features.
multiple_action_types = len(privacy_request.policy.get_all_action_types()) > 1
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Check that test coverage includes the new multiple action types logic - existing tests in test_erasure_email_connector.py don't validate that needs_email() returns False when a policy has multiple action types (e.g., consent + erasure)

@erosselli erosselli self-requested a review November 10, 2025 21:33
Copy link
Contributor

@erosselli erosselli left a comment

Choose a reason for hiding this comment

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

This makes sense :) let's add a test that checks that the multiple action DSRs don't trigger the erasure email send

@tvandort tvandort requested a review from erosselli November 12, 2025 17:32
@tvandort
Copy link
Contributor Author

This makes sense :) let's add a test that checks that the multiple action DSRs don't trigger the erasure email send

Good call, added the requested test.

Let me know if you'd like me to add something more thorough but I think this should be sufficient.

Copy link
Contributor

@erosselli erosselli left a comment

Choose a reason for hiding this comment

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

ship it

@tvandort tvandort enabled auto-merge November 12, 2025 19:12
@tvandort tvandort added this pull request to the merge queue Nov 12, 2025
Merged via the queue into main with commit ca922dc Nov 12, 2025
69 checks passed
@tvandort tvandort deleted the ENG-1945 branch November 12, 2025 20:23
jjdaurora pushed a commit that referenced this pull request Dec 5, 2025
Co-authored-by: Eliana Rosselli <eliana@ethyca.com>
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.

3 participants