refactor: consolidates ObjectTag validation

MANIFEST.in

@@ -3,4 +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
+recursive-include openedx_tagging *.html *.png *.gif *.js *.css *.jpg *.jpeg *.svg *.py *.yaml

openedx_tagging/core/tagging/fixtures/system_defined.yaml

@@ -7,3 +7,5 @@
     required: true
     allow_multiple: false
     allow_free_text: false
+    system_defined: true
+    visible_to_authors: true

openedx_tagging/core/tagging/models.py

@@ -1,5 +1,4 @@
 """ Tagging app data models """
-from enum import Enum
 from typing import List, Type
 
 from django.db import models
@@ -529,9 +528,6 @@ def _check_object(self):
         """
         # Must have a valid object id/type:
         return self.object_id and self.object_type
-
-    def _check_tag(self):
-        return self.value
 
 
 class ClosedObjectTag(OpenObjectTag):

openedx_tagging/core/tagging/system_defined_taxonomies/object_tags.py

@@ -11,7 +11,6 @@
 
 
 from openedx_tagging.core.tagging.models import (
-    Tag,
     Taxonomy,
     OpenObjectTag,
     ClosedObjectTag,
@@ -42,21 +41,20 @@ class SystemDefinedObjectTagMixin:
     taxonomy but with different objects.
 
     Using this approach makes the connection between the ObjectTag
-    and system defined taxonomy as hardcoded.
+    and system defined taxonomy as hardcoded and can't be changed.
     """
 
     system_defined_taxonomy_id = None
 
-    @classmethod
-    def _validate_taxonomy(cls, taxonomy: Taxonomy = None):
+    def _check_system_taxonomy(self, taxonomy: Taxonomy = None):
         """
         Validates if the taxonomy is system-defined and match
         with the name stored in the object tag
         """
         return (
-            bool(taxonomy) and 
-            taxonomy.system_defined and 
-            taxonomy.id == cls.system_defined_taxonomy_id
+            bool(taxonomy) and
+            taxonomy.system_defined and
+            taxonomy.id == self.system_defined_taxonomy_id
         )
 
 
@@ -68,12 +66,11 @@ class OpenSystemObjectTag(OpenObjectTag, SystemDefinedObjectTagMixin):
     class Meta:
         proxy = True
 
-    @classmethod
-    def valid_for(cls, taxonomy: Taxonomy = None, **kwars):
-        """
-        Returns True if the the taxonomy is system-defined
-        """
-        return super().valid_for(taxonomy=taxonomy) and cls._validate_taxonomy(taxonomy)
+    def _check_taxonomy(self):
+        return (
+            super()._check_taxonomy() and
+            self._check_system_taxonomy(self.taxonomy)
+        )
 
 
 class ClosedSystemObjectTag(ClosedObjectTag, SystemDefinedObjectTagMixin):
@@ -84,12 +81,11 @@ class ClosedSystemObjectTag(ClosedObjectTag, SystemDefinedObjectTagMixin):
     class Meta:
         proxy = True
 
-    @classmethod
-    def valid_for(cls, taxonomy: Taxonomy = None, tag: Tag = None, **kargs):
-        """
-        Returns True if the the taxonomy is system-defined
-        """
-        return super().valid_for(taxonomy=taxonomy, tag=tag) and cls._validate_taxonomy(taxonomy)
+    def _check_taxonomy(self):
+        return (
+            super()._check_taxonomy() and
+            self._check_system_taxonomy(self.taxonomy)
+        )
 
 
 class ModelObjectTag(OpenSystemObjectTag):
@@ -104,21 +100,24 @@ class Meta:
 
     tag_class_model = None
 
-    @classmethod
-    def valid_for(cls, taxonomy: Taxonomy = None, **kwars):
+
+    def _check_taxonomy(self):
         """
         Validates if has an associated Django model that has an Id
         """
-        if not super().valid_for(taxonomy=taxonomy) or not cls.tag_class_model:
+        if not super()._check_taxonomy():
             return False
-
+
+        if not self.tag_class_model:
+            return False
+
         # Verify that is a Django model
-        if not issubclass(cls.tag_class_model, models.Model):
+        if not issubclass(self.tag_class_model, models.Model):
             return False
 
         # Verify that the model has 'id' field
         try:
