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

chiragbaid7 · 2021-10-14T19:29:01Z

Overview

This PR fixes or fixes part of Write schemas for handler class arguments #13162.
This PR adds schema validation for suggestion handler.
This PR introduces new approach for handling images for suggestion translation.

Essential Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The linter/Karma presubmit checks have passed locally on your machine.
"Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
- This lets reviewers restart your CircleCI tests for you.
The PR is made from a branch that's not called "develop".

Proof that changes are correct

suggestion-handler.mp4

Suggestion.images.mp4

Translate.images.suggestion.mp4

PR Pointers

Make sure to follow the instructions for making a code change.
Oppiabot will notify you when you don't add a PR_CHANGELOG label. If you are unable to do so, please @-mention a code owner (who will be in the Reviewers list), or ask on Gitter.
For what code owners will expect, see the Code Owner's wiki page.
Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays.
Never force push. If you do, your PR will be closed.
Oppiabot can assign anyone for review/help if you leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL"
Some of the e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.

* change is an object of diff domain types

oppiabot · 2021-10-14T19:29:04Z

Hi @DubeySandeep, could you please add the appropriate changelog label to this pull request? Thanks!

Nik-09

@chiragbaid7 Left one question for clarification.
Thank you

core/controllers/domain_objects_validator.py

vojtechjelinek

Thanks! the overall design looks good. Left a few comments.

vojtechjelinek · 2021-10-16T19:18:42Z

core/controllers/domain_objects_validator.py

 from core.domain import state_domain

 from typing import Dict, Optional, Union


+def validate_change(obj):


Suggested change

def validate_change(obj):

def validate_exploration_or_question_change(obj):

vojtechjelinek · 2021-10-16T19:19:49Z

core/controllers/domain_objects_validator.py

 from core.domain import state_domain

 from typing import Dict, Optional, Union


+def validate_change(obj):
+    """Validates Exploration and Question domain change.


Suggested change

"""Validates Exploration and Question domain change.

"""Validates Exploration or Question change.

vojtechjelinek · 2021-10-16T19:20:30Z

core/controllers/domain_objects_validator.py

+        exp_domain.ExplorationChange(obj)
+    elif obj['cmd'] in feconf.LIST_CMD_QUESTION_CHANGE:
+        question_domain.QuestionSuggestionChange(obj)


Should we call validate explicitly?

Are you talking about validation method in the schema for change param?

Nevermind. Can you just add a comment that the change is validated when the object is created?

vojtechjelinek · 2021-10-16T19:20:47Z

core/controllers/suggestion.py

+            'target_version_at_submission':
+            {


Suggested change

'target_version_at_submission':

{

'target_version_at_submission': {

core/controllers/suggestion.py

vojtechjelinek · 2021-10-16T19:22:06Z

core/controllers/suggestion.py

        except utils.ValidationError as e:
            raise self.InvalidInputException(e)


Can this still happen?

Yes this can be removed as validation is being performed,

vojtechjelinek · 2021-10-16T19:22:26Z

core/feconf.py

+# Constants defining various domain commands.
+CMD_ADD_WRITTEN_TRANSLATION = 'add_written_translation'
+CMD_EDIT_STATE_PROPERTY = 'edit_state_property'
+CMD_CREATE_NEW_FULLY_SPECIFIED_QUESTION = 'create_new_fully_specified_question'
+
+# Possible allowed commands for exploration change.
+LIST_CMD_EXPLORATION_CHANGE = [
+    CMD_ADD_WRITTEN_TRANSLATION,
+    CMD_EDIT_STATE_PROPERTY
+]
+
+# Possible allowed commands for question change.
+LIST_CMD_QUESTION_CHANGE = [
+    CMD_CREATE_NEW_FULLY_SPECIFIED_QUESTION
+]


Why are we redefining these?

The above constants are to check which domain the passed change object's parameter cmd belongs,
The object value cmd is then compared with possibles commands in the list and then corresponding domain object is called accordingly.

if obj['cmd'] in feconf.LIST_CMD_EXPLORATION_CHANGE: exp_domain.ExplorationChange(obj) elif obj['cmd'] in feconf.LIST_CMD_QUESTION_CHANGE: question_domain.QuestionSuggestionChange(obj)

https://github.com/chiragbaid7/oppia/blob/a18acf4a359546845c107093bb9change717f818a9d978/core/controllers/domain_objects_validator.py#L44

Yeah, but these commands are already defined in another file, we shouldn't redefine them, we should import them one way or the other.

OK, I have created a list of possible allowed commands in question_domain and exp_domain and removed the redefined constants from feconf.

…iragbaid7/oppia into add-schema-suggestion-handler

chiragbaid7 · 2021-12-10T14:04:55Z

PTAL @seanlip

oppiabot · 2021-12-10T14:07:59Z

Unassigning @chiragbaid7 since a re-review was requested. @chiragbaid7, please make sure you have addressed all review comments. Thanks!

seanlip

LGTM for my codeowner file. Thanks @chiragbaid7!

seanlip · 2021-12-11T07:21:00Z

@DubeySandeep PTAL

oppiabot · 2021-12-13T07:06:54Z

Hi @chiragbaid7, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

DubeySandeep

LGTM for the changes in core/domain/image_validation_services.py

oppiabot · 2021-12-13T16:40:34Z

Unassigning @DubeySandeep since they have already approved the PR.

…ving multiple images in suggestion handler. (oppia#14063)" This reverts commit e400be5.

seanlip · 2022-03-10T05:15:21Z

Heads-up -- it looks like this PR inadvertently broke a key workflow in the contributor dashboard. See #15101.

@chiragbaid7 @vojtechjelinek would it be possible to make a fix quickly? We will need to hotfix this.

Thanks!

/cc @sagangwee @kevintab95

vojtechjelinek · 2022-03-11T20:09:59Z

@chiragbaid7 Is working on the fix.

Chirag Baid added 8 commits October 14, 2021 23:48

Add schema validation for suggestion handler

601bb98

* change is an object of diff domain types

Add cmd constants of diffetent domain types

e662578

Add validate_change method

be889c1

Remove suggestion handler name

2e07409

Add skill_difficulty attribute

f7ea2f5

Add changes after Linter checks

0d898ce

Add changes after Linter checks

09e2a69

Remove Trailing whitespace

96bc10e

chiragbaid7 requested review from aks681, DubeySandeep, nithusha21, seanlip and vojtechjelinek as code owners October 14, 2021 19:29

oppiabot bot added the REVIEWERS: Please add changelog label label Oct 14, 2021

oppiabot bot assigned DubeySandeep Oct 14, 2021

Chirag Baid and others added 4 commits October 15, 2021 01:06

Remove quotes from a string

6ff7c73

Remove skill_difficulty attribute

eb9a4f1

Add QuestionSuggestionChange class in validate_change method

901a1ae

Merge branch 'oppia:develop' into add-schema-suggestion-handler

d8584bc

vojtechjelinek added PR CHANGELOG: Miscellaneous -- @vojtechjelinek and removed REVIEWERS: Please add changelog label labels Oct 15, 2021

vojtechjelinek assigned Nik-09 and unassigned DubeySandeep Oct 15, 2021

Merge branch 'oppia:develop' into add-schema-suggestion-handler

092e575

Nik-09 reviewed Oct 16, 2021

View reviewed changes

core/controllers/domain_objects_validator.py Show resolved Hide resolved

Nik-09 assigned chiragbaid7 and unassigned Nik-09 Oct 16, 2021

vojtechjelinek requested changes Oct 16, 2021

View reviewed changes

seanlip assigned chiragbaid7 and unassigned seanlip Dec 10, 2021

Chirag Baid added 2 commits December 10, 2021 19:11

Address review comments

06630ee

Merge branch 'add-schema-suggestion-handler' of https://github.com/ch…

c13841a

…iragbaid7/oppia into add-schema-suggestion-handler

oppiabot bot assigned seanlip and unassigned chiragbaid7 Dec 10, 2021

Update image_validation_services_test.py

9a95622

seanlip approved these changes Dec 11, 2021

View reviewed changes

seanlip removed their assignment Dec 11, 2021

oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Dec 13, 2021

Merge branch 'oppia:develop' into add-schema-suggestion-handler

31ee29f

oppiabot bot removed the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Dec 13, 2021

DubeySandeep approved these changes Dec 13, 2021

View reviewed changes

oppiabot bot unassigned DubeySandeep Dec 13, 2021

vojtechjelinek enabled auto-merge (squash) December 15, 2021 09:43

vojtechjelinek self-assigned this Dec 15, 2021

vojtechjelinek merged commit e400be5 into oppia:develop Dec 15, 2021

vojtechjelinek mentioned this pull request Dec 15, 2021

Fix part of #13162: Write schemas for some handlers #14337

Closed

4 tasks

chiragbaid7 deleted the add-schema-suggestion-handler branch December 17, 2021 14:03

vojtechjelinek added a commit to vojtechjelinek/oppia that referenced this pull request Dec 21, 2021

Revert "Fix oppia#13162: Add schema and introduce new approach for sa…

bd8380c

…ving multiple images in suggestion handler. (oppia#14063)" This reverts commit e400be5.

chiragbaid7 mentioned this pull request Dec 30, 2021

[CD] Allow filtering translation suggestions by topic (and possibly chapter name) in reviewer dashboard #14355

Closed

seanlip assigned chiragbaid7 Mar 10, 2022

soumyo123-prog mentioned this pull request Apr 21, 2022

Fix part of #13822: Validation for rubric explanations #15173

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chiragbaid7 commented Oct 14, 2021 •

edited

oppiabot bot commented Oct 14, 2021

Nik-09 left a comment

vojtechjelinek left a comment

vojtechjelinek Oct 16, 2021

chiragbaid7 Oct 17, 2021

vojtechjelinek Oct 16, 2021

chiragbaid7 Oct 17, 2021

vojtechjelinek Oct 16, 2021

chiragbaid7 Oct 17, 2021

vojtechjelinek Oct 17, 2021

chiragbaid7 Oct 19, 2021

vojtechjelinek Oct 16, 2021

chiragbaid7 Oct 17, 2021

vojtechjelinek Oct 16, 2021

chiragbaid7 Oct 17, 2021

vojtechjelinek Oct 16, 2021

chiragbaid7 Oct 17, 2021

vojtechjelinek Oct 17, 2021

chiragbaid7 Oct 19, 2021 •

edited

chiragbaid7 commented Dec 10, 2021

oppiabot bot commented Dec 10, 2021

seanlip left a comment

seanlip commented Dec 11, 2021

oppiabot bot commented Dec 13, 2021

DubeySandeep left a comment

oppiabot bot commented Dec 13, 2021

seanlip commented Mar 10, 2022

vojtechjelinek commented Mar 11, 2022

	def validate_change(obj):
	def validate_exploration_or_question_change(obj):

	"""Validates Exploration and Question domain change.
	"""Validates Exploration or Question change.

	'target_version_at_submission':
	{
	'target_version_at_submission': {

		except utils.ValidationError as e:
		raise self.InvalidInputException(e)

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

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

Conversation

chiragbaid7 commented Oct 14, 2021 • edited

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

oppiabot bot commented Oct 14, 2021

Nik-09 left a comment

Choose a reason for hiding this comment

vojtechjelinek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiragbaid7 Oct 19, 2021 • edited

Choose a reason for hiding this comment

chiragbaid7 commented Dec 10, 2021

oppiabot bot commented Dec 10, 2021

seanlip left a comment

Choose a reason for hiding this comment

seanlip commented Dec 11, 2021

oppiabot bot commented Dec 13, 2021

DubeySandeep left a comment

Choose a reason for hiding this comment

oppiabot bot commented Dec 13, 2021

seanlip commented Mar 10, 2022

vojtechjelinek commented Mar 11, 2022

chiragbaid7 commented Oct 14, 2021 •

edited

chiragbaid7 Oct 19, 2021 •

edited