Skip to content

Conversation

@kaustavb12
Copy link
Contributor

@kaustavb12 kaustavb12 commented May 19, 2022

Description

Builds on the shim added by #29190 to deprecate the following ModuleSystem attributes:

  • get_module: in favour of the new ModuleService
  • descriptor_runtime: The DescriptorSystem object was passed to ModuleSystem using this attribute.
    This was used to access the id_reader, id_generator and get_block properties of the the DescriptorSystem from the ModuleSystem. Each of these are now being handled differently as explained below and so this descriptor_runtime attribute has been removed and has not been replaced with anything else.

How the properties of DescriptorSystem which were accessed from ModuleSystem are now being handled:

  • id_reader: This property of the DescriptorSystem was fetched to set the id_reader keyword argument of the ModuleSystem in its __init__ method as this is required by the base Runtime class. This is now being supplied during initialization as keyword argument from the calling functions.
  • id_generator: Same as id_reader
  • get_block: This property of the DescriptorSystem was invoked from the get_block method of the ModuleSystem to fetch the block descriptor based on block_id and for_parent. This descriptor was then passed to get_module method (being deprecated) to bind the user data and module system.

As part of the PR, we have removed the get_block method from the ModuleSystem and created a new get_block method in the CombinedSystem class. Here, the get_block method of the descriptor_system is invoked to fetch the block. Further, its checked if the _module_system is initalized and if it contains the new ModuleService. If so, then this service is used to bind the user data and module system to the block descriptor.

This change should not affect any user of edx-platform

Supporting information

BB-5967 - OpenCraft internal ticket

Testing instructions

LMS: https://pr30436.sandbox.opencraft.hosting/
Studio: https://studio.pr30436.sandbox.opencraft.hosting/

LMS:

  1. Login as demo staff (staff@example.com / edx)
  2. Go to the CAPA unit
  3. Attempt the multiple choice problem
  4. Go to the Poll-Conditional unit
  5. Verify the conditional xblock works correctly after attempting the CAPA unit. The content should be visible.
  6. Reset the user data in CAPA unit
  7. Verify conditional xblock works correctly. The content should now be hidden.
  8. Go to the split test unit
  9. Verify the split test xblock works correctly and content for both groups are displayed
  10. Go to randomized-library unit
  11. Verify the randomized library xblock works without any errors
  12. Navigate to the BlockstoreContent unit
  13. Verify the html is rendered without any errors.

Studio:

  1. Login as a demo staff (staff@example.com / edx)
  2. Open sample course Introduction to Edx
  3. Navigate to different units already configured
  4. Edit some units and create new ones
  5. Verify everything works without any errors

@openedx-webhooks openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage labels May 19, 2022
@openedx-webhooks
Copy link

openedx-webhooks commented May 19, 2022

Thanks for the pull request, @kaustavb12! I've created BLENDED-1233 to keep track of it in Jira. More details are on the BD-13 project page.

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

@kaustavb12 kaustavb12 force-pushed the kaustav/bd_13_delete_descriptor_runtime branch 3 times, most recently from c6976aa to 8ef2622 Compare May 21, 2022 06:00
@kaustavb12 kaustavb12 force-pushed the kaustav/bd_13_delete_descriptor_runtime branch from a99af04 to db4a335 Compare May 24, 2022 14:41
@kaustavb12 kaustavb12 marked this pull request as draft May 30, 2022 01:22
@Agrendalath
Copy link
Member

Update: we're moving this to the point when we'll be merging the DescriptorSystem into ModuleSystem. We could merge this now, but it would introduce unnecessary temporary changes that affect all XBlocks.

@natabene
Copy link
Contributor

@kaustavb12 Just checking if this is still needs to be a draft. Do you know when this will be ready for a review?

@Agrendalath
Copy link
Member

@natabene, this one depends on #30715 and #30046, so we can move forward once we merge both of them.

@kaustavb12
Copy link
Contributor Author

@natabene Sorry for not responding here .. I somehow missed the ping.

@Agrendalath Thanks

@Agrendalath
Copy link
Member

Closing, as this is handled in #31472.

@Agrendalath Agrendalath closed this Mar 3, 2023
@openedx-webhooks
Copy link

@kaustavb12 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@Agrendalath Agrendalath deleted the kaustav/bd_13_delete_descriptor_runtime branch March 3, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blended PR is managed through 2U's blended developmnt program needs triage

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants