-
Notifications
You must be signed in to change notification settings - Fork 71
feat: add pluggable override for learner home enterprise customer lookup #2554
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
base: master
Are you sure you want to change the base?
Changes from all commits
ddba068
968432a
588a5f4
aad269e
65c1663
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,4 +2,4 @@ | |
| Your project description goes here. | ||
| """ | ||
|
|
||
| __version__ = "8.4.0" | ||
| __version__ = "8.5.0" | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,76 @@ | ||||||
| """ | ||||||
| Pluggable override for the learner home enterprise customer lookup. | ||||||
|
|
||||||
| .. note:: | ||||||
| TODO: Wire this up to OVERRIDE_LEARNER_HOME_GET_ENTERPRISE_CUSTOMER once | ||||||
| edx-enterprise is a plugin (ENT-11584), before the plugin becomes optional. | ||||||
|
|
||||||
| This override is **not yet wired up** to the | ||||||
| ``OVERRIDE_LEARNER_HOME_GET_ENTERPRISE_CUSTOMER`` setting. Registering it requires | ||||||
| edx-enterprise to be installable/importable as an openedx plugin, which is not yet | ||||||
| the case. | ||||||
| """ | ||||||
| import logging | ||||||
| from typing import TYPE_CHECKING, Optional, TypedDict | ||||||
|
|
||||||
| from rest_framework.request import Request | ||||||
|
|
||||||
| # Will be replaced with an internal path in ENT-11576. | ||||||
| try: | ||||||
| from openedx.features.enterprise_support.api import ( | ||||||
| enterprise_customer_from_session_or_learner_data, | ||||||
| get_enterprise_learner_data_from_db, | ||||||
| ) | ||||||
| except ImportError: | ||||||
| enterprise_customer_from_session_or_learner_data = None | ||||||
| get_enterprise_learner_data_from_db = None | ||||||
|
|
||||||
| if TYPE_CHECKING: | ||||||
| from django.contrib.auth.models import User | ||||||
|
|
||||||
| log = logging.getLogger(__name__) | ||||||
|
|
||||||
|
|
||||||
| class EnterpriseCustomerData(TypedDict): | ||||||
| """ | ||||||
| Required keys for the learner home enterprise dashboard. | ||||||
|
|
||||||
| Mirrors ``lms.djangoapps.learner_home.views.EnterpriseCustomerData`` | ||||||
| """ | ||||||
| name: str | ||||||
| uuid: str | ||||||
| slug: str | ||||||
| auth_org_id: Optional[str] | ||||||
| enable_learner_portal: bool | ||||||
|
|
||||||
|
|
||||||
| def enterprise_get_enterprise_customer( | ||||||
| prev_fn, | ||||||
| user: "User", | ||||||
| request: Request, | ||||||
| is_masquerading: bool, | ||||||
| ) -> Optional[EnterpriseCustomerData]: | ||||||
| """ | ||||||
| Return the enterprise customer dict for the given user, or None. | ||||||
|
|
||||||
| This function overrides the default ``get_enterprise_customer`` implementation in | ||||||
| ``lms/djangoapps/learner_home/views.py`` via the pluggable override mechanism. | ||||||
|
|
||||||
| Arguments: | ||||||
| prev_fn: the previous (default) implementation, called when the enterprise | ||||||
| utility is unavailable. | ||||||
| user: the Django User object. | ||||||
| request: the current HTTP request. | ||||||
| is_masquerading (bool): True when the request is a staff masquerade. | ||||||
|
|
||||||
| Returns: | ||||||
| dict or None: enterprise customer data dict, or None if the user is not an | ||||||
| enterprise customer user. | ||||||
| """ | ||||||
| if enterprise_customer_from_session_or_learner_data is None: | ||||||
| return prev_fn(user, request, is_masquerading) | ||||||
|
Comment on lines
+70
to
+71
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can just have this fail fatally. Eventually this function will get migrated to edx-enterprise anyway.
Suggested change
|
||||||
|
|
||||||
| if is_masquerading: | ||||||
| learner_data = get_enterprise_learner_data_from_db(user) | ||||||
| return learner_data[0]['enterprise_customer'] if learner_data else None | ||||||
| return enterprise_customer_from_session_or_learner_data(request) | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| """ | ||
| Tests for enterprise.overrides.learner_home pluggable override. | ||
| """ | ||
| from unittest import TestCase | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| ENTERPRISE_SUPPORT_API_PATH = 'openedx.features.enterprise_support.api' | ||
|
|
||
|
|
||
| class TestEnterpriseGetEnterpriseCustomer(TestCase): | ||
| """ | ||
| Tests for enterprise_get_enterprise_customer override function. | ||
| """ | ||
|
|
||
| def _call(self, user=None, request=None, is_masquerading=False): | ||
| """Import and call the override function.""" | ||
| # pylint: disable=import-outside-toplevel | ||
| from enterprise.overrides.learner_home import enterprise_get_enterprise_customer | ||
| prev_fn = MagicMock() | ||
| return enterprise_get_enterprise_customer(prev_fn, user, request, is_masquerading) | ||
|
Comment on lines
+15
to
+20
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this needs to be abstracted. Just do this in the tests directly: enterprise_get_enterprise_customer(MagicMock(), user, request, is_masquerading) |
||
|
|
||
| def test_non_masquerading_delegates_to_session_api(self): | ||
| """ | ||
| When is_masquerading is False, should call | ||
| enterprise_customer_from_session_or_learner_data(request) and return its result. | ||
| """ | ||
| request = MagicMock() | ||
| user = MagicMock() | ||
| expected = {'uuid': 'test-uuid'} | ||
| mock_api = MagicMock() | ||
| mock_api.enterprise_customer_from_session_or_learner_data.return_value = expected | ||
| mock_api.get_enterprise_learner_data_from_db.return_value = [] | ||
|
|
||
| with patch.dict('sys.modules', {ENTERPRISE_SUPPORT_API_PATH: mock_api}): | ||
| result = self._call(user=user, request=request, is_masquerading=False) | ||
|
Comment on lines
+30
to
+35
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this was just how Claude Sonnet decided to mock an API, but it's extremely weird and indirect. I'd rather just see us patch the paths directly and do away with @patch('enterprise.overrides.learner_home.enterprise_customer_from_session_or_learner_data')
@patch('enterprise.overrides.learner_home.get_enterprise_learner_data_from_db')
def test_blah(
self,
mock_get_enterprise_learner_data_from_db,
mock_enterprise_customer_from_session_or_learner_data
):
...
mock_get_enterprise_learner_data_from_db.return_value = []
mock_enterprise_customer_from_session_or_learner_data.return_value = expected |
||
|
|
||
| mock_api.enterprise_customer_from_session_or_learner_data.assert_called_once_with(request) | ||
| mock_api.get_enterprise_learner_data_from_db.assert_not_called() | ||
| self.assertEqual(result, expected) | ||
|
|
||
| def test_masquerading_returns_enterprise_customer_from_db(self): | ||
| """ | ||
| When is_masquerading is True and learner data exists, should return the | ||
| enterprise_customer from the first learner data entry. | ||
| """ | ||
| user = MagicMock() | ||
| request = MagicMock() | ||
| enterprise_customer = {'uuid': 'ec-uuid', 'name': 'Test Enterprise'} | ||
| mock_api = MagicMock() | ||
| mock_api.get_enterprise_learner_data_from_db.return_value = [ | ||
| {'enterprise_customer': enterprise_customer} | ||
| ] | ||
|
|
||
| with patch.dict('sys.modules', {ENTERPRISE_SUPPORT_API_PATH: mock_api}): | ||
| result = self._call(user=user, request=request, is_masquerading=True) | ||
|
|
||
| mock_api.get_enterprise_learner_data_from_db.assert_called_once_with(user) | ||
| mock_api.enterprise_customer_from_session_or_learner_data.assert_not_called() | ||
| self.assertEqual(result, enterprise_customer) | ||
|
|
||
| def test_masquerading_returns_none_when_no_learner_data(self): | ||
| """ | ||
| When is_masquerading is True but no learner data exists, should return None. | ||
| """ | ||
| user = MagicMock() | ||
| request = MagicMock() | ||
| mock_api = MagicMock() | ||
| mock_api.get_enterprise_learner_data_from_db.return_value = [] | ||
|
|
||
| with patch.dict('sys.modules', {ENTERPRISE_SUPPORT_API_PATH: mock_api}): | ||
| result = self._call(user=user, request=request, is_masquerading=True) | ||
|
|
||
| mock_api.get_enterprise_learner_data_from_db.assert_called_once_with(user) | ||
| mock_api.enterprise_customer_from_session_or_learner_data.assert_not_called() | ||
| self.assertIsNone(result) | ||
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.
This is oudated. You should wire up the override in settings as part of this PR. See for inspiration:
edx-enterprise/enterprise/settings/common.py
Lines 86 to 88 in a045f9c