[FAL-2457] Let ModuleSystem use UserService #29190

pomegranited · 2021-11-02T09:18:35Z

Description

Re-submits the changes from https://github.com/edx/edx-platform/pull/28440 which were reverted by https://github.com/edx/edx-platform/pull/28857, and fixes the bug that caused this revert.

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)

Testing instructions

LMS: https://pr29190.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.pr29190.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.

Test the anonymous User ID bug:

This bug occurs only in the Legacy Course Experience mode, for Problem blocks which rely on the anonymous_student_id context variable.

For OpenCraft staff:

Appserver 1 contains the broken code, so you can verify the bug there.
Appserver 3 is running the fixed code on this branch, so you can activate that appserver to verify the bug is fixed.

Login to sandbox as a demo staff (staff@example.com / edx)
Visit the Anonymous User ID Test Course and navigate to the example problem in the Legacy Course Experience.
This problem is expecting the ProblemBlock's anonymous student ID as an answer.
If verifying the bug exists: note that the answer expected when you first view the problem in the Legacy experience changes after you've submitted the problem.

If verifying that the bug is fixed: ensure that the answer expected remains consistent after submitting the problem.

Deadline

ASAP please -- the BD-13 project deliverables are in jeopardy because of delays incurred by this issue.

…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. (cherry picked from commit c41e7fb)

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 (cherry picked from commit 7538392)

* 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. (cherry picked from commit 927016a)

This bug occurs when get_module_system_for_user is called more than once per request, for blocks with different values for the requires_per_student_anonymous_id property.

openedx-webhooks · 2021-11-02T09:18:40Z

Thanks for the pull request, @pomegranited! I've created OSPR-6198 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

…oUserService This user object is shared between invocations, and so the anonymous_user_id gets altered for different types of blocks.

pomegranited · 2021-11-02T10:16:02Z

lms/djangoapps/courseware/tests/test_module_render.py

+        assert vertical_runtime.anonymous_student_id == anonymous_id_for_user(self.user, self.course.id)
+
+        # Ensure the problem runtime's anonymous student ID is unchanged after the above call.
+        assert problem_runtime.anonymous_student_id == anonymous_id_for_user(self.user, None)


Without a14f7e5. this test fails with:

AssertionError: assert 'e9018d2873e89d4dc26ea4d7ef631a1f' == '8538ca55520679e6d31b0c709d19e640' - 8538ca55520679e6d31b0c709d19e640 + e9018d2873e89d4dc26ea4d7ef631a1f Stacktrace self = <lms.djangoapps.courseware.tests.test_module_render.LmsModuleSystemShimTest testMethod=test_anonymous_student_id_bug> def test_anonymous_student_id_bug(self): """ Verifies that subsequent calls to get_module_system_for_user have no effect on each block runtime's anonymous_student_id value. """ problem_runtime, _ = render.get_module_system_for_user( self.user, self.student_data, self.problem_descriptor, self.course.id, self.track_function, self.xqueue_callback_url_prefix, self.request_token, course=self.course, ) # Ensure the problem block returns a per-user anonymous id assert problem_runtime.anonymous_student_id == anonymous_id_for_user(self.user, None) vertical_runtime, _ = render.get_module_system_for_user( self.user, self.student_data, self.descriptor, self.course.id, self.track_function, self.xqueue_callback_url_prefix, self.request_token, course=self.course, ) # Ensure the vertical block returns a per-course+user anonymous id assert vertical_runtime.anonymous_student_id == anonymous_id_for_user(self.user, self.course.id) # Ensure the problem runtime's anonymous student ID is unchanged after the above call. > assert problem_runtime.anonymous_student_id == anonymous_id_for_user(self.user, None) E AssertionError: assert 'e9018d2873e89d4dc26ea4d7ef631a1f' == '8538ca55520679e6d31b0c709d19e640' E - 8538ca55520679e6d31b0c709d19e640 E + e9018d2873e89d4dc26ea4d7ef631a1f lms/djangoapps/courseware/tests/test_module_render.py:2673: AssertionError

See https://build.testeng.edx.org/job/edx-platform-python-pipeline-pr/36211/testReport/junit/lms.djangoapps.courseware.tests.test_module_render/LmsModuleSystemShimTest/Run_Tests___lms_unit___test_anonymous_student_id_bug/

edx-status-bot · 2021-11-02T10:37:12Z

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

natabene · 2021-11-02T17:07:46Z

@pomegranited Thank you for your contribution. Please let me know once this is ready for our review.

