-
Notifications
You must be signed in to change notification settings - Fork 15
feat: bulk approve for lcr #873
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
Conversation
a374d8c to
2b4e3d4
Compare
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.
Nice work 💯 Left a few questions
| raise Exception(f"Consistency Error: Missing assignment for approved request {request.uuid}") | ||
| approved_requests_map[request.uuid] = { |
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.
To keep the exceptions consistent, let's raise SubisidyAccessPolicyRequestApprovalError here.
| return valid_requests, failed_requests | ||
|
|
||
|
|
||
| def can_allocate_all(self, learner_credit_requests): |
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.
I think instead of creating a new method, let's just move this code into can_approve() or assignment_request_can_allocate if we are relying on any content assignments functionality.
| # Verifies that the price on each LearnerCreditRequest is within the | ||
| # acceptable tolerance range of its canonical price. | ||
| valid_requests, failed_price_requests = self.validate_learner_credit_requests_allocation_prices( | ||
| requests_in_catalog, all_metadata | ||
| ) |
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.
Can we use validate_requested_allocation_price for handling this?
|
|
||
| # If the request has a valid lms_user_id, also check for a match on that. | ||
| if request.user.lms_user_id: | ||
| sub_query |= Q(lms_user_id=request.user.lms_user_id, content_key=request.course_id) |
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 is not needed as the a learner submitting a request will always be a valid user.
5a3cecb to
a19f4f9
Compare
f6aa027 to
2414502
Compare
2414502 to
be4fc6b
Compare
Description:
Add a description of your changes here.
Jira:
ENT-XXXX
Merge checklist:
./manage.py makemigrationshas been runPost merge: