fix: make the studio user_id the actual anonymous_user_id by schenedx · Pull Request #30248 · openedx/openedx-platform

schenedx · 2022-04-14T15:26:13Z

Description

MST-1455
Currently, during studio preview of the xblock, we would render the xblock using the anonymous_user_id with value student. Why not just render the xblock using the actual anonymous_user_id?
One uncertainty is that the anonymous_user_id for non edX staff user would be none. That might be a problem?

Testing instructions

Create a course override waffle flag in the django admin on edx-platform. The name of the waffle flag is contentstore.individualize_anonymous_user_id
Enable and activate the above waffle flag for a course in test
Go into the studio website and go into the course where you had the waffle flag turned on
Create a new LTI xblock
Configure the xblock connecting against a TurnItIn content instance. You can find examples of it on stage or prod
Preview the xblock instance upon save.
During preview rendering, monitor the network tab of the Chrome developer tool and see the data posted into the LTI handler endpoint has user_id to be a unique value, instead of student value.

schenedx · 2022-04-14T17:10:36Z

@pomegranited Can you think of any negative effects of this change?

pomegranited · 2022-04-18T12:00:43Z

@schenedx These are the effects I can think of.. But note that I don't think they're a reason not to continue with this change, they may just require some communication.

In Studio:

Any previously-submitted answers to JS problems that rely on anonymous_student_id for randomization will be marked incorrect, as the anonymous_student_id will have changed.
Similarly, 2 different course authors which use Studio to test JS problems using anonymous_student_id will see different values and require different answers to randomized questions. (This can also be seen as a feature, as they can also test said randomization.)
Any previous integrations with LTIs or XBlocks that rely on the Studio student value in anonymous_student_id will lose access to their Studio user data in the LTI/XBlock.

Are any of these concerns a worry for you @pdpinch ? For context, see the Slack discussion.

pomegranited · 2022-04-18T12:04:51Z

            "settings": SettingsService(),
            "user": DjangoXBlockUserService(
                request.user,
-                anonymous_user_id='student',


@schenedx This is not enough to fix the issue.. you'll need to actually compute the anonymous_user_id for the current user, and pass it into the UserService as is done by the LMS ModuleSystem.

Alternatively, you can move the logic for computing this value into a new function on the UserService (it will need access to the module descriptor and the course_id) to avoid duplicating code.

Nudge @schenedx :) Do you have an estimate for when you'll be able to address my comments?
Just planning my week, and want to make sure I allow time for a 2nd review. Thanks!

@pomegranited This fell off my radar last week. I apologize. I thought the DjangoXBlockUserService instance already have the get_anynomous_user_id that returns the calculated value, no? Also, this is on CMS. The LMS side has the correct value returned.

No worries @schenedx :)

Also, this is on CMS. The LMS side has the correct value returned.

Yes. What I'm saying above is the CMS needs to return the same value as the LMS, and this requires some code changes.

I thought the DjangoXBlockUserService instance already have the get_anynomous_user_id that returns the calculated value, no? ([sic: corrected source link])

Yes it does have that method, but there's some issues with using it directly:

performance: that method hits the database to get the User object for the given username, so we'd likely want to cache it

There's some subtleties of the LMS code -- some blocks expect a per-student anonymous ID, while some expect a per-course anonymous ID. The DjangoXBlockUserService currently has no knowledge of course_id, or of the block's requires_per_student_anonymous_id flag.

All of these issues can be solved, but they require code changes to implement them.

I have no developer experience in this area, so this is difficult for me to setup.

Ah ok.. I misunderstood. Do you have time and interest in making this change? If not, then we should find someone who does. OpenCraft could do it (for a fee), if there's no one in the community who wants to do it?

How do we test the anynomous_user_id with randomized content? Do I just need to set it up on my local environment?

Yep, if you want to implement this change, you'll need a devstack. You can test it manually, but we'd also want some automated tests to protect the change, and ensure that future code changes don't break it.

Also, I think all the side effect of this change @pomegranited have listed is worth a heads up for 2U internal.

