Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: [AXM-748] fix problem block offline generations #2579

Merged
merged 4 commits into from
Jun 19, 2024
Merged

fix: [AXM-748] fix problem block offline generations #2579

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
87393cd
fix: fix problem block offline generations
NiedielnitsevIvan Jun 17, 2024
1072927
fix: add logs for 404 error on gettings xblock
NiedielnitsevIvan Jun 17, 2024
1c30aea
fix: fix xblock close checking for HTML xblock
NiedielnitsevIvan Jun 17, 2024
4de60bd
fix: [AXM-748] fix after review
NiedielnitsevIvan Jun 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion openedx/features/offline_mode/assets_management.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def save_asset_file(temp_dir, xblock, path, filename):
static_path = get_static_file_path(filename)
content = read_static_file(static_path)
file_path = os.path.join(temp_dir, 'assets', filename)
except (ItemNotFoundError, NotFoundError):
except (FileNotFoundError, ItemNotFoundError, NotFoundError):
log.info(f"Asset not found: {filename}")

else:
Expand Down
2 changes: 1 addition & 1 deletion openedx/features/offline_mode/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be a conflict with #2576

Let's merge this fix firstly and then rebase the mentioned PR

OFFLINE_SUPPORTED_XBLOCKS = getattr(settings, 'OFFLINE_SUPPORTED_XBLOCKS', DEFAULT_OFFLINE_SUPPORTED_XBLOCKS)

RETRY_BACKOFF_INITIAL_TIMEOUT = 60 # one minute
Expand Down
2 changes: 1 addition & 1 deletion openedx/features/offline_mode/renderer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value for this argument is False, so I removed it to avoid unnecessary questions)

)

enable_completion_on_view_service = False
Expand Down
6 changes: 4 additions & 2 deletions openedx/features/offline_mode/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What xBlock closed mean? please mention this in docstring

block_id = str(xblock.location)
html_data = XBlockRenderer(block_id).render_xblock_from_lms()
generate_offline_content_for_block.apply_async([block_id, html_data])


@shared_task(
Expand Down
3 changes: 3 additions & 0 deletions openedx/features/offline_mode/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

from django.contrib.auth import get_user_model
from django.core.files.storage import default_storage
from django.http.response import Http404

from zipfile import ZipFile

Expand Down Expand Up @@ -36,6 +37,8 @@ def generate_offline_content(xblock, html_data):
try:
save_xblock_html(tmp_dir, xblock, html_data)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should rename this function and call it sanitize or prepare xblock html data

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this function actually saves the rendered xblock html to the index.html file.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's leave it for now

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.')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think log message should be enhanced by stating for example that "block X cannot be fetched from course Y during offline content generation."

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

finally:
shutil.rmtree(tmp_dir, ignore_errors=True)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, thanks, added


Expand Down
Loading