fix: Add enterpriseBranding to MFE context serializer#142
fix: Add enterpriseBranding to MFE context serializer#142ssurendrannair merged 6 commits intorelease-ulmofrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for enterprise branding data in the MFE (Micro Frontend) context API by adding the enterpriseBranding field to the ContextDataSerializer. This allows the serializer to properly handle enterprise branding information that is already being passed from the get_mfe_context utility function in utils.py.
Changes:
- Added
enterpriseBrandingfield toContextDataSerializerusing the existingEnterpriseBrandingSerializer - Field is configured as optional and nullable to maintain backward compatibility
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'optional_fields': optional_fields if optional_fields else { | ||
| 'fields': {}, | ||
| 'extended_profile': [], | ||
| }, |
There was a problem hiding this comment.
This welcome-page code path changes behavior when _get_optional_fields_context() returns {} (it now falls back to a non-empty structure and always serializes context). There’s no test covering the is_welcome_page + no-optional-fields case; adding one would prevent regressions where welcomePageRedirectUrl gets dropped again.
| 'syncLearnerProfileData': False, | ||
| 'countryCode': '', | ||
| 'welcomePageRedirectUrl': '', | ||
| 'enterpriseBranding': None, | ||
| 'pipeline_user_details': { |
There was a problem hiding this comment.
Serializer tests were updated to assert enterpriseBranding exists, but the mock data only covers the None case. Add a mock example where enterpriseBranding is a populated dict and assert the serialized nested keys/values, so we catch mistakes like wrong key names or missing nested fields.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -88,12 +88,18 @@ def get(self, request, **kwargs): # lint-amnesty, pylint: disable=unused-argume | |||
| 'context_data': { | |||
| 'welcomePageRedirectUrl': redirect_to if redirect_to == request_params.get('next') else None, | |||
| }, | |||
There was a problem hiding this comment.
The welcome page code path completely replaces the context_data dictionary with only the welcomePageRedirectUrl field. This discards all the context data retrieved from get_mfe_context, including the newly added enterpriseBranding field, as well as other important fields like countryCode, platformName, providers, etc. The context_data should be updated rather than replaced. Consider using context['context_data'].update() to preserve the existing context while adding the welcomePageRedirectUrl.
| @mock.patch("lms.djangoapps.discussion.rest_api.api.get_course_user_stats") | ||
| def test_sorting( | ||
| self, | ||
| mock_get_course_user_stats, | ||
| username, | ||
| ordering_requested, | ||
| ordering_performed, | ||
| ): | ||
| """ | ||
| Test valid sorting options and defaults | ||
| """ | ||
| mock_get_course_user_stats.return_value = { | ||
| "user_stats": [], | ||
| "page": 1, | ||
| "num_pages": 1, | ||
| "count": 0, | ||
| } | ||
| self.client.login(username=username, password=self.TEST_PASSWORD) | ||
| params = {} | ||
| if ordering_requested: | ||
| params = {"order_by": ordering_requested} | ||
| self.client.get(self.url, params) | ||
| assert ( | ||
| urlparse( | ||
| httpretty.last_request().path # lint-amnesty, pylint: disable=no-member | ||
| ).path | ||
| == f"/api/v1/users/{self.course_key}/stats" | ||
| ) | ||
| assert parse_qs( | ||
| urlparse( | ||
| httpretty.last_request().path | ||
| ).query # lint-amnesty, pylint: disable=no-member | ||
| ).get("sort_key", None) == [ordering_performed] | ||
|
|
||
| call_args, call_kwargs = mock_get_course_user_stats.call_args | ||
| called_params = call_kwargs.get("params") or call_args[1] | ||
| assert called_params.get("sort_key") == ordering_performed |
There was a problem hiding this comment.
The refactoring of this test from httpretty to mock.patch appears unrelated to the PR's stated purpose of adding enterpriseBranding to the MFE context serializer. While this is an improvement (more focused mocking), it's unclear why it's included in this PR. Consider either: 1) adding a comment in the PR description explaining why this change is necessary, or 2) moving this test refactoring to a separate PR focused on test improvements.
| assert response.data['registrationFields']['fields'] == {} | ||
| assert response.data['optionalFields']['fields'] == {} | ||
| assert response.data['optionalFields']['extended_profile'] == [] | ||
| assert response.data['contextData']['welcomePageRedirectUrl'] == redirect_url |
There was a problem hiding this comment.
The new test case test_welcome_page_context_without_optional_fields only verifies the structure of the response when optional fields are empty, but it doesn't test that enterpriseBranding is present in the contextData for the welcome page scenario. Consider adding an assertion to verify that response.data['contextData']['enterpriseBranding'] is None (or testing with actual enterprise branding data if applicable). This would help catch the bug where enterpriseBranding and other context data are lost in the welcome page code path.
| assert response.data['contextData']['welcomePageRedirectUrl'] == redirect_url | |
| assert response.data['contextData']['welcomePageRedirectUrl'] == redirect_url | |
| assert 'enterpriseBranding' in response.data['contextData'] | |
| assert response.data['contextData']['enterpriseBranding'] is None |
| data=MFEContextSerializer( | ||
| context if optional_fields else {} | ||
| context | ||
| ).data |
There was a problem hiding this comment.
This change ensures that the MFE context serializer always receives a full context object instead of an empty dict when optional_fields is falsy. While this prevents serialization errors, it doesn't address the fundamental issue that the context_data is being replaced with only welcomePageRedirectUrl (losing enterpriseBranding and other fields). The serializer will receive an incomplete context_data in the welcome page scenario.
Add enterpriseBranding to MFE context serializer