Yep, that's worth doing, and can be done as part of the OSPR too. It's always easier to see the effects of a change when the code is in place so it can be tested.

schenedx · 2022-04-26T13:12:01Z

@pomegranited I am grateful for your feedback. How do we test the anynomous_user_id with randomized content? Do I just need to set it up on my local environment? I have no developer experience in this area, so this is difficult for me to setup.

schenedx · 2022-04-26T13:23:34Z

Also, I think all the side effect of this change @pomegranited have listed is worth a heads up for 2U internal. The true question is, should we manage the communication of these side effects? Or the current behavior/no fix, would be the better behavior going forward. I'll need sometime to have this conversation with 2U stakeholders.

schenedx · 2022-05-04T19:24:10Z

@pomegranited OK, with your explanation and a bit of my devstack testing, I now understand you better.
To limit the potential ramification of this change, I am putting the change behind a course waffle flag to test out on stage and prod.
If all goes well, I can slowly release it.
Please review again.

pomegranited · 2022-05-11T04:03:15Z


+    preview_anonymous_user_id = 'student'
+    if individualize_anonymous_user_id(course_id):
+        preview_anonymous_user_id = anonymous_id_for_user(request.user, course_id)


@schenedx In order to ensure the same anonymous ID is used in Studio as in the LMS, you need to check the block descriptor to find out whether the anonymous user ID should be course-specific or not.

Suggested change

preview_anonymous_user_id = anonymous_id_for_user(request.user, course_id)

if getattr(descriptor, 'requires_per_student_anonymous_id', False):

preview_anonymous_user_id = anonymous_id_for_user(request.user, None)

else:

preview_anonymous_user_id = anonymous_id_for_user(request.user, course_id)

I see. Thank you for pointing this out! This just points out my lack of knowledge in this area.
Made the change and please review again @pomegranited

Thanks @schenedx -- I can review the code, but need a functional LTI provider in order to test it. And since Open edX no longer supports this with the new Learner MFE, I need your help with this step.

Can you provide a secret:key and URL that I can use to test this from my devstack?

@pomegranited You can use https://saltire.lti.app/tool (check this URL for config parameters, but the values below should work).

Launch URL: https://saltire.lti.app/tool

ID: test

LTI passport: test:jisc.ac.uk:secret

You're the best @giovannicimolin !

pomegranited · 2022-05-11T04:04:44Z

@schenedx

To limit the potential ramification of this change, I am putting the change behind a course waffle flag to test out on stage and prod.

Great idea!

Could you flesh out your PR description to provide full Testing Instructions for how to verify this change? It'll be useful for anyone looking to figure out the history of this request.

…n xblocks

pomegranited

Provisional 👍 : Works perfectly! Just needs tests added, see schen/MST-1455...open-craft:jill/schen/MST-1455

I tested this on my devstack using the LTI Test tool described here.
I read through the code
~~I checked for accessibility issues~~ N/A
Includes documentation for the new course waffle flag added.

@schenedx FYI, I don't have merge rights on edx-platform, so you'll still need a core committer or someone at 2U/edX to approve this.

pomegranited · 2022-05-16T06:44:00Z

+            preview_anonymous_user_id = anonymous_id_for_user(request.user, None)
+        else:
+            preview_anonymous_user_id = anonymous_id_for_user(request.user, course_id)
+


This is working great @schenedx , exactly as expected.

But to make sure your change doesn't get broken somewhere down the line, we need to update the unit tests to test these different cases. Here's what I'd recommend: schen/MST-1455...open-craft:jill/schen/MST-1455

Great. I made a PR on the unit tests you wrote above and merged it. Once this PR is going green, I plan to merge as well.

and the INDIVIDUALIZE_ANONYMOUS_USER_ID flag. Co-authored-by: Jillian Vogel <jill@opencraft.com>

schenedx · 2022-05-16T14:38:15Z

Merged! Once it deploys to prod. I'll start adding in CourseWaffleFlags and see what happens

edx-pipeline-bot · 2022-05-16T16:40:48Z

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

edx-pipeline-bot · 2022-05-16T17:52:51Z

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