pomegranited · 2021-11-03T04:39:32Z

@symbolist This PR is ready for your review. CC @ormsbee @natabene

symbolist

@pomegranited Thanks!

I tested this as per instructions.
I read through the code
I checked for accessibility issues. N/A
Includes documentation

@ormsbee This is ready for merge as well.

openedx-webhooks · 2021-11-08T14:18:14Z

@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-11-08T15:20:16Z

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

edx-pipeline-bot · 2021-11-08T16:51:25Z

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

pomegranited · 2021-11-09T02:43:59Z

Hooray, thank you for helping to diagnose the issue here @ormsbee, so we could get this change merged!

pomegranited added 4 commits November 2, 2021 18:24

test: Adds a test to verify the bug introduced by the previous change

d1741f8

This bug occurs when get_module_system_for_user is called more than once per request, for blocks with different values for the requires_per_student_anonymous_id property.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Nov 2, 2021

pomegranited marked this pull request as draft November 2, 2021 09:19

pomegranited mentioned this pull request Nov 2, 2021

[BD-13] Let ModuleSystem use UserService #28440

Merged

pomegranited added 2 commits November 2, 2021 20:04

chore: pylint

f8c96c1

fix: do not modify the django user object when creating a XBlockDjang…

a14f7e5

…oUserService This user object is shared between invocations, and so the anonymous_user_id gets altered for different types of blocks.

pomegranited commented Nov 2, 2021

View reviewed changes

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Nov 3, 2021

pomegranited marked this pull request as ready for review November 3, 2021 04:38

openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Nov 3, 2021

symbolist approved these changes Nov 4, 2021

View reviewed changes

ormsbee merged commit 15eca3b into openedx:master Nov 8, 2021

openedx-webhooks added merged and removed needs triage labels Nov 8, 2021

pomegranited deleted the jill/BD-13-user-service-fix branch November 9, 2021 02:43

pomegranited mentioned this pull request Nov 9, 2021

[BD-13] Deprecate ModuleSystem.render_template #28571

Merged

This was referenced Nov 10, 2021

[BD-13] refactor: deprecates ModuleSystem properties related to code sandboxing #29260

Merged

[BD-13] deprecates runtime.xqueue in favor of the XQueueService #29312

Merged

pomegranited mentioned this pull request Nov 25, 2021

[BD-13] Deprecates remaining user-related properties on ModuleSystem #29375

Merged

kaustavb12 mentioned this pull request Feb 7, 2022

[BD-13] [BB-5363] refactor: deprecates replace url related properties from ModuleSystem #29880

Merged

kaustavb12 mentioned this pull request May 19, 2022

[BD-13] [BB-5967] refactor: deprecate get_module and descriptor_runtime attributes from ModuleSystem #30436

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FAL-2457] Let ModuleSystem use UserService #29190

[FAL-2457] Let ModuleSystem use UserService #29190

Uh oh!

pomegranited commented Nov 2, 2021 •

edited

Loading

Uh oh!

openedx-webhooks commented Nov 2, 2021 •

edited

Loading

Uh oh!

pomegranited Nov 2, 2021

Uh oh!

edx-status-bot commented Nov 2, 2021

Uh oh!

natabene commented Nov 2, 2021

Uh oh!

pomegranited commented Nov 3, 2021

Uh oh!

symbolist left a comment

Uh oh!

openedx-webhooks commented Nov 8, 2021

Uh oh!

edx-pipeline-bot commented Nov 8, 2021

Uh oh!

edx-pipeline-bot commented Nov 8, 2021

Uh oh!

pomegranited commented Nov 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

[FAL-2457] Let ModuleSystem use UserService #29190

[FAL-2457] Let ModuleSystem use UserService #29190

Uh oh!

Conversation

pomegranited commented Nov 2, 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 Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pomegranited Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

edx-status-bot commented Nov 2, 2021

Uh oh!

natabene commented Nov 2, 2021

Uh oh!

pomegranited commented Nov 3, 2021

Uh oh!

symbolist left a comment

Choose a reason for hiding this comment

Uh oh!

openedx-webhooks commented Nov 8, 2021

Uh oh!

edx-pipeline-bot commented Nov 8, 2021

Uh oh!

edx-pipeline-bot commented Nov 8, 2021

Uh oh!

pomegranited commented Nov 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pomegranited commented Nov 2, 2021 •

edited

Loading

openedx-webhooks commented Nov 2, 2021 •

edited

Loading