[BD-13] Let ModuleSystem use UserService #28440

pomegranited · 2021-08-11T08:29:48Z

Description

Initial step towards combining the ModuleSystem and DescriptorSystem classes.

Adds a shim which deprecates these ModuleSystem attributes, pulling them from the user service instead of requiring them in the constructor.

anonymous_student_id
seed
user_id
user_is_staff

This change is only a refactoring, and should not affect Learners, Course Authors, or anyone else using edx-platform.

Supporting information

Content Core Platform Simplication project
BLENDED-113 (edX internal ticket)
FAL-2247 (OpenCraft internal ticket)

Dependent on:

https://github.com/edx/XBlock/pull/542

Testing instructions

LMS: https://pr28440.sandbox.opencraft.hosting/

Login as a demo learner (honor@example.com / edx)
Browse around the Demo course and ensure that blocks render as expected, and that you can submit answers to problems.
Switch to the Legacy Course Experience and browse around in that too.

Studio: https://studio.pr28440.sandbox.opencraft.hosting/

Login as a demo staff (staff@example.com / edx)
Navigate to the Demo course and browse through the blocks. Ensure they render and edit as expected.
Check that you can submit answers to problems, and save changes to blocks.
Preview unpublished changes and ensure they render as expected.
Publish changes, and ensure they render in the LMS as expected.

Deadline

None

openedx-webhooks · 2021-08-11T08:29:53Z

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

When this pull request is ready, tag your edX technical lead.

common/djangoapps/xblock_django/user_service.py

common/lib/xmodule/xmodule/x_module.py

lms/djangoapps/courseware/module_render.py

common/lib/xmodule/xmodule/x_module.py

common/djangoapps/xblock_django/user_service.py

common/lib/xmodule/xmodule/html_module.py

common/djangoapps/xblock_django/user_service.py

symbolist

Thanks, I have done a pass.

lms/djangoapps/edxnotes/decorators.py

lms/djangoapps/courseware/module_render.py

common/lib/xmodule/xmodule/x_module.py

common/lib/xmodule/xmodule/tests/helpers.py

common/lib/xmodule/xmodule/capa_module.py

common/lib/xmodule/xmodule/x_module.py

common/lib/xmodule/xmodule/tests/test_html_module.py

lms/djangoapps/courseware/module_render.py

common/lib/xmodule/xmodule/x_module.py

openedx/core/djangoapps/xblock/runtime/runtime.py

common/lib/xmodule/xmodule/tests/test_conditional.py

pomegranited · 2021-09-01T01:51:18Z

Hi @ormsbee ! This PR is ready for your eyes. There's a question in the Author Notes that could use your feedback too, when you get a chance. CC @symbolist

symbolist · 2021-09-05T18:27:45Z

@pomegranited I missed this earlier:

The UserTagService relies on runtime.anonymous_student_id, which should now be pulled from the UserService instead.
How to deal with a service that depends on another service?

It looks like it only needs the Django User object? Or does it need the UserService for anything else? In general, if a service needs another, the later can be passed in as an init parameter to the former.

pomegranited · 2021-09-10T03:21:26Z

@symbolist I've addressed your last comment, so this should be ready for your final review. CC @ormsbee

symbolist

I tested this by checking changes to HTML and Problem XBlocks, due dates, timed exams.
I read through the code
I checked for accessibility issues
Includes documentation

lms/djangoapps/lms_xblock/test/test_runtime.py

lms/djangoapps/courseware/tests/test_views.py

lms/djangoapps/courseware/tests/test_module_render.py

symbolist · 2021-09-16T19:28:25Z

@pomegranited Have a couple of questions and then this just needs a rebase!

lms/djangoapps/lms_xblock/runtime.py

…vice The following ModuleSystem attributes are deprecated by this change, and should be pulled directly from the user service instead: * anonymous_student_id * seed * user_id * user_is_staff Related changes: * Removes the `user` and `anonymous_student_id` parameters from the ModuleService constructor. * Stores anonymous_user_id in XBlockDjangoUserService's opt_attr * Pulls out constants used by DjangoXBlockUserService opt_attr so they can be used in the platform code. * LmsModuleSystem uses the user service created in wrapper function for runtime.publish to avoid requiring the user service to be "needed" by all XBlocks. * LmsModuleSystem no longer checks for instances of XModuleDescriptor when deciding what kind of anonymous_user_id to provide: all XModules are XBlocks, so this check is unnecessary. * XBlockRuntime returns a user service when requested * Adds tests for deprecated ModuleSystem attributes and changes to XBlockDjangoUserService.

