From e2b4f021376476ee644eb36248205671a03c2391 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 13 Jun 2023 13:48:06 +0930 Subject: [PATCH 1/7] build: added openedx_tagging to build and test config files --- .coveragerc | 4 +++- MANIFEST.in | 1 + Makefile | 5 +++++ README.rst | 1 + tox.ini | 2 +- 5 files changed, 11 insertions(+), 2 deletions(-) diff --git a/.coveragerc b/.coveragerc index 368d35820..32fc62c81 100644 --- a/.coveragerc +++ b/.coveragerc @@ -1,7 +1,9 @@ [run] branch = True data_file = .coverage -source=openedx_learning +source = + openedx_learning + openedx_tagging omit = test_settings *migrations* diff --git a/MANIFEST.in b/MANIFEST.in index 6ce1f3b20..8cbdc3a65 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -3,3 +3,4 @@ include LICENSE.txt include README.rst include requirements/base.in recursive-include openedx_learning *.html *.png *.gif *.js *.css *.jpg *.jpeg *.svg *.py +recursive-include openedx_tagging *.html *.png *.gif *.js *.css *.jpg *.jpeg *.svg *.py diff --git a/Makefile b/Makefile index 2440de3e1..3b2e8f1eb 100644 --- a/Makefile +++ b/Makefile @@ -77,12 +77,16 @@ extract_translations: ## extract strings to be translated, outputting .mo files rm -rf docs/_build cd openedx_learning && ../manage.py makemessages -l en -v1 -d django cd openedx_learning && ../manage.py makemessages -l en -v1 -d djangojs + cd openedx_tagging && ../manage.py makemessages -l en -v1 -d django + cd openedx_tagging && ../manage.py makemessages -l en -v1 -d djangojs compile_translations: ## compile translation files, outputting .po files for each supported language cd openedx_learning && ../manage.py compilemessages + cd openedx_tagging && ../manage.py compilemessages detect_changed_source_translations: cd openedx_learning && i18n_tool changed + cd openedx_tagging && i18n_tool changed pull_translations: ## pull translations from Transifex tx pull -a -f -t --mode reviewed @@ -92,6 +96,7 @@ push_translations: ## push source translation files (.po) from Transifex dummy_translations: ## generate dummy translation (.po) files cd openedx_learning && i18n_tool dummy + cd openedx_tagging && i18n_tool dummy build_dummy_translations: extract_translations dummy_translations compile_translations ## generate and compile dummy translation files diff --git a/README.rst b/README.rst index 4bf7e06da..5ad362c6b 100644 --- a/README.rst +++ b/README.rst @@ -27,6 +27,7 @@ Parts * ``openedx_learning.lib`` is for shared utilities, and may include things like custom field types, plugin registration code, etc. * ``openedx_learning.core`` contains our Core Django apps, where foundational data structures and APIs will live. +* ``openedx_tagging.core`` contains the core Tagging app, which provides data structures and apis for tagging Open edX objects. App Dependencies ~~~~~~~~~~~~~~~~ diff --git a/tox.ini b/tox.ini index 23e2650ad..ad8e2cb5e 100644 --- a/tox.ini +++ b/tox.ini @@ -31,7 +31,7 @@ match-dir = (?!migrations) [pytest] DJANGO_SETTINGS_MODULE = test_settings -addopts = --cov openedx_learning --cov-report term-missing --cov-report xml +addopts = --cov openedx_learning --cov openedx_tagging --cov-report term-missing --cov-report xml norecursedirs = .* docs requirements site-packages [testenv] From 6c49368639cc5dd5e2a850d52bd7da10824721cf Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 13 Jun 2023 13:49:30 +0930 Subject: [PATCH 2/7] feat: adds Taxonomy, Tag, ObjectTag models, APIs and tests Also: * Adds compile-requirements make target and updates (without upgrading) the requirements, which pulled in some other requirements, since it's been a while since they were compiled. * Adds ddt as a test dependency. * Replaces the placeholder TagContent class, and squashes migrations down to a single file * use the openedx_learning.lib multi-collation char and text fields --- Makefile | 9 +- openedx_tagging/core/tagging/admin.py | 6 +- openedx_tagging/core/tagging/api.py | 109 ++++ openedx_tagging/core/tagging/apps.py | 2 +- .../core/tagging/migrations/0001_initial.py | 207 +++++++- openedx_tagging/core/tagging/models.py | 469 +++++++++++++++++- requirements/dev.txt | 23 + requirements/doc.txt | 2 + requirements/quality.txt | 14 + requirements/test.in | 1 + requirements/test.txt | 2 + .../core/fixtures/tagging.yaml | 156 ++++++ .../openedx_tagging/core/tagging/test_api.py | 190 +++++++ .../core/tagging/test_models.py | 326 +++++++++++- 14 files changed, 1467 insertions(+), 49 deletions(-) create mode 100644 openedx_tagging/core/tagging/api.py create mode 100644 tests/openedx_tagging/core/fixtures/tagging.yaml create mode 100644 tests/openedx_tagging/core/tagging/test_api.py diff --git a/Makefile b/Makefile index 3b2e8f1eb..34eadfc08 100644 --- a/Makefile +++ b/Makefile @@ -30,10 +30,10 @@ docs: ## generate Sphinx HTML documentation, including API docs $(BROWSER)docs/_build/html/index.html # Define PIP_COMPILE_OPTS=-v to get more information during make upgrade. -PIP_COMPILE = pip-compile --rebuild --upgrade $(PIP_COMPILE_OPTS) +PIP_COMPILE = pip-compile --rebuild $(PIP_COMPILE_OPTS) -upgrade: export CUSTOM_COMPILE_COMMAND=make upgrade -upgrade: ## update the requirements/*.txt files with the latest packages satisfying requirements/*.in +compile-requirements: export CUSTOM_COMPILE_COMMAND=make upgrade +compile-requirements: ## update the requirements/*.txt files with the latest packages satisfying requirements/*.in pip install -qr requirements/pip-tools.txt # Make sure to compile files after any other files they include! $(PIP_COMPILE) -o requirements/pip-tools.txt requirements/pip-tools.in @@ -47,6 +47,9 @@ upgrade: ## update the requirements/*.txt files with the latest packages satisfy sed '/^[dD]jango==/d' requirements/test.txt > requirements/test.tmp mv requirements/test.tmp requirements/test.txt +upgrade: ## update the pip requirements files to use the latest releases satisfying our constraints + make compile-requirements PIP_COMPILE_OPTS="--upgrade" + quality: ## check coding style with pycodestyle and pylint tox -e quality diff --git a/openedx_tagging/core/tagging/admin.py b/openedx_tagging/core/tagging/admin.py index e993198a7..91b1d753d 100644 --- a/openedx_tagging/core/tagging/admin.py +++ b/openedx_tagging/core/tagging/admin.py @@ -1,6 +1,8 @@ """ Tagging app admin """ from django.contrib import admin -from .models import TagContent +from .models import ObjectTag, Tag, Taxonomy -admin.site.register(TagContent) +admin.site.register(Taxonomy) +admin.site.register(Tag) +admin.site.register(ObjectTag) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py new file mode 100644 index 000000000..6b9bf2a04 --- /dev/null +++ b/openedx_tagging/core/tagging/api.py @@ -0,0 +1,109 @@ +"""" +Tagging API + +Anyone using the openedx_tagging app should use these APIs instead of creating +or modifying the models directly, since there might be other related model +changes that you may not know about. + +No permissions/rules are enforced by these methods -- these must be enforced in the views. + +Please look at the models.py file for more information about the kinds of data +are stored in this app. +""" +from typing import List, Type + +from django.db.models import QuerySet +from django.utils.translation import gettext_lazy as _ + +from .models import ObjectTag, Tag, Taxonomy + + +def create_taxonomy( + name, + description=None, + enabled=True, + required=False, + allow_multiple=False, + allow_free_text=False, +) -> Taxonomy: + """ + Creates, saves, and returns a new Taxonomy with the given attributes. + """ + return Taxonomy.objects.create( + name=name, + description=description, + enabled=enabled, + required=required, + allow_multiple=allow_multiple, + allow_free_text=allow_free_text, + ) + + +def get_taxonomies(enabled=True) -> QuerySet: + """ + Returns a queryset containing the enabled taxonomies, sorted by name. + If you want the disabled taxonomies, pass enabled=False. + If you want all taxonomies (both enabled and disabled), pass enabled=None. + """ + queryset = Taxonomy.objects.order_by("name", "id") + if enabled is None: + return queryset.all() + return queryset.filter(enabled=enabled) + + +def get_tags(taxonomy: Taxonomy) -> List[Tag]: + """ + Returns a list of predefined tags for the given taxonomy. + + Note that if the taxonomy allows free-text tags, then the returned list will be empty. + """ + return taxonomy.get_tags() + + +def resync_object_tags(object_tags: QuerySet = None) -> int: + """ + Reconciles ObjectTag entries with any changes made to their associated taxonomies and tags. + + By default, we iterate over all ObjectTags. Pass a filtered ObjectTags queryset to limit which tags are resynced. + """ + if not object_tags: + object_tags = ObjectTag.objects.all() + + num_changed = 0 + for object_tag in object_tags: + changed = object_tag.resync() + if changed: + object_tag.save() + num_changed += 1 + return num_changed + + +def get_object_tags( + taxonomy: Taxonomy, object_id: str, object_type: str, valid_only=True +) -> List[ObjectTag]: + """ + Returns a list of tags for a given taxonomy + content. + + Pass valid_only=False when displaying tags to content authors, so they can see invalid tags too. + Invalid tags will likely be hidden from learners. + """ + tags = ObjectTag.objects.filter( + taxonomy=taxonomy, object_id=object_id, object_type=object_type + ).order_by("id") + return [tag for tag in tags if not valid_only or taxonomy.validate_object_tag(tag)] + + +def tag_object( + taxonomy: Taxonomy, tags: List, object_id: str, object_type: str +) -> List[ObjectTag]: + """ + Replaces the existing ObjectTag entries for the given taxonomy + object_id with the given list of tags. + + If taxonomy.allows_free_text, then the list should be a list of tag values. + Otherwise, it should be a list of existing Tag IDs. + + Raised ValueError if the proposed tags are invalid for this taxonomy. + Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. + """ + + return taxonomy.tag_object(tags, object_id, object_type) diff --git a/openedx_tagging/core/tagging/apps.py b/openedx_tagging/core/tagging/apps.py index 2cb90974f..d5877dfee 100644 --- a/openedx_tagging/core/tagging/apps.py +++ b/openedx_tagging/core/tagging/apps.py @@ -12,5 +12,5 @@ class TaggingConfig(AppConfig): name = "openedx_tagging.core.tagging" verbose_name = "Tagging" - default_auto_field = 'django.db.models.BigAutoField' + default_auto_field = "django.db.models.BigAutoField" label = "oel_tagging" diff --git a/openedx_tagging/core/tagging/migrations/0001_initial.py b/openedx_tagging/core/tagging/migrations/0001_initial.py index f3b336ef3..1599f8769 100644 --- a/openedx_tagging/core/tagging/migrations/0001_initial.py +++ b/openedx_tagging/core/tagging/migrations/0001_initial.py @@ -1,23 +1,212 @@ -# Generated by Django 3.2.19 on 2023-05-25 21:08 +# Generated by Django 3.2.19 on 2023-06-22 07:37 +import django.db.models.deletion from django.db import migrations, models +import openedx_learning.lib.fields -class Migration(migrations.Migration): +class Migration(migrations.Migration): initial = True - dependencies = [ - ] + dependencies = [] operations = [ migrations.CreateModel( - name='TagContent', + name="Taxonomy", + fields=[ + ("id", models.BigAutoField(primary_key=True, serialize=False)), + ( + "name", + openedx_learning.lib.fields.MultiCollationCharField( + db_collations={ + "mysql": "utf8mb4_unicode_ci", + "sqlite": "NOCASE", + }, + db_index=True, + help_text="User-facing label used when applying tags from this taxonomy to Open edX objects.", + max_length=255, + ), + ), + ( + "description", + openedx_learning.lib.fields.MultiCollationTextField( + blank=True, + help_text="Provides extra information for the user when applying tags from this taxonomy to an object.", + null=True, + ), + ), + ( + "enabled", + models.BooleanField( + default=True, + help_text="Only enabled taxonomies will be shown to authors.", + ), + ), + ( + "required", + models.BooleanField( + default=False, + help_text="Indicates that one or more tags from this taxonomy must be added to an object.", + ), + ), + ( + "allow_multiple", + models.BooleanField( + default=False, + help_text="Indicates that multiple tags from this taxonomy may be added to an object.", + ), + ), + ( + "allow_free_text", + models.BooleanField( + default=False, + help_text="Indicates that tags in this taxonomy need not be predefined; authors may enter their own tag values.", + ), + ), + ], + options={ + "verbose_name_plural": "Taxonomies", + }, + ), + migrations.CreateModel( + name="Tag", fields=[ - ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('content_id', models.CharField(max_length=255)), - ('name', models.CharField(max_length=255)), - ('value', models.CharField(max_length=765)), + ("id", models.BigAutoField(primary_key=True, serialize=False)), + ( + "value", + openedx_learning.lib.fields.MultiCollationCharField( + db_collations={ + "mysql": "utf8mb4_unicode_ci", + "sqlite": "NOCASE", + }, + help_text="Content of a given tag, occupying the 'value' part of the key:value pair.", + max_length=500, + ), + ), + ( + "external_id", + openedx_learning.lib.fields.MultiCollationCharField( + blank=True, + db_collations={ + "mysql": "utf8mb4_unicode_ci", + "sqlite": "NOCASE", + }, + help_text="Used to link an Open edX Tag with a tag in an externally-defined taxonomy.", + max_length=255, + null=True, + ), + ), + ( + "parent", + models.ForeignKey( + default=None, + help_text="Tag that lives one level up from the current tag, forming a hierarchy.", + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="children", + to="oel_tagging.tag", + ), + ), + ( + "taxonomy", + models.ForeignKey( + default=None, + help_text="Namespace and rules for using a given set of tags.", + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to="oel_tagging.taxonomy", + ), + ), ], ), + migrations.CreateModel( + name="ObjectTag", + fields=[ + ("id", models.BigAutoField(primary_key=True, serialize=False)), + ( + "object_id", + openedx_learning.lib.fields.MultiCollationCharField( + db_collations={ + "mysql": "utf8mb4_unicode_ci", + "sqlite": "NOCASE", + }, + help_text="Identifier for the object being tagged", + max_length=255, + ), + ), + ( + "object_type", + openedx_learning.lib.fields.MultiCollationCharField( + db_collations={ + "mysql": "utf8mb4_unicode_ci", + "sqlite": "NOCASE", + }, + help_text="Type of object being tagged", + max_length=255, + ), + ), + ( + "_name", + openedx_learning.lib.fields.MultiCollationCharField( + db_collations={ + "mysql": "utf8mb4_unicode_ci", + "sqlite": "NOCASE", + }, + help_text="User-facing label used for this tag, stored in case taxonomy is (or becomes) null. If the taxonomy field is set, then taxonomy.name takes precedence over this field.", + max_length=255, + ), + ), + ( + "_value", + openedx_learning.lib.fields.MultiCollationCharField( + db_collations={ + "mysql": "utf8mb4_unicode_ci", + "sqlite": "NOCASE", + }, + help_text="User-facing value used for this tag, stored in case tag is null, e.g if taxonomy is free text, or if it becomes null (e.g. if the Tag is deleted). If the tag field is set, then tag.value takes precedence over this field.", + max_length=500, + ), + ), + ( + "tag", + models.ForeignKey( + default=None, + help_text="Tag associated with this object tag. Provides the tag's 'value' if set.", + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to="oel_tagging.tag", + ), + ), + ( + "taxonomy", + models.ForeignKey( + default=None, + help_text="Taxonomy that this object tag belongs to. Used for validating the tag and provides the tag's 'name' if set.", + null=True, + on_delete=django.db.models.deletion.SET_NULL, + to="oel_tagging.taxonomy", + ), + ), + ], + ), + migrations.AddIndex( + model_name="tag", + index=models.Index( + fields=["taxonomy", "value"], name="oel_tagging_taxonom_89e779_idx" + ), + ), + migrations.AddIndex( + model_name="tag", + index=models.Index( + fields=["taxonomy", "external_id"], + name="oel_tagging_taxonom_44e355_idx", + ), + ), + migrations.AddIndex( + model_name="objecttag", + index=models.Index( + fields=["taxonomy", "_value"], name="oel_tagging_taxonom_3668ec_idx" + ), + ), ] diff --git a/openedx_tagging/core/tagging/models.py b/openedx_tagging/core/tagging/models.py index 8521b65e7..c90f8a34a 100644 --- a/openedx_tagging/core/tagging/models.py +++ b/openedx_tagging/core/tagging/models.py @@ -1,39 +1,458 @@ """ Tagging app data models """ +from typing import List, Type + from django.db import models +from django.utils.translation import gettext_lazy as _ + +from openedx_learning.lib.fields import MultiCollationTextField, case_insensitive_char_field + + +# Maximum depth allowed for a hierarchical taxonomy's tree of tags. +TAXONOMY_MAX_DEPTH = 3 + +# Ancestry of a given tag; the Tag.value fields of a given tag and its parents, starting from the root. +# Will contain 0...TAXONOMY_MAX_DEPTH elements. +Lineage = List[str] + + +class Tag(models.Model): + """ + Represents a single value in a list or tree of values which can be applied to a particular Open edX object. + + Open edX tags are "name:value" pairs which can be applied to objects like content libraries, units, or people. + Tag.taxonomy.name provides the "name" and the Tag.value provides the "value". + (And an ObjectTag links a Tag with an object.) + """ + + id = models.BigAutoField(primary_key=True) + taxonomy = models.ForeignKey( + "Taxonomy", + null=True, + default=None, + on_delete=models.SET_NULL, + help_text=_("Namespace and rules for using a given set of tags."), + ) + parent = models.ForeignKey( + "self", + null=True, + default=None, + on_delete=models.SET_NULL, + related_name="children", + help_text=_( + "Tag that lives one level up from the current tag, forming a hierarchy." + ), + ) + value = case_insensitive_char_field( + max_length=500, + help_text=_( + "Content of a given tag, occupying the 'value' part of the key:value pair." + ), + ) + external_id = case_insensitive_char_field( + max_length=255, + null=True, + blank=True, + help_text=_( + "Used to link an Open edX Tag with a tag in an externally-defined taxonomy." + ), + ) + + class Meta: + indexes = [ + models.Index(fields=["taxonomy", "value"]), + models.Index(fields=["taxonomy", "external_id"]), + ] + + def __repr__(self): + """ + Developer-facing representation of a Tag. + """ + return str(self) + + def __str__(self): + """ + User-facing string representation of a Tag. + """ + return f"Tag ({self.id}) {self.value}" + + def get_lineage(self) -> Lineage: + """ + Queries and returns the lineage of the current tag as a list of Tag.value strings. + + The root Tag.value is first, followed by its child.value, and on down to self.value. + + Performance note: may perform as many as TAXONOMY_MAX_DEPTH select queries. + """ + lineage: Lineage = [] + tag = self + depth = TAXONOMY_MAX_DEPTH + while tag and depth > 0: + lineage.insert(0, tag.value) + tag = tag.parent + depth -= 1 + return lineage -class TagContent(models.Model): +class Taxonomy(models.Model): """ - TODO: This is a basic representation of TagContent, it may change - depending on the discovery in: - https://docs.google.com/document/d/13zfsGDfomSTCp_G-_ncevQHAb4Y4UW0d_6N8R2PdHlA/edit#heading=h.o6fm1hktwp7b + Represents a namespace and rules for a group of tags which can be applied to a particular Open edX object. + """ + + id = models.BigAutoField(primary_key=True) + name = case_insensitive_char_field( + null=False, + max_length=255, + db_index=True, + help_text=_( + "User-facing label used when applying tags from this taxonomy to Open edX objects." + ), + ) + description = MultiCollationTextField( + null=True, + blank=True, + help_text=_( + "Provides extra information for the user when applying tags from this taxonomy to an object." + ), + ) + enabled = models.BooleanField( + default=True, + help_text=_("Only enabled taxonomies will be shown to authors."), + ) + required = models.BooleanField( + default=False, + help_text=_( + "Indicates that one or more tags from this taxonomy must be added to an object." + ), + ) + allow_multiple = models.BooleanField( + default=False, + help_text=_( + "Indicates that multiple tags from this taxonomy may be added to an object." + ), + ) + allow_free_text = models.BooleanField( + default=False, + help_text=_( + "Indicates that tags in this taxonomy need not be predefined; authors may enter their own tag values." + ), + ) + + class Meta: + verbose_name_plural = "Taxonomies" + + @property + def system_defined(self) -> bool: + """ + Base taxonomies are user-defined, not system-defined. + + System-defined taxonomies cannot be edited by ordinary users. + + Subclasses should override this property as required. + """ + return False + + def get_tags(self) -> List[Tag]: + """ + Returns a list of all Tags in the current taxonomy, from the root(s) down to TAXONOMY_MAX_DEPTH tags, in tree order. + + Annotates each returned Tag with its ``depth`` in the tree (starting at 0). + + Performance note: may perform as many as TAXONOMY_MAX_DEPTH select queries. + """ + tags = [] + if self.allow_free_text: + return tags + + parents = None + for depth in range(TAXONOMY_MAX_DEPTH): + filtered_tags = self.tag_set.prefetch_related("parent") + if parents is None: + filtered_tags = filtered_tags.filter(parent=None) + else: + filtered_tags = filtered_tags.filter(parent__in=parents) + next_parents = list( + filtered_tags.annotate( + annotated_field=models.Value( + depth, output_field=models.IntegerField() + ) + ) + .order_by("parent__value", "value", "id") + .all() + ) + tags.extend(next_parents) + parents = next_parents + if not parents: + break + return tags + + def validate_object_tag( + self, + object_tag: "ObjectTag", + check_taxonomy=True, + check_tag=True, + check_object=True, + ) -> bool: + """ + Returns True if the given object tag is valid for the current Taxonomy. + + Subclasses can override this method to perform their own validation checks, e.g. against dynamically generated + tag lists. + + If `check_taxonomy` is False, then we skip validating the object tag's taxonomy reference. + If `check_tag` is False, then we skip validating the object tag's tag reference. + If `check_object` is False, then we skip validating the object ID/type. + """ + # Must be linked to this taxonomy + if check_taxonomy and ( + not object_tag.taxonomy_id or object_tag.taxonomy_id != self.id + ): + return False + + # Must be linked to a Tag unless its a free-text taxonomy + if check_tag and (not self.allow_free_text and not object_tag.tag_id): + return False + + # Must have a valid object id/type: + if check_object and (not object_tag.object_id or not object_tag.object_type): + return False + + return True - This is the representation of a tag that is linked to a content + def tag_object( + self, tags: List, object_id: str, object_type: str + ) -> List["ObjectTag"]: + """ + Replaces the existing ObjectTag entries for the current taxonomy + object_id with the given list of tags. - Name-Value pair - ----------------- + If self.allows_free_text, then the list should be a list of tag values. + Otherwise, it should be a list of existing Tag IDs. - We use a Field-Value Pair representation, where `name` functions - as the constant that defines the field data set, and `value` functions - as the variable tag lineage that belong to the set. + Raised ValueError if the proposed tags are invalid for this taxonomy. + Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. + """ - Value lineage - --------------- + if not self.allow_multiple and len(tags) > 1: + raise ValueError(_(f"Taxonomy ({self.id}) only allows one tag per object.")) - `value` are stored as null character-delimited strings to preserve the tag's lineage. - We use the null character to avoid choosing a character that may exist in a tag's name. - We don't use the Taxonomy's user-facing delimiter so that this delimiter can be changed - without disrupting stored tags. + if self.required and len(tags) == 0: + raise ValueError( + _(f"Taxonomy ({self.id}) requires at least one tag per object.") + ) + + current_tags = { + tag.tag_ref: tag + for tag in ObjectTag.objects.filter( + taxonomy=self, object_id=object_id, object_type=object_type + ) + } + updated_tags = [] + for tag_ref in tags: + if tag_ref in current_tags: + object_tag = current_tags.pop(tag_ref) + else: + object_tag = ObjectTag( + taxonomy=self, + object_id=object_id, + object_type=object_type, + ) + + try: + object_tag.tag = self.tag_set.get( + id=tag_ref, + ) + except (ValueError, Tag.DoesNotExist): + # This might be ok, e.g. if self.allow_free_text. + # We'll validate below before saving. + object_tag.value = tag_ref + + object_tag.resync() + if not self.validate_object_tag(object_tag): + raise ValueError( + _(f"Invalid object tag for taxonomy ({self.id}): {tag_ref}") + ) + updated_tags.append(object_tag) + + # Save all updated tags at once to avoid partial updates + for object_tag in updated_tags: + object_tag.save() + + # ...and delete any omitted existing tags + for old_tag in current_tags.values(): + old_tag.delete() + + return updated_tags + + +class ObjectTag(models.Model): """ + Represents the association between a tag and an Open edX object. + + Tagging content in Open edX involves linking the object to a particular name:value "tag", where the "name" is the + tag's label, and the value is the content of the tag itself. + + Tagging objects can be time-consuming for users, and so we guard against the deletion of Taxonomies and Tags by + providing fields to cache the name:value stored for an object. + + However, sometimes Taxonomy names or Tag values change (e.g if there's a typo, or a policy change about how a label + is used), and so we still store a link to the original Taxonomy and Tag, so that these changes will take precedence + over the original name:value used. + + Also, if an ObjectTag is associated with free-text Taxonomy, then the tag's value won't be stored as a standalone + Tag in the database -- it'll be stored here. + """ + + id = models.BigAutoField(primary_key=True) + object_id = case_insensitive_char_field( + max_length=255, + help_text=_("Identifier for the object being tagged"), + ) + object_type = case_insensitive_char_field( + max_length=255, + help_text=_("Type of object being tagged"), + ) + taxonomy = models.ForeignKey( + Taxonomy, + null=True, + default=None, + on_delete=models.SET_NULL, + help_text=_( + "Taxonomy that this object tag belongs to. Used for validating the tag and provides the tag's 'name' if set." + ), + ) + tag = models.ForeignKey( + Tag, + null=True, + default=None, + on_delete=models.SET_NULL, + help_text=_( + "Tag associated with this object tag. Provides the tag's 'value' if set." + ), + ) + _name = case_insensitive_char_field( + null=False, + max_length=255, + help_text=_( + "User-facing label used for this tag, stored in case taxonomy is (or becomes) null." + " If the taxonomy field is set, then taxonomy.name takes precedence over this field." + ), + ) + _value = case_insensitive_char_field( + null=False, + max_length=500, + help_text=_( + "User-facing value used for this tag, stored in case tag is null, e.g if taxonomy is free text, or if it" + " becomes null (e.g. if the Tag is deleted)." + " If the tag field is set, then tag.value takes precedence over this field." + ), + ) + + class Meta: + indexes = [ + models.Index(fields=["taxonomy", "_value"]), + ] + + @property + def name(self) -> str: + """ + Returns this tag's name/label. + + If taxonomy is set, then returns its name. + Otherwise, returns the cached _name field. + """ + return self.taxonomy.name if self.taxonomy_id else self._name + + @name.setter + def name(self, name: str): + """ + Stores to the _name field. + """ + self._name = name + + @property + def value(self) -> str: + """ + Returns this tag's value. + + If tag is set, then returns its value. + Otherwise, returns the cached _value field. + """ + return self.tag.value if self.tag_id else self._value + + @value.setter + def value(self, value: str): + """ + Stores to the _value field. + """ + self._value = value + + @property + def tag_ref(self) -> str: + """ + Returns this tag's reference string. + + If tag is set, then returns its id. + Otherwise, returns the cached _value field. + """ + return self.tag.id if self.tag_id else self._value + + @property + def is_valid(self) -> bool: + """ + Returns True if this ObjectTag represents a valid taxonomy tag. + + A valid ObjectTag must be linked to a Taxonomy, and be a valid tag in that taxonomy. + """ + return self.taxonomy_id and self.taxonomy.validate_object_tag(self) + + def get_lineage(self) -> Lineage: + """ + Returns the lineage of the current tag as a list of value strings. + + If linked to a Tag, returns its lineage. + Otherwise, returns an array containing its value string. + """ + return self.tag.get_lineage() if self.tag_id else [self._value] + + def resync(self) -> bool: + """ + Reconciles the stored ObjectTag properties with any changes made to its associated taxonomy or tag. + + This method is useful to propagate changes to a Taxonomy name or Tag value. + + It's also useful for a set of ObjectTags are imported from an external source prior to when a Taxonomy exists to + validate or store its available Tags. + + Returns True if anything was changed, False otherwise. + """ + changed = False + + # Locate a taxonomy matching _name + if not self.taxonomy_id: + for taxonomy in Taxonomy.objects.filter(name=self.name, enabled=True): + # Make sure this taxonomy will accept object tags like this. + self.taxonomy = taxonomy + if taxonomy.validate_object_tag(self, check_tag=False): + changed = True + break + # If not, try the next one + else: + self.taxonomy = None + + # Sync the stored _name with the taxonomy.name + elif self._name != self.taxonomy.name: + self.name = self.taxonomy.name + changed = True - # Content id to which this tag is associated - content_id = models.CharField(max_length=255) + # Locate a tag matching _value + if self.taxonomy and not self.tag_id and not self.taxonomy.allow_free_text: + tag = self.taxonomy.tag_set.filter(value=self.value).first() + if tag: + self.tag = tag + changed = True - # It's not usually large - name = models.CharField(max_length=255) + # Sync the stored _value with the tag.name + elif self.tag and self._value != self.tag.value: + self.value = self.tag.value + changed = True - # Tag lineage value. - # - # The lineage can be large. - # TODO: The length is under discussion - value = models.CharField(max_length=765) + return changed diff --git a/requirements/dev.txt b/requirements/dev.txt index b2dcdaf2e..e816277f9 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -25,6 +25,10 @@ certifi==2023.5.7 # via # -r requirements/quality.txt # requests +cffi==1.15.1 + # via + # -r requirements/quality.txt + # cryptography chardet==5.1.0 # via diff-cover charset-normalizer==3.1.0 @@ -53,6 +57,12 @@ coverage[toml]==7.2.7 # via # -r requirements/quality.txt # pytest-cov +cryptography==41.0.1 + # via + # -r requirements/quality.txt + # secretstorage +ddt==1.6.0 + # via -r requirements/quality.txt diff-cover==7.6.0 # via -r requirements/dev.in dill==0.3.6 @@ -125,6 +135,11 @@ jaraco-classes==3.2.3 # via # -r requirements/quality.txt # keyring +jeepney==0.8.0 + # via + # -r requirements/quality.txt + # keyring + # secretstorage jinja2==3.1.2 # via # -r requirements/quality.txt @@ -201,6 +216,10 @@ py==1.11.0 # tox pycodestyle==2.10.0 # via -r requirements/quality.txt +pycparser==2.21 + # via + # -r requirements/quality.txt + # cffi pydocstyle==6.3.0 # via -r requirements/quality.txt pygments==2.15.1 @@ -277,6 +296,10 @@ rich==13.4.2 # via # -r requirements/quality.txt # twine +secretstorage==3.3.3 + # via + # -r requirements/quality.txt + # keyring six==1.16.0 # via # -r requirements/ci.txt diff --git a/requirements/doc.txt b/requirements/doc.txt index d475be069..b4e6b5d5b 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -35,6 +35,8 @@ coverage[toml]==7.2.7 # via # -r requirements/test.txt # pytest-cov +ddt==1.6.0 + # via -r requirements/test.txt django==3.2.19 # via # -c requirements/constraints.txt diff --git a/requirements/quality.txt b/requirements/quality.txt index ab8f3977d..c70f66a72 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -16,6 +16,8 @@ bleach==6.0.0 # via readme-renderer certifi==2023.5.7 # via requests +cffi==1.15.1 + # via cryptography charset-normalizer==3.1.0 # via requests click==8.1.3 @@ -35,6 +37,10 @@ coverage[toml]==7.2.7 # via # -r requirements/test.txt # pytest-cov +cryptography==41.0.1 + # via secretstorage +ddt==1.6.0 + # via -r requirements/test.txt dill==0.3.6 # via pylint django==3.2.19 @@ -76,6 +82,10 @@ isort==5.12.0 # pylint jaraco-classes==3.2.3 # via keyring +jeepney==0.8.0 + # via + # keyring + # secretstorage jinja2==3.1.2 # via # -r requirements/test.txt @@ -116,6 +126,8 @@ pluggy==1.0.0 # pytest pycodestyle==2.10.0 # via -r requirements/quality.in +pycparser==2.21 + # via cffi pydocstyle==6.3.0 # via -r requirements/quality.in pygments==2.15.1 @@ -170,6 +182,8 @@ rfc3986==2.0.0 # via twine rich==13.4.2 # via twine +secretstorage==3.3.3 + # via keyring six==1.16.0 # via # bleach diff --git a/requirements/test.in b/requirements/test.in index 48446899b..6b4e096d4 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -12,3 +12,4 @@ pytest pytest-cov # pytest extension for code coverage statistics pytest-django # pytest extension for better Django support code-annotations # provides commands used by the pii_check make target. +ddt # supports data driven tests diff --git a/requirements/test.txt b/requirements/test.txt index 8865d16b8..dafcddb06 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -18,6 +18,8 @@ coverage[toml]==7.2.7 # via # -r requirements/test.in # pytest-cov +ddt==1.6.0 + # via -r requirements/test.in # via # -c requirements/constraints.txt # -r requirements/base.txt diff --git a/tests/openedx_tagging/core/fixtures/tagging.yaml b/tests/openedx_tagging/core/fixtures/tagging.yaml new file mode 100644 index 000000000..50b9459bb --- /dev/null +++ b/tests/openedx_tagging/core/fixtures/tagging.yaml @@ -0,0 +1,156 @@ +- model: oel_tagging.tag + pk: 1 + fields: + taxonomy: 1 + parent: null + value: Bacteria + external_id: null +- model: oel_tagging.tag + pk: 2 + fields: + taxonomy: 1 + parent: null + value: Archaea + external_id: null +- model: oel_tagging.tag + pk: 3 + fields: + taxonomy: 1 + parent: null + value: Eukaryota + external_id: null +- model: oel_tagging.tag + pk: 4 + fields: + taxonomy: 1 + parent: 1 + value: Eubacteria + external_id: null +- model: oel_tagging.tag + pk: 5 + fields: + taxonomy: 1 + parent: 1 + value: Archaebacteria + external_id: null +- model: oel_tagging.tag + pk: 6 + fields: + taxonomy: 1 + parent: 2 + value: DPANN + external_id: null +- model: oel_tagging.tag + pk: 7 + fields: + taxonomy: 1 + parent: 2 + value: Euryarchaeida + external_id: null +- model: oel_tagging.tag + pk: 8 + fields: + taxonomy: 1 + parent: 2 + value: Proteoarchaeota + external_id: null +- model: oel_tagging.tag + pk: 9 + fields: + taxonomy: 1 + parent: 3 + value: Animalia + external_id: null +- model: oel_tagging.tag + pk: 10 + fields: + taxonomy: 1 + parent: 3 + value: Plantae + external_id: null +- model: oel_tagging.tag + pk: 11 + fields: + taxonomy: 1 + parent: 3 + value: Fungi + external_id: null +- model: oel_tagging.tag + pk: 12 + fields: + taxonomy: 1 + parent: 3 + value: Protista + external_id: null +- model: oel_tagging.tag + pk: 13 + fields: + taxonomy: 1 + parent: 3 + value: Monera + external_id: null +- model: oel_tagging.tag + pk: 14 + fields: + taxonomy: 1 + parent: 9 + value: Arthropoda + external_id: null +- model: oel_tagging.tag + pk: 15 + fields: + taxonomy: 1 + parent: 9 + value: Chordata + external_id: null +- model: oel_tagging.tag + pk: 16 + fields: + taxonomy: 1 + parent: 9 + value: Gastrotrich + external_id: null +- model: oel_tagging.tag + pk: 17 + fields: + taxonomy: 1 + parent: 9 + value: Cnidaria + external_id: null +- model: oel_tagging.tag + pk: 18 + fields: + taxonomy: 1 + parent: 9 + value: Ctenophora + external_id: null +- model: oel_tagging.tag + pk: 19 + fields: + taxonomy: 1 + parent: 9 + value: Placozoa + external_id: null +- model: oel_tagging.tag + pk: 20 + fields: + taxonomy: 1 + parent: 9 + value: Porifera + external_id: null +- model: oel_tagging.tag + pk: 21 + fields: + taxonomy: 1 + parent: 15 + value: Mammalia + external_id: null +- model: oel_tagging.taxonomy + pk: 1 + fields: + name: Life on Earth + description: null + enabled: true + required: false + allow_multiple: false + allow_free_text: false diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py new file mode 100644 index 000000000..7a418e687 --- /dev/null +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -0,0 +1,190 @@ +""" Test the tagging APIs """ + +from unittest.mock import patch + +from django.test.testcases import TestCase + +import openedx_tagging.core.tagging.api as tagging_api +from openedx_tagging.core.tagging.models import ObjectTag, Tag + +from .test_models import TestTagTaxonomyMixin + + +class TestApiTagging(TestTagTaxonomyMixin, TestCase): + """ + Test the Tagging API methods. + """ + + def test_create_taxonomy(self): + params = { + "name": "Difficulty", + "description": "This taxonomy contains tags describing the difficulty of an activity", + "enabled": False, + "required": True, + "allow_multiple": True, + "allow_free_text": True, + } + taxonomy = tagging_api.create_taxonomy(**params) + for param, value in params.items(): + assert getattr(taxonomy, param) == value + + def test_get_taxonomies(self): + tax1 = tagging_api.create_taxonomy("Enabled") + tax2 = tagging_api.create_taxonomy("Disabled", enabled=False) + enabled = tagging_api.get_taxonomies() + assert list(enabled) == [tax1, self.taxonomy] + + disabled = tagging_api.get_taxonomies(enabled=False) + assert list(disabled) == [tax2] + + both = tagging_api.get_taxonomies(enabled=None) + assert list(both) == [tax2, tax1, self.taxonomy] + + def test_get_tags(self): + self.setup_tag_depths() + assert tagging_api.get_tags(self.taxonomy) == [ + *self.domain_tags, + *self.kingdom_tags, + *self.phylum_tags, + ] + + def check_object_tag(self, object_tag, taxonomy, tag, name, value): + """ + Verifies that the properties of the given object_tag (once refreshed from the database) match those given. + """ + object_tag.refresh_from_db() + assert object_tag.taxonomy == taxonomy + assert object_tag.tag == tag + assert object_tag.name == name + assert object_tag.value == value + + def test_resync_object_tags(self): + missing_links = ObjectTag(object_id="abc", object_type="alpha") + missing_links.name = self.taxonomy.name + missing_links.value = self.mammalia.value + missing_links.save() + changed_links = ObjectTag( + object_id="def", + object_type="alpha", + taxonomy=self.taxonomy, + tag=self.mammalia, + ) + changed_links.name = "Life" + changed_links.value = "Animals" + changed_links.save() + + no_changes = ObjectTag( + object_id="ghi", + object_type="beta", + taxonomy=self.taxonomy, + tag=self.mammalia, + ) + no_changes.name = self.taxonomy.name + no_changes.value = self.mammalia.value + no_changes.save() + + changed = tagging_api.resync_object_tags() + assert changed == 2 + for object_tag in (missing_links, changed_links, no_changes): + self.check_object_tag( + object_tag, self.taxonomy, self.mammalia, "Life on Earth", "Mammalia" + ) + + # Once all tags are resynced, they stay that way + changed = tagging_api.resync_object_tags() + assert changed == 0 + + # ObjectTag value preserved even if linked tag is deleted + self.mammalia.delete() + for object_tag in (missing_links, changed_links, no_changes): + self.check_object_tag( + object_tag, self.taxonomy, None, "Life on Earth", "Mammalia" + ) + + # ObjectTag name preserved even if linked taxonomy is deleted + self.taxonomy.delete() + for object_tag in (missing_links, changed_links, no_changes): + self.check_object_tag(object_tag, None, None, "Life on Earth", "Mammalia") + + # Resyncing the tags for code coverage + changed = tagging_api.resync_object_tags() + assert changed == 0 + + # Recreate the taxonomy and resync some tags + first_taxonomy = tagging_api.create_taxonomy("Life on Earth") + second_taxonomy = tagging_api.create_taxonomy("Life on Earth") + new_tag = Tag.objects.create( + value="Mammalia", + taxonomy=second_taxonomy, + ) + + with patch( + "openedx_tagging.core.tagging.models.Taxonomy.validate_object_tag", + side_effect=[False, True, False, True], + ): + changed = tagging_api.resync_object_tags( + ObjectTag.objects.filter(object_type="alpha") + ) + assert changed == 2 + for object_tag in (missing_links, changed_links): + self.check_object_tag( + object_tag, second_taxonomy, new_tag, "Life on Earth", "Mammalia" + ) + + # Ensure the omitted tag was not updated + self.check_object_tag(no_changes, None, None, "Life on Earth", "Mammalia") + + # Update that one too (without the patching) + changed = tagging_api.resync_object_tags( + ObjectTag.objects.filter(object_type="beta") + ) + assert changed == 1 + self.check_object_tag( + no_changes, first_taxonomy, None, "Life on Earth", "Mammalia" + ) + + def test_tag_object(self): + self.taxonomy.allow_multiple = True + test_tags = [ + [ + self.archaea.id, + self.eubacteria.id, + self.chordata.id, + ], + [ + self.chordata.id, + self.archaebacteria.id, + ], + [ + self.archaebacteria.id, + self.archaea.id, + ], + ] + + # Tag and re-tag the object, checking that the expected tags are returned and deleted + for tag_list in test_tags: + object_tags = tagging_api.tag_object( + self.taxonomy, + tag_list, + "biology101", + "course", + ) + + # Ensure the expected number of tags exist in the database + assert ( + tagging_api.get_object_tags( + taxonomy=self.taxonomy, + object_id="biology101", + object_type="course", + ) + == object_tags + ) + # And the expected number of tags were returned + assert len(object_tags) == len(tag_list) + for index, object_tag in enumerate(object_tags): + assert object_tag.tag_id == tag_list[index] + assert object_tag.is_valid + assert object_tag.taxonomy == self.taxonomy + assert object_tag.name == self.taxonomy.name + assert object_tag.object_id == "biology101" + assert object_tag.object_type == "course" diff --git a/tests/openedx_tagging/core/tagging/test_models.py b/tests/openedx_tagging/core/tagging/test_models.py index d1a4d76a4..d74c041db 100644 --- a/tests/openedx_tagging/core/tagging/test_models.py +++ b/tests/openedx_tagging/core/tagging/test_models.py @@ -1,17 +1,325 @@ +""" Test the tagging models """ + +import ddt from django.test.testcases import TestCase -from openedx_tagging.core.tagging.models import TagContent +from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy + + +def get_tag(value): + """ + Fetches and returns the tag with the given value. + """ + return Tag.objects.get(value=value) + + +class TestTagTaxonomyMixin: + """ + Base class that uses the taxonomy fixture to load a base taxonomy and tags for testing. + """ + + fixtures = ["tests/openedx_tagging/core/fixtures/tagging.yaml"] + + def setUp(self): + super().setUp() + self.taxonomy = Taxonomy.objects.get(name="Life on Earth") + self.archaea = get_tag("Archaea") + self.archaebacteria = get_tag("Archaebacteria") + self.bacteria = get_tag("Bacteria") + self.eubacteria = get_tag("Eubacteria") + self.chordata = get_tag("Chordata") + self.mammalia = get_tag("Mammalia") + # Domain tags (depth=0) + # https://en.wikipedia.org/wiki/Domain_(biology) + self.domain_tags = [ + get_tag("Archaea"), + get_tag("Bacteria"), + get_tag("Eukaryota"), + ] + # Kingdom tags (depth=1) + self.kingdom_tags = [ + # Kingdoms of https://en.wikipedia.org/wiki/Archaea + get_tag("DPANN"), + get_tag("Euryarchaeida"), + get_tag("Proteoarchaeota"), + # Kingdoms of https://en.wikipedia.org/wiki/Bacterial_taxonomy + get_tag("Archaebacteria"), + get_tag("Eubacteria"), + # Kingdoms of https://en.wikipedia.org/wiki/Eukaryote + get_tag("Animalia"), + get_tag("Fungi"), + get_tag("Monera"), + get_tag("Plantae"), + get_tag("Protista"), + ] + # Phylum tags (depth=2) + self.phylum_tags = [ + # Some phyla of https://en.wikipedia.org/wiki/Animalia + get_tag("Arthropoda"), + get_tag("Chordata"), + get_tag("Cnidaria"), + get_tag("Ctenophora"), + get_tag("Gastrotrich"), + get_tag("Placozoa"), + get_tag("Porifera"), + ] -class TestModelTagContent(TestCase): + def setup_tag_depths(self): + """ + Annotate our tags with depth so we can compare them. + """ + for tag in self.domain_tags: + tag.depth = 0 + for tag in self.kingdom_tags: + tag.depth = 1 + for tag in self.phylum_tags: + tag.depth = 2 + + +@ddt.ddt +class TestModelTagTaxonomy(TestTagTaxonomyMixin, TestCase): """ - Test that TagContent objects can be created and edited. + Test the Tag and Taxonomy models' properties and methods. """ - def test_tag_content(self): - content_tag = TagContent.objects.create( - content_id="lb:Axim:video:abc", - name="Subject areas", - value="Chemistry", + def test_system_defined(self): + assert not self.taxonomy.system_defined + + def test_representations(self): + assert str(self.bacteria) == "Tag (1) Bacteria" + assert repr(self.bacteria) == "Tag (1) Bacteria" + + @ddt.data( + # Root tags just return their own value + ("bacteria", ["Bacteria"]), + # Second level tags return two levels + ("eubacteria", ["Bacteria", "Eubacteria"]), + # Third level tags return three levels + ("chordata", ["Eukaryota", "Animalia", "Chordata"]), + # Lineage beyond TAXONOMY_MAX_DEPTH won't trace back to the root + ("mammalia", ["Animalia", "Chordata", "Mammalia"]), + ) + @ddt.unpack + def test_get_lineage(self, tag_attr, lineage): + assert getattr(self, tag_attr).get_lineage() == lineage + + def test_get_tags(self): + self.setup_tag_depths() + assert self.taxonomy.get_tags() == [ + *self.domain_tags, + *self.kingdom_tags, + *self.phylum_tags, + ] + + def test_get_tags_free_text(self): + self.taxonomy.allow_free_text = True + with self.assertNumQueries(0): + assert self.taxonomy.get_tags() == [] + + def test_get_tags_shallow_taxonomy(self): + taxonomy = Taxonomy.objects.create(name="Difficulty") + tags = [ + Tag.objects.create(taxonomy=taxonomy, value="1. Easy"), + Tag.objects.create(taxonomy=taxonomy, value="2. Moderate"), + Tag.objects.create(taxonomy=taxonomy, value="3. Hard"), + ] + with self.assertNumQueries(2): + assert taxonomy.get_tags() == tags + + +class TestModelObjectTag(TestTagTaxonomyMixin, TestCase): + """ + Test the ObjectTag model and the related Taxonomy methods and fields. + """ + + def setUp(self): + super().setUp() + self.tag = self.bacteria + + def test_object_tag_name(self): + # ObjectTag's name defaults to its taxonomy's name + object_tag = ObjectTag.objects.create( + object_id="object:id", + object_type="any_old_object", + taxonomy=self.taxonomy, + ) + assert object_tag.name == self.taxonomy.name + + # Even if we overwrite the name, it still uses the taxonomy's name + object_tag.name = "Another tag" + assert object_tag.name == self.taxonomy.name + object_tag.save() + assert object_tag.name == self.taxonomy.name + + # But if the taxonomy is deleted, then the object_tag's name reverts to our cached name + self.taxonomy.delete() + object_tag.refresh_from_db() + assert object_tag.name == "Another tag" + + def test_object_tag_value(self): + # ObjectTag's value defaults to its tag's value + object_tag = ObjectTag.objects.create( + object_id="object:id", + object_type="any_old_object", + taxonomy=self.taxonomy, + tag=self.tag, + ) + assert object_tag.value == self.tag.value + + # Even if we overwrite the value, it still uses the tag's value + object_tag.value = "Another tag" + assert object_tag.value == self.tag.value + object_tag.save() + assert object_tag.value == self.tag.value + + # But if the tag is deleted, then the object_tag's value reverts to our cached value + self.tag.delete() + object_tag.refresh_from_db() + assert object_tag.value == "Another tag" + + def test_object_tag_lineage(self): + # ObjectTag's value defaults to its tag's lineage + object_tag = ObjectTag.objects.create( + object_id="object:id", + object_type="any_old_object", + taxonomy=self.taxonomy, + tag=self.tag, + ) + assert object_tag.get_lineage() == self.tag.get_lineage() + + # Even if we overwrite the value, it still uses the tag's lineage + object_tag.value = "Another tag" + assert object_tag.get_lineage() == self.tag.get_lineage() + object_tag.save() + assert object_tag.get_lineage() == self.tag.get_lineage() + + # But if the tag is deleted, then the object_tag's lineage reverts to our cached value + self.tag.delete() + object_tag.refresh_from_db() + assert object_tag.get_lineage() == ["Another tag"] + + def test_object_tag_is_valid(self): + object_tag = ObjectTag( + object_id="object:id", + object_type="any_old_object", + ) + assert not object_tag.is_valid + + object_tag.taxonomy = self.taxonomy + assert not object_tag.is_valid + + object_tag.tag = self.tag + assert object_tag.is_valid + + # or, we can have no tag, and a free-text taxonomy + object_tag.tag = None + self.taxonomy.allow_free_text = True + assert object_tag.is_valid + + def test_validate_object_tag_invalid(self): + taxonomy = Taxonomy.objects.create( + name="Another taxonomy", ) - assert content_tag.id + object_tag = ObjectTag( + taxonomy=self.taxonomy, + ) + assert not taxonomy.validate_object_tag(object_tag) + + object_tag.taxonomy = taxonomy + assert not taxonomy.validate_object_tag(object_tag) + + taxonomy.allow_free_text = True + assert not taxonomy.validate_object_tag(object_tag) + + object_tag.object_id = "object:id" + object_tag.object_type = "object:type" + assert taxonomy.validate_object_tag(object_tag) + + def test_tag_object(self): + self.taxonomy.allow_multiple = True + + test_tags = [ + [ + self.archaea.id, + self.eubacteria.id, + self.chordata.id, + ], + [ + self.archaebacteria.id, + self.chordata.id, + ], + [ + self.archaea.id, + self.archaebacteria.id, + ], + ] + + # Tag and re-tag the object, checking that the expected tags are returned and deleted + for tag_list in test_tags: + object_tags = self.taxonomy.tag_object( + tag_list, + "biology101", + "course", + ) + + # Ensure the expected number of tags exist in the database + assert ObjectTag.objects.filter( + taxonomy=self.taxonomy, + object_id="biology101", + object_type="course", + ).count() == len(tag_list) + # And the expected number of tags were returned + assert len(object_tags) == len(tag_list) + for index, object_tag in enumerate(object_tags): + assert object_tag.tag_id == tag_list[index] + assert object_tag.is_valid + assert object_tag.taxonomy == self.taxonomy + assert object_tag.name == self.taxonomy.name + assert object_tag.object_id == "biology101" + assert object_tag.object_type == "course" + + def test_tag_object_free_text(self): + self.taxonomy.allow_free_text = True + object_tags = self.taxonomy.tag_object( + ["Eukaryota Xenomorph"], + "biology101", + "course", + ) + assert len(object_tags) == 1 + object_tag = object_tags[0] + assert object_tag.is_valid + assert object_tag.taxonomy == self.taxonomy + assert object_tag.name == self.taxonomy.name + assert object_tag.tag_ref == "Eukaryota Xenomorph" + assert object_tag.get_lineage() == ["Eukaryota Xenomorph"] + assert object_tag.object_id == "biology101" + assert object_tag.object_type == "course" + + def test_tag_object_no_multiple(self): + with self.assertRaises(ValueError) as exc: + self.taxonomy.tag_object( + ["A", "B"], + "biology101", + "course", + ) + assert "only allows one tag per object" in str(exc.exception) + + def test_tag_object_required(self): + self.taxonomy.required = True + with self.assertRaises(ValueError) as exc: + self.taxonomy.tag_object( + [], + "biology101", + "course", + ) + assert "requires at least one tag per object" in str(exc.exception) + + def test_tag_object_invalid_tag(self): + with self.assertRaises(ValueError) as exc: + self.taxonomy.tag_object( + ["Eukaryota Xenomorph"], + "biology101", + "course", + ) + assert "Invalid object tag for taxonomy" in str(exc.exception) From 92f739cca4f2a64468bd7683efbd15615d7c41df Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 15 Jun 2023 11:12:37 +0930 Subject: [PATCH 3/7] feat: adds django-rules based permissions for tagging app Also: * Adds rules requirement and app settings to enable it * Adds mock to test requirements, so we can test system taxonomy rules * ADR: Clarifies that rules will be enforced in the views, not the model or APIs --- .../decisions/0009-tagging-administrators.rst | 3 + openedx_tagging/core/tagging/rules.py | 74 ++++++ projects/dev.py | 6 + requirements/base.in | 2 + requirements/base.txt | 2 + requirements/dev.txt | 4 + requirements/doc.txt | 4 + requirements/quality.txt | 4 + requirements/test.in | 1 + requirements/test.txt | 4 + test_settings.py | 6 + .../core/tagging/test_rules.py | 230 ++++++++++++++++++ 12 files changed, 340 insertions(+) create mode 100644 openedx_tagging/core/tagging/rules.py create mode 100644 tests/openedx_tagging/core/tagging/test_rules.py diff --git a/docs/decisions/0009-tagging-administrators.rst b/docs/decisions/0009-tagging-administrators.rst index 652dcd243..33b0ab523 100644 --- a/docs/decisions/0009-tagging-administrators.rst +++ b/docs/decisions/0009-tagging-administrators.rst @@ -26,6 +26,8 @@ But because permissions #2 + #3 require access to the edx-platform CMS model `Co Per `OEP-9`_, ``openedx_tagging`` will allow applications to use the standard Django API to query permissions, for example: ``user.has_perm('openedx_tagging.edit_taxonomy', taxonomy)``, and the appropriate permissions will be applied in that application's context. +These rules will be enforced in the tagging `views`_, not the API or models, so that external code using this library need not have a logged-in user in order to call the API. So please use with care. + Rejected Alternatives --------------------- @@ -37,3 +39,4 @@ This is a standard way to grant access in Django apps, but it is not used in Ope .. _get_organizations: https://github.com/openedx/edx-platform/blob/4dc35c73ffa6d6a1dcb6e9ea1baa5bed40721125/cms/djangoapps/contentstore/views/course.py#L1958 .. _CourseCreator: https://github.com/openedx/edx-platform/blob/4dc35c73ffa6d6a1dcb6e9ea1baa5bed40721125/cms/djangoapps/course_creators/models.py#L27 .. _OEP-9: https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0009-bp-permissions.html +.. _views: https://github.com/dfunckt/django-rules#permissions-in-views diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py new file mode 100644 index 000000000..7ef984b86 --- /dev/null +++ b/openedx_tagging/core/tagging/rules.py @@ -0,0 +1,74 @@ +"""Django rules-based permissions for tagging""" + +import rules + +# Global staff are taxonomy admins. +# (Superusers can already do anything) +is_taxonomy_admin = rules.is_staff + + +@rules.predicate +def can_view_taxonomy(user, taxonomy=None): + """ + Anyone can view an enabled taxonomy, + but only taxonomy admins can view a disabled taxonomy. + """ + return (taxonomy and taxonomy.enabled) or is_taxonomy_admin(user) + + +@rules.predicate +def can_change_taxonomy(user, taxonomy=None): + """ + Even taxonomy admins cannot change system taxonomies. + """ + return is_taxonomy_admin(user) and ( + not taxonomy or not taxonomy or (taxonomy and not taxonomy.system_defined) + ) + + +@rules.predicate +def can_change_taxonomy_tag(user, tag=None): + """ + Even taxonomy admins cannot add tags to system taxonomies (their tags are system-defined), or free-text taxonomies + (these don't have predefined tags). + """ + return is_taxonomy_admin(user) and ( + not tag + or not tag.taxonomy + or ( + tag.taxonomy + and not tag.taxonomy.allow_free_text + and not tag.taxonomy.system_defined + ) + ) + + +@rules.predicate +def can_change_object_tag(user, object_tag=None): + """ + Taxonomy admins can create or modify object tags on enabled taxonomies. + """ + return is_taxonomy_admin(user) and ( + not object_tag + or not object_tag.taxonomy + or (object_tag.taxonomy and object_tag.taxonomy.enabled) + ) + + +# Taxonomy +rules.add_perm("oel_tagging.add_taxonomy", can_change_taxonomy) +rules.add_perm("oel_tagging.change_taxonomy", can_change_taxonomy) +rules.add_perm("oel_tagging.delete_taxonomy", can_change_taxonomy) +rules.add_perm("oel_tagging.view_taxonomy", can_view_taxonomy) + +# Tag +rules.add_perm("oel_tagging.add_tag", can_change_taxonomy_tag) +rules.add_perm("oel_tagging.change_tag", can_change_taxonomy_tag) +rules.add_perm("oel_tagging.delete_tag", is_taxonomy_admin) +rules.add_perm("oel_tagging.view_tag", rules.always_allow) + +# ObjectTag +rules.add_perm("oel_tagging.add_object_tag", can_change_object_tag) +rules.add_perm("oel_tagging.change_object_tag", can_change_object_tag) +rules.add_perm("oel_tagging.delete_object_tag", is_taxonomy_admin) +rules.add_perm("oel_tagging.view_object_tag", rules.always_allow) diff --git a/projects/dev.py b/projects/dev.py index 99903c3f7..a96473d4b 100644 --- a/projects/dev.py +++ b/projects/dev.py @@ -41,6 +41,8 @@ # REST API "rest_framework", "openedx_learning.rest_api.apps.RESTAPIConfig", + # django-rules based authorization + 'rules.apps.AutodiscoverRulesConfig', # Tagging Core Apps "openedx_tagging.core.tagging.apps.TaggingConfig", @@ -48,6 +50,10 @@ "debug_toolbar", ) +AUTHENTICATION_BACKENDS = [ + 'rules.permissions.ObjectPermissionBackend', +] + MIDDLEWARE = [ "debug_toolbar.middleware.DebugToolbarMiddleware", "django.middleware.security.SecurityMiddleware", diff --git a/requirements/base.in b/requirements/base.in index aa00dddd7..d17e06397 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -4,3 +4,5 @@ Django<5.0 # Web application framework djangorestframework<4.0 # REST API + +rules<4.0 # Django extension for rules-based authorization checks diff --git a/requirements/base.txt b/requirements/base.txt index 97ec03540..8c0f00b63 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -17,6 +17,8 @@ pytz==2023.3 # via # django # djangorestframework +rules==3.3 + # via -r requirements/base.in sqlparse==0.4.4 # via django typing-extensions==4.6.3 diff --git a/requirements/dev.txt b/requirements/dev.txt index e816277f9..add9cf1de 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -169,6 +169,8 @@ mdurl==0.1.2 # via # -r requirements/quality.txt # markdown-it-py +mock==5.0.2 + # via -r requirements/quality.txt more-itertools==9.1.0 # via # -r requirements/quality.txt @@ -296,6 +298,8 @@ rich==13.4.2 # via # -r requirements/quality.txt # twine +rules==3.3 + # via -r requirements/quality.txt secretstorage==3.3.3 # via # -r requirements/quality.txt diff --git a/requirements/doc.txt b/requirements/doc.txt index b4e6b5d5b..d9a4c3c2a 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -83,6 +83,8 @@ markupsafe==2.1.3 # via # -r requirements/test.txt # jinja2 +mock==5.0.2 + # via -r requirements/test.txt mysqlclient==2.1.1 # via -r requirements/test.txt packaging==23.1 @@ -139,6 +141,8 @@ requests==2.31.0 # via sphinx restructuredtext-lint==1.4.0 # via doc8 +rules==3.3 + # via -r requirements/test.txt six==1.16.0 # via bleach snowballstemmer==2.2.0 diff --git a/requirements/quality.txt b/requirements/quality.txt index c70f66a72..c580bd7e1 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -104,6 +104,8 @@ mccabe==0.7.0 # via pylint mdurl==0.1.2 # via markdown-it-py +mock==5.0.2 + # via -r requirements/test.txt more-itertools==9.1.0 # via jaraco-classes mysqlclient==2.1.1 @@ -182,6 +184,8 @@ rfc3986==2.0.0 # via twine rich==13.4.2 # via twine +rules==3.3 + # via -r requirements/test.txt secretstorage==3.3.3 # via keyring six==1.16.0 diff --git a/requirements/test.in b/requirements/test.in index 6b4e096d4..1d521351d 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -13,3 +13,4 @@ pytest-cov # pytest extension for code coverage statistics pytest-django # pytest extension for better Django support code-annotations # provides commands used by the pii_check make target. ddt # supports data driven tests +mock # supports overriding classes and methods in tests diff --git a/requirements/test.txt b/requirements/test.txt index dafcddb06..e64d8190b 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -38,6 +38,8 @@ jinja2==3.1.2 # via code-annotations markupsafe==2.1.3 # via jinja2 +mock==5.0.2 + # via -r requirements/test.in mysqlclient==2.1.1 # via -r requirements/test.in packaging==23.1 @@ -64,6 +66,8 @@ pytz==2023.3 # djangorestframework pyyaml==6.0 # via code-annotations +rules==3.3 + # via -r requirements/base.txt sqlparse==0.4.4 # via # -r requirements/base.txt diff --git a/test_settings.py b/test_settings.py index 46e52fa59..2e41694e4 100644 --- a/test_settings.py +++ b/test_settings.py @@ -35,6 +35,8 @@ def root(*args): # Admin # 'django.contrib.admin', # 'django.contrib.admindocs', + # django-rules based authorization + 'rules.apps.AutodiscoverRulesConfig', # Our own apps "openedx_learning.core.components.apps.ComponentsConfig", "openedx_learning.core.contents.apps.ContentsConfig", @@ -42,6 +44,10 @@ def root(*args): "openedx_tagging.core.tagging.apps.TaggingConfig", ] +AUTHENTICATION_BACKENDS = [ + 'rules.permissions.ObjectPermissionBackend', +] + LOCALE_PATHS = [ root("openedx_learning", "conf", "locale"), ] diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py new file mode 100644 index 000000000..2c2b4e748 --- /dev/null +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -0,0 +1,230 @@ +"""Tests tagging rules-based permissions""" + +import ddt +from django.contrib.auth import get_user_model +from django.test.testcases import TestCase +from mock import Mock + +from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy + +from .test_models import TestTagTaxonomyMixin + +User = get_user_model() + + +@ddt.ddt +class TestRulesTagging(TestTagTaxonomyMixin, TestCase): + """ + Tests that the expected rules have been applied to the tagging models. + """ + + def setUp(self): + super().setUp() + self.superuser = User.objects.create( + username="superuser", + email="superuser@example.com", + is_superuser=True, + ) + self.staff = User.objects.create( + username="staff", + email="staff@example.com", + is_staff=True, + ) + self.learner = User.objects.create( + username="learner", + email="learner@example.com", + ) + + self.object_tag = ObjectTag( + taxonomy=self.taxonomy, + tag=self.bacteria, + ) + self.object_tag.resync() + self.object_tag.save() + + # Taxonomy + + @ddt.data( + "oel_tagging.add_taxonomy", + "oel_tagging.change_taxonomy", + ) + def test_add_change_taxonomy(self, perm): + """Taxonomy administrators can create or modify any Taxonomy""" + assert self.superuser.has_perm(perm) + assert self.superuser.has_perm(perm, self.taxonomy) + assert self.staff.has_perm(perm) + assert self.staff.has_perm(perm, self.taxonomy) + assert not self.learner.has_perm(perm) + assert not self.learner.has_perm(perm, self.taxonomy) + + @ddt.data( + "oel_tagging.add_taxonomy", + "oel_tagging.change_taxonomy", + "oel_tagging.delete_taxonomy", + ) + def test_system_taxonomy(self, perm): + """Taxonomy administrators cannot edit system taxonomies""" + # TODO: use SystemTaxonomy when available + system_taxonomy = Mock(spec=Taxonomy) + system_taxonomy.system_defined.return_value = True + assert self.superuser.has_perm(perm, system_taxonomy) + assert not self.staff.has_perm(perm, system_taxonomy) + assert not self.learner.has_perm(perm, system_taxonomy) + + @ddt.data( + True, + False, + ) + def test_delete_taxonomy(self, enabled): + """Taxonomy administrators can delete any Taxonomy""" + self.taxonomy.enabled = enabled + assert self.superuser.has_perm("oel_tagging.delete_taxonomy") + assert self.superuser.has_perm("oel_tagging.delete_taxonomy", self.taxonomy) + assert self.staff.has_perm("oel_tagging.delete_taxonomy") + assert self.staff.has_perm("oel_tagging.delete_taxonomy", self.taxonomy) + assert not self.learner.has_perm("oel_tagging.delete_taxonomy") + assert not self.learner.has_perm("oel_tagging.delete_taxonomy", self.taxonomy) + + @ddt.data( + True, + False, + ) + def test_view_taxonomy_enabled(self, enabled): + """Anyone can see enabled taxonomies, but learners cannot see disabled taxonomies""" + self.taxonomy.enabled = enabled + assert self.superuser.has_perm("oel_tagging.view_taxonomy") + assert self.superuser.has_perm("oel_tagging.view_taxonomy", self.taxonomy) + assert self.staff.has_perm("oel_tagging.view_taxonomy") + assert self.staff.has_perm("oel_tagging.view_taxonomy", self.taxonomy) + assert not self.learner.has_perm("oel_tagging.view_taxonomy") + assert ( + self.learner.has_perm("oel_tagging.view_taxonomy", self.taxonomy) == enabled + ) + + # Tag + + @ddt.data( + "oel_tagging.add_tag", + "oel_tagging.change_tag", + ) + def test_add_change_tag(self, perm): + """Taxonomy administrators can modify tags on non-free-text taxonomies""" + assert self.superuser.has_perm(perm) + assert self.superuser.has_perm(perm, self.bacteria) + assert self.staff.has_perm(perm) + assert self.staff.has_perm(perm, self.bacteria) + assert not self.learner.has_perm(perm) + assert not self.learner.has_perm(perm, self.bacteria) + + @ddt.data( + "oel_tagging.add_tag", + "oel_tagging.change_tag", + ) + def test_tag_free_text_taxonomy(self, perm): + """Taxonomy administrators cannot modify tags on a free-text Taxonomy""" + self.taxonomy.allow_free_text = True + self.taxonomy.save() + assert self.superuser.has_perm(perm, self.bacteria) + assert not self.staff.has_perm(perm, self.bacteria) + assert not self.learner.has_perm(perm, self.bacteria) + + @ddt.data( + "oel_tagging.add_tag", + "oel_tagging.change_tag", + "oel_tagging.delete_tag", + ) + def test_tag_no_taxonomy(self, perm): + """Taxonomy administrators can modify any Tag, even those with no Taxonnmy.""" + tag = Tag() + assert self.superuser.has_perm(perm, tag) + assert self.staff.has_perm(perm, tag) + assert not self.learner.has_perm(perm, tag) + + @ddt.data( + True, + False, + ) + def test_delete_tag(self, allow_free_text): + """Taxonomy administrators can delete any Tag, even those associated with a free-text Taxonomy.""" + self.taxonomy.allow_free_text = allow_free_text + self.taxonomy.save() + assert self.superuser.has_perm("oel_tagging.delete_tag") + assert self.superuser.has_perm("oel_tagging.delete_tag", self.bacteria) + assert self.staff.has_perm("oel_tagging.delete_tag") + assert self.staff.has_perm("oel_tagging.delete_tag", self.bacteria) + assert not self.learner.has_perm("oel_tagging.delete_tag") + assert not self.learner.has_perm("oel_tagging.delete_tag", self.bacteria) + + def test_view_tag(self): + """Anyone can view any Tag""" + assert self.superuser.has_perm("oel_tagging.view_tag") + assert self.superuser.has_perm("oel_tagging.view_tag", self.bacteria) + assert self.staff.has_perm("oel_tagging.view_tag") + assert self.staff.has_perm("oel_tagging.view_tag", self.bacteria) + assert self.learner.has_perm("oel_tagging.view_tag") + assert self.learner.has_perm("oel_tagging.view_tag", self.bacteria) + + # ObjectTag + + @ddt.data( + "oel_tagging.add_object_tag", + "oel_tagging.change_object_tag", + ) + def test_add_change_object_tag(self, perm): + """Taxonomy administrators can create/edit an ObjectTag with an enabled Taxonomy""" + assert self.superuser.has_perm(perm) + assert self.superuser.has_perm(perm, self.object_tag) + assert self.staff.has_perm(perm) + assert self.staff.has_perm(perm, self.object_tag) + assert not self.learner.has_perm(perm) + assert not self.learner.has_perm(perm, self.object_tag) + + @ddt.data( + "oel_tagging.add_object_tag", + "oel_tagging.change_object_tag", + ) + def test_object_tag_disabled_taxonomy(self, perm): + """Taxonomy administrators cannot create/edit an ObjectTag with a disabled Taxonomy""" + self.taxonomy.enabled = False + self.taxonomy.save() + assert self.superuser.has_perm(perm, self.object_tag) + assert not self.staff.has_perm(perm, self.object_tag) + assert not self.learner.has_perm(perm, self.object_tag) + + @ddt.data( + True, + False, + ) + def test_delete_object_tag(self, enabled): + """Taxonomy administrators can delete any ObjectTag, even those associated with a disabled Taxonomy.""" + self.taxonomy.enabled = enabled + self.taxonomy.save() + assert self.superuser.has_perm("oel_tagging.delete_object_tag") + assert self.superuser.has_perm("oel_tagging.delete_object_tag", self.object_tag) + assert self.staff.has_perm("oel_tagging.delete_object_tag") + assert self.staff.has_perm("oel_tagging.delete_object_tag", self.object_tag) + assert not self.learner.has_perm("oel_tagging.delete_object_tag") + assert not self.learner.has_perm( + "oel_tagging.delete_object_tag", self.object_tag + ) + + @ddt.data( + "oel_tagging.add_object_tag", + "oel_tagging.change_object_tag", + "oel_tagging.delete_object_tag", + ) + def test_object_tag_no_taxonomy(self, perm): + """Taxonomy administrators can modify an ObjectTag with no Taxonomy""" + object_tag = ObjectTag() + assert self.superuser.has_perm(perm, object_tag) + assert self.staff.has_perm(perm, object_tag) + assert not self.learner.has_perm(perm, object_tag) + + def test_view_object_tag(self): + """Anyone can view any ObjectTag""" + assert self.superuser.has_perm("oel_tagging.view_object_tag") + assert self.superuser.has_perm("oel_tagging.view_object_tag", self.object_tag) + assert self.staff.has_perm("oel_tagging.view_object_tag") + assert self.staff.has_perm("oel_tagging.view_object_tag", self.object_tag) + assert self.learner.has_perm("oel_tagging.view_object_tag") + assert self.learner.has_perm("oel_tagging.view_object_tag", self.object_tag) From 821a53c38da47addec3135d32ac1ce70544c5263 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 27 Jun 2023 07:19:32 +0930 Subject: [PATCH 4/7] fix: remove db_collation check Tests are failing in openedx on sqlite. --- openedx_learning/lib/collations.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/openedx_learning/lib/collations.py b/openedx_learning/lib/collations.py index 2afdcc377..561b1f316 100644 --- a/openedx_learning/lib/collations.py +++ b/openedx_learning/lib/collations.py @@ -32,11 +32,6 @@ def __init__(self, *args, db_collations=None, db_collation=None, **kwargs): it for Django 3.2 compatibility (see the ``db_collation`` method docstring for details). """ - if db_collation is not None: - raise ValueError( - f"Cannot use db_collation with {self.__class__.__name__}. " - + "Please use a db_collations dict instead." - ) super().__init__(*args, **kwargs) self.db_collations = db_collations or {} @@ -106,7 +101,7 @@ def db_parameters(self, connection): def deconstruct(self): """ How to serialize our Field for the migration file. - + For our mixin fields, this is just doing what the field's superclass would do and then tacking on our custom ``db_collations`` dict data. """ From d7538c78fd6562d7f266dc6dc5f0c75b17add8c2 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Mon, 19 Jun 2023 14:37:36 -0500 Subject: [PATCH 5/7] feat: import/export Taxonomy API functions --- openedx_tagging/core/tagging/api.py | 193 +++++++++++++++++- .../core/fixtures/tagging.yaml | 44 ++-- .../openedx_tagging/core/tagging/test_api.py | 178 +++++++++++++++- 3 files changed, 390 insertions(+), 25 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 6b9bf2a04..bbe18361a 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -10,13 +10,28 @@ Please look at the models.py file for more information about the kinds of data are stored in this app. """ +import csv +import json +from enum import Enum +from io import StringIO, BytesIO, TextIOWrapper from typing import List, Type +from django.db import transaction from django.db.models import QuerySet +from django.core.exceptions import ObjectDoesNotExist from django.utils.translation import gettext_lazy as _ from .models import ObjectTag, Tag, Taxonomy +csv_fields = ['id', 'name', 'parent_id', 'parent_name'] + +class TaxonomyDataFormat(Enum): + """ + Formats used to export and import Taxonomies + """ + CSV = 'CSV' + JSON = 'JSON' + def create_taxonomy( name, @@ -29,6 +44,7 @@ def create_taxonomy( """ Creates, saves, and returns a new Taxonomy with the given attributes. """ + return Taxonomy.objects.create( name=name, description=description, @@ -105,5 +121,180 @@ def tag_object( Raised ValueError if the proposed tags are invalid for this taxonomy. Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. """ - return taxonomy.tag_object(tags, object_id, object_type) + + +def import_tags(taxonomy: Taxonomy, tags: BytesIO, format: TaxonomyDataFormat, replace=False): + """ + Imports the hierarchical tags from the given blob into the Taxonomy. + The blob can be CSV or JSON format. + + If replace, then removes any existing child Tags linked to this taxonomy before performing the import. + """ + + # Validations + if taxonomy.allow_free_text: + raise ValueError( + _( + f"Invalid taxonomy ({taxonomy.id}): You can't import free-from tags taxonomies" + ) + ) + if format not in TaxonomyDataFormat.__members__.values(): + raise ValueError( + _( + f"Invalid format: {format}" + ) + ) + + # Read file and build the tags data to be uploaded + try: + tags_data = {} + tags.seek(0) + if format == TaxonomyDataFormat.CSV: + text_tags = TextIOWrapper(tags, encoding='utf-8') + csv_reader = csv.DictReader(text_tags) + header_fields = csv_reader.fieldnames + if csv_fields != header_fields: + raise ValueError( + _( + f"Invalid CSV header: {header_fields}. Must be: {csv_fields}." + ) + ) + tags_data = list(csv_reader) + else: + # TaxonomyDataFormat.JSON + tags_data = json.load(tags) + if 'tags' not in tags_data: + raise ValueError( + _( + f"Invalid JSON format: Missing 'tags' list." + ) + ) + tags_data = tags_data.get('tags') + except ValueError as e: + raise e + finally: + tags.close() + + + new_tags = [] + updated_tags = [] + + def create_update_tag(tag): + """ + Function to create a new Tag or update an existing one. + + This function keeps a creation/update history with `new_tags` and `updated_tags`, + a same tag can't be created/updated in a same taxonomy import. + Also, recursively, creates the parents of the `tag`. + + Returns the created/updated Tag. + Raise KeyError if 'id' or 'name' don't exist on `tag` + """ + + tag_id = tag['id'] + tag_name = tag['name'] + tag_parent_id = tag.get('parent_id') + tag_parent_name = tag.get('parent_name') + + # Check if the tag has not already been created or updated + if tag_id not in new_tags and tag_id not in updated_tags: + try: + # Update tag + tag_instance = Tag.objects.get(external_id=tag_id) + tag_instance.value = tag_name + + if tag_instance.parent and (not tag_parent_id or not tag_parent_name): + # if there is no parent in the data import + tag_instance.parent = None + updated_tags.append(tag_id) + except ObjectDoesNotExist: + # Create tag + tag_instance = Tag( + taxonomy=taxonomy, + value=tag_name, + external_id=tag_id, + ) + new_tags.append(tag_id) + + if tag_parent_id and tag_parent_name: + # Parent creation/update + parent = create_update_tag({'id': tag_parent_id, 'name': tag_parent_name}) + tag_instance.parent = parent + + tag_instance.save() + return tag_instance + else: + # Returns the created/updated tag from history + return Tag.objects.get(external_id=tag_id) + + # Create and update tags + with transaction.atomic(): + # Delete all old Tags linked to the taxonomy + if replace: + Tag.objects.filter(taxonomy=taxonomy).delete() + + for tag in tags_data: + try: + create_update_tag(tag) + except KeyError as e: + key = e.args[0] + raise ValueError( + _( + f"Invalid JSON format: Missing '{key}' on a tag ({tag})" + ) + ) + resync_tags() + +def export_tags(taxonomy: Taxonomy, format: TaxonomyDataFormat) -> str: + """ + Creates a blob string describing all the tags in the given Taxonomy. + The output format can be CSV or JSON. + """ + + # Validations + if taxonomy.allow_free_text: + raise ValueError( + _( + f"Invalid taxonomy ({taxonomy.id}): You can't export free-from tags taxonomies" + ) + ) + if format not in TaxonomyDataFormat.__members__.values(): + raise ValueError( + _( + f"Invalid format: {format}" + ) + ) + + # Build list of tags in a dictionary format + tags = get_tags(taxonomy) + result = [] + for tag in tags: + result_tag = { + 'id': tag.external_id or tag.id, + 'name': tag.value, + } + if tag.parent: + result_tag['parent_id'] = tag.parent.external_id or tag.parent.id + result_tag['parent_name'] = tag.parent.value + result.append(result_tag) + + # Convert dictonary into the output format + if format == TaxonomyDataFormat.CSV: + with StringIO() as csv_buffer: + csv_writer = csv.DictWriter(csv_buffer, fieldnames=csv_fields) + csv_writer.writeheader() + + for tag in result: + csv_writer.writerow(tag) + + csv_string = csv_buffer.getvalue() + return csv_string + else: + # TaxonomyDataFormat.JSON + json_result = { + 'name': taxonomy.name, + 'description': taxonomy.description, + 'tags': result + } + return json.dumps(json_result) diff --git a/tests/openedx_tagging/core/fixtures/tagging.yaml b/tests/openedx_tagging/core/fixtures/tagging.yaml index 50b9459bb..7db85aefa 100644 --- a/tests/openedx_tagging/core/fixtures/tagging.yaml +++ b/tests/openedx_tagging/core/fixtures/tagging.yaml @@ -4,152 +4,152 @@ taxonomy: 1 parent: null value: Bacteria - external_id: null + external_id: tag_1 - model: oel_tagging.tag pk: 2 fields: taxonomy: 1 parent: null value: Archaea - external_id: null + external_id: tag_2 - model: oel_tagging.tag pk: 3 fields: taxonomy: 1 parent: null value: Eukaryota - external_id: null + external_id: tag_3 - model: oel_tagging.tag pk: 4 fields: taxonomy: 1 parent: 1 value: Eubacteria - external_id: null + external_id: tag_4 - model: oel_tagging.tag pk: 5 fields: taxonomy: 1 parent: 1 value: Archaebacteria - external_id: null + external_id: tag_5 - model: oel_tagging.tag pk: 6 fields: taxonomy: 1 parent: 2 value: DPANN - external_id: null + external_id: tag_6 - model: oel_tagging.tag pk: 7 fields: taxonomy: 1 parent: 2 value: Euryarchaeida - external_id: null + external_id: tag_7 - model: oel_tagging.tag pk: 8 fields: taxonomy: 1 parent: 2 value: Proteoarchaeota - external_id: null + external_id: tag_8 - model: oel_tagging.tag pk: 9 fields: taxonomy: 1 parent: 3 value: Animalia - external_id: null + external_id: tag_9 - model: oel_tagging.tag pk: 10 fields: taxonomy: 1 parent: 3 value: Plantae - external_id: null + external_id: tag_10 - model: oel_tagging.tag pk: 11 fields: taxonomy: 1 parent: 3 value: Fungi - external_id: null + external_id: tag_11 - model: oel_tagging.tag pk: 12 fields: taxonomy: 1 parent: 3 value: Protista - external_id: null + external_id: tag_12 - model: oel_tagging.tag pk: 13 fields: taxonomy: 1 parent: 3 value: Monera - external_id: null + external_id: tag_13 - model: oel_tagging.tag pk: 14 fields: taxonomy: 1 parent: 9 value: Arthropoda - external_id: null + external_id: tag_14 - model: oel_tagging.tag pk: 15 fields: taxonomy: 1 parent: 9 value: Chordata - external_id: null + external_id: tag_15 - model: oel_tagging.tag pk: 16 fields: taxonomy: 1 parent: 9 value: Gastrotrich - external_id: null + external_id: tag_16 - model: oel_tagging.tag pk: 17 fields: taxonomy: 1 parent: 9 value: Cnidaria - external_id: null + external_id: tag_17 - model: oel_tagging.tag pk: 18 fields: taxonomy: 1 parent: 9 value: Ctenophora - external_id: null + external_id: tag_18 - model: oel_tagging.tag pk: 19 fields: taxonomy: 1 parent: 9 value: Placozoa - external_id: null + external_id: tag_19 - model: oel_tagging.tag pk: 20 fields: taxonomy: 1 parent: 9 value: Porifera - external_id: null + external_id: tag_20 - model: oel_tagging.tag pk: 21 fields: taxonomy: 1 parent: 15 value: Mammalia - external_id: null + external_id: tag_21 - model: oel_tagging.taxonomy pk: 1 fields: name: Life on Earth - description: null + description: This taxonomy contains the Kingdoms of the Earth enabled: true required: false allow_multiple: false diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 7a418e687..a68d487fa 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -1,7 +1,10 @@ """ Test the tagging APIs """ -from unittest.mock import patch +from io import BytesIO +import json +import ddt +from unittest.mock import patch from django.test.testcases import TestCase import openedx_tagging.core.tagging.api as tagging_api @@ -9,12 +12,15 @@ from .test_models import TestTagTaxonomyMixin - +@ddt.ddt class TestApiTagging(TestTagTaxonomyMixin, TestCase): """ Test the Tagging API methods. """ + def get_tag(self, tags, tag_value): + return next((item for item in tags if item.value == tag_value), None) + def test_create_taxonomy(self): params = { "name": "Difficulty", @@ -188,3 +194,171 @@ def test_tag_object(self): assert object_tag.name == self.taxonomy.name assert object_tag.object_id == "biology101" assert object_tag.object_type == "course" + + def test_import_tags_csv(self): + csv_data = "id,name,parent_id,parent_name\n1,Tag 1,,\n2,Tag 2,1,Tag 1\n" + csv_file = BytesIO(csv_data.encode()) + + taxonomy = tagging_api.create_taxonomy("CSV_Taxonomy") + tagging_api.import_tags(taxonomy, csv_file, tagging_api.TaxonomyDataFormat.CSV) + tags = tagging_api.get_tags(taxonomy) + + # Assert that the tags are imported correctly + self.assertEqual(len(tags), 2) + item = self.get_tag(tags, 'Tag 1') + self.assertIsNotNone(item) + item = self.get_tag(tags, 'Tag 2') + self.assertIsNotNone(item) + self.assertEqual(item.parent.value, 'Tag 1') + + def test_import_tags_json(self): + json_data = {"tags": [ + {"id": "1", "name": "Tag 1"}, + {"id": "2", "name": "Tag 2", "parent_id": "1", "parent_name": "Tag 1"} + ]} + json_file = BytesIO(json.dumps(json_data).encode()) + + taxonomy = tagging_api.create_taxonomy("JSON_Taxonomy") + tagging_api.import_tags(taxonomy, json_file, tagging_api.TaxonomyDataFormat.JSON) + tags = tagging_api.get_tags(taxonomy) + + # Assert that the tags are imported correctly + self.assertEqual(len(tags), 2) + item = self.get_tag(tags, 'Tag 1') + self.assertIsNotNone(item) + item = self.get_tag(tags, 'Tag 2') + self.assertIsNotNone(item) + self.assertEqual(item.parent.value, 'Tag 1') + + def test_import_tags_replace(self): + self.assertEqual(len(tagging_api.get_tags(self.taxonomy)), 20) + + json_data = {"tags": [{'id': "tag_1", "name": "Bacteria"},{'id': "tag_2", "name": "Archaea"}]} + json_file = BytesIO(json.dumps(json_data).encode()) + + tagging_api.import_tags(self.taxonomy, json_file, tagging_api.TaxonomyDataFormat.JSON, replace=True) + tags = tagging_api.get_tags(self.taxonomy) + self.assertEqual(len(tags), 2) + item = self.get_tag(tags, 'Bacteria') + self.assertIsNotNone(item) + item = self.get_tag(tags, 'Archaea') + self.assertIsNotNone(item) + + def test_import_tags_update(self): + self.assertEqual(len(tagging_api.get_tags(self.taxonomy)), 20) + + json_data = {"tags": [ + # Name update + {"id": "tag_1", "name": "Bacteria 2.0"}, + # Parent update + {"id": "tag_4", "name": "Eubacteria 2.0", "parent_id": "tag_2", "parent_name": "Archaea"}, + # Parent deletion + {"id": "tag_5", "name": "Archaebacteria 2.0"}, + # New Tag + {"id": "tag_22", "name": "My new tag 2.0", "parent_id": "tag_1", "parent_name": "Bacteria 2.0"}, + ]} + + json_file = BytesIO(json.dumps(json_data).encode()) + tagging_api.import_tags(self.taxonomy, json_file, tagging_api.TaxonomyDataFormat.JSON) + tags = tagging_api.get_tags(self.taxonomy) + self.assertEqual(len(tags), 21) + + # Check name update + item = self.get_tag(tags, 'Bacteria 2.0') + self.assertIsNotNone(item) + + # Check parent update + item = self.get_tag(tags, 'Eubacteria 2.0') + self.assertIsNotNone(item) + + # Check parent deletion + self.assertEqual(item.parent.value, 'Archaea') + item = self.get_tag(tags, 'Archaebacteria 2.0') + self.assertIsNotNone(item) + self.assertIsNone(item.parent) + + # Check new tag + item = self.get_tag(tags, 'My new tag 2.0') + self.assertIsNotNone(item) + self.assertEqual(item.parent.value, 'Bacteria 2.0') + + # Check existing tag + item = self.get_tag(tags, 'Porifera') + self.assertIsNotNone(item) + + def test_import_tags_validations(self): + invalid_csv_data = "id,name,tag_parent_id,tag_parent_name\n1,Tag 1,,\n2,Tag 2,1,Tag 1\n" + invalid_csv_file = BytesIO(invalid_csv_data.encode()) + taxonomy = tagging_api.create_taxonomy("Validations_Taxonomy") + taxonomy_free_text = tagging_api.create_taxonomy("Free_Taxonomy", allow_free_text=True) + + with self.assertRaises(ValueError): + tagging_api.import_tags(taxonomy_free_text, invalid_csv_file, tagging_api.TaxonomyDataFormat.CSV) + + with self.assertRaises(ValueError): + tagging_api.import_tags(taxonomy, "", "XML") + + with self.assertRaises(ValueError): + tagging_api.import_tags(taxonomy, invalid_csv_file, tagging_api.TaxonomyDataFormat.CSV) + + @ddt.data( + # Invalid json format + {"taxonomy_tags": [ + {"id": "1", "name": "Tag 1"}, + {"id": "2", "name": "Tag 2", "parent_id": "1", "parent_name": "Tag 1"} + ]}, + # Invalid 'id' key + {"tags": [ + {"tag_id": "1", "name": "Tag 1"}, + ]}, + # Invalid 'name' key + {"tags": [ + {"id": "1", "tag_name": "Tag 1"}, + ]} + ) + def test_import_tags_json_validations(self, json_data): + json_file = BytesIO(json.dumps(json_data).encode()) + taxonomy = tagging_api.create_taxonomy("Validations_Taxonomy") + + with self.assertRaises(ValueError): + tagging_api.import_tags(taxonomy, json_file, tagging_api.TaxonomyDataFormat.JSON) + + def test_export_tags_json(self): + json_data = { + "name": "Life on Earth", + "description": "This taxonomy contains the Kingdoms of the Earth", + "tags": [ + {'id': "tag_2", "name": "Archaea"}, + {'id': "tag_1", "name": "Bacteria"}, + {'id': "tag_4", "name": "Euryarchaeida", "parent_id": "tag_2", "parent_name": "Archaea"}, + {'id': "tag_3", "name": "Eubacteria", "parent_id": "tag_1", "parent_name": "Bacteria"}, + ] + } + json_file = BytesIO(json.dumps(json_data).encode()) + + # Import and replace + tagging_api.import_tags(self.taxonomy, json_file, tagging_api.TaxonomyDataFormat.JSON, replace=True) + + result = tagging_api.export_tags(self.taxonomy, tagging_api.TaxonomyDataFormat.JSON) + self.assertEqual(json.loads(result), json_data) + + def test_export_tags_csv(self): + csv_data = "id,name,parent_id,parent_name\r\ntag_2,Archaea,,\r\ntag_1,Bacteria,,\r\n" \ + "tag_4,Euryarchaeida,tag_2,Archaea\r\ntag_3,Eubacteria,tag_1,Bacteria\r\n" + + csv_file = BytesIO(csv_data.encode()) + + # Import and replace + tagging_api.import_tags(self.taxonomy, csv_file, tagging_api.TaxonomyDataFormat.CSV, replace=True) + result = tagging_api.export_tags(self.taxonomy, tagging_api.TaxonomyDataFormat.CSV) + self.assertEqual(result, csv_data) + + def test_export_tags_validation(self): + taxonomy_free_text = tagging_api.create_taxonomy("Free_Taxonomy", allow_free_text=True) + taxonomy_xml = tagging_api.create_taxonomy("XML_Taxonomy") + + with self.assertRaises(ValueError): + tagging_api.export_tags(taxonomy_free_text, tagging_api.TaxonomyDataFormat.JSON) + + with self.assertRaises(ValueError): + tagging_api.export_tags(taxonomy_xml, "XML") From 577f44bcf13a337c1853656b3067a0dfec4c8dd1 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Wed, 21 Jun 2023 11:55:10 -0500 Subject: [PATCH 6/7] fix: Nits on import/export functions and on tests --- openedx_tagging/core/tagging/api.py | 28 +++++++++---------- .../openedx_tagging/core/tagging/test_api.py | 11 +++++--- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index bbe18361a..590efb7a6 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -136,13 +136,7 @@ def import_tags(taxonomy: Taxonomy, tags: BytesIO, format: TaxonomyDataFormat, r if taxonomy.allow_free_text: raise ValueError( _( - f"Invalid taxonomy ({taxonomy.id}): You can't import free-from tags taxonomies" - ) - ) - if format not in TaxonomyDataFormat.__members__.values(): - raise ValueError( - _( - f"Invalid format: {format}" + f"Invalid taxonomy ({taxonomy.id}): You cannot import into a free-form taxonomy." ) ) @@ -161,8 +155,7 @@ def import_tags(taxonomy: Taxonomy, tags: BytesIO, format: TaxonomyDataFormat, r ) ) tags_data = list(csv_reader) - else: - # TaxonomyDataFormat.JSON + elif format == TaxonomyDataFormat.JSON: tags_data = json.load(tags) if 'tags' not in tags_data: raise ValueError( @@ -171,6 +164,12 @@ def import_tags(taxonomy: Taxonomy, tags: BytesIO, format: TaxonomyDataFormat, r ) ) tags_data = tags_data.get('tags') + else: + raise ValueError( + _( + f"Invalid format: {format}" + ) + ) except ValueError as e: raise e finally: @@ -201,14 +200,14 @@ def create_update_tag(tag): if tag_id not in new_tags and tag_id not in updated_tags: try: # Update tag - tag_instance = Tag.objects.get(external_id=tag_id) + tag_instance = taxonomy.tag_set.get(external_id=tag_id) tag_instance.value = tag_name if tag_instance.parent and (not tag_parent_id or not tag_parent_name): # if there is no parent in the data import tag_instance.parent = None updated_tags.append(tag_id) - except ObjectDoesNotExist: + except Tag.DoesNotExist: # Create tag tag_instance = Tag( taxonomy=taxonomy, @@ -226,7 +225,7 @@ def create_update_tag(tag): return tag_instance else: # Returns the created/updated tag from history - return Tag.objects.get(external_id=tag_id) + return taxonomy.tag_set.get(external_id=tag_id) # Create and update tags with transaction.atomic(): @@ -256,7 +255,7 @@ def export_tags(taxonomy: Taxonomy, format: TaxonomyDataFormat) -> str: if taxonomy.allow_free_text: raise ValueError( _( - f"Invalid taxonomy ({taxonomy.id}): You can't export free-from tags taxonomies" + f"Invalid taxonomy ({taxonomy.id}): You cannot import into a free-form taxonomy." ) ) if format not in TaxonomyDataFormat.__members__.values(): @@ -266,7 +265,7 @@ def export_tags(taxonomy: Taxonomy, format: TaxonomyDataFormat) -> str: ) ) - # Build list of tags in a dictionary format + # Build tags in a dictionary format tags = get_tags(taxonomy) result = [] for tag in tags: @@ -292,6 +291,7 @@ def export_tags(taxonomy: Taxonomy, format: TaxonomyDataFormat) -> str: return csv_string else: # TaxonomyDataFormat.JSON + # Verification is made at the beginning before bringing and assembling tags data. json_result = { 'name': taxonomy.name, 'description': taxonomy.description, diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index a68d487fa..5701e0f19 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -213,8 +213,8 @@ def test_import_tags_csv(self): def test_import_tags_json(self): json_data = {"tags": [ - {"id": "1", "name": "Tag 1"}, - {"id": "2", "name": "Tag 2", "parent_id": "1", "parent_name": "Tag 1"} + {"id": "tag_1", "name": "Tag 1"}, + {"id": "tag_2", "name": "Tag 2", "parent_id": "tag_1", "parent_name": "Tag 1"} ]} json_file = BytesIO(json.dumps(json_data).encode()) @@ -288,17 +288,20 @@ def test_import_tags_update(self): def test_import_tags_validations(self): invalid_csv_data = "id,name,tag_parent_id,tag_parent_name\n1,Tag 1,,\n2,Tag 2,1,Tag 1\n" - invalid_csv_file = BytesIO(invalid_csv_data.encode()) taxonomy = tagging_api.create_taxonomy("Validations_Taxonomy") taxonomy_free_text = tagging_api.create_taxonomy("Free_Taxonomy", allow_free_text=True) + # Open the file in each test since it always closes even if there is an error with self.assertRaises(ValueError): + invalid_csv_file = BytesIO(invalid_csv_data.encode()) tagging_api.import_tags(taxonomy_free_text, invalid_csv_file, tagging_api.TaxonomyDataFormat.CSV) with self.assertRaises(ValueError): - tagging_api.import_tags(taxonomy, "", "XML") + invalid_csv_file = BytesIO(invalid_csv_data.encode()) + tagging_api.import_tags(taxonomy, invalid_csv_file, "XML") with self.assertRaises(ValueError): + invalid_csv_file = BytesIO(invalid_csv_data.encode()) tagging_api.import_tags(taxonomy, invalid_csv_file, tagging_api.TaxonomyDataFormat.CSV) @ddt.data( From e5b1698fe93208ab88fc07d4635b007797cb7644 Mon Sep 17 00:00:00 2001 From: XnpioChV Date: Thu, 22 Jun 2023 15:54:33 -0500 Subject: [PATCH 7/7] chore: Improving 'reaplce' functionality of 'import_tags' --- openedx_tagging/core/tagging/api.py | 18 +++++----- .../openedx_tagging/core/tagging/test_api.py | 33 ++++++++++++++++++- 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/openedx_tagging/core/tagging/api.py b/openedx_tagging/core/tagging/api.py index 590efb7a6..46d5e623f 100644 --- a/openedx_tagging/core/tagging/api.py +++ b/openedx_tagging/core/tagging/api.py @@ -176,14 +176,13 @@ def import_tags(taxonomy: Taxonomy, tags: BytesIO, format: TaxonomyDataFormat, r tags.close() - new_tags = [] updated_tags = [] def create_update_tag(tag): """ Function to create a new Tag or update an existing one. - This function keeps a creation/update history with `new_tags` and `updated_tags`, + This function keeps a creation/update history with `updated_tags`, a same tag can't be created/updated in a same taxonomy import. Also, recursively, creates the parents of the `tag`. @@ -197,7 +196,7 @@ def create_update_tag(tag): tag_parent_name = tag.get('parent_name') # Check if the tag has not already been created or updated - if tag_id not in new_tags and tag_id not in updated_tags: + if tag_id not in updated_tags: try: # Update tag tag_instance = taxonomy.tag_set.get(external_id=tag_id) @@ -214,7 +213,7 @@ def create_update_tag(tag): value=tag_name, external_id=tag_id, ) - new_tags.append(tag_id) + updated_tags.append(tag_id) if tag_parent_id and tag_parent_name: # Parent creation/update @@ -229,10 +228,6 @@ def create_update_tag(tag): # Create and update tags with transaction.atomic(): - # Delete all old Tags linked to the taxonomy - if replace: - Tag.objects.filter(taxonomy=taxonomy).delete() - for tag in tags_data: try: create_update_tag(tag) @@ -243,7 +238,12 @@ def create_update_tag(tag): f"Invalid JSON format: Missing '{key}' on a tag ({tag})" ) ) - resync_tags() + + # If replace, delete all not updated tags (Not present in the file) + if replace: + taxonomy.tag_set.exclude(external_id__in=updated_tags).delete() + + resync_object_tags(ObjectTag.objects.filter(taxonomy=taxonomy)) def export_tags(taxonomy: Taxonomy, format: TaxonomyDataFormat) -> str: """ diff --git a/tests/openedx_tagging/core/tagging/test_api.py b/tests/openedx_tagging/core/tagging/test_api.py index 5701e0f19..eb32d1983 100644 --- a/tests/openedx_tagging/core/tagging/test_api.py +++ b/tests/openedx_tagging/core/tagging/test_api.py @@ -2,9 +2,9 @@ from io import BytesIO import json +from unittest.mock import patch import ddt -from unittest.mock import patch from django.test.testcases import TestCase import openedx_tagging.core.tagging.api as tagging_api @@ -326,6 +326,37 @@ def test_import_tags_json_validations(self, json_data): with self.assertRaises(ValueError): tagging_api.import_tags(taxonomy, json_file, tagging_api.TaxonomyDataFormat.JSON) + def test_import_tags_resync(self): + object_id = 'course_1' + object_tag = ObjectTag( + object_id=object_id, + object_type='course', + taxonomy=self.taxonomy, + tag=self.bacteria, + ) + tagging_api.resync_object_tags([object_tag]) + object_tag = ObjectTag.objects.get(object_id=object_id) + self.assertEqual(object_tag.tag.value, 'Bacteria') + self.assertEqual(object_tag._value, 'Bacteria') + + json_data = {"tags": [{"id": "tag_1", "name": "Bacteria 2.0"}]} + json_file = BytesIO(json.dumps(json_data).encode()) + + # Import + tagging_api.import_tags(self.taxonomy, json_file, tagging_api.TaxonomyDataFormat.JSON) + object_tag = ObjectTag.objects.get(object_id=object_id) + self.assertEqual(object_tag.tag.value, 'Bacteria 2.0') + self.assertEqual(object_tag._value, 'Bacteria 2.0') + + json_data = {"tags": [{"id": "tag_1", "name": "Bacteria 3.0"}]} + json_file = BytesIO(json.dumps(json_data).encode()) + + # Import and replace + tagging_api.import_tags(self.taxonomy, json_file, tagging_api.TaxonomyDataFormat.JSON, replace=True) + object_tag = ObjectTag.objects.get(object_id=object_id) + self.assertEqual(object_tag.tag.value, 'Bacteria 3.0') + self.assertEqual(object_tag._value, 'Bacteria 3.0') + def test_export_tags_json(self): json_data = { "name": "Life on Earth",