Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding ability to resubmit DSRs #5658

Merged
merged 25 commits into from
Jan 29, 2025
Merged

Adding ability to resubmit DSRs #5658

merged 25 commits into from
Jan 29, 2025

Conversation

galvana
Copy link
Contributor

@galvana galvana commented Jan 11, 2025 •
edited
Loading

Closes LJ-112

Description Of Changes

Adds a new /resubmit endpoint to fully resubmit any errored privacy requests.

Code Changes

  • DB migration to cascade privacy request deletes to provided identities and custom privacy request fields
  • Refactoring of select privacy request and messaging flows into dedicated PrivacyRequestService and MessagingService
  • Addition of /resubmit endpoint
  • Automatic resubmission when trying to reprocess a privacy request with an expired cache

Steps to Confirm

  1. Run nox -s dev -- postgres
  2. Misconfigure the Postgres integration so that any privacy requests error
  3. Submit a privacy request, make sure it fails
  4. Fix the integration
  5. Take the privacy request ID and all the resubmit endpoint via the Swagger UI
  6. Go back to the Admin UI and make sure the request was reprocessed successfully, you shouldn't see any errored information from the first attempt

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
  • Followup issues:
    • Followup issues created (include link)
    • 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 Vercel
Copy link

vercel bot commented Jan 11, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fides-plus-nightly ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 29, 2025 7:33pm

@galvana galvana added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Jan 11, 2025
@cypress Cypress
Copy link

cypress bot commented Jan 11, 2025 •
edited
Loading

fides    Run #12025

Run Properties:  status check passed Passed #12025  •  git commit 42d5e93a71 ℹ️: Merge 6787aa7241e3dabd39ca2efc850bc415a6cfbbc5 into 8e151f5fa1e7989a6b8e17eae9a4...
Project fides
Branch Review refs/pull/5658/merge
Run status status check passed Passed #12025
Run duration 00m 48s
Commit git commit 42d5e93a71 ℹ️: Merge 6787aa7241e3dabd39ca2efc850bc415a6cfbbc5 into 8e151f5fa1e7989a6b8e17eae9a4...
Committer Adrian Galvan
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 5
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

@codecov Codecov
Copy link

codecov bot commented Jan 29, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.85640% with 58 lines in your changes missing coverage. Please review.

Project coverage is 87.17%. Comparing base (af9cacf) to head (6bbd5db).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...service/privacy_request/privacy_request_service.py 87.43% 18 Missing and 7 partials ⚠️
src/fides/service/messaging/messaging_service.py 76.08% 13 Missing and 9 partials ⚠️
.../api/api/v1/endpoints/privacy_request_endpoints.py 68.18% 5 Missing and 2 partials ⚠️
.../api/service/messaging/message_dispatch_service.py 76.47% 2 Missing and 2 partials ⚠️

❌ Your patch check has failed because the patch coverage (84.85%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5658      +/-   ##
==========================================
+ Coverage   86.85%   87.17%   +0.32%     
==========================================
  Files         388      389       +1     
  Lines       24101    24220     +119     
  Branches     2606     2624      +18     
==========================================
+ Hits        20932    21114     +182     
+ Misses       2611     2532      -79     
- Partials      558      574      +16

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

@galvana galvana marked this pull request as ready for review January 29, 2025 07:17
galvana
galvana commented Jan 29, 2025
View reviewed changes
src/fides/service/privacy_request/privacy_request_service.py
Comment on lines +330 to +335
if self.config_proxy.execution.require_manual_request_approval:
self.approve_privacy_requests(
[privacy_request_id],
reviewed_by=reviewed_by,
suppress_notification=True,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if the env var changes between runs?

src/fides/api/alembic/migrations/versions/1088e8353890_update_privacy_request_relationships.py Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test upgrade/downgrade

src/fides/api/api/v1/endpoints/consent_request_endpoints.py
authenticated=True,

# It's a bit weird to initialize the privacy request service here,
# but this logic is slated for deprecation so it's not worth doing a larger refactor
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow up on deprecation

src/fides/api/models/privacy_request.py
Comment on lines +634 to +639
def get_cached_encryption_key(self) -> Optional[str]:
"""Gets the cached encryption key for this privacy request."""
cache: FidesopsRedis = get_cache()
encryption_key = cache.get(get_encryption_cache_key(self.id, "key"))
return encryption_key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Create ticket to persist encryption key

tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py
Comment on lines +8637 to +8648
def test_resubmit_privacy_request(
self,
url,
api_client: TestClient,
generate_auth_header,
) -> None:
auth_header = generate_auth_header(scopes=[PRIVACY_REQUEST_CREATE])
response = api_client.post(
url,
headers=auth_header,
)
assert response.status_code == HTTP_200_OK
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add more tests around completed and pending tests + add docs

eastandwestwind
eastandwestwind approved these changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@eastandwestwind eastandwestwind left a comment

Choose a reason for hiding this comment

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

We did a screenshare CR session with @galvana , and all comments / requests are above, non-blocking.

@galvana galvana merged commit 3d49406 into main Jan 29, 2025
22 of 23 checks passed
@galvana galvana deleted the LA-112-add-ability-to-resubmit-a-DSR branch January 29, 2025 19:31
@cypress Cypress
Copy link

cypress bot commented Jan 29, 2025

fides    Run #12026

Run Properties:  status check passed Passed #12026  •  git commit 3d494069c0: Adding ability to resubmit DSRs (#5658)
Project fides
Branch Review main
Run status status check passed Passed #12026
Run duration 00m 50s
Commit git commit 3d494069c0: Adding ability to resubmit DSRs (#5658)
Committer Adrian Galvan
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 5
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eastandwestwind eastandwestwind eastandwestwind approved these changes

Assignees
No one assigned
Labels
run unsafe ci checks Runs fides-related CI checks that require sensitive credentials
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@galvana @eastandwestwind