Removes references to these deprecated attributes from the platform code: * runtime.anonymous_student_id * runtime.seed * runtime.user_id * runtime.user_is_staff Related changes: * Ensure that all platform XBlocks which use these attributes "need" the user service. * ProblemBlock: Removes check for existence of runtime.seed attribute in preparation for removal of this attribute from ModuleSystem. * edxnotes: Catches NoSuchServiceError just in case some XBlocks using notes don't have the user service. * UserTagsService refactor: pass user and course_id on creation

* UserStubService now takes user, user_is_staff, and anonymous_user_id * get_test_system() creates a UserStubService with an anonymous_user_id of 'student' * Removes references to deprecated ModuleSystem attributes from test code * Fixes and simplifies the ConditionalBlock tests, using get_module provided by TestModuleSystem instead of trying to mock out all the pieces.

edx-status-bot · 2021-09-23T03:33:29Z

Your PR has finished running tests. There were no failures.

symbolist

@ormsbee This is ready for merge at a convenient time (preferably early in the day). 🙂

ormsbee · 2021-09-23T19:41:39Z

Talked to @symbolist about this and I'll merge tomorrow morning, around 9 AM EDT.

pomegranited · 2021-09-24T05:40:23Z

Yay!! Thank you @ormsbee and @symbolist !

openedx-webhooks · 2021-09-24T13:26:49Z

@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2021-09-24T14:28:42Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2021-09-24T14:54:34Z

EdX Release Notice: This PR has been deployed to the production environment.

pomegranited · 2021-10-26T07:14:29Z

common/lib/xmodule/xmodule/capa_module.py

            text = self.data

+        user_service = self.runtime.service(self, 'user')
+        anonymous_student_id = user_service.get_current_user().opt_attrs.get(ATTR_KEY_ANONYMOUS_USER_ID)


@symbolist @ormsbee I was able to reproduce the issue with the flip-flopping anonymous_student_id values and this branch, but only on the legacy courseware view.

Problem blocks have requires_per_student_anonymous_id = True set on their descriptors, and so should be using the per-user anonymous_user_id value.

However, with this branch:

When the problem block is first viewed in the legacy courseware view, it incorrectly has the per-course anonymous_user_id in its user service, instead of the per-user anonymous_user_id.

When the problem block is viewed in the new learning MFE, or when the problem block is scored via AJAX, it correctly uses the per-user anonymous_user_id.

I hacked a few things and it seems like the runtime (and user service) used to initialize the ProblemBlock in the legacy courseware view isn't actually the problem block's runtime, it's the parent unit's runtime + user service. Does that sound possible? And do you have any idea how/where that's being passed in?

Hmm, the child of a block is loaded via https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/module_render.py#L501. Checking what happens inside it with a debugger may provide some insights. You may also want to particularly check what happens when bind_for_student() gets called in these different scenarios on the ProblemBlock.

@symbolist CC @ormsbee Here's the line that caused the issue, in DjangoXBlockUserService.init

That same django_user object gets passed through when creating the ProblemBlock's user service, and its parent Vertical and Sequence blocks. I unwisely stored the anonymous_user_id attribute on that shared django_user object (mimicking the user_is_staff attribute storage in the line above), and since the anonymous_user_id value is different for Problem/HTML blocks vs Vertical/Sequence blocks (cf requires_per_student_anonymous_id), it was being altered during rendering.

https://github.com/edx/edx-platform/pull/29190 will address this issue and add a test to verify the fix.

openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage labels Aug 11, 2021

pomegranited commented Aug 11, 2021

View reviewed changes

pomegranited marked this pull request as draft August 11, 2021 08:35

pomegranited mentioned this pull request Aug 13, 2021

[BD-13] depr: deprecated the user_id attribute openedx/XBlock#542

Merged

pomegranited force-pushed the jill/BD-13-user-service branch 4 times, most recently from 640078b to e029feb Compare August 18, 2021 10:52

symbolist reviewed Aug 18, 2021

View reviewed changes

symbolist suggested changes Aug 18, 2021

View reviewed changes

symbolist suggested changes Aug 19, 2021

View reviewed changes

pomegranited marked this pull request as ready for review August 20, 2021 07:22

symbolist suggested changes Aug 26, 2021

