feat: add pluggable override for learner home enterprise customer lookup#2554
feat: add pluggable override for learner home enterprise customer lookup#2554pwnage101 wants to merge 5 commits into
Conversation
| dict or None: enterprise customer data dict, or None if the user is not an | ||
| enterprise customer user. | ||
| """ | ||
| # Deferred imports — will be replaced with internal paths in epic 17. |
There was a problem hiding this comment.
Clarify comment that this defensive import is only here temporarily because the plan is ultimately to migrate the entire enterprise_support app into edx-enterprise.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (77.27%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #2554 +/- ##
==========================================
- Coverage 86.68% 86.67% -0.02%
==========================================
Files 258 259 +1
Lines 16995 17017 +22
Branches 1680 1682 +2
==========================================
+ Hits 14732 14749 +17
- Misses 1927 1931 +4
- Partials 336 337 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
335315e to
588a5f4
Compare
| if enterprise_customer_from_session_or_learner_data is None: | ||
| return prev_fn(user, request, is_masquerading) |
There was a problem hiding this comment.
I think we can just have this fail fatally. Eventually this function will get migrated to edx-enterprise anyway.
| if enterprise_customer_from_session_or_learner_data is None: | |
| return prev_fn(user, request, is_masquerading) |
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def enterprise_get_enterprise_customer(prev_fn, user, request, is_masquerading): |
There was a problem hiding this comment.
See edx/edx-platform#383 (comment)
We could do the same on this side:
| def enterprise_get_enterprise_customer(prev_fn, user, request, is_masquerading): | |
| 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]: |
| .. 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. |
There was a problem hiding this comment.
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
| """ | ||
| import logging | ||
|
|
||
| # Will be replaced with an internal path in epic 17. |
There was a problem hiding this comment.
epic 17 has no meaning outside my local environment. Refer to it as ENT-11576.
| # Will be replaced with an internal path in epic 17. | |
| # Will be replaced with an internal path in ENT-11576. |
| 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) |
There was a problem hiding this comment.
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)| 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) |
There was a problem hiding this comment.
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 ENTERPRISE_SUPPORT_API_PATH:
@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
ENT-11571
Migrates functionality from openedx-platform to look up a user's enterprise status and populate
enterpriseDashboardin/api/learner_home/init/endpoint.Related: