SE-183 Enable video content for anonymous and unenrolled learners by pkulkark · Pull Request #19305 · openedx/openedx-platform

pkulkark · 2018-11-22T17:41:27Z

This PR is based on #19284 and is part of the series of work related to the proposal #18134.

Description:

Adds VideoModule.public_view() to enable unenrolled and anonymous users to view the video contents of a public course.
When an unenrolled or anonymous user accesses the video content of a public course,
the public_view() introduced in the previous PR is used instead of student_view() method.

Sandbox server:

LMS : https://pr19305.sandbox.opencraft.hosting/
Studio : https://studio-pr19305.sandbox.opencraft.hosting/

Contains 2 courses:

edX Demo Course unmodified, and with no added course waffle flags.
Test Course has the seo.enable_anonymous_courseware_access course waffle flag, and the "Advanced Settings > Course Visibility For Unenrolled Learners" set to public.

Testing Instructions:

Open the test course above and click on the unit without logging in. The video should be visible and all the options can be used without errors.
Open the test course above and login using any of demo user accounts but do not enrol in the course. The video should be visible and all options can be used without errors.

symbolist · 2018-11-23T09:48:14Z

jenkins run all

symbolist · 2018-11-26T13:53:24Z

Hi @natabene! Is there any specific reason Jenkins is not running for this PR?

mduboseedx · 2018-11-26T18:27:15Z

jenkins ok to test

symbolist · 2018-11-26T20:54:48Z

Instead of isPublic, it would be better to have parameters which disable specific behaviors. See https://github.com/edx/edx-platform/pull/18344/files.

@symbolist Sorry this is not very clear to me. Can you please elaborate? I am disabling specific behaviour (i.e. saving user state) based on the isPublic parameter here. Do you mean isPublic is not a correct parameter name to be used? Or that I should have this check where the function (in the methods below) is called? https://github.com/edx/edx-platform/blob/f3d58debb7cb62682abfb289479c4ca89c038ed9/common/lib/xmodule/xmodule/js/src/video/09_save_state_plugin.js#L69-L98

Sorry, I meant to say that the Javascript does not need to know about what kind of view it is. So we can call it something like saveStateEnabled just like we have completionEnabled flag which toggles the completion feature.

Thanks that makes sense. I've changed it accordingly.

symbolist · 2018-11-26T21:14:31Z

Instead of this, can you look into not enabling the completion service if the user is anonymous here: https://github.com/edx/edx-platform/blob/master/lms/djangoapps/lms_xblock/runtime.py#L140.

In case the user us signed-in but unenrolled, can you try setting the effective_user to AnonymousUser() if self.view is PublicView here: https://github.com/edx/edx-platform/blob/5f9fe4a529237f348059a77baa7218275fab8145/lms/djangoapps/courseware/views/index.py#L162

@symbolist I added the check to disable completion service if user is anonymous at the above mentioned line in runtime.py file and also set the effective_user to AnonymousUser() in the index.py file. However, while testing I noticed that the completion service was being set before the effective_user change (i.e. runtime.py file is executed before the index.py) and so the completion service was being enable for the unenrolled (but signed-in) users. I removed the change from the index.py file and added a check for unenrolled user while setting the completion service. Let me know if that needs to be changed.

Hmm. In my testing, LMSModuleSystem is initialized inside render(). From which method do you see it getting called before _setup_masquerade_for_effective_user()?

__init__ [LmsModuleSystem] runtime.py:140 get_module_system_for_user module_render.py:820 get_module_for_descriptor_internal module_render.py:877 get_module_for_descriptor module_render.py:433 _prefetch_and_bind_course [CoursewareIndex] index.py:331 render [CoursewareIndex] index.py:150 get [CoursewareIndex] index.py:126

