Implement public cohort for anonymous and unenrolled users - #19385
Conversation
|
Thanks for the pull request, @pkulkark! I've created OSPR-2885 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. |
openedx-webhooks
added
needs triage
open-source-contribution
7757a0e to
a1d857e
Compare
|
@pkulkark Thank you for your contribution. Please let me know once it is ready to be looked at. |
openedx-webhooks
added
waiting on author
a1d857e to
cf5fbb5
Compare
|
Hi @natabene, do we need to trigger Jenkins manually here? |
There was a problem hiding this comment.
Can we move this to the top of the method? I do not see why we need to do any of above if it's an anonymous user?
There was a problem hiding this comment.
For the sake of completeness can you also check that it returns None if the course is not cohorted?
And this docstring needs to be updated.
There was a problem hiding this comment.
Does get_student_module_as_dict() not return {} if the user is Anonymous. I know this saves a query but we are calling out to the StudentModule in the courseware as well.
There was a problem hiding this comment.
The query to database fails when queried with anonymous user object with TypeError("'AnonymousUser' object is not iterable",).
There was a problem hiding this comment.
Does it make sense to push this logic down into get_student_module_as_dict, even if it means that we have to add an explicit check there for .is_authenticated there? The CSM state dict for any unauthenticated user for a given block is always going to be empty right?
The reason I ask is that unauthenticated access is a new dimension of complexity, and I'd like to see that complexity contained to as few places as possible. The more places people have to sprinkle their code with is_authenticated(), the more likely they're going to forget and we're going to get mysterious breakages related to this functionality.
There was a problem hiding this comment.
I do think that some higher level code (including XBlocks) will want to differentiate between cases where there is a valid user and where there is not. For example, even in the case of the library xblock, maybe it would be better that for anonymous users, it returns the first n children xblocks always instead of a random subset on each page refresh as it is doing right now? (Library xblocks aren't really important for open courses, so we didn't spend too much time on figuring out the right product strategy here but this can be changed if required)
But maybe a lot of places do not need to differentiate. In that case do we want the lower layer to properly support being passed anonymous user objects? My thinking is "higher layer should only call out to the KVS/SM if there is a valid user" because the state is scoped to users and "please get or save (in the case below) this data unless this user is None in which case do nothing." seems a bit unergonomic? Is there a nice way of doing it while pushing the logic down?
There was a problem hiding this comment.
I think it's fine that XBlocks differentiate when they find it useful to do so. But I think it's also reasonable to make get_student_module_as_dict check for anonymous users and do the right thing for them (as opposed to just blowing up).
In any case, it's not something I feel strongly enough to block merging over. Just a long term concern I have. If it's really just this XModule doing it, then it's not worth worrying over. If it becomes a recurring pattern in other classes, then I would get more concerned.
There was a problem hiding this comment.
I moved the is_authenticated check to get_student_module_as_dict method.
There was a problem hiding this comment.
This thing with this approach is that it is going to cause make_selection to return a different set of XBlocks on every view. To keep it stable may make more sense to add a mode first which returns the first max_count items from library_children.
And we can move the check to the first line: if not isinstance(usage_info.user, AnonymousUser) and any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')):
There was a problem hiding this comment.
Sorry I don't think I quite understood. We don't want it to return a different set of Xblocks on every view? I was under the impression that since we select Randomized Content Block while creating the course, the expected behaviour is that it should return random block(s) from the library. Is that not the case?
There was a problem hiding this comment.
My understanding is that the first time a learner accesses the XBlock a random set of children is calculated and stored in state[selected]. And they continue to see those same children in subsequence views (unless the children or the max_count values change where the list of children is updated).
Having said so I just realize that we do not have a public_view for the library xblock (the learner will be seeing the "Please enroll to view" message) and since we do not need to support it either for now lets just move the check to the outer if and let make_selection return a random set every time as it is the case right now.
Nit: the convention is to use [is_authenticated](https://docs.djangoproject.com/en/2.1/ref/contrib/auth/#django.contrib.auth.models.User.is_authenticated) instead of the type of user objects.
There was a problem hiding this comment.
Thanks for the clarification. I've moved the check to the outer if.
|
jenkins ok to test |
12f1c97 to
1cab41f
Compare
There was a problem hiding this comment.
For edX reviewers: Library module is not supported right now. This change is just to prevent errors in case the course has one.
There was a problem hiding this comment.
Nit: The indent here is a bit awkward. I think the recommendation is to use parens around the whole statement and pull the any to the second line.
More importantly: This might be another place where it makes sense to push the logic down the stack. Say a new classmethod on StudentModule that essentially does update_or_create() like the call below, but has a check for is_authenticated() -- assuming of course that it's always wrong to update state for unauthenticated users (which I think is correct?).
This is a question/suggestion, and not a requirement for merging.
There was a problem hiding this comment.
I've changed the indent. As for moving the logic down the stack, I don't really know much about which level to contain the unauthenticated user check makes sense, but wouldn't moving it down increase the function call overhead when the user is not authenticated? Since we'll now be calling the StudentModule classmethod which then calls the is_authenticated, in place of just is_authenticated?
There was a problem hiding this comment.
I guess I just assumed that is_authenticated was really cheap. Is it not?
There was a problem hiding this comment.
is_authenticated is probably cheap since it's used in a lot of places. I was more concerned with the call to the StudentModule classmethod. But on second thought, it's probably not something to worry about either. I've created a new classmethod and moved the logic to there. Please let me know if that's fine.
There was a problem hiding this comment.
We also need to check the course_visibility setting.
b256f7f to
2612a14
Compare
|
@mduboseedx Apparently the xsscommitlint run is failing with quite a few errors in one file which was slightly edited in this PR. Will it require fixing all of them? |
|
jenkins run quality |
|
@symbolist I think those errors will need to be addressed in order to merge this change. @edx/testeng do all xsscommitlint errors need to be fixed? |
|
Fixing the XSS warnings is strongly encouraged, but not strictly required. We have a backlog of these "might be a serious problem" code spots identified by the linter, and are trying to fix them as other work is being done in the same files (when the context of that part of the code is already fresh in mind). Documentation on how to avoid and fix these problems can be found here, and this is information that we'd like anybody working on HTML templates or code-generated HTML in Open edX to understand. But if there's a good case for not tackling the issues surfaced by xsscommitlint right now, we can merge the PR despite the linter failure. |
|
@jmbowman @mduboseedx Thanks for the details! Yes, we do follow the guidelines for preventing xss issues. However there are 28 xsslint warnings showing up in this file (none of which are being introduced in this PR) and fixing all those will likely require refactoring it quite a bit. I think it would be better if that is handled separately with any other cleanup the edX team would like. |
pkulkark
changed the title
2612a14 to
e7d3b32
Compare
|
@pkulkark @symbolist Do you think it should be reviewed now? |
|
@natabene The quality check failures are not related to the the changes made in this PR, as mentioned in earlier comments. So this can be reviewed. |
openedx-webhooks
removed
the
waiting on author
|
@nedbat This is a big change, but it's been done in careful consultation with @ormsbee and @marcotuts (cf https://github.com/edx/edx-platform/pull/18134). It also rounds out the Anonymous Courseware feature really nicely, so would be great if we could get it into Ironwood. Re documentation -- I know this is no substitute for real docs, but we're working on a blog post for the edX website that also explains (with screenshots) how to enable and use Anonymous Courseware, so there will be something for people to reference. |
|
@pkulkark: Is this ready for another review round? |
|
@ormsbee Yes it is |
openedx-webhooks
removed
the
waiting on author
|
@pkulkark: Got it. FWIW, I'm trying to use JIRA more diligently now that I'm tracking more community PRs at once. If you see that a PR has a "Waiting on Author" label when it's really ready for another review round, please let me know that it's ready in a PR comment. Otherwise, it might be a while before I notice. :( Thank you! |
6a77a35 to
a267537
Compare
|
jenkins run bokchoy |
2 similar comments
|
jenkins run bokchoy |
|
jenkins run bokchoy |
This PR is based on openedx#19284 and is part of the series of work related to the proposal openedx#18134. This PR avoids the assignment of anonymous/unenrolled users to any cohort when course is public. Anonymous or unenrolled users will only see content that does not have a content group assigned. The "View Course" link to the course outline is shown on the course about page for a course marked public/public outline. It also makes course handouts available for public courses (not for public_outline). This PR also hides the different warnings and messages asking the user to sign-in and enroll in the course, when the course is marked public. It modifies the default public_view text to include the component display_name when unenrolled access is not available.
a267537 to
9ddb1cc
Compare
|
@ormsbee This is ready for another round of review, thank you! @edx/ospr @natabene -- @pkulkark is unable to trigger jenkins runs with PR comments like |
|
Your PR has finished running tests. The following contexts failed:
|
|
Note for anyone watching: I am merging this in spite of quality check failures because it's XSS linting issues that were not introduced by this PR. |
|
@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 Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, February 11, 2019. |
|
EdX Release Notice: This PR may have caused e2e tests to fail on Stage. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/STAGE_edxapp_M-D |
|
Even if we do bring it in, I'd definitely want to wait a little bit just to make sure there are no surprise regressions when we go to prod with this over the next few days. |
|
@ormsbee Understood. Our client has noted an issue with the "Enroll" links that are shown on public and public_outline Course pages -- if the course enrollments are "invite only", then the user is looped back to the dashboard, but not enrolled in the course (and no explanatory message shown). They'd like the "Enroll" links to be displayed only if self-enrollment is possible. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
|
@ormsbee Could we get an Ironwood pull request today? |
|
Added to Ironwood via #19877 |
12 participants
This PR is based on #19284 and is part of the series of work related to the proposal #18134 .
Description:
Allow anonymous and unenrolled users access to public courses with cohorts enabled.
Current behaviour is that if the course had cohorts enabled and any user accessing the course that is not assigned to a cohort gets assigned to a "default" cohort. This PR avoids the assignment of anonymous/unenrolled users to any cohort when course is public. Anonymous or unenrolled users will only see content that does not have a content group assigned.
The "View Course" link to the course outline is shown on the course about page for a course marked public/public outline. It also makes course handouts available for public courses (not for public_outline).
This PR also hides the different warnings and messages asking the user to sign-in and enroll in the course, when the course is marked public. It modifies the default public_view text to include the component display_name when unenrolled access is not available.
Sandbox server:
LMS: https://pr19385.sandbox.opencraft.hosting/
Studio: https://studio-pr19385.sandbox.opencraft.hosting/
Contains 2 courses:
edX Demo Course has
seo.enable_anonymous_courseware_accesscourse waffle flag, and the "Advanced Settings > Course Visibility For Unenrolled Learners" set to public.Test Course also is set to public. It contains 3 units : Unit-1 is accessible to all, Unit-2 has access restricted to "test_cohort" and can only be seen by the
verified@example.comuser and Unit-3 is a randomized content block that can be viewed only when the user is enrolled.Testing Instructions:
Create a course and enable cohorts.
Mark the course as public and try accessing the course content as an anonymous or unenrolled user. The course content not assigned to any content group should be accessible. None of the warnings should be seen.
Mark the course as public/public_outline and access the course about page. There should be a "View Course" button visible which links to the course outline.
If the course is marked public, the course handouts should be available to anonymous/unenrolled users as well. If course is marked public_outline, handouts should not be available.