fix: [AXM-748] fix problem block offline generations #2579
Conversation
NiedielnitsevIvan
changed the title
| @@ -9,7 +9,7 @@ | |||
| MATHJAX_CDN_URL = f'https://cdn.jsdelivr.net/npm/mathjax@{MATHJAX_VERSION}/MathJax.js' | |||
| MATHJAX_STATIC_PATH = os.path.join('assets', 'js', f'MathJax-{MATHJAX_VERSION}.js') | |||
|
|
|||
| DEFAULT_OFFLINE_SUPPORTED_XBLOCKS = ['html'] | |||
| DEFAULT_OFFLINE_SUPPORTED_XBLOCKS = ['html', 'problem'] | |||
There was a problem hiding this comment.
There will be a conflict with #2576
Let's merge this fix firstly and then rebase the mentioned PR
| @@ -81,7 +81,7 @@ def render_xblock_from_lms(self): | |||
| str(self.usage_key), | |||
| disable_staff_debug_info=True, | |||
| course=course, | |||
| will_recheck_access='1', | |||
| will_recheck_access=False, | |||
There was a problem hiding this comment.
The docstring for this function can be enhanced in just wording and also it should be mentioned that we are not checking an access when rendering a specific block
There was a problem hiding this comment.
The default value for this argument is False, so I removed it to avoid unnecessary questions)
| @@ -21,8 +21,10 @@ def generate_offline_content_for_course(course_id): | |||
| course_key = CourseKey.from_string(course_id) | |||
| for offline_supported_block_type in OFFLINE_SUPPORTED_XBLOCKS: | |||
| for xblock in modulestore().get_items(course_key, qualifiers={'category': offline_supported_block_type}): | |||
| html_data = XBlockRenderer(str(xblock.location)).render_xblock_from_lms() | |||
| generate_offline_content_for_block.apply_async([str(xblock.location), html_data]) | |||
| if not hasattr(xblock, 'closed') or not xblock.closed(): | |||
There was a problem hiding this comment.
What xBlock closed mean? please mention this in docstring
| @@ -36,6 +37,8 @@ def generate_offline_content(xblock, html_data): | |||
| try: | |||
| save_xblock_html(tmp_dir, xblock, html_data) | |||
| create_zip_file(tmp_dir, base_path, f'{xblock.location.block_id}.zip') | |||
| except Http404: | |||
| log.error(f'Block {xblock.location.block_id} for course {xblock.location.course_key} not found.') | |||
There was a problem hiding this comment.
I think log message should be enhanced by stating for example that "block X cannot be fetched from course Y during offline content generation."
| @@ -36,6 +37,8 @@ def generate_offline_content(xblock, html_data): | |||
| try: | |||
| save_xblock_html(tmp_dir, xblock, html_data) | |||
There was a problem hiding this comment.
We should rename this function and call it sanitize or prepare xblock html data
There was a problem hiding this comment.
But this function actually saves the rendered xblock html to the index.html file.
There was a problem hiding this comment.
Ok, let's leave it for now
| @@ -36,6 +37,8 @@ def generate_offline_content(xblock, html_data): | |||
| try: | |||
| save_xblock_html(tmp_dir, xblock, html_data) | |||
| create_zip_file(tmp_dir, base_path, f'{xblock.location.block_id}.zip') | |||
| except Http404: | |||
| log.error(f'Block {xblock.location.block_id} for course {xblock.location.course_key} not found.') | |||
| finally: | |||
| shutil.rmtree(tmp_dir, ignore_errors=True) | |||
There was a problem hiding this comment.
If there's return on line 31, it's better to make celery task return result in all outcomes, the best option is to return True/False depending on the task result
No description provided.