Skip to content

refactor: deprecate static_url argument from ModuleSystem [BD-13] #30992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Agrendalath merged 1 commit into from
Sep 21, 2022
Merged

refactor: deprecate static_url argument from ModuleSystem [BD-13] #30992

Agrendalath merged 1 commit into from
Sep 21, 2022

Conversation

@Agrendalath
Copy link
Member

@Agrendalath Agrendalath commented Sep 14, 2022 •
edited
Loading

Description

As a part of the ModuleSystem removal, we want to get rid of its static_url attribute.

Supporting information

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

Testing instructions

  1. This attribute was used only by the CAPA XBlock, so we can test this by checking that the chemical equation problem loads the chemical_equation_preview.js.

Other information

Private-ref: BB-5518

@openedx-webhooks
Copy link

openedx-webhooks commented Sep 14, 2022

Thanks for the pull request, @Agrendalath!

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

@openedx-webhooks openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Sep 14, 2022
@Agrendalath Agrendalath mentioned this pull request Sep 14, 2022
Merged
@Agrendalath Agrendalath force-pushed the agrendalath/bd-13-deprecate_static_url_in_modulesystem branch from a866c49 to f8bd36a Compare September 14, 2022 18:09
@Agrendalath
Copy link
Member Author

Agrendalath commented Sep 14, 2022

@ormsbee, I extracted this change from #30046.

ormsbee
ormsbee requested changes Sep 15, 2022
View reviewed changes
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.

Content looks good, but please change the commit message:

  1. Since it's removing something, the prefix should be feat!:, not feat:
  2. Please use "remove" instead of "deprecate"–deprecate implies that it's still there but people shouldn't use it because we plan to remove it. This PR actually removes it.
  3. Please add something in the commit message explaining what people should use instead.

@jristau1984: I think this would be a good one for merging on Monday. The highest risk would be that static course assets (e.g. images) fail to display properly in XBlocks.

@Agrendalath Agrendalath changed the title feat: deprecate static_url argument from ModuleSystem [BD-13] feat: remove static_url argument from ModuleSystem [BD-13] Sep 16, 2022
@Agrendalath Agrendalath force-pushed the agrendalath/bd-13-deprecate_static_url_in_modulesystem branch from f8bd36a to 8dc1099 Compare September 16, 2022 09:13
@Agrendalath
Copy link
Member Author

Agrendalath commented Sep 16, 2022

@ormsbee, thanks for the quick review!

I addressed your points. Just one note:

Please use "remove" instead of "deprecate"–deprecate implies that it's still there but people shouldn't use it because we plan to remove it. This PR actually removes it.

That's true for the XBlock runtime. Initially, I added a shim to the ModuleSystemShim, so it was a bit ambiguous. I added a fixup commit (8dc1099047f422f8d7a2f7ace512ef76ac3a68e4) to revert adding it, so we can have a clean removal scenario here (and a unified experience for both runtimes). Please let me know if you think we should keep it, though.

@ormsbee
Copy link
Contributor

ormsbee commented Sep 16, 2022 •
edited
Loading

@Agrendalath: Ack! I totally missed that somehow (for some reason I thought it was being shifted into capa). No, you folks were right, please keep it in, so 2U can monitor to see if anything is still using it before it's finally and completely killed. @jristau1984: The relevant log message to search for in Splunk would be runtime.STATIC_URL is deprecated. Please use settings.STATIC_URL instead.

@Agrendalath Agrendalath force-pushed the agrendalath/bd-13-deprecate_static_url_in_modulesystem branch from 8dc1099 to 8f8763a Compare September 20, 2022 09:40
@Agrendalath
Copy link
Member Author

Agrendalath commented Sep 20, 2022

@ormsbee, I dropped the commit that was removing the XModule shim. For full backward compatibility, I also added back the shim for the XBlock runtime (8f8763a334b6c6fb8561729fd1dec4bdb35d0682), modifying it to show the DeprecationWarning.

ormsbee
ormsbee approved these changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@ormsbee ormsbee left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your patience. @jristau1984: Please look for the log messages after this has merged:

runtime.STATIC_URL is deprecated. Please use settings.STATIC_URL instead

@Agrendalath Agrendalath force-pushed the agrendalath/bd-13-deprecate_static_url_in_modulesystem branch from 8f8763a to edc0cbd Compare September 21, 2022 13:49
@Agrendalath Agrendalath changed the title feat: remove static_url argument from ModuleSystem [BD-13] feat: deprecate static_url argument from ModuleSystem [BD-13] Sep 21, 2022
@Agrendalath
refactor: deprecate static_url argument from ModuleSystem
ca3e457 
This argument was officially used only by the ProblemBlock.
If you need to get the base URL for static assets in your XBlock, please use
`settings.STATIC_URL` directly, instead of `runtime.STATIC_URL`.
@Agrendalath Agrendalath force-pushed the agrendalath/bd-13-deprecate_static_url_in_modulesystem branch from edc0cbd to ca3e457 Compare September 21, 2022 13:53
@Agrendalath Agrendalath changed the title feat: deprecate static_url argument from ModuleSystem [BD-13] refactor: deprecate static_url argument from ModuleSystem [BD-13] Sep 21, 2022
@Agrendalath Agrendalath merged commit 6686835 into openedx:master Sep 21, 2022
@Agrendalath Agrendalath deleted the agrendalath/bd-13-deprecate_static_url_in_modulesystem branch September 21, 2022 16:28
@openedx-webhooks
Copy link

openedx-webhooks commented Sep 21, 2022

@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
Copy link
Contributor

edx-pipeline-bot commented Sep 21, 2022

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-pipeline-bot commented Sep 21, 2022

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

Reviewers

@ormsbee ormsbee ormsbee approved these changes

@pkulkark pkulkark Awaiting requested review from pkulkark

Assignees

No one assigned

Labels

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

Projects

No open projects
2U Blended Project
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Agrendalath @openedx-webhooks @ormsbee @edx-pipeline-bot