From 8edefe74ff536c0c8084d7232e6b1b0367999606 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Fri, 5 Aug 2022 16:34:00 -0400 Subject: [PATCH] feat!: change names of dynamic MFE config Django settings Formerly, the settings were: * `MFE_CONFIG` for common config. * `MFE_CONFIG_` for app-specific overrides, with each app getting its own Django setting. This commit changes it to: * `MFE_CONFIG` for common config (unchanged) * `MFE_CONFIG_OVERRIDES` for app-specific overrides, where each app gets a top-level key in the dictionary. Why the change? * We want common.py to have a complete list of overridable settings, as it helps operators reason about configuration and allows us to generate config documentation using toggle annotations. Dynamically generating setting names based on arbitrary APP_IDs makes this impossible. * getattr(...) generally makes code more complicated bug prone. Tools like pylint and mypy cannot effectively analyze any code that uses dynamic attribute access. --- .../docs/decisions/0001-mfe-config-api.rst | 2 +- .../mfe_config_api/tests/test_views.py | 97 ++++++++++++------- lms/djangoapps/mfe_config_api/views.py | 12 ++- lms/envs/common.py | 29 +++++- lms/envs/test.py | 12 ++- 5 files changed, 105 insertions(+), 47 deletions(-) diff --git a/lms/djangoapps/mfe_config_api/docs/decisions/0001-mfe-config-api.rst b/lms/djangoapps/mfe_config_api/docs/decisions/0001-mfe-config-api.rst index d26a40dc9a..f9bd7e6361 100644 --- a/lms/djangoapps/mfe_config_api/docs/decisions/0001-mfe-config-api.rst +++ b/lms/djangoapps/mfe_config_api/docs/decisions/0001-mfe-config-api.rst @@ -18,7 +18,7 @@ Decision - A lightweight API will be created that returns the mfe configuration variables from the site configuration or django settings. `PR Discussion about django settings`_ - The API will be enabled or disabled using the setting ``ENABLE_MFE_CONFIG_API``. - The API will take the mfe configuration in the ``MFE_CONFIG`` keyset in the site configuration (admin > site configuration > your domain) or in django settings. -- This API allows to consult the configurations by specific MFE. Making a request like ``api/v1/mfe_config?mfe=mymfe`` will return the configuration defined in ``MFE_CONFIG_MYMFE`` merged with the ``MFE_CONFIG`` configuration. +- This API allows to consult the configurations by specific MFE. Making a request like ``/api/v1/mfe_config?mfe=mymfe`` will return the configuration defined in ``MFE_CONFIG_OVERRIDES["mymfe"]`` merged with the ``MFE_CONFIG`` configuration. - The API will have a mechanism to cache the response with ``MFE_CONFIG_API_CACHE_TIMEOUT`` variable. - The API will live in lms/djangoapps because this is not something Studio needs to serve and it is a lightweight API. `PR Discussion`_ - The API will not require authentication or authorization. diff --git a/lms/djangoapps/mfe_config_api/tests/test_views.py b/lms/djangoapps/mfe_config_api/tests/test_views.py index 35ad520d34..fef7574f59 100644 --- a/lms/djangoapps/mfe_config_api/tests/test_views.py +++ b/lms/djangoapps/mfe_config_api/tests/test_views.py @@ -27,14 +27,14 @@ class MFEConfigTestCase(APITestCase): Expected result: - The get_value method of the configuration_helpers in the views is called once with the - parameters ("MFE_CONFIG", getattr(settings, "MFE_CONFIG", {})). + parameters ("MFE_CONFIG", settings.MFE_CONFIG) - The status of the response of the request is a HTTP_200_OK. - The json of the response of the request is equal to the mocked configuration. """ configuration_helpers_mock.get_value.return_value = {"EXAMPLE_VAR": "value"} response = self.client.get(self.mfe_config_api_url) - configuration_helpers_mock.get_value.assert_called_once_with("MFE_CONFIG", getattr(settings, "MFE_CONFIG", {})) + configuration_helpers_mock.get_value.assert_called_once_with("MFE_CONFIG", settings.MFE_CONFIG) self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.json(), {"EXAMPLE_VAR": "value"}) @@ -44,57 +44,87 @@ class MFEConfigTestCase(APITestCase): Expected result: - The get_value method of the configuration_helpers in the views is called twice, once with the - parameters ("MFE_CONFIG", getattr(settings, "MFE_CONFIG", {})) and once with the parameters - ("MFE_CONFIG_MYMFE", getattr(settings, "MFE_CONFIG_MYMFE", {})). - and one for get_value("MFE_CONFIG_MYMFE", getattr(settings, "MFE_CONFIG_MYMFE", {})). + parameters ("MFE_CONFIG", settings.MFE_CONFIG) + and once with the parameters ("MFE_CONFIG_OVERRIDES", settings.MFE_CONFIG_OVERRIDES). - The json of the response is the merge of both mocked configurations. """ - configuration_helpers_mock.get_value.side_effect = [{"EXAMPLE_VAR": "value", "OTHER": "other"}, - {"EXAMPLE_VAR": "mymfe_value"}] + configuration_helpers_mock.get_value.side_effect = [ + {"EXAMPLE_VAR": "value", "OTHER": "other"}, + {"mymfe": {"EXAMPLE_VAR": "mymfe_value"}}, + ] response = self.client.get(f"{self.mfe_config_api_url}?mfe=mymfe") self.assertEqual(response.status_code, status.HTTP_200_OK) - calls = [call("MFE_CONFIG", getattr(settings, "MFE_CONFIG", {})), - call("MFE_CONFIG_MYMFE", getattr(settings, "MFE_CONFIG_MYMFE", {}))] + calls = [call("MFE_CONFIG", settings.MFE_CONFIG), + call("MFE_CONFIG_OVERRIDES", settings.MFE_CONFIG_OVERRIDES)] configuration_helpers_mock.get_value.assert_has_calls(calls) self.assertEqual(response.json(), {"EXAMPLE_VAR": "mymfe_value", "OTHER": "other"}) - @patch("lms.djangoapps.mfe_config_api.views.configuration_helpers") - @ddt.data( - [{}, {}, {}], - [{"EXAMPLE_VAR": "value"}, {}, {"EXAMPLE_VAR": "value"}], - [{}, {"EXAMPLE_VAR": "mymfe_value"}, {"EXAMPLE_VAR": "mymfe_value"}], - [{"EXAMPLE_VAR": "value"}, {"EXAMPLE_VAR": "mymfe_value"}, {"EXAMPLE_VAR": "mymfe_value"}], - [{"EXAMPLE_VAR": "value", "OTHER": "other"}, {"EXAMPLE_VAR": "mymfe_value"}, - {"EXAMPLE_VAR": "mymfe_value", "OTHER": "other"}], - ) @ddt.unpack + @ddt.data( + dict( + mfe_config={}, + mfe_config_overrides={}, + expected_response={}, + ), + dict( + mfe_config={"EXAMPLE_VAR": "value"}, + mfe_config_overrides={}, + expected_response={"EXAMPLE_VAR": "value"}, + ), + dict( + mfe_config={}, + mfe_config_overrides={"mymfe": {"EXAMPLE_VAR": "mymfe_value"}}, + expected_response={"EXAMPLE_VAR": "mymfe_value"}, + ), + dict( + mfe_config={"EXAMPLE_VAR": "value"}, + mfe_config_overrides={"mymfe": {"EXAMPLE_VAR": "mymfe_value"}}, + expected_response={"EXAMPLE_VAR": "mymfe_value"}, + ), + dict( + mfe_config={"EXAMPLE_VAR": "value", "OTHER": "other"}, + mfe_config_overrides={"mymfe": {"EXAMPLE_VAR": "mymfe_value"}}, + expected_response={"EXAMPLE_VAR": "mymfe_value", "OTHER": "other"}, + ), + dict( + mfe_config={"EXAMPLE_VAR": "value"}, + mfe_config_overrides={"yourmfe": {"EXAMPLE_VAR": "yourmfe_value"}}, + expected_response={"EXAMPLE_VAR": "value"}, + ), + dict( + mfe_config={"EXAMPLE_VAR": "value"}, + mfe_config_overrides={ + "yourmfe": {"EXAMPLE_VAR": "yourmfe_value"}, + "mymfe": {"EXAMPLE_VAR": "mymfe_value"}, + }, + expected_response={"EXAMPLE_VAR": "mymfe_value"}, + ), + ) + @patch("lms.djangoapps.mfe_config_api.views.configuration_helpers") def test_get_mfe_config_with_queryparam_multiple_configs( self, + configuration_helpers_mock, mfe_config, - mfe_config_mymfe, + mfe_config_overrides, expected_response, - configuration_helpers_mock ): - """Test the get mfe config with a query param and different settings in mfe_config and mfe_config_mfe inside + """Test the get mfe config with a query param and different settings in mfe_config and mfe_config_overrides with the site configuration to test that the merge of the configurations is done correctly and mymfe config take precedence. - In the ddt data the following structure is being passed: - [mfe_config, mfe_config_mymfe, expected_response] - Expected result: - The get_value method of the configuration_helpers in the views is called twice, once with the - parameters ("MFE_CONFIG", getattr(settings, "MFE_CONFIG", {})) and once with the parameters - ("MFE_CONFIG_MYMFE", getattr(settings, "MFE_CONFIG_MYMFE", {})). + parameters ("MFE_CONFIG", settings.MFE_CONFIG) + and once with the parameters ("MFE_CONFIG_OVERRIDES", settings.MFE_CONFIG_OVERRIDES). - The json of the response is the expected_response passed by ddt.data. """ - configuration_helpers_mock.get_value.side_effect = [mfe_config, mfe_config_mymfe] + configuration_helpers_mock.get_value.side_effect = [mfe_config, mfe_config_overrides] response = self.client.get(f"{self.mfe_config_api_url}?mfe=mymfe") self.assertEqual(response.status_code, status.HTTP_200_OK) - calls = [call("MFE_CONFIG", getattr(settings, "MFE_CONFIG", {})), - call("MFE_CONFIG_MYMFE", getattr(settings, "MFE_CONFIG_MYMFE", {}))] + calls = [call("MFE_CONFIG", settings.MFE_CONFIG), + call("MFE_CONFIG_OVERRIDES", settings.MFE_CONFIG_OVERRIDES)] configuration_helpers_mock.get_value.assert_has_calls(calls) self.assertEqual(response.json(), expected_response) @@ -106,20 +136,19 @@ class MFEConfigTestCase(APITestCase): - The json response is equal to MFE_CONFIG in lms/envs/test.py""" response = self.client.get(self.mfe_config_api_url) self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.json(), getattr(settings, "MFE_CONFIG", {})) + self.assertEqual(response.json(), settings.MFE_CONFIG) def test_get_mfe_config_with_queryparam_from_django_settings(self): """Test that when there is no site configuration, the API with queryparam takes the django settings. Expected result: - The status of the response of the request is a HTTP_200_OK. - - The json response is equal to MFE_CONFIG merged with MFE_CONFIG_MYMFE in lms/envs/test.py + - The json response is equal to MFE_CONFIG merged with MFE_CONFIG_OVERRIDES['mymfe'] """ response = self.client.get(f"{self.mfe_config_api_url}?mfe=mymfe") self.assertEqual(response.status_code, status.HTTP_200_OK) - expected_response = getattr(settings, "MFE_CONFIG", {}) - expected_response.update(getattr(settings, "MFE_CONFIG_MYMFE", {})) - self.assertEqual(response.json(), expected_response) + expected = {**settings.MFE_CONFIG, **settings.MFE_CONFIG_OVERRIDES["mymfe"]} + self.assertEqual(response.json(), expected) @patch("lms.djangoapps.mfe_config_api.views.configuration_helpers") @override_settings(ENABLE_MFE_CONFIG_API=False) diff --git a/lms/djangoapps/mfe_config_api/views.py b/lms/djangoapps/mfe_config_api/views.py index 13f4e4955c..42a5d60338 100644 --- a/lms/djangoapps/mfe_config_api/views.py +++ b/lms/djangoapps/mfe_config_api/views.py @@ -43,10 +43,12 @@ class MFEConfigView(APIView): if not settings.ENABLE_MFE_CONFIG_API: return HttpResponseNotFound() - mfe_config = configuration_helpers.get_value('MFE_CONFIG', getattr(settings, 'MFE_CONFIG', {})) + mfe_config = configuration_helpers.get_value('MFE_CONFIG', settings.MFE_CONFIG) if request.query_params.get('mfe'): - mfe = str(request.query_params.get('mfe')).upper() - mfe_config.update(configuration_helpers.get_value( - f'MFE_CONFIG_{mfe}', getattr(settings, f'MFE_CONFIG_{mfe}', {}))) - + mfe = str(request.query_params.get('mfe')) + app_config = configuration_helpers.get_value( + 'MFE_CONFIG_OVERRIDES', + settings.MFE_CONFIG_OVERRIDES, + ) + mfe_config.update(app_config.get(mfe, {})) return JsonResponse(mfe_config, status=status.HTTP_200_OK) diff --git a/lms/envs/common.py b/lms/envs/common.py index c05f7caac3..10d085c033 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -5171,8 +5171,10 @@ ENABLE_MFE_CONFIG_API = False # .. setting_name: MFE_CONFIG # .. setting_implementation: DjangoSetting # .. setting_default: {} -# .. setting_description: Is a configuration that will be exposed by the MFE Config API to be consumed by the mfes -# Example: { +# .. setting_description: Is a configuration that will be exposed by the MFE Config API to be consumed by the MFEs. +# Contains configuration common to all MFEs. When a specific MFE's configuration is requested, these values +# will be treated as a base and then overriden/supplemented by those in `MFE_CONFIG_OVERRIDES`. +# Example: { # "BASE_URL": "https://name_of_mfe.example.com", # "LANGUAGE_PREFERENCE_COOKIE_NAME": "example-language-preference", # "CREDENTIALS_BASE_URL": "https://credentials.example.com", @@ -5182,11 +5184,30 @@ ENABLE_MFE_CONFIG_API = False # "LOGOUT_URL": "https://courses.example.com/logout", # "STUDIO_BASE_URL": "https://studio.example.com", # "LOGO_URL": "https://courses.example.com/logo.png" -# } +# } # .. setting_use_cases: open_edx -# .. setting_creation_date: 2022-07-08 +# .. setting_creation_date: 2022-08-05 MFE_CONFIG = {} +# .. setting_name: MFE_CONFIG_OVERRIDES +# .. setting_implementation: DjangoSetting +# .. setting_default: {} +# .. setting_description: Overrides or additions to `MFE_CONFIG` for when a specific MFE is requested +# by the MFE Config API. Top-level keys are APP_IDs, a.k.a. the name of the MFE (for example, +# for an MFE named "frontend-app-xyz", the top-level key would be "xyz"). +# Example: { +# "gradebook": { +# "BASE_URL": "https://gradebook.example.com", +# }, +# "profile": { +# "BASE_URL": "https://profile.example.com", +# "ENABLE_LEARNER_RECORD_MFE": "true", +# }, +# } +# .. setting_use_cases: open_edx +# .. setting_creation_date: 2022-08-05 +MFE_CONFIG_OVERRIDES = {} + # .. setting_name: MFE_CONFIG_API_CACHE_TIMEOUT # .. setting_default: 60*5 # .. setting_description: The MFE Config API response will be cached during the diff --git a/lms/envs/test.py b/lms/envs/test.py index 08abdbdcf6..57de824ad0 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -656,7 +656,13 @@ MFE_CONFIG = { "LOGO_URL": "https://courses.example.com/logo.png" } -MFE_CONFIG_MYMFE = { - "LANGUAGE_PREFERENCE_COOKIE_NAME": "mymfe-language-preference", - "LOGO_URL": "https://courses.example.com/mymfe-logo.png" +MFE_CONFIG_OVERRIDES = { + "mymfe": { + "LANGUAGE_PREFERENCE_COOKIE_NAME": "mymfe-language-preference", + "LOGO_URL": "https://courses.example.com/mymfe-logo.png", + }, + "yourmfe": { + "LANGUAGE_PREFERENCE_COOKIE_NAME": "yourmfe-language-preference", + "LOGO_URL": "https://courses.example.com/yourmfe-logo.png", + }, }