Skill Explanation
'), [ + state_domain.SubtitledHtml('2', 'Example 1
'), + state_domain.SubtitledHtml('3', 'Example 2
')], {'1': {}, '2': {}, '3': {}}, state_domain.WrittenTranslations.from_dict({ 'translations_mapping': {'1': {}, '2': {}, '3': {}} @@ -55,15 +55,15 @@ def test_get_concept_card(self): json_response = self.get_json( '%s/%s' % (feconf.CONCEPT_CARD_DATA_URL_PREFIX, self.skill_id)) self.assertEqual( - 'Skill Explanation', + 'Skill Explanation
', json_response['concept_card_dict']['explanation']['html']) self.assertEqual( [{ 'content_id': '2', - 'html': 'Example 1' + 'html': 'Example 1
' }, { 'content_id': '3', - 'html': 'Example 2' + 'html': 'Example 2
' }], json_response['concept_card_dict']['worked_examples']) diff --git a/core/controllers/editor_test.py b/core/controllers/editor_test.py index 1473c157b966f..9dc2ea5aae062 100644 --- a/core/controllers/editor_test.py +++ b/core/controllers/editor_test.py @@ -1103,7 +1103,7 @@ def setUp(self): 'state_name': exploration.init_state_name, 'new_value': { 'content_id': 'content', - 'html': 'ABC' + 'html': 'ABC
' }, })], 'Change objective and init state content') diff --git a/core/controllers/feedback_test.py b/core/controllers/feedback_test.py index 6c9aeea0d5544..3696c58c37d02 100644 --- a/core/controllers/feedback_test.py +++ b/core/controllers/feedback_test.py @@ -527,7 +527,7 @@ def test_feedback_threads(self): def test_feedback_threads_with_suggestions(self): new_content = state_domain.SubtitledHtml( - 'content', 'new content html').to_dict() + 'content', 'new content html
').to_dict() change_cmd = { 'cmd': exp_domain.CMD_EDIT_STATE_PROPERTY, 'property_name': exp_domain.STATE_PROPERTY_CONTENT, @@ -598,7 +598,7 @@ def test_post_feedback_threads_with_updated_suggestion_status_raises_400( csrf_token = self.get_csrf_token_from_response(response) new_content = state_domain.SubtitledHtml( - 'content', 'new content html').to_dict() + 'content', 'new content html
').to_dict() change = { 'cmd': exp_domain.CMD_EDIT_STATE_PROPERTY, 'property_name': exp_domain.STATE_PROPERTY_CONTENT, diff --git a/core/controllers/learner_dashboard_test.py b/core/controllers/learner_dashboard_test.py index 105375f76a244..685b1cb567fcd 100644 --- a/core/controllers/learner_dashboard_test.py +++ b/core/controllers/learner_dashboard_test.py @@ -395,7 +395,7 @@ def test_get_suggestions_after_updating_suggestion_summary(self): self.assertFalse(messages_summary.get('description')) new_content = state_domain.SubtitledHtml( - 'content', 'new content html').to_dict() + 'content', 'new content html
').to_dict() change_cmd = { 'cmd': exp_domain.CMD_EDIT_STATE_PROPERTY, 'property_name': exp_domain.STATE_PROPERTY_CONTENT, @@ -424,7 +424,7 @@ def test_get_suggestions_after_updating_suggestion_summary(self): messages_summary['author_picture_data_url'].startswith( 'data:image/png;')) self.assertEqual( - messages_summary['suggestion_html'], 'new content html') + messages_summary['suggestion_html'], 'new content html
') self.assertEqual( messages_summary['current_content_html'], current_content_html) self.assertEqual( diff --git a/core/controllers/subtopic_viewer_test.py b/core/controllers/subtopic_viewer_test.py index 8bf25cd7f470d..46f5d8da16f9c 100644 --- a/core/controllers/subtopic_viewer_test.py +++ b/core/controllers/subtopic_viewer_test.py @@ -64,11 +64,11 @@ def setUp(self): 'content_ids_to_audio_translations': self.content_ids_to_audio_translations_dict, 'subtitled_html': { - 'content_id': 'content', 'html': 'hello world' + 'content_id': 'content', 'html': 'hello world
' } } self.subtopic_page.update_page_contents_html({ - 'html': 'hello world', + 'html': 'hello world
', 'content_id': 'content' }) self.subtopic_page.update_page_contents_audio( @@ -96,7 +96,7 @@ def test_get(self): self.content_ids_to_audio_translations_dict, 'subtitled_html': { 'content_id': 'content', - 'html': 'hello world' + 'html': 'hello world
' }, 'written_translations': self.written_translations_dict } diff --git a/core/controllers/suggestion_test.py b/core/controllers/suggestion_test.py index e04b6e90e56b2..e4fe98618edc1 100644 --- a/core/controllers/suggestion_test.py +++ b/core/controllers/suggestion_test.py @@ -71,7 +71,7 @@ def setUp(self): ['TextInput'], category='Algebra')) self.old_content = state_domain.SubtitledHtml( - 'content', 'old content html').to_dict() + 'content', 'old content html
').to_dict() exploration.states['State 1'].update_content(self.old_content) exploration.states['State 2'].update_content(self.old_content) @@ -84,9 +84,9 @@ def setUp(self): rights_manager.ROLE_EDITOR) self.new_content = state_domain.SubtitledHtml( - 'content', 'new content html').to_dict() + 'content', 'new content html
').to_dict() self.resubmit_change_content = state_domain.SubtitledHtml( - 'content', 'resubmit change content html').to_dict() + 'content', 'resubmit change content html
').to_dict() self.logout() @@ -318,7 +318,7 @@ def test_owner_of_exploration_cannot_repond_to_own_suggestion(self): self.save_new_default_exploration(exp_id, self.editor_id) new_content = state_domain.SubtitledHtml( - 'content', 'new content html').to_dict() + 'content', 'new content html
').to_dict() change_cmd = { 'cmd': exp_domain.CMD_EDIT_STATE_PROPERTY, 'property_name': exp_domain.STATE_PROPERTY_CONTENT, @@ -806,7 +806,7 @@ def test_suggestion_to_topic_handler_with_invalid_target_type(self): self.save_new_default_exploration(exp_id, self.admin_id) new_content = state_domain.SubtitledHtml( - 'content', 'new content html').to_dict() + 'content', 'new content html
').to_dict() change_cmd = { 'cmd': exp_domain.CMD_EDIT_STATE_PROPERTY, 'property_name': exp_domain.STATE_PROPERTY_CONTENT, diff --git a/core/controllers/topic_editor.py b/core/controllers/topic_editor.py index 165032e9de972..091bfd2669920 100644 --- a/core/controllers/topic_editor.py +++ b/core/controllers/topic_editor.py @@ -238,7 +238,8 @@ def put(self, topic_id): 'topic_and_subtopic_page_change_dicts') topic_and_subtopic_page_change_list = [] for change in topic_and_subtopic_page_change_dicts: - if change['change_affects_subtopic_page']: + if change['cmd'] == ( + subtopic_page_domain.CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY): topic_and_subtopic_page_change_list.append( subtopic_page_domain.SubtopicPageChange(change)) else: diff --git a/core/controllers/topic_editor_test.py b/core/controllers/topic_editor_test.py index d7ff51d81ee20..ac3b749307092 100644 --- a/core/controllers/topic_editor_test.py +++ b/core/controllers/topic_editor_test.py @@ -362,7 +362,6 @@ def test_editable_topic_handler_put_raises_error_with_invalid_name(self): 'version': 2, 'commit_message': 'Changed name', 'topic_and_subtopic_page_change_dicts': [{ - 'change_affects_subtopic_page': False, 'cmd': 'update_topic_property', 'property_name': 'name', 'old_value': '', @@ -387,13 +386,11 @@ def test_editable_topic_handler_put(self): 'version': 2, 'commit_message': 'Some changes and added a subtopic.', 'topic_and_subtopic_page_change_dicts': [{ - 'change_affects_subtopic_page': False, 'cmd': 'update_topic_property', 'property_name': 'name', 'old_value': '', 'new_value': 'A new name' }, { - 'change_affects_subtopic_page': True, 'cmd': 'update_subtopic_page_property', 'property_name': 'page_contents_html', 'old_value': { @@ -406,12 +403,10 @@ def test_editable_topic_handler_put(self): 'content_id': 'content' } }, { - 'change_affects_subtopic_page': False, 'cmd': 'add_subtopic', 'subtopic_id': 2, 'title': 'Title2' }, { - 'change_affects_subtopic_page': True, 'cmd': 'update_subtopic_page_property', 'property_name': 'page_contents_html', 'old_value': { @@ -424,7 +419,6 @@ def test_editable_topic_handler_put(self): }, 'subtopic_id': 2 }, { - 'change_affects_subtopic_page': True, 'cmd': 'update_subtopic_page_property', 'property_name': 'page_contents_audio', 'old_value': { @@ -558,13 +552,11 @@ def test_editable_topic_handler_put_for_assigned_topic_manager(self): 'version': 2, 'commit_message': 'Some changes and added a subtopic.', 'topic_and_subtopic_page_change_dicts': [{ - 'change_affects_subtopic_page': False, 'cmd': 'update_topic_property', 'property_name': 'name', 'old_value': '', 'new_value': 'A new name' }, { - 'change_affects_subtopic_page': True, 'cmd': 'update_subtopic_page_property', 'property_name': 'page_contents_html', 'old_value': { @@ -577,12 +569,10 @@ def test_editable_topic_handler_put_for_assigned_topic_manager(self): 'content_id': 'content' } }, { - 'change_affects_subtopic_page': False, 'cmd': 'add_subtopic', 'subtopic_id': 2, 'title': 'Title2' }, { - 'change_affects_subtopic_page': True, 'cmd': 'update_subtopic_page_property', 'property_name': 'page_contents_html', 'old_value': { @@ -595,7 +585,6 @@ def test_editable_topic_handler_put_for_assigned_topic_manager(self): }, 'subtopic_id': 2 }, { - 'change_affects_subtopic_page': True, 'cmd': 'update_subtopic_page_property', 'property_name': 'page_contents_audio', 'old_value': { diff --git a/core/domain/exp_domain_test.py b/core/domain/exp_domain_test.py index 5f036adb21f7e..c48b31b7a1a55 100644 --- a/core/domain/exp_domain_test.py +++ b/core/domain/exp_domain_test.py @@ -37,6 +37,22 @@ def mock_get_filename_with_dimensions(filename, unused_exp_id): filename, 490, 120) +# 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_for_v26(unused_html_list, unused_rte_format): + return {} + + +# 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_for_v27(unused_html_list): + return {} + + class ExplorationChangeTests(test_utils.GenericTestBase): def test_exp_change_object_with_missing_cmd(self): @@ -414,7 +430,7 @@ def test_validation(self): 'dest': exploration.init_state_name, 'feedback': { 'content_id': 'feedback_1', - 'html': 'Feedback' + 'html': 'Feedback
' }, 'labelled_as_correct': False, 'param_changes': [], @@ -4625,9 +4641,13 @@ class HTMLMigrationUnitTests(test_utils.GenericTestBase): feedback: content_id: feedback_1 html: Here is the image1Here is the image1
Here is the image2
Here is the image2
Try again
' }, 'labelled_as_correct': False, 'param_changes': [], @@ -994,11 +994,11 @@ def test_get_image_filenames_from_exploration(self): content1_dict = { 'content_id': 'content', 'html': ( - 'Hello, this is state1
' + '
Hello, this is state1' '
Hello, this is html1 for state2' + '
Hello, this is html1 for state2
' 'Outcome1 for state2
This is value1 for ItemSelection' + '
This is value1 for ItemSelection
' 'This is value3 for ItemSelection' + '
This is value3 for ItemSelection
' 'Try again
' }, 'labelled_as_correct': False, 'param_changes': [], @@ -1918,7 +1918,7 @@ def setUp(self): 'dest': self.init_state_name, 'feedback': { 'content_id': 'default_outcome', - 'html': 'Incorrect' + 'html': 'Incorrect
' }, 'labelled_as_correct': False, 'param_changes': [], @@ -2143,7 +2143,7 @@ def test_update_interaction_answer_groups(self): outcome = init_interaction.answer_groups[0].outcome self.assertEqual(rule_specs[0].rule_type, 'Equals') self.assertEqual(rule_specs[0].inputs, {'x': 0}) - self.assertEqual(outcome.feedback.html, 'Try again') + self.assertEqual(outcome.feedback.html, 'Try again
') self.assertEqual(outcome.dest, self.init_state_name) self.assertEqual(init_interaction.default_outcome.dest, 'State 2') @@ -2217,14 +2217,15 @@ def test_update_content(self): exp_services.update_exploration( self.owner_id, self.EXP_ID, _get_change_list( self.init_state_name, 'content', { - 'html': 'Test content', + 'html': 'Test content
', 'content_id': 'content', }), '') exploration = exp_services.get_exploration_by_id(self.EXP_ID) self.assertEqual( - exploration.init_state.content.html, 'Test content') + exploration.init_state.content.html, + 'Test content
') def test_update_solicit_answer_details(self): """Test updating of solicit_answer_details.""" diff --git a/core/domain/html_validation_service.py b/core/domain/html_validation_service.py index 56f0b0aada216..e766bcc96d659 100644 --- a/core/domain/html_validation_service.py +++ b/core/domain/html_validation_service.py @@ -479,6 +479,16 @@ def convert_to_ckeditor(html_data): br.insert_after('\n') br.unwrap() + # Ensure that any html string is always wrapped in a tag. + # We are doing this since in CKEditor every string should + # be wrapped in some tag. CKEditor will not produce any + # error if we have a string without any tag but it cannot + # be generated directly by typing some content in rte. + # (It may be generated by copy paste). + for content in soup.contents: + if not content.name: + content.wrap(soup.new_tag('p')) + return unicode(soup).replace('oppia '
),
'expected_output': (
- 'Lorem ipsum
Hello this is ' + 'Lorem ipsum
Hello this is
' 'oppia '
)
}, {
diff --git a/core/domain/prod_validation_jobs_one_off.py b/core/domain/prod_validation_jobs_one_off.py
index 6f6434c7118e5..25a806e5a4647 100644
--- a/core/domain/prod_validation_jobs_one_off.py
+++ b/core/domain/prod_validation_jobs_one_off.py
@@ -257,51 +257,58 @@ class BaseSummaryModelValidator(BaseModelValidator):
"""Base class for validating summary models."""
@classmethod
- def _get_related_model_properties(cls):
- """Returns a tuple of related external_models and properties.
+ def _get_external_model_properties(cls):
+ """Returns a tuple of external models and properties.
This should be implemented by subclasses.
Returns:
tuple(str, list(tuple), dict): A tuple with first element as
- related model name, second element as a tuple of
+ external model name, second element as a tuple of
cls.external_instance_details and the third element
as a properties dict with key as property name in summary
- model and value as property name in related model.
+ model and value as property name in external model.
"""
raise NotImplementedError
@classmethod
- def _validate_related_model_properties(cls, item):
+ def _validate_external_model_properties(cls, item):
"""Validate that properties of the model match the corresponding
- properties of the related model.
+ properties of the external model.
Args:
item: ndb.Model. BaseSummaryModel to validate.
"""
for (
- related_model_name,
- related_model_class_model_id_model_tuples,
- related_model_properties_dict
- ) in cls._get_related_model_properties():
-
- for (_, _, related_model) in (
- related_model_class_model_id_model_tuples):
- for (property_name, related_model_property_name) in (
- related_model_properties_dict.iteritems()):
+ external_model_name,
+ external_model_class_model_id_model_tuples,
+ external_model_properties_dict
+ ) in cls._get_external_model_properties():
+
+ for (_, _, external_model) in (
+ external_model_class_model_id_model_tuples):
+ # The case for missing external model is ignored here
+ # since errors for missing external model are already
+ # checked and stored in _validate_external_id_relationships
+ # function.
+ if external_model is None or external_model.deleted:
+ continue
+ for (property_name, external_model_property_name) in (
+ external_model_properties_dict.iteritems()):
value_in_summary_model = getattr(item, property_name)
- value_in_related_model = getattr(
- related_model, related_model_property_name)
+ value_in_external_model = getattr(
+ external_model, external_model_property_name)
- if value_in_summary_model != value_in_related_model:
+ if value_in_summary_model != value_in_external_model:
cls.errors['%s field check' % property_name].append((
'Entity id %s: %s field in entity: %s does not '
'match corresponding %s %s field: %s') % (
item.id, property_name,
value_in_summary_model,
- related_model_name, related_model_property_name,
- value_in_related_model))
+ external_model_name,
+ external_model_property_name,
+ value_in_external_model))
@classmethod
def validate(cls, item):
@@ -312,7 +319,7 @@ def validate(cls, item):
"""
super(BaseSummaryModelValidator, cls).validate(item)
- cls._validate_related_model_properties(item)
+ cls._validate_external_model_properties(item)
class BaseSnapshotContentModelValidator(BaseModelValidator):
@@ -320,16 +327,16 @@ class BaseSnapshotContentModelValidator(BaseModelValidator):
# The name of the model which is to be used in the error messages.
# This can be overridden by subclasses, if needed.
- model_name = 'snapshot content'
+ MODEL_NAME = 'snapshot content'
- # The name of the related model in lowercase which is used to obtain
- # the name of the key for the fetch of related model and the name
- # of the related model to be used in error messages.
- # For example, if related model is CollectionRights, then
- # related_model_name = collection rights, key to fetch = collection_rights
+ # The name of the external model in lowercase which is used to obtain
+ # the name of the key for the fetch of external model and the name
+ # of the external model to be used in error messages.
+ # For example, if external model is CollectionRights, then
+ # EXTERNAL_MODEL_NAME = collection rights, key to fetch = collection_rights
# Name of model to be used in error message = CollectionRights
# This should be overridden by subclasses.
- related_model_name = ''
+ EXTERNAL_MODEL_NAME = ''
@classmethod
def _get_model_id_regex(cls, unused_item):
@@ -337,40 +344,46 @@ def _get_model_id_regex(cls, unused_item):
@classmethod
def _validate_base_model_version_from_item_id(cls, item):
- """Validate that related model corresponding to item.id
+ """Validate that external model corresponding to item.id
has a version greater than or equal to the version in item.id.
Args:
item: ndb.Model. BaseSnapshotContentModel to validate.
"""
- if cls.related_model_name == '':
- raise Exception('Related model name should be specified')
+ if cls.EXTERNAL_MODEL_NAME == '':
+ raise Exception('External model name should be specified')
- related_model_name = cls.related_model_name
+ external_model_name = cls.EXTERNAL_MODEL_NAME
if item.id.startswith('rights'):
- related_model_name = related_model_name + ' rights'
+ external_model_name = external_model_name + ' rights'
- name_split_by_space = related_model_name.split(' ')
+ name_split_by_space = external_model_name.split(' ')
key_to_fetch = ('_').join(name_split_by_space)
- capitalized_related_model_name = ('').join([
+ capitalized_external_model_name = ('').join([
val.capitalize() for val in name_split_by_space])
- related_model_class_model_id_model_tuples = (
+ external_model_class_model_id_model_tuples = (
cls.external_instance_details['%s_ids' % key_to_fetch])
version = item.id[item.id.rfind('-') + 1:]
- for (_, _, related_model) in (
- related_model_class_model_id_model_tuples):
- if int(related_model.version) < int(version):
+ for (_, _, external_model) in (
+ external_model_class_model_id_model_tuples):
+ # The case for missing external model is ignored here
+ # since errors for missing external model are already
+ # checked and stored in _validate_external_id_relationships
+ # function.
+ if external_model is None or external_model.deleted:
+ continue
+ if int(external_model.version) < int(version):
cls.errors[
- '%s model version check' % cls.related_model_name].append((
+ '%s model version check' % cls.EXTERNAL_MODEL_NAME].append((
'Entity id %s: %s model corresponding to '
'id %s has a version %s which is less than '
'the version %s in %s model id' % (
- item.id, capitalized_related_model_name,
- related_model.id, related_model.version, version,
- cls.model_name)))
+ item.id, capitalized_external_model_name,
+ external_model.id, external_model.version, version,
+ cls.MODEL_NAME)))
@classmethod
def validate(cls, item):
@@ -387,7 +400,7 @@ def validate(cls, item):
class BaseSnapshotMetadataModelValidator(BaseSnapshotContentModelValidator):
"""Base class for validating snapshot metadata models."""
- model_name = 'snapshot metadata'
+ MODEL_NAME = 'snapshot metadata'
@classmethod
def _validate_commit_type(cls, item):
@@ -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 the entity is invalid
+ # and no commit command domain object is found for the entity.
+ # For example, if a CollectionCommitLogEntryModel does
+ # not have id starting with collection/rights, there is
+ # no commit command domain object defined for this model.
+ cls.errors['commit cmd check'].append(
+ 'Entity id %s: No commit command domain object defined '
+ 'for entity with commands: %s' % (item.id, item.commit_cmds))
+ return
for commit_cmd_dict in item.commit_cmds:
if not commit_cmd_dict:
continue
@@ -453,7 +476,7 @@ def validate(cls, item):
class BaseCommitLogEntryModelValidator(BaseSnapshotMetadataModelValidator):
"""Base class for validating commit log entry models."""
- model_name = 'commit log entry'
+ MODEL_NAME = 'commit log entry'
@classmethod
def _validate_post_commit_status(cls, item):
@@ -577,8 +600,8 @@ def _get_model_domain_object_instance(cls, item):
@classmethod
def _get_external_id_relationships(cls, item):
snapshot_model_ids = [
- '%s-%d' % (item.id, version) for version in range(
- 1, item.version + 1)]
+ '%s-%d' % (item.id, version)
+ for version in range(1, item.version + 1)]
return {
'exploration_ids': (
exp_models.ExplorationModel,
@@ -605,7 +628,7 @@ class CollectionSnapshotMetadataModelValidator(
BaseSnapshotMetadataModelValidator):
"""Class for validating CollectionSnapshotMetadataModel."""
- related_model_name = 'collection'
+ EXTERNAL_MODEL_NAME = 'collection'
@classmethod
def _get_change_domain_class(cls, unused_item):
@@ -626,7 +649,7 @@ class CollectionSnapshotContentModelValidator(
BaseSnapshotContentModelValidator):
"""Class for validating CollectionSnapshotContentModel."""
- related_model_name = 'collection'
+ EXTERNAL_MODEL_NAME = 'collection'
@classmethod
def _get_external_id_relationships(cls, item):
@@ -643,8 +666,8 @@ class CollectionRightsModelValidator(BaseModelValidator):
@classmethod
def _get_external_id_relationships(cls, item):
snapshot_model_ids = [
- '%s-%d' % (item.id, version) for version in range(
- 1, item.version + 1)]
+ '%s-%d' % (item.id, version)
+ for version in range(1, item.version + 1)]
return {
'collection_ids': (
collection_models.CollectionModel, [item.id]),
@@ -691,7 +714,7 @@ class CollectionRightsSnapshotMetadataModelValidator(
BaseSnapshotMetadataModelValidator):
"""Class for validating CollectionRightsSnapshotMetadataModel."""
- related_model_name = 'collection rights'
+ EXTERNAL_MODEL_NAME = 'collection rights'
@classmethod
def _get_change_domain_class(cls, unused_item):
@@ -712,7 +735,7 @@ class CollectionRightsSnapshotContentModelValidator(
BaseSnapshotContentModelValidator):
"""Class for validating CollectionRightsSnapshotContentModel."""
- related_model_name = 'collection rights'
+ EXTERNAL_MODEL_NAME = 'collection rights'
@classmethod
def _get_external_id_relationships(cls, item):
@@ -726,7 +749,7 @@ def _get_external_id_relationships(cls, item):
class CollectionCommitLogEntryModelValidator(BaseCommitLogEntryModelValidator):
"""Class for validating CollectionCommitLogEntryModel."""
- related_model_name = 'collection'
+ EXTERNAL_MODEL_NAME = 'collection'
@classmethod
def _get_model_id_regex(cls, item):
@@ -740,8 +763,12 @@ def _get_model_id_regex(cls, item):
def _get_change_domain_class(cls, item):
if item.id.startswith('rights'):
return rights_manager.CollectionRightsChange
- else:
+ elif item.id.startswith('collection'):
return collection_domain.CollectionChange
+ else:
+ # The case of invalid id is being ignored here since this
+ # case will already be checked by the id regex test.
+ return None
@classmethod
def _get_external_id_relationships(cls, item):
@@ -810,6 +837,12 @@ def _validate_node_count(cls, item):
for (_, _, collection_model) in (
collection_model_class_model_id_model_tuples):
+ # The case for missing collection external model is ignored here
+ # since errors for missing collection external model are already
+ # checked and stored in _validate_external_id_relationships
+ # function.
+ if collection_model is None or collection_model.deleted:
+ continue
nodes = collection_model.collection_contents['nodes']
if item.node_count != len(nodes):
cls.errors['node count check'].append((
@@ -830,7 +863,7 @@ def _validate_ratings_is_empty(cls, item):
'empty but received %s' % (item.id, item.ratings))
@classmethod
- def _get_related_model_properties(cls):
+ def _get_external_model_properties(cls):
collection_model_class_model_id_model_tuples = (
cls.external_instance_details['collection_ids'])
collection_rights_model_class_model_id_model_tuples = (
@@ -883,8 +916,8 @@ def _get_model_id_regex(cls, unused_item):
@classmethod
def _get_external_id_relationships(cls, item):
snapshot_model_ids = [
- '%s-%d' % (item.id, version) for version in range(
- 1, item.version + 1)]
+ '%s-%d' % (item.id, version)
+ for version in range(1, item.version + 1)]
return {
'snapshot_metadata_ids': (
config_models.ConfigPropertySnapshotMetadataModel,
@@ -899,7 +932,7 @@ class ConfigPropertySnapshotMetadataModelValidator(
BaseSnapshotMetadataModelValidator):
"""Class for validating ConfigPropertySnapshotMetadataModel."""
- related_model_name = 'config property'
+ EXTERNAL_MODEL_NAME = 'config property'
@classmethod
def _get_model_id_regex(cls, unused_item):
@@ -924,7 +957,7 @@ class ConfigPropertySnapshotContentModelValidator(
BaseSnapshotContentModelValidator):
"""Class for validating ConfigPropertySnapshotContentModel."""
- related_model_name = 'config property'
+ EXTERNAL_MODEL_NAME = 'config property'
@classmethod
def _get_model_id_regex(cls, unused_item):
@@ -984,14 +1017,19 @@ def _validate_sender_email(cls, item):
for (_, _, sender_model) in (
sender_model_class_model_id_model_tuples):
- if sender_model and not sender_model.deleted:
- if sender_model.email != item.sender_email:
- cls.errors['sender email check'].append((
- 'Entity id %s: Sender email %s in entity does not '
- 'match with email %s of user obtained through '
- 'sender id %s') % (
- item.id, item.sender_email, sender_model.email,
- item.sender_id))
+ # The case for missing sender external model is ignored here
+ # since errors for missing sender external model are already
+ # checked and stored in _validate_external_id_relationships
+ # function.
+ if sender_model is None or sender_model.deleted:
+ continue
+ if sender_model.email != item.sender_email:
+ cls.errors['sender email check'].append((
+ 'Entity id %s: Sender email %s in entity does not '
+ 'match with email %s of user obtained through '
+ 'sender id %s') % (
+ item.id, item.sender_email, sender_model.email,
+ item.sender_id))
@classmethod
def _validate_recipient_email(cls, item):
@@ -1006,14 +1044,19 @@ def _validate_recipient_email(cls, item):
for (_, _, recipient_model) in (
recipient_model_class_model_id_model_tuples):
- if recipient_model and not recipient_model.deleted:
- if recipient_model.email != item.recipient_email:
- cls.errors['recipient email check'].append((
- 'Entity id %s: Recipient email %s in entity does '
- 'not match with email %s of user obtained through '
- 'recipient id %s') % (
- item.id, item.recipient_email,
- recipient_model.email, item.recipient_id))
+ # The case for missing recipient external model is ignored here
+ # since errors for missing recipient external model are already
+ # checked and stored in _validate_external_id_relationships
+ # function.
+ if recipient_model is None or recipient_model.deleted:
+ continue
+ if recipient_model.email != item.recipient_email:
+ cls.errors['recipient email check'].append((
+ 'Entity id %s: Recipient email %s in entity does '
+ 'not match with email %s of user obtained through '
+ 'recipient id %s') % (
+ item.id, item.recipient_email,
+ recipient_model.email, item.recipient_id))
@classmethod
def _get_custom_validation_functions(cls):
@@ -1061,14 +1104,19 @@ def _validate_sender_email(cls, item):
for (_, _, sender_model) in (
sender_model_class_model_id_model_tuples):
- if sender_model and not sender_model.deleted:
- if sender_model.email != item.sender_email:
- cls.errors['sender email check'].append((
- 'Entity id %s: Sender email %s in entity does not '
- 'match with email %s of user obtained through '
- 'sender id %s') % (
- item.id, item.sender_email, sender_model.email,
- item.sender_id))
+ # The case for missing sender external model is ignored here
+ # since errors for missing sender external model are already
+ # checked and stored in _validate_external_id_relationships
+ # function.
+ if sender_model is None or sender_model.deleted:
+ continue
+ if sender_model.email != item.sender_email:
+ cls.errors['sender email check'].append((
+ 'Entity id %s: Sender email %s in entity does not '
+ 'match with email %s of user obtained through '
+ 'sender id %s') % (
+ item.id, item.sender_email, sender_model.email,
+ item.sender_id))
@classmethod
def _get_custom_validation_functions(cls):
@@ -1131,8 +1179,8 @@ def _get_model_domain_object_instance(cls, item):
@classmethod
def _get_external_id_relationships(cls, item):
snapshot_model_ids = [
- '%s-%d' % (item.id, version) for version in range(
- 1, item.version + 1)]
+ '%s-%d' % (item.id, version)
+ for version in range(1, item.version + 1)]
return {
'exploration_commit_log_entry_ids': (
exp_models.ExplorationCommitLogEntryModel,
@@ -1155,7 +1203,7 @@ class ExplorationSnapshotMetadataModelValidator(
BaseSnapshotMetadataModelValidator):
"""Class for validating ExplorationSnapshotMetadataModel."""
- related_model_name = 'exploration'
+ EXTERNAL_MODEL_NAME = 'exploration'
@classmethod
def _get_change_domain_class(cls, unused_item):
@@ -1175,7 +1223,7 @@ class ExplorationSnapshotContentModelValidator(
BaseSnapshotContentModelValidator):
"""Class for validating ExplorationSnapshotContentModel."""
- related_model_name = 'exploration'
+ EXTERNAL_MODEL_NAME = 'exploration'
@classmethod
def _get_external_id_relationships(cls, item):
@@ -1194,8 +1242,8 @@ def _get_external_id_relationships(cls, item):
if item.cloned_from:
cloned_from_exploration_id.append(item.cloned_from)
snapshot_model_ids = [
- '%s-%d' % (item.id, version) for version in range(
- 1, item.version + 1)]
+ '%s-%d' % (item.id, version)
+ for version in range(1, item.version + 1)]
return {
'exploration_ids': (
exp_models.ExplorationModel, [item.id]),
@@ -1245,7 +1293,7 @@ class ExplorationRightsSnapshotMetadataModelValidator(
BaseSnapshotMetadataModelValidator):
"""Class for validating ExplorationRightsSnapshotMetadataModel."""
- related_model_name = 'exploration rights'
+ EXTERNAL_MODEL_NAME = 'exploration rights'
@classmethod
def _get_change_domain_class(cls, unused_item):
@@ -1266,7 +1314,7 @@ class ExplorationRightsSnapshotContentModelValidator(
BaseSnapshotContentModelValidator):
"""Class for validating ExplorationRightsSnapshotContentModel."""
- related_model_name = 'exploration rights'
+ EXTERNAL_MODEL_NAME = 'exploration rights'
@classmethod
def _get_external_id_relationships(cls, item):
@@ -1280,7 +1328,7 @@ def _get_external_id_relationships(cls, item):
class ExplorationCommitLogEntryModelValidator(BaseCommitLogEntryModelValidator):
"""Class for validating ExplorationCommitLogEntryModel."""
- related_model_name = 'exploration'
+ EXTERNAL_MODEL_NAME = 'exploration'
@classmethod
def _get_model_id_regex(cls, item):
@@ -1294,8 +1342,12 @@ def _get_model_id_regex(cls, item):
def _get_change_domain_class(cls, item):
if item.id.startswith('rights'):
return rights_manager.ExplorationRightsChange
- else:
+ elif item.id.startswith('exploration'):
return exp_domain.ExplorationChange
+ else:
+ # The case of invalid id is being ignored here since this
+ # case will already be checked by the id regex test.
+ return None
@classmethod
def _get_external_id_relationships(cls, item):
@@ -1383,7 +1435,11 @@ def _validate_exploration_model_last_updated(cls, item):
cls.external_instance_details['exploration_ids'])
for (_, _, exploration_model) in (
exploration_model_class_model_id_model_tuples):
- if not exploration_model or exploration_model.deleted:
+ # The case for missing exploration external model is ignored here
+ # since errors for missing exploration external model are already
+ # checked and stored in _validate_external_id_relationships
+ # function.
+ if exploration_model is None or exploration_model.deleted:
continue
last_human_update_ms = exp_services.get_last_updated_by_human_ms(
exploration_model.id)
@@ -1398,7 +1454,7 @@ def _validate_exploration_model_last_updated(cls, item):
last_human_update_time))
@classmethod
- def _get_related_model_properties(cls):
+ def _get_external_model_properties(cls):
exploration_model_class_model_id_model_tuples = (
cls.external_instance_details['exploration_ids'])
exploration_rights_model_class_model_id_model_tuples = (
@@ -1450,8 +1506,8 @@ def _get_model_id_regex(cls, unused_item):
@classmethod
def _get_external_id_relationships(cls, item):
snapshot_model_ids = [
- '%s-%d' % (item.id, version) for version in range(
- 1, item.version + 1)]
+ '%s-%d' % (item.id, version)
+ for version in range(1, item.version + 1)]
# Item id is of the format:
# /exploration/exp_id/assets/(image|audio)/filepath.
@@ -1476,7 +1532,7 @@ class FileMetadataSnapshotMetadataModelValidator(
BaseSnapshotMetadataModelValidator):
"""Class for validating FileMetadataSnapshotMetadataModel."""
- related_model_name = 'file metadata'
+ EXTERNAL_MODEL_NAME = 'file metadata'
@classmethod
def _get_model_id_regex(cls, unused_item):
@@ -1501,7 +1557,7 @@ class FileMetadataSnapshotContentModelValidator(
BaseSnapshotContentModelValidator):
"""Class for validating FileMetadataSnapshotContentModel."""
- related_model_name = 'file metadata'
+ EXTERNAL_MODEL_NAME = 'file metadata'
@classmethod
def _get_model_id_regex(cls, unused_item):
@@ -1526,8 +1582,8 @@ def _get_model_id_regex(cls, unused_item):
@classmethod
def _get_external_id_relationships(cls, item):
snapshot_model_ids = [
- '%s-%d' % (item.id, version) for version in range(
- 1, item.version + 1)]
+ '%s-%d' % (item.id, version)
+ for version in range(1, item.version + 1)]
# Item id is of the format:
# /exploration/exp_id/assets/(image|audio)/filepath.
@@ -1551,7 +1607,7 @@ def _get_external_id_relationships(cls, item):
class FileSnapshotMetadataModelValidator(BaseSnapshotMetadataModelValidator):
"""Class for validating FileSnapshotMetadataModel."""
- related_model_name = 'file'
+ EXTERNAL_MODEL_NAME = 'file'
@classmethod
def _get_model_id_regex(cls, unused_item):
@@ -1575,7 +1631,7 @@ def _get_external_id_relationships(cls, item):
class FileSnapshotContentModelValidator(BaseSnapshotContentModelValidator):
"""Class for validating FileSnapshotContentModel."""
- related_model_name = 'file'
+ EXTERNAL_MODEL_NAME = 'file'
@classmethod
def _get_model_id_regex(cls, unused_item):
@@ -1672,8 +1728,8 @@ def _get_model_domain_object_instance(cls, item):
@classmethod
def _get_external_id_relationships(cls, item):
snapshot_model_ids = [
- '%s-%d' % (item.id, version) for version in range(
- 1, item.version + 1)]
+ '%s-%d' % (item.id, version)
+ for version in range(1, item.version + 1)]
return {
'story_commit_log_entry_ids': (
story_models.StoryCommitLogEntryModel,
@@ -1699,7 +1755,7 @@ def _get_external_id_relationships(cls, item):
class StorySnapshotMetadataModelValidator(BaseSnapshotMetadataModelValidator):
"""Class for validating StorySnapshotMetadataModel."""
- related_model_name = 'story'
+ EXTERNAL_MODEL_NAME = 'story'
@classmethod
def _get_change_domain_class(cls, unused_item):
@@ -1718,7 +1774,7 @@ def _get_external_id_relationships(cls, item):
class StorySnapshotContentModelValidator(BaseSnapshotContentModelValidator):
"""Class for validating StorySnapshotContentModel."""
- related_model_name = 'story'
+ EXTERNAL_MODEL_NAME = 'story'
@classmethod
def _get_external_id_relationships(cls, item):
@@ -1734,8 +1790,8 @@ class StoryRightsModelValidator(BaseModelValidator):
@classmethod
def _get_external_id_relationships(cls, item):
snapshot_model_ids = [
- '%s-%d' % (item.id, version) for version in range(
- 1, item.version + 1)]
+ '%s-%d' % (item.id, version)
+ for version in range(1, item.version + 1)]
return {
'story_ids': (
story_models.StoryModel, [item.id]),
@@ -1754,7 +1810,7 @@ class StoryRightsSnapshotMetadataModelValidator(
BaseSnapshotMetadataModelValidator):
"""Class for validating StoryRightsSnapshotMetadataModel."""
- related_model_name = 'story rights'
+ EXTERNAL_MODEL_NAME = 'story rights'
@classmethod
def _get_change_domain_class(cls, unused_item):
@@ -1775,7 +1831,7 @@ class StoryRightsSnapshotContentModelValidator(
BaseSnapshotContentModelValidator):
"""Class for validating StoryRightsSnapshotContentModel."""
- related_model_name = 'story rights'
+ EXTERNAL_MODEL_NAME = 'story rights'
@classmethod
def _get_external_id_relationships(cls, item):
@@ -1789,7 +1845,7 @@ def _get_external_id_relationships(cls, item):
class StoryCommitLogEntryModelValidator(BaseCommitLogEntryModelValidator):
"""Class for validating StoryCommitLogEntryModel."""
- related_model_name = 'story'
+ EXTERNAL_MODEL_NAME = 'story'
@classmethod
def _get_model_id_regex(cls, item):
@@ -1800,8 +1856,13 @@ def _get_model_id_regex(cls, item):
return regex_string
@classmethod
- def _get_change_domain_class(cls, unused_item):
- return story_domain.StoryChange
+ def _get_change_domain_class(cls, item):
+ if item.id.startswith('story'):
+ return story_domain.StoryChange
+ else:
+ # The case of invalid id is being ignored here since this
+ # case will already be checked by the id regex test.
+ return None
@classmethod
def _get_external_id_relationships(cls, item):
@@ -1839,6 +1900,12 @@ def _validate_node_count(cls, item):
'story_ids']
for (_, _, story_model) in story_model_class_model_id_model_tuples:
+ # The case for missing story external model is ignored here
+ # since errors for missing story external model are already
+ # checked and stored in _validate_external_id_relationships
+ # function.
+ if story_model is None or story_model.deleted:
+ continue
nodes = story_model.story_contents['nodes']
if item.node_count != len(nodes):
cls.errors['node count check'].append((
@@ -1847,7 +1914,7 @@ def _validate_node_count(cls, item):
item.id, item.node_count, nodes))
@classmethod
- def _get_related_model_properties(cls):
+ def _get_external_model_properties(cls):
story_model_class_model_id_model_tuples = cls.external_instance_details[
'story_ids']
diff --git a/core/domain/prod_validation_jobs_one_off_test.py b/core/domain/prod_validation_jobs_one_off_test.py
index 80696ced80ba3..790c61d66ec93 100644
--- a/core/domain/prod_validation_jobs_one_off_test.py
+++ b/core/domain/prod_validation_jobs_one_off_test.py
@@ -127,7 +127,6 @@ def run_job_and_check_output(
self.assertEqual(actual_output, expected_output)
-
def update_datastore_types_for_mock_datetime():
"""Updates datastore types for MockDatetime13Hours to ensure that validation
of ndb datetime properties does not fail.
@@ -175,11 +174,11 @@ def _get_external_id_relationships(cls, item):
class MockSnapshotMetadataModelValidator(
prod_validation_jobs_one_off.BaseSnapshotMetadataModelValidator):
- related_model_name = 'related model'
+ EXTERNAL_MODEL_NAME = 'external model'
@classmethod
def _get_external_id_relationships(cls, item):
return {
- 'related_model_ids': (MockModel, [])
+ 'external_model_ids': (MockModel, [])
}
@@ -194,13 +193,13 @@ def test_error_is_raised_if_fetch_external_properties_is_undefined(self):
with self.assertRaises(NotImplementedError):
MockBaseModelValidator().validate(self.item)
- def test_error_is_get_related_model_properties_is_undefined(self):
+ def test_error_is_get_external_model_properties_is_undefined(self):
with self.assertRaises(NotImplementedError):
MockSummaryModelValidator().validate(self.item)
- def test_error_is_raised_if_related_model_name_is_undefined(self):
+ def test_error_is_raised_if_external_model_name_is_undefined(self):
with self.assertRaisesRegexp(
- Exception, 'Related model name should be specified'):
+ Exception, 'External model name should be specified'):
MockSnapshotContentModelValidator().validate(self.item)
def test_error_is_raised_if_get_change_domain_class_is_undefined(self):
@@ -662,7 +661,9 @@ def setUp(self):
collection_models.CollectionSnapshotMetadataModel.get_by_id(
'2-1'))
- self.job_class = prod_validation_jobs_one_off.CollectionSnapshotMetadataModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .CollectionSnapshotMetadataModelAuditOneOffJob)
def test_standard_operation(self):
collection_services.update_collection(
@@ -827,7 +828,9 @@ def setUp(self):
collection_models.CollectionSnapshotContentModel.get_by_id(
'2-1'))
- self.job_class = prod_validation_jobs_one_off.CollectionSnapshotContentModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .CollectionSnapshotContentModelAuditOneOffJob)
def test_standard_operation(self):
collection_services.update_collection(
@@ -1153,7 +1156,9 @@ def setUp(self):
collection_models.CollectionRightsSnapshotMetadataModel.get_by_id(
'2-1'))
- self.job_class = prod_validation_jobs_one_off.CollectionRightsSnapshotMetadataModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .CollectionRightsSnapshotMetadataModelAuditOneOffJob)
def test_standard_operation(self):
expected_output = [
@@ -1318,7 +1323,9 @@ def setUp(self):
collection_models.CollectionRightsSnapshotContentModel.get_by_id(
'2-1'))
- self.job_class = prod_validation_jobs_one_off.CollectionRightsSnapshotContentModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .CollectionRightsSnapshotContentModelAuditOneOffJob)
def test_standard_operation(self):
expected_output = [
@@ -1444,7 +1451,9 @@ def setUp(self):
collection_models.CollectionCommitLogEntryModel.get_by_id(
'collection-2-1'))
- self.job_class = prod_validation_jobs_one_off.CollectionCommitLogEntryModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .CollectionCommitLogEntryModelAuditOneOffJob)
def test_standard_operation(self):
collection_services.update_collection(
@@ -1553,7 +1562,11 @@ def test_model_with_invalid_id(self):
u'[u\'failed validation check for model id check of '
'CollectionCommitLogEntryModel\', '
'[u\'Entity id %s: Entity id does not match regex pattern\']]'
- ) % (model_with_invalid_id.id),
+ ) % (model_with_invalid_id.id), (
+ u'[u\'failed validation check for commit cmd check of '
+ 'CollectionCommitLogEntryModel\', [u\'Entity id invalid-0-1: '
+ 'No commit command domain object defined for entity with '
+ 'commands: [{}]\']]'),
u'[u\'fully-validated CollectionCommitLogEntryModel\', 4]']
run_job_and_check_output(self, expected_output, sort=True)
@@ -2004,7 +2017,9 @@ def setUp(self):
config_models.ConfigPropertySnapshotMetadataModel.get_by_id(
'oppia_csrf_secret-1'))
- self.job_class = prod_validation_jobs_one_off.ConfigPropertySnapshotMetadataModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .ConfigPropertySnapshotMetadataModelAuditOneOffJob)
def test_standard_operation(self):
self.config_model.commit(self.admin_id, [])
@@ -2137,7 +2152,9 @@ def setUp(self):
config_models.ConfigPropertySnapshotContentModel.get_by_id(
'oppia_csrf_secret-1'))
- self.job_class = prod_validation_jobs_one_off.ConfigPropertySnapshotContentModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .ConfigPropertySnapshotContentModelAuditOneOffJob)
def test_standard_operation(self):
self.config_model.commit(self.admin_id, [])
@@ -2512,7 +2529,9 @@ def setUp(self):
self.user_id, self.thread_id))
self.model_instance.put()
- self.job_class = prod_validation_jobs_one_off.GeneralFeedbackEmailReplyToIdModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .GeneralFeedbackEmailReplyToIdModelAuditOneOffJob)
def test_standard_model(self):
expected_output = [(
@@ -2785,7 +2804,9 @@ def setUp(self):
exp_models.ExplorationSnapshotMetadataModel.get_by_id(
'2-1'))
- self.job_class = prod_validation_jobs_one_off.ExplorationSnapshotMetadataModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .ExplorationSnapshotMetadataModelAuditOneOffJob)
def test_standard_operation(self):
exp_services.update_exploration(
@@ -2939,7 +2960,9 @@ def setUp(self):
exp_models.ExplorationSnapshotContentModel.get_by_id(
'2-1'))
- self.job_class = prod_validation_jobs_one_off.ExplorationSnapshotContentModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .ExplorationSnapshotContentModelAuditOneOffJob)
def test_standard_operation(self):
exp_services.update_exploration(
@@ -3248,7 +3271,9 @@ def setUp(self):
exp_models.ExplorationRightsSnapshotMetadataModel.get_by_id(
'2-1'))
- self.job_class = prod_validation_jobs_one_off.ExplorationRightsSnapshotMetadataModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .ExplorationRightsSnapshotMetadataModelAuditOneOffJob)
def test_standard_operation(self):
expected_output = [
@@ -3400,7 +3425,9 @@ def setUp(self):
exp_models.ExplorationRightsSnapshotContentModel.get_by_id(
'2-1'))
- self.job_class = prod_validation_jobs_one_off.ExplorationRightsSnapshotContentModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .ExplorationRightsSnapshotContentModelAuditOneOffJob)
def test_standard_operation(self):
expected_output = [
@@ -3514,7 +3541,9 @@ def setUp(self):
exp_models.ExplorationCommitLogEntryModel.get_by_id(
'exploration-2-1'))
- self.job_class = prod_validation_jobs_one_off.ExplorationCommitLogEntryModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .ExplorationCommitLogEntryModelAuditOneOffJob)
def test_standard_operation(self):
exp_services.update_exploration(
@@ -3623,7 +3652,11 @@ def test_model_with_invalid_id(self):
u'[u\'failed validation check for model id check of '
'ExplorationCommitLogEntryModel\', '
'[u\'Entity id %s: Entity id does not match regex pattern\']]'
- ) % (model_with_invalid_id.id),
+ ) % (model_with_invalid_id.id), (
+ u'[u\'failed validation check for commit cmd check of '
+ 'ExplorationCommitLogEntryModel\', [u\'Entity id invalid-0-1: '
+ 'No commit command domain object defined for entity with '
+ 'commands: [{}]\']]'),
u'[u\'fully-validated ExplorationCommitLogEntryModel\', 4]']
run_job_and_check_output(self, expected_output, sort=True)
@@ -4135,7 +4168,9 @@ def setUp(self):
file_models.FileMetadataSnapshotMetadataModel.get_by_id(
'%s-1' % self.id_1))
- self.job_class = prod_validation_jobs_one_off.FileMetadataSnapshotMetadataModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .FileMetadataSnapshotMetadataModelAuditOneOffJob)
def test_standard_operation(self):
expected_output = [
@@ -4264,7 +4299,9 @@ def setUp(self):
file_models.FileMetadataSnapshotContentModel.get_by_id(
'%s-1' % self.id_1))
- self.job_class = prod_validation_jobs_one_off.FileMetadataSnapshotContentModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .FileMetadataSnapshotContentModelAuditOneOffJob)
def test_standard_operation(self):
expected_output = [
@@ -4490,7 +4527,9 @@ def setUp(self):
file_models.FileSnapshotMetadataModel.get_by_id(
'%s-1' % self.id_1))
- self.job_class = prod_validation_jobs_one_off.FileSnapshotMetadataModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .FileSnapshotMetadataModelAuditOneOffJob)
def test_standard_operation(self):
expected_output = [
@@ -4617,7 +4656,8 @@ def setUp(self):
file_models.FileSnapshotContentModel.get_by_id(
'%s-1' % self.id_1))
- self.job_class = prod_validation_jobs_one_off.FileSnapshotContentModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off.FileSnapshotContentModelAuditOneOffJob)
def test_standard_operation(self):
expected_output = [
@@ -4720,7 +4760,9 @@ def setUp(self):
recommendations_models.ExplorationRecommendationsModel.get_by_id(
'1'))
- self.job_class = prod_validation_jobs_one_off.ExplorationRecommendationsModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .ExplorationRecommendationsModelAuditOneOffJob)
def test_standard_model(self):
expected_output = [(
@@ -5208,7 +5250,9 @@ def setUp(self):
story_models.StorySnapshotMetadataModel.get_by_id(
'2-1'))
- self.job_class = prod_validation_jobs_one_off.StorySnapshotMetadataModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .StorySnapshotMetadataModelAuditOneOffJob)
def test_standard_operation(self):
story_services.update_story(
@@ -5369,7 +5413,9 @@ def setUp(self):
story_models.StorySnapshotContentModel.get_by_id(
'2-1'))
- self.job_class = prod_validation_jobs_one_off.StorySnapshotContentModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .StorySnapshotContentModelAuditOneOffJob)
def test_standard_operation(self):
story_services.update_story(
@@ -5641,7 +5687,9 @@ def setUp(self):
story_models.StoryRightsSnapshotMetadataModel.get_by_id(
'2-1'))
- self.job_class = prod_validation_jobs_one_off.StoryRightsSnapshotMetadataModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .StoryRightsSnapshotMetadataModelAuditOneOffJob)
def test_standard_operation(self):
expected_output = [
@@ -5798,7 +5846,9 @@ def setUp(self):
story_models.StoryRightsSnapshotContentModel.get_by_id(
'2-1'))
- self.job_class = prod_validation_jobs_one_off.StoryRightsSnapshotContentModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .StoryRightsSnapshotContentModelAuditOneOffJob)
def test_standard_operation(self):
expected_output = [
@@ -5908,7 +5958,9 @@ def setUp(self):
story_models.StoryCommitLogEntryModel.get_by_id(
'story-2-1'))
- self.job_class = prod_validation_jobs_one_off.StoryCommitLogEntryModelAuditOneOffJob # pylint: disable=line-too-long
+ self.job_class = (
+ prod_validation_jobs_one_off
+ .StoryCommitLogEntryModelAuditOneOffJob)
def test_standard_operation(self):
story_services.update_story(
@@ -6002,7 +6054,11 @@ def test_model_with_invalid_id(self):
u'[u\'failed validation check for model id check of '
'StoryCommitLogEntryModel\', '
'[u\'Entity id %s: Entity id does not match regex pattern\']]'
- ) % (model_with_invalid_id.id),
+ ) % (model_with_invalid_id.id), (
+ u'[u\'failed validation check for commit cmd check of '
+ 'StoryCommitLogEntryModel\', [u\'Entity id invalid-0-1: '
+ 'No commit command domain object defined for entity with '
+ 'commands: [{}]\']]'),
u'[u\'fully-validated StoryCommitLogEntryModel\', 3]']
run_job_and_check_output(self, expected_output, sort=True)
diff --git a/core/domain/question_domain.py b/core/domain/question_domain.py
index 9bff25f2fcdae..618102f6e6da3 100644
--- a/core/domain/question_domain.py
+++ b/core/domain/question_domain.py
@@ -17,7 +17,9 @@
"""Domain objects relating to questions."""
from constants import constants
+from core.domain import change_domain
from core.domain import html_cleaner
+from core.domain import html_validation_service
from core.domain import interaction_registry
from core.domain import state_domain
from core.platform import models
@@ -48,73 +50,58 @@
CMD_CREATE_NEW = 'create_new'
-class QuestionChange(object):
- """Domain object for changes made to question object."""
+class QuestionChange(change_domain.BaseChange):
+ """Domain object for changes made to question object.
+
+ The allowed commands, together with the attributes:
+ - 'create_new'
+ - 'update question property' (with property_name, new_value
+ and old_value)
+ - 'create_new_fully_specified_question' (with question_dict,
+ skill_id)
+ - 'migrate_state_schema_to_latest_version' (with from_version
+ and to_version)
+ """
+
+ # The allowed list of question properties which can be used in
+ # update_question_property command.
QUESTION_PROPERTIES = (
QUESTION_PROPERTY_QUESTION_STATE_DATA,
QUESTION_PROPERTY_LANGUAGE_CODE,
QUESTION_PROPERTY_LINKED_SKILL_IDS)
- OPTIONAL_CMD_ATTRIBUTE_NAMES = [
- 'property_name', 'new_value', 'old_value', 'question_dict',
- 'skill_id', 'from_version', 'to_version'
- ]
-
- def __init__(self, change_dict):
- """Initialize a QuestionChange object from a dict.
-
- Args:
- change_dict: dict. Represents a command. It should have a 'cmd'
- key, and one or more other keys. The keys depend on what the
- value for 'cmd' is. The possible values for 'cmd' are listed
- below, together with the other keys in the dict:
- - 'update question property' (with property_name, new_value
- and old_value)
- - 'create_new_fully_specified_question' (with question_dict,
- skill_id)
- - 'migrate_state_schema_to_latest_version' (with from_version
- and to_version)
-
- Raises:
- Exception: The given change dict is not valid.
- """
- if 'cmd' not in change_dict:
- raise Exception('Invalid change_dict: %s' % change_dict)
- self.cmd = change_dict['cmd']
-
- if self.cmd == CMD_UPDATE_QUESTION_PROPERTY:
- if (change_dict['property_name'] in
- self.QUESTION_PROPERTIES):
- self.property_name = change_dict['property_name']
- self.new_value = change_dict['new_value']
- self.old_value = change_dict['old_value']
- else:
- raise Exception('Invalid change_dict: %s' % change_dict)
- elif self.cmd == CMD_CREATE_NEW_FULLY_SPECIFIED_QUESTION:
- self.question_dict = change_dict['question_dict']
- # Note that change_dict['skill_id'] may be None if this change is
- # being done in the context of a suggestion.
- self.skill_id = change_dict['skill_id']
- elif self.cmd == CMD_MIGRATE_STATE_SCHEMA_TO_LATEST_VERSION:
- self.from_version = change_dict['from_version']
- self.to_version = change_dict['to_version']
- else:
- raise Exception('Invalid change_dict: %s' % change_dict)
-
- def to_dict(self):
- """Returns a dict representing the QuestionChange domain object.
-
- Returns:
- A dict, mapping all fields of QuestionChange instance.
- """
- question_change_dict = {}
- question_change_dict['cmd'] = self.cmd
- for attribute_name in self.OPTIONAL_CMD_ATTRIBUTE_NAMES:
- if hasattr(self, attribute_name):
- question_change_dict[attribute_name] = getattr(
- self, attribute_name)
+ ALLOWED_COMMANDS = [{
+ 'name': CMD_CREATE_NEW,
+ 'required_attribute_names': [],
+ 'optional_attribute_names': []
+ }, {
+ 'name': CMD_UPDATE_QUESTION_PROPERTY,
+ 'required_attribute_names': ['property_name', 'new_value', 'old_value'],
+ 'optional_attribute_names': [],
+ 'allowed_values': {'property_name': QUESTION_PROPERTIES}
+ }, {
+ 'name': CMD_CREATE_NEW_FULLY_SPECIFIED_QUESTION,
+ 'required_attribute_names': ['question_dict', 'skill_id'],
+ 'optional_attribute_names': []
+ }, {
+ 'name': CMD_MIGRATE_STATE_SCHEMA_TO_LATEST_VERSION,
+ 'required_attribute_names': ['from_version', 'to_version'],
+ 'optional_attribute_names': []
+ }]
+
+
+class QuestionRightsChange(change_domain.BaseChange):
+ """Domain object for changes made to question rights object.
+
+ The allowed commands, together with the attributes:
+ - 'create_new'.
+ """
- return question_change_dict
+ ALLOWED_COMMANDS = [{
+ 'name': CMD_CREATE_NEW,
+ 'required_attribute_names': [],
+ 'optional_attribute_names': []
+ }]
class Question(object):
@@ -415,7 +402,13 @@ def __init__(
"""
self.id = question_id
self.creator_id = creator_id
- self.question_content = html_cleaner.clean(question_content)
+ # The initial clean up of html by converting it to ckeditor format
+ # is required since user may copy and paste some stuff in the rte
+ # which is not a valid ckeditor html string but can be converted
+ # to a valid ckeditor string without errors. This initial clean up
+ # ensures that validation will not fail in such cases.
+ self.question_content = html_validation_service.convert_to_ckeditor(
+ html_cleaner.clean(question_content))
self.created_on = question_model_created_on
self.last_updated = question_model_last_updated
@@ -433,6 +426,28 @@ def to_dict(self):
'created_on_msec': utils.get_time_in_millisecs(self.created_on)
}
+ def validate(self):
+ """Validates the Question summary domain object before it is saved.
+
+ Raises:
+ ValidationError: One or more attributes of question summary are
+ invalid.
+ """
+ err_dict = html_validation_service.validate_rte_format(
+ [self.question_content], feconf.RTE_FORMAT_CKEDITOR)
+ for key in err_dict:
+ if err_dict[key]:
+ raise utils.ValidationError(
+ 'Invalid html: %s for rte with invalid tags and '
+ 'strings: %s' % (self.question_content, err_dict))
+
+ err_dict = html_validation_service.validate_customization_args([
+ self.question_content])
+ if err_dict:
+ raise utils.ValidationError(
+ 'Invalid html: %s due to errors in customization_args: %s' % (
+ self.question_content, err_dict))
+
class QuestionSkillLink(object):
"""Domain object for Question Skill Link.
diff --git a/core/domain/question_domain_test.py b/core/domain/question_domain_test.py
index 384838895a19f..145c3a24e7935 100644
--- a/core/domain/question_domain_test.py
+++ b/core/domain/question_domain_test.py
@@ -50,8 +50,9 @@ def test_change_dict_without_cmd(self):
"""Test to verify __init__ method of the Question Change object
when change_dict is without cmd key.
"""
- self.assertRaises(
- Exception,
+ self.assertRaisesRegexp(
+ utils.ValidationError,
+ 'Missing cmd key in change dict',
callableObj=question_domain.QuestionChange,
change_dict={}
)
@@ -60,25 +61,88 @@ def test_change_dict_with_wrong_cmd(self):
"""Test to verify __init__ method of the Question Change object
when change_dict is with wrong cmd value.
"""
- self.assertRaises(
- Exception,
+ self.assertRaisesRegexp(
+ utils.ValidationError,
+ 'Command wrong is not allowed',
callableObj=question_domain.QuestionChange,
change_dict={'cmd': 'wrong', }
)
+ def test_change_dict_with_missing_attributes_in_cmd(self):
+ """Test to verify __init__ method of the Question Change object
+ when change_dict is with missing attributes in cmd.
+ """
+ self.assertRaisesRegexp(
+ utils.ValidationError,
+ 'The following required attributes are present: new_value',
+ callableObj=question_domain.QuestionChange,
+ change_dict={
+ 'cmd': 'update_question_property',
+ 'property_name': 'question_state_data',
+ 'old_value': 'old_value'
+ }
+ )
+
+ def test_change_dict_with_extra_attributes_in_cmd(self):
+ """Test to verify __init__ method of the Question Change object
+ when change_dict is with extra attributes in cmd.
+ """
+ self.assertRaisesRegexp(
+ utils.ValidationError,
+ 'The following extra attributes are present: invalid',
+ callableObj=question_domain.QuestionChange,
+ change_dict={'cmd': 'create_new', 'invalid': 'invalid'}
+ )
+
def test_update_question_property_with_wrong_property_name(self):
"""Test to verify __init__ method of the Question Change object
when cmd is update_question_property and wrong property_name is given.
"""
- self.assertRaises(
- Exception,
+ self.assertRaisesRegexp(
+ utils.ValidationError, (
+ 'Value for property_name in cmd update_question_property: '
+ 'wrong is not allowed'),
callableObj=question_domain.QuestionChange,
change_dict={
'cmd': 'update_question_property',
'property_name': 'wrong',
+ 'new_value': 'new_value',
+ 'old_value': 'old_value'
}
)
+ def test_create_new(self):
+ """Test to verify __init__ method of the Question Change object
+ when cmd is create_new.
+ """
+ change_dict = {
+ 'cmd': 'create_new'
+ }
+ observed_object = question_domain.QuestionChange(
+ change_dict=change_dict,
+ )
+
+ self.assertEqual('create_new', observed_object.cmd)
+
+ def test_update_question_property(self):
+ """Test to verify __init__ method of the Question Change object
+ when cmd is update_question_property.
+ """
+ change_dict = {
+ 'cmd': 'update_question_property',
+ 'property_name': 'question_state_data',
+ 'new_value': 'new_value',
+ 'old_value': 'old_value'
+ }
+ observed_object = question_domain.QuestionChange(
+ change_dict=change_dict
+ )
+
+ self.assertEqual('update_question_property', observed_object.cmd)
+ self.assertEqual('question_state_data', observed_object.property_name)
+ self.assertEqual('new_value', observed_object.new_value)
+ self.assertEqual('old_value', observed_object.old_value)
+
def test_create_new_fully_specified_question(self):
"""Test to verify __init__ method of the Question Change object
when cmd is create_new_fully_specified_question.
@@ -92,6 +156,8 @@ def test_create_new_fully_specified_question(self):
change_dict=change_dict,
)
+ self.assertEqual(
+ 'create_new_fully_specified_question', observed_object.cmd)
self.assertEqual('10', observed_object.skill_id)
self.assertEqual({}, observed_object.question_dict)
@@ -108,10 +174,80 @@ def test_migrate_state_schema_to_latest_version(self):
change_dict=change_dict,
)
+ self.assertEqual(
+ 'migrate_state_schema_to_latest_version', observed_object.cmd)
self.assertEqual(0, observed_object.from_version)
self.assertEqual(10, observed_object.to_version)
+class QuestionRightsChangeTest(test_utils.GenericTestBase):
+ """Test for QuestionRights Change object."""
+
+ def test_to_dict(self):
+ """Test to verify to_dict method of the QuestionRights
+ Change object.
+ """
+ expected_object_dict = {
+ 'cmd': 'create_new'
+ }
+
+ change_dict = {
+ 'cmd': 'create_new'
+ }
+ observed_object = question_domain.QuestionRightsChange(
+ change_dict=change_dict,
+ )
+
+ self.assertEqual(expected_object_dict, observed_object.to_dict())
+
+ def test_change_dict_without_cmd(self):
+ """Test to verify __init__ method of the QuestionRights
+ Change object when change_dict is without cmd key.
+ """
+ self.assertRaisesRegexp(
+ utils.ValidationError,
+ 'Missing cmd key in change dict',
+ callableObj=question_domain.QuestionRightsChange,
+ change_dict={}
+ )
+
+ def test_change_dict_with_wrong_cmd(self):
+ """Test to verify __init__ method of the QuestionRights
+ Change object when change_dict is with wrong cmd value.
+ """
+ self.assertRaisesRegexp(
+ utils.ValidationError,
+ 'Command wrong is not allowed',
+ callableObj=question_domain.QuestionRightsChange,
+ change_dict={'cmd': 'wrong', }
+ )
+
+ def test_change_dict_with_extra_attributes_in_cmd(self):
+ """Test to verify __init__ method of the QuestionRights Change
+ object when change_dict is with extra attributes in cmd.
+ """
+ self.assertRaisesRegexp(
+ utils.ValidationError,
+ 'The following extra attributes are present: invalid',
+ callableObj=question_domain.QuestionRightsChange,
+ change_dict={'cmd': 'create_new', 'invalid': 'invalid'}
+ )
+
+ def test_create_new(self):
+ """Test to verify __init__ method of the QuestionRights Change
+ object when cmd is create_new.
+ """
+ change_dict = {
+ 'cmd': 'create_new'
+ }
+ observed_object = question_domain.QuestionRightsChange(
+ change_dict=change_dict,
+ )
+
+ self.assertEqual('create_new', observed_object.cmd)
+
+
+
class QuestionDomainTest(test_utils.GenericTestBase):
"""Tests for Question domain object."""
@@ -178,7 +314,7 @@ def test_strict_validation_for_answer_groups(self):
'dest': 'abc',
'feedback': {
'content_id': 'feedback_1',
- 'html': 'Feedback'
+ 'html': 'Feedback
' }, 'labelled_as_correct': True, 'param_changes': [], @@ -303,29 +439,68 @@ def test_update_question_state_data(self): class QuestionSummaryTest(test_utils.GenericTestBase): """Test for Question Summary object.""" + def setUp(self): + super(QuestionSummaryTest, self).setUp() + self.fake_date_created = datetime.datetime( + 2018, 11, 17, 20, 2, 45, 0) + self.fake_date_updated = datetime.datetime( + 2018, 11, 17, 20, 3, 14, 0) + self.observed_object = question_domain.QuestionSummary( + creator_id='user_1', + question_id='question_1', + question_content='question content
', + question_model_created_on=self.fake_date_created, + question_model_last_updated=self.fake_date_updated, + ) + def test_to_dict(self): """Test to verify to_dict method of the Question Summary object. """ - fake_date_created = datetime.datetime(2018, 11, 17, 20, 2, 45, 0) - fake_date_updated = datetime.datetime(2018, 11, 17, 20, 3, 14, 0) expected_object_dict = { 'id': 'question_1', 'creator_id': 'user_1', - 'question_content': u'question content', - 'last_updated_msec': utils.get_time_in_millisecs(fake_date_updated), - 'created_on_msec': utils.get_time_in_millisecs(fake_date_created), + 'question_content': 'question content
', + 'last_updated_msec': utils.get_time_in_millisecs( + self.fake_date_updated), + 'created_on_msec': utils.get_time_in_millisecs( + self.fake_date_created), } - observed_object = question_domain.QuestionSummary( - creator_id='user_1', - question_id='question_1', - question_content='question content', - question_model_created_on=fake_date_created, - question_model_last_updated=fake_date_updated, - ) + self.assertEqual(expected_object_dict, self.observed_object.to_dict()) - self.assertEqual(expected_object_dict, observed_object.to_dict()) + def test_validation_with_valid_properties(self): + self.observed_object.validate() + + def test_validation_with_invalid_html_in_question_content(self): + """Test validation fails with invalid html in question + content. + """ + self.observed_object.question_content = 'Test' + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Invalid html: Test for rte with invalid tags and ' + 'strings: {\'invalidTags\': \\[u\'a\'], ' + '\'strings\': \\[\'Test\']}')): + self.observed_object.validate() + + def test_validation_with_invalid_customization_args_in_question_content( + self): + """Test validation fails with invalid customization args in question + content. + """ + self.observed_object.question_content = ( + 'Explanation
'), [ + state_domain.SubtitledHtml('2', 'Example 1
')], {'1': {}, '2': {}}, state_domain.WrittenTranslations.from_dict( {'translations_mapping': {'1': {}, '2': {}}})) misconceptions = [skill_domain.Misconception( - self.MISCONCEPTION_ID, 'name', 'notes', 'default_feedback')] + self.MISCONCEPTION_ID, 'name', 'notes
', + 'default_feedback
')] self.skill = skill_domain.Skill( self.SKILL_ID, 'Description', misconceptions, skill_contents, feconf.CURRENT_MISCONCEPTIONS_SCHEMA_VERSION, @@ -166,6 +169,45 @@ def test_misconception_validation(self): self.skill.misconceptions = '' self._assert_validation_error('Expected misconceptions to be a list') + def test_misconception_validation_with_invalid_html_in_notes(self): + self.skill.misconceptions[0].notes = 'Test' + self._assert_validation_error( + 'Invalid html: Test for rte with invalid tags and ' + 'strings: {\'invalidTags\': \\[u\'a\'], ' + '\'strings\': \\[\'Test\']}') + + def test_misconception_validation_with_invalid_customization_args_in_notes( + self): + self.skill.misconceptions[0].notes = ( + 'notes
', + 'default_feedback
'), skill_domain.Misconception( - self.MISCONCEPTION_ID, 'name 2', 'notes 2', 'default_feedback')] + self.MISCONCEPTION_ID, 'name 2', 'notes 2
', + 'default_feedback
')] self._assert_validation_error('Duplicate misconception ID found') def test_skill_migration_validation(self): @@ -268,25 +312,18 @@ def test_conversion_to_and_from_dict(self): """Test that to_dict and from_dict preserve all data within a skill_contents and misconception object. """ - audio_translation = { - 'en': state_domain.AudioTranslation.from_dict({ - 'filename': 'file.mp3', - 'file_size_bytes': 'size', - 'needs_update': True - }) - } skill_contents = skill_domain.SkillContents( - state_domain.SubtitledHtml('1', 'Explanation'), [ - state_domain.SubtitledHtml('2', 'Example 1')], - {'1': audio_translation, '2': {}}, - state_domain.WrittenTranslations.from_dict( + state_domain.SubtitledHtml('1', 'Explanation
'), [ + state_domain.SubtitledHtml('2', 'Example 1
')], + {'1': {}, '2': {}}, state_domain.WrittenTranslations.from_dict( {'translations_mapping': {'1': {}, '2': {}}})) skill_contents_dict = skill_contents.to_dict() skill_contents_from_dict = skill_domain.SkillContents.from_dict( skill_contents_dict) misconceptions = skill_domain.Misconception( - self.MISCONCEPTION_ID, 'Tag Name', 'Description', 'Feedback') + self.MISCONCEPTION_ID, 'Tag Name', 'Description
', + 'Feedback
') misconceptions_dict = misconceptions.to_dict() misconceptions_from_dict = skill_domain.Misconception.from_dict( misconceptions_dict) @@ -331,32 +368,320 @@ 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): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): - skill_domain.SkillChange({}) - def test_cannot_update_skill_misconceptions(self): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): +class SkillChangeTests(test_utils.GenericTestBase): + + def test_skill_change_object_with_missing_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, 'Missing cmd key in change dict'): + skill_domain.SkillChange({'invalid': 'data'}) + + def test_skill_change_object_with_invalid_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, 'Command invalid is not allowed'): + skill_domain.SkillChange({'cmd': 'invalid'}) + + def test_skill_change_object_with_missing_attribute_in_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'The following required attributes are missing: ' + 'new_value, old_value')): skill_domain.SkillChange({ - 'cmd': skill_domain.CMD_UPDATE_SKILL_MISCONCEPTIONS_PROPERTY, - 'property_name': 'invalid_property' + 'cmd': 'update_skill_property', + 'property_name': 'name', }) - def test_cannot_create_skill_rights_change_class_with_invalid_change_list( + def test_skill_change_object_with_extra_attribute_in_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'The following extra attributes are present: invalid')): + skill_domain.SkillChange({ + 'cmd': 'add_skill_misconception', + 'new_misconception_dict': { + 'id': 0, 'name': 'name', 'notes': 'notes
', + 'feedback': 'default_feedback
'}, + 'invalid': 'invalid' + }) + + def test_skill_change_object_with_invalid_skill_property(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Value for property_name in cmd update_skill_property: ' + 'invalid is not allowed')): + skill_domain.SkillChange({ + 'cmd': 'update_skill_property', + 'property_name': 'invalid', + 'old_value': 'old_value', + 'new_value': 'new_value', + }) + + def test_skill_change_object_with_invalid_skill_misconception_property( self): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): - skill_domain.SkillRightsChange({}) + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Value for property_name in cmd ' + 'update_skill_misconceptions_property: invalid is not ' + 'allowed')): + skill_domain.SkillChange({ + 'cmd': 'update_skill_misconceptions_property', + 'misconception_id': 'id', + 'property_name': 'invalid', + 'old_value': 'old_value', + 'new_value': 'new_value', + }) + + def test_skill_change_object_with_invalid_skill_contents_property( + self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Value for property_name in cmd ' + 'update_skill_contents_property: invalid is not allowed')): + skill_domain.SkillChange({ + 'cmd': 'update_skill_contents_property', + 'property_name': 'invalid', + 'old_value': 'old_value', + 'new_value': 'new_value', + }) + + def test_skill_change_object_with_add_skill_misconception(self): + skill_change_object = skill_domain.SkillChange({ + 'cmd': 'add_skill_misconception', + 'new_misconception_dict': { + 'id': 0, 'name': 'name', 'notes': 'notes
', + 'feedback': 'default_feedback
'}, + }) + + self.assertEqual(skill_change_object.cmd, 'add_skill_misconception') + self.assertEqual( + skill_change_object.new_misconception_dict, { + 'id': 0, 'name': 'name', 'notes': 'notes
', + 'feedback': 'default_feedback
'}) + + def test_skill_change_object_with_delete_skill_misconception(self): + skill_change_object = skill_domain.SkillChange({ + 'cmd': 'delete_skill_misconception', + 'misconception_id': 'id' + }) + + self.assertEqual( + skill_change_object.cmd, 'delete_skill_misconception') + self.assertEqual(skill_change_object.misconception_id, 'id') + + def test_skill_change_object_with_update_skill_misconceptions_property( + self): + skill_change_object = skill_domain.SkillChange({ + 'cmd': 'update_skill_misconceptions_property', + 'misconception_id': 'id', + 'property_name': 'name', + 'new_value': 'new_value', + 'old_value': 'old_value' + }) + + self.assertEqual( + skill_change_object.cmd, 'update_skill_misconceptions_property') + self.assertEqual(skill_change_object.misconception_id, 'id') + self.assertEqual(skill_change_object.property_name, 'name') + self.assertEqual(skill_change_object.new_value, 'new_value') + self.assertEqual(skill_change_object.old_value, 'old_value') + + def test_skill_change_object_with_update_skill_property( + self): + skill_change_object = skill_domain.SkillChange({ + 'cmd': 'update_skill_property', + 'property_name': 'description', + 'new_value': 'new_value', + 'old_value': 'old_value' + }) + + self.assertEqual(skill_change_object.cmd, 'update_skill_property') + self.assertEqual(skill_change_object.property_name, 'description') + self.assertEqual(skill_change_object.new_value, 'new_value') + self.assertEqual(skill_change_object.old_value, 'old_value') + + def test_skill_change_object_with_update_skill_contents_property( + self): + skill_change_object = skill_domain.SkillChange({ + 'cmd': 'update_skill_contents_property', + 'property_name': 'explanation', + 'new_value': 'new_value', + 'old_value': 'old_value' + }) - def test_cannot_create_skill_rights_change_class_with_invalid_cmd(self): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): + self.assertEqual( + skill_change_object.cmd, 'update_skill_contents_property') + self.assertEqual(skill_change_object.property_name, 'explanation') + self.assertEqual(skill_change_object.new_value, 'new_value') + self.assertEqual(skill_change_object.old_value, 'old_value') + + def test_skill_change_object_with_create_new(self): + skill_change_object = skill_domain.SkillChange({ + 'cmd': 'create_new' + }) + + self.assertEqual(skill_change_object.cmd, 'create_new') + + def test_skill_change_object_with_migrate_contents_schema_to_latest_version( + self): + skill_change_object = skill_domain.SkillChange({ + 'cmd': 'migrate_contents_schema_to_latest_version', + 'from_version': 'from_version', + 'to_version': 'to_version', + }) + + self.assertEqual( + skill_change_object.cmd, + 'migrate_contents_schema_to_latest_version') + self.assertEqual(skill_change_object.from_version, 'from_version') + self.assertEqual(skill_change_object.to_version, 'to_version') + + def test_skill_change_object_with_migrate_misconceptions_schema_to_latest_version( # pylint: disable=line-too-long + self): + skill_change_object = skill_domain.SkillChange({ + 'cmd': 'migrate_misconceptions_schema_to_latest_version', + 'from_version': 'from_version', + 'to_version': 'to_version' + }) + + self.assertEqual( + skill_change_object.cmd, + 'migrate_misconceptions_schema_to_latest_version') + self.assertEqual(skill_change_object.from_version, 'from_version') + self.assertEqual(skill_change_object.to_version, 'to_version') + + def test_to_dict(self): + skill_change_dict = { + 'cmd': 'migrate_misconceptions_schema_to_latest_version', + 'from_version': 'from_version', + 'to_version': 'to_version' + } + skill_change_object = skill_domain.SkillChange(skill_change_dict) + self.assertEqual(skill_change_object.to_dict(), skill_change_dict) + + +class SkillRightsChangeTests(test_utils.GenericTestBase): + + def test_skill_rights_change_object_with_missing_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, 'Missing cmd key in change dict'): + skill_domain.SkillRightsChange({'invalid': 'data'}) + + def test_skill_change_rights_object_with_invalid_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, 'Command invalid is not allowed'): + skill_domain.SkillRightsChange({'cmd': 'invalid'}) + + def test_skill_rights_change_object_with_extra_attribute_in_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'The following extra attributes are present: invalid')): skill_domain.SkillRightsChange({ - 'cmd': 'invalid_cmd' + 'cmd': 'publish_skill', + 'invalid': 'invalid' }) - def test_create_skill_rights_change_class(self): - skill_rights = skill_domain.SkillRightsChange({ - 'cmd': skill_domain.CMD_CREATE_NEW + def test_skill_rights_change_object_with_create_new(self): + skill_rights_change_object = skill_domain.SkillRightsChange({ + 'cmd': 'create_new' + }) + + self.assertEqual(skill_rights_change_object.cmd, 'create_new') + + def test_skill_rights_change_object_with_publish_skill(self): + skill_rights_change_object = skill_domain.SkillRightsChange({ + 'cmd': 'publish_skill' }) + self.assertEqual(skill_rights_change_object.cmd, 'publish_skill') + + def test_to_dict(self): + skill_rights_change_dict = { + 'cmd': 'publish_skill' + } + skill_rights_change_object = skill_domain.SkillRightsChange( + skill_rights_change_dict) self.assertEqual( - skill_rights.to_dict(), {'cmd': skill_domain.CMD_CREATE_NEW}) + skill_rights_change_object.to_dict(), skill_rights_change_dict) + + +class SkillSummaryTests(test_utils.GenericTestBase): + + def setUp(self): + super(SkillSummaryTests, self).setUp() + current_time = datetime.datetime.utcnow() + time_in_millisecs = utils.get_time_in_millisecs(current_time) + self.skill_summary_dict = { + 'id': 'skill_id', + 'description': 'description', + 'language_code': 'en', + 'version': 1, + 'misconception_count': 1, + 'worked_examples_count': 1, + 'skill_model_created_on': time_in_millisecs, + 'skill_model_last_updated': time_in_millisecs + } + + self.skill_summary = skill_domain.SkillSummary( + 'skill_id', 'description', 'en', 1, 1, 1, + current_time, current_time) + + def test_skill_summary_gets_created(self): + self.assertEqual( + self.skill_summary.to_dict(), self.skill_summary_dict) + + def test_validation_passes_with_valid_properties(self): + self.skill_summary.validate() + + def test_validation_fails_with_invalid_description(self): + self.skill_summary.description = 0 + with self.assertRaisesRegexp( + utils.ValidationError, 'Description should be a string.'): + self.skill_summary.validate() + + def test_validation_fails_with_empty_description(self): + self.skill_summary.description = '' + with self.assertRaisesRegexp( + utils.ValidationError, 'Description field should not be empty'): + self.skill_summary.validate() + + def test_validation_fails_with_invalid_language_code(self): + self.skill_summary.language_code = 0 + with self.assertRaisesRegexp( + utils.ValidationError, + 'Expected language code to be a string, received 0'): + self.skill_summary.validate() + + def test_validation_fails_with_unallowed_language_code(self): + self.skill_summary.language_code = 'invalid' + with self.assertRaisesRegexp( + utils.ValidationError, 'Invalid language code: invalid'): + self.skill_summary.validate() + + def test_validation_fails_with_invalid_misconception_count(self): + self.skill_summary.misconception_count = '10' + with self.assertRaisesRegexp( + utils.ValidationError, + 'Expected misconception_count to be an int, received \'10\''): + self.skill_summary.validate() + + def test_validation_fails_with_negative_misconception_count(self): + self.skill_summary.misconception_count = -1 + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Expected misconception_count to be non-negative, ' + 'received \'-1\'')): + self.skill_summary.validate() + + def test_validation_fails_with_invalid_worked_examples_count(self): + self.skill_summary.worked_examples_count = '10' + with self.assertRaisesRegexp( + utils.ValidationError, + 'Expected worked_examples_count to be an int, received \'10\''): + self.skill_summary.validate() + + def test_validation_fails_with_negative_worked_examples_count(self): + self.skill_summary.worked_examples_count = -1 + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Expected worked_examples_count to be non-negative, ' + 'received \'-1\'')): + self.skill_summary.validate() diff --git a/core/domain/skill_services.py b/core/domain/skill_services.py index 055fd2324b9df..1888e013b15fe 100644 --- a/core/domain/skill_services.py +++ b/core/domain/skill_services.py @@ -428,7 +428,7 @@ def apply_change_list(skill_id, change_list, committer_id): skill_domain.SKILL_CONTENTS_PROPERTY_WORKED_EXAMPLES): skill.update_worked_examples(change.new_value) elif change.cmd == skill_domain.CMD_ADD_SKILL_MISCONCEPTION: - skill.add_misconception(change.new_value) + skill.add_misconception(change.new_misconception_dict) elif change.cmd == skill_domain.CMD_DELETE_SKILL_MISCONCEPTION: skill.delete_misconception(change.misconception_id) elif (change.cmd == @@ -445,6 +445,8 @@ def apply_change_list(skill_id, change_list, committer_id): skill_domain.SKILL_MISCONCEPTIONS_PROPERTY_FEEDBACK): skill.update_misconception_feedback( change.misconception_id, change.new_value) + else: + raise Exception('Invalid change dict.') elif (change.cmd == skill_domain.CMD_MIGRATE_CONTENTS_SCHEMA_TO_LATEST_VERSION or change.cmd == diff --git a/core/domain/skill_services_test.py b/core/domain/skill_services_test.py index 041ca4a732fbe..7c490076cd56f 100644 --- a/core/domain/skill_services_test.py +++ b/core/domain/skill_services_test.py @@ -38,12 +38,13 @@ class SkillServicesUnitTests(test_utils.GenericTestBase): def setUp(self): super(SkillServicesUnitTests, self).setUp() skill_contents = skill_domain.SkillContents( - state_domain.SubtitledHtml('1', 'Explanation'), [ - state_domain.SubtitledHtml('2', 'Example 1')], + state_domain.SubtitledHtml('1', 'Explanation
'), [ + state_domain.SubtitledHtml('2', 'Example 1
')], {'1': {}, '2': {}}, state_domain.WrittenTranslations.from_dict( {'translations_mapping': {'1': {}, '2': {}}})) misconceptions = [skill_domain.Misconception( - self.MISCONCEPTION_ID_1, 'name', 'description', 'default_feedback')] + self.MISCONCEPTION_ID_1, 'name', 'description
', + 'default_feedback
')] self.SKILL_ID = skill_services.get_new_skill_id() self.signup('a@example.com', 'A') @@ -106,15 +107,15 @@ def test_get_skill_descriptions_by_ids(self): self.save_new_skill( 'skill_2', self.USER_ID, 'Description 2', misconceptions=[], skill_contents=skill_domain.SkillContents( - state_domain.SubtitledHtml('1', 'Explanation'), [ - state_domain.SubtitledHtml('2', 'Example 1')], + state_domain.SubtitledHtml('1', 'Explanation
'), [ + state_domain.SubtitledHtml('2', 'Example 1
')], {'1': {}, '2': {}}, state_domain.WrittenTranslations.from_dict( {'translations_mapping': {'1': {}, '2': {}}}))) self.save_new_skill( 'skill_3', self.USER_ID, 'Description 3', misconceptions=[], skill_contents=skill_domain.SkillContents( - state_domain.SubtitledHtml('1', 'Explanation'), [ - state_domain.SubtitledHtml('2', 'Example 1')], + state_domain.SubtitledHtml('1', 'Explanation
'), [ + state_domain.SubtitledHtml('2', 'Example 1
')], {'1': {}, '2': {}}, state_domain.WrittenTranslations.from_dict( {'translations_mapping': {'1': {}, '2': {}}}))) @@ -172,15 +173,15 @@ def test_update_skill(self): 'new_misconception_dict': { 'id': self.skill.next_misconception_id, 'name': 'test name', - 'notes': 'test notes', - 'feedback': 'test feedback' + 'notes': 'test notes
', + 'feedback': 'test feedback
' } }), skill_domain.SkillChange({ 'cmd': skill_domain.CMD_UPDATE_SKILL_MISCONCEPTIONS_PROPERTY, 'property_name': ( skill_domain.SKILL_MISCONCEPTIONS_PROPERTY_NAME), - 'id': self.skill.next_misconception_id, + 'misconception_id': self.skill.next_misconception_id, 'old_value': 'test name', 'new_value': 'Name' }) @@ -201,7 +202,6 @@ def test_merge_skill(self): 'cmd': skill_domain.CMD_UPDATE_SKILL_PROPERTY, 'property_name': ( skill_domain.SKILL_PROPERTY_SUPERSEDING_SKILL_ID), - 'id': 0, 'old_value': '', 'new_value': 'TestSkillId' }), @@ -209,7 +209,6 @@ def test_merge_skill(self): 'cmd': skill_domain.CMD_UPDATE_SKILL_PROPERTY, 'property_name': ( skill_domain.SKILL_PROPERTY_ALL_QUESTIONS_MERGED), - 'id': 0, 'old_value': None, 'new_value': False }) @@ -235,7 +234,6 @@ def test_set_merge_complete_for_skill(self): 'cmd': skill_domain.CMD_UPDATE_SKILL_PROPERTY, 'property_name': ( skill_domain.SKILL_PROPERTY_ALL_QUESTIONS_MERGED), - 'id': 0, 'old_value': False, 'new_value': True }) @@ -256,7 +254,6 @@ def test_get_merged_skill_ids(self): 'cmd': skill_domain.CMD_UPDATE_SKILL_PROPERTY, 'property_name': ( skill_domain.SKILL_PROPERTY_SUPERSEDING_SKILL_ID), - 'id': 0, 'old_value': '', 'new_value': 'TestSkillId' }) @@ -280,15 +277,15 @@ def test_get_unpublished_skill_rights_by_creator(self): self.save_new_skill( 'skill_a', self.user_id_admin, 'Description A', misconceptions=[], skill_contents=skill_domain.SkillContents( - state_domain.SubtitledHtml('1', 'Explanation'), [ - state_domain.SubtitledHtml('2', 'Example 1')], + state_domain.SubtitledHtml('1', 'Explanation
'), [ + state_domain.SubtitledHtml('2', 'Example 1
')], {'1': {}, '2': {}}, state_domain.WrittenTranslations.from_dict( {'translations_mapping': {'1': {}, '2': {}}}))) self.save_new_skill( 'skill_b', self.user_id_admin, 'Description B', misconceptions=[], skill_contents=skill_domain.SkillContents( - state_domain.SubtitledHtml('1', 'Explanation'), [ - state_domain.SubtitledHtml('2', 'Example 1')], + state_domain.SubtitledHtml('1', 'Explanation
'), [ + state_domain.SubtitledHtml('2', 'Example 1
')], {'1': {}, '2': {}}, state_domain.WrittenTranslations.from_dict( {'translations_mapping': {'1': {}, '2': {}}}))) @@ -307,15 +304,15 @@ def test_get_multi_skills(self): self.save_new_skill( 'skill_a', self.user_id_admin, 'Description A', misconceptions=[], skill_contents=skill_domain.SkillContents( - state_domain.SubtitledHtml('1', 'Explanation'), [ - state_domain.SubtitledHtml('2', 'Example 1')], + state_domain.SubtitledHtml('1', 'Explanation
'), [ + state_domain.SubtitledHtml('2', 'Example 1
')], {'1': {}, '2': {}}, state_domain.WrittenTranslations.from_dict( {'translations_mapping': {'1': {}, '2': {}}}))) self.save_new_skill( 'skill_b', self.user_id_admin, 'Description B', misconceptions=[], skill_contents=skill_domain.SkillContents( - state_domain.SubtitledHtml('1', 'Explanation'), [ - state_domain.SubtitledHtml('2', 'Example 1')], + state_domain.SubtitledHtml('1', 'Explanation
'), [ + state_domain.SubtitledHtml('2', 'Example 1
')], {'1': {}, '2': {}}, state_domain.WrittenTranslations.from_dict( {'translations_mapping': {'1': {}, '2': {}}}))) @@ -499,17 +496,10 @@ def test_normal_user_cannot_update_skill_property(self): self.user_id_a, self.SKILL_ID, changelist, 'Change description.') - def test_cannot_update_skill_property_with_invalid_change_dict(self): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): - skill_domain.SkillChange({ - 'cmd': skill_domain.CMD_UPDATE_SKILL_PROPERTY, - 'property_name': 'invalid property_name' - }) - def test_update_skill_explanation(self): skill = skill_services.get_skill_by_id(self.SKILL_ID) - old_explanation = {'content_id': '1', 'html': 'Explanation'} - new_explanation = {'content_id': '1', 'html': 'New explanation'} + old_explanation = {'content_id': '1', 'html': 'Explanation
'} + new_explanation = {'content_id': '1', 'html': 'New explanation
'} self.assertEqual( skill.skill_contents.explanation.to_dict(), old_explanation) @@ -532,8 +522,8 @@ def test_update_skill_explanation(self): def test_update_skill_worked_examples(self): skill = skill_services.get_skill_by_id(self.SKILL_ID) - old_worked_examples = {'content_id': '2', 'html': 'Example 1'} - new_worked_examples = {'content_id': '2', 'html': 'Example 2'} + old_worked_examples = {'content_id': '2', 'html': 'Example 1
'} + new_worked_examples = {'content_id': '2', 'html': 'Example 2
'} self.assertEqual(len(skill.skill_contents.worked_examples), 1) self.assertEqual( @@ -558,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( - self): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): - skill_domain.SkillChange({ - 'cmd': skill_domain.CMD_UPDATE_SKILL_CONTENTS_PROPERTY, - 'property_name': 'invalid property_name' - }) - def test_delete_skill_misconception(self): skill = skill_services.get_skill_by_id(self.SKILL_ID) @@ -575,7 +557,7 @@ def test_delete_skill_misconception(self): changelist = [ skill_domain.SkillChange({ 'cmd': skill_domain.CMD_DELETE_SKILL_MISCONCEPTION, - 'id': self.MISCONCEPTION_ID_1, + 'misconception_id': self.MISCONCEPTION_ID_1, }) ] @@ -590,16 +572,16 @@ def test_update_skill_misconception_notes(self): self.assertEqual(len(skill.misconceptions), 1) self.assertEqual(skill.misconceptions[0].id, self.MISCONCEPTION_ID_1) - self.assertEqual(skill.misconceptions[0].notes, 'description') + self.assertEqual(skill.misconceptions[0].notes, 'description
') changelist = [ skill_domain.SkillChange({ 'cmd': skill_domain.CMD_UPDATE_SKILL_MISCONCEPTIONS_PROPERTY, 'property_name': ( skill_domain.SKILL_MISCONCEPTIONS_PROPERTY_NOTES), - 'id': self.MISCONCEPTION_ID_1, - 'old_value': 'description', - 'new_value': 'new description' + 'misconception_id': self.MISCONCEPTION_ID_1, + 'old_value': 'description
', + 'new_value': 'new description
' }) ] @@ -610,23 +592,25 @@ def test_update_skill_misconception_notes(self): self.assertEqual(len(skill.misconceptions), 1) self.assertEqual(skill.misconceptions[0].id, self.MISCONCEPTION_ID_1) - self.assertEqual(skill.misconceptions[0].notes, 'new description') + self.assertEqual( + skill.misconceptions[0].notes, 'new description
') def test_update_skill_misconception_feedback(self): skill = skill_services.get_skill_by_id(self.SKILL_ID) self.assertEqual(len(skill.misconceptions), 1) self.assertEqual(skill.misconceptions[0].id, self.MISCONCEPTION_ID_1) - self.assertEqual(skill.misconceptions[0].feedback, 'default_feedback') + self.assertEqual( + skill.misconceptions[0].feedback, 'default_feedback
') changelist = [ skill_domain.SkillChange({ 'cmd': skill_domain.CMD_UPDATE_SKILL_MISCONCEPTIONS_PROPERTY, 'property_name': ( skill_domain.SKILL_MISCONCEPTIONS_PROPERTY_FEEDBACK), - 'id': self.MISCONCEPTION_ID_1, - 'old_value': 'default_feedback', - 'new_value': 'new feedback' + 'misconception_id': self.MISCONCEPTION_ID_1, + 'old_value': 'default_feedback
', + 'new_value': 'new feedback
' }) ] @@ -637,7 +621,8 @@ def test_update_skill_misconception_feedback(self): self.assertEqual(len(skill.misconceptions), 1) self.assertEqual(skill.misconceptions[0].id, self.MISCONCEPTION_ID_1) - self.assertEqual(skill.misconceptions[0].feedback, 'new feedback') + self.assertEqual( + skill.misconceptions[0].feedback, 'new feedback
') def test_cannot_update_skill_with_invalid_change_list(self): observed_log_messages = [] @@ -659,16 +644,12 @@ def _mock_logging_function(msg, *args): observed_log_messages[0], 'AttributeError \'str\' object has no ' 'attribute \'cmd\' %s invalid_change_list' % self.SKILL_ID) - def test_cannot_update_skill_with_invalid_cmd(self): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): - skill_domain.SkillChange({'cmd': 'invalid_cmd'}) - def test_cannot_update_misconception_name_with_invalid_id(self): changelist = [skill_domain.SkillChange({ 'cmd': skill_domain.CMD_UPDATE_SKILL_MISCONCEPTIONS_PROPERTY, 'property_name': ( skill_domain.SKILL_MISCONCEPTIONS_PROPERTY_NAME), - 'id': 'invalid_id', + 'misconception_id': 'invalid_id', 'old_value': 'test name', 'new_value': 'Name' })] @@ -682,7 +663,7 @@ def test_cannot_update_misconception_name_with_invalid_id(self): def test_cannot_delete_misconception_with_invalid_id(self): changelist = [skill_domain.SkillChange({ 'cmd': skill_domain.CMD_DELETE_SKILL_MISCONCEPTION, - 'id': 'invalid_id' + 'misconception_id': 'invalid_id' })] with self.assertRaisesRegexp( @@ -695,7 +676,7 @@ def test_cannot_update_misconception_notes_with_invalid_id(self): 'cmd': skill_domain.CMD_UPDATE_SKILL_MISCONCEPTIONS_PROPERTY, 'property_name': ( skill_domain.SKILL_MISCONCEPTIONS_PROPERTY_NOTES), - 'id': 'invalid_id', + 'misconception_id': 'invalid_id', 'old_value': 'description', 'new_value': 'new description' })] @@ -711,7 +692,7 @@ def test_cannot_update_misconception_feedback_with_invalid_id(self): 'cmd': skill_domain.CMD_UPDATE_SKILL_MISCONCEPTIONS_PROPERTY, 'property_name': ( skill_domain.SKILL_MISCONCEPTIONS_PROPERTY_FEEDBACK), - 'id': 'invalid_id', + 'misconception_id': 'invalid_id', 'old_value': 'default_feedback', 'new_value': 'new feedback' })] diff --git a/core/domain/state_domain.py b/core/domain/state_domain.py index 2a04d175e55aa..94c181ab4949b 100644 --- a/core/domain/state_domain.py +++ b/core/domain/state_domain.py @@ -22,6 +22,7 @@ from constants import constants from core.domain import customization_args_util from core.domain import html_cleaner +from core.domain import html_validation_service from core.domain import interaction_registry from core.domain import param_domain import feconf @@ -1288,7 +1289,13 @@ def __init__(self, content_id, html): a way as to contain a restricted set of HTML tags. """ self.content_id = content_id - self.html = html_cleaner.clean(html) + # The initial clean up of html by converting it to ckeditor format + # is required since user may copy and paste some stuff in the rte + # which is not a valid ckeditor html string but can be converted + # to a valid ckeditor string without errors. This initial clean up + # ensures that validation will not fail in such cases. + self.html = html_validation_service.convert_to_ckeditor( + html_cleaner.clean(html)) self.validate() def to_dict(self): @@ -1328,12 +1335,25 @@ def validate(self): 'Expected content id to be a string, received %s' % self.content_id) - # TODO(sll): Add HTML sanitization checking. - # TODO(sll): Validate customization args for rich-text components. if not isinstance(self.html, basestring): raise utils.ValidationError( 'Invalid content HTML: %s' % self.html) + err_dict = html_validation_service.validate_rte_format( + [self.html], feconf.RTE_FORMAT_CKEDITOR) + for key in err_dict: + if err_dict[key]: + raise utils.ValidationError( + 'Invalid html: %s for rte with invalid tags and ' + 'strings: %s' % (self.html, err_dict)) + + err_dict = html_validation_service.validate_customization_args([ + self.html]) + if err_dict: + raise utils.ValidationError( + 'Invalid html: %s due to errors in customization_args: %s' % ( + self.html, err_dict)) + def to_html(self, params): """Exports this SubtitledHTML object to an HTML string. The HTML is parameterized using the parameters in `params`. diff --git a/core/domain/state_domain_test.py b/core/domain/state_domain_test.py index 1d535ddae6152..7570e5192ecb4 100644 --- a/core/domain/state_domain_test.py +++ b/core/domain/state_domain_test.py @@ -392,9 +392,10 @@ def test_convert_html_fields_in_state(self): state_dict, add_dimensions_to_image_tags), state_dict_with_image_dimensions) - def test_subtitled_html_validation(self): - """Test validation of subtitled HTML.""" - subtitled_html = state_domain.SubtitledHtml('content_id', 'some html') + def test_subtitled_html_validation_with_invalid_html_type(self): + """Test validation of subtitled HTML with invalid html type.""" + subtitled_html = state_domain.SubtitledHtml( + 'content_id', 'some html
') subtitled_html.validate() with self.assertRaisesRegexp( @@ -403,6 +404,45 @@ def test_subtitled_html_validation(self): with self.swap(subtitled_html, 'html', 20): subtitled_html.validate() + def test_subtitled_html_validation_with_invalid_html_for_rte(self): + """Test validation of subtitled HTML with invalid html for rte.""" + subtitled_html = state_domain.SubtitledHtml( + 'content_id', 'some html
') + subtitled_html.validate() + + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Invalid html: Test for rte with invalid tags and ' + 'strings: {\'invalidTags\': \\[u\'a\'], ' + '\'strings\': \\[\'Test\']}')): + with self.swap(subtitled_html, 'html', 'Test'): + subtitled_html.validate() + + def test_subtitled_html_validation_with_invalid_customization_args(self): + """Test validation of subtitled HTML with invalid customization args.""" + subtitled_html = state_domain.SubtitledHtml( + 'content_id', 'some html
') + subtitled_html.validate() + + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Invalid html:some html
') + subtitled_html.validate() with self.assertRaisesRegexp( utils.ValidationError, 'Expected content id to be a string, ' + 'received 20'): @@ -480,7 +520,7 @@ def test_hints_validation(self): hints_list.append({ 'hint_content': { 'content_id': 'hint_1', - 'html': 'hint one' + 'html': 'hint one
' }, }) init_state.update_interaction_hints(hints_list) @@ -490,7 +530,7 @@ def test_hints_validation(self): 'correct_answer': 'helloworld!', 'explanation': { 'content_id': 'solution', - 'html': 'hello_world is a string' + 'html': 'hello_world is a string
' }, } @@ -500,19 +540,19 @@ def test_hints_validation(self): hints_list.append({ 'hint_content': { 'content_id': 'hint_2', - 'html': 'new hint' + 'html': 'new hint
' } }) init_state.update_interaction_hints(hints_list) self.assertEqual( init_state.interaction.hints[1].hint_content.html, - 'new hint') + 'new hint
') hints_list.append({ 'hint_content': { 'content_id': 'hint_3', - 'html': 'hint three' + 'html': 'hint three
' } }) init_state.update_interaction_hints(hints_list) @@ -562,7 +602,7 @@ def test_solution_validation(self): 'correct_answer': 'hello_world!', 'explanation': { 'content_id': 'solution', - 'html': 'hello_world is a string' + 'html': 'hello_world is a string
' } } init_state.update_interaction_solution(solution) diff --git a/core/domain/stats_jobs_continuous_test.py b/core/domain/stats_jobs_continuous_test.py index a10d5b823544d..fbd6653fbeee3 100644 --- a/core/domain/stats_jobs_continuous_test.py +++ b/core/domain/stats_jobs_continuous_test.py @@ -519,7 +519,7 @@ def test_uses_old_answers_if_updated_exploration_has_same_interaction(self): 'property_name': exp_domain.STATE_PROPERTY_CONTENT, 'new_value': { 'content_id': 'content', - 'html': 'New content' + 'html': 'New content
' }, })], 'Change state content') @@ -635,7 +635,7 @@ def test_answers_across_multiple_exp_versions_different_interactions(self): 'property_name': exp_domain.STATE_PROPERTY_CONTENT, 'new_value': { 'content_id': 'content', - 'html': 'New content description' + 'html': 'New content description
' } })], 'Change content description') diff --git a/core/domain/stats_services_test.py b/core/domain/stats_services_test.py index 6e36fa119a63e..15ac3f3d38c11 100644 --- a/core/domain/stats_services_test.py +++ b/core/domain/stats_services_test.py @@ -2023,7 +2023,7 @@ def test_retrieves_vis_info_across_multiple_exploration_versions(self): self._record_answer('Answer B') # Change the exploration version and submit a new answer. - self._change_state_content('New content') + self._change_state_content('New content
') self._record_answer('Answer A') self._run_answer_summaries_aggregator() diff --git a/core/domain/story_domain.py b/core/domain/story_domain.py index 59ac476a9a021..41d0927245c8a 100644 --- a/core/domain/story_domain.py +++ b/core/domain/story_domain.py @@ -84,6 +84,7 @@ class StoryChange(change_domain.BaseChange): to_version) - 'create_new' (with title) """ + # The allowed list of story properties which can be used in # update_story_property command. STORY_PROPERTIES = ( @@ -1080,7 +1081,12 @@ def validate(self): if not isinstance(self.node_count, int): raise utils.ValidationError( - 'Expected node_count to be a int, received \'%s\'' % ( + 'Expected node_count to be an int, received \'%s\'' % ( + self.node_count)) + + if self.node_count < 0: + raise utils.ValidationError( + 'Expected node_count to be non-negative, received \'%s\'' % ( self.node_count)) if not isinstance(self.language_code, basestring): diff --git a/core/domain/story_domain_test.py b/core/domain/story_domain_test.py index 06262091efd1c..ba3e0cdae5fca 100644 --- a/core/domain/story_domain_test.py +++ b/core/domain/story_domain_test.py @@ -908,7 +908,15 @@ def test_validation_fails_with_invalid_node_count(self): self.story_summary.node_count = '10' with self.assertRaisesRegexp( utils.ValidationError, - 'Expected node_count to be a int, received \'10\''): + 'Expected node_count to be an int, received \'10\''): + self.story_summary.validate() + + def test_validation_fails_with_negative_node_count(self): + self.story_summary.node_count = -1 + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Expected node_count to be non-negative, ' + 'received \'-1\'')): self.story_summary.validate() def test_validation_fails_with_invalid_language_code(self): diff --git a/core/domain/subtopic_page_domain.py b/core/domain/subtopic_page_domain.py index ae41507af2699..cd2c7eee2979e 100644 --- a/core/domain/subtopic_page_domain.py +++ b/core/domain/subtopic_page_domain.py @@ -16,9 +16,8 @@ """Domain objects for the pages for subtopics, and related models.""" -import copy - from constants import constants +from core.domain import change_domain from core.domain import state_domain from core.platform import models import feconf @@ -30,73 +29,39 @@ SUBTOPIC_PAGE_PROPERTY_PAGE_CONTENTS_AUDIO = 'page_contents_audio' SUBTOPIC_PAGE_PROPERTY_PAGE_WRITTEN_TRANSLATIONS = 'page_written_translations' - -CMD_ADD_SUBTOPIC = 'add_subtopic' CMD_CREATE_NEW = 'create_new' -CMD_DELETE_SUBTOPIC = 'delete_subtopic' # These take additional 'property_name' and 'new_value' parameters and, # optionally, 'old_value'. CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY = 'update_subtopic_page_property' -class SubtopicPageChange(object): - """Domain object for changes made to subtopic_page object.""" +class SubtopicPageChange(change_domain.BaseChange): + """Domain object for changes made to subtopic_page object. + The allowed commands, together with the attributes: + - 'create_new' (with topic_id, subtopic_id) + - 'update_subtopic_page_property' ( + with property_name, new_value, old_value, subtopic_id). + """ + + # The allowed list of subtopic page properties which can be used in + # update_subtopic_page_property command. SUBTOPIC_PAGE_PROPERTIES = ( SUBTOPIC_PAGE_PROPERTY_PAGE_CONTENTS_HTML, SUBTOPIC_PAGE_PROPERTY_PAGE_CONTENTS_AUDIO, SUBTOPIC_PAGE_PROPERTY_PAGE_WRITTEN_TRANSLATIONS) - OPTIONAL_CMD_ATTRIBUTE_NAMES = [ - 'property_name', 'new_value', 'old_value', 'name', 'subtopic_id', - 'topic_id' - ] - - def __init__(self, change_dict): - """Initialize a SubtopicPageChange object from a dict. - - Args: - change_dict: dict. Represents a command. It should have a 'cmd' - key, and one or more other keys. The keys depend on what the - value for 'cmd' is. The possible values for 'cmd' are listed - below, together with the other keys in the dict: - - 'update_topic_property' (with property_name, new_value - and old_value) - - Raises: - Exception: The given change dict is not valid. - """ - if 'cmd' not in change_dict: - raise Exception('Invalid change_dict: %s' % change_dict) - self.cmd = change_dict['cmd'] - - if self.cmd == CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY: - if (change_dict['property_name'] not in - self.SUBTOPIC_PAGE_PROPERTIES): - raise Exception('Invalid change_dict: %s' % change_dict) - self.property_name = change_dict['property_name'] - self.new_value = copy.deepcopy(change_dict['new_value']) - self.old_value = copy.deepcopy(change_dict['old_value']) - self.id = change_dict['subtopic_id'] - elif self.cmd == CMD_CREATE_NEW: - self.topic_id = change_dict['topic_id'] - else: - raise Exception('Invalid change_dict: %s' % change_dict) - - def to_dict(self): - """Returns a dict representing the SubtopicPageChange domain object. - - Returns: - A dict, mapping all fields of SubtopicPageChange instance. - """ - subtopic_page_change_dict = {} - subtopic_page_change_dict['cmd'] = self.cmd - for attribute_name in self.OPTIONAL_CMD_ATTRIBUTE_NAMES: - if hasattr(self, attribute_name): - subtopic_page_change_dict[attribute_name] = getattr( - self, attribute_name) - - return subtopic_page_change_dict + ALLOWED_COMMANDS = [{ + 'name': CMD_CREATE_NEW, + 'required_attribute_names': ['topic_id', 'subtopic_id'], + 'optional_attribute_names': [] + }, { + 'name': CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY, + 'required_attribute_names': [ + 'property_name', 'new_value', 'old_value', 'subtopic_id'], + 'optional_attribute_names': [], + 'allowed_values': {'property_name': SUBTOPIC_PAGE_PROPERTIES} + }] class SubtopicPageContents(object): diff --git a/core/domain/subtopic_page_domain_test.py b/core/domain/subtopic_page_domain_test.py index 9580b43e16fee..7cc270a75d196 100644 --- a/core/domain/subtopic_page_domain_test.py +++ b/core/domain/subtopic_page_domain_test.py @@ -159,7 +159,7 @@ def test_update_html(self): 'topic_id': 'topic_id', 'page_contents': { 'subtitled_html': { - 'html': 'hello world', + 'html': 'hello world
', 'content_id': 'content' }, 'content_ids_to_audio_translations': { @@ -177,7 +177,7 @@ def test_update_html(self): 'version': 0 } self.subtopic_page.update_page_contents_html({ - 'html': 'hello world', + 'html': 'hello world
', 'content_id': 'content' }) self.assertEqual(self.subtopic_page.to_dict(), @@ -270,7 +270,7 @@ def test_content_ids_to_audio_translations_validation(self): def test_to_and_from_dict(self): subtopic_page_contents_dict = { 'subtitled_html': { - 'html': 'test', + 'html': 'test
', 'content_id': 'content' }, 'content_ids_to_audio_translations': { @@ -298,3 +298,92 @@ def test_to_and_from_dict(self): subtopic_page_contents_dict)) self.assertEqual(subtopic_page_contents.to_dict(), subtopic_page_contents_dict) + + +class SubtopicPageChangeTests(test_utils.GenericTestBase): + + def test_subtopic_page_change_object_with_missing_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, 'Missing cmd key in change dict'): + subtopic_page_domain.SubtopicPageChange({'invalid': 'data'}) + + def test_subtopic_page_change_object_with_invalid_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, 'Command invalid is not allowed'): + subtopic_page_domain.SubtopicPageChange({'cmd': 'invalid'}) + + def test_subtopic_page_change_object_with_missing_attribute_in_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'The following required attributes are missing: ' + 'new_value, old_value')): + subtopic_page_domain.SubtopicPageChange({ + 'cmd': 'update_subtopic_page_property', + 'property_name': 'page_contents_html
', + }) + + def test_subtopic_page_change_object_with_extra_attribute_in_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'The following extra attributes are present: invalid')): + subtopic_page_domain.SubtopicPageChange({ + 'cmd': 'create_new', + 'topic_id': 'topic_id', + 'subtopic_id': 'subtopic_id', + 'invalid': 'invalid' + }) + + def test_subtopic_page_change_object_with_invalid_subtopic_page_property( + self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Value for property_name in cmd update_subtopic_page_property: ' + 'invalid is not allowed')): + subtopic_page_domain.SubtopicPageChange({ + 'cmd': 'update_subtopic_page_property', + 'subtopic_id': 'subtopic_id', + 'property_name': 'invalid', + 'old_value': 'old_value', + 'new_value': 'new_value', + }) + + def test_subtopic_page_change_object_with_update_subtopic_page_property( + self): + subtopic_page_change_object = subtopic_page_domain.SubtopicPageChange({ + 'cmd': 'update_subtopic_page_property', + 'subtopic_id': 'subtopic_id', + 'property_name': 'page_contents_html', + 'new_value': 'new_value', + 'old_value': 'old_value' + }) + + self.assertEqual( + subtopic_page_change_object.cmd, 'update_subtopic_page_property') + self.assertEqual(subtopic_page_change_object.subtopic_id, 'subtopic_id') + self.assertEqual( + subtopic_page_change_object.property_name, 'page_contents_html') + self.assertEqual(subtopic_page_change_object.new_value, 'new_value') + self.assertEqual(subtopic_page_change_object.old_value, 'old_value') + + def test_subtopic_page_change_object_with_create_new(self): + subtopic_page_change_object = ( + subtopic_page_domain.SubtopicPageChange({ + 'cmd': 'create_new', + 'topic_id': 'topic_id', + 'subtopic_id': 'subtopic_id' + })) + + self.assertEqual(subtopic_page_change_object.cmd, 'create_new') + self.assertEqual(subtopic_page_change_object.topic_id, 'topic_id') + self.assertEqual(subtopic_page_change_object.subtopic_id, 'subtopic_id') + + def test_to_dict(self): + subtopic_page_change_dict = { + 'cmd': 'create_new', + 'topic_id': 'topic_id', + 'subtopic_id': 'subtopic_id' + } + subtopic_page_change_object = subtopic_page_domain.SubtopicPageChange( + subtopic_page_change_dict) + self.assertEqual( + subtopic_page_change_object.to_dict(), subtopic_page_change_dict) diff --git a/core/domain/subtopic_page_services_test.py b/core/domain/subtopic_page_services_test.py index 91fb867d9cfcd..d94776f74fca6 100644 --- a/core/domain/subtopic_page_services_test.py +++ b/core/domain/subtopic_page_services_test.py @@ -115,7 +115,7 @@ def test_get_subtopic_page_contents_by_id(self): 'content_ids_to_audio_translations': content_ids_to_audio_translations_dict, 'subtitled_html': { - 'content_id': 'content', 'html': 'hello world' + 'content_id': 'content', 'html': 'hello world
' }, 'written_translations': { 'translations_mapping': { @@ -124,7 +124,7 @@ def test_get_subtopic_page_contents_by_id(self): } } self.subtopic_page.update_page_contents_html({ - 'html': 'hello world', + 'html': 'hello world
', 'content_id': 'content' }) self.subtopic_page.update_page_contents_audio( diff --git a/core/domain/suggestion_registry_test.py b/core/domain/suggestion_registry_test.py index 6a362d1d9b3fe..11c9a234ee8f1 100644 --- a/core/domain/suggestion_registry_test.py +++ b/core/domain/suggestion_registry_test.py @@ -552,7 +552,7 @@ def test_pre_update_validate_change_new_value(self): self.reviewer_id, expected_suggestion_dict['change'], expected_suggestion_dict['score_category'], self.fake_date) new_content = state_domain.SubtitledHtml( - 'content', 'new suggestion html').to_dict() + 'content', 'new suggestion html
').to_dict() suggestion.change.new_value = new_content @@ -759,8 +759,13 @@ def test_validate_change_question_state_data_schema_version(self): suggestion.validate() - suggestion.change.question_dict[ - 'question_state_data_schema_version'] = 0 + # We are not setting value in suggestion.change.question_dict + # directly since pylint produces unsupported-assignment-operation + # error. The detailed analysis for the same can be checked + # in this issue: https://github.com/oppia/oppia/issues/7008. + question_dict = suggestion.change.question_dict + question_dict['question_state_data_schema_version'] = 0 + suggestion.change.question_dict = question_dict with self.assertRaisesRegexp( Exception, @@ -809,8 +814,13 @@ def test_pre_accept_validate_change_question_state_data_schema_version( suggestion.pre_accept_validate() - suggestion.change.question_dict[ - 'question_state_data_schema_version'] = 1 + # We are not setting value in suggestion.change.question_dict + # directly since pylint produces unsupported-assignment-operation + # error. The detailed analysis for the same can be checked + # in this issue: https://github.com/oppia/oppia/issues/7008. + question_dict = suggestion.change.question_dict + question_dict['question_state_data_schema_version'] = 1 + suggestion.change.question_dict = question_dict with self.assertRaisesRegexp( Exception, 'Question state schema version is not up to date.'): diff --git a/core/domain/suggestion_services_test.py b/core/domain/suggestion_services_test.py index 54ee2dc3d75de..92ffb94bea46c 100644 --- a/core/domain/suggestion_services_test.py +++ b/core/domain/suggestion_services_test.py @@ -256,7 +256,7 @@ def test_accept_suggestion_and_send_email_to_author(self): self.author_id, self.target_id, change_list, 'Add state.') new_suggestion_content = state_domain.SubtitledHtml( - 'content', 'new suggestion content html').to_dict() + 'content', 'new suggestion content html
').to_dict() change_dict = { 'cmd': exp_domain.CMD_EDIT_STATE_PROPERTY, 'property_name': exp_domain.STATE_PROPERTY_CONTENT, @@ -833,7 +833,7 @@ def setUp(self): ['TextInput'], category='Algebra')) self.old_content = state_domain.SubtitledHtml( - 'content', 'old content').to_dict() + 'content', 'old content
').to_dict() recorded_voiceovers_dict = { 'voiceovers_mapping': { 'content': { @@ -860,7 +860,7 @@ def setUp(self): rights_manager.ROLE_EDITOR) self.new_content = state_domain.SubtitledHtml( - 'content', 'new content').to_dict() + 'content', 'new content
').to_dict() self.change = { 'cmd': exp_domain.CMD_EDIT_STATE_PROPERTY, @@ -891,7 +891,7 @@ def test_create_and_accept_suggestion(self): self.assertEqual( exploration.states['State 1'].content.html, - 'new content') + 'new content
') self.assertEqual(suggestion.status, suggestion_models.STATUS_ACCEPTED) @@ -918,7 +918,7 @@ def test_create_and_reject_suggestion(self): last_message.text, 'Reject message') self.assertEqual( exploration.states['State 1'].content.html, - 'old content') + 'old content
') self.assertEqual(suggestion.status, suggestion_models.STATUS_REJECTED) @@ -947,7 +947,7 @@ def test_create_and_accept_suggestion_with_message(self): self.assertEqual( exploration.states['State 1'].content.html, - 'new content') + 'new content
') self.assertEqual(suggestion.status, suggestion_models.STATUS_ACCEPTED) diff --git a/core/domain/topic_domain.py b/core/domain/topic_domain.py index cfaa5c38fb100..a6ada72b3ae77 100644 --- a/core/domain/topic_domain.py +++ b/core/domain/topic_domain.py @@ -19,6 +19,7 @@ import copy from constants import constants +from core.domain import change_domain from core.domain import skill_services from core.domain import user_services from core.platform import models @@ -59,163 +60,121 @@ CMD_MIGRATE_SUBTOPIC_SCHEMA_TO_LATEST_VERSION = 'migrate_subtopic_schema_to_latest_version' # pylint: disable=line-too-long -class TopicChange(object): - """Domain object for changes made to topic object.""" +class TopicChange(change_domain.BaseChange): + """Domain object for changes made to topic object. + + The allowed commands, together with the attributes: + - 'add_subtopic' (with title, subtopic_id) + - 'delete_subtopic' (with subtopic_id) + - 'add_uncategorized_skill_id' (with + new_uncategorized_skill_id) + - 'remove_uncategorized_skill_id' (with uncategorized_skill_id) + - 'move_skill_id_to_subtopic' (with old_subtopic_id, + new_subtopic_id and skill_id) + - 'remove_skill_id_from_subtopic' (with subtopic_id and + skill_id) + - 'update_topic_property' (with property_name, new_value + and old_value) + - 'update_subtopic_property' (with subtopic_id, property_name, + new_value and old_value) + - 'migrate_subtopic_schema_to_latest_version' (with + from_version and to_version) + - 'create_new' (with name) + """ + + # The allowed list of topic properties which can be used in + # update_topic_property command. TOPIC_PROPERTIES = ( TOPIC_PROPERTY_NAME, TOPIC_PROPERTY_DESCRIPTION, TOPIC_PROPERTY_CANONICAL_STORY_IDS, TOPIC_PROPERTY_ADDITIONAL_STORY_IDS, TOPIC_PROPERTY_LANGUAGE_CODE) + # The allowed list of subtopic properties which can be used in + # update_subtopic_property command. SUBTOPIC_PROPERTIES = (SUBTOPIC_PROPERTY_TITLE,) - OPTIONAL_CMD_ATTRIBUTE_NAMES = [ - 'property_name', 'new_value', 'old_value', 'name', 'id', 'title', - 'old_subtopic_id', 'new_subtopic_id', 'subtopic_id', 'from_version', - 'to_version' - ] - - def __init__(self, change_dict): - """Initialize a TopicChange object from a dict. - - Args: - change_dict: dict. Represents a command. It should have a 'cmd' - key, and one or more other keys. The keys depend on what the - value for 'cmd' is. The possible values for 'cmd' are listed - below, together with the other keys in the dict: - - 'add_subtopic' (with title) - - 'delete_subtopic' (with subtopic_id) - - 'add_uncategorized_skill_id' (with - new_uncategorized_skill_id) - - 'remove_uncategorized_skill_id' (with subtopic_id - and skill_id) - - 'move_skill_id_to_subtopic' (with old_subtopic_id, - new_subtopic_id and skill_id) - - 'remove_skill_id_from_subtopic' (with subtopic_id and - skill_id) - - 'update_topic_property' (with property_name, new_value - and old_value) - - 'update_subtopic_property' (with property_name, new_value - and old_value) - - 'migrate_subtopic_schema_to_latest_version' (with - from_version and to_version) - - 'create_new' (with name) - - Raises: - Exception: The given change dict is not valid. - """ - if 'cmd' not in change_dict: - raise Exception('Invalid change_dict: %s' % change_dict) - self.cmd = change_dict['cmd'] - - if self.cmd == CMD_ADD_SUBTOPIC: - self.title = change_dict['title'] - self.subtopic_id = change_dict['subtopic_id'] - elif self.cmd == CMD_DELETE_SUBTOPIC: - self.id = change_dict['subtopic_id'] - elif self.cmd == CMD_ADD_UNCATEGORIZED_SKILL_ID: - self.id = change_dict['new_uncategorized_skill_id'] - elif self.cmd == CMD_REMOVE_UNCATEGORIZED_SKILL_ID: - self.id = change_dict['uncategorized_skill_id'] - elif self.cmd == CMD_MOVE_SKILL_ID_TO_SUBTOPIC: - self.old_subtopic_id = change_dict['old_subtopic_id'] - self.new_subtopic_id = change_dict['new_subtopic_id'] - self.skill_id = change_dict['skill_id'] - elif self.cmd == CMD_REMOVE_SKILL_ID_FROM_SUBTOPIC: - self.subtopic_id = change_dict['subtopic_id'] - self.skill_id = change_dict['skill_id'] - elif self.cmd == CMD_UPDATE_TOPIC_PROPERTY: - if change_dict['property_name'] not in self.TOPIC_PROPERTIES: - raise Exception('Invalid change_dict: %s' % change_dict) - self.property_name = change_dict['property_name'] - self.new_value = copy.deepcopy(change_dict['new_value']) - self.old_value = copy.deepcopy(change_dict['old_value']) - elif self.cmd == CMD_UPDATE_SUBTOPIC_PROPERTY: - if change_dict['property_name'] not in self.SUBTOPIC_PROPERTIES: - raise Exception('Invalid change_dict: %s' % change_dict) - self.id = change_dict['subtopic_id'] - self.property_name = change_dict['property_name'] - self.new_value = copy.deepcopy(change_dict['new_value']) - self.old_value = copy.deepcopy(change_dict['old_value']) - elif self.cmd == CMD_MIGRATE_SUBTOPIC_SCHEMA_TO_LATEST_VERSION: - self.from_version = change_dict['from_version'] - self.to_version = change_dict['to_version'] - elif self.cmd == CMD_CREATE_NEW: - self.name = change_dict['name'] - else: - raise Exception('Invalid change_dict: %s' % change_dict) - - def to_dict(self): - """Returns a dict representing the TopicChange domain object. - - Returns: - A dict, mapping all fields of TopicChange instance. - """ - topic_change_dict = {} - topic_change_dict['cmd'] = self.cmd - for attribute_name in self.OPTIONAL_CMD_ATTRIBUTE_NAMES: - if hasattr(self, attribute_name): - topic_change_dict[attribute_name] = getattr( - self, attribute_name) - - return topic_change_dict - - -class TopicRightsChange(object): - """Domain object for changes made to a topic rights object.""" - - OPTIONAL_CMD_ATTRIBUTE_NAMES = [ - 'assignee_id', 'new_role', 'old_role', 'removed_user_id' - ] - - def __init__(self, change_dict): - """Initialize a TopicRightsChange object from a dict. - - Args: - change_dict: dict. Represents a command. It should have a 'cmd' - key, and one or more other keys. The keys depend on what the - value for 'cmd' is. The possible values for 'cmd' are listed - below, together with the other keys in the dict: - - 'change_role' (with assignee_id, new_role and old_role) - - 'create_new' - - 'publish_topic' - - 'unpublish_topic' - - Raises: - Exception: The given change dict is not valid. - """ - if 'cmd' not in change_dict: - raise Exception('Invalid change_dict: %s' % change_dict) - self.cmd = change_dict['cmd'] - - if self.cmd == CMD_CHANGE_ROLE: - self.assignee_id = change_dict['assignee_id'] - self.new_role = change_dict['new_role'] - self.old_role = change_dict['old_role'] - elif self.cmd == CMD_REMOVE_MANAGER_ROLE: - self.removed_user_id = change_dict['removed_user_id'] - elif self.cmd == CMD_CREATE_NEW: - pass - elif self.cmd == CMD_PUBLISH_TOPIC: - pass - elif self.cmd == CMD_UNPUBLISH_TOPIC: - pass - else: - raise Exception('Invalid change_dict: %s' % change_dict) - - def to_dict(self): - """Returns a dict representing the TopicRightsChange domain object. - - Returns: - A dict, mapping all fields of TopicRightsChange instance. - """ - topic_rights_change_dict = {} - topic_rights_change_dict['cmd'] = self.cmd - for attribute_name in self.OPTIONAL_CMD_ATTRIBUTE_NAMES: - if hasattr(self, attribute_name): - topic_rights_change_dict[attribute_name] = getattr( - self, attribute_name) - - return topic_rights_change_dict + ALLOWED_COMMANDS = [{ + 'name': CMD_CREATE_NEW, + 'required_attribute_names': ['name'], + 'optional_attribute_names': [] + }, { + 'name': CMD_ADD_SUBTOPIC, + 'required_attribute_names': ['title', 'subtopic_id'], + 'optional_attribute_names': [] + }, { + 'name': CMD_DELETE_SUBTOPIC, + 'required_attribute_names': ['subtopic_id'], + 'optional_attribute_names': [] + }, { + 'name': CMD_ADD_UNCATEGORIZED_SKILL_ID, + 'required_attribute_names': ['new_uncategorized_skill_id'], + 'optional_attribute_names': [] + }, { + 'name': CMD_REMOVE_UNCATEGORIZED_SKILL_ID, + 'required_attribute_names': ['uncategorized_skill_id'], + 'optional_attribute_names': [], + }, { + 'name': CMD_MOVE_SKILL_ID_TO_SUBTOPIC, + 'required_attribute_names': [ + 'old_subtopic_id', 'new_subtopic_id', 'skill_id'], + 'optional_attribute_names': [], + }, { + 'name': CMD_REMOVE_SKILL_ID_FROM_SUBTOPIC, + 'required_attribute_names': ['subtopic_id', 'skill_id'], + 'optional_attribute_names': [], + }, { + 'name': CMD_UPDATE_SUBTOPIC_PROPERTY, + 'required_attribute_names': [ + 'subtopic_id', 'property_name', 'new_value', 'old_value'], + 'optional_attribute_names': [], + 'allowed_values': {'property_name': SUBTOPIC_PROPERTIES} + }, { + 'name': CMD_UPDATE_TOPIC_PROPERTY, + 'required_attribute_names': ['property_name', 'new_value', 'old_value'], + 'optional_attribute_names': [], + 'allowed_values': {'property_name': TOPIC_PROPERTIES} + }, { + 'name': CMD_MIGRATE_SUBTOPIC_SCHEMA_TO_LATEST_VERSION, + 'required_attribute_names': ['from_version', 'to_version'], + 'optional_attribute_names': [] + }] + + +class TopicRightsChange(change_domain.BaseChange): + """Domain object for changes made to a topic rights object. + + The allowed commands, together with the attributes: + - 'change_role' (with assignee_id, new_role and old_role) + - 'create_new' + - 'publish_story' + - 'unpublish_story'. + """ + + # The allowed list of roles which can be used in change_role command. + ALLOWED_ROLES = [ROLE_NONE, ROLE_MANAGER] + + ALLOWED_COMMANDS = [{ + 'name': CMD_CREATE_NEW, + 'required_attribute_names': [], + 'optional_attribute_names': [] + }, { + 'name': CMD_CHANGE_ROLE, + 'required_attribute_names': ['assignee_id', 'new_role', 'old_role'], + 'optional_attribute_names': [], + 'allowed_values': {'new_role': ALLOWED_ROLES, 'old_role': ALLOWED_ROLES} + }, { + 'name': CMD_REMOVE_MANAGER_ROLE, + 'required_attribute_names': ['removed_user_id'], + 'optional_attribute_names': [] + }, { + 'name': CMD_PUBLISH_TOPIC, + 'required_attribute_names': [], + 'optional_attribute_names': [] + }, { + 'name': CMD_UNPUBLISH_TOPIC, + 'required_attribute_names': [], + 'optional_attribute_names': [] + }] class Subtopic(object): @@ -871,6 +830,88 @@ def __init__( self.topic_model_created_on = topic_model_created_on self.topic_model_last_updated = topic_model_last_updated + def validate(self): + """Validates all properties of this topic summary. + + Raises: + ValidationError: One or more attributes of the Topic summary + are not valid. + """ + if not isinstance(self.name, basestring): + raise utils.ValidationError('Name should be a string.') + if self.name == '': + raise utils.ValidationError('Name field should not be empty') + + if not isinstance(self.canonical_name, basestring): + raise utils.ValidationError('Canonical name should be a string.') + if self.canonical_name == '': + raise utils.ValidationError( + 'Canonical name field should not be empty') + + if not isinstance(self.language_code, basestring): + raise utils.ValidationError( + 'Expected language code to be a string, received %s' % + self.language_code) + if not utils.is_valid_language_code(self.language_code): + raise utils.ValidationError( + 'Invalid language code: %s' % self.language_code) + + if not isinstance(self.canonical_story_count, int): + raise utils.ValidationError( + 'Expected canonical story count to be an integer, ' + 'received \'%s\'' % self.canonical_story_count) + + if self.canonical_story_count < 0: + raise utils.ValidationError( + 'Expected canonical_story_count to be non-negative, ' + 'received \'%s\'' % self.canonical_story_count) + + if not isinstance(self.additional_story_count, int): + raise utils.ValidationError( + 'Expected additional story count to be an integer, ' + 'received \'%s\'' % self.additional_story_count) + + if self.additional_story_count < 0: + raise utils.ValidationError( + 'Expected additional_story_count to be non-negative, ' + 'received \'%s\'' % self.additional_story_count) + + if not isinstance(self.uncategorized_skill_count, int): + raise utils.ValidationError( + 'Expected uncategorized skill count to be an integer, ' + 'received \'%s\'' % self.uncategorized_skill_count) + + if self.uncategorized_skill_count < 0: + raise utils.ValidationError( + 'Expected uncategorized_skill_count to be non-negative, ' + 'received \'%s\'' % self.uncategorized_skill_count) + + if not isinstance(self.total_skill_count, int): + raise utils.ValidationError( + 'Expected total skill count to be an integer, received \'%s\'' + % self.total_skill_count) + + if self.total_skill_count < 0: + raise utils.ValidationError( + 'Expected total_skill_count to be non-negative, ' + 'received \'%s\'' % self.total_skill_count) + + if self.total_skill_count < self.uncategorized_skill_count: + raise utils.ValidationError( + 'Expected total_skill_count to be greater than or equal to ' + 'uncategorized_skill_count %s, received \'%s\'' % ( + self.uncategorized_skill_count, self.total_skill_count)) + + if not isinstance(self.subtopic_count, int): + raise utils.ValidationError( + 'Expected subtopic count to be an integer, received \'%s\'' + % self.subtopic_count) + + if self.subtopic_count < 0: + raise utils.ValidationError( + 'Expected subtopic_count to be non-negative, ' + 'received \'%s\'' % self.subtopic_count) + def to_dict(self): """Returns a dictionary representation of this domain object. diff --git a/core/domain/topic_domain_test.py b/core/domain/topic_domain_test.py index 009c2664899f6..24b73ab120f26 100644 --- a/core/domain/topic_domain_test.py +++ b/core/domain/topic_domain_test.py @@ -16,6 +16,8 @@ """Tests for topic domain objects.""" +import datetime + from constants import constants from core.domain import skill_domain from core.domain import state_domain @@ -230,8 +232,8 @@ def test_fail_to_add_unpublished_skill_id(self): 'skill_a', self.user_id_a, 'Description A', misconceptions=[], skill_contents=skill_domain.SkillContents( state_domain.SubtitledHtml( - '1', 'Explanation'), [ - state_domain.SubtitledHtml('2', 'Example 1')], + '1', 'Explanation
'), [ + state_domain.SubtitledHtml('2', 'Example 1
')], {'1': {}, '2': {}}, state_domain.WrittenTranslations.from_dict( {'translations_mapping': {'1': {}, '2': {}}}))) @@ -288,14 +290,16 @@ def test_is_manager(self): self.assertFalse(topic_rights.is_manager('fakeuser')) def test_cannot_create_topic_rights_change_class_with_invalid_cmd(self): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): + with self.assertRaisesRegexp( + Exception, 'Command invalid cmd is not allowed'): topic_domain.TopicRightsChange({ 'cmd': 'invalid cmd' }) def test_cannot_create_topic_rights_change_class_with_invalid_changelist( self): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): + with self.assertRaisesRegexp( + Exception, 'Missing cmd key in change dict'): topic_domain.TopicRightsChange({}) def test_create_new_topic_rights_change_class(self): @@ -362,3 +366,436 @@ def test_cannot_move_existing_skill_to_subtopic(self): Exception, 'Skill id skill_id_1 is already present in the target subtopic'): self.topic.move_skill_id_to_subtopic(1, 2, 'skill_id_1') + + +class TopicChangeTests(test_utils.GenericTestBase): + + def test_topic_change_object_with_missing_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, 'Missing cmd key in change dict'): + topic_domain.TopicChange({'invalid': 'data'}) + + def test_topic_change_object_with_invalid_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, 'Command invalid is not allowed'): + topic_domain.TopicChange({'cmd': 'invalid'}) + + def test_topic_change_object_with_missing_attribute_in_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'The following required attributes are missing: ' + 'new_value, old_value')): + topic_domain.TopicChange({ + 'cmd': 'update_topic_property', + 'property_name': 'name', + }) + + def test_topic_change_object_with_extra_attribute_in_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'The following extra attributes are present: invalid')): + topic_domain.TopicChange({ + 'cmd': 'add_subtopic', + 'title': 'title', + 'subtopic_id': 'subtopic_id', + 'invalid': 'invalid' + }) + + def test_topic_change_object_with_invalid_topic_property(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Value for property_name in cmd update_topic_property: ' + 'invalid is not allowed')): + topic_domain.TopicChange({ + 'cmd': 'update_topic_property', + 'property_name': 'invalid', + 'old_value': 'old_value', + 'new_value': 'new_value', + }) + + def test_topic_change_object_with_invalid_subtopic_property(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Value for property_name in cmd update_subtopic_property: ' + 'invalid is not allowed')): + topic_domain.TopicChange({ + 'cmd': 'update_subtopic_property', + 'subtopic_id': 'subtopic_id', + 'property_name': 'invalid', + 'old_value': 'old_value', + 'new_value': 'new_value', + }) + + def test_topic_change_object_with_add_subtopic(self): + topic_change_object = topic_domain.TopicChange({ + 'cmd': 'add_subtopic', + 'subtopic_id': 'subtopic_id', + 'title': 'title' + }) + + self.assertEqual(topic_change_object.cmd, 'add_subtopic') + self.assertEqual(topic_change_object.subtopic_id, 'subtopic_id') + self.assertEqual(topic_change_object.title, 'title') + + def test_topic_change_object_with_delete_subtopic(self): + topic_change_object = topic_domain.TopicChange({ + 'cmd': 'delete_subtopic', + 'subtopic_id': 'subtopic_id' + }) + + self.assertEqual(topic_change_object.cmd, 'delete_subtopic') + self.assertEqual(topic_change_object.subtopic_id, 'subtopic_id') + + def test_topic_change_object_with_add_uncategorized_skill_id(self): + topic_change_object = topic_domain.TopicChange({ + 'cmd': 'add_uncategorized_skill_id', + 'new_uncategorized_skill_id': 'new_uncategorized_skill_id' + }) + + self.assertEqual(topic_change_object.cmd, 'add_uncategorized_skill_id') + self.assertEqual( + topic_change_object.new_uncategorized_skill_id, + 'new_uncategorized_skill_id') + + def test_topic_change_object_with_remove_uncategorized_skill_id(self): + topic_change_object = topic_domain.TopicChange({ + 'cmd': 'remove_uncategorized_skill_id', + 'uncategorized_skill_id': 'uncategorized_skill_id' + }) + + self.assertEqual( + topic_change_object.cmd, 'remove_uncategorized_skill_id') + self.assertEqual( + topic_change_object.uncategorized_skill_id, + 'uncategorized_skill_id') + + def test_topic_change_object_with_move_skill_id_to_subtopic(self): + topic_change_object = topic_domain.TopicChange({ + 'cmd': 'move_skill_id_to_subtopic', + 'skill_id': 'skill_id', + 'old_subtopic_id': 'old_subtopic_id', + 'new_subtopic_id': 'new_subtopic_id' + }) + + self.assertEqual(topic_change_object.cmd, 'move_skill_id_to_subtopic') + self.assertEqual(topic_change_object.skill_id, 'skill_id') + self.assertEqual(topic_change_object.old_subtopic_id, 'old_subtopic_id') + self.assertEqual(topic_change_object.new_subtopic_id, 'new_subtopic_id') + + def test_topic_change_object_with_remove_skill_id_from_subtopic(self): + topic_change_object = topic_domain.TopicChange({ + 'cmd': 'remove_skill_id_from_subtopic', + 'skill_id': 'skill_id', + 'subtopic_id': 'subtopic_id' + }) + + self.assertEqual( + topic_change_object.cmd, 'remove_skill_id_from_subtopic') + self.assertEqual(topic_change_object.skill_id, 'skill_id') + self.assertEqual(topic_change_object.subtopic_id, 'subtopic_id') + + def test_topic_change_object_with_update_subtopic_property(self): + topic_change_object = topic_domain.TopicChange({ + 'cmd': 'update_subtopic_property', + 'subtopic_id': 'subtopic_id', + 'property_name': 'title', + 'new_value': 'new_value', + 'old_value': 'old_value' + }) + + self.assertEqual(topic_change_object.cmd, 'update_subtopic_property') + self.assertEqual(topic_change_object.subtopic_id, 'subtopic_id') + self.assertEqual(topic_change_object.property_name, 'title') + self.assertEqual(topic_change_object.new_value, 'new_value') + self.assertEqual(topic_change_object.old_value, 'old_value') + + def test_topic_change_object_with_update_topic_property(self): + topic_change_object = topic_domain.TopicChange({ + 'cmd': 'update_topic_property', + 'property_name': 'name', + 'new_value': 'new_value', + 'old_value': 'old_value' + }) + + self.assertEqual(topic_change_object.cmd, 'update_topic_property') + self.assertEqual(topic_change_object.property_name, 'name') + self.assertEqual(topic_change_object.new_value, 'new_value') + self.assertEqual(topic_change_object.old_value, 'old_value') + + def test_topic_change_object_with_create_new(self): + topic_change_object = topic_domain.TopicChange({ + 'cmd': 'create_new', + 'name': 'name', + }) + + self.assertEqual(topic_change_object.cmd, 'create_new') + self.assertEqual(topic_change_object.name, 'name') + + def test_topic_change_object_with_migrate_subtopic_schema_to_latest_version( + self): + topic_change_object = topic_domain.TopicChange({ + 'cmd': 'migrate_subtopic_schema_to_latest_version', + 'from_version': 'from_version', + 'to_version': 'to_version', + }) + + self.assertEqual( + topic_change_object.cmd, + 'migrate_subtopic_schema_to_latest_version') + self.assertEqual(topic_change_object.from_version, 'from_version') + self.assertEqual(topic_change_object.to_version, 'to_version') + + def test_to_dict(self): + topic_change_dict = { + 'cmd': 'create_new', + 'name': 'name' + } + topic_change_object = topic_domain.TopicChange(topic_change_dict) + self.assertEqual(topic_change_object.to_dict(), topic_change_dict) + + +class TopicRightsChangeTests(test_utils.GenericTestBase): + + def test_topic_rights_change_object_with_missing_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, 'Missing cmd key in change dict'): + topic_domain.TopicRightsChange({'invalid': 'data'}) + + def test_topic_change_rights_object_with_invalid_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, 'Command invalid is not allowed'): + topic_domain.TopicRightsChange({'cmd': 'invalid'}) + + def test_topic_rights_change_object_with_missing_attribute_in_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'The following required attributes are missing: ' + 'new_role, old_role')): + topic_domain.TopicRightsChange({ + 'cmd': 'change_role', + 'assignee_id': 'assignee_id', + }) + + def test_topic_rights_change_object_with_extra_attribute_in_cmd(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'The following extra attributes are present: invalid')): + topic_domain.TopicRightsChange({ + 'cmd': 'publish_topic', + 'invalid': 'invalid' + }) + + def test_topic_rights_change_object_with_invalid_role(self): + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Value for old_role in cmd change_role: ' + 'invalid is not allowed')): + topic_domain.TopicRightsChange({ + 'cmd': 'change_role', + 'assignee_id': 'assignee_id', + 'old_role': 'invalid', + 'new_role': topic_domain.ROLE_MANAGER + }) + + def test_topic_rights_change_object_with_create_new(self): + topic_rights_change_object = topic_domain.TopicRightsChange({ + 'cmd': 'create_new' + }) + + self.assertEqual(topic_rights_change_object.cmd, 'create_new') + + def test_topic_rights_change_object_with_change_role(self): + topic_rights_change_object = topic_domain.TopicRightsChange({ + 'cmd': 'change_role', + 'assignee_id': 'assignee_id', + 'old_role': topic_domain.ROLE_NONE, + 'new_role': topic_domain.ROLE_MANAGER + }) + + self.assertEqual(topic_rights_change_object.cmd, 'change_role') + self.assertEqual(topic_rights_change_object.assignee_id, 'assignee_id') + self.assertEqual( + topic_rights_change_object.old_role, topic_domain.ROLE_NONE) + self.assertEqual( + topic_rights_change_object.new_role, topic_domain.ROLE_MANAGER) + + def test_topic_rights_change_object_with_publish_topic(self): + topic_rights_change_object = topic_domain.TopicRightsChange({ + 'cmd': 'publish_topic' + }) + + self.assertEqual(topic_rights_change_object.cmd, 'publish_topic') + + def test_topic_rights_change_object_with_unpublish_topic(self): + topic_rights_change_object = topic_domain.TopicRightsChange({ + 'cmd': 'unpublish_topic' + }) + + self.assertEqual(topic_rights_change_object.cmd, 'unpublish_topic') + + def test_to_dict(self): + topic_rights_change_dict = { + 'cmd': 'change_role', + 'assignee_id': 'assignee_id', + 'old_role': topic_domain.ROLE_NONE, + 'new_role': topic_domain.ROLE_MANAGER + } + topic_rights_change_object = topic_domain.TopicRightsChange( + topic_rights_change_dict) + self.assertEqual( + topic_rights_change_object.to_dict(), topic_rights_change_dict) + + +class TopicSummaryTests(test_utils.GenericTestBase): + + def setUp(self): + super(TopicSummaryTests, self).setUp() + current_time = datetime.datetime.utcnow() + time_in_millisecs = utils.get_time_in_millisecs(current_time) + self.topic_summary_dict = { + 'id': 'topic_id', + 'name': 'name', + 'language_code': 'en', + 'version': 1, + 'canonical_story_count': 1, + 'additional_story_count': 1, + 'uncategorized_skill_count': 1, + 'subtopic_count': 1, + 'total_skill_count': 1, + 'topic_model_created_on': time_in_millisecs, + 'topic_model_last_updated': time_in_millisecs + } + + self.topic_summary = topic_domain.TopicSummary( + 'topic_id', 'name', 'name', 'en', 1, 1, 1, 1, 1, 1, + current_time, current_time) + + def test_topic_summary_gets_created(self): + self.assertEqual( + self.topic_summary.to_dict(), self.topic_summary_dict) + + def test_validation_passes_with_valid_properties(self): + self.topic_summary.validate() + + def test_validation_fails_with_invalid_name(self): + self.topic_summary.name = 0 + with self.assertRaisesRegexp( + utils.ValidationError, 'Name should be a string.'): + self.topic_summary.validate() + + def test_validation_fails_with_empty_name(self): + self.topic_summary.name = '' + with self.assertRaisesRegexp( + utils.ValidationError, 'Name field should not be empty'): + self.topic_summary.validate() + + def test_validation_fails_with_invalid_canonical_name(self): + self.topic_summary.canonical_name = 0 + with self.assertRaisesRegexp( + utils.ValidationError, 'Canonical name should be a string.'): + self.topic_summary.validate() + + def test_validation_fails_with_empty_canonical_name(self): + self.topic_summary.canonical_name = '' + with self.assertRaisesRegexp( + utils.ValidationError, 'Canonical name field should not be empty'): + self.topic_summary.validate() + + def test_validation_fails_with_invalid_language_code(self): + self.topic_summary.language_code = 0 + with self.assertRaisesRegexp( + utils.ValidationError, + 'Expected language code to be a string, received 0'): + self.topic_summary.validate() + + def test_validation_fails_with_unallowed_language_code(self): + self.topic_summary.language_code = 'invalid' + with self.assertRaisesRegexp( + utils.ValidationError, 'Invalid language code: invalid'): + self.topic_summary.validate() + + def test_validation_fails_with_invalid_canonical_story_count(self): + self.topic_summary.canonical_story_count = '10' + with self.assertRaisesRegexp( + utils.ValidationError, + 'Expected canonical story count to be an integer, received \'10\''): + self.topic_summary.validate() + + def test_validation_fails_with_negative_canonical_story_count(self): + self.topic_summary.canonical_story_count = -1 + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Expected canonical_story_count to be non-negative, ' + 'received \'-1\'')): + self.topic_summary.validate() + + def test_validation_fails_with_invalid_additional_story_count(self): + self.topic_summary.additional_story_count = '10' + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Expected additional story count to be an ' + 'integer, received \'10\'')): + self.topic_summary.validate() + + def test_validation_fails_with_negative_additional_story_count(self): + self.topic_summary.additional_story_count = -1 + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Expected additional_story_count to be non-negative, ' + 'received \'-1\'')): + self.topic_summary.validate() + + def test_validation_fails_with_invalid_uncategorized_skill_count(self): + self.topic_summary.uncategorized_skill_count = '10' + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Expected uncategorized skill count to be an integer, ' + 'received \'10\'')): + self.topic_summary.validate() + + def test_validation_fails_with_negative_uncategorized_skill_count(self): + self.topic_summary.uncategorized_skill_count = -1 + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Expected uncategorized_skill_count to be non-negative, ' + 'received \'-1\'')): + self.topic_summary.validate() + + def test_validation_fails_with_invalid_total_skill_count(self): + self.topic_summary.total_skill_count = '10' + with self.assertRaisesRegexp( + utils.ValidationError, + 'Expected total skill count to be an integer, received \'10\''): + self.topic_summary.validate() + + def test_validation_fails_with_negative_total_skill_count(self): + self.topic_summary.total_skill_count = -1 + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Expected total_skill_count to be non-negative, ' + 'received \'-1\'')): + self.topic_summary.validate() + + def test_validation_fails_with_invalid_total_skill_count_value(self): + self.topic_summary.total_skill_count = 5 + self.topic_summary.uncategorized_skill_count = 10 + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Expected total_skill_count to be greater than or equal to ' + 'uncategorized_skill_count 10, received \'5\'')): + self.topic_summary.validate() + + def test_validation_fails_with_invalid_subtopic_count(self): + self.topic_summary.subtopic_count = '10' + with self.assertRaisesRegexp( + utils.ValidationError, + 'Expected subtopic count to be an integer, received \'10\''): + self.topic_summary.validate() + + def test_validation_fails_with_negative_subtopic_count(self): + self.topic_summary.subtopic_count = -1 + with self.assertRaisesRegexp( + utils.ValidationError, ( + 'Expected subtopic_count to be non-negative, ' + 'received \'-1\'')): + self.topic_summary.validate() diff --git a/core/domain/topic_services.py b/core/domain/topic_services.py index 5c9cc414d00bc..f41ab82fc3dd1 100644 --- a/core/domain/topic_services.py +++ b/core/domain/topic_services.py @@ -16,6 +16,7 @@ """Commands for operations on topics, and related models.""" +import collections import copy import logging @@ -332,11 +333,12 @@ def apply_change_list(topic_id, change_list): Exception. The incoming changelist had simultaneuous creation and deletion of subtopics. Returns: - Topic, dict, list(int), list(int). The modified topic - object, the modified subtopic pages dict keyed by subtopic page id - containing the updated domain objects of each subtopic page, a list - of ids of the deleted subtopics and a list of ids of the newly - created subtopics. + Topic, dict, list(int), list(int), list(SubtopicPageChange). + The modified topic object, the modified subtopic pages dict keyed + by subtopic page id containing the updated domain objects of + each subtopic page, a list of ids of the deleted subtopics, + a list of ids of the newly created subtopics and a list of changes + applied to modified subtopic pages. """ topic = get_topic_by_id(topic_id) newly_created_subtopic_ids = [] @@ -344,12 +346,19 @@ def apply_change_list(topic_id, change_list): deleted_subtopic_ids = [] modified_subtopic_pages_list = [] modified_subtopic_pages = {} + modified_subtopic_change_cmds = collections.defaultdict(list) for change in change_list: if (change.cmd == subtopic_page_domain.CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY): - if change.id < topic.next_subtopic_id: - existing_subtopic_page_ids_to_be_modified.append(change.id) + if change.subtopic_id < topic.next_subtopic_id: + existing_subtopic_page_ids_to_be_modified.append( + change.subtopic_id) + subtopic_page_id = ( + subtopic_page_domain.SubtopicPage.get_subtopic_page_id( + topic_id, change.subtopic_id)) + modified_subtopic_change_cmds[subtopic_page_id].append( + change) modified_subtopic_pages_list = ( subtopic_page_services.get_subtopic_pages_with_ids( topic_id, existing_subtopic_page_ids_to_be_modified)) @@ -366,18 +375,26 @@ def apply_change_list(topic_id, change_list): subtopic_page_domain.SubtopicPage.create_default_subtopic_page( #pylint: disable=line-too-long change.subtopic_id, topic_id) ) + modified_subtopic_change_cmds[subtopic_page_id].append( + subtopic_page_domain.SubtopicPageChange({ + 'cmd': 'create_new', + 'topic_id': topic_id, + 'subtopic_id': change.subtopic_id + })) newly_created_subtopic_ids.append(change.subtopic_id) elif change.cmd == topic_domain.CMD_DELETE_SUBTOPIC: - topic.delete_subtopic(change.id) - if change.id in newly_created_subtopic_ids: + topic.delete_subtopic(change.subtopic_id) + if change.subtopic_id in newly_created_subtopic_ids: raise Exception( 'The incoming changelist had simultaneous' ' creation and deletion of subtopics.') - deleted_subtopic_ids.append(change.id) + deleted_subtopic_ids.append(change.subtopic_id) elif change.cmd == topic_domain.CMD_ADD_UNCATEGORIZED_SKILL_ID: - topic.add_uncategorized_skill_id(change.id) + topic.add_uncategorized_skill_id( + change.new_uncategorized_skill_id) elif change.cmd == topic_domain.CMD_REMOVE_UNCATEGORIZED_SKILL_ID: - topic.remove_uncategorized_skill_id(change.id) + topic.remove_uncategorized_skill_id( + change.uncategorized_skill_id) elif change.cmd == topic_domain.CMD_MOVE_SKILL_ID_TO_SUBTOPIC: topic.move_skill_id_to_subtopic( change.old_subtopic_id, change.new_subtopic_id, @@ -407,11 +424,12 @@ def apply_change_list(topic_id, change_list): subtopic_page_domain.CMD_UPDATE_SUBTOPIC_PAGE_PROPERTY): subtopic_page_id = ( subtopic_page_domain.SubtopicPage.get_subtopic_page_id( - topic_id, change.id)) + topic_id, change.subtopic_id)) if ((modified_subtopic_pages[subtopic_page_id] is None) or - (change.id in deleted_subtopic_ids)): + (change.subtopic_id in deleted_subtopic_ids)): raise Exception( - 'The subtopic with id %s doesn\'t exist' % change.id) + 'The subtopic with id %s doesn\'t exist' % ( + change.subtopic_id)) if (change.property_name == subtopic_page_domain. @@ -431,7 +449,8 @@ def apply_change_list(topic_id, change_list): elif change.cmd == topic_domain.CMD_UPDATE_SUBTOPIC_PROPERTY: if (change.property_name == topic_domain.SUBTOPIC_PROPERTY_TITLE): - topic.update_subtopic_title(change.id, change.new_value) + topic.update_subtopic_title( + change.subtopic_id, change.new_value) else: raise Exception('Invalid change dict.') elif ( @@ -446,7 +465,7 @@ def apply_change_list(topic_id, change_list): raise Exception('Invalid change dict.') return ( topic, modified_subtopic_pages, deleted_subtopic_ids, - newly_created_subtopic_ids) + newly_created_subtopic_ids, modified_subtopic_change_cmds) except Exception as e: logging.error( @@ -530,7 +549,8 @@ def update_topic_and_subtopic_pages( ( updated_topic, updated_subtopic_pages_dict, - deleted_subtopic_ids, newly_created_subtopic_ids + deleted_subtopic_ids, newly_created_subtopic_ids, + updated_subtopic_pages_change_cmds_dict ) = apply_change_list(topic_id, change_list) _save_topic( @@ -545,12 +565,15 @@ def update_topic_and_subtopic_pages( for subtopic_page_id in updated_subtopic_pages_dict: subtopic_page = updated_subtopic_pages_dict[subtopic_page_id] + subtopic_page_change_list = updated_subtopic_pages_change_cmds_dict[ + subtopic_page_id] subtopic_id = subtopic_page.get_subtopic_id_from_subtopic_page_id() # The following condition prevents the creation of subtopic pages that # were deleted above. if subtopic_id not in deleted_subtopic_ids: subtopic_page_services.save_subtopic_page( - committer_id, subtopic_page, commit_message, change_list) + committer_id, subtopic_page, commit_message, + subtopic_page_change_list) create_topic_summary(topic_id) diff --git a/core/domain/topic_services_test.py b/core/domain/topic_services_test.py index f4597feb33fa9..fce4157a29534 100644 --- a/core/domain/topic_services_test.py +++ b/core/domain/topic_services_test.py @@ -199,7 +199,8 @@ def test_get_all_skill_ids_assigned_to_some_topic(self): set([self.skill_id_1, self.skill_id_2, 'skill_3'])) def test_cannot_create_topic_change_class_with_invalid_changelist(self): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): + with self.assertRaisesRegexp( + Exception, 'Missing cmd key in change dict'): topic_domain.TopicChange({ 'invalid_cmd': topic_domain.CMD_UPDATE_TOPIC_PROPERTY, 'property_name': topic_domain.TOPIC_PROPERTY_DESCRIPTION, @@ -208,7 +209,10 @@ def test_cannot_create_topic_change_class_with_invalid_changelist(self): }) def test_cannot_update_topic_property_with_invalid_changelist(self): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): + with self.assertRaisesRegexp( + Exception, ( + 'Value for property_name in cmd update_topic_property: ' + 'invalid property is not allowed')): topic_domain.TopicChange({ 'cmd': topic_domain.CMD_UPDATE_TOPIC_PROPERTY, 'property_name': 'invalid property', @@ -217,7 +221,10 @@ def test_cannot_update_topic_property_with_invalid_changelist(self): }) def test_cannot_update_subtopic_property_with_invalid_changelist(self): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): + with self.assertRaisesRegexp( + Exception, ( + 'The following required attributes are ' + 'missing: subtopic_id')): topic_domain.TopicChange({ 'cmd': topic_domain.CMD_UPDATE_SUBTOPIC_PROPERTY, 'property_name': 'invalid property', @@ -247,7 +254,8 @@ def test_update_subtopic_property(self): self.assertEqual(topic.subtopics[0].title, 'New Title') def test_cannot_create_topic_change_class_with_invalid_cmd(self): - with self.assertRaisesRegexp(Exception, 'Invalid change_dict'): + with self.assertRaisesRegexp( + Exception, 'Command invalid cmd is not allowed'): topic_domain.TopicChange({ 'cmd': 'invalid cmd', 'property_name': 'title', diff --git a/core/jobs_registry.py b/core/jobs_registry.py index bcbed0167b24e..26af6b2ce0129 100644 --- a/core/jobs_registry.py +++ b/core/jobs_registry.py @@ -57,27 +57,43 @@ prod_validation_jobs_one_off.CollectionSnapshotMetadataModelAuditOneOffJob, prod_validation_jobs_one_off.CollectionSnapshotContentModelAuditOneOffJob, prod_validation_jobs_one_off.CollectionRightsModelAuditOneOffJob, - prod_validation_jobs_one_off.CollectionRightsSnapshotMetadataModelAuditOneOffJob, # pylint: disable=line-too-long - prod_validation_jobs_one_off.CollectionRightsSnapshotContentModelAuditOneOffJob, # pylint: disable=line-too-long + ( + prod_validation_jobs_one_off + .CollectionRightsSnapshotMetadataModelAuditOneOffJob), + ( + prod_validation_jobs_one_off + .CollectionRightsSnapshotContentModelAuditOneOffJob), prod_validation_jobs_one_off.CollectionCommitLogEntryModelAuditOneOffJob, prod_validation_jobs_one_off.CollectionSummaryModelAuditOneOffJob, prod_validation_jobs_one_off.ConfigPropertyModelAuditOneOffJob, - prod_validation_jobs_one_off.ConfigPropertySnapshotMetadataModelAuditOneOffJob, # pylint: disable=line-too-long - prod_validation_jobs_one_off.ConfigPropertySnapshotContentModelAuditOneOffJob, # pylint: disable=line-too-long + ( + prod_validation_jobs_one_off + .ConfigPropertySnapshotMetadataModelAuditOneOffJob), + ( + prod_validation_jobs_one_off + .ConfigPropertySnapshotContentModelAuditOneOffJob), prod_validation_jobs_one_off.SentEmailModelAuditOneOffJob, prod_validation_jobs_one_off.BulkEmailModelAuditOneOffJob, - prod_validation_jobs_one_off.GeneralFeedbackEmailReplyToIdModelAuditOneOffJob, # pylint: disable=line-too-long + ( + prod_validation_jobs_one_off + .GeneralFeedbackEmailReplyToIdModelAuditOneOffJob), prod_validation_jobs_one_off.ExplorationModelAuditOneOffJob, prod_validation_jobs_one_off.ExplorationSnapshotMetadataModelAuditOneOffJob, prod_validation_jobs_one_off.ExplorationSnapshotContentModelAuditOneOffJob, prod_validation_jobs_one_off.ExplorationRightsModelAuditOneOffJob, - prod_validation_jobs_one_off.ExplorationRightsSnapshotMetadataModelAuditOneOffJob, # pylint: disable=line-too-long - prod_validation_jobs_one_off.ExplorationRightsSnapshotContentModelAuditOneOffJob, # pylint: disable=line-too-long + ( + prod_validation_jobs_one_off + .ExplorationRightsSnapshotMetadataModelAuditOneOffJob), + ( + prod_validation_jobs_one_off + .ExplorationRightsSnapshotContentModelAuditOneOffJob), prod_validation_jobs_one_off.ExplorationCommitLogEntryModelAuditOneOffJob, prod_validation_jobs_one_off.ExpSummaryModelAuditOneOffJob, prod_validation_jobs_one_off.ExplorationRecommendationsModelAuditOneOffJob, prod_validation_jobs_one_off.FileMetadataModelAuditOneOffJob, - prod_validation_jobs_one_off.FileMetadataSnapshotMetadataModelAuditOneOffJob, # pylint: disable=line-too-long + ( + prod_validation_jobs_one_off + .FileMetadataSnapshotMetadataModelAuditOneOffJob), prod_validation_jobs_one_off.FileMetadataSnapshotContentModelAuditOneOffJob, prod_validation_jobs_one_off.FileModelAuditOneOffJob, prod_validation_jobs_one_off.FileSnapshotMetadataModelAuditOneOffJob, diff --git a/core/templates/dev/head/domain/skill/SkillUpdateService.ts b/core/templates/dev/head/domain/skill/SkillUpdateService.ts index 1391f576e420e..708f4f3dc9727 100644 --- a/core/templates/dev/head/domain/skill/SkillUpdateService.ts +++ b/core/templates/dev/head/domain/skill/SkillUpdateService.ts @@ -69,7 +69,7 @@ oppia.factory('SkillUpdateService', [ property_name: propertyName, new_value: angular.copy(newValue), old_value: angular.copy(oldValue), - id: misconceptionId, + misconception_id: misconceptionId, }, apply, reverse); }; @@ -206,7 +206,7 @@ oppia.factory('SkillUpdateService', [ deleteMisconception: function(skill, misconceptionId) { var params = { - id: misconceptionId + misconception_id: misconceptionId }; var oldMisconception = skill.findMisconceptionById(misconceptionId); _applyChange( diff --git a/core/templates/dev/head/domain/skill/SkillUpdateServiceSpec.ts b/core/templates/dev/head/domain/skill/SkillUpdateServiceSpec.ts index f5c0828cfe8e8..1d9cab4ecbb33 100644 --- a/core/templates/dev/head/domain/skill/SkillUpdateServiceSpec.ts +++ b/core/templates/dev/head/domain/skill/SkillUpdateServiceSpec.ts @@ -151,7 +151,7 @@ describe('Skill update service', function() { SkillUpdateService.deleteMisconception(skill, '2'); expect(UndoRedoService.getCommittableChangeList()).toEqual([{ cmd: 'delete_skill_misconception', - id: '2' + misconception_id: '2' }]); expect(skill.getMisconceptions().length).toEqual(1); UndoRedoService.undoChange(skill); @@ -167,7 +167,7 @@ describe('Skill update service', function() { property_name: 'name', old_value: 'test name', new_value: 'new name', - id: '2' + misconception_id: '2' }]); expect(skill.findMisconceptionById('2').getName()).toEqual('new name'); UndoRedoService.undoChange(skill); @@ -183,7 +183,7 @@ describe('Skill update service', function() { property_name: 'notes', old_value: 'test notes', new_value: 'new notes', - id: '2' + misconception_id: '2' }]); expect(skill.findMisconceptionById('2').getNotes()).toEqual('new notes'); UndoRedoService.undoChange(skill); @@ -202,7 +202,7 @@ describe('Skill update service', function() { property_name: 'feedback', old_value: 'test feedback', new_value: 'new feedback', - id: '2' + misconception_id: '2' }]); expect(skill.findMisconceptionById('2').getFeedback()) .toEqual('new feedback'); diff --git a/core/templates/dev/head/domain/topic/TopicUpdateService.ts b/core/templates/dev/head/domain/topic/TopicUpdateService.ts index 8df80f5acffd6..2fa2926b85aa1 100644 --- a/core/templates/dev/head/domain/topic/TopicUpdateService.ts +++ b/core/templates/dev/head/domain/topic/TopicUpdateService.ts @@ -67,8 +67,7 @@ oppia.factory('TopicUpdateService', [ _applyChange(topic, CMD_UPDATE_TOPIC_PROPERTY, { property_name: propertyName, new_value: angular.copy(newValue), - old_value: angular.copy(oldValue), - change_affects_subtopic_page: false + old_value: angular.copy(oldValue) }, apply, reverse); }; @@ -78,8 +77,7 @@ oppia.factory('TopicUpdateService', [ subtopic_id: subtopicId, property_name: propertyName, new_value: angular.copy(newValue), - old_value: angular.copy(oldValue), - change_affects_subtopic_page: false + old_value: angular.copy(oldValue) }, apply, reverse); }; @@ -90,8 +88,7 @@ oppia.factory('TopicUpdateService', [ subtopic_id: subtopicId, property_name: propertyName, new_value: angular.copy(newValue), - old_value: angular.copy(oldValue), - change_affects_subtopic_page: true + old_value: angular.copy(oldValue) }, apply, reverse); }; @@ -169,8 +166,7 @@ oppia.factory('TopicUpdateService', [ var nextSubtopicId = topic.getNextSubtopicId(); _applyChange(topic, CMD_ADD_SUBTOPIC, { subtopic_id: nextSubtopicId, - title: title, - change_affects_subtopic_page: false + title: title }, function(changeDict, topic) { // Apply. topic.addSubtopic(title); @@ -287,8 +283,7 @@ oppia.factory('TopicUpdateService', [ return; } _applyChange(topic, CMD_DELETE_SUBTOPIC, { - subtopic_id: subtopicId, - change_affects_subtopic_page: false + subtopic_id: subtopicId }, function(changeDict, topic) { // Apply. topic.deleteSubtopic(subtopicId, newlyCreated); @@ -314,8 +309,7 @@ oppia.factory('TopicUpdateService', [ _applyChange(topic, CMD_MOVE_SKILL_ID_TO_SUBTOPIC, { old_subtopic_id: oldSubtopicId, new_subtopic_id: newSubtopicId, - skill_id: skillSummary.getId(), - change_affects_subtopic_page: false + skill_id: skillSummary.getId() }, function(changeDict, topic) { // Apply. if (oldSubtopicId === null) { @@ -346,8 +340,7 @@ oppia.factory('TopicUpdateService', [ var subtopic = topic.getSubtopicById(subtopicId); _applyChange(topic, CMD_REMOVE_SKILL_ID_FROM_SUBTOPIC, { subtopic_id: subtopicId, - skill_id: skillSummary.getId(), - change_affects_subtopic_page: false + skill_id: skillSummary.getId() }, function(changeDict, topic) { // Apply. subtopic.removeSkill(skillSummary.getId()); @@ -520,8 +513,7 @@ oppia.factory('TopicUpdateService', [ */ addUncategorizedSkill: function(topic, skillSummary) { _applyChange(topic, CMD_ADD_UNCATEGORIZED_SKILL_ID, { - new_uncategorized_skill_id: skillSummary.getId(), - change_affects_subtopic_page: false + new_uncategorized_skill_id: skillSummary.getId() }, function(changeDict, topic) { // Apply. var newSkillId = _getParameterFromChangeDict( @@ -542,8 +534,7 @@ oppia.factory('TopicUpdateService', [ */ removeUncategorizedSkill: function(topic, skillSummary) { _applyChange(topic, CMD_REMOVE_UNCATEGORIZED_SKILL_ID, { - uncategorized_skill_id: skillSummary.getId(), - change_affects_subtopic_page: false + uncategorized_skill_id: skillSummary.getId() }, function(changeDict, topic) { // Apply. var newSkillId = _getParameterFromChangeDict( diff --git a/core/templates/dev/head/domain/topic/TopicUpdateServiceSpec.ts b/core/templates/dev/head/domain/topic/TopicUpdateServiceSpec.ts index c2eb0802fd317..78dc0c381e8be 100644 --- a/core/templates/dev/head/domain/topic/TopicUpdateServiceSpec.ts +++ b/core/templates/dev/head/domain/topic/TopicUpdateServiceSpec.ts @@ -136,8 +136,7 @@ describe('Topic update service', function() { cmd: 'update_topic_property', property_name: 'additional_story_ids', new_value: ['story_2', 'story_3'], - old_value: ['story_2'], - change_affects_subtopic_page: false + old_value: ['story_2'] }]); }); @@ -169,8 +168,7 @@ describe('Topic update service', function() { cmd: 'update_topic_property', property_name: 'additional_story_ids', new_value: [], - old_value: ['story_2'], - change_affects_subtopic_page: false + old_value: ['story_2'] }]); }); @@ -204,8 +202,7 @@ describe('Topic update service', function() { cmd: 'update_topic_property', property_name: 'canonical_story_ids', new_value: ['story_1', 'story_3'], - old_value: ['story_1'], - change_affects_subtopic_page: false + old_value: ['story_1'] }]); }); @@ -237,8 +234,7 @@ describe('Topic update service', function() { cmd: 'update_topic_property', property_name: 'canonical_story_ids', new_value: [], - old_value: ['story_1'], - change_affects_subtopic_page: false + old_value: ['story_1'] }]); }); @@ -276,8 +272,7 @@ describe('Topic update service', function() { _sampleTopic, _thirdSkillSummary); expect(UndoRedoService.getCommittableChangeList()).toEqual([{ cmd: 'add_uncategorized_skill_id', - new_uncategorized_skill_id: 'skill_3', - change_affects_subtopic_page: false + new_uncategorized_skill_id: 'skill_3' }]); }); @@ -315,8 +310,7 @@ describe('Topic update service', function() { _sampleTopic, _firstSkillSummary); expect(UndoRedoService.getCommittableChangeList()).toEqual([{ cmd: 'remove_uncategorized_skill_id', - uncategorized_skill_id: 'skill_1', - change_affects_subtopic_page: false + uncategorized_skill_id: 'skill_1' }]); }); @@ -345,8 +339,7 @@ describe('Topic update service', function() { cmd: 'update_topic_property', property_name: 'name', new_value: 'new name', - old_value: 'Topic name', - change_affects_subtopic_page: false + old_value: 'Topic name' }]); } ); @@ -367,8 +360,7 @@ describe('Topic update service', function() { cmd: 'update_topic_property', property_name: 'description', new_value: 'new description', - old_value: 'Topic description', - change_affects_subtopic_page: false + old_value: 'Topic description' }]); } ); @@ -390,8 +382,7 @@ describe('Topic update service', function() { subtopic_id: 1, property_name: 'title', new_value: 'new title', - old_value: 'Title', - change_affects_subtopic_page: false + old_value: 'Title' }]); } ); @@ -423,8 +414,7 @@ describe('Topic update service', function() { expect(UndoRedoService.getCommittableChangeList()).toEqual([{ cmd: 'add_subtopic', subtopic_id: 2, - title: 'Title2', - change_affects_subtopic_page: false + title: 'Title2' }]); } ); @@ -463,8 +453,7 @@ describe('Topic update service', function() { TopicUpdateService.deleteSubtopic(_sampleTopic, 1); expect(UndoRedoService.getCommittableChangeList()).toEqual([{ cmd: 'delete_subtopic', - subtopic_id: 1, - change_affects_subtopic_page: false + subtopic_id: 1 }]); } ); @@ -524,14 +513,12 @@ describe('Topic update service', function() { expect(UndoRedoService.getCommittableChangeList()).toEqual([{ cmd: 'remove_skill_id_from_subtopic', skill_id: 'skill_2', - subtopic_id: 1, - change_affects_subtopic_page: false + subtopic_id: 1 }, { cmd: 'move_skill_id_to_subtopic', skill_id: 'skill_2', new_subtopic_id: 1, - old_subtopic_id: null, - change_affects_subtopic_page: false + old_subtopic_id: null }]); UndoRedoService.clearChanges(); @@ -546,8 +533,7 @@ describe('Topic update service', function() { expect(UndoRedoService.getCommittableChangeList()).toEqual([{ cmd: 'remove_skill_id_from_subtopic', skill_id: 'skill_2', - subtopic_id: 1, - change_affects_subtopic_page: false + subtopic_id: 1 }]); }); @@ -562,14 +548,12 @@ describe('Topic update service', function() { expect(UndoRedoService.getCommittableChangeList()).toEqual([{ cmd: 'add_subtopic', title: 'Title 3', - change_affects_subtopic_page: false, subtopic_id: 2 }, { cmd: 'move_skill_id_to_subtopic', old_subtopic_id: 1, new_subtopic_id: 2, - skill_id: 'skill_2', - change_affects_subtopic_page: false + skill_id: 'skill_2' }]); }); @@ -582,8 +566,7 @@ describe('Topic update service', function() { cmd: 'move_skill_id_to_subtopic', old_subtopic_id: null, new_subtopic_id: 1, - skill_id: 'skill_1', - change_affects_subtopic_page: false + skill_id: 'skill_1' }]); }); @@ -632,8 +615,7 @@ describe('Topic update service', function() { expect(UndoRedoService.getCommittableChangeList()).toEqual([{ cmd: 'remove_skill_id_from_subtopic', subtopic_id: 1, - skill_id: 'skill_2', - change_affects_subtopic_page: false + skill_id: 'skill_2' }]); }); @@ -663,8 +645,7 @@ describe('Topic update service', function() { cmd: 'update_topic_property', property_name: 'language_code', new_value: 'fi', - old_value: 'en', - change_affects_subtopic_page: false + old_value: 'en' }]); } ); @@ -747,8 +728,7 @@ describe('Topic update service', function() { old_value: { html: 'test content', content_id: 'content' - }, - change_affects_subtopic_page: true + } }]); } ); @@ -783,8 +763,7 @@ describe('Topic update service', function() { needs_update: false } } - }, - change_affects_subtopic_page: true + } }]); }); }); diff --git a/core/templates/dev/head/pages/topics-and-skills-dashboard-page/skills-list/skills-list.directive.ts b/core/templates/dev/head/pages/topics-and-skills-dashboard-page/skills-list/skills-list.directive.ts index af00b7634f955..618648d875a86 100644 --- a/core/templates/dev/head/pages/topics-and-skills-dashboard-page/skills-list/skills-list.directive.ts +++ b/core/templates/dev/head/pages/topics-and-skills-dashboard-page/skills-list/skills-list.directive.ts @@ -132,8 +132,7 @@ oppia.directive('skillsList', [ modalInstance.result.then(function(topicIds) { var changeList = [{ cmd: 'add_uncategorized_skill_id', - new_uncategorized_skill_id: skillId, - change_affects_subtopic_page: false + new_uncategorized_skill_id: skillId }]; var topicSummaries = $scope.getEditableTopicSummaries(); for (var i = 0; i < topicIds.length; i++) { diff --git a/core/tests/test_utils.py b/core/tests/test_utils.py index 06a656268cb42..f7465822cd6ca 100644 --- a/core/tests/test_utils.py +++ b/core/tests/test_utils.py @@ -201,7 +201,7 @@ class TestBase(unittest.TestCase): 'correct_answer': u'Solution', 'explanation': { 'content_id': u'solution', - 'html': u'Solution explanation' + 'html': u'Solution explanation
' }, 'answer_is_exclusive': False }, @@ -226,7 +226,7 @@ class TestBase(unittest.TestCase): 'hints': [{ 'hint_content': { 'content_id': u'hint_1', - 'html': u'Hint 1' + 'html': u'Hint 1
' } }] }, @@ -270,7 +270,7 @@ class TestBase(unittest.TestCase): 'dest': 'END', 'feedback': { 'content_id': 'feedback_1', - 'html': 'Correct!'}, + 'html': 'Correct!
'}, 'labelled_as_correct': False, 'missing_prerequisite_skill_id': None, 'param_changes': [], @@ -1855,7 +1855,7 @@ def _create_valid_question_data(self, default_dest_state_name): 'correct_answer': 'Solution', 'explanation': { 'content_id': 'solution', - 'html': 'This is a solution.' + 'html': 'This is a solution.
' } } hints_list = [{ diff --git a/data/explorations/about_oppia.yaml b/data/explorations/about_oppia.yaml index 1480b58a38fb4..c0a4267006aa8 100644 --- a/data/explorations/about_oppia.yaml +++ b/data/explorations/about_oppia.yaml @@ -13,7 +13,7 @@ states: content: audio_translations: {} html: "\_
It works almost the same as when we added the third try!
When\ \ we had only two tries, we could guess any number from 1 to 3.
\_
With 3 tries, and the trick of asking\ + \ alt-with-value=\"""\">
\_
With 3 tries, and the trick of asking\
\ \"Is it 4?\" first, we added 4 more numbers, so we could guess from 1 to\
\ 7.
\_
If we have 4 tries, we can use the\ + \ alt-with-value=\"""\">
\_
If we have 4 tries, we can use the\
\ same trick:
\_
You can write in all the numbers\ + \ alt-with-value=\"""\">
\_
You can write in all the numbers\
\ from left to right (or just count them up!).
So, how many is that?
" content_ids_to_audio_translations: content: {} @@ -483,12 +483,12 @@ states: content_id: content html: "\_
\"So, no matter which number the\ + \ alt-with-value=\"""\">
\_
\"So, no matter which number the\
\ magician is thinking of, if we guess 2 on our first try, we can eliminate\
\ two numbers! Then for the second try (if we even need it), we know exactly\
\ what number to guess!\"
\"So maybe we don't save TOO much time\
+ \ alt-with-value=\"""\">
\"So maybe we don't save TOO much time\
\ by being able to guess a number between 1-3 in just two tries... but I wonder\
\ what we could do with one more try?\"
\"If we had three tries, what\ \ kind of range of numbers would we be able to always guess?\"
" @@ -640,11 +640,11 @@ states: The simplest thing would be if I had to guess a number from 1 to 1. Then I\ \ know the answer in one try - it's 1!\"\_
\"The next simplest thing\ + \ alt-with-value=\"""\">
\"The next simplest thing\
\ is if I had a number between 1 and 2. How many tries would I need?..\"
\_\
"(there's
+ filepath-with-value=""middleworst1-10_height_511_width_647.png"" alt-with-value="""">
"(there's
no whole number exactly between 1 and 10... we could of course try guessing
5 and a half, but that doesn't make a whole lot of sense!)"
\_
What would be the worst of those\ + \ alt-with-value=\"""\">
\_
What would be the worst of those\ \ cases for me?
" content_ids_to_audio_translations: content: {} @@ -1808,12 +1808,12 @@ states: every time you make a guess."
"Then,
+ caption-with-value="""" filepath-with-value=""range0-11_height_131_width_644.png"" alt-with-value="""">
"Then,
suppose we guess 5, and the Magician says it's bigger than that."
"Now
+ caption-with-value="""" filepath-with-value=""rangeadd5_height_246_width_695.png"" alt-with-value="""">
"Now
we just have to remember that it's greater than 5, but smaller
than 11. In other words, the range is 5-11 "
"Now
+ caption-with-value="""" filepath-with-value=""range5-11_height_90_width_467.png"" alt-with-value="""">
"Now
suppose after that, we guess 7; and the Magician says it's smaller than that!"
"What
is the range now? put it in the same format as '0-11' or '5-11' from before."
\_
\"So our first guess, and the information\ + \ alt-with-value=\"""\">
\_
\"So our first guess, and the information\
\ the magician gave us in response, narrowed down our possibilities to numbers\
\ between 1 and 3.\"
\_
\"What could his response have been?\"\ + \ alt-with-value=\"""\">
\_
\"What could his response have been?\"\
\_
" + \ alt-with-value=\"""\">\_
" content_ids_to_audio_translations: content: {} default_outcome: {} @@ -1982,7 +1982,7 @@ states: \ and that told us that the number is between 1 and 3, what must our guess\ \ have been?\"\_
" + \ alt-with-value=\"""\">\_
" content_ids_to_audio_translations: content: {} default_outcome: {} @@ -2064,7 +2064,7 @@ states: \ and 3? Now I think there's some kind of trick I can use to make it go faster.\"\\_
" + \ alt-with-value=\"""\">\_
" content_ids_to_audio_translations: content: {} default_outcome: {} @@ -2141,7 +2141,7 @@ states: content_id: content html: "\_
\"Wow! having three tries means\ + \ alt-with-value=\"""\">
\_
\"Wow! having three tries means\ \ that we can guess any number from 1 to 7! That's a lot better than what\ \ we could do with 2 tries!\"
\"In fact, the extra try more than doubled\ \ our range! On the first guess, we split our range into two, and then no\ @@ -2320,14 +2320,14 @@ states: \ guess a number in 3 tries. And we decided that on our first try, we should\ \ guess the number 4.\"
\_
\"If the magician says his number\ + \ alt-with-value=\"""\">
\_
\"If the magician says his number\ \ is less than 4, we already know what to do, and we can definitely guess\ \ that number in 2 more tries!\"
\_
\"OK, now suppose we guessed 4 but\ + \ alt-with-value=\"""\">
\_
\"OK, now suppose we guessed 4 but\ \ the magician told us his number was bigger than that!\"
\_
\"Now we have two tries left, and\ + \ alt-with-value=\"""\">
\_
\"Now we have two tries left, and\ \ we know that the number is between 5 and SOMETHING... and we're trying to\ \ figure out what that SOMETHING can be, so that we can still do this in two\ \ tries.\"
\_
\"I have a feeling that we can do almost the same\ @@ -2435,7 +2435,7 @@ states: \ numbers! So I figured out what I can do so I don't have to do too much!\ \ See?\"
He shows you his hand. It's covered in ink scribblings:
\"Just like we did!\"
\"That's\
+ \ alt-with-value=\"""\">
\"Just like we did!\"
\"That's\
\ right! \_I call these little notes algorithms. Hey, you\
\ guys are pretty clever - maybe you can help me figure out another one! You\
\ see, I really want to travel to a Far Away land, but I really don't want\
diff --git a/data/explorations/modeling_graphs/Graph Modeling.yaml b/data/explorations/modeling_graphs/Graph Modeling.yaml
index 973e2ae02aaf9..5f4129865ddd0 100644
--- a/data/explorations/modeling_graphs/Graph Modeling.yaml
+++ b/data/explorations/modeling_graphs/Graph Modeling.yaml
@@ -525,8 +525,8 @@ states:
\ it isn't: in simple terms, graphs are just dots joined by lines! The dots\
\ are called \"vertices\" and the lines are called \"edges\". An example of\
\ a graph is shown below:
\_
\_
\_
Despite this simplicity,\ + ""\" filepath-with-value=\""Asimplegraph_height_100_width_112.png"\"\ + \ alt-with-value=\"""\">
\_
\_
Despite this simplicity,\ \ graphs are useful for modeling all kinds of real world behavior. Not convinced?\ \ Let's take a look at some things that graphs can model...
" content_ids_to_audio_translations: diff --git a/data/explorations/modeling_graphs/assets/image/A simple graph_height_100_width_112.png b/data/explorations/modeling_graphs/assets/image/Asimplegraph_height_100_width_112.png similarity index 100% rename from data/explorations/modeling_graphs/assets/image/A simple graph_height_100_width_112.png rename to data/explorations/modeling_graphs/assets/image/Asimplegraph_height_100_width_112.png diff --git a/data/explorations/multiples.yaml b/data/explorations/multiples.yaml index d97b7018a677b..aebb857aa2bf6 100644 --- a/data/explorations/multiples.yaml +++ b/data/explorations/multiples.yaml @@ -105,6 +105,7 @@ states: audio_translations: {} html: "\_
" + \ alt-with-value=\"""\">\_
" -Option B
Can you see a pattern here, or a\
+ \ alt-with-value=\"""\">
Can you see a pattern here, or a\
\ systematic way to count them? Let's have a look at the 3-ball case.
--------------------------------------------------------------------------------------------
First\
\ you pick the ball on the left. This could be red, blue or yellow. There\
\ are three cases to consider:
If the first ball is red, then there\
\ are two balls left to arrange in the other two slots. And there are F2 ways\
\ to do this.
If the first ball is blue,\ + \ alt-with-value=\"""\">
If the first ball is blue,\ \ then there are two balls left to ... hey, this is the same thing, it's just\ \ F2.
\_
And, if the first ball is yellow,\
+ \ alt-with-value=\"""\">
\_
And, if the first ball is yellow,\
\ then ... yada, yada, F2.
So the total number of ways to arrange\
+ \ alt-with-value=\"""\">
So the total number of ways to arrange\
\ 3 balls, F3, is equal to 3 * F2. And all this works out correctly, because\
\ F2 = 2, and F3 = 3 * 2 = 6.
--------------------------------------------------------------------------------------------
Now,\
\ can you write out a similar expression for F2, in terms of F1? Then we'll\
@@ -539,8 +539,8 @@ states:
content:
content_id: content
html: "
\_
Suppose you were given three balls:\ + \ filepath-with-value=\""3balls_height_312_width_328.png"\"\ + \ alt-with-value=\"""\">
\_
Suppose you were given three balls:\ \ one red, one blue, and one yellow -- and one day, you are idly playing around\ \ with them. Soon, you discover that you can make patterns by arranging the\ \ balls in a straight line; let's pretend you're doing this with the line\ @@ -666,7 +666,7 @@ states: \ the red ball on the left, the blue ball in the middle, and the yellow ball\ \ on the right, like this:
\_
Can you list all the ways you found?
\_\
+ \ alt-with-value=\"""\">
\_
Can you list all the ways you found?
\_\
\ \_\_
\_
" + \ alt-with-value=\"""\">\_
" content_ids_to_audio_translations: content: {} default_outcome: {} diff --git a/data/explorations/three_balls/assets/image/3 balls_height_312_width_328.png b/data/explorations/three_balls/assets/image/3balls_height_312_width_328.png similarity index 100% rename from data/explorations/three_balls/assets/image/3 balls_height_312_width_328.png rename to data/explorations/three_balls/assets/image/3balls_height_312_width_328.png diff --git a/extensions/rich_text_components/components.py b/extensions/rich_text_components/components.py index 1c862d5dd5145..78269e5caacf9 100644 --- a/extensions/rich_text_components/components.py +++ b/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))$' filepath = value_dict['filepath-with-value'] if not re.match(filename_re, filepath): raise Exception('Invalid filepath')