Skip to content

Conversation

@Agrendalath
Copy link
Member

@Agrendalath Agrendalath commented May 2, 2023

Description

After changes from #31472, the user service of a "leaf" XBlock. gets overridden with the one created for its parent (SequenceBlock). Therefore, the requires_per_student_anonymous_id is ignored in these XBlocks. This removes choosing the proper ID (course-specific or student-specific) from the runtime initialization. Instead, both IDs are passed to the user service. The subsequent renders of an XBlock (e.g., when requesting the solution) use the student-specific IDs.

There are only two XBlocks that relied on the requires_per_student_anonymous_id - ProblemBlock and HtmlBlock. They now request the "deprecated" (student-specific) user ID directly from the user service.

Supporting information

Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.

Testing instructions

  1. Create a Problem XBlock with the following content:
<problem>
	<p>anonymous_student_id: $anonymous_student_id</p>
	<solution>
		<p>anonymous_student_id: $anonymous_student_id</p>
	</solution>
</problem>
  1. Go to Preview and press the "Show answer" button. The new ID should be the same as the previous one.

Deadline

As soon as possible.

@Agrendalath Agrendalath requested a review from ormsbee May 2, 2023 13:56
@openedx-webhooks openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label May 2, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @Agrendalath!

When this pull request is ready, tag your edX technical lead.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's probably worth noting that anonymous_ids basically have to be hex strings at this point, because that's what they've always been, and there's content out there that depends on this for randomization behavior. 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right in thinking that individualize_anonymous_user_id() is leftover rollout code and that just makes preview behavior incorrect now and doesn't need to be here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess dealing with that is not worth adding to this PR in terms of risk either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to have been added on purpose: #30248.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, okay, good to not touch... 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

Misc. note, but now I think I better understand why the anonymous ID parsing in capa content that I've seen has something like this:

import random
try:
	randomseed = int(anonymous_student_id, 16)
except:
	randomseed = 1
random.seed(randomseed)

... so it doesn't blow up the problem in preview where it can't parse because the strings are not hex strings. There's no need to change this behavior–I just put this note here because it was something that was puzzling me.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, the motivation for that is that they want to have a handful of random variables that are generated the same way for a given student across multiple problems so that they can tie them together. Like if problem 1 asks you to calculate your monthly mortgage payments based on some variables, and then problem 2 asks you what those payments would be if you paid made a $100K payment after the first year, etc.

Copy link
Member Author

@Agrendalath Agrendalath May 2, 2023

Choose a reason for hiding this comment

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

Unless I'm missing something, the hash function works the same way as int(hex_string, 16) for hex values. It works for non-hex strings, too (without raising an exception).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would actually be a very useful feature if we could assign variables to some namespace that lives across problems and let XBlocks access that–people have basically hacked that together in a few different ways over the years. Maybe as an XBlock service some day, though we'd have to think through the implications pretty critically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm missing something, the hash function works the same way as int(hex_string, 16) for hex values. It works for non-hex strings, too.

Python's builtin hash function is only guaranteed to return consistently within a given run of a Python process, so it would be a bad fit for deriving anything stable.

edx-platform on  ormsbee-anonymous-id-xb-service via 🐍 v3.8.12 (tutor_nightly) is 📦 v0.1.0 took 12s
❯ python
Python 3.8.12 (default, Dec 10 2021, 16:41:45)
[Clang 13.0.0 (clang-1300.0.29.3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> hash("Hello World!")
6517261053207716808
>>> hash("Hello World!")
6517261053207716808
>>>

edx-platform on  ormsbee-anonymous-id-xb-service via 🐍 v3.8.12 (tutor_nightly) is 📦 v0.1.0 took 2m18s
❯ python
Python 3.8.12 (default, Dec 10 2021, 16:41:45)
[Clang 13.0.0 (clang-1300.0.29.3)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> hash("Hello World!")
740855705488943589
>>> hash("Hello World!")
740855705488943589
>>>

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good point. I forgot it's consistent only for numeric types.

Copy link
Member Author

Choose a reason for hiding this comment

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

The VideoBlock does not use the anonymous student ID anywhere. The only usage of the user service is for requesting the ATTR_KEY_REQUEST_COUNTRY_CODE.

Copy link
Contributor

@ormsbee ormsbee left a comment

Choose a reason for hiding this comment

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

Just squash, and I think we're good to go.

@Agrendalath Agrendalath force-pushed the agrendalath/bd-13-fix_anonymous_student_ids branch from 899ed17 to aef43ad Compare May 2, 2023 15:57
After changes from openedx#31472, the user service of a "leaf" XBlock gets overridden
with the one created for its parent (SequenceBlock). Therefore, the
`requires_per_student_anonymous_id` is ignored in these XBlocks. The
subsequent renders of an XBlock (e.g., when requesting the solution) use
the student-specific IDs.

This removes choosing the proper ID (course-specific or student-specific) from
the runtime initialization. Instead, both IDs are passed to the user service.

There are only two XBlocks that relied on the
`requires_per_student_anonymous_id` - `ProblemBlock` and `HtmlBlock`. They
now request the "deprecated" (student-specific) user ID directly from the user
service.
@Agrendalath Agrendalath force-pushed the agrendalath/bd-13-fix_anonymous_student_ids branch from aef43ad to aa85257 Compare May 2, 2023 15:58
@jristau1984 jristau1984 merged commit bdd2a0d into openedx:master May 2, 2023
@openedx-webhooks
Copy link

@Agrendalath 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@Agrendalath Agrendalath deleted the agrendalath/bd-13-fix_anonymous_student_ids branch May 2, 2023 16:33
@edx-pipeline-bot
Copy link
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
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

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

@ormsbee
Copy link
Contributor

ormsbee commented May 3, 2023

More context about the observed issue this PR is fixing, in case anyone comes back to this PR later:

Say we’re rendering a ProblemBlock from the /xblock/{usage_key} URL. This was happening:

  1. render_xblock is called for the student view.
  2. render_xblock then gets the ProblemBlock using the UsageKey that was passed into the view. This initializes the runtime services for this XBlock, including the UserService. 3. This does the right thing and sets up the ProblemBlock to use a per-student anonymous ID.
  3. render_xblock then looks at the enclosing sequence that this ProblemBlock is located in. It does this to determine access checks. But this causes the runtime to be re-initialized for the SequenceBlock, which uses the per-student+course anonymous IDs.
  4. Finally, block.render is called on the original ProblemBlock. This invokes the ProblemBlock’s student_view, which initializes its internal LonCapaProblem. But when that code reaches for runtime services to get user info, it’s seeing the UserService created for SequenceBlock.

The fix here was to create an explicit new attribute for the old-style, course-agnostic anonymous student IDs in UserService instead of trying to intelligently make different UserService instances based on the requesting XBlock type. So the code is still recreating the UserService for that SequenceBlock before ProblemBlock executes its view, but now it just regenerates the exact same UserService, and ProblemBlock knows to ask for the newly created attribute that has the course-agnostic anonymous student ID in it.

This sort of thing is not a problem for handler invocation, since those XBlock handlers do not currently invoke the gating-style checks that render_xblock (i.e. the /xblock/{usage_key} endpoint) does. But this is more fragile than it should be.

I think that as a follow-up to this, I'd like to discuss the feasibility of removing block-specific handling from prepare_runtime_for_user, so that it becomes something that only needs to be initialized once for a student during a request, instead of run for each XBlock being invoked.

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

Labels

blended PR is managed through 2U's blended developmnt program

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants