refactor: deprecate course_id from ModuleSystem [BD-13] #30715

Agrendalath · 2022-07-07T20:08:49Z

Description

As a part of the ModuleSystem removal, we want to get rid of the course_id from its attributes. We have replaced its usages in the platform with block.scope_ids.usage_id.context_key. For backward compatibility, external XBlocks will still be able to implicitly obtain it either from the CachingDescriptorSystem or XModuleMixin as a fallback.

Supporting information

Sandbox instance: https://pr30715.sandbox.opencraft.hosting
Use staff@example.com:edx to log in.

Testing instructions

Go to the Instructor Dashboard and check that generating a report of problem responses works correctly for XBLocks with custom Python scripts.
Check that the edX notes functionality works.
1. First, go to this page and add the certificate extension to your browser. Setting up edX notes for sandboxes needs some more steps than we have documented, but I didn't look into this. I've added verify=False to requests in lms/djangoapps/edxnotes/helpers.py to quickly get this working.
2. Go to a page with an HTML XBlock. Create, edit, and delete some notes.
3. View all your notes here.
Go to this page and check that the export functionality of the Poll XBlock is working.
Go through all existing XBlocks and verify that they are working correctly.
Check that Instructor tasks work correctly for special exams:
1. View the course as a learner and start the timed exam.
2. Refresh the page (something is wrong with the Learning MFE, as it doesn't immediately show the exam bar) and submit the exam.
3. Switch back to the staff user and reset the attempt through the Instructor Dashboard.

Other information

Private-ref: BB-5969

openedx-webhooks · 2022-07-07T20:08:52Z

Thanks for the pull request, @Agrendalath!

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

natabene · 2022-07-27T15:12:08Z

@Agrendalath Just checking if you are planning to pursue this any time soon.

Agrendalath · 2022-08-08T11:06:45Z

@natabene, sorry, I just came back from vacation. Yes, I have this, #30072, and #30046 on my list for the upcoming two weeks.

natabene · 2022-08-08T13:25:33Z

@Agrendalath I hope you had a great vacation, thanks for letting me know about the plans for the PRs.

Agrendalath · 2022-09-05T18:01:20Z

@ormsbee, would you like to review this PR?

xmodule/discussion_block.py

xmodule/tests/test_video.py

lms/djangoapps/courseware/module_render.py

ormsbee

Two main line of questions:

Is it possible that this refactoring is introducing versioned CourseKeys in places they previously were not? I'm concerned that this might cause regressions for things like serialization or configuration lookup (e.g. the thing is enabled for the string that is the CourseKey, but not the string that is the CourseKey+branch/version information).
Why are the service additions part of the same PR? Is it because course_id swichover requires it for some reason?

ormsbee

Just a few comments and questions.

xmodule/discussion_block.py

ormsbee · 2022-09-13T02:19:52Z

xmodule/x_module.py

@jristau1984: Sometime after this PR merges, you might want to look for this line in the Splunk logs. In particular, this could happen if edx.org uses extensions or XBlocks that try to access this deprecated attribute directly instead of deriving it directly from the UsageKey.

xmodule/capa_module.py

xmodule/tests/test_video.py

pkulkark

I haven't done a deep code review since @ormsbee is already doing that. But I tested it out on the sandbox and it works as expected. LGTM 👍

Agrendalath · 2022-09-16T09:48:07Z

@ormsbee, I've resolved the existing conversations (except for this one). Please let me know if you'd like to have another look before we move forward here.

ormsbee

Looks good to me.

I haven't done a deep code review since @ormsbee is already doing that. But I tested it out on the sandbox and it works as expected. LGTM 👍

@pkulkark: I actually meant to review only things that you had already reviewed, and did this one by accident without realizing. But as a general rule, please feel free to closely review everything, regardless of whether I do a pass at it. The Modulestore is tricky enough where I'm always grateful for an extra set of eyes on these things.

xmodule/tests/test_poll.py

We will use `context_key` instead.

…errides We will use `context_key` instead.

We will use `context_key` instead.

This attribute is already deprecated for XBlocks in favour of directly retrieving it like `block.scope_ids.usage_id.context_key`. This commit also removes some redundant logging code which was omitted in the Datadog removal in openedx#19420.

openedx-webhooks · 2022-09-26T12:23:14Z

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

edx-pipeline-bot · 2022-09-26T16:17:27Z

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

edx-pipeline-bot · 2022-09-27T13:21:52Z

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

regisb · 2022-09-30T13:39:14Z

FYI I believe that this PR causes the following issue when building the openedx Docker image on Tutor: overhangio/tutor#726

EDIT 2022-10-03: nope! I was wrong

openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Jul 7, 2022

Agrendalath force-pushed the agrendalath/bd-13-deprecate_course_id branch 2 times, most recently from 81ff675 to 12b8d61 Compare July 13, 2022 15:07

Agrendalath mentioned this pull request Aug 8, 2022

refactor: replace deprecated rebind_noauth_module_to_user, get_real_user, runtime.hostname, runtime.course_id [BD-13] openedx/xblock-lti-consumer#249

Merged

Agrendalath mentioned this pull request Aug 8, 2022

[BD-13] [BB-5967] refactor: deprecate get_module and descriptor_runtime attributes from ModuleSystem #30436

Closed

Agrendalath force-pushed the agrendalath/bd-13-deprecate_course_id branch from 12b8d61 to fc44ba9 Compare August 17, 2022 17:06

Agrendalath force-pushed the agrendalath/bd-13-deprecate_course_id branch 2 times, most recently from e319158 to b295d5a Compare September 5, 2022 15:56

Agrendalath changed the title ~~refactor: deprecate course_id from ModuleSystem [BD-13][BB-5969]~~ refactor: deprecate course_id from ModuleSystem [BD-13] Sep 5, 2022

Agrendalath marked this pull request as ready for review September 5, 2022 17:55

Agrendalath force-pushed the agrendalath/bd-13-deprecate_course_id branch from a6025af to 68091e8 Compare September 5, 2022 17:58

Agrendalath requested a review from pkulkark September 5, 2022 18:00

ormsbee reviewed Sep 8, 2022

View reviewed changes

xmodule/discussion_block.py Outdated Show resolved Hide resolved

ormsbee reviewed Sep 8, 2022

View reviewed changes

xmodule/tests/test_video.py Outdated Show resolved Hide resolved

ormsbee reviewed Sep 8, 2022

View reviewed changes

lms/djangoapps/courseware/module_render.py Outdated Show resolved Hide resolved

ormsbee requested changes Sep 8, 2022

View reviewed changes

ormsbee reviewed Sep 13, 2022

View reviewed changes

ormsbee self-assigned this Sep 13, 2022

pkulkark approved these changes Sep 16, 2022

View reviewed changes

ormsbee approved these changes Sep 16, 2022

View reviewed changes

xmodule/tests/test_poll.py Outdated Show resolved Hide resolved

Agrendalath added 4 commits September 21, 2022 18:52

refactor: use SandboxService while generating CAPA responses report

086e651

refactor: remove deprecated runtime.course_id from VideoBlock

98bb30d

We will use `context_key` instead.

refactor: remove deprecated runtime.course_id from student field ov…

8c89c75

…errides We will use `context_key` instead.

refactor: remove deprecated runtime.course_id from Timed Exams

99042f6

We will use `context_key` instead.

Agrendalath added 2 commits September 21, 2022 18:52

refactor: remove deprecated runtime.course_id from edxnotes decorator

c2ac3d8

We will use `context_key` instead.

Agrendalath force-pushed the agrendalath/bd-13-deprecate_course_id branch from d1525b7 to dcabc5a Compare September 21, 2022 17:01

fix: move service initialization from LMS runtime init to module render

1afb32c

Agrendalath force-pushed the agrendalath/bd-13-deprecate_course_id branch from dcabc5a to 1afb32c Compare September 21, 2022 17:05

Agrendalath merged commit fcb594d into openedx:master Sep 26, 2022

Agrendalath deleted the agrendalath/bd-13-deprecate_course_id branch September 26, 2022 12:23

ormsbee mentioned this pull request Sep 30, 2022

Revert "refactor: deprecate course_id from ModuleSystem [BD-13]" #31071

Closed

Agrendalath mentioned this pull request Nov 14, 2022

[BB-6875] Add option to drop decoy item from scores openedx/xblock-drag-and-drop-v2#303

Merged

refactor: deprecate course_id from ModuleSystem [BD-13] #30715

refactor: deprecate course_id from ModuleSystem [BD-13] #30715

Uh oh!

Conversation

Agrendalath commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Other information

Uh oh!

openedx-webhooks commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natabene commented Jul 27, 2022

Uh oh!

Agrendalath commented Aug 8, 2022

Uh oh!

natabene commented Aug 8, 2022

Uh oh!

Agrendalath commented Sep 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ormsbee Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pkulkark left a comment

Choose a reason for hiding this comment

Uh oh!

Agrendalath commented Sep 16, 2022

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openedx-webhooks commented Sep 26, 2022

Uh oh!

edx-pipeline-bot commented Sep 26, 2022

Uh oh!

edx-pipeline-bot commented Sep 27, 2022

Uh oh!

regisb commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Agrendalath commented Jul 7, 2022 •

edited

Loading

openedx-webhooks commented Jul 7, 2022 •

edited

Loading

regisb commented Sep 30, 2022 •

edited

Loading