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

PROD-2700 Implement Soft Delete for PrivacyRequests #5321

Merged
merged 11 commits into from
Sep 26, 2024
Merged

PROD-2700 Implement Soft Delete for PrivacyRequests #5321

merged 11 commits into from
Sep 26, 2024

Conversation

erosselli
Copy link
Contributor

@erosselli erosselli commented Sep 23, 2024 •
edited
Loading

Closes PROD-2700

Description Of Changes

Adds support for soft deleting PrivacyRequests. The deletion is defined by the new column deleted_at, and we also stored who deleted it in the deleted_by column. A new POST /privacy-request/id/soft-delete endpoint is added in order to allow soft deleting requests, as well as an endpoint to bulk delete multiple requests at the same time POST /privacy-request/bulk/soft-delete.

Code Changes

  • Added columns deleted_at and deleted_by to PrivacyRequest , both nullable
  • Added a new endpoint to allow soft deleting privacy requests, POST /privacy-request/id/soft-delete and for bulk deleting them POST /privacy-request/bulk/soft-delete
  • Added a new optional param to the existing search / list privacy requests endpoints, include_deleted_requests, that is False by default
  • Added validation to the existing privacy request endpoints that modify privacy request data, to throw a 422 error when attempting to update any data related to a deleted privacy request

Steps to Confirm

  • Run the admin UI and the privacy center (or alternatively, just run the entire test project)
  • Create four different privacy requests, each with a different email, by clicking the "Access my data" blob of the privacy center
  • If you go to the Admin UI, you should see all privacy requests
  • Open SwaggerUI (or your Postman collection if you prefer) and call the soft delete endpoint with the id of one of the privacy requests
  • Next, use the bulk soft delete endpoint to delete two other requests
  • Go back to Admin UI -- you should now see only one privacy request, the one that hasn't been deleted

Error cases

Trying to do anything that modifies the deleted privacy request should cause a 422 error. Some examples:

  • approving the request
  • deleting the request
  • restarting the request

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • 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
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md
  • For API changes, the Postman collection has been updated
  • If there are any 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!

@vercel Vercel
Copy link

vercel bot commented Sep 23, 2024 •
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 Sep 26, 2024 4:34pm

erosselli
erosselli commented Sep 23, 2024
View reviewed changes
src/fides/api/alembic/migrations/versions/75bb9ee843f5_add_soft_delete_for_privacy_request.py Outdated
Comment on lines 14 to 15
revision = "75bb9ee843f5"
down_revision = "9de4bb76307a"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note to self: remember to update before merging

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py Show resolved Hide resolved
src/fides/api/api/v1/endpoints/privacy_request_endpoints.py Show resolved Hide resolved
src/fides/api/api/v1/endpoints/privacy_request_endpoints.py Show resolved Hide resolved
src/fides/api/models/privacy_request.py Outdated Show resolved Hide resolved
src/fides/api/service/privacy_request/email_batch_service.py Show resolved Hide resolved
src/fides/api/service/privacy_request/request_runner_service.py Show resolved Hide resolved
tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py Outdated Show resolved Hide resolved
@cypress Cypress
Copy link

cypress bot commented Sep 23, 2024 •
edited
Loading

fides    Run #10196

Run Properties:  status check passed Passed #10196  •  git commit 42ddaa1100 ℹ️: Merge bf1cc0e21fa930a387a04104d2013d804cb11ae5 into feeeae1569e506af354d0adbde8b...
Project fides
Branch Review refs/pull/5321/merge
Run status status check passed Passed #10196
Run duration 00m 40s
Commit git commit 42ddaa1100 ℹ️: Merge bf1cc0e21fa930a387a04104d2013d804cb11ae5 into feeeae1569e506af354d0adbde8b...
Committer erosselli
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 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

@erosselli erosselli marked this pull request as ready for review September 24, 2024 14:30
erosselli
erosselli commented Sep 24, 2024
View reviewed changes
Copy link
Contributor Author

@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.

Just wanted to call out that I tried to filter out the deleted requests from the places where it made sense, but also I don't know if I found all the places where we get a list of privacy requests. Wanted to point it out to get some extra eyes around that if possible

@pattisdr
Copy link
Contributor

starting review -

pattisdr
pattisdr reviewed Sep 24, 2024
View reviewed changes
Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

This looks pretty good but let's step back for a second and clarify with product about expected behavior around "soft deletes". What should be able to be retrieved about a privacy request once it has been soft deleted?

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py Outdated Show resolved Hide resolved
src/fides/api/alembic/migrations/versions/75bb9ee843f5_add_soft_delete_for_privacy_request.py Outdated Show resolved Hide resolved
src/fides/api/api/v1/endpoints/privacy_request_endpoints.py Outdated Show resolved Hide resolved
src/fides/api/models/privacy_request.py Outdated Show resolved Hide resolved
src/fides/api/api/v1/endpoints/privacy_request_endpoints.py Show resolved Hide resolved
src/fides/api/api/v1/endpoints/privacy_request_endpoints.py Show resolved Hide resolved
src/fides/api/service/privacy_request/email_batch_service.py Show resolved Hide resolved
@pattisdr
Copy link
Contributor

reviewing!

pattisdr
pattisdr approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

Changes look good to me but I didn't do an additional round of testing - leaving that to you @erosselli !

@erosselli erosselli force-pushed the PROD-2700 branch 2 times, most recently from 36022fa to 386c2ab Compare September 26, 2024 13:57
erosselli
erosselli commented Sep 26, 2024
View reviewed changes
src/fides/api/alembic/migrations/versions/75bb9ee843f5_add_soft_delete_for_privacy_request.py

# revision identifiers, used by Alembic.
revision = "75bb9ee843f5"
down_revision = "68c590ff6e89"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

check before merging

@erosselli erosselli merged commit 96cc4e1 into main Sep 26, 2024
43 checks passed
@erosselli erosselli deleted the PROD-2700 branch September 26, 2024 17:55
@cypress Cypress
Copy link

cypress bot commented Sep 26, 2024

fides    Run #10203

Run Properties:  status check passed Passed #10203  •  git commit 96cc4e17fa: PROD-2700 Implement Soft Delete for PrivacyRequests (#5321)
Project fides
Branch Review main
Run status status check passed Passed #10203
Run duration 00m 37s
Commit git commit 96cc4e17fa: PROD-2700 Implement Soft Delete for PrivacyRequests (#5321)
Committer erosselli
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 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

Kelsey-Ethyca pushed a commit that referenced this pull request Oct 3, 2024
@erosselli @Kelsey-Ethyca
PROD-2700 Implement Soft Delete for PrivacyRequests (#5321)
01267a1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pattisdr pattisdr pattisdr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erosselli @pattisdr