Skip to content

Support anonymous users with new (Blockstore-based) XBlock Runtime - #22427

Merged
ormsbee merged 1 commit into
openedx:masterfrom
open-craft:anonymous-user-state
Dec 20, 2019
Merged

Support anonymous users with new (Blockstore-based) XBlock Runtime#22427
ormsbee merged 1 commit into
openedx:masterfrom
open-craft:anonymous-user-state

Conversation

@bradenmacdonald

@bradenmacdonald bradenmacdonald commented Dec 3, 2019

Copy link
Copy Markdown
Contributor

This PR updates the new (blockstore-based) XBlock runtime (python API + REST API) so that it can store state for anonymous (non-registered) users. This can be used to allow people to preview course content (or library content, or any other learning context) without having to register an account.

Implementation details:

  • Anonymous users are assigned a unique ID (like anon42c08f9996194e2a9339) which gets stored in the django session.
    • block.scope_ids.user_id and block.runtime.anonymous_student_id will both return this value.
  • User state for anonymous users is stored in the django cache and automatically expires after two days.
  • There is no mechanism for upgrading to a registered account and keeping user state since the user state store for anonymous users (EphemeralKeyValueStore) is completely different than the one for registered users (DjangoKeyValueStore/"CSM"), and has no "list all keys" functionality.
  • "User State Summary" field values are shared among [recently active] anonymous users but are not shared with registered users.

This also fixes the Studio behavior of XBlocks in the new runtime so that they store user state in the cache too, very similar to the way Studio today stores user state in the session. (Before this PR, user state for Studio testing of XBlocks was just stored in a dict that was destroyed after each request-response cycle.)

Manual test instructions

Make sure blockstore is running.

Install/update the very latest version of Ramshackle.

Check out this branch and restart Studio/LMS.

Go to http://localhost:18010/ramshackle/ and find or create a drag-and-drop-v2 XBlock in a content library. Click on "Actions" then right-click "this link" for testing anonymously and open it in an incognito window:
ramshackle

Drag some items around then refresh the page or open the same URL in another tab (in the same incognito session) and notice that the state is preserved.

Then try with other XBlock types - problem, video, html, etc.

Running the test suite instructions

Unit tests that require blockstore currently do not run automatically because devstack doesn't yet ship with Blockstore. You need to run them manually:

First, from the blockstore directory, run make testserver to start a Blockstore instance for testing.

Then from make studio-shell run the following:

EDXAPP_RUN_BLOCKSTORE_TESTS=1 python -Wd -m pytest --ds=cms.envs.test openedx/core/lib/blockstore_api/ openedx/core/djangolib/tests/test_blockstore_cache.py openedx/core/djangoapps/content_libraries/tests/

Then from make lms-shell run the same command but with cms.envs.test changed to lms.envs.test

@bradenmacdonald
bradenmacdonald force-pushed the anonymous-user-state branch 3 times, most recently from caf2cb5 to 3a04132 Compare December 4, 2019 01:59

@symbolist symbolist left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I tested this out and it is working as expected!

Comment thread cms/envs/common.py Outdated
Comment thread cms/envs/common.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, are we calling it v2 runtime or blockstore runtime?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the v2 runtime can support other backends besides blockstore. But I use the terms somewhat interchangeably.

Comment thread openedx/core/djangoapps/xblock/utils.py Outdated
Comment thread openedx/core/djangoapps/xblock/utils.py Outdated
@ormsbee

ormsbee commented Dec 13, 2019

Copy link
Copy Markdown
Contributor

Okay, the code seems totally reasonable. But at a high level, doesn't this pose a problem for advanced XBlocks like ORA2 that also store database state and expect actual users? The anonymous courseware work outlined in #18134 created a new view so that XBlocks explicitly had to opt into running in this mode. What's the right behavior for something like ORA2 when viewed in this mode?

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

@ormsbee

But at a high level, doesn't this pose a problem for advanced XBlocks like ORA2 that also store database state and expect actual users?

