fix: wrap into try/except block getting icon for xblock #2509
Conversation
xmodule/split_test_block.py
Outdated
| return self.runtime.get_block_for_descriptor(self.child_block) | ||
| else: | ||
| except (NoSuchServiceError, AttributeError): |
There was a problem hiding this comment.
| except (NoSuchServiceError, AttributeError): | |
| except (NoSuchServiceError, AttributeError): | |
| logging.warning("Error fetching child block: %s", e) |
can we add some logging here?
There was a problem hiding this comment.
What this try except is for?
self.child_block or self.runtime.get_block_for_descriptor
if for self.child_block I would enclose in try/except only self.child_block, and keep self.runtime.get_block_for_descriptor outside of the try/except
There was a problem hiding this comment.
Or maybe even put try/except in the def child_block property if possible
ruzniaievdm
force-pushed
the
ruzniaievdm/fix/issue-with-split-test
branch
from
14403d3 to
7df6d68
Compare
ruzniaievdm
force-pushed
the
ruzniaievdm/fix/issue-with-split-test
branch
from
7df6d68 to
edc5739
Compare
ruzniaievdm
force-pushed
the
ruzniaievdm/fix/issue-with-split-test
branch
from
edc5739 to
0557c26
Compare
|
@GlugovGrGlib @monteri added logging and divided try/except block by error location. |
xmodule/split_test_block.py
Outdated
| return self.runtime.get_block_for_descriptor(self.child_block) | ||
| else: | ||
| except AttributeError: | ||
| log.warning("Error while getting block for descriptor") |
There was a problem hiding this comment.
Please add a unique identifier (e.g. location), so it's possible to debug an error
There was a problem hiding this comment.
I also can't understand why you removed if self.child_block is not None
it means that you may break a case when child_block is None
xmodule/split_test_block.py
Outdated
| try: | ||
| group_id = self.get_group_id() | ||
| except NoSuchServiceError: | ||
| log.warning("Error while getting user service in runtime") |
There was a problem hiding this comment.
Please add a unique identifier as in previous logging
xmodule/split_test_block.py
Outdated
| @@ -172,10 +173,14 @@ def child_block(self): | |||
| def child(self): | |||
| """ | |||
| Return the user bound child block for the partition or None. | |||
|
|
|||
| We are using try/except block here because we ran into issues with | |||
| CachingDescriptorSystem has no attribute when we get an icon for the split_test xblock. | |||
There was a problem hiding this comment.
The comment should be changed specifying the technical issue
and note just - we get an icon for the split_test xblock
e.g.
Handles the AttributeError exception that may occur when attempting to retrieve the split_test xBlock information within the CMS.
xmodule/split_test_block.py
Outdated
| @@ -212,8 +217,16 @@ def get_content_titles(self): | |||
| def get_child_blocks(self): | |||
| """ | |||
| For grading--return just the chosen child. | |||
|
|
|||
| We are using try/except block here because we ran into issues with | |||
| User service being undefined when we get an icon for the split_test xblock. | |||
There was a problem hiding this comment.
Update docstring as in the previous function
ruzniaievdm
force-pushed
the
ruzniaievdm/fix/issue-with-split-test
branch
from
e7353b1 to
f90846b
Compare
ruzniaievdm
force-pushed
the
ruzniaievdm/fix/issue-with-split-test
branch
from
f90846b to
98b5d33
Compare
ruzniaievdm
force-pushed
the
ruzniaievdm/fix/issue-with-split-test
branch
from
98b5d33 to
c42d7dd
Compare
* fix: wrap into try/except block getting icon for xblock * fix: revision after review
* feat: XBlock's children API as DRF * fix: 500 error appears if user adds a Content Experiment * fix: wrap into try/except block getting icon for xblock (#2509) * fix: wrap into try/except block getting icon for xblock * fix: revision after review
Related upstream issue: openedx#34055
Results:
When trying to display a unit which contains two
split_testblock's (the former without group applied the latter with applied group) we getothericon typeWhen trying to display a unit which contains video and
split_testblock we getvideoicon type