-
Notifications
You must be signed in to change notification settings - Fork 196
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
feat: API to perform ORA Workflows batch update #2031
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2031 +/- ##
==========================================
- Coverage 95.04% 94.92% -0.12%
==========================================
Files 178 183 +5
Lines 18145 18619 +474
Branches 1656 1704 +48
==========================================
+ Hits 17246 17675 +429
- Misses 677 712 +35
- Partials 222 232 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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 have some questions about the overall approach.
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 a start but still feels unpolished. There are certainly more elegant ways and with less code duplication by focusing on more modular scope (single submission, group of submissions for a single ORA, and all candidate submissions for an ORA) instead of re-writing the function over and over with slight modification.
I think @jansenk has good points of how to make a smaller set of functions that can be called with the appropriate scope.
Based on our discussions I have introduced chained asynchronous task execution and data caching.
I left out the Additional thoughts on implemented changes comparing to earlier (flattened) execution sequence: Chained/nested Celery tasks execution may prove to be problematic in maintenance:
Data cache - Implemented approach is not the most efficient but it is a compromise to achieve scalability and the API clarity: |
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.
Moving in the right direction, but could still use some cleanup for better modularity and removing duplicate item lookups.
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.
Still one necessary optimization blocking approval, then a few nits which can be taken into consideration or ignored.
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.
A few suggestions. it seems like you may be making this more complicated than this needs to be?
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.
Sorry, I want to merge this too, but I noticed a couple more issues
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.
There are a few small nits I'd like if you'd address but other than that I believe this is complete
@@ -259,7 +262,7 @@ def get_blocked_peer_workflows(course_id=None, item_id=None, submission_uuid=Non | |||
|
|||
filters = { | |||
'created_at__lte': timezone.now() - datetime.timedelta(days=7), | |||
'completed_at__isnull': True, | |||
'grading_completed_at__isnull': True, |
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.
You can also add completed_at__isnull: False
here. If a learner hasn't completed the number of peer reviews they need to complete, their workflow isn't going to do anything when we update it.
4c1410d
to
5f75d72
Compare
* feat: added API to perform batch ORA workflow update * feat: added Celery async tasks for batch ORA workflow update
* feat: added API to perform batch ORA workflow update * feat: added Celery async tasks for batch ORA workflow update
TL;DR - Added API to batch-update ORA Workflows for different scopes. Each endpoint is decorated as a Celery task to be executed asynchronously
JIRA: JIRA-1295
What changed?
Developer Checklist
Testing Instructions
pytest openassessment/tests/test_workflow_batch_update_api.py::TestWorkflowBatchUpdateAPI
Reviewer Checklist
Collectively, these should be completed by reviewers of this PR:
FYI: @openedx/content-aurora