Skip to content
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

Merged
merged 1 commit into from
Apr 19, 2022

Conversation

mikix
Copy link
Contributor

@mikix mikix commented Apr 12, 2022

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:

  • Use preview mode if the test cares about the template chrome surrounding an xblock. This requires using a staff user.
  • Use the xblock_render endpoint (like the MFE does) if we just care about the xblock rendering itself (which lets the test continue using real normal users).
  • Removal of the test if it specifically cares about the template chrome we are disabling but also needs normal-user mode for that chrome (like access checks or some such).

Landing this should let us close DEPR openedx/public-engineering#53

@mikix mikix force-pushed the mikix/drop-courseware-view branch 7 times, most recently from ea65383 to 93aecc3 Compare April 18, 2022 21:40
@mikix mikix marked this pull request as ready for review April 18, 2022 21:40
@@ -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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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):
Copy link
Contributor Author

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.
@mikix mikix force-pushed the mikix/drop-courseware-view branch from 93aecc3 to afd19f0 Compare April 19, 2022 16:27
Copy link
Contributor

@cdeery cdeery left a 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

@mikix mikix merged commit f416308 into master Apr 19, 2022
@mikix mikix deleted the mikix/drop-courseware-view branch April 19, 2022 18:05
@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 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-pipeline-bot
Copy link
Contributor

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

@kdmccormick
Copy link
Member

Happy anniversary to this amazing PR. Thanks again @mikix 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants