Fix #13162: Add schema and introduce new approach for saving multiple images in suggestion handler.

core/controllers/domain_objects_validator.py

@@ -26,11 +26,40 @@
 from core.domain import collection_domain
 from core.domain import config_domain
 from core.domain import exp_domain
+from core.domain import image_validation_services
+from core.domain import question_domain
 from core.domain import state_domain
 
 from typing import Dict, Optional, Union
 
 
+def validate_exploration_or_question_change(obj):
+    """Validates Exploration or Question change.
+
+    Args:
+        obj: dict. Data that needs to be validated.
+    """
+    # No explicit call to validate_dict is required, because
+    # ExplorationChange or QuestionSuggestionChange calls
+    # validate method while initialization.
+    if obj.get('cmd') is None:
+        raise base.BaseHandler.InvalidInputException(
+            'Missing cmd key in change dict')
+    else:
+        exp_change_commands = [command['name'] for command in
+            exp_domain.ExplorationChange.ALLOWED_COMMANDS]
+        question_change_commands = [command['name'] for command in
+            question_domain.QuestionChange.ALLOWED_COMMANDS]
+
+        if obj['cmd'] in exp_change_commands:
+            exp_domain.ExplorationChange(obj)
+        elif obj['cmd'] in question_change_commands:
+            question_domain.QuestionSuggestionChange(obj)
+        else:
+            raise base.BaseHandler.InvalidInputException(
+                '%s cmd is not allowed.' % obj['cmd'])
+
+
 def validate_exploration_change(obj):
     """Validates exploration change.
 
@@ -180,6 +209,17 @@ def validate_aggregated_stats(aggregated_stats):
                     'stats dict.' % (state_stats_property, state_name))
 
 
+def validate_suggestion_images(files):
+    """Validates the files dict.
+
+    Args:
+        files: dict. Data that needs to be validated.
+    """
+    for filename, raw_image in files.items():
+        image_validation_services.validate_image_and_filename(
+            raw_image, filename)
+
+
 def validate_params_dict(params):
     """validates params data type
 

core/controllers/domain_objects_validator_test.py

@@ -16,6 +16,10 @@
 
 from __future__ import annotations
 
+import os
+
+from core import feconf
+from core import python_utils
 from core import utils
 from core.controllers import domain_objects_validator
 from core.tests import test_utils
@@ -46,6 +50,53 @@ def test_correct_object_do_not_raises_exception(self) -> None:
             correct_change_dict)
 
 
+class ValidateExplorationOrQuestionChangeTests(test_utils.GenericTestBase):
+    """Tests to validate domain objects coming from frontend."""
+
+    def test_incorrect_exp_domain_object_raises_exception(self) -> None:
+        incorrect_change_dict = {
+            'state_name': 'State 3',
+            'content_id': 'content',
+            'language_code': 'hi',
+            'content_html': '<p>old content html</p>',
+            'translation_html': '<p>In Hindi</p>',
+            'data_format': 'html'
+        }
+        with self.assertRaisesRegexp( # type: ignore[no-untyped-call]
+            Exception, 'Missing cmd key in change dict'
+        ):
+            domain_objects_validator.validate_exploration_or_question_change(
+                incorrect_change_dict)
+
+        incorrect_change_dict = {
+            'cmd': 'add_subtopic',
+            'state_name': 'State 3',
+            'content_id': 'content',
+            'language_code': 'hi',
+            'content_html': '<p>old content html</p>',
+            'translation_html': '<p>In Hindi</p>',
+            'data_format': 'html'
+        }
+        with self.assertRaisesRegexp( # type: ignore[no-untyped-call]
+            Exception, '%s cmd is not allowed.' % incorrect_change_dict['cmd']
+        ):
+            domain_objects_validator.validate_exploration_or_question_change(
+                incorrect_change_dict)
+
+    def test_correct_exp_domain_object_do_not_raises_exception(self) -> None:
+        correct_change_dict = {
+            'cmd': 'add_written_translation',
+            'state_name': 'State 3',
+            'content_id': 'content',
+            'language_code': 'hi',
+            'content_html': '<p>old content html</p>',
+            'translation_html': '<p>In Hindi</p>',
+            'data_format': 'html'
+        }
+        domain_objects_validator.validate_exploration_or_question_change(
+            correct_change_dict)
+
+
 class ValidateCollectionChangeTests(test_utils.GenericTestBase):
     """Tests to validate domain objects coming from API."""
 
@@ -225,6 +276,27 @@ def test_invalid_object_raises_exception(self) -> None:
             domain_objects_validator.validate_state_dict(invalid_state_dict)
 
 
