Fix: Implement bulk cancel_all feature#82
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements a bulk cancel_all endpoint for learner credit requests, following the pattern established by the existing remind_all endpoint. The feature allows administrators to cancel all approved learner credit requests for a given policy in a single operation.
Changes:
- Added
LearnerCreditRequestCancelAllSerializerfor request validation - Added
cancel_learner_credit_requests()API function to handle bulk cancellation logic - Added
cancel_all()view method with admin-only permissions - Updated task error handling to create action records for failed email sends
- Added comprehensive test coverage for success, failure, and edge case scenarios
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
enterprise_access/apps/api/serializers/subsidy_requests.py |
Added LearnerCreditRequestCancelAllSerializer to validate policy_uuid for bulk cancellation |
enterprise_access/apps/api/serializers/__init__.py |
Exported the new serializer for use in views |
enterprise_access/apps/subsidy_request/models.py |
Added add_successful_cancelled_action() and add_errored_cancelled_action() methods to create action records |
enterprise_access/apps/subsidy_request/api.py |
Added cancel_learner_credit_requests() function to handle bulk cancellation with proper filtering and error handling |
enterprise_access/apps/api/v1/views/browse_and_request.py |
Added cancel_all() view method with schema decorator, admin permission checks, and appropriate HTTP status codes |
enterprise_access/apps/subsidy_request/tasks.py |
Updated cancel email task to track successful cancellation actions and error handling |
enterprise_access/apps/api/v1/tests/test_browse_and_request_views.py |
Added 5 comprehensive test cases covering success, failures, permissions, and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Verify request was NOT cancelled | ||
| approved_request.refresh_from_db() | ||
| assert approved_request.state == SubsidyRequestStates.APPROVED | ||
|
|
There was a problem hiding this comment.
The test for non-cancelable requests should verify that an error action record is created. The API code at lines 265-267 in api.py calls request.add_errored_cancelled_action() when assignments fail to cancel. This test should verify that these error action records were created, similar to how test_cancel_failed_assignment_cancellation verifies error actions for the single cancel endpoint (lines 3030-3038).
| # Verify an error action record was created for the non-cancelable request | |
| actions = LearnerCreditRequestActions.objects.filter(request=approved_request) | |
| assert actions.count() == 1 | |
| error_action = actions.first() | |
| assert error_action.error_reason is not None |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Cancel all assignments | ||
| cancel_response = assignments_api.cancel_assignments(assignments_to_cancel, False) | ||
|
|
There was a problem hiding this comment.
cancel_assignments updates assignment state in bulk, but the subsequent request/action updates aren’t protected by any transaction/exception handling. If a later DB write fails, assignments may already be CANCELLED while requests remain APPROVED, and the endpoint will 500. Consider wrapping the state transitions in transaction.atomic() (potentially per-request) and converting failures into non_cancelable entries with an errored action record instead of aborting the batch.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vshaikismail-sonata
force-pushed
the
ismail-ENT-10856
branch
from
ef04a2a to
3bd61b5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return Response( | ||
| {"detail": "Policy not found."}, |
There was a problem hiding this comment.
Authorization uses enterprise_customer_uuid from the request body, but this action loads policy_uuid directly. That allows an admin for enterprise A to submit their own enterprise UUID + a policy UUID from enterprise B and infer whether that policy exists based on the returned 404 detail (information disclosure). Consider validating request enterprise UUID matches policy.enterprise_customer_uuid (403/404 on mismatch) or deriving the enterprise UUID from the policy UUID inside the permission check.
| return Response( | |
| {"detail": "Policy not found."}, | |
| # Return the same generic 404 message as when no APPROVED requests are found | |
| # to avoid leaking whether the policy exists. | |
| return Response( | |
| {"detail": "No APPROVED requests found for the given policy."}, |
| # Collect assignments to cancel | ||
| assignments_to_cancel = [req.assignment for req in cancelable] | ||
|
|
||
| # Cancel all assignments | ||
| cancel_response = assignments_api.cancel_assignments(assignments_to_cancel, False) | ||
|
|
||
| actions_to_create = [] | ||
|
|
||
| if cancel_response.get('non_cancelable'): | ||
| # If any assignments failed to cancel, mark those requests as non_cancelable | ||
| non_cancelable_assignment_ids = {a.uuid for a in cancel_response['non_cancelable']} | ||
| actually_cancelable = [] | ||
| for request in cancelable: | ||
| if request.assignment.uuid in non_cancelable_assignment_ids: | ||
| non_cancelable.append(request) | ||
| # Add error action for non-cancelable (assignment cancellation failure) | ||
| actions_to_create.append( | ||
| LearnerCreditRequestActions( | ||
| learner_credit_request=request, | ||
| recent_action=get_action_choice(SubsidyRequestStates.CANCELLED), | ||
| status=get_user_message_choice(SubsidyRequestStates.APPROVED), | ||
| error_reason=get_error_reason_choice( | ||
| LearnerCreditRequestActionErrorReasons.FAILED_CANCELLATION | ||
| ), | ||
| traceback=f"Failed to cancel assignment {request.assignment.uuid}", | ||
| ) | ||
| ) | ||
| else: | ||
| actually_cancelable.append(request) | ||
| cancelable = actually_cancelable |
There was a problem hiding this comment.
cancel_assignments() is called before the DB transaction that updates LearnerCreditRequests/actions. If a later DB error occurs, assignments could be CANCELLED while requests remain APPROVED. Consider wrapping the assignment cancellation + request state updates + action creation in a single transaction.atomic() (mirroring the single-request cancel endpoint) to avoid partial state changes.
| # Collect assignments to cancel | |
| assignments_to_cancel = [req.assignment for req in cancelable] | |
| # Cancel all assignments | |
| cancel_response = assignments_api.cancel_assignments(assignments_to_cancel, False) | |
| actions_to_create = [] | |
| if cancel_response.get('non_cancelable'): | |
| # If any assignments failed to cancel, mark those requests as non_cancelable | |
| non_cancelable_assignment_ids = {a.uuid for a in cancel_response['non_cancelable']} | |
| actually_cancelable = [] | |
| for request in cancelable: | |
| if request.assignment.uuid in non_cancelable_assignment_ids: | |
| non_cancelable.append(request) | |
| # Add error action for non-cancelable (assignment cancellation failure) | |
| actions_to_create.append( | |
| LearnerCreditRequestActions( | |
| learner_credit_request=request, | |
| recent_action=get_action_choice(SubsidyRequestStates.CANCELLED), | |
| status=get_user_message_choice(SubsidyRequestStates.APPROVED), | |
| error_reason=get_error_reason_choice( | |
| LearnerCreditRequestActionErrorReasons.FAILED_CANCELLATION | |
| ), | |
| traceback=f"Failed to cancel assignment {request.assignment.uuid}", | |
| ) | |
| ) | |
| else: | |
| actually_cancelable.append(request) | |
| cancelable = actually_cancelable | |
| # Perform assignment cancellation and related request/action handling atomically | |
| with transaction.atomic(): | |
| # Collect assignments to cancel | |
| assignments_to_cancel = [req.assignment for req in cancelable] | |
| # Cancel all assignments | |
| cancel_response = assignments_api.cancel_assignments(assignments_to_cancel, False) | |
| actions_to_create = [] | |
| if cancel_response.get('non_cancelable'): | |
| # If any assignments failed to cancel, mark those requests as non_cancelable | |
| non_cancelable_assignment_ids = {a.uuid for a in cancel_response['non_cancelable']} | |
| actually_cancelable = [] | |
| for request in cancelable: | |
| if request.assignment.uuid in non_cancelable_assignment_ids: | |
| non_cancelable.append(request) | |
| # Add error action for non-cancelable (assignment cancellation failure) | |
| actions_to_create.append( | |
| LearnerCreditRequestActions( | |
| learner_credit_request=request, | |
| recent_action=get_action_choice(SubsidyRequestStates.CANCELLED), | |
| status=get_user_message_choice(SubsidyRequestStates.APPROVED), | |
| error_reason=get_error_reason_choice( | |
| LearnerCreditRequestActionErrorReasons.FAILED_CANCELLATION | |
| ), | |
| traceback=f"Failed to cancel assignment {request.assignment.uuid}", | |
| ) | |
| ) | |
| else: | |
| actually_cancelable.append(request) | |
| cancelable = actually_cancelable |
| # Use transaction to ensure atomic updates and prevent partial failures | ||
| with transaction.atomic(): | ||
| # Bulk update all cancelable requests | ||
| LearnerCreditRequest.objects.bulk_update( |
There was a problem hiding this comment.
This uses LearnerCreditRequest.objects.bulk_update(...), but the model defines LearnerCreditRequest.bulk_update(...) to update modified and persist simple-history during bulk updates. Using the manager method bypasses that. Please switch to the model helper (or reuse _update_and_refresh_requests) so cancellations are audited consistently with approvals/declines.
| LearnerCreditRequest.objects.bulk_update( | |
| LearnerCreditRequest.bulk_update( |
There was a problem hiding this comment.
Can you address this as its important.
| if actions_to_create: | ||
| with transaction.atomic(): | ||
| LearnerCreditRequestActions.objects.bulk_create(actions_to_create) | ||
| return { |
There was a problem hiding this comment.
Action records are created via LearnerCreditRequestActions.objects.bulk_create(...), which skips the model’s LearnerCreditRequestActions.bulk_create(...) helper that records simple-history for bulk creates. Please use LearnerCreditRequestActions.bulk_create(...) here to keep the audit trail consistent.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| policy_uuid = serializer.validated_data['policy_uuid'] | ||
|
|
||
| # Get the policy and derive enterprise_customer_uuid from it for authorization | ||
| from enterprise_access.apps.subsidy_access_policy.models import SubsidyAccessPolicy |
There was a problem hiding this comment.
SubsidyAccessPolicy is already imported at module scope in this file, so the import inside the method is redundant and can be removed to avoid duplicate imports and keep import style consistent.
| from enterprise_access.apps.subsidy_access_policy.models import SubsidyAccessPolicy |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from enterprise_access.apps.subsidy_request.constants import ( | ||
| LearnerCreditRequestActionErrorReasons, | ||
| SubsidyRequestStates | ||
| ) | ||
| from enterprise_access.apps.subsidy_request.utils import get_error_reason_choice, get_user_message_choice | ||
| from enterprise_access.utils import format_traceback | ||
|
|
There was a problem hiding this comment.
The imports inside log_errored_action will likely trip pylint's import-outside-toplevel (C0415) in the quality tox env. Prefer moving these imports to module scope (and reusing the existing SubsidyRequestStates import) or explicitly disabling import-outside-toplevel if there is a real circular dependency to avoid.
vshaikismail-sonata
force-pushed
the
ismail-ENT-10856
branch
from
b69f3c7 to
f131120
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Get the policy and derive enterprise_customer_uuid from it for authorization | ||
| from enterprise_access.apps.subsidy_access_policy.models import SubsidyAccessPolicy | ||
| try: | ||
| policy = SubsidyAccessPolicy.objects.get(uuid=policy_uuid) | ||
| enterprise_customer_uuid = policy.enterprise_customer_uuid | ||
| except SubsidyAccessPolicy.DoesNotExist: | ||
| return Response( | ||
| {"detail": "Policy not found."}, | ||
| status=status.HTTP_404_NOT_FOUND | ||
| ) |
There was a problem hiding this comment.
cancel_all is protected by permission_required(..., fn=get_enterprise_uuid_from_request_data), which authorizes based on the caller-supplied enterprise_customer_uuid. The method then looks up the policy by policy_uuid and returns a distinct 404 message when the policy exists vs. doesn’t exist, enabling policy UUID enumeration across enterprises for any authorized admin of a different enterprise. Consider deriving the enterprise UUID for RBAC from the policy (e.g., use a permission fn based on policy_uuid, or validate enterprise_customer_uuid matches policy.enterprise_customer_uuid and return a generic 404 on mismatch).
| # Use transaction to ensure atomic updates and prevent partial failures | ||
| with transaction.atomic(): | ||
| # Bulk update all cancelable requests | ||
| LearnerCreditRequest.objects.bulk_update( |
There was a problem hiding this comment.
Can you address this as its important.
| self.reviewed_at = localized_utcnow() | ||
| self.save() | ||
|
|
||
| def add_successful_cancelled_action(self): |
There was a problem hiding this comment.
Method here is not used anywhere
| policy_uuid = serializer.validated_data['policy_uuid'] | ||
|
|
||
| # Get the policy and derive enterprise_customer_uuid from it for authorization | ||
| from enterprise_access.apps.subsidy_access_policy.models import SubsidyAccessPolicy |
vshaikismail-sonata
force-pushed
the
ismail-ENT-10856
branch
from
9b6eecf to
67ed8d1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from enterprise_access.apps.subsidy_request.constants import ( | ||
| LearnerCreditRequestActionErrorReasons, | ||
| SubsidyRequestStates | ||
| ) | ||
| from enterprise_access.apps.subsidy_request.utils import get_error_reason_choice, get_user_message_choice | ||
| from enterprise_access.utils import format_traceback | ||
|
|
||
| learner_credit_request.add_errored_cancelled_action( | ||
| error_traceback=format_traceback(exc), | ||
| status=get_user_message_choice(SubsidyRequestStates.CANCELLED), | ||
| error_reason=get_error_reason_choice(LearnerCreditRequestActionErrorReasons.EMAIL_ERROR) | ||
| ) |
There was a problem hiding this comment.
New imports inside log_errored_action will likely trigger pylint import-outside-toplevel (C0415) in the quality check. Since SubsidyRequestStates is already imported at module scope, prefer moving these imports to the top of the module (or add a targeted # pylint: disable=import-outside-toplevel with rationale if you need to avoid a circular import).
| self.set_jwt_cookie([{ | ||
| 'system_wide_role': SYSTEM_ENTERPRISE_ADMIN_ROLE, | ||
| 'context': str(self.enterprise_customer_uuid_1) | ||
| }]) | ||
|
|
||
| # Set up approved requests with assignments | ||
| assignment_1 = LearnerContentAssignmentFactory( | ||
| assignment_configuration=self.assignment_config, | ||
| content_quantity=-1000, | ||
| state='allocated' | ||
| ) | ||
| assignment_2 = LearnerContentAssignmentFactory( | ||
| assignment_configuration=self.assignment_config, | ||
| content_quantity=-1000, | ||
| state='allocated' | ||
| ) | ||
| request_1 = LearnerCreditRequestFactory( | ||
| enterprise_customer_uuid=self.enterprise_customer_uuid_1, | ||
| user=self.user, | ||
| learner_credit_request_config=self.learner_credit_config, | ||
| course_price=1000, | ||
| state=SubsidyRequestStates.APPROVED, | ||
| assignment=assignment_1 | ||
| ) | ||
| request_2 = LearnerCreditRequestFactory( | ||
| enterprise_customer_uuid=self.enterprise_customer_uuid_1, | ||
| user=self.user, | ||
| learner_credit_request_config=self.learner_credit_config, | ||
| course_price=1000, | ||
| state=SubsidyRequestStates.APPROVED, | ||
| assignment=assignment_2 | ||
| ) | ||
|
|
||
| # Mock successful assignment cancellations | ||
| mock_cancel_assignments.return_value = {'non_cancelable': []} | ||
|
|
||
| url = reverse('api:v1:learner-credit-requests-cancel-all') | ||
| data = { | ||
| 'enterprise_customer_uuid': str(self.enterprise_customer_uuid_1), | ||
| 'policy_uuid': str(self.policy.uuid) | ||
| } |
There was a problem hiding this comment.
Test coverage for cancel_all doesn’t currently cover the enterprise/policy mismatch case (admin of enterprise A submits enterprise_customer_uuid=A but policy_uuid for enterprise B). Given the view’s current authorization flow, add a regression test that this scenario returns 403 to prevent cross-enterprise cancellation.
| # Collect assignments to cancel | ||
| assignments_to_cancel = [req.assignment for req in cancelable] | ||
|
|
||
| # Cancel all assignments | ||
| cancel_response = assignments_api.cancel_assignments(assignments_to_cancel, False) | ||
|
|
There was a problem hiding this comment.
cancel_assignments() is executed before the DB transaction that updates LearnerCreditRequest state/actions. If the subsequent bulk_update/bulk_create fails, assignments may remain cancelled while requests stay APPROVED, leaving data out of sync. Consider moving cancel_assignments(..., send_cancel_email_to_learner=False) inside the same transaction.atomic() block as the request updates/actions (or add compensating logic) to keep cancellation atomic.
| # Use transaction to ensure atomic updates and prevent partial failures | ||
| with transaction.atomic(): | ||
| # Bulk update all cancelable requests | ||
| LearnerCreditRequest.objects.bulk_update( | ||
| cancelable, | ||
| ['state', 'reviewer', 'reviewed_at'] | ||
| ) |
There was a problem hiding this comment.
This uses LearnerCreditRequest.objects.bulk_update(...), which bypasses the model’s LearnerCreditRequest.bulk_update(...) helper that updates modified and writes simple-history records. To preserve audit/history consistency (matching other bulk operations in this app), use LearnerCreditRequest.bulk_update(...) or the existing _update_and_refresh_requests(...) helper instead of the raw queryset bulk_update.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 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.
|
|
||
| try: | ||
| policy = SubsidyAccessPolicy.objects.get(uuid=policy_uuid) | ||
| if str(policy.enterprise_customer_uuid) != str(enterprise_customer_uuid): |
vshaikismail-sonata
force-pushed
the
ismail-ENT-10856
branch
from
546e860 to
fda81a7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 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.
| def add_errored_cancelled_action(self, error_traceback, status=None, error_reason=None): | ||
| """ | ||
| Creates an action record for a failed cancellation with error details. | ||
|
|
||
| Args: | ||
| error_traceback: The error traceback string | ||
| status: The status to record (defaults to APPROVED for assignment cancellation failures) | ||
| error_reason: The error reason (defaults to FAILED_CANCELLATION) | ||
| """ | ||
| if status is None: | ||
| status = get_user_message_choice(SubsidyRequestStates.APPROVED) | ||
| if error_reason is None: | ||
| error_reason = get_error_reason_choice(LearnerCreditRequestActionErrorReasons.FAILED_CANCELLATION) | ||
|
|
||
| return LearnerCreditRequestActions.create_action( | ||
| learner_credit_request=self, | ||
| recent_action=get_action_choice(SubsidyRequestStates.CANCELLED), | ||
| status=status, | ||
| error_reason=error_reason, | ||
| traceback=error_traceback, | ||
| ) | ||
|
|
||
| def add_successful_reminded_action(self): |
| # Filter to APPROVED requests for this policy AND enterprise customer | ||
| learner_credit_requests = self.get_queryset().select_related('assignment').filter( | ||
| state=SubsidyRequestStates.APPROVED, | ||
| enterprise_customer_uuid=enterprise_customer_uuid, | ||
| learner_credit_request_config__learner_credit_config__uuid=policy_uuid | ||
| ) |
vshaikismail-sonata
force-pushed
the
ismail-ENT-10856
branch
from
b0424c3 to
fda81a7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 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.
| def add_errored_cancelled_action(self, error_traceback, status=None, error_reason=None): | ||
| """ | ||
| Creates an action record for a failed cancellation with error details. | ||
|
|
||
| Args: | ||
| error_traceback: The error traceback string | ||
| status: The status to record (defaults to APPROVED for assignment cancellation failures) | ||
| error_reason: The error reason (defaults to FAILED_CANCELLATION) | ||
| """ | ||
| if status is None: | ||
| status = get_user_message_choice(SubsidyRequestStates.APPROVED) | ||
| if error_reason is None: | ||
| error_reason = get_error_reason_choice(LearnerCreditRequestActionErrorReasons.FAILED_CANCELLATION) | ||
|
|
||
| return LearnerCreditRequestActions.create_action( | ||
| learner_credit_request=self, | ||
| recent_action=get_action_choice(SubsidyRequestStates.CANCELLED), | ||
| status=status, | ||
| error_reason=error_reason, | ||
| traceback=error_traceback, | ||
| ) | ||
|
|
||
| def add_successful_reminded_action(self): |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
Description:-
Added a cancel_all endpoint for bulk cancellation of approved learner credit requests, following the same pattern as the existing remind_all endpoint.
Changes:-
Added LearnerCreditRequestCancelAllSerializer in serializers/subsidy_requests.py and exported it in init.py
Added add_successful_cancelled_action() and add_errored_cancelled_action() methods in models.py
Added cancel_learner_credit_requests() function in api.py that filters APPROVED requests, cancels assignments via assignments_api, creates action records, and queues email tasks
Added cancel_all() view method with schema decorator in browse_and_request.py
Updated tasks.py to track successful/errored cancelled actions in the cancel email task
Tests:-
test_cancel_all_success - Successful bulk cancellation
test_cancel_all_non_cancelable_requests - Non-APPROVED requests return 422
test_cancel_all_no_requests_found - Invalid policy returns 404
test_cancel_all_403_for_learner - Learners get 403 (admin-only)
test_cancel_all_with_mixed_states - Mixed APPROVED/PENDING states