edx-pipeline-bot · 2022-05-16T17:52:51Z

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

connorhaugh · 2022-05-16T21:44:33Z

Creating A Revert PR as this has potentially caused an outage in the content store.

…0248)" This reverts commit 4f339e2.

edx-pipeline-bot · 2022-05-17T12:44:10Z

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

* feat: test out individualized anonymouse_user_id for studio preview on xblocks * test: adds tests for the PreviewModuleSystem anonymous_user_id (#30400) and the INDIVIDUALIZE_ANONYMOUS_USER_ID flag. Co-authored-by: Jillian Vogel <jill@opencraft.com> Co-authored-by: Simon Chen <schen@edX-C02FW0GUML85.local> Co-authored-by: Jillian Vogel <jill@opencraft.com>

schenedx requested review from giovannicimolin and ormsbee April 14, 2022 17:10

pomegranited reviewed Apr 18, 2022

View reviewed changes

schenedx force-pushed the schen/MST-1455 branch 3 times, most recently from bfeed04 to 4e9a35e Compare May 4, 2022 19:22

schenedx force-pushed the schen/MST-1455 branch 2 times, most recently from 13d73e1 to 79b3b02 Compare May 4, 2022 22:48

schenedx mentioned this pull request May 5, 2022

[BD-13] [BB-6144] deprecate hostname attribute from xmodule ModuleSystem #30308

Merged

schenedx force-pushed the schen/MST-1455 branch 2 times, most recently from 26e69f0 to 6ede845 Compare May 6, 2022 18:13

Agrendalath mentioned this pull request May 9, 2022

[BD-13][BB-6145] Move the rebind_noauth_module_to_user from ModuleSystem argument to service #30320

Merged

pomegranited reviewed May 11, 2022

View reviewed changes

schenedx force-pushed the schen/MST-1455 branch from 6ede845 to f206c3d Compare May 11, 2022 15:39

feat: test out individualized anonymouse_user_id for studio preview o…

fa10857

…n xblocks

schenedx force-pushed the schen/MST-1455 branch from f206c3d to fa10857 Compare May 11, 2022 18:21

pomegranited requested changes May 16, 2022

View reviewed changes

test: adds tests for the PreviewModuleSystem anonymous_user_id (#30400)

f5897ed

and the INDIVIDUALIZE_ANONYMOUS_USER_ID flag. Co-authored-by: Jillian Vogel <jill@opencraft.com>

schenedx merged commit 4f339e2 into master May 16, 2022

schenedx deleted the schen/MST-1455 branch May 16, 2022 14:37

connorhaugh added a commit that referenced this pull request May 16, 2022

Revert "fix: make the studio user_id the actual anonymous_user_id (#3…

adb99a1

…0248)" This reverts commit 4f339e2.

connorhaugh mentioned this pull request May 16, 2022

Revert "fix: make the studio user_id the actual anonymous_user_id" #30412

Closed

Agrendalath mentioned this pull request May 2, 2023

fix: anonymous student IDs [BD-13] #32165

Merged

Uh oh!

Conversation

schenedx commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Uh oh!

schenedx commented Apr 14, 2022

Uh oh!

pomegranited commented Apr 18, 2022 • 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!

schenedx commented Apr 26, 2022

Uh oh!

schenedx commented Apr 26, 2022

Uh oh!

schenedx commented May 4, 2022 • 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!

pomegranited commented May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pomegranited left a comment

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!

schenedx commented May 16, 2022

Uh oh!

edx-pipeline-bot commented May 16, 2022

Uh oh!

edx-pipeline-bot commented May 16, 2022

Uh oh!

edx-pipeline-bot commented May 16, 2022

Uh oh!

connorhaugh commented May 16, 2022

Uh oh!

edx-pipeline-bot commented May 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

schenedx commented Apr 14, 2022 •

edited

Loading

pomegranited commented Apr 18, 2022 •

edited

Loading

schenedx commented May 4, 2022 •

edited

Loading

pomegranited commented May 11, 2022 •

edited

Loading