Yes and no. I think ORA2 uses edx-submissions which already accepts a string for the user ID and does not have foreign keys, so this might well be compatible with it. The main concerns would be that unwanted rows accumulate in the edx-submissions database table, and/or that instructors start being prompted to grade submissions by anonymous users who aren't enrolled.

(I didn't test ORA because I haven't yet implemented the user service in the new runtime, and it depends on the user service to get data about the user. So it currently just shows an "UNABLE TO LOAD" error message if one tries to view it. But I think that if I wired up the user service, it might "just work", though that may not be desired for the reasons I just mentioned.)

The anonymous courseware work outlined in #18134 created a new view so that XBlocks explicitly had to opt into running in this mode. What's the right behavior for something like ORA2 when viewed in this mode?

At one level, this is completely compatible with that approach: this low-level change to the runtime enables support for anonymous users and is indifferent to which view is being used. Some higher-level code, such as the LMS itself or Ramshackle or LabXchange, could easily implement logic to request public_view when the user is not logged in, and student_view when the user is logged in. I don't think the runtime should do that implicitly.

Though I suppose that to prevent exceptionally knowledgeable users who aren't logged in from using the REST API to directly request student_view, we should consider adding a check to the runtime that prohibits users who aren't logged in from requesting any view other than public_view. Do you think that restriction is important?

I could also add a fallback for public_view, like the one that exists in x_module.py.

Also note that this doesn't provide any mechanism for, say, determining which parts of a course can be shown to users who aren't logged in, irrespective of what type of xblocks are used - that would also be done at a higher level. In general, whether or not to show a specific block to a user who isn't logged in should be determined based on the content and the context (e.g. course policies) rather than the block type.


Here are some various next steps I can do:

  1. Update Ramshackle to request public_view when the user isn't logged in, to make it easy to test that approach.
  2. Prohibit users who aren't logged in from requesting any view other than public_view (enforce this in the runtime)
  3. Provide a default version of public_view for XBlocks that don't provide their own (which simply says "You must log in to see this component").
    or
  4. Provide a default that public_view = student_view and require ORA2 and any other XBlocks that store state in the database to override public_view with a message (that says "You must log in to see this component"), since xblocks that don't work with this approach are definitely the exception rather than the rule.

I want to do 1+4, but I think that 1+2+3 is probably the best approach here. What do you think?

@bradenmacdonald

bradenmacdonald commented Dec 14, 2019

Copy link
Copy Markdown
Contributor Author

Follow up: it looks like capa problems and drag-and-drop-v2 currently don't have a public_view, but I think we'd like to be able to allow authors to make those public sometimes. I'm not sure if it was a technical or policy reason those don't support public_view, but if it's technical, then that restriction isn't relevant anymore as this works well with both capa and drag-and-drop.

(But that could be a problem if we want to enable public_view for the new runtime but not the old...)

If I do 1+4 above, things will work well, because only the new runtime will define that public_view defaults to student_view.

@symbolist

Copy link
Copy Markdown
Contributor

@bradenmacdonald

it looks like capa problems and drag-and-drop-v2 currently don't have a public_view, but I think we'd like to be able to allow authors to make those public sometimes. I'm not sure if it was a technical or policy reason those don't support public_view

The reason was that the client requirement was only for video and html xblocks to be available for non-logged in users. And since non-read only views would have required additional work, they were out of scope.

I want to do 1+4, but I think that 1+2+3 is probably the best approach here. What do you think?

Thinking again about this I agree with @ormsbee that we should keep the same behavior as LMS where xblocks opt into being available for non-logged in users by explicitly providing a public_view. So my vote is for 1+2+3.

@symbolist

Copy link
Copy Markdown
Contributor

Having said so for capa and drag-and-drop-v2 we will have to do a check so that they only serve the student_view when public_view is called by the new runtime because otherwise it will break the existing LMS anonymous users feature.

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

