[ENG-1578] Support bulk dsr operations: finalize #6775
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
JadeCara
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6775 +/- ##
=======================================
Coverage 87.39% 87.40%
=======================================
Files 518 518
Lines 33821 33846 +25
Branches 3892 3892
=======================================
+ Hits 29559 29584 +25
Misses 3407 3407
Partials 855 855 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Summary
Added bulk finalization support for privacy requests by introducing a shared validation helper _validate_privacy_request_for_bulk_operation that consolidates common validation logic (request existence, deletion status) used across bulk operations. The new finalize_privacy_requests method allows bulk finalization of privacy requests in requires_manual_finalization status, setting finalization metadata and queuing them for processing.
Key changes:
- Extracted common validation logic into
_validate_privacy_request_for_bulk_operationhelper - Refactored
approve_privacy_requestsanddeny_privacy_requeststo use shared validation - Added
finalize_privacy_requestsmethod with status validation and queue integration - Comprehensive test coverage including edge cases (not found, deleted, wrong status, exceptions)
Issues found:
- Database state persisted before queue operation in
finalize_privacy_requests, which could leave inconsistent state if queueing fails
Confidence Score: 4/5
- Safe to merge with one logical issue to consider
- Code follows existing patterns, includes excellent test coverage, and successfully refactors duplicate validation logic. Score reduced by 1 due to database save occurring before queue operation which could leave inconsistent state on queue failure (though this may match existing behavior in similar methods)
- Pay close attention to
src/fides/service/privacy_request/privacy_request_service.pylines 680-688 regarding database persistence ordering
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/fides/service/privacy_request/privacy_request_service.py | 4/5 | Added _validate_privacy_request_for_bulk_operation helper and finalize_privacy_requests method. Refactored existing bulk operations to use shared validation. Minor issue: database saved before queue operation could leave inconsistent state on queue failure. |
| tests/service/test_privacy_request_service.py | 5/5 | Comprehensive test coverage for validation helper and finalize functionality. Tests cover not found, deleted, wrong status, success, mixed results, and exception handling scenarios. |
Sequence Diagram
sequenceDiagram
participant Client
participant Service as PrivacyRequestService
participant Validator as _validate_privacy_request_for_bulk_operation
participant DB as Database
participant Queue as queue_privacy_request
Client->>Service: finalize_privacy_requests(request_ids, user_id)
loop For each request_id
Service->>Validator: _validate_privacy_request_for_bulk_operation(request_id)
Validator->>DB: get_privacy_request(request_id)
alt Request not found
Validator-->>Service: BulkUpdateFailed (not found)
else Request deleted
Validator-->>Service: BulkUpdateFailed (deleted)
else Valid request
Validator-->>Service: PrivacyRequest
alt Status != requires_manual_finalization
Service-->>Service: Add to failed list
else Valid status
Service->>DB: Set finalized_at, finalized_by
Service->>DB: save()
Service->>Queue: queue_privacy_request(id, from_step=finalization)
alt Queue success
Service-->>Service: Add to succeeded list
else Queue failure (Exception)
Service-->>Service: Add to failed list
end
end
end
end
Service-->>Client: BulkReviewResponse(succeeded, failed)
2 files reviewed, 1 comment
| privacy_request.finalized_at = datetime.utcnow() | ||
| privacy_request.finalized_by = user_id | ||
| privacy_request.save(db=self.db) | ||
|
|
||
| # Queue the privacy request for finalization | ||
| queue_privacy_request( | ||
| privacy_request_id=privacy_request.id, | ||
| from_step=CurrentStep.finalization.value, | ||
| ) |
There was a problem hiding this comment.
logic: database persisted before queue operation - if queue_privacy_request fails, finalized_at and finalized_by are already saved, leaving the privacy request in an inconsistent state
consider either:
- saving after successful queue (but may need transaction management)
- adding rollback logic in the exception handler
- documenting this behavior as acceptable if the queue operation is expected to be highly reliable
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/fides/service/privacy_request/privacy_request_service.py
Line: 680:688
Comment:
**logic:** database persisted before queue operation - if `queue_privacy_request` fails, `finalized_at` and `finalized_by` are already saved, leaving the privacy request in an inconsistent state
consider either:
1. saving after successful queue (but may need transaction management)
2. adding rollback logic in the exception handler
3. documenting this behavior as acceptable if the queue operation is expected to be highly reliable
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
This follows the existing pattern in approve_privacy_requests (line 555 → 569) and deny_privacy_requests (line 618 → 630). This code is consistent with the codebase, but may be something we want to review in the future. The exception handler will catch queue failures and add them to the failed list, which is appropriate behavior. If this needs to change, it should be a separate refactoring PR for all three methods.
| if privacy_request.deleted_at is not None: | ||
| return BulkUpdateFailed( | ||
| message="Cannot transition status for a deleted request", | ||
| data=PrivacyRequestResponse.model_validate(privacy_request).model_dump( |
There was a problem hiding this comment.
Do we want to include the whole model in this response, or just the ID? (Might it contain data we don't want to send?)
There was a problem hiding this comment.
This is the pattern the other bulk operations were following. If we want to change it for all the operations its probably more of a refactor than just pulling out the common functionality - not that I am against the refactor, but its just out of scope for this ticket I think.
There was a problem hiding this comment.
Sounds good, more a question than a need to change it - but I agree we should be consistent
| data={"privacy_request_id": request_id}, | ||
| ) | ||
| ) | ||
| result = self._validate_privacy_request_for_bulk_operation(request_id) |
There was a problem hiding this comment.
I think it might be better for _validate_privacy_request... to raise an exception if something is wrong and return a well-known-object when it returns correctly rather than checking isinstance - thoughts?
There was a problem hiding this comment.
yep love it, update incoming!
| data={"privacy_request_id": request_id}, | ||
| ) | ||
| ) | ||
| result = self._validate_privacy_request_for_bulk_operation(request_id) |
|
|
||
| return BulkReviewResponse(succeeded=succeeded, failed=failed) | ||
|
|
||
| def finalize_privacy_requests( |
There was a problem hiding this comment.
Isn't this a "private-ish" method? (Inasmuch as anything in Python is private) Also, where does this get called? I see tests for it but that seems to be the only place it's invoked; is that intentional?
There was a problem hiding this comment.
This is being called in the api in the next PR - I was following the pattern of the other bulk functionality here. I think having 1 private and the other bulk functions not being private would be odd. I can agree it should be in line for a refactor, but that should be its own PR (as mentioned in the response to the greptile comment below)
There was a problem hiding this comment.
👍 Just wanted to make sure it wasn't on accident
| except ObjectDeletedError: | ||
| pass | ||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
This is a more general observation about the way we test (in lots of our fixtures); this doesn't really need to be persisted, does it? It should be safe to assume the DB works and we're not doing anything but creating, storing and then deleting it
| assert privacy_request.location is None | ||
| assert privacy_request.status == PrivacyRequestStatus.pending | ||
|
|
||
| def test_validate_privacy_request_for_bulk_operation_not_found( |
There was a problem hiding this comment.
These seem to be covered by the tests below, aren't they? (in that the correct thing happens when there are non-existent, deleted, or valid requests)
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
JadeCara
deleted the
ENG-1578-be-support-bulk-dsr-operations-finalize-service
branch
Issue: ENG-1578
Description Of Changes
🎯 Purpose
As a privacy admin I want to take bulk actions on many privacy requests at once, so that I can quickly approve requests or deny requests.
Details
We really want to support the use case where a privacy admin applies some search criteria, then selects DSRS that meet that criteria and applies a bulk action. This will aid us in better supporting the bulk closure of duplicate requests and unverified requests.
This PR adds a
_validate_privacy_request_for_bulk_operationfunction to the privacy request service that all bulk functions can use and adds a new bulk functionfinalize_privacy_requeststo the service as well. This is part of the effort to provide bulk DSR functionality for users. The endpoint that uses this function will be in the next PR (#6776).Code Changes
_validate_privacy_request_for_bulk_operationandfinalize_privacy_requeststo src/fides/service/privacy_request/privacy_request_service.pytests/service/test_privacy_request_service.pySteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works