Skip to content

[MNG-1904] Fix: update new runtime's runtime.anonymous_student_id to store in DB - #26135

Merged
ormsbee merged 1 commit into
openedx:masterfrom
open-craft:libraries-v2-anon-ids-shim-fix
Jan 22, 2021
Merged

[MNG-1904] Fix: update new runtime's runtime.anonymous_student_id to store in DB#26135
ormsbee merged 1 commit into
openedx:masterfrom
open-craft:libraries-v2-anon-ids-shim-fix

Conversation

@bradenmacdonald

@bradenmacdonald bradenmacdonald commented Jan 21, 2021

Copy link
Copy Markdown
Contributor

This addresses a longstanding TODO item to make runtime.anonymous_student_id for content libraries v2 work the same way as it does for XBlocks in regular courses, persisting the anonymous ID string to the database. This way, if SECRET_KEY is changed, existing anonymous IDs will continue to work unchanged.

This is a potentially breaking change for XBlocks in these blockstore-based libraries (v2 libraries), because the actual anonymous_student_id values generated are now different (old ones were based on only user ID and secret key but didn't also incorporate the "context key" into the hash as the new IDs do). But based on my searching for anonymous_student_id in the codebase, this should only affect capa problems using external code graders or Matlab code input, and I'm not aware of any such usage of the new runtime / libraries v2.

Risks

There is a low risk that this change to anonymous_student_id causes issues for some LabXchange XBlocks (LabXchange being essentially the only user of the new runtime so far). Based on my code searching and manual testing, I don't believe anonymous_student_id is actually used meaningfully (they don't use external graders nor matlab input), so I don't expect any problems. However, if we wanted to be more conservative we could use a transition period where the existing IDs are stored into the table, before we change to the newer ID format

Database migration

It has one, but it doesn't make any changes (no SQL); it's just a change to the django model.

Testing

How I tested this change:

  1. Used Ramshackle to create a v2 content library with a multiple choice capa problem. Published it, then using the "Learn" tab, submitted an answer.
  2. Then I made these changes, and refreshed the page. I verified using debug logging that the anonymous_student_id for that problem+user did change, but the previously submitted answer was still selected.
  3. I ran the integration tests for libraries v2 as described at https://github.com/edx/blockstore/#running-integration-tests - they mostly passed and there were no issues related to this PR; however, a few failed in the CMS test suite because [BD-19] Transition to the Elasticsearch 7.8.0 version edx-search#104 seems to have introduced a breaking change to edx-search and nobody has yet updated the libraries code accordingly. I've asked my team to look into that.

This addresses a longstanding TODO item to make runtime.anonymous_student_id for content libraries v2 work the same way as it does for XBlocks in regular courses, persisting the "context ID" (equivalent to course ID) to the database. This way, if SECRET KEY is changed, existing anonymous IDs will continue to work unchanged.

This is a potentially breaking change, but should mostly affect capa problems using external code graders or Matlab code input, and I'm not aware of any such usage of the new runtime / libraries v2.
@openedx-webhooks openedx-webhooks added core committer open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 21, 2021
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @bradenmacdonald! I've created OSPR-5430 to keep track of it in JIRA.

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

@feanil Here's a PR for the issue we discussed on Slack.

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

@pomegranited @symbolist Do you have any concerns about this change to runtime.anonymous_student_id for XBlocks on LabXchange?

@edx-status-bot

Copy link
Copy Markdown

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

@pomegranited

Copy link
Copy Markdown
Contributor

@pomegranited @symbolist Do you have any concerns about this change to runtime.anonymous_student_id for XBlocks on LabXchange?

No concerns from me -- I agree with your assessment that LabXchange content doesn't use the anonymous_student_id. There's some content types in planning stages for LX which might need to use this identifier though, so it's great that you've fixed this now.

this should only affect capa problems using external code graders or Matlab code input

edx-ora2 submissions also use anonymous_student_id, but LX doesn't use them ORA2 either.

@feanil
feanil requested review from feanil and ormsbee January 22, 2021 14:14

@feanil feanil 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.

Looks good to me but I'm not as familiar with this part of the system so a review from @symbolist or @ormsbee would be great before I merge this.

@ormsbee
ormsbee merged commit a2fc100 into openedx:master Jan 22, 2021
@openedx-webhooks

Copy link
Copy Markdown

@bradenmacdonald 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@openedx-webhooks

Copy link
Copy Markdown

@ormsbee, @kdmccormick: thought you might like to know that bradenmacdonald merged this pull request.

@openedx-webhooks openedx-webhooks added merged and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 22, 2021
@bradenmacdonald
bradenmacdonald deleted the libraries-v2-anon-ids-shim-fix branch January 22, 2021 18:13
@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.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants