System defined taxonomies

[ChrisChV]

Description

Subclasses related to build System-defined taxonomies:

• Docs updated to reflect the new architecture of taxonomies.
• models.py moved to models/base.py
• tag_ref setter to allow override how to set a tag on an object_tag
• object_tag added to Taxonomy
• Base system-defined subclasses added.
• Language tags fixture.
• Taxonomy.system_defined updated to be a hardcoded property.
• TaxonomyAdmin updated to avoid edit and delete system taxonomies: openedx@1847248
• TagAdmin updated to avoid edit and delete tags from system taxonomies: openedx@1847248
• TagAmdin updated to avoid create Tags on system taxonomies: openedx@1847248

Testing instructions

Manual testing is possible but not very useful, since we don't yet have views for these models and APIs.

But you can check that the added tests cover the expected polymorphism behavior supported by this change, and that tests are still covering 100% of the openedx_tagging module.

Testing admin:

• On the backend shell create Taxonomies an Tags:

from openedx_tagging.core.tagging.models import Taxonomy, Tag
closed_taxonomy = Taxonomy(name='Test1', allow_free_text=False)
closed_taxonomy.save()
open_taxonomy = Taxonomy(name='Test2', allow_free_text=True)
open_taxonomy.save()
system_taxonomy = Taxonomy(name='Language', allow_free_text=False)
from openedx_tagging.core.tagging.models import LanguageTaxonomy
system_taxonomy.taxonomy_class = LanguageTaxonomy
system_taxonomy.save()
tag = Tag(taxonomy=closed_taxonomy, value='test')
tag.save()
tag = Tag(taxonomy=system_taxonomy, value='es')
tag.save()

• Go to Django admin and check that you can see all taxonomies.
• Click on the language taxonomy and check that you cannot edit or delete the taxonomy.
• Go to Tags and check that you can see all tags.
• Click on the language tag and check that you cannot edit or delete the taxonomy.
• Create a Tag and check that you cannot add the tag to the open taxonomy nor system taxonomy

Other information

Relates to openedx/modular-learning#77

[pomegranited]

Hi @ChrisChV, this is really interesting.. I couldn't see how the system taxonomies would fit together until I read your code! Very cool.

I've requested some changes, but: Feel free to wait until we've got approval at least in principle on openedx#62 before taking time to apply them, just in case that approach gets changed again.

Also, feel free to push back on anything that seems unreasonable?

[pomegranited]

It's ok that all of these are explicitly laid out, but you can just say this if you want to:

Suggested change

	result = object_tag.is_valid(
	check_taxonomy=True,
	check_object=True,
	check_tag=True,
	)
	self.assertEqual(result, assert_value)
	assert object_tag.is_valid() == assert_value

[ChrisChV]

All test has been updated with this

@pomegranited In your experience, is there a difference in using one approach or another? For my tests I have always used self.assertEqual()

[pomegranited]

@ChrisChV There's no functional difference.. I just think using plain python wherever possible looks cleaner. But I'm not that fussy about it -- we're using TestClass as a base, so all of its .assert* methods are available regardless.

[pomegranited]

I really like how you used external_id to store the language code for the language object tag , and then used the full language name as the value. So when we actually fetch an ObjectTag for display, we have a nice human-readable value to show.

I'd like to support something like that here with model tags. So that when we have a tag that references a User or Organization, we can show the User/Organization full name instead of the ID. Plus, that name will always be up-to-date, because we're fetching it from the source table.

So I'm wondering, can we do something like this:

class ObjectTag:
    @property
    def value_pretty(self):
        """
        Returns a human-friendly version of the tag value stored here.
        
        Subclasses should override this if they don't store human-readable values.
        """
        return self.value

And then we can have here:

class ModelObjectTag(...):
    @property
    def tag_class_model_pretty_field(self):
        """
        Name of the field to use on the tag_class_model instance to which stores the "pretty value" of the tag.
        """
        raise NotImplementedError

    @property
    def value_pretty(self):
        """
        Returns a human-friendly version of the tag value stored here.
        
        Subclasses should override this if they don't store human-readable values.
        """
        value = self.value
        try:
            instance = self.tag_class_model.objects.get(pk=value)
            value = getattr(instance, self.tag_class_model_pretty_field, value)
        except self.tag_class_model.DoesNotExist:
            # Means the tag is invalid, but that's ok here
            pass
            
        return value
...

class UserObjectTag(ModelObjectTag):
    @property
    def tag_class_model_pretty_field(self):
        """
        Name of the field to use on the tag_class_model instance to which stores the "pretty value" of the tag.
        """
        return "full_name"

[ChrisChV]

Updated to use this approach #3 (comment)

[bradenmacdonald]

Please make sure we're thinking about how to translate these before displaying them in the UI. For a German user, this language tag needs to appear as "Deutsch", not "German".

We should probably use a similar mechanism for users, storing their user ID as the tag value but displaying their username or their "full name (username)" in the UI.

You don't have to implement this in this PR (it's a bad idea to, actually) - just think about how it will work in the future.

[pomegranited]

We should probably use a similar mechanism for users, storing their user ID as the tag value but displaying their username or their "full name (username)" in the UI.

What do you think of this idea?

I think it could be adapted for translations too..

[bradenmacdonald]

I think something like that would make sense, yes. Just not clear on how it would be adapted to the current approach. But let's not worry too much about that just yet; I don't want to muddy the waters any further here :)

[pomegranited]

@ChrisChV Since we've gone back to subclassing Taxonomy again, I wonder if we could create a ModelTaxonomy class, which can:

• Create Tag objects on the fly when new ObjectTags are added
  • set external_id=model.pk
  • set value = value we want to show to the world, e.g. User.username or Organization.name
  • "Resync" could somehow re-fetch the related object and update this value? Or maybe we need another method.
• Can the ModelTaxonomy can be "closed" even though the Tag values don't exist (yet)?
• Can we return "autocomplete_tags" based on the values available in the linked model?

[ChrisChV]

Create Tag objects on the fly when new ObjectTags are added

This has been one of the rejected options for creating Tags. But now that I see it, it's easier than it seemed and has many more advantages. Yes, I will build this type of taxonomy

[ChrisChV]

@pomegranited I updated the ADR e031a5e

[pomegranited]

Hi @ChrisChV , I started suggesting things, but then decided it would be clearer to put the changes into a PR: #6

What do you think about that approach instead?

I'm also thinking we could rearrange the model code a bit to shorten the (now quite long) models.py:

• move the base models (Tag, Taxonomy, ObjectTag) to openedx_tagging/core/tagging/models/base.py
• move the system-defined models to openedx_tagging/core/tagging/models/system_defined.py
• import the models we want people to use outside this library (Tag, Taxonomy, ObjectTag, LanguageTaxonomy, UserModelTaxonomy, etc.) in openedx_tagging/core/tagging/models/__init__.py

If you agree, feel free to split out a test_system_defined_models.py as well.

[pomegranited]

If you have time, could you a test for a simple, fully dynamic SystemTaxonomy as @bradenmacdonald suggested here?

[ChrisChV]

I'm also thinking we could rearrange the model code a bit to shorten the (now quite long) models.py:

@pomegranited Yes, I agree with that. Thanks. I will add it

[pomegranited]

Closed in favor of openedx#67