-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix: lgeacy library block child.save fails in production instance while migrating #37573
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
Conversation
|
Thanks for the pull request, @navinkarkera! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
@kdmccormick @bradenmacdonald We noticed this issue in the sandbox. I am not sure if cc: @ChrisChV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested, but I'll take your word that you've tested that this still properly updates the upstream_version of the source block.
You weren't accidentally triggering this code from the LMS, were you? I'm not sure why you would be, but that's the only logical reason I could think of for .save() to cause this exception (of course, given how messy xblock field data APIs are, there are plenty of illogical reasons for .save() to cause this exception...)
xmodule/library_content_block.py
Outdated
| # Save fails with InvalidScopeError due to `_bound_field_data` field containing LmsFieldData which | ||
| # is ready-only. This was only seen in production instance and not observed in local dev instance. | ||
| # child.save() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Save fails with InvalidScopeError due to `_bound_field_data` field containing LmsFieldData which | |
| # is ready-only. This was only seen in production instance and not observed in local dev instance. | |
| # child.save() | |
| # Save fails with InvalidScopeError due to `_bound_field_data` field containing LmsFieldData which | |
| # is ready-only. This was only seen in production instance and not observed in local dev instance. | |
| # child.save() |
Unless you hope to reinstate child.save(), there's no reason to leave a comment on dead code. Just remove it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense.
20adba8 to
52d0e68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You weren't accidentally triggering this code from the LMS, were you?
Nope, there is no option to trigger it in lms. It is really weird.
xmodule/library_content_block.py
Outdated
| # Save fails with InvalidScopeError due to `_bound_field_data` field containing LmsFieldData which | ||
| # is ready-only. This was only seen in production instance and not observed in local dev instance. | ||
| # child.save() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense.
…le migrating (openedx#37573) The child.save() call while migrating legacy library block to library version 2 fails with `InvalidScopeError` due to presence of `LmsFieldData` in` _bound_field_data` field. Removing this call does not seem to have any adverse affect.
Description
The
child.save()call while migrating legacy library block to library version 2 fails withInvalidScopeErrordue to presence ofLmsFieldDatain_bound_field_datafield.Removing this call does not seem to have any adverse affect.
Error log:
Child field data:
{'_asides': [], '_cds_init_args': False, '_parent_block': <LegacyLibraryContentBlockWithMixins @5303 copied_from_block=None, downstream_customized=[], top_level_downstream_parent_key=None, upstream=None, upstream_display_name=None, upstream_version=None, upstream_version_declined=None, name=None, parent=BlockUsageLocator(CourseLocator('SampleTaxonomyOrg1', 'Chris456', 'chris456', None, None), 'vertical', 'd6b6fc5fd9104961aa83406773ddeea9'), tags=[], display_name='Randomized Content Block', course_edit_method='Studio', days_early_for_beta=None, due=None, edxnotes=False, edxnotes_visibility=True, giturl=None, graceperiod=None, graded=False, group_access={}, hide_from_toc=False, in_entrance_exam=False, matlab_api_key=None, max_attempts=None, relative_weeks_due=None, rerandomize='never', self_paced=False, show_correctness='always', show_reset_button=False, showanswer='finished', start=datetime.datetime(2030, 1, 1, 0, 0, tzinfo=tzlocal()), static_asset_path='', use_latex_compiler=False, user_partitions=[], video_auto_advance=False, video_bumper={}, video_speed_optimizations=True, visible_to_staff_only=False, xqa_key=None, chrome=None, default_tab=None, format=None, source_file=None, allow_resetting_children=False, children=[BlockUsageLocator(CourseLocator('SampleTaxonomyOrg1', 'Chris456', 'chris456', None, None), 'video', '4efeb593d14c61314395')], max_count=1, selected=[], xml_attributes={}, capa_type='any', is_migrated_to_v2=False, source_library_id='library-v1:tlmigration+tl123', source_library_version='68dc402f7ef716eebf702cf0'>, '_parent_block_id': BlockUsageLocator(CourseLocator('SampleTaxonomyOrg1', 'Chris456', 'chris456', None, None), 'library_content', '69629111a73b4504a0467511696a42ef'), '_child_cache': {}, '_runtime': <xmodule.modulestore.split_mongo.runtime.SplitModuleStoreRuntime object at 0x7f69e3bc34d0>, '_deprecated_per_instance_field_data': None, '_field_data_cache': {'upstream': 'lb:OpenCraft:chrislibrary:video:2cd6ffa4549b40ea8eb0521f9d34c3a8', 'upstream_version': 0}, '_dirty_fields': {<String upstream>: fields.EXPLICITLY_SET, <Integer upstream_version>: fields.EXPLICITLY_SET}, 'scope_ids': ScopeIds(user_id=40, block_type='video', def_id=ObjectId('68dc402f7ef716eebf702cef'), usage_id=BlockUsageLocator(CourseLocator('SampleTaxonomyOrg1', 'Chris456', 'chris456', None, None), 'video', '4efeb593d14c61314395')), '_edited_by': 40, '_edited_on': datetime.datetime(2025, 10, 30, 5, 51, 54, 319000, tzinfo=<bson.tz_util.FixedOffset object at 0x7f69e3bcdc50>), 'previous_version': None, 'update_version': ObjectId('6902fcfab505035c80dbd175'), 'source_version': None, 'definition_locator': DefinitionLocator(ObjectId('68dc402f7ef716eebf702cef'), 'video'), 'course_version': ObjectId('6902fcfab505035c80dbd175'), '_bound_field_data': LmsFieldData(ReadOnlyFieldData(InheritingFieldData(<xmodule.modulestore.split_mongo.split_mongo_kvs.SplitMongoKVS object at 0x7f69e3c2f010>)), KvsFieldData(<cms.djangoapps.contentstore.views.session_kv_store.SessionKeyValueStore object at 0x7f69e3c2c390>))}Supporting information
Deadline
Before ULMO Cut off
Other information
Include anything else that will help reviewers and consumers understand the change.