From bdefb87757ac26b56fd582db3f69344ea3093b2c Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Fri, 14 Jul 2023 13:50:46 -0500 Subject: [PATCH 01/20] feat: Initial import/export configuration and Parsers - Added Parser, JSONParser and CSVParser classes to validate file and tags format - Tests added --- .../core/tagging/import_export/__init__.py | 0 .../core/tagging/import_export/actions.py | 79 ++++++ .../core/tagging/import_export/api.py | 38 +++ .../core/tagging/import_export/dsl.py | 47 ++++ .../core/tagging/import_export/exceptions.py | 37 +++ .../core/tagging/import_export/parsers.py | 228 ++++++++++++++++++ .../core/tagging/import_export/utils.py | 0 .../tagging/import_export/test_parsers.py | 204 ++++++++++++++++ 8 files changed, 633 insertions(+) create mode 100644 openedx_tagging/core/tagging/import_export/__init__.py create mode 100644 openedx_tagging/core/tagging/import_export/actions.py create mode 100644 openedx_tagging/core/tagging/import_export/api.py create mode 100644 openedx_tagging/core/tagging/import_export/dsl.py create mode 100644 openedx_tagging/core/tagging/import_export/exceptions.py create mode 100644 openedx_tagging/core/tagging/import_export/parsers.py create mode 100644 openedx_tagging/core/tagging/import_export/utils.py create mode 100644 tests/openedx_tagging/core/tagging/import_export/test_parsers.py diff --git a/openedx_tagging/core/tagging/import_export/__init__.py b/openedx_tagging/core/tagging/import_export/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/openedx_tagging/core/tagging/import_export/actions.py b/openedx_tagging/core/tagging/import_export/actions.py new file mode 100644 index 000000000..f6cc297e6 --- /dev/null +++ b/openedx_tagging/core/tagging/import_export/actions.py @@ -0,0 +1,79 @@ +from ..models import Taxonomy +from .dsl import TagDSL + +class ImportAction: + + @classmethod + def valid_for(cls, taxonomy: Taxonomy, tag: TagDSL): + raise NotImplementedError + + def validate(self): + raise NotImplementedError + + def execute(self): + raise NotImplementedError + + def __init__(self, taxonomy: Taxonomy, tag: TagDSL): + self.taxonomy = taxonomy + self.tag = tag + + +class CreateTag(ImportAction): + + @classmethod + def valid_for(cls, taxonomy: Taxonomy, tag: TagDSL): + pass + + def validate(self): + pass + + def execute(self): + pass + + +class UpdateParentTag(ImportAction): + + @classmethod + def valid_for(cls, taxonomy: Taxonomy, tag: TagDSL): + pass + + def validate(self): + pass + + def execute(self): + pass + + +class RenameTag(ImportAction): + + @classmethod + def valid_for(cls, taxonomy: Taxonomy, tag: TagDSL): + pass + + def validate(self): + pass + + def execute(self): + pass + + +class DeleteTag(ImportAction): + + @classmethod + def valid_for(cls, taxonomy: Taxonomy, tag: TagDSL): + pass + + def validate(self): + pass + + def execute(self): + pass + + +# Register actions here +available_actions = [ + CreateTag, + UpdateParentTag, + RenameTag, + DeleteTag, +] diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py new file mode 100644 index 000000000..a60b6d552 --- /dev/null +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -0,0 +1,38 @@ +from io import BytesIO +from typing import List + +from ..models import Taxonomy +from .parsers import get_parser, ParserFormat +from .exceptions import ImportError +from .dsl import TagImportDSL + +# TODO This function must be a celery task +def import_tags( + taxonomy: Taxonomy, + file: BytesIO, + format: ParserFormat, + replace=False, + execute=False, +): + # Get the parser and parse the file + parser = get_parser(format) + tags, errors = parser.parse_import(file) + + # Check if there are errors in the parse + if errors: + return errors + + # Generate the actions + dsl = TagImportDSL() + errors = dsl.generate_actions(tags, taxonomy, replace) + + # Check if there are inconsistent actions or errors + if errors: + return errors + + # Execute the plan + if execute: + return dsl.execute() + + # Or return the plan + return dsl.plan() diff --git a/openedx_tagging/core/tagging/import_export/dsl.py b/openedx_tagging/core/tagging/import_export/dsl.py new file mode 100644 index 000000000..0be516cce --- /dev/null +++ b/openedx_tagging/core/tagging/import_export/dsl.py @@ -0,0 +1,47 @@ +from typing import List + +from ..models import Taxonomy +from .actions import ImportAction, available_actions + +class TagDSL: + """ + Tag representation on the import DSL + """ + id: str + value: str + parent_id: str + action: str + + def __init__( + self, + id: str, + value: str, + parent_id: str=None, + action: str=None, + ): + self.id = id + self.value = value + self.parent_id = parent_id + self.action = action + + +class TagImportDSL: + actions: List[ImportAction] + taxonomy: Taxonomy + + def __init__(self, taxonomy: Taxonomy): + self.actions = [] + self.taxonomy = taxonomy + + def generate_actions( + self, + tags: List[TagDSL], + reaplace=False, + ) -> List[ImportError]: + pass + + def execute(self) -> List[ImportError]: + pass + + def plan(self) -> List[str]: + pass diff --git a/openedx_tagging/core/tagging/import_export/exceptions.py b/openedx_tagging/core/tagging/import_export/exceptions.py new file mode 100644 index 000000000..6c0dd69f5 --- /dev/null +++ b/openedx_tagging/core/tagging/import_export/exceptions.py @@ -0,0 +1,37 @@ +from django.utils.translation import gettext_lazy as _ + +class ImportError(Exception): + def __init__(self, **kargs): + self.message = _(f"Import error") + + def __str__(self): + return str(self.message) + + +class ParserError(ImportError): + def __init__(self, tag: str, **kargs): + self.message = _(f"Import error on {tag}") + + +class InvalidFormat(ParserError): + def __init__(self, tag: dict, format: str, message: str, **kargs): + self.tag = tag + self.message = _(f"Invalid '{format}' format: {message}") + + +class FieldJSONError(ParserError): + def __init__(self, tag, field, **kargs): + self.tag = tag + self.message = _(f"Missing '{field}' field on {tag}") + + +class EmptyJSONField(ParserError): + def __init__(self, tag, field, **kargs): + self.tag = tag + self.message = _(f"Empty '{field}' field on {tag}") + + +class EmptyCSVField(ParserError): + def __init__(self, tag, field, row, **kargs): + self.tag = tag + self.message = _(f"Empty '{field}' field on the row {row}") diff --git a/openedx_tagging/core/tagging/import_export/parsers.py b/openedx_tagging/core/tagging/import_export/parsers.py new file mode 100644 index 000000000..6e2291cb2 --- /dev/null +++ b/openedx_tagging/core/tagging/import_export/parsers.py @@ -0,0 +1,228 @@ +""" +Parsers to import and export tags +""" +import csv +import json +from enum import Enum +from io import BytesIO, TextIOWrapper +from typing import List, Tuple + +from django.utils.translation import gettext_lazy as _ + +from .dsl import TagDSL +from .exceptions import ( + ParserError, + InvalidFormat, + FieldJSONError, + EmptyJSONField, + EmptyCSVField, +) + +class ParserFormat(Enum): + """ + Format of import tags to taxonomies + """ + JSON = ".json" + CSV = ".csv" + + +class Parser: + """ + Base class to create a parser + + This contains the base functions to load data, + validate required fields and convert tags to DLS format + """ + required_fields = ['id', 'value'] + optional_fields = ['parent_id', 'action'] + + # Set the format associated to the parser + format = None + # We can change the error when is missing a required field + missing_field_error = ParserError + # We can change the error when a required field is empty + empty_field_error = ParserError + # We can change the initial row + inital_row = 1 + + @classmethod + def parse_import(cls, file: BytesIO) -> Tuple[List[TagDSL], List[ParserError]]: + """ + Top function that calls `_load_data` and `_parse_tags`. + Handle the errors returned both functions + """ + try: + tags_data, load_errors = cls._load_data(file) + if load_errors: + return [], load_errors + finally: + file.close() + + return cls._parse_tags(tags_data) + + def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[ParserError]]: + """ + Each parser implements this function according to its format. + This function reads the file and returns a list with the values of each tag. + + This function does not do field validations, it only does validations of the + file structure in the parser format. Field validations are done in `_parse_tags` + """ + raise NotImplementedError + + @classmethod + def _parse_tags(cls, tags_data: dict) -> Tuple[List[TagDSL], List[ParserError]]: + """ + Validate the required fields of each tag. + + Return a list of tags in the DSL format + and a list of validation errors. + """ + tags = [] + errors = [] + row = cls.inital_row + for tag in tags_data: + has_error = False + + # Verify the required fields + for req_field in cls.required_fields: + if req_field not in tag: + # Verify if the field exists + errors.append(cls.missing_field_error( + tag, + field=req_field, + row=row, + )) + has_error = True + elif not tag.get(req_field): + # Verify if the value of the field is not empty + errors.append(cls.empty_field_error( + tag, + field=req_field, + row=row, + )) + has_error = True + + row += 1 + + # Skip parse if there is an error + if has_error: + continue + + # Updating any empty optional field to None + for opt_field in cls.optional_fields: + if opt_field in tag and not tag.get(opt_field): + tag[opt_field] = None + + tags.append(TagDSL(**tag)) + + return tags, errors + + +class JSONParser(Parser): + """ + Parser used with .json files + + Valid file: + ``` + { + "tags": [ + { + "id": "tag_1", + "value": "tag 1", + "parent_id": "tag_2", + } + ] + } + ``` + """ + format = ParserFormat.JSON + missing_field_error = FieldJSONError + empty_field_error = EmptyJSONField + + @classmethod + def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[ParserError]]: + """ + Read a .json file and validates the root structure of the json + """ + file.seek(0) + tags_data = json.load(file) + if 'tags' not in tags_data: + return None, [InvalidFormat( + tag=None, + format=cls.format.value, + message=_("Missing 'tags' field on the .json file") + )] + + tags_data = tags_data.get('tags') + return tags_data, [] + + +class CSVParser(Parser): + """ + Parser used with .csv files + + Valid file: + ``` + id,value,parent_id + tag_1,tag 1, + tag_2,tag 2,tag_1 + ``` + """ + format = ParserFormat.CSV + empty_field_error = EmptyCSVField + inital_row = 2 + + @classmethod + def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[ParserError]]: + """ + Read a .csv file and validates the header fields + """ + file.seek(0) + text_tags = TextIOWrapper(file, encoding='utf-8') + csv_reader = csv.DictReader(text_tags) + header_fields = csv_reader.fieldnames + errors = cls._veify_header(header_fields) + if errors: + return None, errors + return list(csv_reader), [] + + @classmethod + def _veify_header(cls, header_fields: List[str]) -> List[ParserError]: + """ + Verify that the header contains the required fields + """ + errors = [] + print(header_fields) + for req_field in cls.required_fields: + if req_field not in header_fields: + errors.append(InvalidFormat( + tag=None, + format=cls.format.value, + message=_(f"Missing '{req_field}' field on CSV headers") + )) + return errors + + +# Add parsers here +_parsers = [ + JSONParser, + CSVParser +] + + +def get_parser(format: ParserFormat) -> Parser: + """ + Get the parser for the respective `format` + + Raise `ValueError` if no parser found + """ + for parser in _parsers: + if format == parser.format: + return parser + + raise ValueError( + _( + f"Parser not found for format {format}" + ) + ) diff --git a/openedx_tagging/core/tagging/import_export/utils.py b/openedx_tagging/core/tagging/import_export/utils.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py new file mode 100644 index 000000000..1aea855cb --- /dev/null +++ b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py @@ -0,0 +1,204 @@ +""" +Test for import/export parsers +""" +from io import BytesIO +import json +import ddt + +from django.test.testcases import TestCase + +from openedx_tagging.core.tagging.import_export.parsers import ( + get_parser, + JSONParser, + CSVParser, + ParserFormat, +) + + +class TestParser(TestCase): + """ + Test for general parser functions + """ + + def test_get_parser(self): + for format in ParserFormat: + parser = get_parser(format) + self.assertEqual(parser.format, format) + + def test_parser_not_found(self): + with self.assertRaises(ValueError): + get_parser(None) + +@ddt.ddt +class TestJSONParser(TestCase): + """ + Test for .json parser + """ + + def test_load_data_errors(self): + json_data = {"invalid": [ + {"id": "tag_1", "name": "Tag 1"}, + ]} + + json_file = BytesIO(json.dumps(json_data).encode()) + + tags, errors = JSONParser.parse_import(json_file) + self.assertEqual(len(tags), 0) + self.assertEqual(len(errors), 1) + self.assertEqual( + str(errors[0]), + "Invalid '.json' format: Missing 'tags' field on the .json file" + ) + + @ddt.data( + ( + {"tags": [ + {"id": "tag_1", "value": "Tag 1"}, # Valid + ]}, + [] + ), + ( + {"tags": [ + {"id": "tag_1"}, + {"value": "tag_1"}, + {}, + ]}, + [ + "Missing 'value' field on {'id': 'tag_1'}", + "Missing 'id' field on {'value': 'tag_1'}", + "Missing 'value' field on {}", + "Missing 'id' field on {}", + ] + ), + ( + {"tags": [ + {"id": "", "value": "tag 1"}, + {"id": "tag_2", "value": ""}, + {"id": "tag_3", "value": "tag 3", "parent_id": ""}, # Valid + ]}, + [ + "Empty 'id' field on {'id': '', 'value': 'tag 1'}", + "Empty 'value' field on {'id': 'tag_2', 'value': ''}", + ] + ) + ) + @ddt.unpack + def test_parse_tags_errors(self, json_data, expected_errors): + json_file = BytesIO(json.dumps(json_data).encode()) + + _, errors = JSONParser.parse_import(json_file) + self.assertEqual(len(errors), len(expected_errors)) + + for error in errors: + self.assertIn(str(error), expected_errors) + + def test_parse_tags(self): + expected_tags = [ + {"id": "tag_1", "value": "tag 1"}, + {"id": "tag_2", "value": "tag 2"}, + {"id": "tag_3", "value": "tag 3", "parent_id": "tag_1"}, + {"id": "tag_4", "value": "tag 4", "action": "delete"}, + ] + json_data = {"tags": expected_tags} + + json_file = BytesIO(json.dumps(json_data).encode()) + + tags, errors = JSONParser.parse_import(json_file) + self.assertEqual(len(errors), 0) + self.assertEqual(len(tags), 4) + + # Result tags must be in the same order of the file + for index in range(0, len(expected_tags)): + self.assertEqual(tags[index].id, expected_tags[index].get('id')) + self.assertEqual(tags[index].value, expected_tags[index].get('value')) + self.assertEqual(tags[index].parent_id, expected_tags[index].get('parent_id')) + self.assertEqual(tags[index].action, expected_tags[index].get('action')) + +@ddt.ddt +class TestCSVParser(TestCase): + """ + Test for .csv parser + """ + + @ddt.data( + ( + "value\n", + ["Invalid '.csv' format: Missing 'id' field on CSV headers"], + ), + ( + "id\n", + ["Invalid '.csv' format: Missing 'value' field on CSV headers"], + ), + ( + "id_name,value_name\n", + [ + "Invalid '.csv' format: Missing 'id' field on CSV headers", + "Invalid '.csv' format: Missing 'value' field on CSV headers" + ], + ), + ( + # Valid + "id,value\n", + [] + ) + ) + @ddt.unpack + def test_load_data_errors(self, csv_data, expected_errors): + csv_file = BytesIO(csv_data.encode()) + + tags, errors = CSVParser.parse_import(csv_file) + self.assertEqual(len(tags), 0) + self.assertEqual(len(errors), len(expected_errors)) + + for error in errors: + self.assertIn(str(error), expected_errors) + + @ddt.data( + ( + "id,value\ntag_1\ntag_2,\n", + [ + "Empty 'value' field on the row 2", + "Empty 'value' field on the row 3", + ] + ), + ( + "id,value\ntag_1,tag 1\n", # Valid + [] + ) + ) + @ddt.unpack + def test_parse_tags_errors(self, csv_data, expected_errors): + csv_file = BytesIO(csv_data.encode()) + + _, errors = CSVParser.parse_import(csv_file) + self.assertEqual(len(errors), len(expected_errors)) + + for error in errors: + self.assertIn(str(error), expected_errors) + + def _build_csv(self, tags): + csv = "id,value,parent_id,action\n" + for tag in tags: + csv += f"{tag.get('id')},{tag.get('value')},{tag.get('parent_id') or ''},{tag.get('action') or ''}\n" + return csv + + def test_parse_tags(self): + expected_tags = [ + {"id": "tag_1", "value": "tag 1"}, + {"id": "tag_2", "value": "tag 2"}, + {"id": "tag_3", "value": "tag 3", "parent_id": "tag_1"}, + {"id": "tag_4", "value": "tag 4", "action": "delete"}, + ] + csv_data = self._build_csv(expected_tags) + csv_file = BytesIO(csv_data.encode()) + tags, errors = CSVParser.parse_import(csv_file) + + self.assertEqual(len(errors), 0) + self.assertEqual(len(tags), 4) + + # Result tags must be in the same order of the file + for index in range(0, len(expected_tags)): + self.assertEqual(tags[index].id, expected_tags[index].get('id')) + self.assertEqual(tags[index].value, expected_tags[index].get('value')) + self.assertEqual(tags[index].parent_id, expected_tags[index].get('parent_id')) + self.assertEqual(tags[index].action, expected_tags[index].get('action')) From f601c5b4d22d28ee54e718cc073c97eed1770745 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Mon, 17 Jul 2023 16:24:20 -0500 Subject: [PATCH 02/20] feat: TagImportDSL and action implementation --- .../core/tagging/import_export/actions.py | 321 ++++++++++++++++-- .../core/tagging/import_export/dsl.py | 92 ++++- .../core/tagging/import_export/exceptions.py | 26 +- .../core/tagging/import_export/parsers.py | 4 +- .../tagging/import_export/test_parsers.py | 53 ++- 5 files changed, 455 insertions(+), 41 deletions(-) diff --git a/openedx_tagging/core/tagging/import_export/actions.py b/openedx_tagging/core/tagging/import_export/actions.py index f6cc297e6..70c7db887 100644 --- a/openedx_tagging/core/tagging/import_export/actions.py +++ b/openedx_tagging/core/tagging/import_export/actions.py @@ -1,79 +1,350 @@ -from ..models import Taxonomy -from .dsl import TagDSL +from typing import List + +from django.utils.translation import gettext_lazy as _ + +from ..models import Taxonomy, Tag +from .exceptions import ActionError, ActionConflict + class ImportAction: + """ + Base class to create actions + """ + + name = '' + + def __init__(self, taxonomy: Taxonomy, tag, index: int): + self.taxonomy = taxonomy + self.tag = tag + self.index = index @classmethod - def valid_for(cls, taxonomy: Taxonomy, tag: TagDSL): + def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: + """ + Implement this to meet the conditions that a `TagDSL` needs + to have for this action. All actions that are valid with + this function are created. + """ raise NotImplementedError - def validate(self): + def validate(self, indexed_actions) -> List[ActionError]: + """ + Implement this to find inconsistencies with tags in the + database or with previous actions. + """ raise NotImplementedError def execute(self): raise NotImplementedError - def __init__(self, taxonomy: Taxonomy, tag: TagDSL): - self.taxonomy = taxonomy - self.tag = tag + def _search_action( + self, + indexed_actions: dict, + action_name: str, + attr: str, + search_value: str, + ): + """ + Use this function to find and action using an `attr` of `TagDSL` + """ + for action in indexed_actions[action_name]: + if search_value == getattr(action.tag, attr): + return action + + return None + + def _validate_parent(self, indexed_actions) -> ActionError: + """ + Validates if the parent is created + """ + # Validates that the parent exists on the taxonomy + try: + self.taxonomy.tag_set.get(external_id=self.tag.parent_id) + return None + except Tag.DoesNotExist: + # Or if the parent is created on previous actions + if not self._search_action(indexed_actions, CreateTag.name, 'id', self.tag.parent_id): + return ActionError( + action=self, + tag_id=self.tag.id, + message=_( + f"Unknown parent tag ({self.tag.parent_id})." + "You need to add parent before the child in your file" + ) + ) + + def _validate_value(self, indexed_actions): + """ + Check for value duplicates in the models and in previous create/rename + actions + """ + try: + # Validates if exists a tag with the same value on the Taxonomy + tag = self.taxonomy.tag_set.get(value=self.tag.value) + return ActionError( + action=self, + tag_id=self.tag.id, + message=_(f"Duplicated tag value with tag ({tag.id})") + ) + except Tag.DoesNotExist: + # Validates value duplication on create actions + action = self._search_action( + indexed_actions, + CreateTag.name, + 'value', + self.tag.value, + ) + + if not action: + # Validates value duplication on rename actions + action = self._search_action( + indexed_actions, + RenameTag.name, + 'value', + self.tag.value, + ) + + if action: + return ActionConflict( + action=self, + tag_id=self.tag_id, + conflict_action_index=action.index, + message=_("Duplicated tag value") + ) class CreateTag(ImportAction): + """ + Action for create a Tag + + Action created if the tag doesn't exist on the database + + Validations: + - Id duplicates with previous create actions. + - Value duplicates with tags on the database. + - Value duplicates with previous create and rename actions. + - Parent validation. If the parent is in the database or created + in previous actions. + """ + + name = 'create' @classmethod - def valid_for(cls, taxonomy: Taxonomy, tag: TagDSL): - pass + def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: + """ + Validates if the tag does not exist + """ + try: + taxonomy.tag_set.get(external_id=tag.id) + return False + except Tag.DoesNotExist: + return True - def validate(self): - pass + def _validate_id(self, indexed_actions): + """ + Check for id duplicates in previous create actions + """ + action = self._search_action( + indexed_actions, + self.name, + 'id', + self.tag.id + ) + if action: + return ActionConflict( + action=self, + tag_id=self.tag_id, + conflict_action_index=action.index, + message=_("Duplicated id tag") + ) + + def validate(self, indexed_actions) -> List[ActionError]: + """ + Validates the creation action + """ + errors = [] + + # Duplicate id validation with previous create actions + error = self._validate_id(indexed_actions) + if error: + errors.append(error) + + # Duplicate value validation + error = self._validate_value(indexed_actions) + if error: + errors.append(error) + + # Parent validation + if self.tag.parent_id: + error = self._validate_parent(indexed_actions) + if error: + errors.append(error) + + return errors def execute(self): pass class UpdateParentTag(ImportAction): + """ + Action for update the parent of a Tag + + Action created if there is a change on the parent + + Validations: + - Parent validation. If the parent is in the database + or created in previous actions. + """ + + name = 'update_parent' @classmethod - def valid_for(cls, taxonomy: Taxonomy, tag: TagDSL): - pass + def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: + """ + Validates there is a change on the parent + """ + try: + taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) + return ( + ( + taxonomy_tag.parent is not None + and taxonomy_tag.parent.external_id != tag.parent_id + ) + or + ( + taxonomy_tag.parent is None and tag.parent_id is not None + ) + ) + except Tag.DoesNotExist: + return False - def validate(self): - pass + def validate(self, indexed_actions) -> List[ActionError]: + """ + Validates the update parent action + """ + errors = [] + + # Parent validation + if self.tag.parent_id: + error = self._validate_parent(indexed_actions) + if error: + errors.append(error) + + return errors def execute(self): pass class RenameTag(ImportAction): + """ + Action for rename a Tag + + Action created if there is a change on the tag value + + Validations: + - Value duplicates with tags on the database. + - Value duplicates with previous create and rename actions. + """ + + name = 'rename' @classmethod - def valid_for(cls, taxonomy: Taxonomy, tag: TagDSL): - pass + def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: + """ + Validates there is a change on the tag value + """ + try: + taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) + return ( + taxonomy_tag.value != tag.value + ) + except Tag.DoesNotExist: + return False - def validate(self): - pass + def validate(self, indexed_actions) -> List[ActionError]: + """ + Validates the rename action + """ + errors = [] + + # Duplicate value validation + error = self._validate_value(indexed_actions) + if error: + errors.append(error) + + return errors def execute(self): pass class DeleteTag(ImportAction): + """ + Action for delete a Tag + + Action created if the action of the tag is 'delete' + + Does not require validations + """ + + name = 'delete' @classmethod - def valid_for(cls, taxonomy: Taxonomy, tag: TagDSL): - pass + def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: + """ + Validates if the action is delete and the tag exists + """ + try: + taxonomy.tag_set.get(external_id=tag.id) + return tag.action == cls.name + except Tag.DoesNotExist: + return False - def validate(self): + def validate(self, indexed_actions) -> List[ActionError]: + """ + No validations necessary + """ + # TODO: Will it be necessary to check if this tag has children? + return True + + def execute(self): pass +class WithoutChanges(ImportAction): + """ + Action when there is no change on the Tag + + Does not require validations + """ + + name = 'without_changes' + + @classmethod + def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: + """ + No validations necessary + """ + return True + + + def validate(self, indexed_actions) -> List[ActionError]: + """ + No validations necessary + """ + return True + def execute(self): - pass + pass -# Register actions here +# Register actions here in the order in which you want to check +# 'WithoutChanges' action goes last always available_actions = [ - CreateTag, UpdateParentTag, RenameTag, + CreateTag, DeleteTag, + WithoutChanges, ] diff --git a/openedx_tagging/core/tagging/import_export/dsl.py b/openedx_tagging/core/tagging/import_export/dsl.py index 0be516cce..a4b5e93b0 100644 --- a/openedx_tagging/core/tagging/import_export/dsl.py +++ b/openedx_tagging/core/tagging/import_export/dsl.py @@ -1,7 +1,10 @@ from typing import List +from django.utils.translation import gettext_lazy as _ + from ..models import Taxonomy -from .actions import ImportAction, available_actions +from .actions import DeleteTag, ImportAction, UpdateParentTag, available_actions + class TagDSL: """ @@ -11,34 +14,115 @@ class TagDSL: value: str parent_id: str action: str + index: int def __init__( self, id: str, value: str, + index: str, parent_id: str=None, action: str=None, ): self.id = id self.value = value + self.index = index self.parent_id = parent_id self.action = action class TagImportDSL: actions: List[ImportAction] + indexed_actions: dict + actions_dict: dict taxonomy: Taxonomy def __init__(self, taxonomy: Taxonomy): self.actions = [] + self.errors = [] self.taxonomy = taxonomy + self.indexed_actions = {} + self.actions_dict = {} + for action in available_actions: + self.indexed_actions[action.name] = [] + + def _build_action(self, action_cls, tag: TagDSL): + """ + Build an action with `tag`. + + Run action validation and adds the errors to the errors lists + Add to the action list and the indexed actions + """ + action = action_cls(self.taxonomy, tag, len(self.actions)) + + # We validate if there are no inconsistencies when executing this action + self.errors.append(action.validate(self.indexed_actions)) + + # Add action + self.actions.append(action) + + # Index the actions for search + self.indexed_actions[action.name].append(action) + + def _delete_tags(self, tags: List[str]): + """ + Delete `tags` + """ + for tag in tags: + for child in tag.children: + if child.external_id not in tags: + # If the child is not to be removed, + # then update its parent + self._build_action( + UpdateParentTag, + TagDSL( + id=child.external_id, + value=child.value, + parent_id=None, + ) + ) + + # Delete action + self._build_action( + DeleteTag, + TagDSL( + id=tag.external_id, + ) + ) def generate_actions( self, tags: List[TagDSL], - reaplace=False, - ) -> List[ImportError]: - pass + replace=False, + ): + """ + Generates actions from `tags`. + + Validates each action and create respective errors + If `replace` is True, then deletes the tags that have not been read + """ + self.actions.clear() + self.errors.clear() + tags_for_delete = {} + + if replace: + tags_for_delete = { + tag.external_id: tag + for tag in self.taxonomy.tag_set + } + + for tag in tags: + # Check all available actions and add which ones should be executed + for action_cls in available_actions: + if action_cls.valid_for(self.taxonomy, tag): + self._build_action(action_cls, tag) + + if replace: + tags_for_delete.pop(tag.id) + + if replace: + # Delete all not readed tags + self._delete_tags(tags_for_delete) def execute(self) -> List[ImportError]: pass diff --git a/openedx_tagging/core/tagging/import_export/exceptions.py b/openedx_tagging/core/tagging/import_export/exceptions.py index 6c0dd69f5..599058aa8 100644 --- a/openedx_tagging/core/tagging/import_export/exceptions.py +++ b/openedx_tagging/core/tagging/import_export/exceptions.py @@ -1,8 +1,8 @@ from django.utils.translation import gettext_lazy as _ class ImportError(Exception): - def __init__(self, **kargs): - self.message = _(f"Import error") + def __init__(self, message:str='', **kargs): + self.message = message def __str__(self): return str(self.message) @@ -13,6 +13,28 @@ def __init__(self, tag: str, **kargs): self.message = _(f"Import error on {tag}") +class ActionError(ImportError): + def __init__(self, action: str, tag_id: str, message: str, **kargs): + self.message = _( + f"Action error in '{action.name}' (#{action.index}) in tag ({tag_id}): {message}" + ) + + +class ActionConflict(ActionError): + def __init__( + self, + action: str, + tag_id: str, + conflict_action_index: int, + message: str, + **kargs + ): + self.message = _( + f"Conflict with '{action.name}' (#{action.index}) in tag ({tag_id})" + f" and action #{conflict_action_index}: {message}" + ) + + class InvalidFormat(ParserError): def __init__(self, tag: dict, format: str, message: str, **kargs): self.tag = tag diff --git a/openedx_tagging/core/tagging/import_export/parsers.py b/openedx_tagging/core/tagging/import_export/parsers.py index 6e2291cb2..c11b9b936 100644 --- a/openedx_tagging/core/tagging/import_export/parsers.py +++ b/openedx_tagging/core/tagging/import_export/parsers.py @@ -42,7 +42,7 @@ class Parser: missing_field_error = ParserError # We can change the error when a required field is empty empty_field_error = ParserError - # We can change the initial row + # We can change the initial row/index inital_row = 1 @classmethod @@ -103,6 +103,7 @@ def _parse_tags(cls, tags_data: dict) -> Tuple[List[TagDSL], List[ParserError]]: )) has_error = True + tag["index"] = row row += 1 # Skip parse if there is an error @@ -139,6 +140,7 @@ class JSONParser(Parser): format = ParserFormat.JSON missing_field_error = FieldJSONError empty_field_error = EmptyJSONField + inital_row = 0 @classmethod def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[ParserError]]: diff --git a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py index 1aea855cb..04147238b 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py @@ -109,10 +109,26 @@ def test_parse_tags(self): # Result tags must be in the same order of the file for index in range(0, len(expected_tags)): - self.assertEqual(tags[index].id, expected_tags[index].get('id')) - self.assertEqual(tags[index].value, expected_tags[index].get('value')) - self.assertEqual(tags[index].parent_id, expected_tags[index].get('parent_id')) - self.assertEqual(tags[index].action, expected_tags[index].get('action')) + self.assertEqual( + tags[index].id, + expected_tags[index].get('id') + ) + self.assertEqual( + tags[index].value, + expected_tags[index].get('value') + ) + self.assertEqual( + tags[index].parent_id, + expected_tags[index].get('parent_id') + ) + self.assertEqual( + tags[index].action, + expected_tags[index].get('action') + ) + self.assertEqual( + tags[index].index, + index + JSONParser.inital_row + ) @ddt.ddt class TestCSVParser(TestCase): @@ -179,7 +195,10 @@ def test_parse_tags_errors(self, csv_data, expected_errors): def _build_csv(self, tags): csv = "id,value,parent_id,action\n" for tag in tags: - csv += f"{tag.get('id')},{tag.get('value')},{tag.get('parent_id') or ''},{tag.get('action') or ''}\n" + csv += ( + f"{tag.get('id')},{tag.get('value')}," + f"{tag.get('parent_id') or ''},{tag.get('action') or ''}\n" + ) return csv def test_parse_tags(self): @@ -198,7 +217,23 @@ def test_parse_tags(self): # Result tags must be in the same order of the file for index in range(0, len(expected_tags)): - self.assertEqual(tags[index].id, expected_tags[index].get('id')) - self.assertEqual(tags[index].value, expected_tags[index].get('value')) - self.assertEqual(tags[index].parent_id, expected_tags[index].get('parent_id')) - self.assertEqual(tags[index].action, expected_tags[index].get('action')) + self.assertEqual( + tags[index].id, + expected_tags[index].get('id') + ) + self.assertEqual( + tags[index].value, + expected_tags[index].get('value') + ) + self.assertEqual( + tags[index].parent_id, + expected_tags[index].get('parent_id') + ) + self.assertEqual( + tags[index].action, + expected_tags[index].get('action') + ) + self.assertEqual( + tags[index].index, + index + CSVParser.inital_row + ) From 66d93ed01ca0895bb9ba1a10e5298e2325cd1865 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Tue, 18 Jul 2023 14:34:05 -0500 Subject: [PATCH 03/20] test: Import actions tests & DSL tests --- .../core/tagging/import_export/actions.py | 35 +- .../core/tagging/import_export/api.py | 10 +- .../core/tagging/import_export/dsl.py | 63 ++- .../core/tagging/import_export/exceptions.py | 3 + .../core/tagging/import_export/parsers.py | 6 +- .../core/fixtures/tagging.yaml | 38 ++ .../core/tagging/import_export/__init__.py | 0 .../tagging/import_export/test_actions.py | 389 ++++++++++++++++++ .../core/tagging/import_export/test_dsl.py | 203 +++++++++ .../tagging/import_export/test_parsers.py | 35 +- .../openedx_tagging/core/tagging/test_api.py | 16 +- 11 files changed, 735 insertions(+), 63 deletions(-) create mode 100644 tests/openedx_tagging/core/tagging/import_export/__init__.py create mode 100644 tests/openedx_tagging/core/tagging/import_export/test_actions.py create mode 100644 tests/openedx_tagging/core/tagging/import_export/test_dsl.py diff --git a/openedx_tagging/core/tagging/import_export/actions.py b/openedx_tagging/core/tagging/import_export/actions.py index 70c7db887..df02df9f8 100644 --- a/openedx_tagging/core/tagging/import_export/actions.py +++ b/openedx_tagging/core/tagging/import_export/actions.py @@ -1,3 +1,6 @@ +""" +Actions for import tags +""" from typing import List from django.utils.translation import gettext_lazy as _ @@ -11,13 +14,16 @@ class ImportAction: Base class to create actions """ - name = '' + name = 'import_action' def __init__(self, taxonomy: Taxonomy, tag, index: int): self.taxonomy = taxonomy self.tag = tag self.index = index + def __repr__(self): + return f"Action {self.name} (index={self.index},id={self.tag.id})" + @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: """ @@ -57,11 +63,9 @@ def _validate_parent(self, indexed_actions) -> ActionError: """ Validates if the parent is created """ - - # Validates that the parent exists on the taxonomy try: + # Validates that the parent exists on the taxonomy self.taxonomy.tag_set.get(external_id=self.tag.parent_id) - return None except Tag.DoesNotExist: # Or if the parent is created on previous actions if not self._search_action(indexed_actions, CreateTag.name, 'id', self.tag.parent_id): @@ -69,8 +73,8 @@ def _validate_parent(self, indexed_actions) -> ActionError: action=self, tag_id=self.tag.id, message=_( - f"Unknown parent tag ({self.tag.parent_id})." - "You need to add parent before the child in your file" + f"Unknown parent tag ({self.tag.parent_id}). " + "You need to add parent before the child in your file." ) ) @@ -85,7 +89,7 @@ def _validate_value(self, indexed_actions): return ActionError( action=self, tag_id=self.tag.id, - message=_(f"Duplicated tag value with tag ({tag.id})") + message=_(f"Duplicated tag value with tag (pk={tag.id}).") ) except Tag.DoesNotExist: # Validates value duplication on create actions @@ -108,9 +112,9 @@ def _validate_value(self, indexed_actions): if action: return ActionConflict( action=self, - tag_id=self.tag_id, + tag_id=self.tag.id, conflict_action_index=action.index, - message=_("Duplicated tag value") + message=_("Duplicated tag value.") ) class CreateTag(ImportAction): @@ -153,9 +157,9 @@ def _validate_id(self, indexed_actions): if action: return ActionConflict( action=self, - tag_id=self.tag_id, + tag_id=self.tag.id, conflict_action_index=action.index, - message=_("Duplicated id tag") + message=_("Duplicated id tag.") ) def validate(self, indexed_actions) -> List[ActionError]: @@ -307,7 +311,7 @@ def validate(self, indexed_actions) -> List[ActionError]: No validations necessary """ # TODO: Will it be necessary to check if this tag has children? - return True + return [] def execute(self): pass @@ -326,21 +330,20 @@ def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: """ No validations necessary """ - return True + return False def validate(self, indexed_actions) -> List[ActionError]: """ No validations necessary """ - return True + return [] def execute(self): pass -# Register actions here in the order in which you want to check -# 'WithoutChanges' action goes last always +# Register actions here in the order in which you want to check. available_actions = [ UpdateParentTag, RenameTag, diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py index a60b6d552..38aecf413 100644 --- a/openedx_tagging/core/tagging/import_export/api.py +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -10,12 +10,12 @@ def import_tags( taxonomy: Taxonomy, file: BytesIO, - format: ParserFormat, + parser_format: ParserFormat, replace=False, execute=False, ): # Get the parser and parse the file - parser = get_parser(format) + parser = get_parser(parser_format) tags, errors = parser.parse_import(file) # Check if there are errors in the parse @@ -24,11 +24,7 @@ def import_tags( # Generate the actions dsl = TagImportDSL() - errors = dsl.generate_actions(tags, taxonomy, replace) - - # Check if there are inconsistent actions or errors - if errors: - return errors + dsl.generate_actions(tags, taxonomy, replace) # Execute the plan if execute: diff --git a/openedx_tagging/core/tagging/import_export/dsl.py b/openedx_tagging/core/tagging/import_export/dsl.py index a4b5e93b0..00d590e2c 100644 --- a/openedx_tagging/core/tagging/import_export/dsl.py +++ b/openedx_tagging/core/tagging/import_export/dsl.py @@ -1,9 +1,18 @@ -from typing import List +""" +Model and functions to create a plan/execution with DSL actions. +""" +from typing import List, Optional from django.utils.translation import gettext_lazy as _ -from ..models import Taxonomy -from .actions import DeleteTag, ImportAction, UpdateParentTag, available_actions +from ..models import Taxonomy, Tag +from .actions import ( + DeleteTag, + ImportAction, + UpdateParentTag, + WithoutChanges, + available_actions, +) class TagDSL: @@ -12,15 +21,15 @@ class TagDSL: """ id: str value: str - parent_id: str - action: str - index: int + index: Optional[int] + parent_id: Optional[str] + action: Optional[str] def __init__( self, id: str, value: str, - index: str, + index: str=0, parent_id: str=None, action: str=None, ): @@ -32,6 +41,10 @@ def __init__( class TagImportDSL: + """ + Class with functions to build an import plan and excute the plan + """ + actions: List[ImportAction] indexed_actions: dict actions_dict: dict @@ -41,8 +54,14 @@ def __init__(self, taxonomy: Taxonomy): self.actions = [] self.errors = [] self.taxonomy = taxonomy - self.indexed_actions = {} self.actions_dict = {} + self._init_indexed_actions() + + def _init_indexed_actions(self): + """ + Initialize the `indexed_actions` dict + """ + self.indexed_actions = {} for action in available_actions: self.indexed_actions[action.name] = [] @@ -56,7 +75,7 @@ def _build_action(self, action_cls, tag: TagDSL): action = action_cls(self.taxonomy, tag, len(self.actions)) # We validate if there are no inconsistencies when executing this action - self.errors.append(action.validate(self.indexed_actions)) + self.errors.extend(action.validate(self.indexed_actions)) # Add action self.actions.append(action) @@ -64,12 +83,12 @@ def _build_action(self, action_cls, tag: TagDSL): # Index the actions for search self.indexed_actions[action.name].append(action) - def _delete_tags(self, tags: List[str]): + def _build_delete_actions(self, tags: dict): """ - Delete `tags` + Adds delete actions for `tags` """ - for tag in tags: - for child in tag.children: + for tag in tags.values(): + for child in tag.children.all(): if child.external_id not in tags: # If the child is not to be removed, # then update its parent @@ -87,6 +106,8 @@ def _delete_tags(self, tags: List[str]): DeleteTag, TagDSL( id=tag.external_id, + value=tag.value, + ) ) @@ -100,29 +121,41 @@ def generate_actions( Validates each action and create respective errors If `replace` is True, then deletes the tags that have not been read + + TODO: Missing join/reduce actions. Ex. A tag may have no changes, + but then its parent needs to be updated because its parent is deleted. + Those two actions should be merged. """ self.actions.clear() self.errors.clear() + self._init_indexed_actions() tags_for_delete = {} if replace: tags_for_delete = { tag.external_id: tag - for tag in self.taxonomy.tag_set + for tag in self.taxonomy.tag_set.all() } for tag in tags: + has_action = False + # Check all available actions and add which ones should be executed for action_cls in available_actions: if action_cls.valid_for(self.taxonomy, tag): self._build_action(action_cls, tag) + has_action = True + + if not has_action: + # If it doesn't find an action, a "without changes" is added + self._build_action(WithoutChanges, tag) if replace: tags_for_delete.pop(tag.id) if replace: # Delete all not readed tags - self._delete_tags(tags_for_delete) + self._build_delete_actions(tags_for_delete) def execute(self) -> List[ImportError]: pass diff --git a/openedx_tagging/core/tagging/import_export/exceptions.py b/openedx_tagging/core/tagging/import_export/exceptions.py index 599058aa8..237d4292b 100644 --- a/openedx_tagging/core/tagging/import_export/exceptions.py +++ b/openedx_tagging/core/tagging/import_export/exceptions.py @@ -7,6 +7,9 @@ def __init__(self, message:str='', **kargs): def __str__(self): return str(self.message) + def __repr__(self): + return f"{self.__class__.__name__}({str(self)})" + class ParserError(ImportError): def __init__(self, tag: str, **kargs): diff --git a/openedx_tagging/core/tagging/import_export/parsers.py b/openedx_tagging/core/tagging/import_export/parsers.py index c11b9b936..c3ccde649 100644 --- a/openedx_tagging/core/tagging/import_export/parsers.py +++ b/openedx_tagging/core/tagging/import_export/parsers.py @@ -213,18 +213,18 @@ def _veify_header(cls, header_fields: List[str]) -> List[ParserError]: ] -def get_parser(format: ParserFormat) -> Parser: +def get_parser(parser_format: ParserFormat) -> Parser: """ Get the parser for the respective `format` Raise `ValueError` if no parser found """ for parser in _parsers: - if format == parser.format: + if parser_format == parser.format: return parser raise ValueError( _( - f"Parser not found for format {format}" + f"Parser not found for format {parser_format}" ) ) diff --git a/tests/openedx_tagging/core/fixtures/tagging.yaml b/tests/openedx_tagging/core/fixtures/tagging.yaml index 5bb3936c9..98d27d138 100644 --- a/tests/openedx_tagging/core/fixtures/tagging.yaml +++ b/tests/openedx_tagging/core/fixtures/tagging.yaml @@ -173,6 +173,34 @@ parent: null value: System Tag 4 external_id: 'tag_4' +- model: oel_tagging.tag + pk: 26 + fields: + taxonomy: 3 + parent: null + value: Tag 1 + external_id: tag_1 +- model: oel_tagging.tag + pk: 27 + fields: + taxonomy: 5 + parent: 26 + value: Tag 2 + external_id: tag_2 +- model: oel_tagging.tag + pk: 28 + fields: + taxonomy: 5 + parent: null + value: Tag 3 + external_id: tag_3 +- model: oel_tagging.tag + pk: 29 + fields: + taxonomy: 5 + parent: 28 + value: Tag 4 + external_id: tag_4 - model: oel_tagging.taxonomy pk: 1 fields: @@ -202,3 +230,13 @@ allow_multiple: false allow_free_text: false _taxonomy_class: openedx_tagging.core.tagging.models.system_defined.SystemDefinedTaxonomy +- model: oel_tagging.taxonomy + pk: 5 + fields: + name: Import Taxonomy Test + description: null + enabled: true + required: false + allow_multiple: false + allow_free_text: false + diff --git a/tests/openedx_tagging/core/tagging/import_export/__init__.py b/tests/openedx_tagging/core/tagging/import_export/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/openedx_tagging/core/tagging/import_export/test_actions.py b/tests/openedx_tagging/core/tagging/import_export/test_actions.py new file mode 100644 index 000000000..5fe3d28eb --- /dev/null +++ b/tests/openedx_tagging/core/tagging/import_export/test_actions.py @@ -0,0 +1,389 @@ +""" +Tests for actions +""" +import ddt + +from django.test.testcases import TestCase + +from openedx_tagging.core.tagging.models import Taxonomy +from openedx_tagging.core.tagging.import_export.dsl import TagDSL +from openedx_tagging.core.tagging.import_export.actions import ( + ImportAction, + CreateTag, + UpdateParentTag, + RenameTag, + DeleteTag, + WithoutChanges, +) + + +class TestImportActionMixin: + """ + Mixin for import action tests + """ + + fixtures = ["tests/openedx_tagging/core/fixtures/tagging.yaml"] + + def setUp(self): + self.taxonomy = Taxonomy.objects.get(name="Import Taxonomy Test") + self.indexed_actions = { + 'create': [ + CreateTag( + taxonomy=self.taxonomy, + tag=TagDSL( + id='tag_10', + value='Tag 10', + index=0 + ), + index=0, + ) + ], + 'rename': [ + RenameTag( + taxonomy=self.taxonomy, + tag=TagDSL( + id='tag_11', + value='Tag 11', + index=1 + ), + index=1, + ) + ] + } + + +@ddt.ddt +class TestImportAction(TestImportActionMixin, TestCase): + """ + Test for general function of the ImportAction class + """ + + def test_not_implemented_functions(self): + with self.assertRaises(NotImplementedError): + ImportAction.valid_for(None, None) + action = ImportAction(None, None, None) + with self.assertRaises(NotImplementedError): + action.validate(None) + with self.assertRaises(NotImplementedError): + action.execute() + + @ddt.data( + ('create', 'id', 'tag_10', True), + ('rename', 'value', 'Tag 11', True), + ('rename', 'id', 'tag_10', False), + ('create', 'value', 'Tag 11', False), + ) + @ddt.unpack + def test_search_action(self, action_name, attr, search_value, expected): + import_action = ImportAction(self.taxonomy, None, None) + action = import_action._search_action( # pylint: disable=protected-access + self.indexed_actions, + action_name, + attr, + search_value, + ) + if expected: + self.assertEqual(getattr(action.tag, attr), search_value) + else: + self.assertIsNone(action) + + @ddt.data( + ('tag_1', True), + ('tag_10', True), + ('tag_100', False), + ) + @ddt.unpack + def test_validate_parent(self, parent_id, expected): + action = ImportAction( + self.taxonomy, + TagDSL( + id='tag_110', + value='_', + parent_id=parent_id, + index=100 + ), + index=100, + ) + error = action._validate_parent(self.indexed_actions) # pylint: disable=protected-access + if expected: + self.assertIsNone(error) + else: + self.assertEqual( + str(error), + ( + "Action error in 'import_action' (#100) in tag (tag_110): " + "Unknown parent tag (tag_100). " + "You need to add parent before the child in your file." + ) + ) + + @ddt.data( + ( + 'Tag 1', + ( + "Action error in 'import_action' (#100) in tag (tag_110): " + "Duplicated tag value with tag (pk=22)." + ) + ), + ( + 'Tag 10', + ( + "Conflict with 'import_action' (#100) in tag (tag_110) " + "and action #0: Duplicated tag value." + ) + ), + ( + 'Tag 11', + ( + "Conflict with 'import_action' (#100) in tag (tag_110) " + "and action #1: Duplicated tag value." + ) + ), + ('Tag 20', None) + ) + @ddt.unpack + def test_validate_value(self, value, expected): + action = ImportAction( + self.taxonomy, + TagDSL( + id='tag_110', + value=value, + index=100 + ), + index=100, + ) + error = action._validate_value(self.indexed_actions) # pylint: disable=protected-access + if not expected: + self.assertIsNone(error) + else: + self.assertEqual(str(error), expected) + + +@ddt.ddt +class TestCreateTag(TestImportActionMixin, TestCase): + """ + Test for 'create' action + """ + + @ddt.data( + ('tag_1', False), + ('tag_100', True), + ) + @ddt.unpack + def test_valid_for(self, tag_id, expected): + result = CreateTag.valid_for( + self.taxonomy, + TagDSL( + id=tag_id, + value='_', + index=100, + ) + ) + self.assertEqual(result, expected) + + @ddt.data( + ('tag_10', False), + ('tag_100', True), + ) + @ddt.unpack + def test_validate_id(self, tag_id, expected): + action = CreateTag( + taxonomy=self.taxonomy, + tag=TagDSL( + id=tag_id, + value='_', + index=100, + ), + index=100 + ) + error = action._validate_id(self.indexed_actions) # pylint: disable=protected-access + if expected: + self.assertIsNone(error) + else: + self.assertEqual( + str(error), + ( + f"Conflict with 'create' (#100) in tag ({tag_id}) " + "and action #0: Duplicated id tag." + ) + ) + + @ddt.data( + ('tag_10', "Tag 20", None, 1), # Invalid tag id + ('tag_20', "Tag 10", None, 1), # Invalid value, + ('tag_20', "Tag 20", 'tag_100', 1), # Invalid parent id, + ('tag_10', "Tag 10", None, 2), # Invalid tag id and value, + ('tag_10', "Tag 10", 'tag_100', 3), # Invalid tag id, value and parent, + ('tag_20', "Tag 20", 'tag_1', 0), # Valid + ) + @ddt.unpack + def test_validate(self, tag_id, tag_value, parent_id, expected): + action = CreateTag( + taxonomy=self.taxonomy, + tag=TagDSL( + id=tag_id, + value=tag_value, + index=100, + parent_id=parent_id + ), + index=100 + ) + errors = action.validate(self.indexed_actions) + self.assertEqual(len(errors), expected) + + +@ddt.ddt +class TestUpdateParentTag(TestImportActionMixin, TestCase): + """ + Test for 'update_parent' action + """ + + @ddt.data( + ('tag_100', None, False), # Tag doesn't exist on database + ('tag_2', 'tag_1', False), # Parent don't change + ('tag_2', 'tag_3', True), # Valid + ('tag_1', None, False), # Both parent id are None + ('tag_1', 'tag_3', True), # Valid + ) + @ddt.unpack + def test_valid_for(self, tag_id, parent_id, expected): + result = UpdateParentTag.valid_for( + taxonomy=self.taxonomy, + tag=TagDSL( + id=tag_id, + value='_', + parent_id=parent_id, + index=100 + ) + ) + self.assertEqual(result, expected) + + @ddt.data( + ('tag_2', 'tag_30', 1), # Invalid parent + ('tag_2', None, 0), # Without parent + ('tag_2', 'tag_10', 0), # Valid + ) + @ddt.unpack + def test_validate(self, tag_id, parent_id, expected): + action = UpdateParentTag( + taxonomy=self.taxonomy, + tag=TagDSL( + id=tag_id, + value='_', + parent_id=parent_id, + index=100, + ), + index=100 + ) + errors = action.validate(self.indexed_actions) + self.assertEqual(len(errors), expected) + +@ddt.ddt +class TestRenameTag(TestImportActionMixin, TestCase): + """ + Test for 'rename' action + """ + + @ddt.data( + ('tag_10', 'value', False), # Tag doesn't exist on database + ('tag_1', 'Tag 1', False), # Same value + ('tag_1', 'Tag 1 v2', True), # Valid + ) + @ddt.unpack + def test_valid_for(self, tag_id, value, expected): + result = RenameTag.valid_for( + taxonomy=self.taxonomy, + tag=TagDSL( + id=tag_id, + value=value, + index=100, + ) + ) + self.assertEqual(result, expected) + + @ddt.data( + ('Tag 2', 1), # There is a tag with the same value on database + ('Tag 10', 1), # There is a tag with the same value on create action + ('Tag 11', 1), # There is a tag with the same value on rename action + ('Tag 12', 0), # Valid + ) + @ddt.unpack + def test_validate(self, value, expected): + action = RenameTag( + taxonomy=self.taxonomy, + tag=TagDSL( + id='tag_1', + value=value, + index=100, + ), + index=100, + ) + errors = action.validate(self.indexed_actions) + self.assertEqual(len(errors), expected) + + +@ddt.ddt +class TestDeleteTag(TestImportActionMixin, TestCase): + """ + Test for 'delete' action + """ + + @ddt.data( + ('tag_10', None, False), # Tag doesn't exist on database + ('tag_1', 'rename', False), # Invalid action + ('tag_1', 'delete', True), # Valid + ) + @ddt.unpack + def test_valid_for(self, tag_id, action, expected): + result = DeleteTag.valid_for( + taxonomy=self.taxonomy, + tag=TagDSL( + id=tag_id, + value='_', + action=action, + index=100, + ), + ) + self.assertEqual(result, expected) + + def test_validate(self): + action = DeleteTag( + taxonomy=self.taxonomy, + tag=TagDSL( + id='_', + value='_', + index=100, + ), + index=100, + ) + result = action.validate(self.indexed_actions) + self.assertEqual(result, []) + + +class TestWithoutChanges(TestImportActionMixin, TestCase): + """ + Test for 'without_changes' action + """ + def test_valid_for(self): + result = WithoutChanges.valid_for( + self.taxonomy, + tag=TagDSL( + id='_', + value='_', + index=100, + ), + ) + self.assertFalse(result) + + def test_validate(self): + action = WithoutChanges( + taxonomy=self.taxonomy, + tag=TagDSL( + id='_', + value='_', + index=100, + ), + index=100, + ) + result = action.validate(self.indexed_actions) + self.assertEqual(result, []) diff --git a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py b/tests/openedx_tagging/core/tagging/import_export/test_dsl.py new file mode 100644 index 000000000..d43da4f0c --- /dev/null +++ b/tests/openedx_tagging/core/tagging/import_export/test_dsl.py @@ -0,0 +1,203 @@ +""" +Test for DSL functions +""" +import ddt + +from django.test.testcases import TestCase + +from openedx_tagging.core.tagging.import_export.dsl import TagDSL, TagImportDSL +from openedx_tagging.core.tagging.import_export.actions import CreateTag +from .test_actions import TestImportActionMixin + +@ddt.ddt +class TestTagImportDSL(TestImportActionMixin, TestCase): + """ + Test for DSL functions + """ + + def setUp(self): + super().setUp() + self.dsl = TagImportDSL(self.taxonomy) + + @ddt.data( + ('tag_10', 1), + ('tag_30', 0), + ) + @ddt.unpack + def test_build_action(self, tag_id, errors_expected): + self.dsl.indexed_actions = self.indexed_actions + self.dsl._build_action( # pylint: disable=protected-access + CreateTag, + TagDSL( + id=tag_id, + value='_', + index=100 + ) + ) + self.assertEqual(len(self.dsl.errors), errors_expected) + self.assertEqual(len(self.dsl.actions), 1) + self.assertEqual(self.dsl.actions[0].name, 'create') + self.assertEqual(self.dsl.indexed_actions['create'][1].tag.id, tag_id) + + def test_build_delete_actions(self): + tags = { + tag.external_id: tag + for tag in self.taxonomy.tag_set.exclude(pk=25) + } + # Clear other actions to only have the delete ones + self.dsl.actions.clear() + + self.dsl._build_delete_actions(tags) # pylint: disable=protected-access + self.assertEqual(len(self.dsl.errors), 0) + + # Check actions in order + # #1 Delete 'tag_1' + self.assertEqual(self.dsl.actions[0].name, 'delete') + self.assertEqual(self.dsl.actions[0].tag.id, 'tag_1') + # #2 Delete 'tag_2' + self.assertEqual(self.dsl.actions[1].name, 'delete') + self.assertEqual(self.dsl.actions[1].tag.id, 'tag_2') + # #3 Update parent of 'tag_4' + self.assertEqual(self.dsl.actions[2].name, 'update_parent') + self.assertEqual(self.dsl.actions[2].tag.id, 'tag_4') + self.assertIsNone(self.dsl.actions[2].tag.parent_id) + # #4 Delete 'tag_3' + self.assertEqual(self.dsl.actions[3].name, 'delete') + self.assertEqual(self.dsl.actions[3].tag.id, 'tag_3') + + @ddt.data( + ([ + { + 'id': 'tag_31', + 'value': 'Tag 31', + }, + { + 'id': 'tag_32', + 'value': 'Tag 32', + 'parent_id': 'tag_1', + }, + { + 'id': 'tag_2', + 'value': 'Tag 2 v2', + 'parent_id': 'tag_1' + }, + { + 'id': 'tag_4', + 'value': 'Tag 4 v2', + 'parent_id': 'tag_1', + }, + { + 'id': 'tag_1', + 'value': 'Tag 1', + }, + ], + False, + 0, + [ + { + 'name': 'create', + 'id': 'tag_31' + }, + { + 'name': 'create', + 'id': 'tag_32' + }, + { + 'name': 'rename', + 'id': 'tag_2' + }, + { + 'name': 'update_parent', + 'id': 'tag_4' + }, + { + 'name': 'rename', + 'id': 'tag_4' + }, + { + 'name': 'without_changes', + 'id': 'tag_1' + }, + ]), + ([ + { + 'id': 'tag_31', + 'value': 'Tag 31', + }, + { + 'id': 'tag_31', + 'value': 'Tag 32', + }, + { + 'id': 'tag_1', + 'value': 'Tag 2', + }, + { + 'id': 'tag_4', + 'value': 'Tag 4', + 'parent_id': 'tag_100', + }, + ], + False, + 3, + [ + { + 'name': 'create', + 'id': 'tag_31', + }, + { + 'name': 'create', + 'id': 'tag_31', + }, + { + 'name': 'rename', + 'id': 'tag_1', + }, + { + 'name': 'update_parent', + 'id': 'tag_4', + } + ]), + ([ + { + 'id': 'tag_4', + 'value': 'Tag 4', + 'parent_id': 'tag_3', + }, + ], + True, + 0, + [ + { + 'name': 'without_changes', + 'id': 'tag_4', + }, + { + 'name': 'delete', + 'id': 'tag_1' + }, + { + 'name': 'delete', + 'id': 'tag_2', + }, + { + 'name': 'update_parent', + 'id': 'tag_4', + }, + { + 'name': 'delete', + 'id': 'tag_3', + }, + ]) + ) + @ddt.unpack + def test_generate_actions(self, tags, replace, expected_errors, expected_actions): + tags = [TagDSL(**tag) for tag in tags] + self.dsl.generate_actions(tags=tags, replace=replace) + self.assertEqual(len(self.dsl.errors), expected_errors) + self.assertEqual(len(self.dsl.actions), len(expected_actions)) + + for index, action in enumerate(expected_actions): + self.assertEqual(self.dsl.actions[index].name, action['name']) + self.assertEqual(self.dsl.actions[index].tag.id, action['id']) + self.assertEqual(self.dsl.actions[index].index, index) diff --git a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py index 04147238b..e69fc1a7b 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py @@ -21,10 +21,10 @@ class TestParser(TestCase): """ def test_get_parser(self): - for format in ParserFormat: - parser = get_parser(format) - self.assertEqual(parser.format, format) - + for parser_format in ParserFormat: + parser = get_parser(parser_format) + self.assertEqual(parser.format, parser_format) + def test_parser_not_found(self): with self.assertRaises(ValueError): get_parser(None) @@ -108,22 +108,22 @@ def test_parse_tags(self): self.assertEqual(len(tags), 4) # Result tags must be in the same order of the file - for index in range(0, len(expected_tags)): + for index, expected_tag in enumerate(expected_tags): self.assertEqual( tags[index].id, - expected_tags[index].get('id') + expected_tag.get('id') ) self.assertEqual( tags[index].value, - expected_tags[index].get('value') + expected_tag.get('value') ) self.assertEqual( tags[index].parent_id, - expected_tags[index].get('parent_id') + expected_tag.get('parent_id') ) self.assertEqual( tags[index].action, - expected_tags[index].get('action') + expected_tag.get('action') ) self.assertEqual( tags[index].index, @@ -135,7 +135,7 @@ class TestCSVParser(TestCase): """ Test for .csv parser """ - + @ddt.data( ( "value\n", @@ -193,6 +193,9 @@ def test_parse_tags_errors(self, csv_data, expected_errors): self.assertIn(str(error), expected_errors) def _build_csv(self, tags): + """ + Builds a csv from 'tags' dict + """ csv = "id,value,parent_id,action\n" for tag in tags: csv += ( @@ -211,27 +214,27 @@ def test_parse_tags(self): csv_data = self._build_csv(expected_tags) csv_file = BytesIO(csv_data.encode()) tags, errors = CSVParser.parse_import(csv_file) - + self.assertEqual(len(errors), 0) self.assertEqual(len(tags), 4) # Result tags must be in the same order of the file - for index in range(0, len(expected_tags)): + for index, expected_tag in enumerate(expected_tags): self.assertEqual( tags[index].id, - expected_tags[index].get('id') + expected_tag.get('id') ) self.assertEqual( tags[index].value, - expected_tags[index].get('value') + expected_tag.get('value') ) self.assertEqual( tags[index].parent_id, - expected_tags[index].get('parent_id') + expected_tag.get('parent_id') ) self.assertEqual( tags[index].action, - expected_tags[index].get('action') + expected_tag.get('action') ) self.assertEqual( tags[index].index, diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index a75aebb61..a361f8507 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -3,7 +3,7 @@ from django.test.testcases import TestCase, override_settings import openedx_tagging.core.tagging.api as tagging_api -from openedx_tagging.core.tagging.models import ObjectTag, Tag +from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy from .test_models import TestTagTaxonomyMixin, get_tag @@ -53,20 +53,22 @@ def test_get_taxonomy(self): def test_get_taxonomies(self): tax1 = tagging_api.create_taxonomy("Enabled") tax2 = tagging_api.create_taxonomy("Disabled", enabled=False) + tax3 = Taxonomy.objects.get(name="Import Taxonomy Test") with self.assertNumQueries(1): enabled = list(tagging_api.get_taxonomies()) - assert enabled == [ tax1, + tax3, self.language_taxonomy, self.taxonomy, self.system_taxonomy, self.user_taxonomy, ] assert str(enabled[0]) == f" ({tax1.id}) Enabled" - assert str(enabled[1]) == " (-1) Languages" - assert str(enabled[2]) == " (1) Life on Earth" - assert str(enabled[3]) == " (4) System defined taxonomy" + assert str(enabled[1]) == " (5) Import Taxonomy Test" + assert str(enabled[2]) == " (-1) Languages" + assert str(enabled[3]) == " (1) Life on Earth" + assert str(enabled[4]) == " (4) System defined taxonomy" with self.assertNumQueries(1): disabled = list(tagging_api.get_taxonomies(enabled=False)) @@ -75,13 +77,15 @@ def test_get_taxonomies(self): with self.assertNumQueries(1): both = list(tagging_api.get_taxonomies(enabled=None)) + print(both) assert both == [ tax2, tax1, + tax3, self.language_taxonomy, self.taxonomy, self.system_taxonomy, - self.user_taxonomy, + self.user_taxonomy ] @override_settings(LANGUAGES=test_languages) From 2db85637419c87d2d0d0bbd1a282055565015901 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Tue, 18 Jul 2023 16:07:05 -0500 Subject: [PATCH 04/20] feat: plan() function --- .../core/tagging/import_export/actions.py | 46 +++++++- .../core/tagging/import_export/dsl.py | 24 +++- .../core/tagging/import_export/exceptions.py | 6 +- .../tagging/import_export/test_actions.py | 14 +-- .../core/tagging/import_export/test_dsl.py | 107 ++++++++++++++++++ 5 files changed, 181 insertions(+), 16 deletions(-) diff --git a/openedx_tagging/core/tagging/import_export/actions.py b/openedx_tagging/core/tagging/import_export/actions.py index df02df9f8..e907e8879 100644 --- a/openedx_tagging/core/tagging/import_export/actions.py +++ b/openedx_tagging/core/tagging/import_export/actions.py @@ -22,7 +22,10 @@ def __init__(self, taxonomy: Taxonomy, tag, index: int): self.index = index def __repr__(self): - return f"Action {self.name} (index={self.index},id={self.tag.id})" + return _(f"Action {self.name} (index={self.index},id={self.tag.id})") + + def __str__(self): + return str(self.__repr__) @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: @@ -89,7 +92,7 @@ def _validate_value(self, indexed_actions): return ActionError( action=self, tag_id=self.tag.id, - message=_(f"Duplicated tag value with tag (pk={tag.id}).") + message=_(f"Duplicated tag value with tag (id={tag.id}).") ) except Tag.DoesNotExist: # Validates value duplication on create actions @@ -133,6 +136,13 @@ class CreateTag(ImportAction): name = 'create' + def __str__(self): + return str(_( + "Create a new tag with values " + f"(external_id={self.tag.id}, value={self.tag.value}, " + f"parent_id={self.tag.parent_id})." + )) + @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: """ @@ -159,7 +169,7 @@ def _validate_id(self, indexed_actions): action=self, tag_id=self.tag.id, conflict_action_index=action.index, - message=_("Duplicated id tag.") + message=_("Duplicated external_id tag.") ) def validate(self, indexed_actions) -> List[ActionError]: @@ -203,6 +213,18 @@ class UpdateParentTag(ImportAction): name = 'update_parent' + def __str__(self): + taxonomy_tag = self.taxonomy.tag_set.get(external_id=self.tag.id) + if not taxonomy_tag.parent: + from_str = _("from empty parent") + else: + from_str = _(f"from parent (external_id={taxonomy_tag.parent.external_id})") + + return str(_( + f"Update the parent of tag (id={taxonomy_tag.id}) " + f"{from_str} to parent (external_id={self.tag.parent_id})." + )) + @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: """ @@ -254,6 +276,13 @@ class RenameTag(ImportAction): name = 'rename' + def __str__(self): + taxonomy_tag = self.taxonomy.tag_set.get(external_id=self.tag.id) + return str(_( + f"Rename tag value of tag (id={taxonomy_tag.id}) " + f"from '{taxonomy_tag.value}' to '{self.tag.value}'" + )) + @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: """ @@ -293,6 +322,12 @@ class DeleteTag(ImportAction): Does not require validations """ + def __str__(self): + taxonomy_tag = self.taxonomy.tag_set.get(external_id=self.tag.id) + return str(_( + f"Delete tag (id={taxonomy_tag.id})" + )) + name = 'delete' @classmethod @@ -325,6 +360,11 @@ class WithoutChanges(ImportAction): name = 'without_changes' + def __str__(self): + return str(_( + f"No changes needed for tag (external_id={self.tag.id})" + )) + @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: """ diff --git a/openedx_tagging/core/tagging/import_export/dsl.py b/openedx_tagging/core/tagging/import_export/dsl.py index 00d590e2c..113630636 100644 --- a/openedx_tagging/core/tagging/import_export/dsl.py +++ b/openedx_tagging/core/tagging/import_export/dsl.py @@ -5,7 +5,7 @@ from django.utils.translation import gettext_lazy as _ -from ..models import Taxonomy, Tag +from ..models import Taxonomy from .actions import ( DeleteTag, ImportAction, @@ -160,5 +160,23 @@ def generate_actions( def execute(self) -> List[ImportError]: pass - def plan(self) -> List[str]: - pass + def plan(self) -> str: + """ + Returns an string with the plan and errors + """ + result = ( + "Plan\n" + "--------------------------------\n" + ) + for action in self.actions: + result += f"#{action.index}: {str(action)}\n" + + if self.errors: + result += ( + "Output errors\n" + "--------------------------------\n" + ) + for error in self.errors: + result += f"{str(error)}\n" + + return result diff --git a/openedx_tagging/core/tagging/import_export/exceptions.py b/openedx_tagging/core/tagging/import_export/exceptions.py index 237d4292b..6e9c97342 100644 --- a/openedx_tagging/core/tagging/import_export/exceptions.py +++ b/openedx_tagging/core/tagging/import_export/exceptions.py @@ -19,7 +19,7 @@ def __init__(self, tag: str, **kargs): class ActionError(ImportError): def __init__(self, action: str, tag_id: str, message: str, **kargs): self.message = _( - f"Action error in '{action.name}' (#{action.index}) in tag ({tag_id}): {message}" + f"Action error in '{action.name}' (#{action.index}): {message}" ) @@ -33,8 +33,8 @@ def __init__( **kargs ): self.message = _( - f"Conflict with '{action.name}' (#{action.index}) in tag ({tag_id})" - f" and action #{conflict_action_index}: {message}" + f"Conflict with '{action.name}' (#{action.index}) " + f"and action #{conflict_action_index}: {message}" ) diff --git a/tests/openedx_tagging/core/tagging/import_export/test_actions.py b/tests/openedx_tagging/core/tagging/import_export/test_actions.py index 5fe3d28eb..3d5c0ea86 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_actions.py @@ -111,7 +111,7 @@ def test_validate_parent(self, parent_id, expected): self.assertEqual( str(error), ( - "Action error in 'import_action' (#100) in tag (tag_110): " + "Action error in 'import_action' (#100): " "Unknown parent tag (tag_100). " "You need to add parent before the child in your file." ) @@ -121,21 +121,21 @@ def test_validate_parent(self, parent_id, expected): ( 'Tag 1', ( - "Action error in 'import_action' (#100) in tag (tag_110): " - "Duplicated tag value with tag (pk=22)." + "Action error in 'import_action' (#100): " + "Duplicated tag value with tag (id=22)." ) ), ( 'Tag 10', ( - "Conflict with 'import_action' (#100) in tag (tag_110) " + "Conflict with 'import_action' (#100) " "and action #0: Duplicated tag value." ) ), ( 'Tag 11', ( - "Conflict with 'import_action' (#100) in tag (tag_110) " + "Conflict with 'import_action' (#100) " "and action #1: Duplicated tag value." ) ), @@ -203,8 +203,8 @@ def test_validate_id(self, tag_id, expected): self.assertEqual( str(error), ( - f"Conflict with 'create' (#100) in tag ({tag_id}) " - "and action #0: Duplicated id tag." + "Conflict with 'create' (#100) " + "and action #0: Duplicated external_id tag." ) ) diff --git a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py b/tests/openedx_tagging/core/tagging/import_export/test_dsl.py index d43da4f0c..57a406f3b 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_dsl.py @@ -201,3 +201,110 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions self.assertEqual(self.dsl.actions[index].name, action['name']) self.assertEqual(self.dsl.actions[index].tag.id, action['id']) self.assertEqual(self.dsl.actions[index].index, index) + + @ddt.data( + ([ + { + 'id': 'tag_31', + 'value': 'Tag 31', + }, + { + 'id': 'tag_31', + 'value': 'Tag 32', + }, + { + 'id': 'tag_1', + 'value': 'Tag 2', + }, + { + 'id': 'tag_4', + 'value': 'Tag 4', + 'parent_id': 'tag_100', + }, + { + 'id': 'tag_33', + 'value': 'Tag 32', + }, + { + 'id': 'tag_2', + 'value': 'Tag 31', + }, + ], + False, + "Plan\n" + "--------------------------------\n" + "#0: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" + "#1: Create a new tag with values (external_id=tag_31, value=Tag 32, parent_id=None).\n" + "#2: Rename tag value of tag (id=22) from 'Tag 1' to 'Tag 2'\n" + "#3: Update the parent of tag (id=25) from parent (external_id=tag_3) to parent (external_id=tag_100).\n" + "#4: Create a new tag with values (external_id=tag_33, value=Tag 32, parent_id=None).\n" + "#5: Update the parent of tag (id=23) from parent (external_id=tag_1) to parent (external_id=None).\n" + "#6: Rename tag value of tag (id=23) from 'Tag 2' to 'Tag 31'\n" + "Output errors\n" + "--------------------------------\n" + "Conflict with 'create' (#1) and action #0: Duplicated external_id tag.\n" + "Action error in 'rename' (#2): Duplicated tag value with tag (id=23).\n" + "Action error in 'update_parent' (#3): Unknown parent tag (tag_100). " + "You need to add parent before the child in your file.\n" + "Conflict with 'create' (#4) and action #1: Duplicated tag value.\n" + "Conflict with 'rename' (#6) and action #0: Duplicated tag value.\n" + ), + ([ + { + 'id': 'tag_31', + 'value': 'Tag 31', + }, + { + 'id': 'tag_32', + 'value': 'Tag 32', + 'parent_id': 'tag_1', + }, + { + 'id': 'tag_2', + 'value': 'Tag 2 v2', + 'parent_id': 'tag_1' + }, + { + 'id': 'tag_4', + 'value': 'Tag 4 v2', + 'parent_id': 'tag_1', + }, + { + 'id': 'tag_1', + 'value': 'Tag 1', + }, + ], + False, + "Plan\n" + "--------------------------------\n" + "#0: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" + "#1: Create a new tag with values (external_id=tag_32, value=Tag 32, parent_id=tag_1).\n" + "#2: Rename tag value of tag (id=23) from 'Tag 2' to 'Tag 2 v2'\n" + "#3: Update the parent of tag (id=25) from parent (external_id=tag_3) to parent (external_id=tag_1).\n" + "#4: Rename tag value of tag (id=25) from 'Tag 4' to 'Tag 4 v2'\n" + "#5: No changes needed for tag (external_id=tag_1)\n" + ), + ([ + { + 'id': 'tag_4', + 'value': 'Tag 4', + 'parent_id': 'tag_3', + }, + ], + True, + "Plan\n" + "--------------------------------\n" + "#0: No changes needed for tag (external_id=tag_4)\n" + "#1: Delete tag (id=22)\n" + "#2: Delete tag (id=23)\n" + "#3: Update the parent of tag (id=25) from parent (external_id=tag_3) to parent (external_id=None).\n" + "#4: Delete tag (id=24)\n" + ) + ) + @ddt.unpack + def test_plan(self, tags, replace, expected): + tags = [TagDSL(**tag) for tag in tags] + self.dsl.generate_actions(tags=tags, replace=replace) + plan = self.dsl.plan() + print(plan) + self.assertEqual(plan, expected) From 1da03091597ab2012687e90c0bcf6e6e76f4ec34 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 26 Jul 2023 14:17:17 -0500 Subject: [PATCH 05/20] style: Rename exceptions and running black --- .../core/tagging/import_export/actions.py | 142 +++++++++--------- .../core/tagging/import_export/api.py | 9 +- .../core/tagging/import_export/dsl.py | 38 ++--- .../core/tagging/import_export/exceptions.py | 25 +-- .../core/tagging/import_export/parsers.py | 102 +++++++------ .../core/tagging/import_export/utils.py | 0 6 files changed, 154 insertions(+), 162 deletions(-) delete mode 100644 openedx_tagging/core/tagging/import_export/utils.py diff --git a/openedx_tagging/core/tagging/import_export/actions.py b/openedx_tagging/core/tagging/import_export/actions.py index e907e8879..261f8ed75 100644 --- a/openedx_tagging/core/tagging/import_export/actions.py +++ b/openedx_tagging/core/tagging/import_export/actions.py @@ -6,15 +6,15 @@ from django.utils.translation import gettext_lazy as _ from ..models import Taxonomy, Tag -from .exceptions import ActionError, ActionConflict +from .exceptions import ImportActionError, ImportActionConflict class ImportAction: """ - Base class to create actions + Base class to create actions """ - name = 'import_action' + name = "import_action" def __init__(self, taxonomy: Taxonomy, tag, index: int): self.taxonomy = taxonomy @@ -23,7 +23,7 @@ def __init__(self, taxonomy: Taxonomy, tag, index: int): def __repr__(self): return _(f"Action {self.name} (index={self.index},id={self.tag.id})") - + def __str__(self): return str(self.__repr__) @@ -36,7 +36,7 @@ def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: """ raise NotImplementedError - def validate(self, indexed_actions) -> List[ActionError]: + def validate(self, indexed_actions) -> List[ImportActionError]: """ Implement this to find inconsistencies with tags in the database or with previous actions. @@ -59,10 +59,10 @@ def _search_action( for action in indexed_actions[action_name]: if search_value == getattr(action.tag, attr): return action - + return None - def _validate_parent(self, indexed_actions) -> ActionError: + def _validate_parent(self, indexed_actions) -> ImportActionError: """ Validates if the parent is created """ @@ -71,14 +71,16 @@ def _validate_parent(self, indexed_actions) -> ActionError: self.taxonomy.tag_set.get(external_id=self.tag.parent_id) except Tag.DoesNotExist: # Or if the parent is created on previous actions - if not self._search_action(indexed_actions, CreateTag.name, 'id', self.tag.parent_id): - return ActionError( + if not self._search_action( + indexed_actions, CreateTag.name, "id", self.tag.parent_id + ): + return ImportActionError( action=self, tag_id=self.tag.id, message=_( f"Unknown parent tag ({self.tag.parent_id}). " "You need to add parent before the child in your file." - ) + ), ) def _validate_value(self, indexed_actions): @@ -89,37 +91,38 @@ def _validate_value(self, indexed_actions): try: # Validates if exists a tag with the same value on the Taxonomy tag = self.taxonomy.tag_set.get(value=self.tag.value) - return ActionError( + return ImportActionError( action=self, tag_id=self.tag.id, - message=_(f"Duplicated tag value with tag (id={tag.id}).") + message=_(f"Duplicated tag value with tag (id={tag.id})."), ) except Tag.DoesNotExist: # Validates value duplication on create actions action = self._search_action( indexed_actions, CreateTag.name, - 'value', + "value", self.tag.value, ) - + if not action: # Validates value duplication on rename actions action = self._search_action( indexed_actions, RenameTag.name, - 'value', + "value", self.tag.value, ) - + if action: - return ActionConflict( + return ImportActionConflict( action=self, tag_id=self.tag.id, conflict_action_index=action.index, - message=_("Duplicated tag value.") + message=_("Duplicated tag value."), ) + class CreateTag(ImportAction): """ Action for create a Tag @@ -130,18 +133,20 @@ class CreateTag(ImportAction): - Id duplicates with previous create actions. - Value duplicates with tags on the database. - Value duplicates with previous create and rename actions. - - Parent validation. If the parent is in the database or created + - Parent validation. If the parent is in the database or created in previous actions. """ - name = 'create' + name = "create" def __str__(self): - return str(_( - "Create a new tag with values " - f"(external_id={self.tag.id}, value={self.tag.value}, " - f"parent_id={self.tag.parent_id})." - )) + return str( + _( + "Create a new tag with values " + f"(external_id={self.tag.id}, value={self.tag.value}, " + f"parent_id={self.tag.parent_id})." + ) + ) @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: @@ -158,21 +163,16 @@ def _validate_id(self, indexed_actions): """ Check for id duplicates in previous create actions """ - action = self._search_action( - indexed_actions, - self.name, - 'id', - self.tag.id - ) + action = self._search_action(indexed_actions, self.name, "id", self.tag.id) if action: - return ActionConflict( + return ImportActionConflict( action=self, tag_id=self.tag.id, conflict_action_index=action.index, - message=_("Duplicated external_id tag.") + message=_("Duplicated external_id tag."), ) - def validate(self, indexed_actions) -> List[ActionError]: + def validate(self, indexed_actions) -> List[ImportActionError]: """ Validates the creation action """ @@ -182,7 +182,7 @@ def validate(self, indexed_actions) -> List[ActionError]: error = self._validate_id(indexed_actions) if error: errors.append(error) - + # Duplicate value validation error = self._validate_value(indexed_actions) if error: @@ -193,7 +193,7 @@ def validate(self, indexed_actions) -> List[ActionError]: error = self._validate_parent(indexed_actions) if error: errors.append(error) - + return errors def execute(self): @@ -207,23 +207,25 @@ class UpdateParentTag(ImportAction): Action created if there is a change on the parent Validations: - - Parent validation. If the parent is in the database + - Parent validation. If the parent is in the database or created in previous actions. """ - name = 'update_parent' + name = "update_parent" def __str__(self): - taxonomy_tag = self.taxonomy.tag_set.get(external_id=self.tag.id) + taxonomy_tag = self.taxonomy.tag_set.get(external_id=self.tag.id) if not taxonomy_tag.parent: from_str = _("from empty parent") else: from_str = _(f"from parent (external_id={taxonomy_tag.parent.external_id})") - return str(_( - f"Update the parent of tag (id={taxonomy_tag.id}) " - f"{from_str} to parent (external_id={self.tag.parent_id})." - )) + return str( + _( + f"Update the parent of tag (id={taxonomy_tag.id}) " + f"{from_str} to parent (external_id={self.tag.parent_id})." + ) + ) @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: @@ -233,19 +235,13 @@ def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: try: taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) return ( - ( - taxonomy_tag.parent is not None - and taxonomy_tag.parent.external_id != tag.parent_id - ) - or - ( - taxonomy_tag.parent is None and tag.parent_id is not None - ) - ) + taxonomy_tag.parent is not None + and taxonomy_tag.parent.external_id != tag.parent_id + ) or (taxonomy_tag.parent is None and tag.parent_id is not None) except Tag.DoesNotExist: return False - def validate(self, indexed_actions) -> List[ActionError]: + def validate(self, indexed_actions) -> List[ImportActionError]: """ Validates the update parent action """ @@ -274,14 +270,16 @@ class RenameTag(ImportAction): - Value duplicates with previous create and rename actions. """ - name = 'rename' + name = "rename" def __str__(self): - taxonomy_tag = self.taxonomy.tag_set.get(external_id=self.tag.id) - return str(_( - f"Rename tag value of tag (id={taxonomy_tag.id}) " - f"from '{taxonomy_tag.value}' to '{self.tag.value}'" - )) + taxonomy_tag = self.taxonomy.tag_set.get(external_id=self.tag.id) + return str( + _( + f"Rename tag value of tag (id={taxonomy_tag.id}) " + f"from '{taxonomy_tag.value}' to '{self.tag.value}'" + ) + ) @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: @@ -290,13 +288,11 @@ def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: """ try: taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) - return ( - taxonomy_tag.value != tag.value - ) + return taxonomy_tag.value != tag.value except Tag.DoesNotExist: return False - def validate(self, indexed_actions) -> List[ActionError]: + def validate(self, indexed_actions) -> List[ImportActionError]: """ Validates the rename action """ @@ -323,12 +319,10 @@ class DeleteTag(ImportAction): """ def __str__(self): - taxonomy_tag = self.taxonomy.tag_set.get(external_id=self.tag.id) - return str(_( - f"Delete tag (id={taxonomy_tag.id})" - )) + taxonomy_tag = self.taxonomy.tag_set.get(external_id=self.tag.id) + return str(_(f"Delete tag (id={taxonomy_tag.id})")) - name = 'delete' + name = "delete" @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: @@ -341,7 +335,7 @@ def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: except Tag.DoesNotExist: return False - def validate(self, indexed_actions) -> List[ActionError]: + def validate(self, indexed_actions) -> List[ImportActionError]: """ No validations necessary """ @@ -351,6 +345,7 @@ def validate(self, indexed_actions) -> List[ActionError]: def execute(self): pass + class WithoutChanges(ImportAction): """ Action when there is no change on the Tag @@ -358,12 +353,10 @@ class WithoutChanges(ImportAction): Does not require validations """ - name = 'without_changes' + name = "without_changes" def __str__(self): - return str(_( - f"No changes needed for tag (external_id={self.tag.id})" - )) + return str(_(f"No changes needed for tag (external_id={self.tag.id})")) @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: @@ -372,8 +365,7 @@ def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: """ return False - - def validate(self, indexed_actions) -> List[ActionError]: + def validate(self, indexed_actions) -> List[ImportActionError]: """ No validations necessary """ diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py index 38aecf413..2d534bbf0 100644 --- a/openedx_tagging/core/tagging/import_export/api.py +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -1,11 +1,10 @@ from io import BytesIO -from typing import List from ..models import Taxonomy from .parsers import get_parser, ParserFormat -from .exceptions import ImportError from .dsl import TagImportDSL + # TODO This function must be a celery task def import_tags( taxonomy: Taxonomy, @@ -21,14 +20,14 @@ def import_tags( # Check if there are errors in the parse if errors: return errors - + # Generate the actions dsl = TagImportDSL() dsl.generate_actions(tags, taxonomy, replace) - + # Execute the plan if execute: return dsl.execute() - + # Or return the plan return dsl.plan() diff --git a/openedx_tagging/core/tagging/import_export/dsl.py b/openedx_tagging/core/tagging/import_export/dsl.py index 113630636..0a237d088 100644 --- a/openedx_tagging/core/tagging/import_export/dsl.py +++ b/openedx_tagging/core/tagging/import_export/dsl.py @@ -6,6 +6,7 @@ from django.utils.translation import gettext_lazy as _ from ..models import Taxonomy +from .exceptions import TagImportError from .actions import ( DeleteTag, ImportAction, @@ -19,6 +20,7 @@ class TagDSL: """ Tag representation on the import DSL """ + id: str value: str index: Optional[int] @@ -29,9 +31,9 @@ def __init__( self, id: str, value: str, - index: str=0, - parent_id: str=None, - action: str=None, + index: str = 0, + parent_id: str = None, + action: str = None, ): self.id = id self.value = value @@ -39,7 +41,7 @@ def __init__( self.parent_id = parent_id self.action = action - + class TagImportDSL: """ Class with functions to build an import plan and excute the plan @@ -90,7 +92,7 @@ def _build_delete_actions(self, tags: dict): for tag in tags.values(): for child in tag.children.all(): if child.external_id not in tags: - # If the child is not to be removed, + # If the child is not to be removed, # then update its parent self._build_action( UpdateParentTag, @@ -98,7 +100,7 @@ def _build_delete_actions(self, tags: dict): id=child.external_id, value=child.value, parent_id=None, - ) + ), ) # Delete action @@ -107,8 +109,7 @@ def _build_delete_actions(self, tags: dict): TagDSL( id=tag.external_id, value=tag.value, - - ) + ), ) def generate_actions( @@ -118,11 +119,11 @@ def generate_actions( ): """ Generates actions from `tags`. - + Validates each action and create respective errors If `replace` is True, then deletes the tags that have not been read - TODO: Missing join/reduce actions. Ex. A tag may have no changes, + TODO: Missing join/reduce actions. Ex. A tag may have no changes, but then its parent needs to be updated because its parent is deleted. Those two actions should be merged. """ @@ -133,8 +134,7 @@ def generate_actions( if replace: tags_for_delete = { - tag.external_id: tag - for tag in self.taxonomy.tag_set.all() + tag.external_id: tag for tag in self.taxonomy.tag_set.all() } for tag in tags: @@ -145,7 +145,7 @@ def generate_actions( if action_cls.valid_for(self.taxonomy, tag): self._build_action(action_cls, tag) has_action = True - + if not has_action: # If it doesn't find an action, a "without changes" is added self._build_action(WithoutChanges, tag) @@ -157,25 +157,19 @@ def generate_actions( # Delete all not readed tags self._build_delete_actions(tags_for_delete) - def execute(self) -> List[ImportError]: + def execute(self) -> List[TagImportError]: pass def plan(self) -> str: """ Returns an string with the plan and errors """ - result = ( - "Plan\n" - "--------------------------------\n" - ) + result = "Plan\n" "--------------------------------\n" for action in self.actions: result += f"#{action.index}: {str(action)}\n" if self.errors: - result += ( - "Output errors\n" - "--------------------------------\n" - ) + result += "Output errors\n" "--------------------------------\n" for error in self.errors: result += f"{str(error)}\n" diff --git a/openedx_tagging/core/tagging/import_export/exceptions.py b/openedx_tagging/core/tagging/import_export/exceptions.py index 6e9c97342..83bfaed43 100644 --- a/openedx_tagging/core/tagging/import_export/exceptions.py +++ b/openedx_tagging/core/tagging/import_export/exceptions.py @@ -1,36 +1,37 @@ from django.utils.translation import gettext_lazy as _ -class ImportError(Exception): - def __init__(self, message:str='', **kargs): + +class TagImportError(Exception): + def __init__(self, message: str = "", **kargs): self.message = message def __str__(self): return str(self.message) - + def __repr__(self): return f"{self.__class__.__name__}({str(self)})" - -class ParserError(ImportError): + +class TagParserError(TagImportError): def __init__(self, tag: str, **kargs): self.message = _(f"Import error on {tag}") -class ActionError(ImportError): +class ImportActionError(TagImportError): def __init__(self, action: str, tag_id: str, message: str, **kargs): self.message = _( f"Action error in '{action.name}' (#{action.index}): {message}" ) -class ActionConflict(ActionError): +class ImportActionConflict(ImportActionError): def __init__( self, action: str, tag_id: str, conflict_action_index: int, message: str, - **kargs + **kargs, ): self.message = _( f"Conflict with '{action.name}' (#{action.index}) " @@ -38,25 +39,25 @@ def __init__( ) -class InvalidFormat(ParserError): +class InvalidFormat(TagParserError): def __init__(self, tag: dict, format: str, message: str, **kargs): self.tag = tag self.message = _(f"Invalid '{format}' format: {message}") -class FieldJSONError(ParserError): +class FieldJSONError(TagParserError): def __init__(self, tag, field, **kargs): self.tag = tag self.message = _(f"Missing '{field}' field on {tag}") -class EmptyJSONField(ParserError): +class EmptyJSONField(TagParserError): def __init__(self, tag, field, **kargs): self.tag = tag self.message = _(f"Empty '{field}' field on {tag}") -class EmptyCSVField(ParserError): +class EmptyCSVField(TagParserError): def __init__(self, tag, field, row, **kargs): self.tag = tag self.message = _(f"Empty '{field}' field on the row {row}") diff --git a/openedx_tagging/core/tagging/import_export/parsers.py b/openedx_tagging/core/tagging/import_export/parsers.py index c3ccde649..e36d1820c 100644 --- a/openedx_tagging/core/tagging/import_export/parsers.py +++ b/openedx_tagging/core/tagging/import_export/parsers.py @@ -11,17 +11,19 @@ from .dsl import TagDSL from .exceptions import ( - ParserError, + TagParserError, InvalidFormat, FieldJSONError, EmptyJSONField, EmptyCSVField, ) + class ParserFormat(Enum): """ Format of import tags to taxonomies """ + JSON = ".json" CSV = ".csv" @@ -33,20 +35,21 @@ class Parser: This contains the base functions to load data, validate required fields and convert tags to DLS format """ - required_fields = ['id', 'value'] - optional_fields = ['parent_id', 'action'] + + required_fields = ["id", "value"] + optional_fields = ["parent_id", "action"] # Set the format associated to the parser format = None # We can change the error when is missing a required field - missing_field_error = ParserError + missing_field_error = TagParserError # We can change the error when a required field is empty - empty_field_error = ParserError + empty_field_error = TagParserError # We can change the initial row/index inital_row = 1 @classmethod - def parse_import(cls, file: BytesIO) -> Tuple[List[TagDSL], List[ParserError]]: + def parse_import(cls, file: BytesIO) -> Tuple[List[TagDSL], List[TagParserError]]: """ Top function that calls `_load_data` and `_parse_tags`. Handle the errors returned both functions @@ -59,8 +62,8 @@ def parse_import(cls, file: BytesIO) -> Tuple[List[TagDSL], List[ParserError]]: file.close() return cls._parse_tags(tags_data) - - def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[ParserError]]: + + def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[TagParserError]]: """ Each parser implements this function according to its format. This function reads the file and returns a list with the values of each tag. @@ -71,7 +74,7 @@ def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[ParserError]]: raise NotImplementedError @classmethod - def _parse_tags(cls, tags_data: dict) -> Tuple[List[TagDSL], List[ParserError]]: + def _parse_tags(cls, tags_data: dict) -> Tuple[List[TagDSL], List[TagParserError]]: """ Validate the required fields of each tag. @@ -88,21 +91,25 @@ def _parse_tags(cls, tags_data: dict) -> Tuple[List[TagDSL], List[ParserError]]: for req_field in cls.required_fields: if req_field not in tag: # Verify if the field exists - errors.append(cls.missing_field_error( - tag, - field=req_field, - row=row, - )) + errors.append( + cls.missing_field_error( + tag, + field=req_field, + row=row, + ) + ) has_error = True elif not tag.get(req_field): # Verify if the value of the field is not empty - errors.append(cls.empty_field_error( - tag, - field=req_field, - row=row, - )) + errors.append( + cls.empty_field_error( + tag, + field=req_field, + row=row, + ) + ) has_error = True - + tag["index"] = row row += 1 @@ -116,7 +123,7 @@ def _parse_tags(cls, tags_data: dict) -> Tuple[List[TagDSL], List[ParserError]]: tag[opt_field] = None tags.append(TagDSL(**tag)) - + return tags, errors @@ -137,26 +144,29 @@ class JSONParser(Parser): } ``` """ + format = ParserFormat.JSON missing_field_error = FieldJSONError empty_field_error = EmptyJSONField inital_row = 0 @classmethod - def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[ParserError]]: + def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[TagParserError]]: """ Read a .json file and validates the root structure of the json """ file.seek(0) tags_data = json.load(file) - if 'tags' not in tags_data: - return None, [InvalidFormat( - tag=None, - format=cls.format.value, - message=_("Missing 'tags' field on the .json file") - )] - - tags_data = tags_data.get('tags') + if "tags" not in tags_data: + return None, [ + InvalidFormat( + tag=None, + format=cls.format.value, + message=_("Missing 'tags' field on the .json file"), + ) + ] + + tags_data = tags_data.get("tags") return tags_data, [] @@ -171,17 +181,18 @@ class CSVParser(Parser): tag_2,tag 2,tag_1 ``` """ + format = ParserFormat.CSV empty_field_error = EmptyCSVField inital_row = 2 @classmethod - def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[ParserError]]: + def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[TagParserError]]: """ Read a .csv file and validates the header fields """ file.seek(0) - text_tags = TextIOWrapper(file, encoding='utf-8') + text_tags = TextIOWrapper(file, encoding="utf-8") csv_reader = csv.DictReader(text_tags) header_fields = csv_reader.fieldnames errors = cls._veify_header(header_fields) @@ -190,7 +201,7 @@ def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[ParserError]]: return list(csv_reader), [] @classmethod - def _veify_header(cls, header_fields: List[str]) -> List[ParserError]: + def _veify_header(cls, header_fields: List[str]) -> List[TagParserError]: """ Verify that the header contains the required fields """ @@ -198,19 +209,18 @@ def _veify_header(cls, header_fields: List[str]) -> List[ParserError]: print(header_fields) for req_field in cls.required_fields: if req_field not in header_fields: - errors.append(InvalidFormat( - tag=None, - format=cls.format.value, - message=_(f"Missing '{req_field}' field on CSV headers") - )) + errors.append( + InvalidFormat( + tag=None, + format=cls.format.value, + message=_(f"Missing '{req_field}' field on CSV headers"), + ) + ) return errors - + # Add parsers here -_parsers = [ - JSONParser, - CSVParser -] +_parsers = [JSONParser, CSVParser] def get_parser(parser_format: ParserFormat) -> Parser: @@ -223,8 +233,4 @@ def get_parser(parser_format: ParserFormat) -> Parser: if parser_format == parser.format: return parser - raise ValueError( - _( - f"Parser not found for format {parser_format}" - ) - ) + raise ValueError(_(f"Parser not found for format {parser_format}")) diff --git a/openedx_tagging/core/tagging/import_export/utils.py b/openedx_tagging/core/tagging/import_export/utils.py deleted file mode 100644 index e69de29bb..000000000 From 7e391d8edddc0763c71ada542943801dd30236a8 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 26 Jul 2023 15:49:12 -0500 Subject: [PATCH 06/20] feat: Implemented execute() func in actions --- .../core/tagging/import_export/actions.py | 61 ++++++-- .../core/tagging/import_export/api.py | 5 +- .../core/tagging/import_export/dsl.py | 11 +- .../tagging/import_export/test_actions.py | 134 +++++++++++++++++- 4 files changed, 194 insertions(+), 17 deletions(-) diff --git a/openedx_tagging/core/tagging/import_export/actions.py b/openedx_tagging/core/tagging/import_export/actions.py index 261f8ed75..d1ec460bc 100644 --- a/openedx_tagging/core/tagging/import_export/actions.py +++ b/openedx_tagging/core/tagging/import_export/actions.py @@ -15,6 +15,7 @@ class ImportAction: """ name = "import_action" + success = False def __init__(self, taxonomy: Taxonomy, tag, index: int): self.taxonomy = taxonomy @@ -22,10 +23,10 @@ def __init__(self, taxonomy: Taxonomy, tag, index: int): self.index = index def __repr__(self): - return _(f"Action {self.name} (index={self.index},id={self.tag.id})") + return str(_(f"Action {self.name} (index={self.index},id={self.tag.id})")) def __str__(self): - return str(self.__repr__) + return self.__repr__() @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: @@ -44,8 +45,17 @@ def validate(self, indexed_actions) -> List[ImportActionError]: raise NotImplementedError def execute(self): + """ + Implement this to execute the action. + """ raise NotImplementedError + def _get_tag(self): + """ + Returns the respective tag of this actions + """ + return self.taxonomy.tag_set.get(external_id=self.tag.id) + def _search_action( self, indexed_actions: dict, @@ -197,7 +207,19 @@ def validate(self, indexed_actions) -> List[ImportActionError]: return errors def execute(self): - pass + """ + Creates a Tag + """ + parent = None + if self.tag.parent_id: + parent = self.taxonomy.tag_set.get(external_id=self.tag.parent_id) + tag = Tag( + taxonomy=self.taxonomy, + parent=parent, + value=self.tag.value, + external_id=self.tag.id, + ) + tag.save() class UpdateParentTag(ImportAction): @@ -214,7 +236,7 @@ class UpdateParentTag(ImportAction): name = "update_parent" def __str__(self): - taxonomy_tag = self.taxonomy.tag_set.get(external_id=self.tag.id) + taxonomy_tag = self._get_tag() if not taxonomy_tag.parent: from_str = _("from empty parent") else: @@ -256,7 +278,15 @@ def validate(self, indexed_actions) -> List[ImportActionError]: return errors def execute(self): - pass + """ + Updates the parent of a tag + """ + tag = self._get_tag() + parent = None + if self.tag.parent_id: + parent = self.taxonomy.tag_set.get(external_id=self.tag.parent_id) + tag.parent = parent + tag.save() class RenameTag(ImportAction): @@ -273,7 +303,7 @@ class RenameTag(ImportAction): name = "rename" def __str__(self): - taxonomy_tag = self.taxonomy.tag_set.get(external_id=self.tag.id) + taxonomy_tag = self._get_tag() return str( _( f"Rename tag value of tag (id={taxonomy_tag.id}) " @@ -306,7 +336,12 @@ def validate(self, indexed_actions) -> List[ImportActionError]: return errors def execute(self): - pass + """ + Rename a tag + """ + tag = self._get_tag() + tag.value = self.tag.value + tag.save() class DeleteTag(ImportAction): @@ -319,7 +354,7 @@ class DeleteTag(ImportAction): """ def __str__(self): - taxonomy_tag = self.taxonomy.tag_set.get(external_id=self.tag.id) + taxonomy_tag = self._get_tag() return str(_(f"Delete tag (id={taxonomy_tag.id})")) name = "delete" @@ -343,7 +378,11 @@ def validate(self, indexed_actions) -> List[ImportActionError]: return [] def execute(self): - pass + """ + Delete a tag + """ + tag = self._get_tag() + tag.delete() class WithoutChanges(ImportAction): @@ -372,7 +411,9 @@ def validate(self, indexed_actions) -> List[ImportActionError]: return [] def execute(self): - pass + """ + Do nothing + """ # Register actions here in the order in which you want to check. diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py index 2d534bbf0..5360d11a0 100644 --- a/openedx_tagging/core/tagging/import_export/api.py +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -27,7 +27,10 @@ def import_tags( # Execute the plan if execute: - return dsl.execute() + try: + return dsl.execute() + except Exception as error: + return str(error) # Or return the plan return dsl.plan() diff --git a/openedx_tagging/core/tagging/import_export/dsl.py b/openedx_tagging/core/tagging/import_export/dsl.py index 0a237d088..777ec76b8 100644 --- a/openedx_tagging/core/tagging/import_export/dsl.py +++ b/openedx_tagging/core/tagging/import_export/dsl.py @@ -3,10 +3,10 @@ """ from typing import List, Optional +from django.db import transaction from django.utils.translation import gettext_lazy as _ from ..models import Taxonomy -from .exceptions import TagImportError from .actions import ( DeleteTag, ImportAction, @@ -123,7 +123,7 @@ def generate_actions( Validates each action and create respective errors If `replace` is True, then deletes the tags that have not been read - TODO: Missing join/reduce actions. Ex. A tag may have no changes, + TODO: Missing join/reduce actions. Eg. A tag may have no changes, but then its parent needs to be updated because its parent is deleted. Those two actions should be merged. """ @@ -157,8 +157,11 @@ def generate_actions( # Delete all not readed tags self._build_delete_actions(tags_for_delete) - def execute(self) -> List[TagImportError]: - pass + @transaction.atomic() + def execute(self): + for action in self.actions: + action.execute() + action.success = True def plan(self) -> str: """ diff --git a/tests/openedx_tagging/core/tagging/import_export/test_actions.py b/tests/openedx_tagging/core/tagging/import_export/test_actions.py index 3d5c0ea86..0ca9b0573 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_actions.py @@ -5,7 +5,7 @@ from django.test.testcases import TestCase -from openedx_tagging.core.tagging.models import Taxonomy +from openedx_tagging.core.tagging.models import Taxonomy, Tag from openedx_tagging.core.tagging.import_export.dsl import TagDSL from openedx_tagging.core.tagging.import_export.actions import ( ImportAction, @@ -67,6 +67,18 @@ def test_not_implemented_functions(self): with self.assertRaises(NotImplementedError): action.execute() + def test_str(self): + expected = "Action import_action (index=100,id=tag_1)" + action = ImportAction( + taxonomy=self.taxonomy, + tag=TagDSL( + id='tag_1', + value='value', + ), + index=100, + ) + assert str(action) == expected + @ddt.data( ('create', 'id', 'tag_10', True), ('rename', 'value', 'Tag 11', True), @@ -231,6 +243,32 @@ def test_validate(self, tag_id, tag_value, parent_id, expected): errors = action.validate(self.indexed_actions) self.assertEqual(len(errors), expected) + @ddt.data( + ('tag_30', 'Tag 30', None), # No parent + ('tag_31', 'Tag 31', 'tag_3'), # With parent + ) + @ddt.unpack + def test_execute(self, tag_id, value, parent_id): + tag = TagDSL( + id=tag_id, + value=value, + parent_id=parent_id, + ) + action = CreateTag( + self.taxonomy, + tag, + index=100, + ) + with self.assertRaises(Tag.DoesNotExist): + self.taxonomy.tag_set.get(external_id=tag_id) + action.execute() + tag = self.taxonomy.tag_set.get(external_id=tag_id) + assert tag.value == value + if parent_id: + assert tag.parent.external_id == parent_id + else: + assert tag.parent is None + @ddt.ddt class TestUpdateParentTag(TestImportActionMixin, TestCase): @@ -238,6 +276,38 @@ class TestUpdateParentTag(TestImportActionMixin, TestCase): Test for 'update_parent' action """ + @ddt.data( + ( + "tag_4", + "tag_3", + ( + "Update the parent of tag (id=25) from parent " + "(external_id=tag_3) to parent (external_id=tag_3)." + ) + ), + ( + "tag_3", + "tag_2", + ( + "Update the parent of tag (id=24) from empty parent " + "to parent (external_id=tag_2)." + ) + ), + ) + @ddt.unpack + def test_str(self, tag_id, parent_id, expected): + tag_dsl = TagDSL( + id=tag_id, + value='_', + parent_id=parent_id, + ) + action = UpdateParentTag( + taxonomy=self.taxonomy, + tag=tag_dsl, + index=100, + ) + assert str(action) == expected + @ddt.data( ('tag_100', None, False), # Tag doesn't exist on database ('tag_2', 'tag_1', False), # Parent don't change @@ -271,13 +341,40 @@ def test_validate(self, tag_id, parent_id, expected): id=tag_id, value='_', parent_id=parent_id, - index=100, ), index=100 ) errors = action.validate(self.indexed_actions) self.assertEqual(len(errors), expected) + @ddt.data( + ('tag_4', 'tag_2'), # Change parent + ('tag_4', None), # Set parent as None + ('tag_3', 'tag_1'), # Add parent + ) + @ddt.unpack + def test_execute(self, tag_id, parent_id): + tag_dsl = TagDSL( + id=tag_id, + value='_', + parent_id=parent_id, + ) + action = UpdateParentTag( + taxonomy=self.taxonomy, + tag=tag_dsl, + index=100, + ) + tag = self.taxonomy.tag_set.get(external_id=tag_id) + if tag.parent: + assert tag.parent.external_id != parent_id + action.execute() + tag = self.taxonomy.tag_set.get(external_id=tag_id) + if not parent_id: + assert tag.parent is None + else: + assert tag.parent.external_id == parent_id + + @ddt.ddt class TestRenameTag(TestImportActionMixin, TestCase): """ @@ -321,6 +418,24 @@ def test_validate(self, value, expected): errors = action.validate(self.indexed_actions) self.assertEqual(len(errors), expected) + def test_execute(self): + tag_id = 'tag_1' + value = 'Tag 1 V2' + tag_dsl = TagDSL( + id=tag_id, + value=value, + ) + action = RenameTag( + taxonomy=self.taxonomy, + tag=tag_dsl, + index=100, + ) + tag = self.taxonomy.tag_set.get(external_id=tag_id) + assert tag.value != value + action.execute() + tag = self.taxonomy.tag_set.get(external_id=tag_id) + assert tag.value == value + @ddt.ddt class TestDeleteTag(TestImportActionMixin, TestCase): @@ -359,6 +474,21 @@ def test_validate(self): result = action.validate(self.indexed_actions) self.assertEqual(result, []) + def test_execute(self): + tag_id = 'tag_3' + tag_dsl = TagDSL( + id=tag_id, + value='_', + ) + action = DeleteTag( + taxonomy=self.taxonomy, + tag=tag_dsl, + index=100, + ) + assert self.taxonomy.tag_set.filter(external_id=tag_id).exists() + action.execute() + assert not self.taxonomy.tag_set.filter(external_id=tag_id).exists() + class TestWithoutChanges(TestImportActionMixin, TestCase): """ From e00885b2611ca8e891bb29c663f5028624bd5522 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 27 Jul 2023 16:26:38 -0500 Subject: [PATCH 07/20] feat: execute() function on DSL --- .../core/tagging/import_export/dsl.py | 37 +-- .../core/tagging/import_export/exceptions.py | 4 +- .../core/tagging/import_export/parsers.py | 3 + .../core/tagging/import_export/test_dsl.py | 227 ++++++++++++++---- .../tagging/import_export/test_parsers.py | 17 ++ 5 files changed, 218 insertions(+), 70 deletions(-) diff --git a/openedx_tagging/core/tagging/import_export/dsl.py b/openedx_tagging/core/tagging/import_export/dsl.py index 777ec76b8..bd165c1e4 100644 --- a/openedx_tagging/core/tagging/import_export/dsl.py +++ b/openedx_tagging/core/tagging/import_export/dsl.py @@ -74,7 +74,7 @@ def _build_action(self, action_cls, tag: TagDSL): Run action validation and adds the errors to the errors lists Add to the action list and the indexed actions """ - action = action_cls(self.taxonomy, tag, len(self.actions)) + action = action_cls(self.taxonomy, tag, len(self.actions) + 1) # We validate if there are no inconsistencies when executing this action self.errors.extend(action.validate(self.indexed_actions)) @@ -91,17 +91,15 @@ def _build_delete_actions(self, tags: dict): """ for tag in tags.values(): for child in tag.children.all(): - if child.external_id not in tags: - # If the child is not to be removed, - # then update its parent - self._build_action( - UpdateParentTag, - TagDSL( - id=child.external_id, - value=child.value, - parent_id=None, - ), - ) + # child parent to avoid delete childs + self._build_action( + UpdateParentTag, + TagDSL( + id=child.external_id, + value=child.value, + parent_id=None, + ), + ) # Delete action self._build_action( @@ -157,12 +155,6 @@ def generate_actions( # Delete all not readed tags self._build_delete_actions(tags_for_delete) - @transaction.atomic() - def execute(self): - for action in self.actions: - action.execute() - action.success = True - def plan(self) -> str: """ Returns an string with the plan and errors @@ -177,3 +169,12 @@ def plan(self) -> str: result += f"{str(error)}\n" return result + + @transaction.atomic() + def execute(self): + if self.errors: + return False + for action in self.actions: + action.execute() + action.success = True + return True diff --git a/openedx_tagging/core/tagging/import_export/exceptions.py b/openedx_tagging/core/tagging/import_export/exceptions.py index 83bfaed43..5d2fa73bd 100644 --- a/openedx_tagging/core/tagging/import_export/exceptions.py +++ b/openedx_tagging/core/tagging/import_export/exceptions.py @@ -13,8 +13,8 @@ def __repr__(self): class TagParserError(TagImportError): - def __init__(self, tag: str, **kargs): - self.message = _(f"Import error on {tag}") + def __init__(self, tag, **kargs): + self.message = _(f"Import parser error on {tag}") class ImportActionError(TagImportError): diff --git a/openedx_tagging/core/tagging/import_export/parsers.py b/openedx_tagging/core/tagging/import_export/parsers.py index e36d1820c..830755d8a 100644 --- a/openedx_tagging/core/tagging/import_export/parsers.py +++ b/openedx_tagging/core/tagging/import_export/parsers.py @@ -58,11 +58,14 @@ def parse_import(cls, file: BytesIO) -> Tuple[List[TagDSL], List[TagParserError] tags_data, load_errors = cls._load_data(file) if load_errors: return [], load_errors + except Exception as error: + raise error finally: file.close() return cls._parse_tags(tags_data) + @classmethod def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[TagParserError]]: """ Each parser implements this function according to its format. diff --git a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py b/tests/openedx_tagging/core/tagging/import_export/test_dsl.py index 57a406f3b..c6e57fb85 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_dsl.py @@ -7,6 +7,7 @@ from openedx_tagging.core.tagging.import_export.dsl import TagDSL, TagImportDSL from openedx_tagging.core.tagging.import_export.actions import CreateTag +from openedx_tagging.core.tagging.import_export.exceptions import TagImportError from .test_actions import TestImportActionMixin @ddt.ddt @@ -19,6 +20,14 @@ def setUp(self): super().setUp() self.dsl = TagImportDSL(self.taxonomy) + def test_tag_import_error(self): + message = "Error message" + expected_repr = f"TagImportError({message})" + error = TagImportError(message) + assert str(error) == message + assert repr(error) == expected_repr + + @ddt.data( ('tag_10', 1), ('tag_30', 0), @@ -34,10 +43,10 @@ def test_build_action(self, tag_id, errors_expected): index=100 ) ) - self.assertEqual(len(self.dsl.errors), errors_expected) - self.assertEqual(len(self.dsl.actions), 1) - self.assertEqual(self.dsl.actions[0].name, 'create') - self.assertEqual(self.dsl.indexed_actions['create'][1].tag.id, tag_id) + assert len(self.dsl.errors) == errors_expected + assert len(self.dsl.actions) == 1 + assert self.dsl.actions[0].name == 'create' + assert self.dsl.indexed_actions['create'][1].tag.id == tag_id def test_build_delete_actions(self): tags = { @@ -48,22 +57,26 @@ def test_build_delete_actions(self): self.dsl.actions.clear() self.dsl._build_delete_actions(tags) # pylint: disable=protected-access - self.assertEqual(len(self.dsl.errors), 0) + assert len(self.dsl.errors) == 0 # Check actions in order - # #1 Delete 'tag_1' - self.assertEqual(self.dsl.actions[0].name, 'delete') - self.assertEqual(self.dsl.actions[0].tag.id, 'tag_1') - # #2 Delete 'tag_2' - self.assertEqual(self.dsl.actions[1].name, 'delete') - self.assertEqual(self.dsl.actions[1].tag.id, 'tag_2') - # #3 Update parent of 'tag_4' - self.assertEqual(self.dsl.actions[2].name, 'update_parent') - self.assertEqual(self.dsl.actions[2].tag.id, 'tag_4') - self.assertIsNone(self.dsl.actions[2].tag.parent_id) - # #4 Delete 'tag_3' - self.assertEqual(self.dsl.actions[3].name, 'delete') - self.assertEqual(self.dsl.actions[3].tag.id, 'tag_3') + # #1 Update parent of 'tag_2' + assert self.dsl.actions[0].name == 'update_parent' + assert self.dsl.actions[0].tag.id == 'tag_2' + assert self.dsl.actions[0].tag.parent_id is None + # #2 Delete 'tag_1' + assert self.dsl.actions[1].name == 'delete' + assert self.dsl.actions[1].tag.id == 'tag_1' + # #3 Delete 'tag_2' + assert self.dsl.actions[2].name == 'delete' + assert self.dsl.actions[2].tag.id == 'tag_2' + # #4 Update parent of 'tag_4' + assert self.dsl.actions[3].name == 'update_parent' + assert self.dsl.actions[3].tag.id == 'tag_4' + assert self.dsl.actions[3].tag.parent_id is None + # #5 Delete 'tag_3' + assert self.dsl.actions[4].name == 'delete' + assert self.dsl.actions[4].tag.id == 'tag_3' @ddt.data( ([ @@ -172,9 +185,13 @@ def test_build_delete_actions(self): 'name': 'without_changes', 'id': 'tag_4', }, + { + 'name': 'update_parent', + 'id': 'tag_2', + }, { 'name': 'delete', - 'id': 'tag_1' + 'id': 'tag_1', }, { 'name': 'delete', @@ -194,13 +211,13 @@ def test_build_delete_actions(self): def test_generate_actions(self, tags, replace, expected_errors, expected_actions): tags = [TagDSL(**tag) for tag in tags] self.dsl.generate_actions(tags=tags, replace=replace) - self.assertEqual(len(self.dsl.errors), expected_errors) - self.assertEqual(len(self.dsl.actions), len(expected_actions)) + assert len(self.dsl.errors) == expected_errors + assert len(self.dsl.actions) == len(expected_actions) for index, action in enumerate(expected_actions): - self.assertEqual(self.dsl.actions[index].name, action['name']) - self.assertEqual(self.dsl.actions[index].tag.id, action['id']) - self.assertEqual(self.dsl.actions[index].index, index) + assert self.dsl.actions[index].name == action['name'] + assert self.dsl.actions[index].tag.id == action['id'] + assert self.dsl.actions[index].index == index + 1 @ddt.data( ([ @@ -233,25 +250,25 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions False, "Plan\n" "--------------------------------\n" - "#0: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" - "#1: Create a new tag with values (external_id=tag_31, value=Tag 32, parent_id=None).\n" - "#2: Rename tag value of tag (id=22) from 'Tag 1' to 'Tag 2'\n" - "#3: Update the parent of tag (id=25) from parent (external_id=tag_3) to parent (external_id=tag_100).\n" - "#4: Create a new tag with values (external_id=tag_33, value=Tag 32, parent_id=None).\n" - "#5: Update the parent of tag (id=23) from parent (external_id=tag_1) to parent (external_id=None).\n" - "#6: Rename tag value of tag (id=23) from 'Tag 2' to 'Tag 31'\n" + "#1: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" + "#2: Create a new tag with values (external_id=tag_31, value=Tag 32, parent_id=None).\n" + "#3: Rename tag value of tag (id=22) from 'Tag 1' to 'Tag 2'\n" + "#4: Update the parent of tag (id=25) from parent (external_id=tag_3) to parent (external_id=tag_100).\n" + "#5: Create a new tag with values (external_id=tag_33, value=Tag 32, parent_id=None).\n" + "#6: Update the parent of tag (id=23) from parent (external_id=tag_1) to parent (external_id=None).\n" + "#7: Rename tag value of tag (id=23) from 'Tag 2' to 'Tag 31'\n" "Output errors\n" "--------------------------------\n" - "Conflict with 'create' (#1) and action #0: Duplicated external_id tag.\n" - "Action error in 'rename' (#2): Duplicated tag value with tag (id=23).\n" - "Action error in 'update_parent' (#3): Unknown parent tag (tag_100). " + "Conflict with 'create' (#2) and action #1: Duplicated external_id tag.\n" + "Action error in 'rename' (#3): Duplicated tag value with tag (id=23).\n" + "Action error in 'update_parent' (#4): Unknown parent tag (tag_100). " "You need to add parent before the child in your file.\n" - "Conflict with 'create' (#4) and action #1: Duplicated tag value.\n" - "Conflict with 'rename' (#6) and action #0: Duplicated tag value.\n" + "Conflict with 'create' (#5) and action #2: Duplicated tag value.\n" + "Conflict with 'rename' (#7) and action #1: Duplicated tag value.\n" ), ([ { - 'id': 'tag_31', + 'id': 'tag_31', 'value': 'Tag 31', }, { @@ -277,12 +294,12 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions False, "Plan\n" "--------------------------------\n" - "#0: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" - "#1: Create a new tag with values (external_id=tag_32, value=Tag 32, parent_id=tag_1).\n" - "#2: Rename tag value of tag (id=23) from 'Tag 2' to 'Tag 2 v2'\n" - "#3: Update the parent of tag (id=25) from parent (external_id=tag_3) to parent (external_id=tag_1).\n" - "#4: Rename tag value of tag (id=25) from 'Tag 4' to 'Tag 4 v2'\n" - "#5: No changes needed for tag (external_id=tag_1)\n" + "#1: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" + "#2: Create a new tag with values (external_id=tag_32, value=Tag 32, parent_id=tag_1).\n" + "#3: Rename tag value of tag (id=23) from 'Tag 2' to 'Tag 2 v2'\n" + "#4: Update the parent of tag (id=25) from parent (external_id=tag_3) to parent (external_id=tag_1).\n" + "#5: Rename tag value of tag (id=25) from 'Tag 4' to 'Tag 4 v2'\n" + "#6: No changes needed for tag (external_id=tag_1)\n" ), ([ { @@ -294,11 +311,12 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions True, "Plan\n" "--------------------------------\n" - "#0: No changes needed for tag (external_id=tag_4)\n" - "#1: Delete tag (id=22)\n" - "#2: Delete tag (id=23)\n" - "#3: Update the parent of tag (id=25) from parent (external_id=tag_3) to parent (external_id=None).\n" - "#4: Delete tag (id=24)\n" + "#1: No changes needed for tag (external_id=tag_4)\n" + "#2: Update the parent of tag (id=23) from parent (external_id=tag_1) to parent (external_id=None).\n" + "#3: Delete tag (id=22)\n" + "#4: Delete tag (id=23)\n" + "#5: Update the parent of tag (id=25) from parent (external_id=tag_3) to parent (external_id=None).\n" + "#6: Delete tag (id=24)\n" ) ) @ddt.unpack @@ -306,5 +324,114 @@ def test_plan(self, tags, replace, expected): tags = [TagDSL(**tag) for tag in tags] self.dsl.generate_actions(tags=tags, replace=replace) plan = self.dsl.plan() - print(plan) - self.assertEqual(plan, expected) + assert plan == expected + + @ddt.data( + ([ + { + 'id': 'tag_31', + 'value': 'Tag 31', + }, + { + 'id': 'tag_32', + 'value': 'Tag 32', + }, + { + 'id': 'tag_1', + 'value': 'Tag 1 V2', + }, + { + 'id': 'tag_4', + 'value': 'Tag 4', + 'parent_id': 'tag_32', + }, + { + 'id': 'tag_33', + 'value': 'Tag 33', + }, + { + 'id': 'tag_2', + 'value': 'Tag 2 V2', + }, + ], False), + ([ + { + 'id': 'tag_31', + 'value': 'Tag 31', + }, + { + 'id': 'tag_32', + 'value': 'Tag 32', + 'parent_id': 'tag_1', + }, + { + 'id': 'tag_2', + 'value': 'Tag 2 v2', + 'parent_id': 'tag_1' + }, + { + 'id': 'tag_4', + 'value': 'Tag 4 v2', + 'parent_id': 'tag_1', + }, + { + 'id': 'tag_1', + 'value': 'Tag 1', + }, + ], False), + ([ + { + 'id': 'tag_4', + 'value': 'Tag 4', + 'parent_id': 'tag_3', + }, + ], True), + ) + @ddt.unpack + def test_execute(self, tags, replace): + tags = [TagDSL(**tag) for tag in tags] + self.dsl.generate_actions(tags=tags, replace=replace) + assert self.dsl.execute() + tag_external_ids = [] + for tag_dsl in tags: + # This checks any creation + print(tag_dsl.id) + tag = self.taxonomy.tag_set.get(external_id=tag_dsl.id) + + # Checks any rename + assert tag.value == tag_dsl.value + + # Checks any parent update + if not replace: + if not tag_dsl.parent_id: + assert tag.parent is None + else: + assert tag.parent.external_id == tag_dsl.parent_id + + tag_external_ids.append(tag_dsl.id) + + for action in self.dsl.actions: + assert action.success + + if replace: + # Checks deletions checking that exists the updated tags + external_ids = list(self.taxonomy.tag_set.values_list("external_id", flat=True)) + assert tag_external_ids == external_ids + + def test_error_in_execute(self): + created_tag = 'tag_31' + tags = [ + TagDSL( + id=created_tag, + value='Tag 31' + ), # Valid tag (creation) + TagDSL( + id='tag_32', + value='Tag 31' + ), # Invalid + ] + self.dsl.generate_actions(tags=tags) + assert not self.taxonomy.tag_set.filter(external_id=created_tag).exists() + assert not self.dsl.execute() + assert not self.taxonomy.tag_set.filter(external_id=created_tag).exists() + \ No newline at end of file diff --git a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py index e69fc1a7b..ea5d30e8f 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py @@ -8,11 +8,15 @@ from django.test.testcases import TestCase from openedx_tagging.core.tagging.import_export.parsers import ( + Parser, get_parser, JSONParser, CSVParser, ParserFormat, ) +from openedx_tagging.core.tagging.import_export.exceptions import ( + TagParserError, +) class TestParser(TestCase): @@ -29,6 +33,19 @@ def test_parser_not_found(self): with self.assertRaises(ValueError): get_parser(None) + def test_not_implemented(self): + with self.assertRaises(NotImplementedError): + Parser.parse_import(BytesIO()) + + def test_tag_parser_error(self): + tag = {"id": 'tag_id', "value": "tag_value"} + expected_str = f"Import parser error on {tag}" + expected_repr = f"TagParserError(Import parser error on {tag})" + error = TagParserError(tag) + assert str(error) == expected_str + assert repr(error) == expected_repr + + @ddt.ddt class TestJSONParser(TestCase): """ From e6a936482e88c0044e024351ab5972d47ac9ff31 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 2 Aug 2023 16:16:33 -0500 Subject: [PATCH 08/20] feat: Added TagImportTask feature --- .../core/tagging/import_export/actions.py | 1 - .../core/tagging/import_export/api.py | 83 +++++++++----- .../import_export/{dsl.py => models.py} | 101 +++++++++++++++++- .../core/tagging/import_export/parsers.py | 2 +- .../core/tagging/import_export/tasks.py | 21 ++++ requirements/base.in | 2 + requirements/base.txt | 35 +++++- requirements/ci.txt | 2 +- requirements/dev.txt | 54 ++++++++-- requirements/doc.txt | 55 ++++++++-- requirements/pip-tools.txt | 2 +- requirements/quality.txt | 58 ++++++++-- requirements/test.txt | 51 ++++++++- .../tagging/import_export/test_actions.py | 2 +- .../core/tagging/import_export/test_dsl.py | 2 +- 15 files changed, 414 insertions(+), 57 deletions(-) rename openedx_tagging/core/tagging/import_export/{dsl.py => models.py} (63%) create mode 100644 openedx_tagging/core/tagging/import_export/tasks.py diff --git a/openedx_tagging/core/tagging/import_export/actions.py b/openedx_tagging/core/tagging/import_export/actions.py index d1ec460bc..a411be60d 100644 --- a/openedx_tagging/core/tagging/import_export/actions.py +++ b/openedx_tagging/core/tagging/import_export/actions.py @@ -15,7 +15,6 @@ class ImportAction: """ name = "import_action" - success = False def __init__(self, taxonomy: Taxonomy, tag, index: int): self.taxonomy = taxonomy diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py index 5360d11a0..c0799c607 100644 --- a/openedx_tagging/core/tagging/import_export/api.py +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -1,36 +1,69 @@ from io import BytesIO +from django.utils.translation import gettext_lazy as _ + from ..models import Taxonomy from .parsers import get_parser, ParserFormat -from .dsl import TagImportDSL +from .models import TagImportDSL, TagImportTask, TagImportTaskState -# TODO This function must be a celery task def import_tags( taxonomy: Taxonomy, file: BytesIO, parser_format: ParserFormat, replace=False, - execute=False, -): - # Get the parser and parse the file - parser = get_parser(parser_format) - tags, errors = parser.parse_import(file) - - # Check if there are errors in the parse - if errors: - return errors - - # Generate the actions - dsl = TagImportDSL() - dsl.generate_actions(tags, taxonomy, replace) - - # Execute the plan - if execute: - try: - return dsl.execute() - except Exception as error: - return str(error) - - # Or return the plan - return dsl.plan() +) -> bool: + # Checks that exists only one tag + if not _check_unique_import_task(taxonomy): + raise ValueError(_( + "There is an import task running. " + "Only one task per taxonomy can be created at a time." + )) + + # Creating import task + task = TagImportTask.create(taxonomy) + + try: + # Get the parser and parse the file + task.log_parser_start() + parser = get_parser(parser_format) + tags, errors = parser.parse_import(file) + task.log_parser_end() + + # Check if there are errors in the parse + if errors: + task.handle_parser_errors(errors) + return False + + # Generate actions + task.log_start_planning() + dsl = TagImportDSL() + dsl.generate_actions(tags, taxonomy, replace) + task.log_end_planning(dsl) + + if dsl.errors: + task.handle_plan_errors() + return False + + return dsl.execute() + except Exception as exception: + # Log any exception + task.log_exception(exception) + return False + + +def _check_unique_import_task(taxonomy: Taxonomy) -> bool: + last_task = ( + TagImportTask.objects + .filter(taxonomy=taxonomy) + .order_by('-creation_date') + .first() + ) + if not last_task: + return False + return (last_task.status != TagImportTaskState.SUCCESS + and last_task.status != TagImportTaskState.ERROR) + + +def check_import_staus(): + pass diff --git a/openedx_tagging/core/tagging/import_export/dsl.py b/openedx_tagging/core/tagging/import_export/models.py similarity index 63% rename from openedx_tagging/core/tagging/import_export/dsl.py rename to openedx_tagging/core/tagging/import_export/models.py index bd165c1e4..c51bf98e8 100644 --- a/openedx_tagging/core/tagging/import_export/dsl.py +++ b/openedx_tagging/core/tagging/import_export/models.py @@ -1,6 +1,8 @@ """ Model and functions to create a plan/execution with DSL actions. """ +from datetime import datetime +from enum import Enum from typing import List, Optional from django.db import transaction @@ -14,6 +16,97 @@ WithoutChanges, available_actions, ) +from .exceptions import ImportActionError, TagParserError +from django.db import models + +class TagImportTaskState(Enum): + LOADING_DATA = 'loading_data' + PLANNING = 'planning' + EXECUTING = 'executing' + SUCCESS = 'success' + ERROR = 'error' + + +class TagImportTask(models.Model): + """ + Stores the state, plan and logs of a tag import task + """ + id = models.BigAutoField(primary_key=True) + + taxonomy = models.ForeignKey( + "Taxonomy", + on_delete=models.CASCADE, + help_text=_("Taxonomy associated with this import"), + ) + + log = models.TextField( + null=True, + default=None, + help_text=_("Action execution logs") + ) + + status = models.CharField( + max_length=20, + choices=[(status, status.value) for status in TagImportTaskState], + help_text=_("Task status"), + ) + + creation_date = models.DateTimeField(auto_now_add=True) + + class Meta: + indexes = [ + models.Index(fields=['taxonomy', '-creation_date']), + ] + + @classmethod + def create(cls, taxonomy: Taxonomy): + task = cls( + taxonomy=taxonomy, + status=TagImportTaskState.LOADING_DATA.value, + log='', + ) + task.save() + return task + + def add_log(self, message: str, save=True): + timestamp = datetime.now().strftime('%Y-%m-%d %H:%M:%S') + log_message = f"[{timestamp}] {message}\n" + self.log += log_message + if save: + self.save() + + def log_exception(self, exception: Exception): + self.add_log(str(exception), save=False) + self.status = TagImportTaskState.ERROR.value + self.save() + + def log_parser_start(self): + self.add_log(_("Starting to load data from file")) + + def log_parser_end(self): + self.add_log(_("Load data finished")) + + def handle_parser_errors(self, errors: List[TagParserError]): + for error in errors: + self.add_log(f"{str(error)}", save=False) + self.status = TagImportTaskState.ERROR.value + self.save() + + def log_start_planning(self): + self.add_log(_("Starting planning the actions"), save=False) + self.status = TagImportTaskState.PLANNING.value + self.save() + + def log_end_planning(self, plan: "TagDSL"): + self.add_log(_("Plan finished")) + plan_str = plan.plan() + self.log += plan_str + self.save() + + def handle_plan_errors(self): + # Error are logged with plan + self.status = TagImportTaskState.ERROR.value + self.save() class TagDSL: @@ -48,6 +141,7 @@ class TagImportDSL: """ actions: List[ImportAction] + errors: List[ImportActionError] indexed_actions: dict actions_dict: dict taxonomy: Taxonomy @@ -171,10 +265,13 @@ def plan(self) -> str: return result @transaction.atomic() - def execute(self): + def execute(self, task: TagImportTask = None): if self.errors: return False for action in self.actions: + if task: + task.add_log(f"{str(action)} [Started]") action.execute() - action.success = True + if task: + task.add_log(f"{str(action)} [Success]") return True diff --git a/openedx_tagging/core/tagging/import_export/parsers.py b/openedx_tagging/core/tagging/import_export/parsers.py index 830755d8a..2f0529bf6 100644 --- a/openedx_tagging/core/tagging/import_export/parsers.py +++ b/openedx_tagging/core/tagging/import_export/parsers.py @@ -9,7 +9,7 @@ from django.utils.translation import gettext_lazy as _ -from .dsl import TagDSL +from .models import TagDSL from .exceptions import ( TagParserError, InvalidFormat, diff --git a/openedx_tagging/core/tagging/import_export/tasks.py b/openedx_tagging/core/tagging/import_export/tasks.py new file mode 100644 index 000000000..f04cb0a30 --- /dev/null +++ b/openedx_tagging/core/tagging/import_export/tasks.py @@ -0,0 +1,21 @@ +from io import BytesIO +from celery import shared_task + +from .api import import_tags +from ..models import Taxonomy +from .parsers import ParserFormat + + +@shared_task +def import_tags_async( + taxonomy: Taxonomy, + file: BytesIO, + parser_format: ParserFormat, + replace=False, +): + import_tags( + taxonomy, + file, + parser_format, + replace, + ) \ No newline at end of file diff --git a/requirements/base.in b/requirements/base.in index d17e06397..8fa8e805d 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -1,6 +1,8 @@ # Core requirements for using this application -c constraints.txt +celery # Asynchronous task execution library + Django<5.0 # Web application framework djangorestframework<4.0 # REST API diff --git a/requirements/base.txt b/requirements/base.txt index 8c0f00b63..0f2ee63e0 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -1,11 +1,29 @@ # -# This file is autogenerated by pip-compile with Python 3.8 +# This file is autogenerated by pip-compile with Python 3.10 # by the following command: # # make upgrade # +amqp==5.1.1 + # via kombu asgiref==3.7.2 # via django +billiard==3.6.4.0 + # via celery +celery==5.2.0 + # via -r requirements/base.in +click==8.1.3 + # via + # celery + # click-didyoumean + # click-plugins + # click-repl +click-didyoumean==0.3.0 + # via celery +click-plugins==1.1.1 + # via celery +click-repl==0.3.0 + # via celery django==3.2.19 # via # -c requirements/constraints.txt @@ -13,8 +31,13 @@ django==3.2.19 # djangorestframework djangorestframework==3.14.0 # via -r requirements/base.in +kombu==5.3.1 + # via celery +prompt-toolkit==3.0.39 + # via click-repl pytz==2023.3 # via + # celery # django # djangorestframework rules==3.3 @@ -23,3 +46,13 @@ sqlparse==0.4.4 # via django typing-extensions==4.6.3 # via asgiref +vine==5.0.0 + # via + # amqp + # celery + # kombu +wcwidth==0.2.6 + # via prompt-toolkit + +# The following packages are considered to be unsafe in a requirements file: +# setuptools diff --git a/requirements/ci.txt b/requirements/ci.txt index 6e1dd7f92..502b1be85 100644 --- a/requirements/ci.txt +++ b/requirements/ci.txt @@ -1,5 +1,5 @@ # -# This file is autogenerated by pip-compile with Python 3.8 +# This file is autogenerated by pip-compile with Python 3.10 # by the following command: # # make upgrade diff --git a/requirements/dev.txt b/requirements/dev.txt index 48cf4fb40..dec4b89ff 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -1,9 +1,13 @@ # -# This file is autogenerated by pip-compile with Python 3.8 +# This file is autogenerated by pip-compile with Python 3.10 # by the following command: # # make upgrade # +amqp==5.1.1 + # via + # -r requirements/quality.txt + # kombu asgiref==3.7.2 # via # -r requirements/quality.txt @@ -13,6 +17,10 @@ astroid==2.15.5 # -r requirements/quality.txt # pylint # pylint-celery +billiard==3.6.4.0 + # via + # -r requirements/quality.txt + # celery bleach==6.0.0 # via # -r requirements/quality.txt @@ -21,6 +29,8 @@ build==0.10.0 # via # -r requirements/pip-tools.txt # pip-tools +celery==5.2.0 + # via -r requirements/quality.txt certifi==2023.5.7 # via # -r requirements/quality.txt @@ -40,15 +50,31 @@ click==8.1.3 # -r requirements/ci.txt # -r requirements/pip-tools.txt # -r requirements/quality.txt + # celery + # click-didyoumean # click-log + # click-plugins + # click-repl # code-annotations # edx-lint # import-linter # pip-tools +click-didyoumean==0.3.0 + # via + # -r requirements/quality.txt + # celery click-log==0.4.0 # via # -r requirements/quality.txt # edx-lint +click-plugins==1.1.1 + # via + # -r requirements/quality.txt + # celery +click-repl==0.3.0 + # via + # -r requirements/quality.txt + # celery code-annotations==1.3.0 # via # -r requirements/quality.txt @@ -121,10 +147,6 @@ importlib-metadata==6.7.0 # -r requirements/quality.txt # keyring # twine -importlib-resources==5.12.0 - # via - # -r requirements/quality.txt - # keyring iniconfig==2.0.0 # via # -r requirements/quality.txt @@ -151,6 +173,10 @@ keyring==24.0.0 # via # -r requirements/quality.txt # twine +kombu==5.3.1 + # via + # -r requirements/quality.txt + # celery lazy-object-proxy==1.9.0 # via # -r requirements/quality.txt @@ -214,6 +240,10 @@ pluggy==1.0.0 # tox polib==1.2.0 # via edx-i18n-tools +prompt-toolkit==3.0.39 + # via + # -r requirements/quality.txt + # click-repl py==1.11.0 # via # -r requirements/ci.txt @@ -272,6 +302,7 @@ python-slugify==8.0.1 pytz==2023.3 # via # -r requirements/quality.txt + # celery # django # djangorestframework pyyaml==6.0 @@ -363,17 +394,25 @@ typing-extensions==4.6.3 # astroid # grimp # import-linter - # pylint - # rich urllib3==2.0.3 # via # -r requirements/quality.txt # requests # twine +vine==5.0.0 + # via + # -r requirements/quality.txt + # amqp + # celery + # kombu virtualenv==20.23.1 # via # -r requirements/ci.txt # tox +wcwidth==0.2.6 + # via + # -r requirements/quality.txt + # prompt-toolkit webencodings==0.5.1 # via # -r requirements/quality.txt @@ -390,7 +429,6 @@ zipp==3.15.0 # via # -r requirements/quality.txt # importlib-metadata - # importlib-resources # The following packages are considered to be unsafe in a requirements file: # pip diff --git a/requirements/doc.txt b/requirements/doc.txt index f63eb9113..03eae3799 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -1,5 +1,5 @@ # -# This file is autogenerated by pip-compile with Python 3.8 +# This file is autogenerated by pip-compile with Python 3.10 # by the following command: # # make upgrade @@ -8,6 +8,10 @@ accessible-pygments==0.0.4 # via pydata-sphinx-theme alabaster==0.7.13 # via sphinx +amqp==5.1.1 + # via + # -r requirements/test.txt + # kombu asgiref==3.7.2 # via # -r requirements/test.txt @@ -18,8 +22,14 @@ babel==2.12.1 # sphinx beautifulsoup4==4.12.2 # via pydata-sphinx-theme +billiard==3.6.4.0 + # via + # -r requirements/test.txt + # celery bleach==6.0.0 # via readme-renderer +celery==5.2.0 + # via -r requirements/test.txt certifi==2023.5.7 # via requests charset-normalizer==3.1.0 @@ -27,8 +37,24 @@ charset-normalizer==3.1.0 click==8.1.3 # via # -r requirements/test.txt + # celery + # click-didyoumean + # click-plugins + # click-repl # code-annotations # import-linter +click-didyoumean==0.3.0 + # via + # -r requirements/test.txt + # celery +click-plugins==1.1.1 + # via + # -r requirements/test.txt + # celery +click-repl==0.3.0 + # via + # -r requirements/test.txt + # celery code-annotations==1.3.0 # via -r requirements/test.txt coverage[toml]==7.2.7 @@ -71,8 +97,6 @@ imagesize==1.4.1 # via sphinx import-linter==1.9.0 # via -r requirements/test.txt -importlib-metadata==6.7.0 - # via sphinx iniconfig==2.0.0 # via # -r requirements/test.txt @@ -82,6 +106,10 @@ jinja2==3.1.2 # -r requirements/test.txt # code-annotations # sphinx +kombu==5.3.1 + # via + # -r requirements/test.txt + # celery markupsafe==2.1.3 # via # -r requirements/test.txt @@ -106,6 +134,10 @@ pluggy==1.0.0 # pytest pprintpp==0.4.0 # via sphinxcontrib-django +prompt-toolkit==3.0.39 + # via + # -r requirements/test.txt + # click-repl pydata-sphinx-theme==0.13.3 # via sphinx-book-theme pygments==2.15.1 @@ -131,7 +163,7 @@ python-slugify==8.0.1 pytz==2023.3 # via # -r requirements/test.txt - # babel + # celery # django # djangorestframework pyyaml==6.0 @@ -204,7 +236,18 @@ typing-extensions==4.6.3 # pydata-sphinx-theme urllib3==2.0.3 # via requests +vine==5.0.0 + # via + # -r requirements/test.txt + # amqp + # celery + # kombu +wcwidth==0.2.6 + # via + # -r requirements/test.txt + # prompt-toolkit webencodings==0.5.1 # via bleach -zipp==3.15.0 - # via importlib-metadata + +# The following packages are considered to be unsafe in a requirements file: +# setuptools diff --git a/requirements/pip-tools.txt b/requirements/pip-tools.txt index fd0cc1c78..92eba9e85 100644 --- a/requirements/pip-tools.txt +++ b/requirements/pip-tools.txt @@ -1,5 +1,5 @@ # -# This file is autogenerated by pip-compile with Python 3.8 +# This file is autogenerated by pip-compile with Python 3.10 # by the following command: # # make upgrade diff --git a/requirements/quality.txt b/requirements/quality.txt index 425cfa514..c857f3b3b 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -1,9 +1,13 @@ # -# This file is autogenerated by pip-compile with Python 3.8 +# This file is autogenerated by pip-compile with Python 3.10 # by the following command: # # make upgrade # +amqp==5.1.1 + # via + # -r requirements/test.txt + # kombu asgiref==3.7.2 # via # -r requirements/test.txt @@ -12,8 +16,14 @@ astroid==2.15.5 # via # pylint # pylint-celery +billiard==3.6.4.0 + # via + # -r requirements/test.txt + # celery bleach==6.0.0 # via readme-renderer +celery==5.2.0 + # via -r requirements/test.txt certifi==2023.5.7 # via requests cffi==1.15.1 @@ -23,12 +33,28 @@ charset-normalizer==3.1.0 click==8.1.3 # via # -r requirements/test.txt + # celery + # click-didyoumean # click-log + # click-plugins + # click-repl # code-annotations # edx-lint # import-linter +click-didyoumean==0.3.0 + # via + # -r requirements/test.txt + # celery click-log==0.4.0 # via edx-lint +click-plugins==1.1.1 + # via + # -r requirements/test.txt + # celery +click-repl==0.3.0 + # via + # -r requirements/test.txt + # celery code-annotations==1.3.0 # via # -r requirements/test.txt @@ -73,8 +99,6 @@ importlib-metadata==6.7.0 # via # keyring # twine -importlib-resources==5.12.0 - # via keyring iniconfig==2.0.0 # via # -r requirements/test.txt @@ -95,6 +119,10 @@ jinja2==3.1.2 # code-annotations keyring==24.0.0 # via twine +kombu==5.3.1 + # via + # -r requirements/test.txt + # celery lazy-object-proxy==1.9.0 # via astroid markdown-it-py==3.0.0 @@ -129,6 +157,10 @@ pluggy==1.0.0 # via # -r requirements/test.txt # pytest +prompt-toolkit==3.0.39 + # via + # -r requirements/test.txt + # click-repl pycodestyle==2.10.0 # via -r requirements/quality.in pycparser==2.21 @@ -169,6 +201,7 @@ python-slugify==8.0.1 pytz==2023.3 # via # -r requirements/test.txt + # celery # django # djangorestframework pyyaml==6.0 @@ -228,17 +261,26 @@ typing-extensions==4.6.3 # astroid # grimp # import-linter - # pylint - # rich urllib3==2.0.3 # via # requests # twine +vine==5.0.0 + # via + # -r requirements/test.txt + # amqp + # celery + # kombu +wcwidth==0.2.6 + # via + # -r requirements/test.txt + # prompt-toolkit webencodings==0.5.1 # via bleach wrapt==1.15.0 # via astroid zipp==3.15.0 - # via - # importlib-metadata - # importlib-resources + # via importlib-metadata + +# The following packages are considered to be unsafe in a requirements file: +# setuptools diff --git a/requirements/test.txt b/requirements/test.txt index 22c43aede..636639552 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -1,17 +1,44 @@ # -# This file is autogenerated by pip-compile with Python 3.8 +# This file is autogenerated by pip-compile with Python 3.10 # by the following command: # # make upgrade # +amqp==5.1.1 + # via + # -r requirements/base.txt + # kombu asgiref==3.7.2 # via # -r requirements/base.txt # django +billiard==3.6.4.0 + # via + # -r requirements/base.txt + # celery +celery==5.2.0 + # via -r requirements/base.txt click==8.1.3 # via + # -r requirements/base.txt + # celery + # click-didyoumean + # click-plugins + # click-repl # code-annotations # import-linter +click-didyoumean==0.3.0 + # via + # -r requirements/base.txt + # celery +click-plugins==1.1.1 + # via + # -r requirements/base.txt + # celery +click-repl==0.3.0 + # via + # -r requirements/base.txt + # celery code-annotations==1.3.0 # via -r requirements/test.in coverage[toml]==7.2.7 @@ -39,6 +66,10 @@ iniconfig==2.0.0 # via pytest jinja2==3.1.2 # via code-annotations +kombu==5.3.1 + # via + # -r requirements/base.txt + # celery markupsafe==2.1.3 # via jinja2 mock==5.0.2 @@ -51,6 +82,10 @@ pbr==5.11.1 # via stevedore pluggy==1.0.0 # via pytest +prompt-toolkit==3.0.39 + # via + # -r requirements/base.txt + # click-repl pytest==7.3.2 # via # -r requirements/test.in @@ -65,6 +100,7 @@ python-slugify==8.0.1 pytz==2023.3 # via # -r requirements/base.txt + # celery # django # djangorestframework pyyaml==6.0 @@ -91,3 +127,16 @@ typing-extensions==4.6.3 # asgiref # grimp # import-linter +vine==5.0.0 + # via + # -r requirements/base.txt + # amqp + # celery + # kombu +wcwidth==0.2.6 + # via + # -r requirements/base.txt + # prompt-toolkit + +# The following packages are considered to be unsafe in a requirements file: +# setuptools diff --git a/tests/openedx_tagging/core/tagging/import_export/test_actions.py b/tests/openedx_tagging/core/tagging/import_export/test_actions.py index 0ca9b0573..98ebd51da 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_actions.py @@ -6,7 +6,7 @@ from django.test.testcases import TestCase from openedx_tagging.core.tagging.models import Taxonomy, Tag -from openedx_tagging.core.tagging.import_export.dsl import TagDSL +from openedx_tagging.core.tagging.import_export.models import TagDSL from openedx_tagging.core.tagging.import_export.actions import ( ImportAction, CreateTag, diff --git a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py b/tests/openedx_tagging/core/tagging/import_export/test_dsl.py index c6e57fb85..4293bdce2 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_dsl.py @@ -5,7 +5,7 @@ from django.test.testcases import TestCase -from openedx_tagging.core.tagging.import_export.dsl import TagDSL, TagImportDSL +from openedx_tagging.core.tagging.import_export.models import TagDSL, TagImportDSL from openedx_tagging.core.tagging.import_export.actions import CreateTag from openedx_tagging.core.tagging.import_export.exceptions import TagImportError from .test_actions import TestImportActionMixin From 1d5d85cde077bc9900b8dc6a451b7aa12c1696ff Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 3 Aug 2023 10:51:10 -0500 Subject: [PATCH 09/20] feat: Creating TagImportTask model migration --- .../core/tagging/import_export/api.py | 2 +- .../{models.py => import_plan.py} | 96 +----------------- .../core/tagging/import_export/parsers.py | 2 +- .../migrations/0006_auto_20230802_1631.py | 29 ++++++ .../core/tagging/models/__init__.py | 1 + .../core/tagging/models/import_export.py | 97 +++++++++++++++++++ .../core/fixtures/tagging.yaml | 2 +- .../tagging/import_export/test_actions.py | 8 +- .../core/tagging/import_export/test_dsl.py | 32 +++--- 9 files changed, 150 insertions(+), 119 deletions(-) rename openedx_tagging/core/tagging/import_export/{models.py => import_plan.py} (65%) create mode 100644 openedx_tagging/core/tagging/migrations/0006_auto_20230802_1631.py create mode 100644 openedx_tagging/core/tagging/models/import_export.py diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py index c0799c607..31facee97 100644 --- a/openedx_tagging/core/tagging/import_export/api.py +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -4,7 +4,7 @@ from ..models import Taxonomy from .parsers import get_parser, ParserFormat -from .models import TagImportDSL, TagImportTask, TagImportTaskState +from .import_plan import TagImportDSL, TagImportTask, TagImportTaskState def import_tags( diff --git a/openedx_tagging/core/tagging/import_export/models.py b/openedx_tagging/core/tagging/import_export/import_plan.py similarity index 65% rename from openedx_tagging/core/tagging/import_export/models.py rename to openedx_tagging/core/tagging/import_export/import_plan.py index c51bf98e8..1e07815e2 100644 --- a/openedx_tagging/core/tagging/import_export/models.py +++ b/openedx_tagging/core/tagging/import_export/import_plan.py @@ -1,14 +1,11 @@ """ Model and functions to create a plan/execution with DSL actions. """ -from datetime import datetime -from enum import Enum from typing import List, Optional from django.db import transaction -from django.utils.translation import gettext_lazy as _ -from ..models import Taxonomy +from ..models import Taxonomy, TagImportTask from .actions import ( DeleteTag, ImportAction, @@ -16,97 +13,8 @@ WithoutChanges, available_actions, ) -from .exceptions import ImportActionError, TagParserError -from django.db import models +from .exceptions import ImportActionError -class TagImportTaskState(Enum): - LOADING_DATA = 'loading_data' - PLANNING = 'planning' - EXECUTING = 'executing' - SUCCESS = 'success' - ERROR = 'error' - - -class TagImportTask(models.Model): - """ - Stores the state, plan and logs of a tag import task - """ - id = models.BigAutoField(primary_key=True) - - taxonomy = models.ForeignKey( - "Taxonomy", - on_delete=models.CASCADE, - help_text=_("Taxonomy associated with this import"), - ) - - log = models.TextField( - null=True, - default=None, - help_text=_("Action execution logs") - ) - - status = models.CharField( - max_length=20, - choices=[(status, status.value) for status in TagImportTaskState], - help_text=_("Task status"), - ) - - creation_date = models.DateTimeField(auto_now_add=True) - - class Meta: - indexes = [ - models.Index(fields=['taxonomy', '-creation_date']), - ] - - @classmethod - def create(cls, taxonomy: Taxonomy): - task = cls( - taxonomy=taxonomy, - status=TagImportTaskState.LOADING_DATA.value, - log='', - ) - task.save() - return task - - def add_log(self, message: str, save=True): - timestamp = datetime.now().strftime('%Y-%m-%d %H:%M:%S') - log_message = f"[{timestamp}] {message}\n" - self.log += log_message - if save: - self.save() - - def log_exception(self, exception: Exception): - self.add_log(str(exception), save=False) - self.status = TagImportTaskState.ERROR.value - self.save() - - def log_parser_start(self): - self.add_log(_("Starting to load data from file")) - - def log_parser_end(self): - self.add_log(_("Load data finished")) - - def handle_parser_errors(self, errors: List[TagParserError]): - for error in errors: - self.add_log(f"{str(error)}", save=False) - self.status = TagImportTaskState.ERROR.value - self.save() - - def log_start_planning(self): - self.add_log(_("Starting planning the actions"), save=False) - self.status = TagImportTaskState.PLANNING.value - self.save() - - def log_end_planning(self, plan: "TagDSL"): - self.add_log(_("Plan finished")) - plan_str = plan.plan() - self.log += plan_str - self.save() - - def handle_plan_errors(self): - # Error are logged with plan - self.status = TagImportTaskState.ERROR.value - self.save() class TagDSL: diff --git a/openedx_tagging/core/tagging/import_export/parsers.py b/openedx_tagging/core/tagging/import_export/parsers.py index 2f0529bf6..b361cf443 100644 --- a/openedx_tagging/core/tagging/import_export/parsers.py +++ b/openedx_tagging/core/tagging/import_export/parsers.py @@ -9,7 +9,7 @@ from django.utils.translation import gettext_lazy as _ -from .models import TagDSL +from .import_plan import TagDSL from .exceptions import ( TagParserError, InvalidFormat, diff --git a/openedx_tagging/core/tagging/migrations/0006_auto_20230802_1631.py b/openedx_tagging/core/tagging/migrations/0006_auto_20230802_1631.py new file mode 100644 index 000000000..3549b3963 --- /dev/null +++ b/openedx_tagging/core/tagging/migrations/0006_auto_20230802_1631.py @@ -0,0 +1,29 @@ +# Generated by Django 3.2.19 on 2023-08-02 21:31 + +from django.db import migrations, models +import django.db.models.deletion +import openedx_tagging.core.tagging.models.import_export + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_tagging', '0005_language_taxonomy'), + ] + + operations = [ + migrations.CreateModel( + name='TagImportTask', + fields=[ + ('id', models.BigAutoField(primary_key=True, serialize=False)), + ('log', models.TextField(default=None, help_text='Action execution logs', null=True)), + ('status', models.CharField(choices=[(openedx_tagging.core.tagging.models.import_export.TagImportTaskState['LOADING_DATA'], 'loading_data'), (openedx_tagging.core.tagging.models.import_export.TagImportTaskState['PLANNING'], 'planning'), (openedx_tagging.core.tagging.models.import_export.TagImportTaskState['EXECUTING'], 'executing'), (openedx_tagging.core.tagging.models.import_export.TagImportTaskState['SUCCESS'], 'success'), (openedx_tagging.core.tagging.models.import_export.TagImportTaskState['ERROR'], 'error')], help_text='Task status', max_length=20)), + ('creation_date', models.DateTimeField(auto_now_add=True)), + ('taxonomy', models.ForeignKey(help_text='Taxonomy associated with this import', on_delete=django.db.models.deletion.CASCADE, to='oel_tagging.taxonomy')), + ], + ), + migrations.AddIndex( + model_name='tagimporttask', + index=models.Index(fields=['taxonomy', '-creation_date'], name='oel_tagging_taxonom_5e948c_idx'), + ), + ] diff --git a/openedx_tagging/core/tagging/models/__init__.py b/openedx_tagging/core/tagging/models/__init__.py index 90640ddfc..adb20d050 100644 --- a/openedx_tagging/core/tagging/models/__init__.py +++ b/openedx_tagging/core/tagging/models/__init__.py @@ -9,3 +9,4 @@ UserSystemDefinedTaxonomy, LanguageTaxonomy, ) +from .import_export import TagImportTask diff --git a/openedx_tagging/core/tagging/models/import_export.py b/openedx_tagging/core/tagging/models/import_export.py new file mode 100644 index 000000000..427cb38fd --- /dev/null +++ b/openedx_tagging/core/tagging/models/import_export.py @@ -0,0 +1,97 @@ +from datetime import datetime +from enum import Enum +from django.db import models + +from django.utils.translation import gettext_lazy as _ + +from .base import Taxonomy + + +class TagImportTaskState(Enum): + LOADING_DATA = 'loading_data' + PLANNING = 'planning' + EXECUTING = 'executing' + SUCCESS = 'success' + ERROR = 'error' + + +class TagImportTask(models.Model): + """ + Stores the state, plan and logs of a tag import task + """ + id = models.BigAutoField(primary_key=True) + + taxonomy = models.ForeignKey( + "Taxonomy", + on_delete=models.CASCADE, + help_text=_("Taxonomy associated with this import"), + ) + + log = models.TextField( + null=True, + default=None, + help_text=_("Action execution logs") + ) + + status = models.CharField( + max_length=20, + choices=[(status, status.value) for status in TagImportTaskState], + help_text=_("Task status"), + ) + + creation_date = models.DateTimeField(auto_now_add=True) + + class Meta: + indexes = [ + models.Index(fields=['taxonomy', '-creation_date']), + ] + + @classmethod + def create(cls, taxonomy: Taxonomy): + task = cls( + taxonomy=taxonomy, + status=TagImportTaskState.LOADING_DATA.value, + log='', + ) + task.save() + return task + + def add_log(self, message: str, save=True): + timestamp = datetime.now().strftime('%Y-%m-%d %H:%M:%S') + log_message = f"[{timestamp}] {message}\n" + self.log += log_message + if save: + self.save() + + def log_exception(self, exception: Exception): + self.add_log(str(exception), save=False) + self.status = TagImportTaskState.ERROR.value + self.save() + + def log_parser_start(self): + self.add_log(_("Starting to load data from file")) + + def log_parser_end(self): + self.add_log(_("Load data finished")) + + def handle_parser_errors(self, errors): + for error in errors: + self.add_log(f"{str(error)}", save=False) + self.status = TagImportTaskState.ERROR.value + self.save() + + def log_start_planning(self): + self.add_log(_("Starting planning the actions"), save=False) + self.status = TagImportTaskState.PLANNING.value + self.save() + + def log_end_planning(self, plan): + self.add_log(_("Plan finished")) + plan_str = plan.plan() + self.log += plan_str + self.save() + + def handle_plan_errors(self): + # Error are logged with plan + self.status = TagImportTaskState.ERROR.value + self.save() \ No newline at end of file diff --git a/tests/openedx_tagging/core/fixtures/tagging.yaml b/tests/openedx_tagging/core/fixtures/tagging.yaml index 98d27d138..3593a095c 100644 --- a/tests/openedx_tagging/core/fixtures/tagging.yaml +++ b/tests/openedx_tagging/core/fixtures/tagging.yaml @@ -176,7 +176,7 @@ - model: oel_tagging.tag pk: 26 fields: - taxonomy: 3 + taxonomy: 5 parent: null value: Tag 1 external_id: tag_1 diff --git a/tests/openedx_tagging/core/tagging/import_export/test_actions.py b/tests/openedx_tagging/core/tagging/import_export/test_actions.py index 98ebd51da..8b8aab6df 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_actions.py @@ -6,7 +6,7 @@ from django.test.testcases import TestCase from openedx_tagging.core.tagging.models import Taxonomy, Tag -from openedx_tagging.core.tagging.import_export.models import TagDSL +from openedx_tagging.core.tagging.import_export.import_plan import TagDSL from openedx_tagging.core.tagging.import_export.actions import ( ImportAction, CreateTag, @@ -134,7 +134,7 @@ def test_validate_parent(self, parent_id, expected): 'Tag 1', ( "Action error in 'import_action' (#100): " - "Duplicated tag value with tag (id=22)." + "Duplicated tag value with tag (id=26)." ) ), ( @@ -281,7 +281,7 @@ class TestUpdateParentTag(TestImportActionMixin, TestCase): "tag_4", "tag_3", ( - "Update the parent of tag (id=25) from parent " + "Update the parent of tag (id=29) from parent " "(external_id=tag_3) to parent (external_id=tag_3)." ) ), @@ -289,7 +289,7 @@ class TestUpdateParentTag(TestImportActionMixin, TestCase): "tag_3", "tag_2", ( - "Update the parent of tag (id=24) from empty parent " + "Update the parent of tag (id=28) from empty parent " "to parent (external_id=tag_2)." ) ), diff --git a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py b/tests/openedx_tagging/core/tagging/import_export/test_dsl.py index 4293bdce2..230bdba75 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_dsl.py @@ -5,7 +5,7 @@ from django.test.testcases import TestCase -from openedx_tagging.core.tagging.import_export.models import TagDSL, TagImportDSL +from openedx_tagging.core.tagging.import_export.import_plan import TagDSL, TagImportDSL from openedx_tagging.core.tagging.import_export.actions import CreateTag from openedx_tagging.core.tagging.import_export.exceptions import TagImportError from .test_actions import TestImportActionMixin @@ -252,15 +252,15 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "--------------------------------\n" "#1: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" "#2: Create a new tag with values (external_id=tag_31, value=Tag 32, parent_id=None).\n" - "#3: Rename tag value of tag (id=22) from 'Tag 1' to 'Tag 2'\n" - "#4: Update the parent of tag (id=25) from parent (external_id=tag_3) to parent (external_id=tag_100).\n" + "#3: Rename tag value of tag (id=26) from 'Tag 1' to 'Tag 2'\n" + "#4: Update the parent of tag (id=29) from parent (external_id=tag_3) to parent (external_id=tag_100).\n" "#5: Create a new tag with values (external_id=tag_33, value=Tag 32, parent_id=None).\n" - "#6: Update the parent of tag (id=23) from parent (external_id=tag_1) to parent (external_id=None).\n" - "#7: Rename tag value of tag (id=23) from 'Tag 2' to 'Tag 31'\n" + "#6: Update the parent of tag (id=27) from parent (external_id=tag_1) to parent (external_id=None).\n" + "#7: Rename tag value of tag (id=27) from 'Tag 2' to 'Tag 31'\n" "Output errors\n" "--------------------------------\n" "Conflict with 'create' (#2) and action #1: Duplicated external_id tag.\n" - "Action error in 'rename' (#3): Duplicated tag value with tag (id=23).\n" + "Action error in 'rename' (#3): Duplicated tag value with tag (id=27).\n" "Action error in 'update_parent' (#4): Unknown parent tag (tag_100). " "You need to add parent before the child in your file.\n" "Conflict with 'create' (#5) and action #2: Duplicated tag value.\n" @@ -296,9 +296,9 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "--------------------------------\n" "#1: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" "#2: Create a new tag with values (external_id=tag_32, value=Tag 32, parent_id=tag_1).\n" - "#3: Rename tag value of tag (id=23) from 'Tag 2' to 'Tag 2 v2'\n" - "#4: Update the parent of tag (id=25) from parent (external_id=tag_3) to parent (external_id=tag_1).\n" - "#5: Rename tag value of tag (id=25) from 'Tag 4' to 'Tag 4 v2'\n" + "#3: Rename tag value of tag (id=27) from 'Tag 2' to 'Tag 2 v2'\n" + "#4: Update the parent of tag (id=29) from parent (external_id=tag_3) to parent (external_id=tag_1).\n" + "#5: Rename tag value of tag (id=29) from 'Tag 4' to 'Tag 4 v2'\n" "#6: No changes needed for tag (external_id=tag_1)\n" ), ([ @@ -312,11 +312,11 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "Plan\n" "--------------------------------\n" "#1: No changes needed for tag (external_id=tag_4)\n" - "#2: Update the parent of tag (id=23) from parent (external_id=tag_1) to parent (external_id=None).\n" - "#3: Delete tag (id=22)\n" - "#4: Delete tag (id=23)\n" - "#5: Update the parent of tag (id=25) from parent (external_id=tag_3) to parent (external_id=None).\n" - "#6: Delete tag (id=24)\n" + "#2: Update the parent of tag (id=27) from parent (external_id=tag_1) to parent (external_id=None).\n" + "#3: Delete tag (id=26)\n" + "#4: Delete tag (id=27)\n" + "#5: Update the parent of tag (id=29) from parent (external_id=tag_3) to parent (external_id=None).\n" + "#6: Delete tag (id=28)\n" ) ) @ddt.unpack @@ -395,7 +395,6 @@ def test_execute(self, tags, replace): tag_external_ids = [] for tag_dsl in tags: # This checks any creation - print(tag_dsl.id) tag = self.taxonomy.tag_set.get(external_id=tag_dsl.id) # Checks any rename @@ -410,9 +409,6 @@ def test_execute(self, tags, replace): tag_external_ids.append(tag_dsl.id) - for action in self.dsl.actions: - assert action.success - if replace: # Checks deletions checking that exists the updated tags external_ids = list(self.taxonomy.tag_set.values_list("external_id", flat=True)) From 2e5266d484eb5504a16105fc9ff0e375ebae53f4 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 3 Aug 2023 12:35:52 -0500 Subject: [PATCH 10/20] test: Testing import task creation, states and logs --- .../core/tagging/import_export/__init__.py | 1 + .../core/tagging/import_export/api.py | 63 +++++--- .../core/tagging/import_export/import_plan.py | 54 +++++-- .../core/tagging/import_export/tasks.py | 4 +- .../migrations/0006_auto_20230802_1631.py | 72 +++++++-- .../core/tagging/models/__init__.py | 5 +- .../core/tagging/models/import_export.py | 50 +++--- .../core/tagging/import_export/test_api.py | 148 ++++++++++++++++++ .../core/tagging/import_export/test_dsl.py | 57 ++----- 9 files changed, 339 insertions(+), 115 deletions(-) create mode 100644 tests/openedx_tagging/core/tagging/import_export/test_api.py diff --git a/openedx_tagging/core/tagging/import_export/__init__.py b/openedx_tagging/core/tagging/import_export/__init__.py index e69de29bb..55bcdaebd 100644 --- a/openedx_tagging/core/tagging/import_export/__init__.py +++ b/openedx_tagging/core/tagging/import_export/__init__.py @@ -0,0 +1 @@ +from .parsers import ParserFormat diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py index 31facee97..2d5cdc42f 100644 --- a/openedx_tagging/core/tagging/import_export/api.py +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -2,9 +2,9 @@ from django.utils.translation import gettext_lazy as _ -from ..models import Taxonomy +from ..models import Taxonomy, TagImportTask, TagImportTaskState from .parsers import get_parser, ParserFormat -from .import_plan import TagImportDSL, TagImportTask, TagImportTaskState +from .import_plan import TagImportDSL, TagImportTask def import_tags( @@ -15,11 +15,13 @@ def import_tags( ) -> bool: # Checks that exists only one tag if not _check_unique_import_task(taxonomy): - raise ValueError(_( - "There is an import task running. " - "Only one task per taxonomy can be created at a time." - )) - + raise ValueError( + _( + "There is an import task running. " + "Only one task per taxonomy can be created at a time." + ) + ) + # Creating import task task = TagImportTask.create(taxonomy) @@ -28,42 +30,57 @@ def import_tags( task.log_parser_start() parser = get_parser(parser_format) tags, errors = parser.parse_import(file) - task.log_parser_end() # Check if there are errors in the parse if errors: task.handle_parser_errors(errors) return False + task.log_parser_end() + # Generate actions task.log_start_planning() - dsl = TagImportDSL() - dsl.generate_actions(tags, taxonomy, replace) - task.log_end_planning(dsl) + dsl = TagImportDSL(taxonomy) + dsl.generate_actions(tags, replace) + task.log_plan(dsl) if dsl.errors: task.handle_plan_errors() return False - return dsl.execute() + task.log_start_execute() + dsl.execute(task) + task.end_success() + return True except Exception as exception: # Log any exception task.log_exception(exception) return False +def get_last_import_status(taxonomy: Taxonomy) -> TagImportTaskState: + task = _get_last_tags(taxonomy) + return task.status + + +def get_last_import_log(taxonomy: Taxonomy) -> str: + task = _get_last_tags(taxonomy) + return task.log + + def _check_unique_import_task(taxonomy: Taxonomy) -> bool: - last_task = ( - TagImportTask.objects - .filter(taxonomy=taxonomy) - .order_by('-creation_date') - .first() - ) + last_task = _get_last_tags(taxonomy) if not last_task: - return False - return (last_task.status != TagImportTaskState.SUCCESS - and last_task.status != TagImportTaskState.ERROR) + return True + return ( + last_task.status == TagImportTaskState.SUCCESS.value + or last_task.status == TagImportTaskState.ERROR.value + ) -def check_import_staus(): - pass +def _get_last_tags(taxonomy: Taxonomy) -> TagImportTask: + return ( + TagImportTask.objects.filter(taxonomy=taxonomy) + .order_by("-creation_date") + .first() + ) diff --git a/openedx_tagging/core/tagging/import_export/import_plan.py b/openedx_tagging/core/tagging/import_export/import_plan.py index 1e07815e2..afec77061 100644 --- a/openedx_tagging/core/tagging/import_export/import_plan.py +++ b/openedx_tagging/core/tagging/import_export/import_plan.py @@ -16,7 +16,6 @@ from .exceptions import ImportActionError - class TagDSL: """ Tag representation on the import DSL @@ -87,21 +86,40 @@ def _build_action(self, action_cls, tag: TagDSL): # Index the actions for search self.indexed_actions[action.name].append(action) + def _search_parent_update( + self, + child_external_id, + parent_external_id, + ): + """ + Checks if there is a parent update in a child + """ + for action in self.indexed_actions["update_parent"]: + if ( + child_external_id == action.tag.id + and parent_external_id != action.tag.parent_id + ): + return True + + return False + def _build_delete_actions(self, tags: dict): """ Adds delete actions for `tags` """ for tag in tags.values(): for child in tag.children.all(): - # child parent to avoid delete childs - self._build_action( - UpdateParentTag, - TagDSL( - id=child.external_id, - value=child.value, - parent_id=None, - ), - ) + # Verify if there is not a parent update before + if not self._search_parent_update(child.external_id, tag.external_id): + # Change parent to avoid delete childs + self._build_action( + UpdateParentTag, + TagDSL( + id=child.external_id, + value=child.value, + parent_id=None, + ), + ) # Delete action self._build_action( @@ -150,7 +168,7 @@ def generate_actions( # If it doesn't find an action, a "without changes" is added self._build_action(WithoutChanges, tag) - if replace: + if replace and tag.id in tags_for_delete: tags_for_delete.pop(tag.id) if replace: @@ -161,12 +179,15 @@ def plan(self) -> str: """ Returns an string with the plan and errors """ - result = "Plan\n" "--------------------------------\n" + result = ( + f"Import plan for {self.taxonomy.name}\n" + "--------------------------------\n" + ) for action in self.actions: result += f"#{action.index}: {str(action)}\n" if self.errors: - result += "Output errors\n" "--------------------------------\n" + result += "\nOutput errors\n" "--------------------------------\n" for error in self.errors: result += f"{str(error)}\n" @@ -175,11 +196,10 @@ def plan(self) -> str: @transaction.atomic() def execute(self, task: TagImportTask = None): if self.errors: - return False + return for action in self.actions: if task: - task.add_log(f"{str(action)} [Started]") + task.add_log(f"#{action.index}: {str(action)} [Started]") action.execute() if task: - task.add_log(f"{str(action)} [Success]") - return True + task.add_log("Success") diff --git a/openedx_tagging/core/tagging/import_export/tasks.py b/openedx_tagging/core/tagging/import_export/tasks.py index f04cb0a30..21260183b 100644 --- a/openedx_tagging/core/tagging/import_export/tasks.py +++ b/openedx_tagging/core/tagging/import_export/tasks.py @@ -7,7 +7,7 @@ @shared_task -def import_tags_async( +def import_tags_task( taxonomy: Taxonomy, file: BytesIO, parser_format: ParserFormat, @@ -18,4 +18,4 @@ def import_tags_async( file, parser_format, replace, - ) \ No newline at end of file + ) diff --git a/openedx_tagging/core/tagging/migrations/0006_auto_20230802_1631.py b/openedx_tagging/core/tagging/migrations/0006_auto_20230802_1631.py index 3549b3963..b608f27bf 100644 --- a/openedx_tagging/core/tagging/migrations/0006_auto_20230802_1631.py +++ b/openedx_tagging/core/tagging/migrations/0006_auto_20230802_1631.py @@ -6,24 +6,76 @@ class Migration(migrations.Migration): - dependencies = [ - ('oel_tagging', '0005_language_taxonomy'), + ("oel_tagging", "0005_language_taxonomy"), ] operations = [ migrations.CreateModel( - name='TagImportTask', + name="TagImportTask", fields=[ - ('id', models.BigAutoField(primary_key=True, serialize=False)), - ('log', models.TextField(default=None, help_text='Action execution logs', null=True)), - ('status', models.CharField(choices=[(openedx_tagging.core.tagging.models.import_export.TagImportTaskState['LOADING_DATA'], 'loading_data'), (openedx_tagging.core.tagging.models.import_export.TagImportTaskState['PLANNING'], 'planning'), (openedx_tagging.core.tagging.models.import_export.TagImportTaskState['EXECUTING'], 'executing'), (openedx_tagging.core.tagging.models.import_export.TagImportTaskState['SUCCESS'], 'success'), (openedx_tagging.core.tagging.models.import_export.TagImportTaskState['ERROR'], 'error')], help_text='Task status', max_length=20)), - ('creation_date', models.DateTimeField(auto_now_add=True)), - ('taxonomy', models.ForeignKey(help_text='Taxonomy associated with this import', on_delete=django.db.models.deletion.CASCADE, to='oel_tagging.taxonomy')), + ("id", models.BigAutoField(primary_key=True, serialize=False)), + ( + "log", + models.TextField( + default=None, help_text="Action execution logs", null=True + ), + ), + ( + "status", + models.CharField( + choices=[ + ( + openedx_tagging.core.tagging.models.import_export.TagImportTaskState[ + "LOADING_DATA" + ], + "loading_data", + ), + ( + openedx_tagging.core.tagging.models.import_export.TagImportTaskState[ + "PLANNING" + ], + "planning", + ), + ( + openedx_tagging.core.tagging.models.import_export.TagImportTaskState[ + "EXECUTING" + ], + "executing", + ), + ( + openedx_tagging.core.tagging.models.import_export.TagImportTaskState[ + "SUCCESS" + ], + "success", + ), + ( + openedx_tagging.core.tagging.models.import_export.TagImportTaskState[ + "ERROR" + ], + "error", + ), + ], + help_text="Task status", + max_length=20, + ), + ), + ("creation_date", models.DateTimeField(auto_now_add=True)), + ( + "taxonomy", + models.ForeignKey( + help_text="Taxonomy associated with this import", + on_delete=django.db.models.deletion.CASCADE, + to="oel_tagging.taxonomy", + ), + ), ], ), migrations.AddIndex( - model_name='tagimporttask', - index=models.Index(fields=['taxonomy', '-creation_date'], name='oel_tagging_taxonom_5e948c_idx'), + model_name="tagimporttask", + index=models.Index( + fields=["taxonomy", "-creation_date"], + name="oel_tagging_taxonom_5e948c_idx", + ), ), ] diff --git a/openedx_tagging/core/tagging/models/__init__.py b/openedx_tagging/core/tagging/models/__init__.py index adb20d050..295e38bdb 100644 --- a/openedx_tagging/core/tagging/models/__init__.py +++ b/openedx_tagging/core/tagging/models/__init__.py @@ -9,4 +9,7 @@ UserSystemDefinedTaxonomy, LanguageTaxonomy, ) -from .import_export import TagImportTask +from .import_export import ( + TagImportTask, + TagImportTaskState, +) diff --git a/openedx_tagging/core/tagging/models/import_export.py b/openedx_tagging/core/tagging/models/import_export.py index 427cb38fd..1deaf9c9d 100644 --- a/openedx_tagging/core/tagging/models/import_export.py +++ b/openedx_tagging/core/tagging/models/import_export.py @@ -8,17 +8,18 @@ class TagImportTaskState(Enum): - LOADING_DATA = 'loading_data' - PLANNING = 'planning' - EXECUTING = 'executing' - SUCCESS = 'success' - ERROR = 'error' + LOADING_DATA = "loading_data" + PLANNING = "planning" + EXECUTING = "executing" + SUCCESS = "success" + ERROR = "error" class TagImportTask(models.Model): """ Stores the state, plan and logs of a tag import task """ + id = models.BigAutoField(primary_key=True) taxonomy = models.ForeignKey( @@ -28,13 +29,11 @@ class TagImportTask(models.Model): ) log = models.TextField( - null=True, - default=None, - help_text=_("Action execution logs") + null=True, default=None, help_text=_("Action execution logs") ) status = models.CharField( - max_length=20, + max_length=20, choices=[(status, status.value) for status in TagImportTaskState], help_text=_("Task status"), ) @@ -43,7 +42,7 @@ class TagImportTask(models.Model): class Meta: indexes = [ - models.Index(fields=['taxonomy', '-creation_date']), + models.Index(fields=["taxonomy", "-creation_date"]), ] @classmethod @@ -51,20 +50,21 @@ def create(cls, taxonomy: Taxonomy): task = cls( taxonomy=taxonomy, status=TagImportTaskState.LOADING_DATA.value, - log='', + log="", ) + task.add_log(_("Import task created"), save=False) task.save() return task - + def add_log(self, message: str, save=True): - timestamp = datetime.now().strftime('%Y-%m-%d %H:%M:%S') + timestamp = datetime.now().strftime("%Y-%m-%d %H:%M:%S") log_message = f"[{timestamp}] {message}\n" self.log += log_message if save: self.save() def log_exception(self, exception: Exception): - self.add_log(str(exception), save=False) + self.add_log(repr(exception), save=False) self.status = TagImportTaskState.ERROR.value self.save() @@ -81,17 +81,27 @@ def handle_parser_errors(self, errors): self.save() def log_start_planning(self): - self.add_log(_("Starting planning the actions"), save=False) + self.add_log(_("Starting plan actions"), save=False) self.status = TagImportTaskState.PLANNING.value self.save() - - def log_end_planning(self, plan): + + def log_plan(self, plan): self.add_log(_("Plan finished")) plan_str = plan.plan() - self.log += plan_str + self.log += f"\n{plan_str}\n" self.save() - + def handle_plan_errors(self): # Error are logged with plan self.status = TagImportTaskState.ERROR.value - self.save() \ No newline at end of file + self.save() + + def log_start_execute(self): + self.add_log(_("Starting execute actions"), save=False) + self.status = TagImportTaskState.EXECUTING.value + self.save() + + def end_success(self): + self.add_log(_("Execution finished"), save=False) + self.status = TagImportTaskState.SUCCESS.value + self.save() diff --git a/tests/openedx_tagging/core/tagging/import_export/test_api.py b/tests/openedx_tagging/core/tagging/import_export/test_api.py new file mode 100644 index 000000000..0f4649954 --- /dev/null +++ b/tests/openedx_tagging/core/tagging/import_export/test_api.py @@ -0,0 +1,148 @@ +""" +Test for import/export API +""" +import json +from io import BytesIO + +from django.test.testcases import TestCase + +from openedx_tagging.core.tagging.models import TagImportTask, TagImportTaskState +from openedx_tagging.core.tagging.import_export import ParserFormat +import openedx_tagging.core.tagging.import_export.api as import_export_api + +from .test_actions import TestImportActionMixin + + +class TestImportApi(TestImportActionMixin, TestCase): + """ + Test import API functions + """ + + def setUp(self): + self.tags = [ + {"id": "tag_31", "value": "Tag 31"}, + {"id": "tag_32", "value": "Tag 32"}, + {"id": "tag_33", "value": "Tag 33", "parent_id": "tag_31"}, + {"id": "tag_1", "value": "Tag 1 V2"}, + {"id": "tag_4", "value": "Tag 4", "parent_id": "tag_32"}, + ] + json_data = {"tags": self.tags} + self.file = BytesIO(json.dumps(json_data).encode()) + + json_data = {"invalid": [ + {"id": "tag_1", "name": "Tag 1"}, + ]} + self.invalid_parser_file = BytesIO(json.dumps(json_data).encode()) + json_data = {"tags": [ + {'id': 'tag_31', 'value': 'Tag 31',}, + {'id': 'tag_31', 'value': 'Tag 32',}, + ]} + self.invalid_plan_file = BytesIO(json.dumps(json_data).encode()) + + self.parser_format = ParserFormat.JSON + return super().setUp() + + def test_check_status(self): + TagImportTask.create(self.taxonomy) + status = import_export_api.get_last_import_status(self.taxonomy) + assert status == TagImportTaskState.LOADING_DATA.value + + def test_check_log(self): + TagImportTask.create(self.taxonomy) + log = import_export_api.get_last_import_log(self.taxonomy) + assert "Import task created" in log + + def test_invalid_import_tags(self): + TagImportTask.create(self.taxonomy) + with self.assertRaises(ValueError): + # Raise error if there is a current in progress task + import_export_api.import_tags( + self.taxonomy, + self.file, + self.parser_format, + ) + + def test_with_python_error(self): + self.file.close() + assert not import_export_api.import_tags( + self.taxonomy, + self.file, + self.parser_format, + ) + status = import_export_api.get_last_import_status(self.taxonomy) + log = import_export_api.get_last_import_log(self.taxonomy) + assert status == TagImportTaskState.ERROR.value + assert "ValueError('I/O operation on closed file.')" in log + + def test_with_parser_error(self): + assert not import_export_api.import_tags( + self.taxonomy, + self.invalid_parser_file, + self.parser_format, + ) + status = import_export_api.get_last_import_status(self.taxonomy) + log = import_export_api.get_last_import_log(self.taxonomy) + assert status == TagImportTaskState.ERROR.value + assert "Starting to load data from file" in log + assert "Invalid '.json' format" in log + + def test_with_plan_errors(self): + assert not import_export_api.import_tags( + self.taxonomy, + self.invalid_plan_file, + self.parser_format, + ) + status = import_export_api.get_last_import_status(self.taxonomy) + log = import_export_api.get_last_import_log(self.taxonomy) + assert status == TagImportTaskState.ERROR.value + assert "Starting to load data from file" in log + assert "Load data finished" in log + assert "Starting plan actions" in log + assert "Plan finished" in log + assert "Conflict with 'create'" in log + + def test_valid(self): + assert import_export_api.import_tags( + self.taxonomy, + self.file, + self.parser_format, + replace=True, + ) + status = import_export_api.get_last_import_status(self.taxonomy) + log = import_export_api.get_last_import_log(self.taxonomy) + assert status == TagImportTaskState.SUCCESS.value + assert "Starting to load data from file" in log + assert "Load data finished" in log + assert "Starting plan actions" in log + assert "Plan finished" in log + assert "Starting execute actions" in log + assert "Execution finished" in log + + def test_start_task_after_error(self): + assert not import_export_api.import_tags( + self.taxonomy, + self.invalid_parser_file, + self.parser_format, + ) + assert import_export_api.import_tags( + self.taxonomy, + self.file, + self.parser_format, + ) + + def test_start_task_after_success(self): + assert import_export_api.import_tags( + self.taxonomy, + self.file, + self.parser_format, + ) + + # Opening again the file + json_data = {"tags": self.tags} + self.file = BytesIO(json.dumps(json_data).encode()) + + assert import_export_api.import_tags( + self.taxonomy, + self.file, + self.parser_format, + ) diff --git a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py b/tests/openedx_tagging/core/tagging/import_export/test_dsl.py index 230bdba75..41ce29675 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_dsl.py @@ -29,8 +29,8 @@ def test_tag_import_error(self): @ddt.data( - ('tag_10', 1), - ('tag_30', 0), + ('tag_10', 1), # Test invalid + ('tag_30', 0), # Test valid ) @ddt.unpack def test_build_action(self, tag_id, errors_expected): @@ -131,7 +131,7 @@ def test_build_delete_actions(self): 'name': 'without_changes', 'id': 'tag_1' }, - ]), + ]), # Test valid actions ([ { 'id': 'tag_31', @@ -170,7 +170,7 @@ def test_build_delete_actions(self): 'name': 'update_parent', 'id': 'tag_4', } - ]), + ]), # Test with errors in actions ([ { 'id': 'tag_4', @@ -205,7 +205,7 @@ def test_build_delete_actions(self): 'name': 'delete', 'id': 'tag_3', }, - ]) + ]) # Test with deletes (replace=True) ) @ddt.unpack def test_generate_actions(self, tags, replace, expected_errors, expected_actions): @@ -248,7 +248,7 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions }, ], False, - "Plan\n" + "Import plan for Import Taxonomy Test\n" "--------------------------------\n" "#1: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" "#2: Create a new tag with values (external_id=tag_31, value=Tag 32, parent_id=None).\n" @@ -257,7 +257,7 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "#5: Create a new tag with values (external_id=tag_33, value=Tag 32, parent_id=None).\n" "#6: Update the parent of tag (id=27) from parent (external_id=tag_1) to parent (external_id=None).\n" "#7: Rename tag value of tag (id=27) from 'Tag 2' to 'Tag 31'\n" - "Output errors\n" + "\nOutput errors\n" "--------------------------------\n" "Conflict with 'create' (#2) and action #1: Duplicated external_id tag.\n" "Action error in 'rename' (#3): Duplicated tag value with tag (id=27).\n" @@ -265,7 +265,7 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "You need to add parent before the child in your file.\n" "Conflict with 'create' (#5) and action #2: Duplicated tag value.\n" "Conflict with 'rename' (#7) and action #1: Duplicated tag value.\n" - ), + ), # Testing plan with errors ([ { 'id': 'tag_31', @@ -292,7 +292,7 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions }, ], False, - "Plan\n" + "Import plan for Import Taxonomy Test\n" "--------------------------------\n" "#1: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" "#2: Create a new tag with values (external_id=tag_32, value=Tag 32, parent_id=tag_1).\n" @@ -300,7 +300,7 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "#4: Update the parent of tag (id=29) from parent (external_id=tag_3) to parent (external_id=tag_1).\n" "#5: Rename tag value of tag (id=29) from 'Tag 4' to 'Tag 4 v2'\n" "#6: No changes needed for tag (external_id=tag_1)\n" - ), + ), # Testing valid plan ([ { 'id': 'tag_4', @@ -309,7 +309,7 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions }, ], True, - "Plan\n" + "Import plan for Import Taxonomy Test\n" "--------------------------------\n" "#1: No changes needed for tag (external_id=tag_4)\n" "#2: Update the parent of tag (id=27) from parent (external_id=tag_1) to parent (external_id=None).\n" @@ -317,7 +317,7 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "#4: Delete tag (id=27)\n" "#5: Update the parent of tag (id=29) from parent (external_id=tag_3) to parent (external_id=None).\n" "#6: Delete tag (id=28)\n" - ) + ) # Testing deletes (replace=True) ) @ddt.unpack def test_plan(self, tags, replace, expected): @@ -327,33 +327,6 @@ def test_plan(self, tags, replace, expected): assert plan == expected @ddt.data( - ([ - { - 'id': 'tag_31', - 'value': 'Tag 31', - }, - { - 'id': 'tag_32', - 'value': 'Tag 32', - }, - { - 'id': 'tag_1', - 'value': 'Tag 1 V2', - }, - { - 'id': 'tag_4', - 'value': 'Tag 4', - 'parent_id': 'tag_32', - }, - { - 'id': 'tag_33', - 'value': 'Tag 33', - }, - { - 'id': 'tag_2', - 'value': 'Tag 2 V2', - }, - ], False), ([ { 'id': 'tag_31', @@ -378,20 +351,20 @@ def test_plan(self, tags, replace, expected): 'id': 'tag_1', 'value': 'Tag 1', }, - ], False), + ], False), # Testing all actions ([ { 'id': 'tag_4', 'value': 'Tag 4', 'parent_id': 'tag_3', }, - ], True), + ], True), # Testing deletes (replace=True) ) @ddt.unpack def test_execute(self, tags, replace): tags = [TagDSL(**tag) for tag in tags] self.dsl.generate_actions(tags=tags, replace=replace) - assert self.dsl.execute() + self.dsl.execute() tag_external_ids = [] for tag_dsl in tags: # This checks any creation From 4dc7c6f8d0dc11d8cf4670c814e2195254b6dcd4 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 3 Aug 2023 15:05:32 -0500 Subject: [PATCH 11/20] feat: export functions --- .../core/tagging/import_export/api.py | 26 ++++++ .../core/tagging/import_export/parsers.py | 87 ++++++++++++++++++- .../migrations/0005_language_taxonomy.py | 2 +- .../core/tagging/import_export/mixins.py | 16 ++++ .../tagging/import_export/test_actions.py | 10 +-- .../core/tagging/import_export/test_api.py | 79 ++++++++++++++++- .../tagging/import_export/test_parsers.py | 54 +++++++++++- 7 files changed, 259 insertions(+), 15 deletions(-) create mode 100644 tests/openedx_tagging/core/tagging/import_export/mixins.py diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py index 2d5cdc42f..76bfb7c44 100644 --- a/openedx_tagging/core/tagging/import_export/api.py +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -1,3 +1,5 @@ +""" +""" from io import BytesIO from django.utils.translation import gettext_lazy as _ @@ -13,6 +15,8 @@ def import_tags( parser_format: ParserFormat, replace=False, ) -> bool: + _import_export_validations(taxonomy) + # Checks that exists only one tag if not _check_unique_import_task(taxonomy): raise ValueError( @@ -68,6 +72,12 @@ def get_last_import_log(taxonomy: Taxonomy) -> str: return task.log +def export_tags(taxonomy: Taxonomy, output_format: ParserFormat) -> str: + _import_export_validations(taxonomy) + parser = get_parser(output_format) + return parser.export(taxonomy) + + def _check_unique_import_task(taxonomy: Taxonomy) -> bool: last_task = _get_last_tags(taxonomy) if not last_task: @@ -84,3 +94,19 @@ def _get_last_tags(taxonomy: Taxonomy) -> TagImportTask: .order_by("-creation_date") .first() ) + + +def _import_export_validations(taxonomy: Taxonomy): + taxonomy = taxonomy.cast() + if taxonomy.allow_free_text: + raise ValueError( + _( + f"Invalid taxonomy ({taxonomy.id}): You cannot import/export a free-form taxonomy." + ) + ) + if taxonomy.system_defined: + raise ValueError( + _( + f"Invalid taxonomy ({taxonomy.id}): You cannot import/export a system-defined taxonomy." + ) + ) diff --git a/openedx_tagging/core/tagging/import_export/parsers.py b/openedx_tagging/core/tagging/import_export/parsers.py index b361cf443..9fc18cbf7 100644 --- a/openedx_tagging/core/tagging/import_export/parsers.py +++ b/openedx_tagging/core/tagging/import_export/parsers.py @@ -4,7 +4,7 @@ import csv import json from enum import Enum -from io import BytesIO, TextIOWrapper +from io import BytesIO, TextIOWrapper, StringIO from typing import List, Tuple from django.utils.translation import gettext_lazy as _ @@ -17,6 +17,8 @@ EmptyJSONField, EmptyCSVField, ) +from ..models import Taxonomy +from ..api import get_tags class ParserFormat(Enum): @@ -51,8 +53,10 @@ class Parser: @classmethod def parse_import(cls, file: BytesIO) -> Tuple[List[TagDSL], List[TagParserError]]: """ + Parse tags in file an returns tags ready for use in TagImportPlan + Top function that calls `_load_data` and `_parse_tags`. - Handle the errors returned both functions + Handle errors returned by both functions. """ try: tags_data, load_errors = cls._load_data(file) @@ -65,6 +69,15 @@ def parse_import(cls, file: BytesIO) -> Tuple[List[TagDSL], List[TagParserError] return cls._parse_tags(tags_data) + @classmethod + def export(cls, taxonomy: Taxonomy) -> str: + """ + Returns all tags in taxonomy. + The output file can be used to recreate the taxonomy with `parse_import` + """ + tags = cls._load_tags_for_export(taxonomy) + return cls._export_data(tags, taxonomy) + @classmethod def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[TagParserError]]: """ @@ -76,6 +89,18 @@ def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[TagParserError]]: """ raise NotImplementedError + @classmethod + def _export_data(cls, tags: List[dict], taxonomy: Taxonomy) -> str: + """ + Each parser implements this function according to its format. + Returns a string with tags data in the parser format. + Can use `taxonomy` to export taxonomy metadata. + + It must be implemented in such a way that the output of + this function works with _load_data + """ + raise NotImplementedError + @classmethod def _parse_tags(cls, tags_data: dict) -> Tuple[List[TagDSL], List[TagParserError]]: """ @@ -129,6 +154,30 @@ def _parse_tags(cls, tags_data: dict) -> Tuple[List[TagDSL], List[TagParserError return tags, errors + @classmethod + def _load_tags_for_export(cls, taxonomy: Taxonomy) -> List[dict]: + """ + Returns a list of taxonomy's tags in the form of a dictionary + with required and optional fields + + The 'action' field is not added to the result because the output + is seen as a creation of all the tags. + + The tags are ordered by hierarchy, first, parents and then children. + `get_tags` is in charge of returning this in a hierarchical way. + """ + tags = get_tags(taxonomy) + result = [] + for tag in tags: + result_tag = { + "id": tag.external_id or tag.id, + "value": tag.value, + } + if tag.parent: + result_tag["parent_id"] = tag.parent.external_id or tag.parent.id + result.append(result_tag) + return result + class JSONParser(Parser): """ @@ -172,6 +221,18 @@ def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[TagParserError]]: tags_data = tags_data.get("tags") return tags_data, [] + @classmethod + def _export_data(cls, tags: List[dict], taxonomy: Taxonomy) -> str: + """ + Export tags and taxonomy metadata in JSON format + """ + json_result = { + "name": taxonomy.name, + "description": taxonomy.description, + "tags": tags, + } + return json.dumps(json_result) + class CSVParser(Parser): """ @@ -203,6 +264,28 @@ def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[TagParserError]]: return None, errors return list(csv_reader), [] + @classmethod + def _export_data(cls, tags: List[dict], taxonomy: Taxonomy) -> str: + """ + Export tags in CSV format + + The 'action' field is not added to the result because the output + is seen as a creation of all the tags. + """ + fields = cls.required_fields + cls.optional_fields + if "action" in fields: # pragma: no cover + fields.remove("action") + + with StringIO() as csv_buffer: + csv_writer = csv.DictWriter(csv_buffer, fieldnames=fields) + csv_writer.writeheader() + + for tag in tags: + csv_writer.writerow(tag) + + csv_string = csv_buffer.getvalue() + return csv_string + @classmethod def _veify_header(cls, header_fields: List[str]) -> List[TagParserError]: """ diff --git a/openedx_tagging/core/tagging/migrations/0005_language_taxonomy.py b/openedx_tagging/core/tagging/migrations/0005_language_taxonomy.py index 48bab478a..a6d4fd0cf 100644 --- a/openedx_tagging/core/tagging/migrations/0005_language_taxonomy.py +++ b/openedx_tagging/core/tagging/migrations/0005_language_taxonomy.py @@ -11,7 +11,7 @@ def load_language_taxonomy(apps, schema_editor): call_command("loaddata", "--app=oel_tagging", "language_taxonomy.yaml") -def revert(apps, schema_editor): +def revert(apps, schema_editor): # pragma: no cover """ Deletes language taxonomy an tags """ diff --git a/tests/openedx_tagging/core/tagging/import_export/mixins.py b/tests/openedx_tagging/core/tagging/import_export/mixins.py new file mode 100644 index 000000000..045d3c180 --- /dev/null +++ b/tests/openedx_tagging/core/tagging/import_export/mixins.py @@ -0,0 +1,16 @@ +""" +Mixins for ImportExport tests +""" +from openedx_tagging.core.tagging.models import Taxonomy + + +class TestImportExportMixin: + """ + Mixin that loads the base data for import/export tests + """ + + fixtures = ["tests/openedx_tagging/core/fixtures/tagging.yaml"] + + def setUp(self): + self.taxonomy = Taxonomy.objects.get(name="Import Taxonomy Test") + return super().setUp() diff --git a/tests/openedx_tagging/core/tagging/import_export/test_actions.py b/tests/openedx_tagging/core/tagging/import_export/test_actions.py index 8b8aab6df..23d99a674 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_actions.py @@ -5,7 +5,7 @@ from django.test.testcases import TestCase -from openedx_tagging.core.tagging.models import Taxonomy, Tag +from openedx_tagging.core.tagging.models import Tag from openedx_tagging.core.tagging.import_export.import_plan import TagDSL from openedx_tagging.core.tagging.import_export.actions import ( ImportAction, @@ -15,17 +15,15 @@ DeleteTag, WithoutChanges, ) +from .mixins import TestImportExportMixin -class TestImportActionMixin: +class TestImportActionMixin(TestImportExportMixin): """ Mixin for import action tests """ - - fixtures = ["tests/openedx_tagging/core/fixtures/tagging.yaml"] - def setUp(self): - self.taxonomy = Taxonomy.objects.get(name="Import Taxonomy Test") + super().setUp() self.indexed_actions = { 'create': [ CreateTag( diff --git a/tests/openedx_tagging/core/tagging/import_export/test_api.py b/tests/openedx_tagging/core/tagging/import_export/test_api.py index 0f4649954..6db167689 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_api.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_api.py @@ -6,16 +6,21 @@ from django.test.testcases import TestCase -from openedx_tagging.core.tagging.models import TagImportTask, TagImportTaskState +from openedx_tagging.core.tagging.models import ( + TagImportTask, + TagImportTaskState, + Taxonomy, + LanguageTaxonomy, +) from openedx_tagging.core.tagging.import_export import ParserFormat import openedx_tagging.core.tagging.import_export.api as import_export_api -from .test_actions import TestImportActionMixin +from .mixins import TestImportExportMixin -class TestImportApi(TestImportActionMixin, TestCase): +class TestImportExportApi(TestImportExportMixin, TestCase): """ - Test import API functions + Test import/export API functions """ def setUp(self): @@ -40,6 +45,16 @@ def setUp(self): self.invalid_plan_file = BytesIO(json.dumps(json_data).encode()) self.parser_format = ParserFormat.JSON + + self.open_taxonomy = Taxonomy( + name="Open taxonomy", + allow_free_text=True + ) + self.system_taxonomy = Taxonomy( + name="System taxonomy", + ) + self.system_taxonomy.taxonomy_class = LanguageTaxonomy + self.system_taxonomy = self.system_taxonomy.cast() return super().setUp() def test_check_status(self): @@ -62,6 +77,23 @@ def test_invalid_import_tags(self): self.parser_format, ) + def test_import_export_validations(self): + # Check that import is invalid with open taxonomy + with self.assertRaises(ValueError): + import_export_api.import_tags( + self.open_taxonomy, + self.file, + self.parser_format, + ) + + # Check that import is invalid with system taxonomy + with self.assertRaises(ValueError): + import_export_api.import_tags( + self.system_taxonomy, + self.file, + self.parser_format, + ) + def test_with_python_error(self): self.file.close() assert not import_export_api.import_tags( @@ -146,3 +178,42 @@ def test_start_task_after_success(self): self.file, self.parser_format, ) + + def test_export_validations(self): + # Check that import is invalid with open taxonomy + with self.assertRaises(ValueError): + import_export_api.export_tags( + self.open_taxonomy, + self.parser_format, + ) + + # Check that import is invalid with system taxonomy + with self.assertRaises(ValueError): + import_export_api.export_tags( + self.system_taxonomy, + self.parser_format, + ) + + def test_import_with_export_output(self): + for parser_format in ParserFormat: + output = import_export_api.export_tags( + self.taxonomy, + parser_format, + ) + file = BytesIO(output.encode()) + new_taxonomy = Taxonomy(name="New taxonomy") + new_taxonomy.save() + assert import_export_api.import_tags( + new_taxonomy, + file, + parser_format, + ) + old_tags = self.taxonomy.tag_set.all() + assert len(old_tags) == new_taxonomy.tag_set.count() + + for tag in old_tags: + new_tag = new_taxonomy.tag_set.get(external_id=tag.external_id) + assert new_tag.value == tag.value + if tag.parent: + assert tag.parent.external_id == new_tag.parent.external_id + \ No newline at end of file diff --git a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py index ea5d30e8f..e09409b07 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py @@ -17,6 +17,8 @@ from openedx_tagging.core.tagging.import_export.exceptions import ( TagParserError, ) +from openedx_tagging.core.tagging.models import Taxonomy +from .mixins import TestImportExportMixin class TestParser(TestCase): @@ -34,8 +36,11 @@ def test_parser_not_found(self): get_parser(None) def test_not_implemented(self): + taxonomy = Taxonomy(name="Test taxonomy") with self.assertRaises(NotImplementedError): Parser.parse_import(BytesIO()) + with self.assertRaises(NotImplementedError): + Parser.export(taxonomy) def test_tag_parser_error(self): tag = {"id": 'tag_id', "value": "tag_value"} @@ -47,7 +52,7 @@ def test_tag_parser_error(self): @ddt.ddt -class TestJSONParser(TestCase): +class TestJSONParser(TestImportExportMixin, TestCase): """ Test for .json parser """ @@ -147,8 +152,40 @@ def test_parse_tags(self): index + JSONParser.inital_row ) + def test_export_data(self): + result = JSONParser.export(self.taxonomy) + tags = json.loads(result).get("tags") + assert len(tags) == self.taxonomy.tag_set.count() + for tag in tags: + taxonomy_tag = self.taxonomy.tag_set.get(external_id=tag.get("id")) + assert tag.get("value") == taxonomy_tag.value + if tag.get("parent_id"): + assert tag.get("parent_id") == taxonomy_tag.parent.external_id + + def test_import_with_export_output(self): + output = JSONParser.export(self.taxonomy) + json_file = BytesIO(output.encode()) + tags, errors = JSONParser.parse_import(json_file) + output_tags = json.loads(output).get("tags") + + self.assertEqual(len(errors), 0) + self.assertEqual(len(tags), len(output_tags)) + + + for tag in tags: + output_tag = None + for out_tag in output_tags: + if out_tag.get("id") == tag.id: + output_tag = out_tag + # Don't break because test coverage + assert output_tag + assert output_tag.get("value") == tag.value + if output_tag.get("parent_id"): + assert output_tag.get("parent_id") == tag.parent_id + + @ddt.ddt -class TestCSVParser(TestCase): +class TestCSVParser(TestImportExportMixin, TestCase): """ Test for .csv parser """ @@ -257,3 +294,16 @@ def test_parse_tags(self): tags[index].index, index + CSVParser.inital_row ) + + def test_import_with_export_output(self): + output = CSVParser.export(self.taxonomy) + csv_file = BytesIO(output.encode()) + tags, errors = CSVParser.parse_import(csv_file) + self.assertEqual(len(errors), 0) + assert len(tags) == self.taxonomy.tag_set.count() + + for tag in tags: + taxonomy_tag = self.taxonomy.tag_set.get(external_id=tag.id) + assert tag.value == taxonomy_tag.value + if tag.parent_id: + assert tag.parent_id == taxonomy_tag.parent.external_id From 49ef4b0e097c99c0455d6a032daaaa8b18c0bc3d Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Fri, 4 Aug 2023 14:31:10 -0500 Subject: [PATCH 12/20] docs: Added docstrings --- .../core/tagging/import_export/actions.py | 27 ++++- .../core/tagging/import_export/api.py | 84 +++++++++++++-- .../core/tagging/import_export/exceptions.py | 35 ++++++ .../core/tagging/import_export/import_plan.py | 28 +++-- .../core/tagging/import_export/parsers.py | 15 ++- .../core/tagging/import_export/tasks.py | 23 +++- .../tagging/import_export/test_actions.py | 52 ++++----- .../{test_dsl.py => test_import_plan.py} | 100 +++++++++--------- .../tagging/import_export/test_parsers.py | 1 + .../core/tagging/import_export/test_tasks.py | 42 ++++++++ 10 files changed, 298 insertions(+), 109 deletions(-) rename tests/openedx_tagging/core/tagging/import_export/{test_dsl.py => test_import_plan.py} (78%) create mode 100644 tests/openedx_tagging/core/tagging/import_export/test_tasks.py diff --git a/openedx_tagging/core/tagging/import_export/actions.py b/openedx_tagging/core/tagging/import_export/actions.py index a411be60d..73e671b73 100644 --- a/openedx_tagging/core/tagging/import_export/actions.py +++ b/openedx_tagging/core/tagging/import_export/actions.py @@ -12,6 +12,25 @@ class ImportAction: """ Base class to create actions + + Each action is a simple operation to be performed on the database. + There are no compound actions or actions that have to do with each other. + + To create an Action you need to implement the following: + + Given a TagItem, the actions to be performed must be deduced + by comparing with the tag on the database. + Ex. The create action is inferred if the tag does not exist in the database. + This check is done in `valid_for` + + Then each action validates if the change is consistent with the database + or with previous actions. + Ex. Verify that when creating a tag, there is not a previous creation action + that has the same tag_id. + This checks is done in `validate` + + Then the actions are executed. Ex. Create the tag on the database + This is done in `execute` """ name = "import_action" @@ -30,7 +49,7 @@ def __str__(self): @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: """ - Implement this to meet the conditions that a `TagDSL` needs + Implement this to meet the conditions that a `TagItem` needs to have for this action. All actions that are valid with this function are created. """ @@ -63,7 +82,7 @@ def _search_action( search_value: str, ): """ - Use this function to find and action using an `attr` of `TagDSL` + Use this function to find and action using an `attr` of `TagItem` """ for action in indexed_actions[action_name]: if search_value == getattr(action.tag, attr): @@ -251,7 +270,7 @@ def __str__(self): @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: """ - Validates there is a change on the parent + Validates if there is a change on the parent """ try: taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) @@ -313,7 +332,7 @@ def __str__(self): @classmethod def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: """ - Validates there is a change on the tag value + Validates if there is a change on the tag value """ try: taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py index 76bfb7c44..dc1aca1af 100644 --- a/openedx_tagging/core/tagging/import_export/api.py +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -1,4 +1,38 @@ """ +Import/export API functions + +A modular implementation has been followed for both functionalities. + +Import +------------ + +In this functionality we have the following pipeline with the following classes: + +Parser.parse_import() -> TagImportPlan.generate_actions() -> [ImportActions] +-> TagImportPlan.plan() -> TagImportPlan.execute() + +Parsers are in charge of reading the input file, +making the respective verifications of its format and returning a list of TagItems. +You need to create parser for each format that the system will accept. +For more information see parsers.py + +TagImportPlan receives a list of TagItems. With this, it generates each +action that will be executed in the import. +Each Action are in charge of verifying and executing specific +and simple operations in the database, such as creating or rename tag. +For more information see actions.py + +In each action it is verified if there are no errors or inconsistencies +with taxonomy tags or with previous actions. +In the end, TagImportPlan contains all actions and possible errors. +You can run `plan()` to see the actions and errors or you can run `execute()` +to execute each action. + +Export +---------- + +The export only uses Parsers. Calls the respective function and +returns a string with the data. """ from io import BytesIO @@ -6,7 +40,7 @@ from ..models import Taxonomy, TagImportTask, TagImportTaskState from .parsers import get_parser, ParserFormat -from .import_plan import TagImportDSL, TagImportTask +from .import_plan import TagImportPlan, TagImportTask def import_tags( @@ -15,6 +49,17 @@ def import_tags( parser_format: ParserFormat, replace=False, ) -> bool: + """ + Execute the necessary actions to import the tags from `file` + + You can read the docstring of the top for more info about the + modular architecture. + + It creates an TagImporTask to keep logs of the execution + of each import step and the current status. + There can only be one task in progress at a time per taxonomy + """ + _import_export_validations(taxonomy) # Checks that exists only one tag @@ -44,16 +89,16 @@ def import_tags( # Generate actions task.log_start_planning() - dsl = TagImportDSL(taxonomy) - dsl.generate_actions(tags, replace) - task.log_plan(dsl) + tag_import_plan = TagImportPlan(taxonomy) + tag_import_plan.generate_actions(tags, replace) + task.log_plan(tag_import_plan) - if dsl.errors: + if tag_import_plan.errors: task.handle_plan_errors() return False task.log_start_execute() - dsl.execute(task) + tag_import_plan.execute(task) task.end_success() return True except Exception as exception: @@ -63,23 +108,36 @@ def import_tags( def get_last_import_status(taxonomy: Taxonomy) -> TagImportTaskState: - task = _get_last_tags(taxonomy) + """ + Get status of the last import task of the given taxonomy + """ + task = _get_last_import_task(taxonomy) return task.status def get_last_import_log(taxonomy: Taxonomy) -> str: - task = _get_last_tags(taxonomy) + """ + Get logs of the last import task of the given taxonomy + """ + task = _get_last_import_task(taxonomy) return task.log def export_tags(taxonomy: Taxonomy, output_format: ParserFormat) -> str: + """ + Returns a string with all tag data of the given taxonomy + """ _import_export_validations(taxonomy) parser = get_parser(output_format) return parser.export(taxonomy) def _check_unique_import_task(taxonomy: Taxonomy) -> bool: - last_task = _get_last_tags(taxonomy) + """ + Verifies if there is another in progress import task for the + given taxonomy + """ + last_task = _get_last_import_task(taxonomy) if not last_task: return True return ( @@ -88,7 +146,10 @@ def _check_unique_import_task(taxonomy: Taxonomy) -> bool: ) -def _get_last_tags(taxonomy: Taxonomy) -> TagImportTask: +def _get_last_import_task(taxonomy: Taxonomy) -> TagImportTask: + """ + Get the last import task for the given taxonomy + """ return ( TagImportTask.objects.filter(taxonomy=taxonomy) .order_by("-creation_date") @@ -97,6 +158,9 @@ def _get_last_tags(taxonomy: Taxonomy) -> TagImportTask: def _import_export_validations(taxonomy: Taxonomy): + """ + Validates if the taxonomy is allowed to import or export tags + """ taxonomy = taxonomy.cast() if taxonomy.allow_free_text: raise ValueError( diff --git a/openedx_tagging/core/tagging/import_export/exceptions.py b/openedx_tagging/core/tagging/import_export/exceptions.py index 5d2fa73bd..2330cae6a 100644 --- a/openedx_tagging/core/tagging/import_export/exceptions.py +++ b/openedx_tagging/core/tagging/import_export/exceptions.py @@ -1,7 +1,14 @@ +""" +Exceptions for tag import/export actions +""" from django.utils.translation import gettext_lazy as _ class TagImportError(Exception): + """ + Base exception for import + """ + def __init__(self, message: str = "", **kargs): self.message = message @@ -13,11 +20,19 @@ def __repr__(self): class TagParserError(TagImportError): + """ + Base exception for parsers + """ + def __init__(self, tag, **kargs): self.message = _(f"Import parser error on {tag}") class ImportActionError(TagImportError): + """ + Base exception for actions + """ + def __init__(self, action: str, tag_id: str, message: str, **kargs): self.message = _( f"Action error in '{action.name}' (#{action.index}): {message}" @@ -25,6 +40,10 @@ def __init__(self, action: str, tag_id: str, message: str, **kargs): class ImportActionConflict(ImportActionError): + """ + Exception used when exists a conflict between actions + """ + def __init__( self, action: str, @@ -40,24 +59,40 @@ def __init__( class InvalidFormat(TagParserError): + """ + Exception used when there is an error with the format + """ + def __init__(self, tag: dict, format: str, message: str, **kargs): self.tag = tag self.message = _(f"Invalid '{format}' format: {message}") class FieldJSONError(TagParserError): + """ + Exception used when missing a required field on the .json + """ + def __init__(self, tag, field, **kargs): self.tag = tag self.message = _(f"Missing '{field}' field on {tag}") class EmptyJSONField(TagParserError): + """ + Exception used when a required field is empty on the .json + """ + def __init__(self, tag, field, **kargs): self.tag = tag self.message = _(f"Empty '{field}' field on {tag}") class EmptyCSVField(TagParserError): + """ + Exception used when a required field is empty on the .csv + """ + def __init__(self, tag, field, row, **kargs): self.tag = tag self.message = _(f"Empty '{field}' field on the row {row}") diff --git a/openedx_tagging/core/tagging/import_export/import_plan.py b/openedx_tagging/core/tagging/import_export/import_plan.py index afec77061..90d89dcd4 100644 --- a/openedx_tagging/core/tagging/import_export/import_plan.py +++ b/openedx_tagging/core/tagging/import_export/import_plan.py @@ -1,5 +1,5 @@ """ -Model and functions to create a plan/execution with DSL actions. +Classes and functions to create an import plan and execution. """ from typing import List, Optional @@ -16,9 +16,9 @@ from .exceptions import ImportActionError -class TagDSL: +class TagItem: """ - Tag representation on the import DSL + Tag representation on the tag import plan """ id: str @@ -42,7 +42,7 @@ def __init__( self.action = action -class TagImportDSL: +class TagImportPlan: """ Class with functions to build an import plan and excute the plan """ @@ -68,7 +68,7 @@ def _init_indexed_actions(self): for action in available_actions: self.indexed_actions[action.name] = [] - def _build_action(self, action_cls, tag: TagDSL): + def _build_action(self, action_cls, tag: TagItem): """ Build an action with `tag`. @@ -114,7 +114,7 @@ def _build_delete_actions(self, tags: dict): # Change parent to avoid delete childs self._build_action( UpdateParentTag, - TagDSL( + TagItem( id=child.external_id, value=child.value, parent_id=None, @@ -124,7 +124,7 @@ def _build_delete_actions(self, tags: dict): # Delete action self._build_action( DeleteTag, - TagDSL( + TagItem( id=tag.external_id, value=tag.value, ), @@ -132,16 +132,17 @@ def _build_delete_actions(self, tags: dict): def generate_actions( self, - tags: List[TagDSL], + tags: List[TagItem], replace=False, ): """ - Generates actions from `tags`. + Reads each tag and generates the corresponding actions. Validates each action and create respective errors - If `replace` is True, then deletes the tags that have not been read + If `replace` is True, then creates the delete action for tags + that has not been readed. - TODO: Missing join/reduce actions. Eg. A tag may have no changes, + TODO: Join/reduce actions. Ex. A tag may have no changes, but then its parent needs to be updated because its parent is deleted. Those two actions should be merged. """ @@ -195,6 +196,11 @@ def plan(self) -> str: @transaction.atomic() def execute(self, task: TagImportTask = None): + """ + Executes each action + + If task is set, creates logs for each action + """ if self.errors: return for action in self.actions: diff --git a/openedx_tagging/core/tagging/import_export/parsers.py b/openedx_tagging/core/tagging/import_export/parsers.py index 9fc18cbf7..23be834b0 100644 --- a/openedx_tagging/core/tagging/import_export/parsers.py +++ b/openedx_tagging/core/tagging/import_export/parsers.py @@ -9,7 +9,7 @@ from django.utils.translation import gettext_lazy as _ -from .import_plan import TagDSL +from .import_plan import TagItem from .exceptions import ( TagParserError, InvalidFormat, @@ -36,6 +36,11 @@ class Parser: This contains the base functions to load data, validate required fields and convert tags to DLS format + + If you want to add a new field, you can add it to + `required_fields` or `optional_fields` depending on the field type + + To create a new Parser you need to implement `_load_data` and `_export_data` """ required_fields = ["id", "value"] @@ -51,7 +56,7 @@ class Parser: inital_row = 1 @classmethod - def parse_import(cls, file: BytesIO) -> Tuple[List[TagDSL], List[TagParserError]]: + def parse_import(cls, file: BytesIO) -> Tuple[List[TagItem], List[TagParserError]]: """ Parse tags in file an returns tags ready for use in TagImportPlan @@ -102,11 +107,11 @@ def _export_data(cls, tags: List[dict], taxonomy: Taxonomy) -> str: raise NotImplementedError @classmethod - def _parse_tags(cls, tags_data: dict) -> Tuple[List[TagDSL], List[TagParserError]]: + def _parse_tags(cls, tags_data: dict) -> Tuple[List[TagItem], List[TagParserError]]: """ Validate the required fields of each tag. - Return a list of tags in the DSL format + Return a list of TagItems and a list of validation errors. """ tags = [] @@ -150,7 +155,7 @@ def _parse_tags(cls, tags_data: dict) -> Tuple[List[TagDSL], List[TagParserError if opt_field in tag and not tag.get(opt_field): tag[opt_field] = None - tags.append(TagDSL(**tag)) + tags.append(TagItem(**tag)) return tags, errors diff --git a/openedx_tagging/core/tagging/import_export/tasks.py b/openedx_tagging/core/tagging/import_export/tasks.py index 21260183b..99d3cd49d 100644 --- a/openedx_tagging/core/tagging/import_export/tasks.py +++ b/openedx_tagging/core/tagging/import_export/tasks.py @@ -1,7 +1,10 @@ +""" +Import and export celery tasks +""" from io import BytesIO from celery import shared_task -from .api import import_tags +import openedx_tagging.core.tagging.import_export.api as import_export_api from ..models import Taxonomy from .parsers import ParserFormat @@ -12,10 +15,24 @@ def import_tags_task( file: BytesIO, parser_format: ParserFormat, replace=False, -): - import_tags( +) -> bool: + """ + Runs import on a celery task + """ + return import_export_api.import_tags( taxonomy, file, parser_format, replace, ) + + +@shared_task +def export_tags_task( + taxonomy: Taxonomy, + output_format: ParserFormat, +) -> str: + """ + Runs export on a celery task + """ + return import_export_api.export_tags(taxonomy, output_format) diff --git a/tests/openedx_tagging/core/tagging/import_export/test_actions.py b/tests/openedx_tagging/core/tagging/import_export/test_actions.py index 23d99a674..6ae205450 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_actions.py @@ -6,7 +6,7 @@ from django.test.testcases import TestCase from openedx_tagging.core.tagging.models import Tag -from openedx_tagging.core.tagging.import_export.import_plan import TagDSL +from openedx_tagging.core.tagging.import_export.import_plan import TagItem from openedx_tagging.core.tagging.import_export.actions import ( ImportAction, CreateTag, @@ -28,7 +28,7 @@ def setUp(self): 'create': [ CreateTag( taxonomy=self.taxonomy, - tag=TagDSL( + tag=TagItem( id='tag_10', value='Tag 10', index=0 @@ -39,7 +39,7 @@ def setUp(self): 'rename': [ RenameTag( taxonomy=self.taxonomy, - tag=TagDSL( + tag=TagItem( id='tag_11', value='Tag 11', index=1 @@ -69,7 +69,7 @@ def test_str(self): expected = "Action import_action (index=100,id=tag_1)" action = ImportAction( taxonomy=self.taxonomy, - tag=TagDSL( + tag=TagItem( id='tag_1', value='value', ), @@ -106,7 +106,7 @@ def test_search_action(self, action_name, attr, search_value, expected): def test_validate_parent(self, parent_id, expected): action = ImportAction( self.taxonomy, - TagDSL( + TagItem( id='tag_110', value='_', parent_id=parent_id, @@ -155,7 +155,7 @@ def test_validate_parent(self, parent_id, expected): def test_validate_value(self, value, expected): action = ImportAction( self.taxonomy, - TagDSL( + TagItem( id='tag_110', value=value, index=100 @@ -183,7 +183,7 @@ class TestCreateTag(TestImportActionMixin, TestCase): def test_valid_for(self, tag_id, expected): result = CreateTag.valid_for( self.taxonomy, - TagDSL( + TagItem( id=tag_id, value='_', index=100, @@ -199,7 +199,7 @@ def test_valid_for(self, tag_id, expected): def test_validate_id(self, tag_id, expected): action = CreateTag( taxonomy=self.taxonomy, - tag=TagDSL( + tag=TagItem( id=tag_id, value='_', index=100, @@ -230,7 +230,7 @@ def test_validate_id(self, tag_id, expected): def test_validate(self, tag_id, tag_value, parent_id, expected): action = CreateTag( taxonomy=self.taxonomy, - tag=TagDSL( + tag=TagItem( id=tag_id, value=tag_value, index=100, @@ -247,7 +247,7 @@ def test_validate(self, tag_id, tag_value, parent_id, expected): ) @ddt.unpack def test_execute(self, tag_id, value, parent_id): - tag = TagDSL( + tag = TagItem( id=tag_id, value=value, parent_id=parent_id, @@ -294,14 +294,14 @@ class TestUpdateParentTag(TestImportActionMixin, TestCase): ) @ddt.unpack def test_str(self, tag_id, parent_id, expected): - tag_dsl = TagDSL( + tag_item = TagItem( id=tag_id, value='_', parent_id=parent_id, ) action = UpdateParentTag( taxonomy=self.taxonomy, - tag=tag_dsl, + tag=tag_item, index=100, ) assert str(action) == expected @@ -317,7 +317,7 @@ def test_str(self, tag_id, parent_id, expected): def test_valid_for(self, tag_id, parent_id, expected): result = UpdateParentTag.valid_for( taxonomy=self.taxonomy, - tag=TagDSL( + tag=TagItem( id=tag_id, value='_', parent_id=parent_id, @@ -335,7 +335,7 @@ def test_valid_for(self, tag_id, parent_id, expected): def test_validate(self, tag_id, parent_id, expected): action = UpdateParentTag( taxonomy=self.taxonomy, - tag=TagDSL( + tag=TagItem( id=tag_id, value='_', parent_id=parent_id, @@ -352,14 +352,14 @@ def test_validate(self, tag_id, parent_id, expected): ) @ddt.unpack def test_execute(self, tag_id, parent_id): - tag_dsl = TagDSL( + tag_item = TagItem( id=tag_id, value='_', parent_id=parent_id, ) action = UpdateParentTag( taxonomy=self.taxonomy, - tag=tag_dsl, + tag=tag_item, index=100, ) tag = self.taxonomy.tag_set.get(external_id=tag_id) @@ -388,7 +388,7 @@ class TestRenameTag(TestImportActionMixin, TestCase): def test_valid_for(self, tag_id, value, expected): result = RenameTag.valid_for( taxonomy=self.taxonomy, - tag=TagDSL( + tag=TagItem( id=tag_id, value=value, index=100, @@ -406,7 +406,7 @@ def test_valid_for(self, tag_id, value, expected): def test_validate(self, value, expected): action = RenameTag( taxonomy=self.taxonomy, - tag=TagDSL( + tag=TagItem( id='tag_1', value=value, index=100, @@ -419,13 +419,13 @@ def test_validate(self, value, expected): def test_execute(self): tag_id = 'tag_1' value = 'Tag 1 V2' - tag_dsl = TagDSL( + tag_item = TagItem( id=tag_id, value=value, ) action = RenameTag( taxonomy=self.taxonomy, - tag=tag_dsl, + tag=tag_item, index=100, ) tag = self.taxonomy.tag_set.get(external_id=tag_id) @@ -450,7 +450,7 @@ class TestDeleteTag(TestImportActionMixin, TestCase): def test_valid_for(self, tag_id, action, expected): result = DeleteTag.valid_for( taxonomy=self.taxonomy, - tag=TagDSL( + tag=TagItem( id=tag_id, value='_', action=action, @@ -462,7 +462,7 @@ def test_valid_for(self, tag_id, action, expected): def test_validate(self): action = DeleteTag( taxonomy=self.taxonomy, - tag=TagDSL( + tag=TagItem( id='_', value='_', index=100, @@ -474,13 +474,13 @@ def test_validate(self): def test_execute(self): tag_id = 'tag_3' - tag_dsl = TagDSL( + tag_item = TagItem( id=tag_id, value='_', ) action = DeleteTag( taxonomy=self.taxonomy, - tag=tag_dsl, + tag=tag_item, index=100, ) assert self.taxonomy.tag_set.filter(external_id=tag_id).exists() @@ -495,7 +495,7 @@ class TestWithoutChanges(TestImportActionMixin, TestCase): def test_valid_for(self): result = WithoutChanges.valid_for( self.taxonomy, - tag=TagDSL( + tag=TagItem( id='_', value='_', index=100, @@ -506,7 +506,7 @@ def test_valid_for(self): def test_validate(self): action = WithoutChanges( taxonomy=self.taxonomy, - tag=TagDSL( + tag=TagItem( id='_', value='_', index=100, diff --git a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py b/tests/openedx_tagging/core/tagging/import_export/test_import_plan.py similarity index 78% rename from tests/openedx_tagging/core/tagging/import_export/test_dsl.py rename to tests/openedx_tagging/core/tagging/import_export/test_import_plan.py index 41ce29675..c75074b5a 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_dsl.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_import_plan.py @@ -1,24 +1,24 @@ """ -Test for DSL functions +Test for import_plan functions """ import ddt from django.test.testcases import TestCase -from openedx_tagging.core.tagging.import_export.import_plan import TagDSL, TagImportDSL +from openedx_tagging.core.tagging.import_export.import_plan import TagItem, TagImportPlan from openedx_tagging.core.tagging.import_export.actions import CreateTag from openedx_tagging.core.tagging.import_export.exceptions import TagImportError from .test_actions import TestImportActionMixin @ddt.ddt -class TestTagImportDSL(TestImportActionMixin, TestCase): +class TestTagImportPlan(TestImportActionMixin, TestCase): """ - Test for DSL functions + Test for import plan functions """ def setUp(self): super().setUp() - self.dsl = TagImportDSL(self.taxonomy) + self.import_plan = TagImportPlan(self.taxonomy) def test_tag_import_error(self): message = "Error message" @@ -34,19 +34,19 @@ def test_tag_import_error(self): ) @ddt.unpack def test_build_action(self, tag_id, errors_expected): - self.dsl.indexed_actions = self.indexed_actions - self.dsl._build_action( # pylint: disable=protected-access + self.import_plan.indexed_actions = self.indexed_actions + self.import_plan._build_action( # pylint: disable=protected-access CreateTag, - TagDSL( + TagItem( id=tag_id, value='_', index=100 ) ) - assert len(self.dsl.errors) == errors_expected - assert len(self.dsl.actions) == 1 - assert self.dsl.actions[0].name == 'create' - assert self.dsl.indexed_actions['create'][1].tag.id == tag_id + assert len(self.import_plan.errors) == errors_expected + assert len(self.import_plan.actions) == 1 + assert self.import_plan.actions[0].name == 'create' + assert self.import_plan.indexed_actions['create'][1].tag.id == tag_id def test_build_delete_actions(self): tags = { @@ -54,29 +54,29 @@ def test_build_delete_actions(self): for tag in self.taxonomy.tag_set.exclude(pk=25) } # Clear other actions to only have the delete ones - self.dsl.actions.clear() + self.import_plan.actions.clear() - self.dsl._build_delete_actions(tags) # pylint: disable=protected-access - assert len(self.dsl.errors) == 0 + self.import_plan._build_delete_actions(tags) # pylint: disable=protected-access + assert len(self.import_plan.errors) == 0 # Check actions in order # #1 Update parent of 'tag_2' - assert self.dsl.actions[0].name == 'update_parent' - assert self.dsl.actions[0].tag.id == 'tag_2' - assert self.dsl.actions[0].tag.parent_id is None + assert self.import_plan.actions[0].name == 'update_parent' + assert self.import_plan.actions[0].tag.id == 'tag_2' + assert self.import_plan.actions[0].tag.parent_id is None # #2 Delete 'tag_1' - assert self.dsl.actions[1].name == 'delete' - assert self.dsl.actions[1].tag.id == 'tag_1' + assert self.import_plan.actions[1].name == 'delete' + assert self.import_plan.actions[1].tag.id == 'tag_1' # #3 Delete 'tag_2' - assert self.dsl.actions[2].name == 'delete' - assert self.dsl.actions[2].tag.id == 'tag_2' + assert self.import_plan.actions[2].name == 'delete' + assert self.import_plan.actions[2].tag.id == 'tag_2' # #4 Update parent of 'tag_4' - assert self.dsl.actions[3].name == 'update_parent' - assert self.dsl.actions[3].tag.id == 'tag_4' - assert self.dsl.actions[3].tag.parent_id is None + assert self.import_plan.actions[3].name == 'update_parent' + assert self.import_plan.actions[3].tag.id == 'tag_4' + assert self.import_plan.actions[3].tag.parent_id is None # #5 Delete 'tag_3' - assert self.dsl.actions[4].name == 'delete' - assert self.dsl.actions[4].tag.id == 'tag_3' + assert self.import_plan.actions[4].name == 'delete' + assert self.import_plan.actions[4].tag.id == 'tag_3' @ddt.data( ([ @@ -209,15 +209,15 @@ def test_build_delete_actions(self): ) @ddt.unpack def test_generate_actions(self, tags, replace, expected_errors, expected_actions): - tags = [TagDSL(**tag) for tag in tags] - self.dsl.generate_actions(tags=tags, replace=replace) - assert len(self.dsl.errors) == expected_errors - assert len(self.dsl.actions) == len(expected_actions) + tags = [TagItem(**tag) for tag in tags] + self.import_plan.generate_actions(tags=tags, replace=replace) + assert len(self.import_plan.errors) == expected_errors + assert len(self.import_plan.actions) == len(expected_actions) for index, action in enumerate(expected_actions): - assert self.dsl.actions[index].name == action['name'] - assert self.dsl.actions[index].tag.id == action['id'] - assert self.dsl.actions[index].index == index + 1 + assert self.import_plan.actions[index].name == action['name'] + assert self.import_plan.actions[index].tag.id == action['id'] + assert self.import_plan.actions[index].index == index + 1 @ddt.data( ([ @@ -321,9 +321,9 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions ) @ddt.unpack def test_plan(self, tags, replace, expected): - tags = [TagDSL(**tag) for tag in tags] - self.dsl.generate_actions(tags=tags, replace=replace) - plan = self.dsl.plan() + tags = [TagItem(**tag) for tag in tags] + self.import_plan.generate_actions(tags=tags, replace=replace) + plan = self.import_plan.plan() assert plan == expected @ddt.data( @@ -362,25 +362,25 @@ def test_plan(self, tags, replace, expected): ) @ddt.unpack def test_execute(self, tags, replace): - tags = [TagDSL(**tag) for tag in tags] - self.dsl.generate_actions(tags=tags, replace=replace) - self.dsl.execute() + tags = [TagItem(**tag) for tag in tags] + self.import_plan.generate_actions(tags=tags, replace=replace) + self.import_plan.execute() tag_external_ids = [] - for tag_dsl in tags: + for tag_item in tags: # This checks any creation - tag = self.taxonomy.tag_set.get(external_id=tag_dsl.id) + tag = self.taxonomy.tag_set.get(external_id=tag_item.id) # Checks any rename - assert tag.value == tag_dsl.value + assert tag.value == tag_item.value # Checks any parent update if not replace: - if not tag_dsl.parent_id: + if not tag_item.parent_id: assert tag.parent is None else: - assert tag.parent.external_id == tag_dsl.parent_id + assert tag.parent.external_id == tag_item.parent_id - tag_external_ids.append(tag_dsl.id) + tag_external_ids.append(tag_item.id) if replace: # Checks deletions checking that exists the updated tags @@ -390,17 +390,17 @@ def test_execute(self, tags, replace): def test_error_in_execute(self): created_tag = 'tag_31' tags = [ - TagDSL( + TagItem( id=created_tag, value='Tag 31' ), # Valid tag (creation) - TagDSL( + TagItem( id='tag_32', value='Tag 31' ), # Invalid ] - self.dsl.generate_actions(tags=tags) + self.import_plan.generate_actions(tags=tags) assert not self.taxonomy.tag_set.filter(external_id=created_tag).exists() - assert not self.dsl.execute() + assert not self.import_plan.execute() assert not self.taxonomy.tag_set.filter(external_id=created_tag).exists() \ No newline at end of file diff --git a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py index e09409b07..50bedf2e6 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py @@ -37,6 +37,7 @@ def test_parser_not_found(self): def test_not_implemented(self): taxonomy = Taxonomy(name="Test taxonomy") + taxonomy.save() with self.assertRaises(NotImplementedError): Parser.parse_import(BytesIO()) with self.assertRaises(NotImplementedError): diff --git a/tests/openedx_tagging/core/tagging/import_export/test_tasks.py b/tests/openedx_tagging/core/tagging/import_export/test_tasks.py new file mode 100644 index 000000000..4b88fc6d6 --- /dev/null +++ b/tests/openedx_tagging/core/tagging/import_export/test_tasks.py @@ -0,0 +1,42 @@ +""" +Test import/export celery tasks +""" +from io import BytesIO +from unittest.mock import patch + +from django.test.testcases import TestCase + +from openedx_tagging.core.tagging.import_export import ParserFormat +import openedx_tagging.core.tagging.import_export.tasks as import_export_tasks + +from .mixins import TestImportExportMixin + + +class TestImportExportCeleryTasks(TestImportExportMixin, TestCase): + """ + Test import/export celery tasks + """ + + def test_import_tags_task(self): + file = BytesIO(b"some_data") + parser_format = ParserFormat.CSV + replace = True + + with patch('openedx_tagging.core.tagging.import_export.api.import_tags') as mock_import_tags: + mock_import_tags.return_value = True + + result = import_export_tasks.import_tags_task(self.taxonomy, file, parser_format, replace) + + self.assertTrue(result) + mock_import_tags.assert_called_once_with(self.taxonomy, file, parser_format, replace) + + def test_export_tags_task(self): + output_format = ParserFormat.JSON + + with patch('openedx_tagging.core.tagging.import_export.api.export_tags') as mock_export_tags: + mock_export_tags.return_value = "exported_data" + + result = import_export_tasks.export_tags_task(self.taxonomy, output_format) + + self.assertEqual(result, "exported_data") + mock_export_tags.assert_called_once_with(self.taxonomy, output_format) From 332db7b50d54f1a133f112609daa3c8438664027 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Fri, 4 Aug 2023 15:02:04 -0500 Subject: [PATCH 13/20] chore: Added attrs dependency --- .../core/tagging/import_export/import_plan.py | 22 +++++-------------- requirements/base.in | 2 ++ requirements/base.txt | 2 ++ requirements/dev.txt | 2 ++ requirements/doc.txt | 2 ++ requirements/quality.txt | 2 ++ requirements/test.txt | 2 ++ 7 files changed, 17 insertions(+), 17 deletions(-) diff --git a/openedx_tagging/core/tagging/import_export/import_plan.py b/openedx_tagging/core/tagging/import_export/import_plan.py index 90d89dcd4..fb8ba08d6 100644 --- a/openedx_tagging/core/tagging/import_export/import_plan.py +++ b/openedx_tagging/core/tagging/import_export/import_plan.py @@ -1,6 +1,7 @@ """ Classes and functions to create an import plan and execution. """ +from attrs import define from typing import List, Optional from django.db import transaction @@ -16,6 +17,7 @@ from .exceptions import ImportActionError +@define class TagItem: """ Tag representation on the tag import plan @@ -23,23 +25,9 @@ class TagItem: id: str value: str - index: Optional[int] - parent_id: Optional[str] - action: Optional[str] - - def __init__( - self, - id: str, - value: str, - index: str = 0, - parent_id: str = None, - action: str = None, - ): - self.id = id - self.value = value - self.index = index - self.parent_id = parent_id - self.action = action + index: Optional[int] = 0 + parent_id: Optional[str] = None + action: Optional[str] = None class TagImportPlan: diff --git a/requirements/base.in b/requirements/base.in index 8fa8e805d..8cdf6e6d4 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -1,6 +1,8 @@ # Core requirements for using this application -c constraints.txt +attrs # Reduces boilerplate code involving class attributes + celery # Asynchronous task execution library Django<5.0 # Web application framework diff --git a/requirements/base.txt b/requirements/base.txt index 0f2ee63e0..d4a7c3fd1 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -8,6 +8,8 @@ amqp==5.1.1 # via kombu asgiref==3.7.2 # via django +attrs==23.1.0 + # via -r requirements/base.in billiard==3.6.4.0 # via celery celery==5.2.0 diff --git a/requirements/dev.txt b/requirements/dev.txt index dec4b89ff..93fd330d7 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -17,6 +17,8 @@ astroid==2.15.5 # -r requirements/quality.txt # pylint # pylint-celery +attrs==23.1.0 + # via -r requirements/quality.txt billiard==3.6.4.0 # via # -r requirements/quality.txt diff --git a/requirements/doc.txt b/requirements/doc.txt index 03eae3799..66e07b443 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -16,6 +16,8 @@ asgiref==3.7.2 # via # -r requirements/test.txt # django +attrs==23.1.0 + # via -r requirements/test.txt babel==2.12.1 # via # pydata-sphinx-theme diff --git a/requirements/quality.txt b/requirements/quality.txt index c857f3b3b..4f251375c 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -16,6 +16,8 @@ astroid==2.15.5 # via # pylint # pylint-celery +attrs==23.1.0 + # via -r requirements/test.txt billiard==3.6.4.0 # via # -r requirements/test.txt diff --git a/requirements/test.txt b/requirements/test.txt index 636639552..cca7cd5e7 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -12,6 +12,8 @@ asgiref==3.7.2 # via # -r requirements/base.txt # django +attrs==23.1.0 + # via -r requirements/base.txt billiard==3.6.4.0 # via # -r requirements/base.txt From 1577938e8ac1c6af7e3714fd6f75db71cda9f3c2 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Mon, 7 Aug 2023 11:09:01 -0500 Subject: [PATCH 14/20] style: typos, docs and new error handler --- openedx_tagging/core/tagging/import_export/api.py | 12 ++++++++++-- .../core/tagging/import_export/parsers.py | 12 ++++++++---- .../core/tagging/import_export/test_parsers.py | 8 ++++++++ 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py index dc1aca1af..5a7893c99 100644 --- a/openedx_tagging/core/tagging/import_export/api.py +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -33,6 +33,14 @@ The export only uses Parsers. Calls the respective function and returns a string with the data. + + +TODO for next versions +--------- +- Function to force clean the status of an import task, or a way to avoid + a lock: a task not in SUCCESS or ERROR due to something unexpected + (ex. server crash) +- Join/reduce actions on TagImportPlan. See `generate_actions()` """ from io import BytesIO @@ -55,14 +63,14 @@ def import_tags( You can read the docstring of the top for more info about the modular architecture. - It creates an TagImporTask to keep logs of the execution + It creates an TagImportTask to keep logs of the execution of each import step and the current status. There can only be one task in progress at a time per taxonomy """ _import_export_validations(taxonomy) - # Checks that exists only one tag + # Checks that exists only one task import in progress at a time per taxonomy if not _check_unique_import_task(taxonomy): raise ValueError( _( diff --git a/openedx_tagging/core/tagging/import_export/parsers.py b/openedx_tagging/core/tagging/import_export/parsers.py index 23be834b0..36c7ab86c 100644 --- a/openedx_tagging/core/tagging/import_export/parsers.py +++ b/openedx_tagging/core/tagging/import_export/parsers.py @@ -213,7 +213,12 @@ def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[TagParserError]]: Read a .json file and validates the root structure of the json """ file.seek(0) - tags_data = json.load(file) + try: + tags_data = json.load(file) + except json.JSONDecodeError as error: + return None, [ + InvalidFormat(tag=None, format=cls.format.value, message=str(error)) + ] if "tags" not in tags_data: return None, [ InvalidFormat( @@ -264,7 +269,7 @@ def _load_data(cls, file: BytesIO) -> Tuple[List[dict], List[TagParserError]]: text_tags = TextIOWrapper(file, encoding="utf-8") csv_reader = csv.DictReader(text_tags) header_fields = csv_reader.fieldnames - errors = cls._veify_header(header_fields) + errors = cls._verify_header(header_fields) if errors: return None, errors return list(csv_reader), [] @@ -292,12 +297,11 @@ def _export_data(cls, tags: List[dict], taxonomy: Taxonomy) -> str: return csv_string @classmethod - def _veify_header(cls, header_fields: List[str]) -> List[TagParserError]: + def _verify_header(cls, header_fields: List[str]) -> List[TagParserError]: """ Verify that the header contains the required fields """ errors = [] - print(header_fields) for req_field in cls.required_fields: if req_field not in header_fields: errors.append( diff --git a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py index 50bedf2e6..15e1faa89 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py @@ -58,6 +58,14 @@ class TestJSONParser(TestImportExportMixin, TestCase): Test for .json parser """ + def test_invalid_json(self): + json_data = "{This is an invalid json}" + json_file = BytesIO(json_data.encode()) + tags, errors = JSONParser.parse_import(json_file) + assert len(tags) == 0 + assert len(errors) == 1 + assert "Expecting property name enclosed in double quotes" in str(errors[0]) + def test_load_data_errors(self): json_data = {"invalid": [ {"id": "tag_1", "name": "Tag 1"}, From ebdd1cc9156ebde7e92e607918b3650fe057d1a7 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 10 Aug 2023 13:33:06 -0500 Subject: [PATCH 15/20] style: Updated valid_for to applies_for --- .../core/tagging/import_export/actions.py | 18 +++++++-------- .../core/tagging/import_export/import_plan.py | 2 +- .../tagging/import_export/test_actions.py | 22 +++++++++---------- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/openedx_tagging/core/tagging/import_export/actions.py b/openedx_tagging/core/tagging/import_export/actions.py index 73e671b73..74b1fd6bf 100644 --- a/openedx_tagging/core/tagging/import_export/actions.py +++ b/openedx_tagging/core/tagging/import_export/actions.py @@ -21,7 +21,7 @@ class ImportAction: Given a TagItem, the actions to be performed must be deduced by comparing with the tag on the database. Ex. The create action is inferred if the tag does not exist in the database. - This check is done in `valid_for` + This check is done in `applies_for` Then each action validates if the change is consistent with the database or with previous actions. @@ -47,11 +47,11 @@ def __str__(self): return self.__repr__() @classmethod - def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: """ Implement this to meet the conditions that a `TagItem` needs - to have for this action. All actions that are valid with - this function are created. + to have for this action. If this function returns `True` for `tag` + then the action is created. """ raise NotImplementedError @@ -177,7 +177,7 @@ def __str__(self): ) @classmethod - def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: """ Validates if the tag does not exist """ @@ -268,7 +268,7 @@ def __str__(self): ) @classmethod - def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: """ Validates if there is a change on the parent """ @@ -330,7 +330,7 @@ def __str__(self): ) @classmethod - def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: """ Validates if there is a change on the tag value """ @@ -378,7 +378,7 @@ def __str__(self): name = "delete" @classmethod - def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: """ Validates if the action is delete and the tag exists """ @@ -416,7 +416,7 @@ def __str__(self): return str(_(f"No changes needed for tag (external_id={self.tag.id})")) @classmethod - def valid_for(cls, taxonomy: Taxonomy, tag) -> bool: + def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: """ No validations necessary """ diff --git a/openedx_tagging/core/tagging/import_export/import_plan.py b/openedx_tagging/core/tagging/import_export/import_plan.py index fb8ba08d6..c8eeeec72 100644 --- a/openedx_tagging/core/tagging/import_export/import_plan.py +++ b/openedx_tagging/core/tagging/import_export/import_plan.py @@ -149,7 +149,7 @@ def generate_actions( # Check all available actions and add which ones should be executed for action_cls in available_actions: - if action_cls.valid_for(self.taxonomy, tag): + if action_cls.applies_for(self.taxonomy, tag): self._build_action(action_cls, tag) has_action = True diff --git a/tests/openedx_tagging/core/tagging/import_export/test_actions.py b/tests/openedx_tagging/core/tagging/import_export/test_actions.py index 6ae205450..55a487439 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_actions.py @@ -58,7 +58,7 @@ class TestImportAction(TestImportActionMixin, TestCase): def test_not_implemented_functions(self): with self.assertRaises(NotImplementedError): - ImportAction.valid_for(None, None) + ImportAction.applies_for(None, None) action = ImportAction(None, None, None) with self.assertRaises(NotImplementedError): action.validate(None) @@ -180,8 +180,8 @@ class TestCreateTag(TestImportActionMixin, TestCase): ('tag_100', True), ) @ddt.unpack - def test_valid_for(self, tag_id, expected): - result = CreateTag.valid_for( + def test_applies_for(self, tag_id, expected): + result = CreateTag.applies_for( self.taxonomy, TagItem( id=tag_id, @@ -314,8 +314,8 @@ def test_str(self, tag_id, parent_id, expected): ('tag_1', 'tag_3', True), # Valid ) @ddt.unpack - def test_valid_for(self, tag_id, parent_id, expected): - result = UpdateParentTag.valid_for( + def test_applies_for(self, tag_id, parent_id, expected): + result = UpdateParentTag.applies_for( taxonomy=self.taxonomy, tag=TagItem( id=tag_id, @@ -385,8 +385,8 @@ class TestRenameTag(TestImportActionMixin, TestCase): ('tag_1', 'Tag 1 v2', True), # Valid ) @ddt.unpack - def test_valid_for(self, tag_id, value, expected): - result = RenameTag.valid_for( + def test_applies_for(self, tag_id, value, expected): + result = RenameTag.applies_for( taxonomy=self.taxonomy, tag=TagItem( id=tag_id, @@ -447,8 +447,8 @@ class TestDeleteTag(TestImportActionMixin, TestCase): ('tag_1', 'delete', True), # Valid ) @ddt.unpack - def test_valid_for(self, tag_id, action, expected): - result = DeleteTag.valid_for( + def test_applies_for(self, tag_id, action, expected): + result = DeleteTag.applies_for( taxonomy=self.taxonomy, tag=TagItem( id=tag_id, @@ -492,8 +492,8 @@ class TestWithoutChanges(TestImportActionMixin, TestCase): """ Test for 'without_changes' action """ - def test_valid_for(self): - result = WithoutChanges.valid_for( + def test_applies_for(self): + result = WithoutChanges.applies_for( self.taxonomy, tag=TagItem( id='_', From 6a472e3b6c7f458d1aeebbc7814b92bf4e0d66c8 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 10 Aug 2023 14:03:01 -0500 Subject: [PATCH 16/20] docs: Updating docstrings and logs --- .../core/tagging/import_export/actions.py | 20 +++++----- .../tagging/import_export/test_actions.py | 6 +-- .../tagging/import_export/test_import_plan.py | 38 ++++++++++++------- 3 files changed, 39 insertions(+), 25 deletions(-) diff --git a/openedx_tagging/core/tagging/import_export/actions.py b/openedx_tagging/core/tagging/import_export/actions.py index 74b1fd6bf..f0ca65cd2 100644 --- a/openedx_tagging/core/tagging/import_export/actions.py +++ b/openedx_tagging/core/tagging/import_export/actions.py @@ -92,7 +92,7 @@ def _search_action( def _validate_parent(self, indexed_actions) -> ImportActionError: """ - Validates if the parent is created + Helper method to validate that the parent tag has already been defined. """ try: # Validates that the parent exists on the taxonomy @@ -122,7 +122,9 @@ def _validate_value(self, indexed_actions): return ImportActionError( action=self, tag_id=self.tag.id, - message=_(f"Duplicated tag value with tag (id={tag.id})."), + message=_( + f"Duplicated tag value with tag in database (external_id={tag.external_id})." + ), ) except Tag.DoesNotExist: # Validates value duplication on create actions @@ -179,7 +181,7 @@ def __str__(self): @classmethod def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: """ - Validates if the tag does not exist + This action applies whenever the tag does not exist """ try: taxonomy.tag_set.get(external_id=tag.id) @@ -262,7 +264,7 @@ def __str__(self): return str( _( - f"Update the parent of tag (id={taxonomy_tag.id}) " + f"Update the parent of tag (external_id={taxonomy_tag.external_id}) " f"{from_str} to parent (external_id={self.tag.parent_id})." ) ) @@ -270,7 +272,7 @@ def __str__(self): @classmethod def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: """ - Validates if there is a change on the parent + This action applies whenever there is a change on the parent """ try: taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) @@ -324,7 +326,7 @@ def __str__(self): taxonomy_tag = self._get_tag() return str( _( - f"Rename tag value of tag (id={taxonomy_tag.id}) " + f"Rename tag value of tag (external_id={taxonomy_tag.external_id}) " f"from '{taxonomy_tag.value}' to '{self.tag.value}'" ) ) @@ -332,7 +334,7 @@ def __str__(self): @classmethod def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: """ - Validates if there is a change on the tag value + This action applies whenever there is a change on the tag value """ try: taxonomy_tag = taxonomy.tag_set.get(external_id=tag.id) @@ -373,14 +375,14 @@ class DeleteTag(ImportAction): def __str__(self): taxonomy_tag = self._get_tag() - return str(_(f"Delete tag (id={taxonomy_tag.id})")) + return str(_(f"Delete tag (external_id={taxonomy_tag.external_id})")) name = "delete" @classmethod def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: """ - Validates if the action is delete and the tag exists + This action applies whenever the action is delete and the tag exists """ try: taxonomy.tag_set.get(external_id=tag.id) diff --git a/tests/openedx_tagging/core/tagging/import_export/test_actions.py b/tests/openedx_tagging/core/tagging/import_export/test_actions.py index 55a487439..fb70c8c55 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_actions.py @@ -132,7 +132,7 @@ def test_validate_parent(self, parent_id, expected): 'Tag 1', ( "Action error in 'import_action' (#100): " - "Duplicated tag value with tag (id=26)." + "Duplicated tag value with tag in database (external_id=tag_1)." ) ), ( @@ -279,7 +279,7 @@ class TestUpdateParentTag(TestImportActionMixin, TestCase): "tag_4", "tag_3", ( - "Update the parent of tag (id=29) from parent " + "Update the parent of tag (external_id=tag_4) from parent " "(external_id=tag_3) to parent (external_id=tag_3)." ) ), @@ -287,7 +287,7 @@ class TestUpdateParentTag(TestImportActionMixin, TestCase): "tag_3", "tag_2", ( - "Update the parent of tag (id=28) from empty parent " + "Update the parent of tag (external_id=tag_3) from empty parent " "to parent (external_id=tag_2)." ) ), diff --git a/tests/openedx_tagging/core/tagging/import_export/test_import_plan.py b/tests/openedx_tagging/core/tagging/import_export/test_import_plan.py index c75074b5a..af1e55f30 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_import_plan.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_import_plan.py @@ -252,15 +252,17 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "--------------------------------\n" "#1: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" "#2: Create a new tag with values (external_id=tag_31, value=Tag 32, parent_id=None).\n" - "#3: Rename tag value of tag (id=26) from 'Tag 1' to 'Tag 2'\n" - "#4: Update the parent of tag (id=29) from parent (external_id=tag_3) to parent (external_id=tag_100).\n" + "#3: Rename tag value of tag (external_id=tag_1) from 'Tag 1' to 'Tag 2'\n" + "#4: Update the parent of tag (external_id=tag_4) from parent (external_id=tag_3) " + "to parent (external_id=tag_100).\n" "#5: Create a new tag with values (external_id=tag_33, value=Tag 32, parent_id=None).\n" - "#6: Update the parent of tag (id=27) from parent (external_id=tag_1) to parent (external_id=None).\n" - "#7: Rename tag value of tag (id=27) from 'Tag 2' to 'Tag 31'\n" + "#6: Update the parent of tag (external_id=tag_2) from parent (external_id=tag_1) " + "to parent (external_id=None).\n" + "#7: Rename tag value of tag (external_id=tag_2) from 'Tag 2' to 'Tag 31'\n" "\nOutput errors\n" "--------------------------------\n" "Conflict with 'create' (#2) and action #1: Duplicated external_id tag.\n" - "Action error in 'rename' (#3): Duplicated tag value with tag (id=27).\n" + "Action error in 'rename' (#3): Duplicated tag value with tag in database (external_id=tag_2).\n" "Action error in 'update_parent' (#4): Unknown parent tag (tag_100). " "You need to add parent before the child in your file.\n" "Conflict with 'create' (#5) and action #2: Duplicated tag value.\n" @@ -296,9 +298,10 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "--------------------------------\n" "#1: Create a new tag with values (external_id=tag_31, value=Tag 31, parent_id=None).\n" "#2: Create a new tag with values (external_id=tag_32, value=Tag 32, parent_id=tag_1).\n" - "#3: Rename tag value of tag (id=27) from 'Tag 2' to 'Tag 2 v2'\n" - "#4: Update the parent of tag (id=29) from parent (external_id=tag_3) to parent (external_id=tag_1).\n" - "#5: Rename tag value of tag (id=29) from 'Tag 4' to 'Tag 4 v2'\n" + "#3: Rename tag value of tag (external_id=tag_2) from 'Tag 2' to 'Tag 2 v2'\n" + "#4: Update the parent of tag (external_id=tag_4) from parent (external_id=tag_3) " + "to parent (external_id=tag_1).\n" + "#5: Rename tag value of tag (external_id=tag_4) from 'Tag 4' to 'Tag 4 v2'\n" "#6: No changes needed for tag (external_id=tag_1)\n" ), # Testing valid plan ([ @@ -312,18 +315,27 @@ def test_generate_actions(self, tags, replace, expected_errors, expected_actions "Import plan for Import Taxonomy Test\n" "--------------------------------\n" "#1: No changes needed for tag (external_id=tag_4)\n" - "#2: Update the parent of tag (id=27) from parent (external_id=tag_1) to parent (external_id=None).\n" - "#3: Delete tag (id=26)\n" - "#4: Delete tag (id=27)\n" - "#5: Update the parent of tag (id=29) from parent (external_id=tag_3) to parent (external_id=None).\n" - "#6: Delete tag (id=28)\n" + "#2: Update the parent of tag (external_id=tag_2) from parent (external_id=tag_1) " + "to parent (external_id=None).\n" + "#3: Delete tag (external_id=tag_1)\n" + "#4: Delete tag (external_id=tag_2)\n" + "#5: Update the parent of tag (external_id=tag_4) from parent (external_id=tag_3) " + "to parent (external_id=None).\n" + "#6: Delete tag (external_id=tag_3)\n" ) # Testing deletes (replace=True) ) @ddt.unpack def test_plan(self, tags, replace, expected): + """ + Test the output of plan() function + + It has been decided to verify the output exactly to detect + any error when printing this information that the user is going to read. + """ tags = [TagItem(**tag) for tag in tags] self.import_plan.generate_actions(tags=tags, replace=replace) plan = self.import_plan.plan() + print(plan) assert plan == expected @ddt.data( From 66b17c9a621f2f9cc0e72e087f4c6cc03696d365 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 10 Aug 2023 14:41:31 -0500 Subject: [PATCH 17/20] style: nits and docstring --- .../core/tagging/import_export/actions.py | 24 +++++++++---------- .../core/tagging/import_export/api.py | 10 +++++--- .../core/tagging/import_export/import_plan.py | 2 +- .../core/tagging/import_export/parsers.py | 5 ++-- .../core/tagging/import_export/test_api.py | 4 ++-- .../openedx_tagging/core/tagging/test_api.py | 3 +-- 6 files changed, 26 insertions(+), 22 deletions(-) diff --git a/openedx_tagging/core/tagging/import_export/actions.py b/openedx_tagging/core/tagging/import_export/actions.py index f0ca65cd2..676c801e5 100644 --- a/openedx_tagging/core/tagging/import_export/actions.py +++ b/openedx_tagging/core/tagging/import_export/actions.py @@ -118,12 +118,12 @@ def _validate_value(self, indexed_actions): """ try: # Validates if exists a tag with the same value on the Taxonomy - tag = self.taxonomy.tag_set.get(value=self.tag.value) + taxonomy_tag = self.taxonomy.tag_set.get(value=self.tag.value) return ImportActionError( action=self, tag_id=self.tag.id, message=_( - f"Duplicated tag value with tag in database (external_id={tag.external_id})." + f"Duplicated tag value with tag in database (external_id={taxonomy_tag.external_id})." ), ) except Tag.DoesNotExist: @@ -233,13 +233,13 @@ def execute(self): parent = None if self.tag.parent_id: parent = self.taxonomy.tag_set.get(external_id=self.tag.parent_id) - tag = Tag( + taxonomy_tag = Tag( taxonomy=self.taxonomy, parent=parent, value=self.tag.value, external_id=self.tag.id, ) - tag.save() + taxonomy_tag.save() class UpdateParentTag(ImportAction): @@ -301,12 +301,12 @@ def execute(self): """ Updates the parent of a tag """ - tag = self._get_tag() + taxonomy_tag = self._get_tag() parent = None if self.tag.parent_id: parent = self.taxonomy.tag_set.get(external_id=self.tag.parent_id) - tag.parent = parent - tag.save() + taxonomy_tag.parent = parent + taxonomy_tag.save() class RenameTag(ImportAction): @@ -359,9 +359,9 @@ def execute(self): """ Rename a tag """ - tag = self._get_tag() - tag.value = self.tag.value - tag.save() + taxonomy_tag = self._get_tag() + taxonomy_tag.value = self.tag.value + taxonomy_tag.save() class DeleteTag(ImportAction): @@ -401,8 +401,8 @@ def execute(self): """ Delete a tag """ - tag = self._get_tag() - tag.delete() + taxonomy_tag = self._get_tag() + taxonomy_tag.delete() class WithoutChanges(ImportAction): diff --git a/openedx_tagging/core/tagging/import_export/api.py b/openedx_tagging/core/tagging/import_export/api.py index 5a7893c99..4e26854ed 100644 --- a/openedx_tagging/core/tagging/import_export/api.py +++ b/openedx_tagging/core/tagging/import_export/api.py @@ -66,8 +66,12 @@ def import_tags( It creates an TagImportTask to keep logs of the execution of each import step and the current status. There can only be one task in progress at a time per taxonomy - """ + Set `replace` to True to delete all not readed Tag of the given taxonomy. + Ex. Given a taxonomy with `tag_1`, `tag_2` and `tag_3`. If there is only `tag_1` + in the file (regardless of action), then `tag_2` and `tag_3` will be deleted + if `replace=True` + """ _import_export_validations(taxonomy) # Checks that exists only one task import in progress at a time per taxonomy @@ -171,9 +175,9 @@ def _import_export_validations(taxonomy: Taxonomy): """ taxonomy = taxonomy.cast() if taxonomy.allow_free_text: - raise ValueError( + raise NotImplementedError( _( - f"Invalid taxonomy ({taxonomy.id}): You cannot import/export a free-form taxonomy." + f"Import/export for free-form taxonomies will be implemented in the future." ) ) if taxonomy.system_defined: diff --git a/openedx_tagging/core/tagging/import_export/import_plan.py b/openedx_tagging/core/tagging/import_export/import_plan.py index c8eeeec72..4eac37b1a 100644 --- a/openedx_tagging/core/tagging/import_export/import_plan.py +++ b/openedx_tagging/core/tagging/import_export/import_plan.py @@ -128,7 +128,7 @@ def generate_actions( Validates each action and create respective errors If `replace` is True, then creates the delete action for tags - that has not been readed. + that are in the existing taxonomy but not the new tags list. TODO: Join/reduce actions. Ex. A tag may have no changes, but then its parent needs to be updated because its parent is deleted. diff --git a/openedx_tagging/core/tagging/import_export/parsers.py b/openedx_tagging/core/tagging/import_export/parsers.py index 36c7ab86c..c7fa9d0e8 100644 --- a/openedx_tagging/core/tagging/import_export/parsers.py +++ b/openedx_tagging/core/tagging/import_export/parsers.py @@ -34,8 +34,9 @@ class Parser: """ Base class to create a parser - This contains the base functions to load data, - validate required fields and convert tags to DLS format + This contains the base functions to convert between + a simple file format like CSV/JSON and a list of TagItems. + It can convert in both directions, for use during import or export. If you want to add a new field, you can add it to `required_fields` or `optional_fields` depending on the field type diff --git a/tests/openedx_tagging/core/tagging/import_export/test_api.py b/tests/openedx_tagging/core/tagging/import_export/test_api.py index 6db167689..453979e01 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_api.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_api.py @@ -79,7 +79,7 @@ def test_invalid_import_tags(self): def test_import_export_validations(self): # Check that import is invalid with open taxonomy - with self.assertRaises(ValueError): + with self.assertRaises(NotImplementedError): import_export_api.import_tags( self.open_taxonomy, self.file, @@ -181,7 +181,7 @@ def test_start_task_after_success(self): def test_export_validations(self): # Check that import is invalid with open taxonomy - with self.assertRaises(ValueError): + with self.assertRaises(NotImplementedError): import_export_api.export_tags( self.open_taxonomy, self.parser_format, diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index a361f8507..4fe0d1302 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -77,7 +77,6 @@ def test_get_taxonomies(self): with self.assertNumQueries(1): both = list(tagging_api.get_taxonomies(enabled=None)) - print(both) assert both == [ tax2, tax1, @@ -85,7 +84,7 @@ def test_get_taxonomies(self): self.language_taxonomy, self.taxonomy, self.system_taxonomy, - self.user_taxonomy + self.user_taxonomy, ] @override_settings(LANGUAGES=test_languages) From 8bee009d87dbcf63d1229ee12cfd642fb6ee278a Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 10 Aug 2023 14:57:14 -0500 Subject: [PATCH 18/20] fix: action in TagItem removed --- .../core/tagging/import_export/actions.py | 9 +++----- .../core/tagging/import_export/import_plan.py | 1 - .../core/tagging/import_export/parsers.py | 9 +------- .../tagging/import_export/test_actions.py | 23 +++---------------- .../tagging/import_export/test_parsers.py | 16 ++++--------- 5 files changed, 11 insertions(+), 47 deletions(-) diff --git a/openedx_tagging/core/tagging/import_export/actions.py b/openedx_tagging/core/tagging/import_export/actions.py index 676c801e5..da757cf4b 100644 --- a/openedx_tagging/core/tagging/import_export/actions.py +++ b/openedx_tagging/core/tagging/import_export/actions.py @@ -382,13 +382,10 @@ def __str__(self): @classmethod def applies_for(cls, taxonomy: Taxonomy, tag) -> bool: """ - This action applies whenever the action is delete and the tag exists + This action is an exception. + These actions are created in `TagImportPlan.generate_actions` if `replace=True` """ - try: - taxonomy.tag_set.get(external_id=tag.id) - return tag.action == cls.name - except Tag.DoesNotExist: - return False + return False def validate(self, indexed_actions) -> List[ImportActionError]: """ diff --git a/openedx_tagging/core/tagging/import_export/import_plan.py b/openedx_tagging/core/tagging/import_export/import_plan.py index 4eac37b1a..3af3597d6 100644 --- a/openedx_tagging/core/tagging/import_export/import_plan.py +++ b/openedx_tagging/core/tagging/import_export/import_plan.py @@ -27,7 +27,6 @@ class TagItem: value: str index: Optional[int] = 0 parent_id: Optional[str] = None - action: Optional[str] = None class TagImportPlan: diff --git a/openedx_tagging/core/tagging/import_export/parsers.py b/openedx_tagging/core/tagging/import_export/parsers.py index c7fa9d0e8..79171e7e5 100644 --- a/openedx_tagging/core/tagging/import_export/parsers.py +++ b/openedx_tagging/core/tagging/import_export/parsers.py @@ -45,7 +45,7 @@ class Parser: """ required_fields = ["id", "value"] - optional_fields = ["parent_id", "action"] + optional_fields = ["parent_id"] # Set the format associated to the parser format = None @@ -166,9 +166,6 @@ def _load_tags_for_export(cls, taxonomy: Taxonomy) -> List[dict]: Returns a list of taxonomy's tags in the form of a dictionary with required and optional fields - The 'action' field is not added to the result because the output - is seen as a creation of all the tags. - The tags are ordered by hierarchy, first, parents and then children. `get_tags` is in charge of returning this in a hierarchical way. """ @@ -280,12 +277,8 @@ def _export_data(cls, tags: List[dict], taxonomy: Taxonomy) -> str: """ Export tags in CSV format - The 'action' field is not added to the result because the output - is seen as a creation of all the tags. """ fields = cls.required_fields + cls.optional_fields - if "action" in fields: # pragma: no cover - fields.remove("action") with StringIO() as csv_buffer: csv_writer = csv.DictWriter(csv_buffer, fieldnames=fields) diff --git a/tests/openedx_tagging/core/tagging/import_export/test_actions.py b/tests/openedx_tagging/core/tagging/import_export/test_actions.py index fb70c8c55..1c0b14852 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_actions.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_actions.py @@ -435,29 +435,13 @@ def test_execute(self): assert tag.value == value -@ddt.ddt class TestDeleteTag(TestImportActionMixin, TestCase): """ Test for 'delete' action """ - @ddt.data( - ('tag_10', None, False), # Tag doesn't exist on database - ('tag_1', 'rename', False), # Invalid action - ('tag_1', 'delete', True), # Valid - ) - @ddt.unpack - def test_applies_for(self, tag_id, action, expected): - result = DeleteTag.applies_for( - taxonomy=self.taxonomy, - tag=TagItem( - id=tag_id, - value='_', - action=action, - index=100, - ), - ) - self.assertEqual(result, expected) + def test_applies_for(self): + assert not DeleteTag.applies_for(self.taxonomy, None) def test_validate(self): action = DeleteTag( @@ -469,8 +453,7 @@ def test_validate(self): ), index=100, ) - result = action.validate(self.indexed_actions) - self.assertEqual(result, []) + assert not action.validate(self.indexed_actions) def test_execute(self): tag_id = 'tag_3' diff --git a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py index 15e1faa89..5b77e3a54 100644 --- a/tests/openedx_tagging/core/tagging/import_export/test_parsers.py +++ b/tests/openedx_tagging/core/tagging/import_export/test_parsers.py @@ -128,7 +128,7 @@ def test_parse_tags(self): {"id": "tag_1", "value": "tag 1"}, {"id": "tag_2", "value": "tag 2"}, {"id": "tag_3", "value": "tag 3", "parent_id": "tag_1"}, - {"id": "tag_4", "value": "tag 4", "action": "delete"}, + {"id": "tag_4", "value": "tag 4"}, ] json_data = {"tags": expected_tags} @@ -152,10 +152,6 @@ def test_parse_tags(self): tags[index].parent_id, expected_tag.get('parent_id') ) - self.assertEqual( - tags[index].action, - expected_tag.get('action') - ) self.assertEqual( tags[index].index, index + JSONParser.inital_row @@ -259,11 +255,11 @@ def _build_csv(self, tags): """ Builds a csv from 'tags' dict """ - csv = "id,value,parent_id,action\n" + csv = "id,value,parent_id\n" for tag in tags: csv += ( f"{tag.get('id')},{tag.get('value')}," - f"{tag.get('parent_id') or ''},{tag.get('action') or ''}\n" + f"{tag.get('parent_id') or ''}\n" ) return csv @@ -272,7 +268,7 @@ def test_parse_tags(self): {"id": "tag_1", "value": "tag 1"}, {"id": "tag_2", "value": "tag 2"}, {"id": "tag_3", "value": "tag 3", "parent_id": "tag_1"}, - {"id": "tag_4", "value": "tag 4", "action": "delete"}, + {"id": "tag_4", "value": "tag 4"}, ] csv_data = self._build_csv(expected_tags) csv_file = BytesIO(csv_data.encode()) @@ -295,10 +291,6 @@ def test_parse_tags(self): tags[index].parent_id, expected_tag.get('parent_id') ) - self.assertEqual( - tags[index].action, - expected_tag.get('action') - ) self.assertEqual( tags[index].index, index + CSVParser.inital_row From 2d4087895d72b553286c215e4b861826297b4a53 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 10 Aug 2023 15:05:24 -0500 Subject: [PATCH 19/20] fix: Migration conflicts --- .../core/tagging/migrations/0006_auto_20230802_1631.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_tagging/core/tagging/migrations/0006_auto_20230802_1631.py b/openedx_tagging/core/tagging/migrations/0006_auto_20230802_1631.py index b608f27bf..87bab9cd7 100644 --- a/openedx_tagging/core/tagging/migrations/0006_auto_20230802_1631.py +++ b/openedx_tagging/core/tagging/migrations/0006_auto_20230802_1631.py @@ -7,7 +7,7 @@ class Migration(migrations.Migration): dependencies = [ - ("oel_tagging", "0005_language_taxonomy"), + ("oel_tagging", "0006_alter_objecttag_unique_together"), ] operations = [ From ed8110fa16d59ce305e0b6801cfd6db3787f8584 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Fri, 11 Aug 2023 11:40:18 -0500 Subject: [PATCH 20/20] chore: bump version to 0.1.3 --- openedx_learning/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index b3f475621..ae7362549 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1 +1 @@ -__version__ = "0.1.2" +__version__ = "0.1.3"