Skip to content

fix: wrap into try/except block getting icon for xblock - #2509

Merged
monteri merged 2 commits into
ts-developfrom
ruzniaievdm/fix/issue-with-split-test
Mar 6, 2024
Merged

monteri merged 2 commits into
ts-developfrom
ruzniaievdm/fix/issue-with-split-test

Conversation

@ruzniaievdm

@ruzniaievdm ruzniaievdm commented Mar 5, 2024 •

Copy link
Copy Markdown

Related upstream issue: openedx#34055

Results:

When trying to display a unit which contains two split_test block's (the former without group applied the latter with applied group) we get other icon type

When trying to display a unit which contains video and split_test block we get video icon type

@ruzniaievdm ruzniaievdm self-assigned this Mar 5, 2024
@ruzniaievdm
ruzniaievdm marked this pull request as ready for review March 5, 2024 16:02
Comment thread xmodule/split_test_block.py Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
except (NoSuchServiceError, AttributeError):
except (NoSuchServiceError, AttributeError):
logging.warning("Error fetching child block: %s", e)

can we add some logging here?

@GlugovGrGlib GlugovGrGlib Mar 5, 2024 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Or maybe even put try/except in the def child_block property if possible

@ruzniaievdm
ruzniaievdm force-pushed the ruzniaievdm/fix/issue-with-split-test branch from 14403d3 to 7df6d68 Compare March 6, 2024 10:15
@ruzniaievdm
ruzniaievdm marked this pull request as draft March 6, 2024 10:24
@ruzniaievdm
ruzniaievdm force-pushed the ruzniaievdm/fix/issue-with-split-test branch from 7df6d68 to edc5739 Compare March 6, 2024 10:38
@ruzniaievdm
ruzniaievdm force-pushed the ruzniaievdm/fix/issue-with-split-test branch from edc5739 to 0557c26 Compare March 6, 2024 10:39
@ruzniaievdm
ruzniaievdm requested a review from monteri March 6, 2024 10:43
@ruzniaievdm

Copy link
Copy Markdown
Author

@GlugovGrGlib @monteri added logging and divided try/except block by error location.

@ruzniaievdm
ruzniaievdm marked this pull request as ready for review March 6, 2024 10:45
Comment thread 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")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add a unique identifier (e.g. location), so it's possible to debug an error

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Location added to both logs

Comment thread xmodule/split_test_block.py Outdated
try:
group_id = self.get_group_id()
except NoSuchServiceError:
log.warning("Error while getting user service in runtime")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add a unique identifier as in previous logging

Comment thread xmodule/split_test_block.py Outdated
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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Docstrings were changed

Comment thread xmodule/split_test_block.py Outdated
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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Update docstring as in the previous function

@ruzniaievdm
ruzniaievdm marked this pull request as draft March 6, 2024 12:09
@ruzniaievdm
ruzniaievdm force-pushed the ruzniaievdm/fix/issue-with-split-test branch from e7353b1 to f90846b Compare March 6, 2024 12:12
@ruzniaievdm
ruzniaievdm marked this pull request as ready for review March 6, 2024 12:14
@ruzniaievdm
ruzniaievdm requested a review from GlugovGrGlib March 6, 2024 12:14
@ruzniaievdm
ruzniaievdm marked this pull request as draft March 6, 2024 12:16
@ruzniaievdm
ruzniaievdm force-pushed the ruzniaievdm/fix/issue-with-split-test branch from f90846b to 98b5d33 Compare March 6, 2024 12:33
@ruzniaievdm
ruzniaievdm force-pushed the ruzniaievdm/fix/issue-with-split-test branch from 98b5d33 to c42d7dd Compare March 6, 2024 12:34
@ruzniaievdm
ruzniaievdm marked this pull request as ready for review March 6, 2024 13:33
@monteri
monteri merged commit b5b6291 into ts-develop Mar 6, 2024
ruzniaievdm added a commit that referenced this pull request Mar 6, 2024
* fix: wrap into try/except block getting icon for xblock

* fix: revision after review
Lunyachek pushed a commit that referenced this pull request Mar 12, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants