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

pomegranited · 2021-11-22T07:49:35Z

Description

Builds on the shim added by https://github.com/edx/edx-platform/pull/29190 to deprecate these remaining user-related ModuleSystem attributes in favour of the user service:

user_location
get_real_user
get_user_role

These properties are only used by a few XBlocks: LTI, LTI2.0, Video (CDN/branding), edxnotes

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-2399 (OpenCraft internal ticket)

Testing instructions

Basic regression testing can be run on the sandbox.

To test the LTI 2.0 (consumer) component:

View the LTI component in Studio.
Note that the LTI URL points to a problem block on a different sandbox server's Demo course.
Ensure you can edit the component, preview it, view it in the LMS, and submit answers to the problems shown.

To test edx-notes:

Login to LMS as a student user.
View the BD-13 test course in the LMS.
Navigate to an HTML block, select some text, and ensure you can add/edit notes.
Toggle the hide/show notes button in the lower right corner to hide/show notes.
Click on the Notes tab and ensure your notes are visible there.

To test the video CDN/branding:

You need to view the site from one of the CDN-configured countries, currently Australia (AU), Pakistan (PK), or Poland (PL).

Note: I don't really have a valid CDN to use here, but enabling a CDN allows a configured branding logo to be shown under the video, so this at least is visible and can be tested.

View the test Video in the LMS.
Note that the configured branding logo is shown under the video when you view this content from one of the configured countries.

Deadline

None

Author Notes & Concerns

Setup for user location/CDN testing:

