Add app scorm_data for mobile sync #48
Conversation
…ile_api feat: [UNESCO-41] Include mobile sync plugin
…nt' and 'NoneType' (#47)
|
@oksana-slu thank you for the PR. Please find some comments, questions, and requests from me inline. Additionally, to land PR the new functionality should be covered by tests and the README file should be also updated. |
|
|
||
|
|
||
| @mobile_view() | ||
| class SyncXBlockData(views.APIView): |
There was a problem hiding this comment.
Please add docstrings with the class description.
| @mobile_view() | ||
| class SyncXBlockData(views.APIView): | ||
|
|
||
| def post(self, request, format=None): |
There was a problem hiding this comment.
The same is here, we need to have the proper description of the post method with the example of the expected request data.
| class SyncXBlockData(views.APIView): | ||
|
|
||
| def post(self, request, format=None): | ||
| # TODO: need to take a handler from the request |
There was a problem hiding this comment.
Why we do not take the handler from the request?
| handler = 'scorm_set_values' | ||
| response_context = {} | ||
|
|
||
| for course_data in request.data.get('courses_data'): |
There was a problem hiding this comment.
If request.data doesn't have the 'courses_data' Type Error raises. I would like us to handle this option.
| response_context = {} | ||
|
|
||
| for course_data in request.data.get('courses_data'): | ||
| course_id = course_data.get('course_id') |
There was a problem hiding this comment.
Are we going to work with the None keys in response_context in case 'course_id' is not found in the course_data?
| return self.data_scorm | ||
|
|
||
| @XBlock.json_handler | ||
| def scorm_set_values(self, data, suffix=''): |
| @XBlock.json_handler | ||
| def scorm_set_values(self, data, suffix=''): | ||
| is_updated = False | ||
| if self.data_scorm.get('last_updated_time', 0) < data.get('last_updated_time', 0): |
There was a problem hiding this comment.
Please describe the logic of the updating process.
BTW are we sure that request.data contains last_updated_time as an numerical value? Otherwise, it should be verified
Additional question how we can check that last_updated_time from the external request provided in UTC?
| 'lms.djangoapp': [ | ||
| 'scorm_app = scorm_app.apps:MobileScormSyncConfig', | ||
| ], | ||
| 'cms.djangoapp': [], |
There was a problem hiding this comment.
A redundant entry point could be omitted.
| ], | ||
| include_package_data=True, |
There was a problem hiding this comment.
Why do we need all files to be included in the package?
| ], | ||
| include_package_data=True, | ||
| zip_safe=False, |
RGOeX-414
Explanation:
We are merging the separate mobile_xblock_sync plugin into the xBlock, so we don't need to maintain 2 interdependent entities anymore (xBlock becomes self-sufficient).
info: https://docs.google.com/document/d/1IPPXYEfEY_sRSvELlMpsHmHuo_GqxzkB1dp6OoEqDH4/edit?usp=sharing