-            cls.tag_class_model._meta.get_field('id')
+            self.tag_class_model._meta.get_field('id')
         except FieldDoesNotExist:
             return False
 
@@ -167,7 +166,7 @@ class LanguageObjectTag(ClosedSystemObjectTag):
     languages available in Django LANGUAGES settings var
     """
 
-    system_defined_taxonomy_id = SystemDefinedIds.LanguageTaxonomy
+    system_defined_taxonomy_id = SystemDefinedIds.LanguageTaxonomy.value
 
     class Meta:
         proxy = True

tests/openedx_tagging/core/fixtures/system_defined.yaml

@@ -7,4 +7,4 @@
     required: false
     allow_multiple: false
     allow_free_text: false
-    system_defined: true
+    system_defined: true

tests/openedx_tagging/core/fixtures/tagging.yaml

@@ -174,4 +174,4 @@
     required: false
     allow_multiple: false
     allow_free_text: true
-    system_defined: true
+    system_defined: true

tests/openedx_tagging/core/tagging/test_system_defined.py

@@ -1,12 +1,12 @@
 """ Test the System-defined taxonomies and object tags """
 
+import ddt
+
 from django.contrib.auth import get_user_model
 from django.db import models
 from django.test.testcases import TestCase, override_settings
 
 from openedx_tagging.core.tagging.system_defined_taxonomies.object_tags import (
-    OpenSystemObjectTag,
-    ClosedSystemObjectTag,
     ModelObjectTag,
     UserObjectTag,
     LanguageObjectTag,
@@ -41,7 +41,7 @@ class Meta:
         app_label = 'oel_tagging'
 
 
-class EmptyObjectTag(ModelObjectTag):
+class EmptyModelObjectTag(ModelObjectTag):
     """
     Model ObjectTag used for testing
     """
@@ -53,10 +53,8 @@ class Meta:
         managed = False
         app_label = 'oel_tagging'
 
-    tag_class_model = EmptyTestClass
-
 
-class EmptyModelObjectTag(ModelObjectTag):
+class NotDjangoModelObjectTag(ModelObjectTag):
     """
     Model ObjectTag used for testing
     """
@@ -68,12 +66,12 @@ class Meta:
         managed = False
         app_label = 'oel_tagging'
 
-    tag_class_model = EmptyTestModel
+    tag_class_model = EmptyTestClass
 
 
-class TestOpenObjectTag(OpenSystemObjectTag):
+class NotIdModelObjectTag(ModelObjectTag):
     """