+class ValidateSuggestionImagesTests(test_utils.GenericTestBase):
+    """Tests to validate suggestion images coming from frontend."""
+
+    def test_invalid_images_raises_exception(self) -> None:
+        files = {'file.svg': None}
+        with self.assertRaisesRegexp( # type: ignore[no-untyped-call]
+            Exception, 'No image supplied'
+        ):
+            domain_objects_validator.validate_suggestion_images(files)
+
+    def test_valid_images_do_not_raises_exception(self) -> None:
+        files = {'img.png': None, 'test2_svg.svg': None}
+        for filename in files:
+            with python_utils.open_file(
+                os.path.join(feconf.TESTS_DATA_DIR, filename), 'rb',
+                encoding=None
+            ) as f:
+                files[filename] = f.read()
+        domain_objects_validator.validate_suggestion_images(files)
+
+
 class ValidateParamsDict(test_utils.GenericTestBase):
     """Tests to validate the data type of params"""
 

core/controllers/suggestion.py

@@ -18,13 +18,15 @@
 
 from __future__ import annotations
 
+import base64
 import logging
 
 from core import feconf
 from core import utils
 from core.constants import constants
 from core.controllers import acl_decorators
 from core.controllers import base
+from core.controllers import domain_objects_validator
 from core.domain import exp_fetchers
 from core.domain import fs_services
 from core.domain import html_cleaner
@@ -38,23 +40,77 @@
 class SuggestionHandler(base.BaseHandler):
     """"Handles operations relating to suggestions."""
 
+    URL_PATH_ARGS_SCHEMAS = {}
+    HANDLER_ARGS_SCHEMAS = {
+        'POST': {
+            'suggestion_type': {
+                'schema': {
+                    'type': 'basestring',
+                    'choices': feconf.SUGGESTION_TYPE_CHOICES
+                }
+            },
+            'target_type': {
+                'schema': {
+                    'type': 'basestring',
+                    'choices': feconf.SUGGESTION_TARGET_TYPE_CHOICES
+                }
+            },
+            'target_id': {
+                'schema': {
+                    'type': 'basestring'
+                }
+            },
+            'target_version_at_submission': {
+                'schema': {
+                    'type': 'int',
+                    'validators': [{
+                        'id': 'is_at_least',
+                        'min_value': 1
+                    }]
+                }
+            },
+            'change': {
+               'schema': {
+                   'type': 'object_dict',
+                    'validation_method': (
+                        domain_objects_validator.
+                        validate_exploration_or_question_change
+                    )
+               }
+            },
+            'description': {
+                'schema': {
+                    'type': 'basestring'
+                }
+            },
+            'files': {
+                'schema': {
+                    'type': 'object_dict',
+                    'validation_method': (
+                        domain_objects_validator.validate_suggestion_images
+                    )
+                },
+                'default_value': None
+            }
+        }
+    }
+
     @acl_decorators.can_suggest_changes
     def post(self):
         """Handles POST requests."""
