-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
Drop courseware legacy view for all but Studio preview mode #30238
Conversation
ea65383
to
93aecc3
Compare
@@ -590,7 +595,6 @@ def test_mfe_link_from_about_page(self): | |||
with self.store.default_store(ModuleStoreEnum.Type.split): | |||
course = CourseFactory.create() | |||
CourseEnrollment.enroll(self.user, course.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you need to login? Is it already done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah in __init__
it was already being done. I don't remember this change, but looks like just a drive by cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these user-setup changes were done because of changing from normal users to staff users or similar. But this one 🤷
@@ -59,7 +57,7 @@ | |||
|
|||
|
|||
@patch("crum.get_current_request") | |||
def _get_content_from_fragment(block, user_id, course, request_factory, mock_get_current_request): | |||
def _get_content_from_fragment(_store, block, user_id, course, request_factory, mock_get_current_request): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's why I named it with an underscore. I added it to this function because in the tests, this method and _get_content_from_lms_index
are both used with the same arguments (see _assert_block_is_gated
) - so they need the same arguments signature.
The only way to access the legacy courseware is now through the Studio preview feature (and at some point, when the MFE supports a preview mode, we can then remove even that). This drops the courseware.use_legacy_frontend waffle.
93aecc3
to
afd19f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Epic work, Mike. Thanks
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage |
EdX Release Notice: This PR has been deployed to the production environment. |
Happy anniversary to this amazing PR. Thanks again @mikix 🙏🏻 |
This leaves most of the legacy view templating in place, because Studio's preview mode still uses it. But I've removed all other access points (casual staff access, opt-out waffle, old mongo, query param).
Test changes have taken a few forms:
Landing this should let us close DEPR #35803