I think I found the reason for that. While testing, I happened to first click on the course which directed to the url /courses/<course-id>/about (which initialised LMSModuleSystem at https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/module_render.py#L765) and then I changed it to /courses/<course-id>/course.
But if I open /courses/<course-id>/course directly (i.e. without opening other urls), it initialises LMSModuleSystem from render(). Having the check while enabling/disabling the completion service handles both the scenarios. But if that's unnecessary, I can go with the other approach of setting the effective_user to AnonymousUser(). Let me know your thoughts.

Great! We do not want to add knowledge about enrollment/access to the Runtime (it is a higher-level property) so lets go with setting the effective_user to AnonymousUser().

When you make this change, can you please squash all the commits you have created in this PR so that I can give it a final pass?

Thanks @symbolist ! That makes sense. I've made the required changes and squashed all the commits. Let me know if any other changes are required.

openedx-webhooks · 2018-11-27T15:19:22Z

Thanks for the pull request, @pkulkark! I've created OSPR-2828 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:

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.

symbolist · 2018-11-29T18:31:37Z

For each of these tests can you add the check for what happens if saveStateEnabled is not true as well? For example, in this test if we put the following as the first two lines it should pass:

state.el.trigger('speedchange', ['2.0']); expect($.ajax).not.toHaveBeenCalled();

With the saveStateEnabled set to false, it makes an ajax call to /event. So I've added checks to verify that it doesn't call savestate when saveStateEnabled is false.

Great, thanks!

symbolist · 2018-11-29T19:13:11Z

A better place for testing this would be in this class: https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/tests/test_video_mongo.py#L227.

Moved it. Thanks.

symbolist · 2018-11-29T19:15:48Z

You can specify the default view=STUDENT_VIEW in the get_html() parameters. Then this method will not be needed as the implementation in the super class XModule will be sufficient.

The student_view method in the super class only has context as a parameter (no view parameter). Hence I've overridden that here.

Yes, but then you can change public_view body to return Fragment(self.get_html(PUBLIC_VIEW)) instead of proxying it through student_view. Also we don't need the docs when just overridding from a superclass without change in behavior. So we can remove that.

symbolist · 2018-11-29T19:26:08Z

The check for enrollment happens inside check_course_access() and we do not want to call this directly (the bugfix PR that this is based on actually fixed this exact bug in get() above). Since we have already checked this above we can simply do something like:

if self.view == PUBLIC_VIEW: self.masquerade, self.effective_user = None, AnonymousUser() else: self.masquerade, self.effective_user = setup_masquerade(

Makes sense. I've updated it accordingly.

symbolist · 2018-11-29T19:32:47Z

We shouldn't initialize the service if we don't need it. Is there a need to put a None in? Or is the following sufficient:

user = kwargs.get('user') if user.is_authenticated: services['completion'] = CompletionService(user=kwargs.get('user'), course_key=kwargs.get('course_id'))

I was just being extra cautious as I wasn't sure if that change might affect any other code using it :-).
I've changed it now. Thanks.

symbolist · 2018-12-03T15:47:32Z

Great, thanks!

symbolist · 2018-12-03T16:19:16Z

Yes, but then you can change public_view body to return Fragment(self.get_html(PUBLIC_VIEW)) instead of proxying it through student_view. Also we don't need the docs when just overridding from a superclass without change in behavior. So we can remove that.

pkulkark · 2018-12-04T11:36:30Z

jenkins run python

symbolist · 2018-12-05T12:50:34Z

@ormsbee This PR is next in the series of work for access for unenrolled users. It adds the public_view for the video XBlock. Have a couple of questions here:

When a signed-in but unenrolled user visits a course open to public self.request.user = self.effective_user causes the template to show them as logged out. One option is that we undo this change and not explicitly set self.effective_user = AnonymousUser() for these users. Another is that we set self.request.user = self.effective_user only for student_view (i.e. in the else clause). What would the better option? Is there a third one?

I see you added the read_only parameter to FieldDataCache.cache_for_descriptor_descendents(). Should we set that for the public_view as well?

Thanks.

When a signed-in-but-unenrolled user visits a video marked for public access, I think it's fine if they have their user state carry with them (i.e. not set them to anonymous).

The read_only flag is there to minimize lock contention. It was added because people would run crawlers with requests that would block each other while trying to update their sequence position in CSM. Since we might want to write CSM state for people who are logged in and unenrolled, I think it's fine if public_view does not use the read_only flag there.

@ormsbee Thanks!

@pkulkark That means we can undo the changes in this file.

symbolist · 2018-12-05T12:52:38Z

@pkulkark Thanks! I have reviewed and tested on the sandbox. Overall looks good now. I do have a couple of questions from Dave about the expected behavior though. And we will have to wait until https://github.com/edx/edx-platform/pull/19284 gets merged.

symbolist

I tested this by checking out video xblocks without being signed in and that publish_completion events continue to fire correctly for logged in users.
I read through the code
I checked for accessibility issues. N/A.
Includes documentation

symbolist · 2018-12-13T20:04:58Z

@ormsbee This is ready for a review from you. It makes a tweak to the completion service init as well which throws an error if enabled and passed an anonymous user.

ormsbee

Looks good, but please supplement the current commit message with the extra context that you provide in the description of this PR (not the testing or sandbox stuff, but the part where you describe code changes and related PRs). I'll merge as soon as that's done and tests pass.

This is based on PR openedx#19284 and is part of the series of work related to the proposal openedx#18134. Adds VideoModule.public_view() to enable unenrolled and anonymous users to view the video contents of a public course. When an unenrolled or anonymous user accesses the video content of a public course, the public_view() introduced in the previous PR is used instead of student_view() method.

edx-status-bot · 2018-12-20T09:02:16Z

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

pkulkark · 2018-12-20T09:04:01Z

@ormsbee I've rebased and updated the commit message as required. Let me know if anything else is needed.

openedx-webhooks · 2018-12-20T14:09:14Z

@pkulkark 🎉 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 · 2018-12-20T15:39:46Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Friday, December 21, 2018.

edx-pipeline-bot · 2018-12-21T15:40:41Z

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

pkulkark force-pushed the pooja/access-videos-for-unenrolled-learners branch from 8af2639 to 1515f10 Compare November 23, 2018 11:40

symbolist suggested changes Nov 26, 2018

View reviewed changes

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Nov 27, 2018

pkulkark force-pushed the pooja/access-videos-for-unenrolled-learners branch from 8747bad to 932eda3 Compare November 29, 2018 08:57

symbolist suggested changes Nov 29, 2018

View reviewed changes

pkulkark force-pushed the pooja/access-videos-for-unenrolled-learners branch 4 times, most recently from 4f87230 to 49e0fb1 Compare December 3, 2018 05:54

symbolist suggested changes Dec 3, 2018

View reviewed changes

pkulkark force-pushed the pooja/access-videos-for-unenrolled-learners branch 2 times, most recently from b17eb58 to 46360eb Compare December 4, 2018 07:22

pkulkark force-pushed the pooja/access-videos-for-unenrolled-learners branch from 46360eb to a2df2a3 Compare December 4, 2018 11:47

symbolist reviewed Dec 5, 2018

View reviewed changes

pkulkark changed the title ~~WIP: Enable video content for anonymous and unenrolled learners~~ Enable video content for anonymous and unenrolled learners Dec 5, 2018

pkulkark changed the title ~~Enable video content for anonymous and unenrolled learners~~ SE-183 Enable video content for anonymous and unenrolled learners Dec 6, 2018

pkulkark force-pushed the pooja/access-videos-for-unenrolled-learners branch 2 times, most recently from 18d3260 to bf53e6e Compare December 12, 2018 11:51

symbolist approved these changes Dec 13, 2018

View reviewed changes

openedx-webhooks removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 14, 2018

openedx-webhooks added the awaiting prioritization label Dec 14, 2018

ormsbee self-assigned this Dec 19, 2018

ormsbee added engineering review and removed awaiting prioritization labels Dec 19, 2018

ormsbee approved these changes Dec 19, 2018

View reviewed changes

ormsbee added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed engineering review labels Dec 19, 2018

pkulkark force-pushed the pooja/access-videos-for-unenrolled-learners branch from bf53e6e to 6f0e5d6 Compare December 20, 2018 08:21

ormsbee merged commit b9eea30 into openedx:master Dec 20, 2018

pkulkark mentioned this pull request Mar 11, 2019

Public Courseware Access open-craft/openedx-platform#158

Merged

openedx-webhooks added merged and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Dec 18, 2020

bradenmacdonald deleted the pooja/access-videos-for-unenrolled-learners branch January 12, 2022 01:46

Uh oh!

Conversation

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

Uh oh!

Uh oh!

symbolist commented Nov 23, 2018

Uh oh!

symbolist commented Nov 26, 2018

Uh oh!

mduboseedx commented Nov 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openedx-webhooks commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkulkark Dec 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkulkark commented Dec 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

symbolist commented Dec 5, 2018

Uh oh!

symbolist left a comment

Choose a reason for hiding this comment

Uh oh!

symbolist commented Dec 13, 2018

Uh oh!

ormsbee left a comment

pkulkark commented Nov 22, 2018 •

edited

Loading

openedx-webhooks commented Nov 27, 2018 •

edited

Loading

pkulkark Dec 3, 2018 •

edited

Loading