fix: throw an error if the scope if None on scope_mappings [FC-0076] #815

rpenido · 2025-01-28T18:16:01Z

Description

While using the XBlock API, we may not have a FieldSet for a specific scope, which resulted in AttributeError: 'NoneType' object has no attribute 'foo' errors.

With this PR, the InvalidScopeError is now thrown in these cases.

Testing instructions

Edit a Survey block on the Library Authoring page and check the error below (before fix: always define a student_data_store to prevent errors on XBlock load [FC-0076] edx-platform#36226)

cms-1          | 2025-01-28 19:36:18,402 ERROR 2163 [celery_utils.logged_task] [user 4] [ip 172.18.0.1] logged_task.py:48 - [9d7cda4b-afd2-404f-a39c-7a185c486907] failed due to Traceback (most recent call last):
cms-1          |   File "/openedx/venv/lib/python3.11/site-packages/celery/app/trace.py", line 453, in trace_task
cms-1          |     R = retval = fun(*args, **kwargs)
cms-1          |                  ^^^^^^^^^^^^^^^^^^^^
cms-1          |   File "/openedx/venv/lib/python3.11/site-packages/celery/app/autoretry.py", line 38, in run
cms-1          |     return task._orig_run(*args, **kwargs)
cms-1          |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1          |   File "/openedx/venv/lib/python3.11/site-packages/edx_django_utils/monitoring/internal/code_owner/utils.py", line 195, in new_function
cms-1          |     return wrapped_function(*args, **kwargs)
cms-1          |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1          |   File "/openedx/edx-platform/openedx/core/djangoapps/content/search/tasks.py", line 57, in upsert_library_block_index_doc
cms-1          |     api.upsert_library_block_index_doc(usage_key)
cms-1          |   File "/openedx/edx-platform/openedx/core/djangoapps/content/search/api.py", line 639, in upsert_library_block_index_doc
cms-1          |     searchable_doc_for_library_block(library_block_metadata)
cms-1          |   File "/openedx/edx-platform/openedx/core/djangoapps/content/search/documents.py", line 381, in searchable_doc_for_library_block
cms-1          |     block = xblock_api.load_block(xblock_metadata.usage_key, user=None)
cms-1          |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1          |   File "/openedx/edx-platform/openedx/core/djangoapps/xblock/api.py", line 117, in load_block
cms-1          |     return runtime.get_block(usage_key, version=version)
cms-1          |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1          |   File "/openedx/edx-platform/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py", line 229, in get_block
cms-1          |     block.force_save_fields(block._get_fields_to_save())  # pylint: disable=protected-access
cms-1          |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cms-1          |   File "/mnt/xblock/xblock/core.py", line 531, in force_save_fields
cms-1          |     self._field_data.set_many(self, fields_to_save_json)
cms-1          |   File "/mnt/xblock/xblock/field_data.py", line 164, in set_many
cms-1          |     field_data.set_many(block, new_update_dict)
cms-1          |     ^^^^^^^^^^^^^^^^^^^
cms-1          | AttributeError: 'NoneType' object has no attribute 'set_many'

Checkout the current PR and mount it on your tutor stack
Check that the error throw now is the correct InvalidScopeError

Private ref: FAL-4033

openedx-webhooks · 2025-01-28T18:16:05Z

Thanks for the pull request, @rpenido!

This repository is currently maintained by @openedx/axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

pomegranited

👍 Fixes the issue, thanks @rpenido !

I tested this on my tutor dev stack in conjunction with fix: advanced editor styling on library authoring [FC-0076] edx-platform#36146, by saving changes to the Survey & Poll XBlocks
I read through the code
~~I checked for accessibility issues~~ N/A
Includes documentation - inline code comment explaining the change
~~User-facing strings are extracted for translation~~ N/A

pomegranited · 2025-01-30T01:21:53Z

xblock/core.py

@@ -687,7 +687,7 @@ class XBlock(Plugin, Blocklike, metaclass=_HasChildrenMetaclass):

    # These are dynamically managed by the awful hackery of _HasChildrenMetaclass.
    # We just declare their types here to make static analyzers happy.
-    # Note that children is only defined iff has_children is defined and True.
+    # Note that children is only defined if has_children is defined and True.


nit: this comment change can be reverted -- iff is a shorthand term for "if and only if"

I didn't know that! Thanks @pomegranited!!
I dropped the commit.

rpenido · 2025-02-03T18:37:33Z

Hi @pdpinch, @ormsbee and @felipemontoya!
Could any of you review this PR?
Thanks!

xblock/field_data.py

bradenmacdonald

Thanks, seems good, just a minor request.

bradenmacdonald · 2025-02-05T22:18:06Z

xblock/field_data.py

@@ -148,7 +148,12 @@ def _field_data(self, block, name):
        if scope not in self._scope_mappings:
            raise InvalidScopeError(scope)


Isn't this line trying to do something similar? Maybe you can consolidate these two conditions using self._scope_mappings.get(...)

Done: e048432

bradenmacdonald · 2025-02-06T23:00:04Z

Approved! Can you try squashing and/or rebasing this? I'm not sure why the docs check hasn't passed, but it's blocking me from merging.

bradenmacdonald · 2025-02-06T23:00:43Z

Also, please include a version bump if you'd like to. But it's also fine to just leave this "unreleased" and wait until it gets combined with some other changes. With the other fix we have in place, I don't think this PR is urgent anymore.

rpenido · 2025-02-07T14:42:56Z

Squased and Rebased!
But the readthedocs pending request persists. 😞
Also, I bumped the package version.

bradenmacdonald · 2025-02-07T18:15:51Z

Closed and re-opened to see if that will fix the docs check.

rpenido · 2025-02-07T19:53:35Z

Thanks for merging it @bradenmacdonald!
I think it got auto-tagged, but could you create a release?

bradenmacdonald · 2025-02-07T22:35:38Z

@rpenido Ah, I tagged it but I guess that wasn't enough. I've done a release now too.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 28, 2025

rpenido force-pushed the rpenido/fal-4033-skip-saving-field-if-no-scope-mapping branch 2 times, most recently from a064796 to e0df689 Compare January 28, 2025 19:31

rpenido marked this pull request as ready for review January 28, 2025 20:03

rpenido changed the title ~~fix: skip setting fields that are not in scope_mappings~~ fix: skip setting fields that are not in scope_mappings [FC-0076] Jan 29, 2025

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Jan 29, 2025

pomegranited approved these changes Jan 30, 2025

View reviewed changes

rpenido force-pushed the rpenido/fal-4033-skip-saving-field-if-no-scope-mapping branch from 97295cc to e0df689 Compare January 30, 2025 13:55

ormsbee reviewed Feb 4, 2025

View reviewed changes

xblock/field_data.py Outdated Show resolved Hide resolved

rpenido marked this pull request as draft February 4, 2025 12:48

rpenido force-pushed the rpenido/fal-4033-skip-saving-field-if-no-scope-mapping branch 4 times, most recently from 52e5ab3 to 6998e09 Compare February 4, 2025 18:13

rpenido mentioned this pull request Feb 5, 2025

fix: always define a student_data_store to prevent errors on XBlock load [FC-0076] openedx/edx-platform#36226

Open

rpenido changed the title ~~fix: skip setting fields that are not in scope_mappings [FC-0076]~~ fix: throw an error if the scope if None on scope_mappings [FC-0076] Feb 5, 2025

rpenido force-pushed the rpenido/fal-4033-skip-saving-field-if-no-scope-mapping branch from 584a398 to ab63faf Compare February 5, 2025 20:40

rpenido marked this pull request as ready for review February 5, 2025 21:03

rpenido requested review from bradenmacdonald and pomegranited February 5, 2025 21:03

bradenmacdonald requested changes Feb 5, 2025

View reviewed changes

rpenido force-pushed the rpenido/fal-4033-skip-saving-field-if-no-scope-mapping branch from ab63faf to e048432 Compare February 6, 2025 19:44

bradenmacdonald approved these changes Feb 6, 2025

View reviewed changes

fix: throws InvalidScopeError if the FieldData for the scope is None

bdd479b

rpenido force-pushed the rpenido/fal-4033-skip-saving-field-if-no-scope-mapping branch from 40cf647 to bdd479b Compare February 7, 2025 14:20

bradenmacdonald closed this Feb 7, 2025

bradenmacdonald reopened this Feb 7, 2025

bradenmacdonald mentioned this pull request Feb 7, 2025

[GH Request] Broken ReadTheDocs configuration for XBlock repo openedx/axim-engineering#1384

Closed

bmtcril closed this Feb 7, 2025

bmtcril reopened this Feb 7, 2025

bradenmacdonald merged commit 599a8b4 into openedx:master Feb 7, 2025
20 checks passed

bradenmacdonald deleted the rpenido/fal-4033-skip-saving-field-if-no-scope-mapping branch February 7, 2025 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: throw an error if the scope if None on scope_mappings [FC-0076] #815

fix: throw an error if the scope if None on scope_mappings [FC-0076] #815

rpenido commented Jan 28, 2025 •

edited

Loading

openedx-webhooks commented Jan 28, 2025 •

edited

Loading

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

Where can I find more information?

When can I expect my changes to be merged?

pomegranited left a comment •

edited

Loading

pomegranited Jan 30, 2025

rpenido Jan 30, 2025

rpenido commented Feb 3, 2025

bradenmacdonald left a comment

bradenmacdonald Feb 5, 2025

rpenido Feb 6, 2025

bradenmacdonald commented Feb 6, 2025

bradenmacdonald commented Feb 6, 2025

rpenido commented Feb 7, 2025

bradenmacdonald commented Feb 7, 2025

rpenido commented Feb 7, 2025

bradenmacdonald commented Feb 7, 2025

		@@ -148,7 +148,12 @@ def _field_data(self, block, name):
		if scope not in self._scope_mappings:
		raise InvalidScopeError(scope)

fix: throw an error if the scope if None on scope_mappings [FC-0076] #815

fix: throw an error if the scope if None on scope_mappings [FC-0076] #815

Conversation

rpenido commented Jan 28, 2025 • edited Loading

Description

Testing instructions

openedx-webhooks commented Jan 28, 2025 • edited Loading

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

Where can I find more information?

When can I expect my changes to be merged?

pomegranited left a comment • edited Loading

Choose a reason for hiding this comment

pomegranited Jan 30, 2025

Choose a reason for hiding this comment

rpenido Jan 30, 2025

Choose a reason for hiding this comment

rpenido commented Feb 3, 2025

bradenmacdonald left a comment

Choose a reason for hiding this comment

bradenmacdonald Feb 5, 2025

Choose a reason for hiding this comment

rpenido Feb 6, 2025

Choose a reason for hiding this comment

bradenmacdonald commented Feb 6, 2025

bradenmacdonald commented Feb 6, 2025

rpenido commented Feb 7, 2025

bradenmacdonald commented Feb 7, 2025

rpenido commented Feb 7, 2025

bradenmacdonald commented Feb 7, 2025

rpenido commented Jan 28, 2025 •

edited

Loading

openedx-webhooks commented Jan 28, 2025 •

edited

Loading

pomegranited left a comment •

edited

Loading