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.

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: [AXIMST-490] display render errors #2506

Merged
merged 2 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ class ChildVerticalContainerSerializer(serializers.Serializer):
actions = serializers.SerializerMethodField()
user_partition_info = serializers.DictField()
user_partitions = serializers.ListField()
has_validation_error = serializers.BooleanField()
validation_errors = MessageValidation(many=True)
validation_messages = MessageValidation(many=True)
render_error = serializers.CharField()

def get_actions(self, obj): # pylint: disable=unused-argument
"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,8 @@ def test_children_content(self):
},
"user_partition_info": expected_user_partition_info,
"user_partitions": expected_user_partitions,
"has_validation_error": False,
"validation_errors": [],
"validation_messages": [],
"render_error": "",
},
{
"name": self.html_unit_second.display_name_with_default,
Expand All @@ -220,8 +220,8 @@ def test_children_content(self):
},
"user_partition_info": expected_user_partition_info,
"user_partitions": expected_user_partitions,
"has_validation_error": False,
"validation_errors": [],
"validation_messages": [],
"render_error": "",
},
]
self.assertEqual(response.data["children"], expected_response)
Expand Down Expand Up @@ -280,13 +280,8 @@ def test_validation_errors(self):
response = self.client.get(url)
children_response = response.data["children"]

# Check for an error in html_unit_first xblock
self.assertTrue(children_response[0]["has_validation_error"])
# Verify that html_unit_first access settings contradict its parent's access settings.
self.assertEqual(children_response[0]["validation_messages"][0]["type"], ValidationMessage.ERROR)

# Verify that html access settings contradict its parent's access settings.
validation = html_unit_first.validate()
self.assertEqual(len(validation.messages), 1)
self.assertEqual(validation.to_json()['messages'][0]['type'], ValidationMessage.ERROR)

# Check for an error in html_unit_second xblock
self.assertFalse(children_response[1]["has_validation_error"])
# Verify that html_unit_second has no validation messages.
self.assertFalse(children_response[1]["validation_messages"])
18 changes: 10 additions & 8 deletions cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
get_user_partition_info,
get_visibility_partition_info,
get_validation_messages,
get_render_error,
)
from cms.djangoapps.contentstore.views.component import _get_item_in_course
from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import get_xblock
Expand Down Expand Up @@ -199,8 +200,8 @@ def get(self, request: Request, usage_key_string: str):
"can_delete": true,
"can_manage_tags": true,
}
"has_validation_error": false,
"validation_errors": [],
"validation_messages": [],
"render_error": "",
},
{
"name": "Text",
Expand All @@ -214,13 +215,13 @@ def get(self, request: Request, usage_key_string: str):
"can_delete": true,
"can_manage_tags": true,
},
"has_validation_error": true,
"validation_errors": [
"validation_messages": [
{
"text": "This component's access settings contradict its parent's access settings.",
"type": "error"
}
]
],
"render_error": "Unterminated control keyword: 'if' in file '../problem.html'",
},
],
"is_published": false,
Expand All @@ -239,16 +240,17 @@ def get(self, request: Request, usage_key_string: str):
child_info = modulestore().get_item(child)
user_partition_info = get_visibility_partition_info(child_info, course=course)
user_partitions = get_user_partition_info(child_info, course=course)
validation_errors, has_validation_error = get_validation_messages(child_info)
validation_messages = get_validation_messages(child_info)
render_error = get_render_error(request, child_info)

children.append({
"name": child_info.display_name_with_default,
"block_id": child_info.location,
"block_type": child_info.location.block_type,
"user_partition_info": user_partition_info,
"user_partitions": user_partitions,
"has_validation_error": has_validation_error,
"validation_errors": validation_errors,
"validation_messages": validation_messages,
"render_error": render_error,
})

is_published = not modulestore().has_changes(current_xblock)
Expand Down
52 changes: 46 additions & 6 deletions cms/djangoapps/contentstore/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2149,11 +2149,51 @@ def get_validation_messages(xblock):
xblock: The xblock object to validate.

Returns:
tuple:
- validation_errors (list): A list of validation error messages.
- has_validation_error (bool): True if there are validation errors, False otherwise.
list: A list of validation error messages.
"""
validation_json = xblock.validate().to_json()
validation_errors = validation_json['messages']
has_validation_error = bool(validation_errors)
return validation_errors, has_validation_error
return validation_json['messages']


def get_render_error(request, xblock):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can call it get_xblock_render_error and the function that is inside will be called get_xblock_render_error_context.

This will allow to put the inner function outside and clarify the idea behind these two functions.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think that we need to move out this function, it will be used only here.
Renamed

"""
Checks if there are any rendering errors for a given block and return these.

Args:
request: WSGI request object
xblock: The xblock object to rendering.

Returns:
str: Error message which happened while rendering of xblock.
"""
from cms.djangoapps.contentstore.views.preview import _load_preview_block
from xmodule.studio_editable import has_author_view
from xmodule.x_module import AUTHOR_VIEW, STUDENT_VIEW

def get_render_context(request, block):
"""
Return a dict of the data needs for render of each block.
"""
can_edit = has_studio_write_access(request.user, block.usage_key.course_key)

return {
"is_unit_page": False,
"can_edit": can_edit,
"root_xblock": xblock,
"reorderable_items": set(),
"paging": None,
"force_render": None,
"item_url": "/container/{block.location}",
"tags_count_map": {},
}

render_error = ""
try:
block = _load_preview_block(request, xblock)
preview_view = AUTHOR_VIEW if has_author_view(block) else STUDENT_VIEW
render_context = get_render_context(request, block)
block.render(preview_view, render_context)
except Exception as exc: # pylint: disable=broad-except
render_error = str(exc)

return render_error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
render_error = ""
try:
block = _load_preview_block(request, xblock)
preview_view = AUTHOR_VIEW if has_author_view(block) else STUDENT_VIEW
render_context = get_render_context(request, block)
block.render(preview_view, render_context)
except Exception as exc: # pylint: disable=broad-except
render_error = str(exc)
return render_error
try:
block = _load_preview_block(request, xblock)
preview_view = AUTHOR_VIEW if has_author_view(block) else STUDENT_VIEW
render_context = get_render_context(request, block)
block.render(preview_view, render_context)
except Exception as exc: # pylint: disable=broad-except
return str(exc)
return ""

Copy link
Author

Choose a reason for hiding this comment

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

Sure, thanks!

Loading