-
Notifications
You must be signed in to change notification settings - Fork 16
feat: added bulk approval endpoint for B&R #840
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.
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
base: main
Are you sure you want to change the base?
Conversation
4ed17e3 to
d16b484
Compare
d16b484 to
71aa76d
Compare
|
@jajjibhai008 [Question] Is there a limit to how many bulk requests can admins approve at a time? Also, wouldn't it be better to create a separate utility to create/update assignments in bulk using Django's |
488681e to
165fe7a
Compare
165fe7a to
3374869
Compare
sameenfatima78
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments/minor suggestion.
Also, curious how the code will handle re-requests for the same course?
| SubsidyRequestStates.ERROR, | ||
| ] | ||
| assert skipped_req.state == SubsidyRequestStates.APPROVED | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need a unit test for approve_all as well.
| for request in approved_requests: | ||
| request.state = SubsidyRequestStates.APPROVED | ||
| request.reviewer = reviewer | ||
| request.reviewed_at = localized_utcnow() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion] let's take this statement outside of loop
now = localized_utcnow()
for request in approved_requests:
request.state = SubsidyRequestStates.APPROVED
request.reviewer = reviewer
request.reviewed_at = now| successful_request_data.append({ | ||
| 'uuid': uuid_val, | ||
| 'state': SubsidyRequestStates.APPROVED, | ||
| 'message': "Successfully approved", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field can be left empty for approvals. In fact, it’s better to rename it to “error,” so the frontend can safely assume it only appears for failed requests.
| with transaction.atomic(): | ||
| LearnerCreditRequest.bulk_approve_requests(approved_requests, request.user) | ||
|
|
||
| # Send notifications and record results | ||
| for request_data in successful_request_data: | ||
| send_learner_credit_bnr_request_approve_task.delay(request_data['assignment_uuid']) | ||
| add_bulk_approve_operation_result( | ||
| results, | ||
| "approved", | ||
| request_data['uuid'], | ||
| request_data['state'], | ||
| request_data['message'], | ||
| ) | ||
|
|
||
| except (ValidationError, IntegrityError, DatabaseError) as exc: | ||
| error_msg = f"[LC REQUEST BULK APPROVAL] Bulk update failed: {exc}" | ||
| logger.exception(error_msg) | ||
| for request_data in successful_request_data: | ||
| add_bulk_approve_operation_result( | ||
| results, "failed", request_data['uuid'], | ||
| SubsidyRequestStates.REQUESTED, str(exc) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this indicate that we are either going to process all requests or none in case of failures?
Description:
Added BNR bulk approval endpoint
Payload for selected requests approval
Payload for All requests approval
Expected Response
Jira:
ENT-XXXX
Merge checklist:
./manage.py makemigrationshas been runPost merge: