Domain object changes for topic, skill, question models #6989

ankita240796 · 2019-06-22T11:15:13Z

Explanation

This PR introduces changes to topic_domain, skill_domain and question_domain by updating change domain objects according to change_domain.BaseChange. It also adds html validation in subtitled html fields and updates test according to the validation. PRs for topic, skill and question models will be stacked on this PR.

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 PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
The PR follows the style guide.
The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue. Do not only request the review but also add the reviewer as an assignee.

codecov · 2019-06-22T15:39:03Z

Codecov Report

Merging #6989 into develop will increase coverage by 0.06%.
The diff coverage is 99.81%.

@@             Coverage Diff             @@
##           develop    #6989      +/-   ##
===========================================
+ Coverage    96.35%   96.41%   +0.06%     
===========================================
  Files          374      374              
  Lines        56070    56381     +311     
===========================================
+ Hits         54024    54361     +337     
+ Misses        2046     2020      -26

Impacted Files	Coverage Δ
core/controllers/editor_test.py	`100% <ø> (ø)`	⬆️
core/controllers/subtopic_viewer_test.py	`100% <ø> (ø)`	⬆️
core/domain/subtopic_page_services_test.py	`100% <ø> (ø)`	⬆️
core/domain/skill_services_test.py	`98.72% <ø> (ø)`	⬆️
core/domain/story_domain.py	`90.52% <ø> (ø)`	⬆️
core/domain/stats_jobs_continuous_test.py	`100% <ø> (ø)`	⬆️
core/controllers/concept_card_viewer_test.py	`100% <ø> (ø)`	⬆️
core/controllers/learner_dashboard_test.py	`100% <ø> (ø)`	⬆️
core/domain/suggestion_services_test.py	`100% <ø> (ø)`	⬆️
core/controllers/suggestion_test.py	`100% <ø> (ø)`	⬆️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f064c9a...4b7f9b0. Read the comment docs.

codecov · 2019-06-22T15:39:03Z

Codecov Report

Merging #6989 into develop will increase coverage by 0.03%.
The diff coverage is 99.45%.

@@             Coverage Diff             @@
##           develop    #6989      +/-   ##
===========================================
+ Coverage    96.71%   96.75%   +0.03%     
===========================================
  Files          374      374              
  Lines        57074    57458     +384     
===========================================
+ Hits         55201    55591     +390     
+ Misses        1873     1867       -6

Impacted Files	Coverage Δ
core/domain/subtopic_page_services_test.py	`100% <ø> (ø)`	⬆️
core/domain/stats_jobs_continuous_test.py	`100% <ø> (ø)`	⬆️
core/controllers/topic_editor_test.py	`100% <ø> (ø)`	⬆️
core/domain/suggestion_services_test.py	`100% <ø> (ø)`	⬆️
core/controllers/editor_test.py	`100% <ø> (ø)`	⬆️
core/controllers/subtopic_viewer_test.py	`100% <ø> (ø)`	⬆️
core/domain/html_validation_service_test.py	`100% <ø> (ø)`	⬆️
core/tests/test_utils.py	`95.72% <ø> (ø)`	⬆️
core/controllers/concept_card_viewer_test.py	`100% <ø> (ø)`	⬆️
core/controllers/learner_dashboard_test.py	`100% <ø> (ø)`	⬆️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1d8e53...743bdfb. Read the comment docs.

seanlip · 2019-06-22T18:58:27Z

core/domain/exp_domain_test.py

@@ -37,6 +37,14 @@ def mock_get_filename_with_dimensions(filename, unused_exp_id):
        filename, 490, 120)


+def mock_validate_rte_format(unused_html_list, unused_rte_format):


Any reason we use mocks rather than actually fixing the yaml?

We will not be able to test the loading from v26 which is text angular since it will not pass the validation for ckeditor.

Ah ok. But then I think you should add a comment or something stating that, it's not obvious. You can do that within these methods, something like "# This mock should only be used for validating textangular content. (The current validation assumes CKEditor formatting, so that content would not pass the current validation.)"

seanlip · 2019-06-22T18:59:13Z

core/domain/exp_jobs_one_off_test.py

@@ -41,6 +43,18 @@
 taskqueue_services = models.Registry.import_taskqueue_services()


+def mock_validate_rte_format(unused_html_list, unused_rte_format):


Ditto -- mocking validate() is a bit weird. It suggests that the validation is too strict? Or do we just need to fix our test data?

We are validating invalid HTML/customization args. The jobs are written to find these invalid HTML/customizations args. If we don't use mocks, the validation will fail during creation and saving of an exploration and the jobs cannot be tested in that case.

I see, OK. But you need to add comments about this to explain, it's not obvious. Do so above line 1793 and elsewhere.

DubeySandeep · 2019-06-22T19:27:11Z

@ankita240796, are we expecting multiple assignees to review this PR at the same time? I think one should be the primary reviewer (maybe the mentor) and they should take care of the overall structure of the PR and once they approve you can assign other code-owners to review the PR for the codeowned files. Will it be fine?

ankita240796 · 2019-06-24T13:54:36Z

Hi @seanlip, I have addressed the two comments by doing further investigation. PTAL, Thanks!

seanlip · 2019-06-25T04:44:26Z

core/domain/exp_domain_test.py

+# This function should be only be used while loading v26 textangular
+# exploration. If we do not use a mock there, the loading will
+# not pass the validation, since the current html validation
+# assumes CKEditor formatting.
 def mock_validate_rte_format(unused_html_list, unused_rte_format):


Can you change the name to mock_validate_rte_format_for_v26?

seanlip · 2019-06-25T04:44:36Z

core/domain/exp_domain_test.py

+# This function should be only be used while loading v27 exploration
+# without image caption. If we do not use a mock there, the loading will
+# not pass the validation, since the current html validation
+# requires image tags to have a caption attribute.
 def mock_validate_customization_args(unused_html_list):


Can you change the name to mock_validate_customization_args_for_v27?

seanlip · 2019-06-25T04:47:35Z

core/domain/prod_validation_jobs_one_off.py

@@ -425,6 +438,16 @@ def _validate_commit_cmds_schema(cls, item):
            item: ndb.Model. Entity to validate.
        """
        change_domain_object = cls._get_change_domain_class(item)
+        if change_domain_object is None:
+            # This is for cases where id of entity is invalid
+            # and no commtit command domain object is found for entity.


Spelling: commit not commtit

entity --> the entity

seanlip · 2019-06-25T04:51:52Z

core/domain/suggestion_registry_test.py

@@ -759,8 +759,9 @@ def test_validate_change_question_state_data_schema_version(self):

        suggestion.validate()

-        suggestion.change.question_dict[
-            'question_state_data_schema_version'] = 0
+        question_dict = suggestion.change.question_dict


Let's leave it, but put a comment in the code that links to this (excellent) analysis, and file an issue to investigate further and figure out what the fix is. It might be worth filing an issue on the pylint repo as well, since it's possible that this is an error on their end. Thank you for following up on this!

seanlip

Basically LGTM, just a few more small things. Thanks @ankita240796!

seanlip · 2019-06-25T04:52:24Z

core/domain/skill_domain_test.py

@@ -359,36 +368,6 @@ def test_misconception_id_range(self):
        self._assert_validation_error(
            'The misconception with id 5 is out of bounds')

-    def test_cannot_create_skill_change_class_with_invalid_change_list(self):


Just to check, why are we deleting this and the following test functions?

This is already being tested in skill_domain_test.SkillChangeTests

seanlip · 2019-06-25T04:52:53Z

core/domain/skill_services_test.py

@@ -555,14 +548,6 @@ def test_update_skill_worked_examples(self):
            skill.skill_contents.worked_examples[0].to_dict(),
            new_worked_examples)

-    def test_cannot_update_skill_contents_property_with_invalid_change_dict(


Why delete?

seanlip · 2019-06-25T04:56:33Z

@AllanYangZhou @nithusha21 @DubeySandeep @aks681 @bansalnitish @apb7 If you could, PTAL and do codeowner reviews on this PR ASAP, since it is blocking three others. Thanks!

bansalnitish

LGTM, Thanks @ankita240796!

nithusha21

LGTM as a codeowner!

nithusha21 · 2019-06-25T08:18:14Z

extensions/rich_text_components/components.py

@@ -106,7 +106,7 @@ class Image(BaseRteComponent):
    def validate(cls, value_dict):
        """Validates Image component."""
        super(Image, cls).validate(value_dict)
-        filename_re = r'^[A-Za-z0-9+/]*\.((png)|(jpeg)|(gif)|(jpg))$'
+        filename_re = r'^[A-Za-z0-9+/_-]*\.((png)|(jpeg)|(gif)|(jpg))$'


/cc @import-keshav this change should fix the issue you mentioned about src validation right?

yeah right.

seanlip · 2019-06-25T08:55:19Z

This is blocking some other PRs and has been reviewed quite thoroughly, so I am going to go ahead and merge this.

Domain object changes for topic, skill, ques models

b03f27e

ankita240796 requested review from aks681, AllanYangZhou, apb7, bansalnitish, brianrodri, DubeySandeep and nithusha21 as code owners June 22, 2019 11:15

ankita240796 requested a review from seanlip June 22, 2019 11:15

ankita240796 assigned brianrodri, AllanYangZhou, seanlip, nithusha21, DubeySandeep, aks681, bansalnitish and apb7 Jun 22, 2019

ankita240796 added the CHANGELOG: Infrastructural changes label Jun 22, 2019

This was referenced Jun 22, 2019

Audit jobs and tests for topic models #6990

Merged

Audit jobs and tests for skill models #6991

Merged

Audit jobs and tests for question models #6992

Merged

ankita240796 added 4 commits June 22, 2019 17:54

Fix test

551c515

Merge remote-tracking branch 'upstream/develop' into domain-fix

38215f3

Ensure strings convertible to ckeditor dont fail validation

d65feea

Merge remote-tracking branch 'upstream/develop' into domain-fix

4b7f9b0

Fix lint

a111176

seanlip reviewed Jun 22, 2019

View reviewed changes

ankita240796 assigned seanlip Jun 24, 2019

ankita240796 added 4 commits June 24, 2019 20:32

Fix test

b94fdf4

Add test for all commit log models

9de9349

Update related model to external model

ed10917

Fix check

15b1536

ankita240796 mentioned this pull request Jun 24, 2019

Audit jobs and tests for classifier, jobs, feedback and suggestion models #6995

Merged

8 tasks

seanlip reviewed Jun 25, 2019

View reviewed changes

seanlip approved these changes Jun 25, 2019

View reviewed changes

seanlip assigned AllanYangZhou, nithusha21, ankita240796, DubeySandeep and apb7 and unassigned seanlip Jun 25, 2019

This was referenced Jun 25, 2019

False positive: unsupported-assignment-operation pylint-dev/pylint#2977

Open

[Blocked by PyCQA/pylint#2977] unsupported-assignment-operation error in suggestion_registry_test #7008

Open

ankita240796 added 3 commits June 25, 2019 10:55

Address review comments

8621cf1

Merge remote-tracking branch 'upstream/develop' into domain-fix

5bc5924

entity -> the entity

743bdfb

ankita240796 removed their assignment Jun 25, 2019

bansalnitish approved these changes Jun 25, 2019

View reviewed changes

bansalnitish removed their assignment Jun 25, 2019

nithusha21 approved these changes Jun 25, 2019

View reviewed changes

nithusha21 removed their assignment Jun 25, 2019

seanlip merged commit 02e5a7e into oppia:develop Jun 25, 2019

ankita240796 deleted the domain-fix branch June 25, 2019 18:37

bansalnitish mentioned this pull request Jul 23, 2019

Fix part of #6550: Write tests for domain/exp_services and domain/exp_domain #6844

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Domain object changes for topic, skill, question models #6989

Domain object changes for topic, skill, question models #6989

ankita240796 commented Jun 22, 2019 •

edited

Loading

codecov bot commented Jun 22, 2019

codecov bot commented Jun 22, 2019 •

edited

Loading

seanlip Jun 22, 2019

ankita240796 Jun 22, 2019

seanlip Jun 23, 2019

ankita240796 Jun 23, 2019

seanlip Jun 22, 2019

ankita240796 Jun 22, 2019 •

edited

Loading

seanlip Jun 23, 2019

ankita240796 Jun 23, 2019

DubeySandeep commented Jun 22, 2019

ankita240796 commented Jun 24, 2019

seanlip Jun 25, 2019

ankita240796 Jun 25, 2019

seanlip Jun 25, 2019

ankita240796 Jun 25, 2019

seanlip Jun 25, 2019

ankita240796 Jun 25, 2019

seanlip Jun 25, 2019

seanlip left a comment

seanlip Jun 25, 2019

ankita240796 Jun 25, 2019

seanlip Jun 25, 2019

ankita240796 Jun 25, 2019

seanlip commented Jun 25, 2019

bansalnitish left a comment

nithusha21 left a comment

nithusha21 Jun 25, 2019

import-keshav Jun 25, 2019

seanlip commented Jun 25, 2019

		@@ -37,6 +37,14 @@ def mock_get_filename_with_dimensions(filename, unused_exp_id):
		filename, 490, 120)


		def mock_validate_rte_format(unused_html_list, unused_rte_format):

		@@ -41,6 +43,18 @@
		taskqueue_services = models.Registry.import_taskqueue_services()


		def mock_validate_rte_format(unused_html_list, unused_rte_format):

Domain object changes for topic, skill, question models #6989

Domain object changes for topic, skill, question models #6989

Conversation

ankita240796 commented Jun 22, 2019 • edited Loading

Explanation

Checklist

codecov bot commented Jun 22, 2019

Codecov Report

codecov bot commented Jun 22, 2019 • edited Loading

Codecov Report

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

ankita240796 Jun 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DubeySandeep commented Jun 22, 2019

ankita240796 commented Jun 24, 2019

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

seanlip 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

seanlip commented Jun 25, 2019

bansalnitish left a comment

Choose a reason for hiding this comment

nithusha21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Jun 25, 2019

ankita240796 commented Jun 22, 2019 •

edited

Loading

codecov bot commented Jun 22, 2019 •

edited

Loading

ankita240796 Jun 22, 2019 •

edited

Loading