View reviewed changes

pomegranited mentioned this pull request Aug 29, 2021

[BD-13] Deprecate ModuleSystem.render_template #28571

Merged

pomegranited marked this pull request as draft August 29, 2021 23:50

pomegranited commented Aug 30, 2021

View reviewed changes

openedx/core/djangoapps/xblock/runtime/runtime.py Outdated Show resolved Hide resolved

pomegranited marked this pull request as ready for review August 30, 2021 06:59

pomegranited force-pushed the jill/BD-13-user-service branch from f99e457 to e24a8f7 Compare August 30, 2021 08:31

symbolist reviewed Aug 30, 2021

View reviewed changes

openedx/core/djangoapps/xblock/runtime/runtime.py Outdated Show resolved Hide resolved

common/lib/xmodule/xmodule/tests/test_conditional.py Outdated Show resolved Hide resolved

pomegranited force-pushed the jill/BD-13-user-service branch 2 times, most recently from 1523b7b to 7c24e84 Compare August 31, 2021 07:29

pomegranited force-pushed the jill/BD-13-user-service branch from 7c24e84 to 4f95b3b Compare September 2, 2021 02:02

symbolist approved these changes Sep 16, 2021

View reviewed changes

lms/djangoapps/lms_xblock/test/test_runtime.py Outdated Show resolved Hide resolved

lms/djangoapps/courseware/tests/test_views.py Outdated Show resolved Hide resolved

lms/djangoapps/courseware/tests/test_module_render.py Outdated Show resolved Hide resolved

pomegranited force-pushed the jill/BD-13-user-service branch from 25dce1d to 7cb20bf Compare September 17, 2021 09:13

symbolist reviewed Sep 19, 2021

View reviewed changes

lms/djangoapps/lms_xblock/runtime.py Outdated Show resolved Hide resolved

symbolist reviewed Sep 19, 2021

View reviewed changes

lms/djangoapps/lms_xblock/runtime.py Outdated Show resolved Hide resolved

symbolist reviewed Sep 20, 2021

View reviewed changes

lms/djangoapps/lms_xblock/runtime.py Outdated Show resolved Hide resolved

pomegranited added 3 commits September 23, 2021 11:41

pomegranited force-pushed the jill/BD-13-user-service branch from b20c99f to 927016a Compare September 23, 2021 02:40

symbolist approved these changes Sep 23, 2021

View reviewed changes

ormsbee merged commit 3827f4c into openedx:master Sep 24, 2021

openedx-webhooks added merged and removed needs triage labels Sep 24, 2021

ormsbee mentioned this pull request Sep 27, 2021

Revert "[BD-13] Let ModuleSystem use UserService" #28857

Merged

openedx-webhooks added engineering review and removed merged labels Sep 29, 2021

pomegranited commented Oct 26, 2021

View reviewed changes

pomegranited deleted the jill/BD-13-user-service branch November 2, 2021 09:08

pomegranited mentioned this pull request Nov 2, 2021

[FAL-2457] Let ModuleSystem use UserService #29190

Merged

openedx-webhooks added merged and removed engineering review labels Nov 8, 2021

pomegranited mentioned this pull request Nov 18, 2021

[BD-13] Deprecate ModuleSystem.render_template (fixed) #29354

Merged

[BD-13] Let ModuleSystem use UserService #28440

[BD-13] Let ModuleSystem use UserService #28440

Uh oh!

Conversation

pomegranited commented Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Deadline

Uh oh!

openedx-webhooks commented Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

symbolist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pomegranited commented Sep 1, 2021

Uh oh!

symbolist commented Sep 5, 2021

Uh oh!

pomegranited commented Sep 10, 2021

Uh oh!

symbolist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

symbolist commented Sep 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edx-status-bot commented Sep 23, 2021

Uh oh!

symbolist left a comment

Choose a reason for hiding this comment

Uh oh!

ormsbee commented Sep 23, 2021

Uh oh!

pomegranited commented Sep 24, 2021

Uh oh!

openedx-webhooks commented Sep 24, 2021

Uh oh!

edx-pipeline-bot commented Sep 24, 2021

Uh oh!

edx-pipeline-bot commented Sep 24, 2021

Uh oh!

pomegranited Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

symbolist Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

pomegranited Nov 2, 2021

pomegranited commented Aug 11, 2021 •

edited

Loading

openedx-webhooks commented Aug 11, 2021 •

edited

Loading