Fix oppia#15995: Adding more strict types to customization args. (opp…

…ia#16551) * added fix phase-1 * added nit fix * added changes * fixed import error * added changes * added changes * nit * added changes * added changes - minor * nit * added changes * added changes * added changes - 2 * updated comment * added minonr fixes
DubeySandeep · Nov 21, 2022 · 28c3966 · 28c3966
1 parent 711c950
commit 28c3966
Show file tree

Hide file tree

Showing 20 changed files with 944 additions and 654 deletions.
diff --git a/core/controllers/contributor_dashboard_test.py b/core/controllers/contributor_dashboard_test.py
@@ -24,6 +24,7 @@
 from core.domain import exp_domain
 from core.domain import exp_fetchers
 from core.domain import exp_services
+from core.domain import state_domain
 from core.domain import story_domain
 from core.domain import story_fetchers
 from core.domain import story_services
@@ -36,7 +37,7 @@
 from core.platform import models
 from core.tests import test_utils
 
-from typing import Dict, List
+from typing import Dict, List, cast
 
 MYPY = False
 if MYPY:  # pragma: no cover
@@ -497,9 +498,18 @@ def test_get_reviewable_translation_opportunities_when_state_is_removed(
 
         # Create a translation suggestion for continue text.
         continue_state = exp_100.states['continue state']
-        content_id_of_continue_button_text = (
+        # Here we use cast because we are narrowing down the type from various
+        # customization args value types to 'SubtitledUnicode' type, and this
+        # is done because here we are accessing 'buttontext' key from continue
+        # customization arg whose value is always of SubtitledUnicode type.
+        subtitled_unicode_of_continue_button_text = cast(
+            state_domain.SubtitledUnicode,
             continue_state.interaction.customization_args[
-                'buttonText'].value.content_id)
+                'buttonText'].value
+        )
+        content_id_of_continue_button_text = (
+            subtitled_unicode_of_continue_button_text.content_id
+        )
         change_dict = {
             'cmd': 'add_translation',
             'content_id': content_id_of_continue_button_text,
@@ -582,9 +592,18 @@ def test_get_reviewable_translation_opportunities_when_original_content_is_remov
 
         # Create a translation suggestion for the continue text.
         continue_state = exp_100.states['continue state']
-        content_id_of_continue_button_text = (
+        # Here we use cast because we are narrowing down the type from various
+        # customization args value types to 'SubtitledUnicode' type, and this
+        # is done because here we are accessing 'buttontext' key from continue
+        # customization arg whose value is always of SubtitledUnicode type.
+        subtitled_unicode_of_continue_button_text = cast(
+            state_domain.SubtitledUnicode,
             continue_state.interaction.customization_args[
-                'buttonText'].value.content_id)
+                'buttonText'].value
+        )
+        content_id_of_continue_button_text = (
+            subtitled_unicode_of_continue_button_text.content_id
+        )
         change_dict = {
             'cmd': 'add_translation',
             'content_id': content_id_of_continue_button_text,

diff --git a/core/controllers/suggestion_test.py b/core/controllers/suggestion_test.py
@@ -46,7 +46,7 @@
 from core.platform import models
 from core.tests import test_utils
 
-from typing import Dict, Final, Union
+from typing import Dict, Final, Union, cast
 
 MYPY = False
 if MYPY: # pragma: no cover
@@ -2862,9 +2862,17 @@ def test_exploration_handler_does_not_return_obsolete_suggestions(
         # Create a translation suggestion for the Continue button text.
         self.login(self.AUTHOR_EMAIL)
         continue_state = exp_100.states['continue state']
+        # Here we use cast because we are narrowing down the type from various
+        # customization args value types to 'SubtitledUnicode' type, and this
+        # is done because here we are accessing 'buttontext' key from continue
+        # customization arg whose value is always of SubtitledUnicode type.
+        subtitled_unicode_of_continue_button_text = cast(
+            state_domain.SubtitledUnicode,
+            continue_state.interaction.customization_args['buttonText'].value
+        )
         content_id_of_continue_button_text = (
-            continue_state.interaction.customization_args[
-                'buttonText'].value.content_id)
+            subtitled_unicode_of_continue_button_text.content_id
+        )
         change_dict = {
             'cmd': 'add_translation',
             'content_id': content_id_of_continue_button_text,

diff --git a/core/domain/customization_args_util.py b/core/domain/customization_args_util.py
@@ -20,55 +20,40 @@
 
 from core import schema_utils
 from core import utils
+from core.domain import state_domain
 from extensions import domain
 
-from typing import Any, Dict, List
+from typing import Dict, List, Mapping, Optional, Union
 
+MYPY = False
+if MYPY:  # pragma: no cover
+    AcceptableCustomizationArgsTypes = Union[
+        str,
+        int,
+        bool,
+        List[str],
+        domain.GraphDict,
+        Dict[str, Optional[str]],
+        List[state_domain.SubtitledHtml],
+        List[state_domain.SubtitledHtmlDict],
+        state_domain.SubtitledHtmlDict,
+        state_domain.SubtitledUnicodeDict,
+        state_domain.SubtitledUnicode,
+        domain.ImageAndRegionDict,
+        domain.CustomizationArgSubtitledUnicodeDefaultDict,
+        List[domain.CustomizationArgSubtitledUnicodeDefaultDict],
+        None
+    ]
 
-# TODO(#15982): Here we use type Any because argument 'customization_args', can
-# accepts the values of customization args and that values can be of type str,
-# int, Dict, bool, List and other types too. So to make it generalize for every
-# type of values, we used Any here.
-def get_full_customization_args(
-    customization_args: Dict[str, Dict[str, Any]],
-    ca_specs: List[domain.CustomizationArgSpec]
-) -> Dict[str, Dict[str, Any]]:
-    """Populates the given customization_args dict with default values
-    if any of the expected customization_args are missing.
-
-    Args:
-        customization_args: dict. The customization dict. The keys are names
-            of customization_args and the values are dicts with a
-            single key, 'value', whose corresponding value is the value of
-            the customization arg.
-        ca_specs: list(dict). List of spec dictionaries. Is used to check if
-            some keys are missing in customization_args. Dicts have the
-            following structure:
-                - name: str. The customization variable name.
-                - description: str. The customization variable description.
-                - default_value: *. The default value of the customization
-                    variable.
-
-    Returns:
-        dict. The customization_args dict where missing keys are populated
-        with the default values.
-    """
-    for ca_spec in ca_specs:
-        if ca_spec.name not in customization_args:
-            customization_args[ca_spec.name] = {
-                'value': ca_spec.default_value
-            }
-    return customization_args
+    CustomizationArgsDictType = Mapping[
+        str, Mapping[str, AcceptableCustomizationArgsTypes]
+    ]
 
 
-# TODO(#15982): Here we use type Any because argument 'customization_args', can
-# accepts the values of customization args and that values can be of type str,
-# int, Dict, bool, List and other types too. So to make it generalize for every
-# type of values, we used Any here.
 def validate_customization_args_and_values(
     item_name: str,
     item_type: str,
-    customization_args: Dict[str, Dict[str, Any]],
+    customization_args: CustomizationArgsDictType,
     ca_specs_to_validate_against: List[domain.CustomizationArgSpec],
     fail_on_validation_errors: bool = False
 ) -> None:

diff --git a/core/domain/customization_args_util_test.py b/core/domain/customization_args_util_test.py
@@ -27,118 +27,12 @@
 from core.domain import interaction_registry
 from core.tests import test_utils
 
-from typing import Dict, List, Optional, Union
+from typing import Dict, List, Union
 
 
 class CustomizationArgsUtilUnitTests(test_utils.GenericTestBase):
     """Test customization args generation and validation."""
 
-    def test_get_full_customization_args(self) -> None:
-        """Test get full customization args method."""
-        ca_continue_specs = interaction_registry.Registry.get_interaction_by_id(
-            'Continue').customization_arg_specs
-        complete_customization_args = {
-            'buttonText': {
-                'value': {
-                    'content_id': None,
-                    'unicode_str': 'Please Continue'
-                }
-            }
-        }
-
-        complete_customization_args_with_extra_arg: (
-            Dict[str, Dict[str, Union[str, Dict[str, Optional[str]]]]]
-        ) = {
-            'buttonText': {
-                'value': {
-                    'content_id': None,
-                    'unicode_str': 'Please Continue'
-                }
-            },
-            'extraArg': {'value': ''}
-        }
-
-        # Check if no new key is added to customization arg dict if all specs
-        # are present.
-        self.assertEqual(
-            complete_customization_args,
-            customization_args_util.get_full_customization_args(
-                complete_customization_args, ca_continue_specs
-            )
-        )
-
-        # Check if no new key is added to customization arg dict and extra keys
-        # are not removed if all specs are present.
-        self.assertEqual(
-            complete_customization_args_with_extra_arg,
-            customization_args_util.get_full_customization_args(
-                complete_customization_args_with_extra_arg,
-                ca_continue_specs
-            )
-        )
-
-        ca_fraction_input_specs = (
-            interaction_registry.Registry.get_interaction_by_id(
-                'FractionInput').customization_arg_specs
-        )
-
-        incomplete_customization_args = {
-            'requireSimplestForm': {'value': False},
-            'allowNonzeroIntegerPart': {'value': False}
-        }
-
-        incomplete_customization_args_with_extra_arg: (
-            Dict[str, Dict[str, Union[str, bool]]]
-        ) = {
-            'requireSimplestForm': {'value': False},
-            'allowNonzeroIntegerPart': {'value': False},
-            'extraArg': {'value': ''}
-        }
-
-        expected_complete_customization_args = {
-            'requireSimplestForm': {'value': False},
-            'allowImproperFraction': {'value': True},
-            'allowNonzeroIntegerPart': {'value': False},
-            'customPlaceholder': {
-                'value': {
-                    'content_id': None,
-                    'unicode_str': ''
-                }
-            }
-        }
-
-        expected_complete_customization_args_with_extra_arg = {
-            'requireSimplestForm': {'value': False},
-            'allowImproperFraction': {'value': True},
-            'allowNonzeroIntegerPart': {'value': False},
-            'customPlaceholder': {
-                'value': {
-                    'content_id': None,
-                    'unicode_str': ''
-                }
-            },
-            'extraArg': {'value': ''}
-        }
-
-        # Check if missing specs are added to customization arg dict without
-        # making any other change.
-        self.assertEqual(
-            expected_complete_customization_args,
-            customization_args_util.get_full_customization_args(
-                incomplete_customization_args, ca_fraction_input_specs
-            )
-        )
-
-        # Check if missing specs are added to customization arg dict without
-        # making any other change and without removing extra args.
-        self.assertEqual(
-            expected_complete_customization_args_with_extra_arg,
-            customization_args_util.get_full_customization_args(
-                incomplete_customization_args_with_extra_arg,
-                ca_fraction_input_specs
-            )
-        )
-
     def test_validate_customization_args_and_values(self) -> None:
         """Test validate customization args and values method."""
 

diff --git a/core/domain/draft_upgrade_services.py b/core/domain/draft_upgrade_services.py
@@ -171,15 +171,27 @@ def _convert_html_in_draft_change_list(
                 )
                 new_value = edit_interaction_cust_args_cmd.new_value
                 if 'choices' in new_value.keys():
+                    # Here we use cast because we are narrowing down the type
+                    # from various customization args value types to List[
+                    # SubtitledHtmlDict] type, and this is done because here
+                    # we are accessing 'choices' keys over customization_args
+                    # and every customization arg that has a 'choices' key will
+                    # contain values of type List[SubtitledHtmlDict].
+                    subtitled_html_new_value_dicts = cast(
+                        List[state_domain.SubtitledHtmlDict],
+                        new_value['choices']['value']
+                    )
                     for value_index, value in enumerate(
-                            new_value['choices']['value']):
+                        subtitled_html_new_value_dicts
+                    ):
                         if isinstance(value, dict) and 'html' in value:
-                            new_value['choices']['value'][value_index][
+                            subtitled_html_new_value_dicts[value_index][
                                 'html'
                             ] = conversion_fn(value['html'])
                         elif isinstance(value, str):
-                            new_value['choices']['value'][value_index] = (
-                                conversion_fn(value))
+                            subtitled_html_new_value_dicts[value_index] = (
+                                conversion_fn(value)
+                            )
             elif (change.property_name ==
                   exp_domain.STATE_PROPERTY_WRITTEN_TRANSLATIONS):
                 # Here we use cast because this 'elif' condition forces change