Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 0 additions & 14 deletions cms/djangoapps/contentstore/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@
from xmodule.modulestore.django import modulestore

from cms.djangoapps.models.settings.course_grading import CourseGradingModel
from common.djangoapps.student import auth
from common.djangoapps.student.roles import CourseCreatorRole, OrgContentCreatorRole
import openedx.core.djangoapps.content_staging.api as content_staging_api

from .utils import reverse_course_url, reverse_library_url, reverse_usage_url
Expand Down Expand Up @@ -377,15 +375,3 @@ def is_item_in_course_tree(item):
ancestor = ancestor.get_parent()

return ancestor is not None


def is_content_creator(user, org):
"""
Check if the user has the role to create content.

This function checks if the User has role to create content
or if the org is supplied, it checks for Org level course content
creator.
"""
return (auth.user_has_role(user, CourseCreatorRole()) or
auth.user_has_role(user, OrgContentCreatorRole(org=org)))
4 changes: 2 additions & 2 deletions cms/djangoapps/contentstore/views/course.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@
has_course_author_access,
has_studio_read_access,
has_studio_write_access,
has_studio_advanced_settings_access
has_studio_advanced_settings_access,
is_content_creator,
)
from common.djangoapps.student.roles import (
CourseInstructorRole,
Expand Down Expand Up @@ -102,7 +103,6 @@
update_course_details,
)
from .component import ADVANCED_COMPONENT_TYPES
from ..helpers import is_content_creator
from cms.djangoapps.contentstore.xblock_services.xblock_service import (
create_xblock_info,
)
Expand Down
4 changes: 2 additions & 2 deletions cms/djangoapps/contentstore/views/library.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
STUDIO_VIEW_USERS,
get_user_permissions,
has_studio_read_access,
has_studio_write_access
has_studio_write_access,
is_content_creator,
)
from common.djangoapps.student.roles import (
CourseInstructorRole,
Expand All @@ -42,7 +43,6 @@
from ..config.waffle import REDIRECT_TO_LIBRARY_AUTHORING_MICROFRONTEND
from ..utils import add_instructor, reverse_library_url
from .component import CONTAINER_TEMPLATES, get_component_templates
from ..helpers import is_content_creator
from cms.djangoapps.contentstore.xblock_services.xblock_service import create_xblock_info
from .user import user_with_role

Expand Down
5 changes: 5 additions & 0 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -1756,6 +1756,11 @@
# API Documentation
'drf_yasg',

# Tagging
'openedx_tagging.core.tagging.apps.TaggingConfig',
'openedx.features.content_tagging',

# Features
'openedx.features.course_duration_limits',
'openedx.features.content_type_gating',
'openedx.features.discounts',
Expand Down
12 changes: 12 additions & 0 deletions common/djangoapps/student/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,18 @@ def has_studio_read_access(user, course_key):
return bool(STUDIO_VIEW_CONTENT & get_user_permissions(user, course_key))


def is_content_creator(user, org):
"""
Check if the user has the role to create content.

This function checks if the User has role to create content
or if the org is supplied, it checks for Org level course content
creator.
"""
return (user_has_role(user, CourseCreatorRole()) or
user_has_role(user, OrgContentCreatorRole(org=org)))


def add_users(caller, role, *users):
"""
The caller requests adding the given users to the role. Checks that the caller
Expand Down
4 changes: 4 additions & 0 deletions lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -3216,6 +3216,10 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring
# Course Goals
'lms.djangoapps.course_goals.apps.CourseGoalsConfig',

# Tagging
'openedx_tagging.core.tagging.apps.TaggingConfig',
'openedx.features.content_tagging',

# Features
'openedx.features.calendar_sync',
'openedx.features.course_bookmarks',
Expand Down
Empty file.
19 changes: 19 additions & 0 deletions openedx/features/content_tagging/admin.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
""" Tagging app admin """
from django.contrib import admin

from .models import ContentTaxonomy


class ContentTaxonomyOrgAdmin(admin.TabularInline):
model = ContentTaxonomy.org_owners.through


class ContentTaxonomyAdmin(admin.ModelAdmin):
"""
Admin form for the content taxonomy table.
"""

inlines = (ContentTaxonomyOrgAdmin,)


admin.site.register(ContentTaxonomy, ContentTaxonomyAdmin)
83 changes: 83 additions & 0 deletions openedx/features/content_tagging/api.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
"""
Content Tagging APIs
"""
from typing import List

import openedx_tagging.core.tagging.api as oel_tagging
from django.db.models import QuerySet
from organizations.models import Organization

from .models import ContentTaxonomy


def create_taxonomy(
name,
org_owners: List[Organization] = None,
description=None,
enabled=True,
required=False,
allow_multiple=False,
allow_free_text=False,
) -> ContentTaxonomy:
"""
Creates, saves, and returns a new ContentTaxonomy with the given attributes.

If `org_owners` is empty/None, then the returned taxonomy is enabled for all organizations.
"""
taxonomy = ContentTaxonomy.objects.create(
name=name,
description=description,
enabled=enabled,
required=required,
allow_multiple=allow_multiple,
allow_free_text=allow_free_text,
)
if org_owners:
set_taxonomy_org_owners(taxonomy, org_owners)
return taxonomy


def set_taxonomy_org_owners(
taxonomy: ContentTaxonomy,
org_owners: List[Organization] = None,
) -> ContentTaxonomy:
"""
Updates the list of org_owners on the given taxonomy.

If `org_owners` is empty/None, then the returned taxonomy is enabled for all organizations.
"""
if org_owners:
taxonomy.org_owners.set(org_owners)
else:
taxonomy.org_owners.clear()
return taxonomy


def get_taxonomies_for_org(org_owner: Organization = None, enabled=True) -> QuerySet:
"""
Returns a queryset containing the enabled ContentTaxonomies owned by the given org, sorted by name.

If you want all Taxonomies, not just ContentTaxonomies, use the get_taxonomies API method, and use `enabled_for_org`
on any returned ContentTaxonomies to filter out by org ownership.

If you want the disabled ContentTaxonomies, pass enabled=False.
If you want all ContentTaxonomies (both enabled and disabled), pass enabled=None.
"""
return (
ContentTaxonomy.objects.filter_enabled(
org_owner,
enabled,
)
.order_by("name", "id")
.select_subclasses()
)


# Expose the oel_tagging APIs that we haven't overridden here:

get_taxonomies = oel_tagging.get_taxonomies
get_taxonomy = oel_tagging.get_taxonomy
get_tags = oel_tagging.get_tags
resync_object_tags = oel_tagging.resync_object_tags
get_object_tags = oel_tagging.get_object_tags
tag_object = oel_tagging.tag_object
12 changes: 12 additions & 0 deletions openedx/features/content_tagging/apps.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
"""
Define the content tagging Django App.
"""

from django.apps import AppConfig


class ContentTaggingConfig(AppConfig):
"""App config for the content tagging feature"""

default_auto_field = "django.db.models.BigAutoField"
name = "openedx.features.content_tagging"
72 changes: 72 additions & 0 deletions openedx/features/content_tagging/migrations/0001_initial.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# Generated by Django 3.2.19 on 2023-06-19 22:44

from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):
initial = True

dependencies = [
("organizations", "0003_historicalorganizationcourse"),
("oel_tagging", "0001_initial"),
]

operations = [
migrations.CreateModel(
name="ContentTaxonomy",
fields=[
(
"taxonomy_ptr",
models.OneToOneField(
auto_created=True,
on_delete=django.db.models.deletion.CASCADE,
parent_link=True,
primary_key=True,
serialize=False,
to="oel_tagging.taxonomy",
),
),
],
options={
"verbose_name_plural": "ContentTaxonomies",
},
bases=("oel_tagging.taxonomy",),
),
migrations.CreateModel(
name="ContentTaxonomyOrg",
fields=[
(
"id",
models.BigAutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
(
"org",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE,
to="organizations.organization",
),
),
(
"taxonomy",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE,
to="content_tagging.contenttaxonomy",
),
),
],
),
migrations.AddField(
model_name="contenttaxonomy",
name="org_owners",
field=models.ManyToManyField(
through="content_tagging.ContentTaxonomyOrg",
to="organizations.Organization",
),
),
]
Empty file.
103 changes: 103 additions & 0 deletions openedx/features/content_tagging/models.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
"""
Content Tagging models
"""
from django.db import models
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey, UsageKey
from openedx_tagging.core.tagging.models import Taxonomy, TaxonomyManager
from organizations.models import Organization


class ContentTaxonomyManager(TaxonomyManager):
"""
Manages ContentTaxonomy objects, providing custom utility methods.

Inherits from InheritanceManager so that subclasses of ContentTaxonomy can be selected in a queryset.
"""

def filter_enabled(self, org: Organization = None, enabled=True) -> models.QuerySet:
"""
Returns a query set filtered to return the enabled ContentTaxonomy objects owned by the given org.
"""
queryset = self
if enabled is not None:
queryset = queryset.filter(enabled=enabled)
org_filter = models.Q(org_owners=None)
if org:
org_filter |= models.Q(org_owners=org)
queryset = queryset.filter(org_filter)
return queryset


class ContentTaxonomy(Taxonomy):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that it makes sense to make this a subclass of Taxonomy. Because it's not a true subclass, right? When ObjectTag calls self.taxonomy.validate_object_tag(self), there's no virtual dispatch involved. Regardless of whether there's a ContentTaxonomy or not, the self.taxonomy property always returns a Taxonomy instance, and only the base class's validate_object_tag() will be called. You would need to use something like django-polymorphic to get this to work (which is not what I am recommending; it's too much magic and we have enough dependencies already).

Copy link
Contributor Author

@pomegranited pomegranited Jun 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh bummer. You're absolutely right. I had fooled myself into thinking this was ok in my tests, because when I created the invalid tags, their taxonomies are ContentTaxonomy instances. But they're not after they're refetched from the database :(

I guess I have to rethink this architecture then..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think we can have the same problem with system-defined taxonomies

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to check if a model is an instance of ContentTaxonomy? (with isistance maybe?)

I remember the validate_object_tag function, it was first thought to be inside the API. We could check and depending of the instance, to call one and other function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exact same thing has happened to me before... sorry I didn't spot this sooner.

I didn't suggest any solutions because I didn't want to bias you one way or another, but I think there are still some nice ways to achieve a similar architecture without too many changes. One thing to think about for example would be if there is only one database/django model for Taxonomy but it has a field called type and that type determines the runtime behavior, and loads a different class or plugin to handle the implementation details. Basically, put your subclasses and hierarchy into pure python classes and keep the SQL data model simpler or even the same for all of them. But there are other ways to go too, that's just one idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradenmacdonald @pomegranited

It still uses only one query, but that query does an additional JOIN per registered subclass. So whether or not this is acceptable may depend on how many subclasses there are. If it's 1-2, should be fine. But if there are many, the query performance will be slower, even though it's still technically a single query

Adding more context with this in mind. For system-defined taxonomies, we were thinking of creating a subclass for each taxonomy, since each one had its special way of validating (validate_object_tag). With the current approach, in the short term it would already be slow.

But, I've been thinking that at the system taxonomies level, the type field (Language, Author, etc) could be implemented to determine which validation to run. That way we would only have the SystemDefinedTaxonomy subclass and it would not be necessary to create a subclass for each Taxonomy.

So far we will have two subclasses: ContentTaxonomy and SystemDefinedTaxonomy

Copy link
Contributor

@ormsbee ormsbee Jul 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pomegranited that seems fine with me in principle, as long as the code and APIs are carefully constructed to make sure select_subclasses is used. But this is really more of an @ormsbee question so I'd like to hear his thoughts.

I haven't used it before, but as long as the number of subclasses is relatively small, I'm fine with a few joins.

Adding more context with this in mind. For system-defined taxonomies, we were thinking of creating a subclass for each taxonomy, since each one had its special way of validating (validate_object_tag). With the current approach, in the short term it would already be slow.

What are the other types of validation that have to happen on ObjectTag for other taxonomy types? The reason I ask is that I think it's a little odd for the ObjectTag to look to their parent Taxonomy subclass to validate themselves–basically looking upward into the thing containing them. It also seems like unnecessary coupling, since we might want to tag one than one type of thing with the same Taxonomy. In fact we're effectively already doing that here with CourseKey and UsageKey.

Maybe it's ObjectTag that needs the subclassing? It could be made abstract, and have subclasses like CourseTag, BlockTag, etc? And instead of having a completely generic object_id field, we make the object_id return a different real field value in the various subclasses (sort of like what pk does for Django models)? Then we could have the actual field type there–e.g. a CourseKeyField.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the other types of validation that have to happen on ObjectTag for other taxonomy types?

In the case of system-defined taxonomies, the validations that are added are more on the tag side than on the object.

  • Language: Verify if the language is on Django LANGUAGES
  • Organization: Validate that the organization exists and is active.
  • Author: Validate that the User exists and is active.

Later we can add more taxonomies that have their own validation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ormsbee @bradenmacdonald

It still uses only one query, but that query does an additional JOIN per registered subclass. So whether or not this is acceptable may depend on how many subclasses there are. If it's 1-2, should be fine.

Yeah, as @ChrisChV noted, there's going to be a lot of these.. so using InheritanceManager like I've done in openedx/openedx-learning#60 not a good idea.

The reason I ask is that I think it's a little odd for the ObjectTag to look to their parent Taxonomy subclass to validate themselves–basically looking upward into the thing containing them.

I agree that ObjectTag.is_valid is an awkward-looking property -- I added it as a convenience method, because we're going to need the REST API to return an is_valid flag for object tags, so that the frontend can highlight invalid tags for the content authors to correct.

In the design stage, we pushed the ObjectTag validation into the Taxonomy class so we could avoid having to subclass both Taxonomies and ObjectTags, and link them together somehow (see below for a proposed way to do this). This made Taxonomy.validate_object_tag() strange, but it was intended to keep the class complexity down. But that decision was clearly flawed.

Maybe it's ObjectTag that needs the subclassing? It could be made abstract, and have subclasses like CourseTag, BlockTag, etc?

Ok sure -- there weren't any requirements in the spec that made those particular distinctions necessary, however..

We could subclass ObjectTag to encapsulate the "closed taxonomy tags" vs "free text tags" distinction. To do this, we need a way to connect a Taxonomy with its appropriate ObjectTag subclass. To try this out, I drafted open-craft/openedx-learning#2; see Taxonony.object_tag_classes for an example of how this could work.

There, I also allowed for custom ObjectTag classes stored with a Taxonomy that could be used to validate system tags for the different system taxonomy use cases listed above. We could replace the SystemTaxonomy subclasses and their validation with class methods on ObjectTag subclasses instead. Python is bad at polymorphism across class methods, but with care, we can do it, e.g

  • Taxonomy subclass LanguageTaxonomy.get_tags() was going to return a dynamically-determined list of Tags created from the django settings.LANGUAGES.
    LanguageObjectTag could implement a class method to do the same thing.
  • AuthorTaxonomy has too many available tags to fetch them all in a list, so it would implement autocomplete_tags() instead, and just return the first N dynamically-created Tags with the given prefix.
    AuthorObjectTag could implement a class method to do the same thing.

So, what do you think of something like open-craft/openedx-learning#2 instead?

Also, if we do this, it'd be best to stop returning ObjectTag Django Models from the API altogether, and instead use the models as data to populate proper python classes and subclasses, as @bradenmacdonald originally suggested.

If we found that we still need Taxonomy subclasses, we could add a Taxonomy.taxonomy_class field+property like Taxonony.object_tag_classes, and load these Taxonomy subclasses on the fly in the API using a generator, as shown with get_object_tags().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ormsbee @bradenmacdonald @ChrisChV

Ok, this is looking really good. I'm going to close this PR in favor of #32661, so can we continue this conversation there?

"""
Taxonomy used for tagging content-related objects.

ContentTaxonomies can be owned by one or more organizations, which allows content authors from that organization to
create taxonomies, edit the taxonomy fields, and add/edit/remove Tags linked to the taxonomy.

.. no_pii:
"""

objects = ContentTaxonomyManager()

org_owners = models.ManyToManyField(Organization, through="ContentTaxonomyOrg")

class Meta:
verbose_name_plural = "ContentTaxonomies"

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 this ContentTaxonomy.

Extends the superclass method by adding its own object checks to ensure:

* object_tag.object_id is a valid UsageKey or CourseKey, and
* object_tag.object_id's "org" is enabled for this taxonomy.
"""
if check_object:
# ContentTaxonomies require object_id to be a valid CourseKey or UsageKey
try:
object_key = UsageKey.from_string(object_tag.object_id)
except InvalidKeyError:
try:
object_key = CourseKey.from_string(object_tag.object_id)
except InvalidKeyError:
return False

# ...and object to be in an org that is enabled for this taxonomy.
if not self.enabled_for_org(object_key.org):
return False

return super().validate_object_tag(
object_tag,
check_taxonomy=check_taxonomy,
check_tag=check_tag,
check_object=check_object,
)

def enabled_for_org(self, org_short_name: str) -> bool:
"""
Returns True if this taxonomy is enabled for the given organization.
"""
enabled = self.enabled
if self.org_owners.count():
enabled &= self.org_owners.filter(
contenttaxonomyorg__org__short_name=org_short_name,
).exists()
return enabled


class ContentTaxonomyOrg(models.Model):
"""
Represents the many-to-many relationship between ContentTaxonomies and Organizations.
"""

taxonomy = models.ForeignKey(ContentTaxonomy, on_delete=models.CASCADE)
org = models.ForeignKey(Organization, on_delete=models.CASCADE)
Loading