-        if (self.payload.get('suggestion_type') ==
+        if (self.normalized_payload.get('suggestion_type') ==
                 feconf.SUGGESTION_TYPE_EDIT_STATE_CONTENT):
             raise self.InvalidInputException(
                 'Content suggestion submissions are no longer supported.')
 
-        try:
-            suggestion = suggestion_services.create_suggestion(
-                self.payload.get('suggestion_type'),
-                self.payload.get('target_type'), self.payload.get('target_id'),
-                self.payload.get('target_version_at_submission'),
-                self.user_id, self.payload.get('change'),
-                self.payload.get('description'))
-        except utils.ValidationError as e:
-            raise self.InvalidInputException(e)
+        suggestion = suggestion_services.create_suggestion(
+            self.normalized_payload.get('suggestion_type'),
+            self.normalized_payload.get('target_type'),
+            self.normalized_payload.get('target_id'),
+            self.normalized_payload.get('target_version_at_submission'),
+            self.user_id,
+            self.normalized_payload.get('change'),
+            self.normalized_payload.get('description'))
 
         suggestion_change = suggestion.change
         if (
@@ -80,7 +136,7 @@ def post(self):
         if self.payload.get(
             'suggestion_type') != (feconf.SUGGESTION_TYPE_ADD_QUESTION):
             _upload_suggestion_images(
-                self.request,
+                self.normalized_payload.get('files'),
                 suggestion,
                 suggestion.get_new_image_filenames_added_in_suggestion())
 
@@ -509,11 +565,11 @@ def _construct_exploration_suggestions(suggestions):
     return suggestion_dicts
 
 
-def _upload_suggestion_images(request, suggestion, filenames):
+def _upload_suggestion_images(files, suggestion, filenames):
     """Saves a suggestion's images to storage.
 
     Args:
-        request: webapp2.Request. Request object containing a mapping of image
+        files: dict. Files containing a mapping of image
             filename to image blob.
         suggestion: BaseSuggestion. The suggestion for which images are being
             uploaded.
@@ -523,7 +579,7 @@ def _upload_suggestion_images(request, suggestion, filenames):
     # TODO(#10513): Find a way to save the images before the suggestion is
     # created.
     for filename in filenames:
-        image = request.get(filename)
+        image = files.get(filename) if files else None
         if not image:
             logging.exception(
                 'Image not provided for file with name %s when the '
@@ -532,12 +588,12 @@ def _upload_suggestion_images(request, suggestion, filenames):
             raise base.BaseHandler.InvalidInputException(
                 'No image data provided for file with name %s.'
                 % (filename))
-        try:
-            file_format = (
-                image_validation_services.validate_image_and_filename(
-                    image, filename))
-        except utils.ValidationError as e:
-            raise base.BaseHandler.InvalidInputException('%s' % (e))
+        # TODO(#14204): Refactor this to use the normalized_payload files.
+        if utils.is_base64_encoded(image):
+            image = base64.decodebytes(image.encode('utf-8'))
+        file_format = (
+            image_validation_services.validate_image_and_filename(
+                image, filename))
         image_is_compressible = (
             file_format in feconf.COMPRESSIBLE_IMAGE_FORMATS)
         fs_services.save_original_and_compressed_versions_of_image(

core/controllers/suggestion_test.py

@@ -18,6 +18,7 @@
 
 from __future__ import annotations
 
+import base64
 import os
 
 from core import feconf
@@ -741,10 +742,14 @@ def test_translation_suggestion_creation_with_new_images(self):
                         '</oppia-noninteractive-image>'),
                     'data_format': 'html'
                 },
-            }, csrf_token=csrf_token,
-            upload_files=(
-                ('translation_image.png', 'translation_image.png', raw_image), )
-            )
+                'description': 'test',
+                'files': {
+                    'translation_image.png': (
+                        base64.b64encode(raw_image).decode('utf-8'))
+                 },
+            },
+            csrf_token=csrf_token
+        )
 
         fs = fs_domain.AbstractFileSystem(
             fs_domain.GcsFileSystem(
@@ -981,7 +986,8 @@ def test_update_suggestion_updates_question_suggestion_content(self):
             suggestion.suggestion_id), {
                 'question_state_data': question_state_data,
                 'skill_difficulty': 0.6
-            }, csrf_token=csrf_token, upload_files=(
+            },
+            csrf_token=csrf_token, upload_files=(
                 ('img.png', 'img.png', raw_image),))
 
         updated_suggestion = suggestion_services.get_suggestion_by_id(
@@ -1330,6 +1336,7 @@ def test_suggestion_creation_when_images_are_not_provided(self):
                     'translation_html': valid_html,
                     'data_format': 'html'
                 },
+                'description': 'test'
             }, csrf_token=csrf_token, expected_status_int=400)
 
         self.assertIn(
@@ -1405,10 +1412,10 @@ def test_suggestion_creation_when_images_are_not_valid(self):
                     'translation_html': valid_html,
                     'data_format': 'html'
                 },
-            }, csrf_token=csrf_token,
-            upload_files=(
-                 ('file.svg', 'file.svg', large_image),),
-            expected_status_int=400)
+                'description': 'test',
+                'files': {'file.svg': large_image},
+            }, csrf_token=csrf_token, expected_status_int=400,
+        )
 
         self.assertIn(
             'Image exceeds file size limit of 100 KB.',
@@ -1742,8 +1749,8 @@ def test_create_suggestion_invalid_target_version_input(self):
 
         self.assertEqual(
             response['error'],
-            'Expected target_version_at_submission to be an int, '
-            'received <class \'str\'>'
+            'Schema validation for \'target_version_at_submission\' failed: '
+            'Could not convert str to int: invalid_target_version'
         )
         self.assertEqual(len(suggestions), 1)
         self.logout()
@@ -1793,9 +1800,12 @@ def test_suggestion_creation_with_valid_images(self):
                     'skill_id': self.SKILL_ID,
                     'skill_difficulty': 0.3
                 },
-                'description': 'Add new question to skill'
-            }, csrf_token=csrf_token, upload_files=(
-                ('file.svg', 'file.svg', raw_image), ))
+                'description': 'Add new question to skill',
+                'files': {
+                    'file.svg': (
+                        base64.b64encode(raw_image).decode('utf-8'))
+                }
+            }, csrf_token=csrf_token,)
         self.logout()