-    Open ObjectTag used for testing
+    Model ObjectTag used for testing
     """
 
     system_defined_taxonomy_id = 3
@@ -83,18 +81,7 @@ class Meta:
         managed = False
         app_label = 'oel_tagging'
 
-
-class TestClosedObjectTag(ClosedSystemObjectTag):
-    """
-    Closed ObjectTag used for testing
-    """
-
-    system_defined_taxonomy_id = 2
-
-    class Meta:
-        proxy = True
-        managed = False
-        app_label = 'oel_tagging'
+    tag_class_model = EmptyTestModel
 
 
 class TestUserObjectTag(UserObjectTag):
@@ -110,100 +97,72 @@ class Meta:
         app_label = 'oel_tagging'
 
 
+@ddt.ddt
 class TestSystemDefinedObjectTags(TestTagTaxonomyMixin, TestCase):
     """
     Test for generic system defined object tags
     """
-    def test_open_valid_for(self):
-        #Valid
-        assert TestOpenObjectTag.valid_for(taxonomy=self.user_system_taxonomy)
-
-        # Not open system taxonomy
-        assert not TestOpenObjectTag.valid_for(taxonomy=self.system_taxonomy)
-
-        # Not system taxonomy
-        assert not TestOpenObjectTag.valid_for(taxonomy=self.taxonomy)
-
-    def test_closed_valid_for(self):
-        #Valid
-        assert TestClosedObjectTag.valid_for(taxonomy=self.system_taxonomy, tag=self.archaea)
-
-        # Not closed system taxonomy
-        assert not TestClosedObjectTag.valid_for(taxonomy=self.user_system_taxonomy, tag=self.archaea)
-
-        # Not system taxonomy
-        assert not TestClosedObjectTag.valid_for(taxonomy=self.taxonomy, tag=self.archaea)
-
-    def test_model_valid_for(self):
-        # Without associated model
-        assert not ModelObjectTag.valid_for(self.user_system_taxonomy)
-
-        # Associated class is not a Django model
-        assert not EmptyObjectTag.valid_for(self.user_system_taxonomy)
-
-        # Associated model has not 'id' field
-        assert not EmptyModelObjectTag.valid_for(self.user_system_taxonomy)
-
-        #Valid
-        assert TestUserObjectTag.valid_for(self.user_system_taxonomy)
-
-    def test_model_is_valid(self):
-        user = get_user_model()(
-            username='username',
-            email='email'
-        )
-        user.save()
-        valid_object_tag = UserObjectTag(
-            taxonomy=self.user_system_taxonomy,
-            object_id='id 1',
-            object_type='object',
-            value=user.id,
-        )
-        invalid_object_tag_1 = UserObjectTag(
-            taxonomy=self.user_system_taxonomy,
-            object_id='id 2',
-            object_type='object',
-            value='user_id',
-        )
-        invalid_object_tag_2 = UserObjectTag(
-            taxonomy=self.user_system_taxonomy,
-            object_id='id 2',
-            object_type='object',
-            value='10000',
-        )
-        invalid_object_tag_3 = UserObjectTag(
+    def test_system_defined_is_valid(self):
+        # Valid
+        assert TestUserObjectTag()._check_system_taxonomy(taxonomy=self.user_system_taxonomy)
+
+        # Null taxonomy
+        assert not UserObjectTag()._check_system_taxonomy()
+
+        # Not system defined taxonomy
+        assert not UserObjectTag()._check_system_taxonomy(taxonomy=self.taxonomy)
+
+        # Not connected with the taxonomy
+        assert not UserObjectTag()._check_system_taxonomy(taxonomy=self.user_system_taxonomy)
+
+    @ddt.data(
+        (EmptyModelObjectTag, False),  # Without associated model
+        (NotDjangoModelObjectTag, False),  # Associated class is not a Django model
+        (NotIdModelObjectTag, False),  # Associated model has not 'id' field
+        (ModelObjectTag, False), # Testing parent class validations
+        (TestUserObjectTag, True),  #Valid
+    )
+    @ddt.unpack
+    def test_model_object_is_valid(self, object_tag, assert_value):
+        args = {
+            'taxonomy': self.user_system_taxonomy,
+            'object_id': 'id',
+            'object_type': 'object',
+            'value': 'value',
+        }
+        result = object_tag(**args).is_valid(check_object=False, check_tag=False, check_taxonomy=True)
+        self.assertEqual(result, assert_value)
+
+    @ddt.data(
+        (None, True),  # Valid
+        ('user_id', False),  # Invalid user id
+        ('10000', False),  # User don't exits
+        (None, False),  # Testing parent class validations
+    )
+    @ddt.unpack
+    def test_user_object_is_valid(self, value, assert_value):
+        if assert_value:
+            user = get_user_model()(
+                username='username',
+                email='email'
+            )
+            user.save()
+            value = user.id
+
+        object_tag = TestUserObjectTag(
             taxonomy=self.user_system_taxonomy,
-            object_id='id 3',
+            object_id='id',
             object_type='object',
+            value=value,
         )
 
-        # Invalid user id
-        assert not invalid_object_tag_1.is_valid(
+        result = object_tag.is_valid(
             check_taxonomy=True,
             check_object=True,
             check_tag=True,
         )
 
-        # User don't exits
-        assert not invalid_object_tag_2.is_valid(
-            check_taxonomy=True,
-            check_object=True,
-            check_tag=True,
-        )
-
-        # Testing parent class validations
-        assert not invalid_object_tag_3.is_valid(
-            check_taxonomy=True,
-            check_object=True,
-            check_tag=True,
-        )
-
-        # Valid
-        assert valid_object_tag.is_valid(
-            check_taxonomy=True,
-            check_object=True,
-            check_tag=True,
-        )
+        self.assertEqual(result, assert_value)
 
 
 @override_settings(LANGUAGES=test_languages)