The reason was that the client requirement was only for video and html xblocks to be available for non-logged in users.

Right. But I don't think that's a good long-term product requirement to stick to. Course authors should be able to enable/disable content for public consumption based on which section of the course is in ("Only allow viewing the first two units before users enroll", or based on other criteria like "allow public viewing of practice questions (which are problem blocks) but not exam questions (which are likewise problem blocks)"), more so than content type. (In fact, I believe that's already possible using content groups and the public cohort, isn't it?)

My preference is still 1+4 - it lets us use capa problems and drag-and-drop in the new runtime and for LabXchange, while allowing some types like ORA2 to opt out, and without impacting the current anonymous access system.

Doing 2 would mean that we can't use capa problems in LabXchange for users who aren't logged in, and future course authors wouldn't be able to show prospective students what their assessments will be like (which they could otherwise do by making all content, including problems, in the first few units of the course available anonymously). Or, we can do 2, but I'd want to enable capa and drag-and-drop-v2 for public_view, which might impact existing users of the anonymous courseware feature?

@symbolist

Copy link
Copy Markdown
Contributor

Course authors should be able to enable/disable content for public consumption based on which section of the course is in ("Only allow viewing the first two units before users enroll", or based on other criteria like "allow public viewing of practice questions (which are problem blocks) but not exam questions (which are likewise problem blocks)"), more so than content type. (In fact, I believe that's already possible using content groups and the public cohort, isn't it?)

Yes, that is already the case.

However the idea of a separate public_view is that xblocks can have separate preview and full views independent of whether the user is logged in or not. For example, "when someone who is not enrolled in a course but is signed in, previews a video they are only shown the first 1 minute". XBlocks can provide such a view for previewing content and then higher level layers can decide which of the two to show in which context.

For capa and drag-and-drop we can do something like this:

def public_view()
  if <check if it is being called by the new runtime which can support storing state for anonymous users>:
    return self.student_view()

  return super().public_view()

Those are my thoughts but I'll defer to @ormsbee preference.

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

However the idea of a separate public_view is that xblocks can have separate preview and full views independent of whether the user is logged in or not. For example, "when someone who is not enrolled in a course but is signed in, previews a video they are only shown the first 1 minute".

Oh I see. Is that clearly documented throughout the code? I was definitely confused by that, thinking it was more to do with logged-in status than enrolment. Though it seems odd for XBlocks to be changing their content based on enrolment status - wouldn't it be more consistent to use content groups and put the 1 minute video into a content group that the public cohort sees, and the full video (a separate XBlock) into a content group that enrolled students see?

@ormsbee

ormsbee commented Dec 19, 2019 via email

Copy link
Copy Markdown
Contributor

@ormsbee

ormsbee commented Dec 19, 2019

Copy link
Copy Markdown
Contributor

@bradenmacdonald: You're right that it's not very clear. I think that my main concern is that the XBlock has control of the situation and has to opt into these new features, so that we're not trying to catch weird edge cases.

Also, I agree that we shouldn't use public_view to determine which content students get to see–that's definitely the purview of content groups, for which there is support. From my perspective, the public_view is more about what the XBlock can reasonably support. So I don't think we'd use it for a 1 minute preview of some piece of content, as that's a content policy decision. But I think it's reasonable for ORA2 to say, "I don't know how to handle submissions and peer grading if you're not a real user, but you can see the question." If capa knows that it can render itself as long as it has state persistence for anonymous users, then that's great. It might be that capa decides that certain problem types will not be rendered in those situations (e.g. code grading). But as long as that logic happens at the level of the individual XBlock and we're not trying to do magic under the covers, I feel much better about it.

Thanks folks. I've been poking at this during the nights since last weekend because I was worried about the downstream effects but couldn't really pin a decent path. Your comments over the last day have made me feel much more at ease with this direction.

Comment thread openedx/core/djangoapps/xblock/utils.py Outdated