Update EDXAPP_VIDEO_CDN_URLS or /edx/etc/lms.yml to provide a CDN URL for a given country code (doesn't matter what the URL is), eg.
```
VIDEO_CDN_URL:
  AU: http://example.com/edx/video?s3_url=
```

As a superuser, navigate to Django Admin > Branding > Branding Info Config, and add a branding url, logo, and logo_tag for your country, e.g.

{
   "AU": {
     "url": "https://www.sa.gov.au",
     "logo_src": "https://www.sa.gov.au/__data/assets/image/0012/251013/sa-logo.png",
     "logo_tag": "Logo for South Australia"
   }
 }

openedx-webhooks · 2021-11-22T07:49:39Z

Thanks for the pull request, @pomegranited! I've created BLENDED-1020 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/video_module/video_module.py

common/lib/xmodule/xmodule/tests/test_lti_unit.py

common/lib/xmodule/xmodule/x_module.py

common/djangoapps/xblock_django/user_service.py

lms/djangoapps/lms_xblock/test/test_runtime.py

common/lib/xmodule/xmodule/video_module/video_module.py

common/djangoapps/xblock_django/user_service.py

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

common/lib/xmodule/xmodule/tests/test_lti20_unit.py

common/lib/xmodule/xmodule/tests/test_lti_unit.py

pomegranited · 2021-12-16T02:11:36Z

Thanks for your detailed review @Agrendalath ! I've addressed your points, and deployed a new appserver. Could you activate that one and use it for your testing?

Also, I'd like to squash my commits before merging, so let me know when you're ready for me to do that?

Agrendalath

👍

I tested this: verified that the refactored properties are working correctly
I read through the code
I checked for accessibility issues: n/a
Includes documentation
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

Agrendalath · 2021-12-16T16:23:01Z

@pomegranited, thank you for preparing the sandbox instance and nice testing instructions! Feel free to squash the commits.

I've noticed an odd behavior of the LTI provider, though it doesn't seem to be related to this PR. When I"m opening the LTI XBlock in the LMS as a "staff" user - it's working correctly. However, when I'm using the "audit", then I'm getting an error page. There is the following traceback on the pr29260 instance:

Traceback (most recent call last):
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/lti_provider/users.py", line 31, in authenticate_lti_user
    lti_user = LtiUser.objects.get(
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/query.py", line 435, in get
    raise self.model.DoesNotExist(
lms.djangoapps.lti_provider.models.LtiUser.DoesNotExist: LtiUser matching query does not exist.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/handlers/exception.py", line 47, in inner
    response = get_response(request)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/handlers/base.py", line 181, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/lib/python3.8/contextlib.py", line 75, in inner
    return func(*args, **kwds)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/newrelic/hooks/framework_django.py", line 562, in wrapper
    return wrapped(*args, **kwargs)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
    return view_func(*args, **kwargs)
  File "/edx/app/edxapp/edx-platform/common/djangoapps/util/views.py", line 188, in inner
    response = view_func(request, *args, **kwargs)
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/lti_provider/views.py", line 91, in lti_launch
    authenticate_lti_user(request, params['user_id'], lti_consumer)
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/lti_provider/users.py", line 37, in authenticate_lti_user
    lti_user = create_lti_user(lti_user_id, lti_consumer)
  File "/edx/app/edxapp/edx-platform/lms/djangoapps/lti_provider/users.py", line 59, in create_lti_user
    edx_user = User.objects.create_user(
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/contrib/auth/models.py", line 152, in create_user
    return self._create_user(username, email, password, **extra_fields)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/contrib/auth/models.py", line 146, in _create_user
    user.save(using=self._db)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/contrib/auth/base_user.py", line 67, in save
    super().save(*args, **kwargs)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/base.py", line 726, in save
    self.save_base(using=using, force_insert=force_insert,
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/base.py", line 774, in save_base
    post_save.send(
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/dispatch/dispatcher.py", line 180, in send
    return [
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/dispatch/dispatcher.py", line 181, in <listcomp>
    (receiver, receiver(signal=self, sender=sender, **named))
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/enterprise/decorators.py", line 88, in wrapper
    signal_handler(*args, **kwargs)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/enterprise/signals.py", line 68, in handle_user_post_save
    for pending_ecu in pending_ecus:
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/query.py", line 280, in __iter__
    self._fetch_all()
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/query.py", line 1324, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/query.py", line 51, in __iter__
    results = compiler.execute_sql(chunked_fetch=self.chunked_fetch, chunk_size=self.chunk_size)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/sql/compiler.py", line 1175, in execute_sql
    cursor.execute(sql, params)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/backends/utils.py", line 66, in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/backends/utils.py", line 75, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/backends/utils.py", line 78, in _execute
    self.db.validate_no_broken_transaction()
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/backends/base/base.py", line 447, in validate_no_broken_transaction
    raise TransactionManagementError(
django.db.transaction.TransactionManagementError: An error occurred in the current transaction. You can't execute queries until the end of the 'atomic' block.

pomegranited · 2021-12-17T06:31:02Z

Thanks for your review @Agrendalath ! I've rebased and squashed my commits, so this should be ready to merge once tests pass again.

I've noticed an odd behavior of the LTI provider, though it doesn't seem to be related to this PR.

The sandbox for https://github.com/edx/edx-platform/pull/29260 is the LTI provider for the LTI problem used here. I rebased that PR on latest master and redeployed it -- and it's working now for student and staff users.

pomegranited · 2021-12-17T06:50:20Z

@ormsbee Are you ok with @Agrendalath merging this PR (and https://github.com/edx/edx-platform/pull/29312), or would you rather deploy them on your timeline? I don't want to mess up anyone's holiday :) But it would be great to get these in soon.

Deprecates these ModuleSystem attributes in favor of the user service: * user_location * get_real_user * get_user_role Related changes: * Stores the user location into DjangoXBlockUserService's optional attribute as request_country_code * Uses the student model's user_by_anonymous_it to fetch the (cached) real user * Updates affected tests

from code and tests.

edx-status-bot · 2021-12-20T14:37:11Z

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

Agrendalath · 2021-12-20T14:40:47Z

@jristau1984, I'm merging this, as discussed on Slack.

openedx-webhooks · 2021-12-20T14:41:30Z

@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-12-20T18:01:45Z

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

edx-pipeline-bot · 2021-12-20T18:27:23Z

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

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

pomegranited marked this pull request as draft November 22, 2021 07:49

pomegranited force-pushed the jill/bd-13-real-user branch from b95a920 to 185bedc Compare November 23, 2021 01:56

pomegranited marked this pull request as ready for review November 25, 2021 07:40

symbolist suggested changes Nov 25, 2021

View reviewed changes

common/djangoapps/xblock_django/user_service.py Outdated Show resolved Hide resolved

common/lib/xmodule/xmodule/video_module/video_module.py Outdated Show resolved Hide resolved

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

pomegranited force-pushed the jill/bd-13-real-user branch from 50d6ecd to 887d00f Compare November 29, 2021 08:24

symbolist suggested changes Dec 2, 2021

View reviewed changes

common/lib/xmodule/xmodule/x_module.py Outdated Show resolved Hide resolved

Agrendalath reviewed Dec 15, 2021

View reviewed changes

Agrendalath approved these changes Dec 16, 2021

View reviewed changes

pomegranited force-pushed the jill/bd-13-real-user branch from bc7f32b to 75eb0d7 Compare December 17, 2021 03:36

pomegranited added 2 commits December 20, 2021 15:10

refactor: use UserService to remove deprecation warnings

e3ed12d

from code and tests.

Agrendalath force-pushed the jill/bd-13-real-user branch from 6292791 to e3ed12d Compare December 20, 2021 14:11

Agrendalath merged commit f828d89 into openedx:master Dec 20, 2021

Agrendalath deleted the jill/bd-13-real-user branch December 20, 2021 14:41

openedx-webhooks added merged and removed needs triage labels Dec 20, 2021

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

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

Uh oh!

Conversation

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

Uh oh!

Description

Supporting information

Testing instructions

Deadline

Author Notes & Concerns

Uh oh!

openedx-webhooks commented Nov 22, 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!

Uh oh!

pomegranited commented Dec 16, 2021

Uh oh!

Agrendalath left a comment

Choose a reason for hiding this comment

Uh oh!

Agrendalath commented Dec 16, 2021

Uh oh!

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

Uh oh!

Uh oh!

pomegranited commented Dec 17, 2021

Uh oh!

edx-status-bot commented Dec 20, 2021

Uh oh!

Agrendalath commented Dec 20, 2021

Uh oh!

openedx-webhooks commented Dec 20, 2021

Uh oh!

edx-pipeline-bot commented Dec 20, 2021

Uh oh!

edx-pipeline-bot commented Dec 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pomegranited commented Nov 22, 2021 •

edited

Loading

openedx-webhooks commented Nov 22, 2021 •

edited

Loading

pomegranited commented Dec 17, 2021 •

edited

Loading