Course visibility options - #18537
Conversation
|
Thanks for the pull request, @viadanna! I've created OSPR-2510 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here. If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information. |
openedx-webhooks
added
needs triage
open-source-contribution
|
@viadanna Thanks for the contribution! Let me know when this is ready for a review. |
openedx-webhooks
added
waiting on author
symbolist
left a comment
symbolist
left a comment
There was a problem hiding this comment.
@viadanna My understanding is that this task adds a setting field to the course and for now that is being kept behind the existing waffle flag. So something like this:
- A setting needs to be added to https://github.com/edx/edx-platform/blob/master/common/lib/xmodule/xmodule/course_module.py#L182 whose values can be
private(default), 'preview' orpublic. - This setting should only be visible in Studio Advanced Settings if the WaffleFlag
enable_anonymous_courseware_accessis enabled for the course. - This setting should only be used by the
CourseHomeViewif the WaffleFlagenable_anonymous_courseware_accessis enabled for the course. If the flag is enabled for the course, then you can do checks likeif course.access is 'preview':and make the relevant changes to theCourseHomerendering.
@xitij2000 Can you please confirm the acceptance criteria for this?
There was a problem hiding this comment.
Lets not make changes to things besides the outline. If the user is anonymous, CourseHomeFragmentView should not render the WelcomeMessageFragmentView.
|
@symbolist @viadanna Yes, that is the correct acceptance criterion. This needs to be a course setting that itself is only activated/visible/effective if the waffle flag is set. |
There was a problem hiding this comment.
The multiple checks here are making the code complex quite complex. Try this instead:
- Move all the assignments to
Nonein theelse:clause above the lineif user_access['is_enrolled'] or user_access['is_staff']:. - Then instead of modifying the
if, add another clause:
else if (course.access is "preview" or course.access is "public"):
outline_fragment = <init>
course_sock_fragment = <init>
This will make it clear what is being set up in each of the three cases.
There was a problem hiding this comment.
Showing the handouts for anonymous users is also out of scope of this task as far as I understand. In that case, you can move this to the first if clause above instead.
There was a problem hiding this comment.
I think the requirement is that if the course access is set to public the outline should show and the links should be enabled. And if the course access is set to preview, the outline should show but the links are not enabled. This code is disabling the links in both cases.
There was a problem hiding this comment.
Hum, that's strange, it's called with the value status of allow_preview and renders correctly in the sandbox.
There was a problem hiding this comment.
Looking at this again, you are right. :) It will likely be clearer if the parameter was made enable_anonymous_access (which would be passed the value of the WaffleFlag) and then in the context the exact criteria was listed:
'enable_links': request.user.is_authenticated or (enable_anonymous_access and request.user.is_anonymous() and course.access is 'public')
There was a problem hiding this comment.
Not setting the href may make more sense than changing the element type (though not sure what the a11y requirements are). If you want to go with the span, the styling needs to be updated. Currently, the unit items are all showing in a single line.
There was a problem hiding this comment.
That's odd, I've updated the scss and this was working here. I'll review that approach.
There was a problem hiding this comment.
Hmm. Okay, it could just be that my assets were not updated correctly. I'll check it again in the next pass.
|
Thanks for clearing up the acceptance criteria @symbolist @xitij2000 |
There was a problem hiding this comment.
You should be able to access the course_key via course_module.id or descriptor.id (if it is a course descriptor) and so shouldn't need to pass it as a separate parameter if either is also a parameter. For a bit of context before XBlocks the equivalent functionality was provided by a Descriptor and Module pair. Course hasn't been migrated over yet which is why the disparate APIs.
There was a problem hiding this comment.
Much better, alright.
There was a problem hiding this comment.
Nit: Can we go with user.is_authenticated? It seems clearer than a not.
There was a problem hiding this comment.
This context manager needs to be moved as well. It is needed when accessing the modulestore which is being done in get_course_with_access().
There was a problem hiding this comment.
On second thoughts, I would recommend leaving the CoursewareIndex without any changes for now. The next task will be making further changes to it and it makes sense to do all in one go.
There was a problem hiding this comment.
You mean no further changes? Reverting this back will break the functionality.
There was a problem hiding this comment.
Well, the only effect of the changes in this file is restricting courseware access to the case where course_visibility is set to public. If you want to keep this change as part of this PR, here are a few points:
- The
get_course_with_access()needs to be inside atrystatement as it was originally. - I am not sure if there is any performance impact of splitting the work across two
bulk_operationscontext managers.
How about instead of moving get_course_with_access() just move the access check and redirect to before self.is_staff = has_access(request.user, 'staff', self.course) below? That should get the expected result but the course content loading/rendering logic remains exactly the same.
There was a problem hiding this comment.
Ok, that's a good solution.
There was a problem hiding this comment.
Nit: Can we go with request.user.is_authenticated? It seems clearer than a not.
There was a problem hiding this comment.
You forgot to update the display_name values.
There was a problem hiding this comment.
Oh, what a distraction, will fix that.
There was a problem hiding this comment.
There is already a test for the flag: https://github.com/open-craft/edx-platform/blob/aa7599b49f1af7566f5a858c784f6b9687d439cd/lms/djangoapps/courseware/tests/test_views.py#L2332 . So instead of creating a new class, you can just add these ddt variations to it.
Also instead of patching the descriptor, you should be able to do course = CourseFactory(course_visibility= course_visibility).
There was a problem hiding this comment.
Otherwise the test looks good.
There was a problem hiding this comment.
I've created a new class to perform a different setUp. It make sense to have all in a single place, but the code was getting very complex.
On the patching, do you mean creating a new course on each variation of the ddt?
Nevermind, I got it.
There was a problem hiding this comment.
Can you explain where all the extra setup that is being done in this setUpClass() is being used? For the purposes of this test, what more do you need than what is already happening in https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/tests/test_views.py#L2333-L2336?
Since the existing test is creating the course in the test, I meant when merging you can just add the parameter to the CourseFactory() call.
There was a problem hiding this comment.
Yeah, you recommendation made sense on a second look at the code, that's unnecessary.
I have updated the code, I'll just make the necessary changes to the other test and push the changes.
There was a problem hiding this comment.
There are now 5-6 places in lms and openedx where this flag is being defined. Can you create one constant in https://github.com/edx/edx-platform/blob/master/openedx/features/course_experience/__init__.py and use it everywhere? (Make sure to follow the naming convention of the existing flags).
There was a problem hiding this comment.
Yeah, this crossed my mind as well, but fell through the cracks.
I'l do that now.
There was a problem hiding this comment.
Is this message not going to be visible in all cases?
There was a problem hiding this comment.
Actually, this was meant for the You must be enrolled in the course to see course content, I just shortened it for readability.
But explicit is better, so I'll adjust that.
There was a problem hiding this comment.
Can this not be merged with test_home_page() so that status of all sub-fragments can be tested? For example, the 3 ddt variations with logged in users can have the flag False and these 6 variations can have CourseUserType.ANONYMOUS.
There was a problem hiding this comment.
Also instead of patching see the update_item(self.course, self.user.id) pattern in courseware/tests/test_views.py for updating the course module fields.
There was a problem hiding this comment.
I tried merging at first but the test was getting very complex, so I decided to keep those separate.
I moved to patching as updating the course module was causing troubles on other tests, even when rolledback the changes.
There was a problem hiding this comment.
Since the existing test checks the visibility of the different fragments, it makes sense to have one which checks all the permutations. The new test is not checking the visibility of the other fragments. The main changes I see are:
test_home_page(self, enable_anonymous_courseware_access, course_visibility, user_type, expected_message)- The
patchandoverride_waffle_flagneed to go around theself.client.get()as in the new test. - Updating one assert:
self.assertContains(response, TEST_CHAPTER_NAME, count=1 if (is_enrolled or is_unenrolled_staff or (enable_anonymous_courseware_access and course_visiblity in ['preview', 'public'])) else 0)
What else needs to change?
There was a problem hiding this comment.
Makes sense, I also had to change the expected number of welcome messages.
6a4e571 to
9df137c
Compare
|
jenkins run bokchoy |
4dfda0c to
e629775
Compare
|
jenkins run quality |
There was a problem hiding this comment.
Thinking about this some more, this method should not care about authentication. If user.is_authenticated is False, user=None should be passed to it so that this extra check is not needed here. I think the only change that will be required is in edx-platform/openedx/features/course_experience.
There was a problem hiding this comment.
You can get the value via kwargs['enable_anonymous_access'] instead of adding a parameter.
There was a problem hiding this comment.
Can you move this where it was so that it does not look like a change?
There was a problem hiding this comment.
Actually, moving this changed the behaviour of this function redirects when the user wasn't logged in and anonymous access was disabled.
I don't like this repetition either.
There was a problem hiding this comment.
I meant to line 99 above. That should not have any effect right?
There was a problem hiding this comment.
Oh, sorry, I misunderstood your request.
There was a problem hiding this comment.
We will likely need a different message for this case but it is something the edX UX can recommend.
There was a problem hiding this comment.
Can you also add checks that CourseUserType.UNENROLLED see the same content as anonymous learners?
There was a problem hiding this comment.
While the checks are all correct, they can be made clearer to read by moving self.assertContains(response, TEST_CHAPTER_NAME, count=expected_chapter_count) and all the variables that are only needed for it to line 261 (and leave an empty line). You can add a comment like "# Verify the outline is shown for enrolled users and if needed for anonymous users."
There was a problem hiding this comment.
Also is your editor line width set to 80? edX uses 120 and I think there are a lot of multi-lines in this PR which would not be.
There was a problem hiding this comment.
Yeah, that makes it more readable.
On the multilines, I've must have set that manually for readability, I'll review it and update accordingly.
|
Hello there, @mduboseedx |
|
@edx/educator-product Could you give this a quick look? This is related to course visibility and uses a waffle flag. |
openedx-webhooks
added
product review
35cb113 to
c2c2dc5
Compare
|
@sstack22 Have you had a chance to give this a look over from a product perspective? |
|
@mduboseedx @sstack22 We've found an issue in our implementation regarding the block list transformers, so you can delay this review until we fix this. Sorry about that. |
|
jenkins run bokchoy |
a6f367d to
b2e28fb
Compare
symbolist
left a comment
symbolist
left a comment
There was a problem hiding this comment.
@viadanna Given the updates to https://github.com/edx/edx-platform/pull/18134, we need to make sure that the logged-in but unenrolled learners see the same content as anonymous learners. I have left some notes about those. Also thanks for the updates to the transformers checks! Can you also add a test for the MilestonesAndSpecialExamsTransformer when usage_info.user.is_authenticated is False?
| assert response.status_code == expected_status | ||
|
|
||
| user = UserFactory() | ||
| CourseEnrollmentFactory(user=user, course_id=course.id) |
There was a problem hiding this comment.
Since now we are using this behavior for unenrolled users as well, can you also check before enrolling the user that they see what anonymous users see?
| depth=CONTENT_DEPTH, | ||
| check_if_enrolled=not self.enable_anonymous_courseware_access, | ||
| ) | ||
| if not (request.user.is_authenticated or self.course.course_visibility == 'public'): |
There was a problem hiding this comment.
Does the check needs to be updated here too?
|
|
||
| # Waffle flag to enable anonymous access to a course | ||
| SEO_WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(name='seo') | ||
| COURSE_ENABLE_ANONYMOUS_ACCESS_FLAG = CourseWaffleFlag(SEO_WAFFLE_FLAG_NAMESPACE, 'enable_anonymous_courseware_access') |
There was a problem hiding this comment.
Can this be renamed to COURSE_ENABLE_UNENROLLED_ACCESS_FLAG ?
There was a problem hiding this comment.
Can you also add checks that CourseUserType.UNENROLLED see the same content as anonymous learners?
| user=request.user, | ||
| course_key=course_key | ||
| ) | ||
| if request.user.is_authenticated: |
There was a problem hiding this comment.
This would need an update too.
| if not course_block_tree: | ||
| return None | ||
|
|
||
| enable_links = request.user.is_authenticated or ( |
There was a problem hiding this comment.
This method may also need a couple of updates for unenrolled but logged-in learners.
42fe970 to
9e77345
Compare
|
Your PR has finished running tests. The following contexts failed:
|
|
Closed in favor of #18720 |
|
@viadanna 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. |
openedx-webhooks
added
rejected
and removed
product review
6 participants
This PR adds the course setting
course visibilityin CMS to allow preview and public access.This requires the course waffle flag
enable_anonymous_access.Feature description based on this PR
Settings / Options
Thinking through potential combinations of settings and override concerns for authors I might suggest a minor change to what you suggested above.
Course Visibility Default Setting: Public, Preview Only, Private
Public: Course outline and all units are available by default publicly, unless a subsection or unit is forced to be private.
Preview Only: Only the course outline is available publicly but content pages are locked / gated with a message explaining that users must log in to access the content.
Private: Our current default, but anybody who attempts to access specific content pages is redirected to the course home page (current behavior.)
Sandbox URL:
LMS
CMS
Testing instructions:
seo.enable_anonymous_acesswaffle flag to the course and make sure theCourse Visibility For Unauthenticated Studentssetting appear for the course in Studio.Course Visibility For Unauthenticated Studentsadvanced setting for the course topreview.Course Visibility For Unauthenticated Studentsadvanced setting for the course topublic.Author notes and concerns:
Reviewers