@ormsbee ormsbee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor questions, suggestions, but nothing blocking. Please just squash and add context to the commit message.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary for this PR, but this class might be a good place to use the TieredCache utility.

Comment thread openedx/core/djangoapps/xblock/utils.py Outdated
Comment thread openedx/core/djangoapps/xblock/runtime/ephemeral_field_data.py Outdated
@symbolist

Copy link
Copy Markdown
Contributor

Oh I see. Is that clearly documented throughout the code? I was definitely confused by that, thinking it was more to do with logged-in status than enrolment.

Yeah, I know the naming was confusing. But the view selection currently happens on the basis of enrollment status:
https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/views/index.py#L132-L152

@bradenmacdonald
bradenmacdonald requested a review from a team December 19, 2019 22:16
@bradenmacdonald
bradenmacdonald requested a review from a team as a code owner December 19, 2019 22:16
@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

@ormsbee @symbolist I have addressed your comments (and implemented "1+2+3") via 2f18c1b89939eddbb3d20245d9e3acded851f452 and open-craft/ramshackle@bb57186. I'm pretty happy with how it's working now.

I will now rebase and squash; you can use the commit hash above to see the latest changes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a seemingly unrelated change, but I found that the public_view of Video XBlocks wasn't working for anonymous users when this VAL code was used, and we have also been sometimes seeing 500 errors on LX when the edx_video_id is present. Also, this was a bit problematic because it makes a lot of blocking API calls to VAL during XML parsing. So for those three reasons, I think that completely omitting this "update VAL" step when parsing video XML in the new runtime will work much better.

@ormsbee

ormsbee commented Dec 19, 2019

Copy link
Copy Markdown
Contributor

Please add the "implementation details" section from your PR message to your commit message. I think it's really informative.

Also, please let me know if you want to change the timeout to None or not. I'm fine with merging it either way.

Implementation details:
* Anonymous users are assigned a unique ID (like 
  `anon42c08f9996194e2a9339`) which gets stored in the django session.
  `block.scope_ids.user_id` and `block.runtime.anonymous_student_id`
  will both return this value.
* User state for anonymous users is stored in the django cache and
  automatically expires as the cache gets pruned. Because user state is
  stored, anonymous users can use interactive blocks like capa problems.
* There is no mechanism for upgrading to a registered account and
  keeping user state since the user state store for anonymous users
  (EphemeralKeyValueStore) is completely different than the one for
  registered users (DjangoKeyValueStore/"CSM"), and has no "list all
  keys" functionality.
* "User State Summary" field values are shared among [recently active]
  anonymous users but are not shared with registered users.
* Anonymous users can only access the `public_view` of XBlocks, not the
  regular `student_view`.
@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

Please add the "implementation details" section from your PR message to your commit message. I think it's really informative.

Sure thing, done!

Also, please let me know if you want to change the timeout to None or not. I'm fine with merging it either way.

I've changed it to None and manually verified that the state is persisting longer than 5 minutes, just to make sure. That should be simpler, no need to specify a timeout if we don't need to.

Should be good to go once the build passes!

@edx-status-bot

Copy link
Copy Markdown

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

@ormsbee
ormsbee merged commit 62eeabd into openedx:master Dec 20, 2019
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@ormsbee

ormsbee commented Jan 8, 2020

Copy link
Copy Markdown
Contributor

@bradenmacdonald, @symbolist: Just as a heads up, this broke some downstream analytics events analysis code because that code expected numerical user IDs. I should be talking with data eng about next steps tomorrow.

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

@ormsbee Ok, let me know if I can help. For now it should be safe to just discard entries that aren't numerical.

@bradenmacdonald
bradenmacdonald deleted the anonymous-user-state branch January 8, 2020 23:15
@openedx-webhooks

Copy link
Copy Markdown

Although this pull request is already merged, I've created OSPR-5398 so that we can track it in Jira.

There is nothing you have to do. No action is needed from your side. Thanks again for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants