feat: decouple enterprise from courseware view redirects - #338
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the enterprise-specific “data sharing consent” redirect decorator from edx-platform courseware/wiki entry points and replaces it with a generic hook point: a new courseware_view_hooks decorator that runs the CoursewareViewStarted filter, allowing plugins (e.g., edx-enterprise) to redirect learners as needed.
Changes:
- Removed
data_sharing_consent_required(and related tests/mixins) fromopenedx.features.enterprise_support. - Added
lms.djangoapps.courseware.decorators.courseware_view_hooksand applied it to key courseware and wiki views/middleware. - Updated/added tests to assert redirects occur when
CoursewareViewStartedraisesRedirectToUrl, and refinedcourse_id_from_urltyping/docs.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| openedx/features/enterprise_support/tests/test_api.py | Removes tests tied to the deleted enterprise consent decorator. |
| openedx/features/enterprise_support/tests/mixins/enterprise.py | Removes the consent-redirect testing helper mixin class. |
| openedx/features/enterprise_support/api.py | Deletes the data_sharing_consent_required decorator implementation. |
| openedx/core/lib/request_utils.py | Adds type hints / clarifies behavior for course_id_from_url returning a CourseKey. |
| lms/djangoapps/discussion/tests/test_views_v2.py | Removes enterprise-consent redirect tests for discussion views. |
| lms/djangoapps/courseware/views/views.py | Swaps enterprise decorator for courseware_view_hooks on courseware views (e.g., progress/tab). |
| lms/djangoapps/courseware/views/index.py | Swaps enterprise decorator for courseware_view_hooks on the courseware index view. |
| lms/djangoapps/courseware/tests/test_views.py | Adds test coverage for filter-driven redirects on progress URLs. |
| lms/djangoapps/courseware/tests/test_view_authentication.py | Removes consent mixin inheritance (but currently leaves a broken import; see comments). |
| lms/djangoapps/courseware/decorators.py | Introduces courseware_view_hooks decorator that runs CoursewareViewStarted. |
| lms/djangoapps/course_wiki/views.py | Applies courseware_view_hooks to wiki redirect view instead of enterprise decorator. |
| lms/djangoapps/course_wiki/tests/tests.py | Updates wiki tests for filter-driven redirect (but currently contains merge-conflict markers and a broken import; see comments). |
| lms/djangoapps/course_wiki/middleware.py | Replaces enterprise consent redirect logic with CoursewareViewStarted filter redirect in middleware. |
Comments suppressed due to low confidence (1)
lms/djangoapps/course_wiki/tests/tests.py:13
- This file still imports
EnterpriseTestConsentRequired, but that helper class has been removed fromopenedx.features.enterprise_support.tests.mixins.enterprise. This will raise ImportError when running tests; remove the import.
from lms.djangoapps.courseware.tests.tests import LoginEnrollmentTestCase
from openedx.features.course_experience.url_helpers import make_learning_mfe_courseware_url
from openedx.features.enterprise_support.tests.mixins.enterprise import EnterpriseTestConsentRequired
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b370e29 to
f480ad8
Compare
f480ad8 to
fd6230e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
lms/djangoapps/course_wiki/tests/tests.py:13
- This test file still imports EnterpriseTestConsentRequired, but that mixin has been removed from openedx.features.enterprise_support.tests.mixins.enterprise in this PR. This will raise ImportError and fail the test suite; the import should be removed.
from lms.djangoapps.courseware.tests.tests import LoginEnrollmentTestCase
from openedx.features.course_experience.url_helpers import make_learning_mfe_courseware_url
from openedx.features.enterprise_support.tests.mixins.enterprise import EnterpriseTestConsentRequired
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order
fd6230e to
cdeb4a4
Compare
cdeb4a4 to
cbb4d0a
Compare
cbb4d0a to
87c2e6a
Compare
87c2e6a to
20a04b3
Compare
Instead of an enterprise-specific view decorator conditionally redirecting learners to a consent view, plugins can now hook into the CoursewareViewStarted filter to redirect anywhere. ENT-11544
20a04b3 to
3a8bceb
Compare
Instead of an enterprise-specific view decorator conditionally redirecting learners to a consent view, plugins can now hook into the CoursewareViewStarted filter to redirect anywhere.
ENT-11544
blocked by:
blocks:
Testing
./scripts/provision-integration-test-ENT-11544.shfrom test: refactor the seed_enterprise_devstack_data command to be more composable openedx/edx-enterprise#2637enterprise_learner_test-enterprise@example.com/edx.