Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Continued simplification of Taxonomy model, squashed migrations [FC-0030] #33438

Merged
merged 4 commits into from
Oct 10, 2023
Merged
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
2 changes: 0 additions & 2 deletions openedx/core/djangoapps/content_tagging/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ def create_taxonomy(
name: str,
description: str = None,
enabled=True,
required=False,
allow_multiple=False,
allow_free_text=False,
) -> Taxonomy:
Expand All @@ -29,7 +28,6 @@ def create_taxonomy(
name=name,
description=description,
enabled=enabled,
required=required,
allow_multiple=allow_multiple,
allow_free_text=allow_free_text,
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# Generated by Django 3.2.21 on 2023-10-09 23:12

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


class Migration(migrations.Migration):

replaces = [
('content_tagging', '0001_initial'),
('content_tagging', '0002_system_defined_taxonomies'),
('content_tagging', '0003_system_defined_fixture'),
('content_tagging', '0004_system_defined_org'),
('content_tagging', '0005_auto_20230830_1517'),
('content_tagging', '0006_simplify_models'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were starting to get problems with some of these migrations, referring to models that have been deleted or depending on migrations which depended on fixtures, which were later changed or deleted (note to self: migrations should never reference fixtures). So I've squashed the migrations here which makes it much simpler and more robust. I'd really like to get the squashed version into Quince. If possible, I'd like to squash the migrations in oel_tagging too but that would be a separate PR.

]

initial = True

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

operations = [
migrations.CreateModel(
name='ContentObjectTag',
fields=[
],
options={
'proxy': True,
'indexes': [],
'constraints': [],
},
bases=('oel_tagging.objecttag',),
),
migrations.CreateModel(
name='TaxonomyOrg',
fields=[
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('rel_type', models.CharField(choices=[('OWN', 'owner')], default='OWN', max_length=3)),
('org', models.ForeignKey(default=None, help_text='Organization that is related to this taxonomy.If None, then this taxonomy is related to all organizations.', null=True, on_delete=django.db.models.deletion.CASCADE, to='organizations.organization')),
('taxonomy', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_tagging.taxonomy')),
],
),
migrations.AddIndex(
model_name='taxonomyorg',
index=models.Index(fields=['taxonomy', 'rel_type'], name='content_tag_taxonom_b04dd1_idx'),
),
migrations.AddIndex(
model_name='taxonomyorg',
index=models.Index(fields=['taxonomy', 'rel_type', 'org'], name='content_tag_taxonom_70d60b_idx'),
),
]
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,6 @@ def load_system_defined_taxonomies(apps, schema_editor):
org_taxonomy.taxonomy_class = ContentOrganizationTaxonomy
org_taxonomy.save()

# Adding taxonomy class to the language taxonomy
language_taxonomy = Taxonomy.objects.get(id=-1)
ContentLanguageTaxonomy = apps.get_model("content_tagging", "ContentLanguageTaxonomy")
language_taxonomy.taxonomy_class = ContentLanguageTaxonomy
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 confused.. The ContentAuthorTaxonomy and ContentOrganizationTaxonomy models have also been deleted, so I would expect them to cause problems just like the ContentLanguageTaxonomy does now? No worries if they're not, I'm just curious.

language_taxonomy.save()


def revert_system_defined_taxonomies(apps, schema_editor):
"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ def load_system_defined_org_taxonomies(apps, _schema_editor):
Associates the system defined taxonomy Language (id=-1) to all orgs and
removes the ContentOrganizationTaxonomy (id=-3) from the database
"""
TaxonomyOrg = apps.get_model("content_tagging", "TaxonomyOrg")
TaxonomyOrg.objects.create(id=-1, taxonomy_id=-1, org=None)
# Disabled for now as the way that this taxonomy is created has changed.
# TaxonomyOrg = apps.get_model("content_tagging", "TaxonomyOrg")
# TaxonomyOrg.objects.create(id=-1, taxonomy_id=-1, org=None)

Taxonomy = apps.get_model("oel_tagging", "Taxonomy")
Taxonomy.objects.get(id=-3).delete()
Expand All @@ -20,8 +21,8 @@ def revert_system_defined_org_taxonomies(apps, _schema_editor):
Deletes association of system defined taxonomy Language (id=-1) to all orgs and
creates the ContentOrganizationTaxonomy (id=-3) in the database
"""
TaxonomyOrg = apps.get_model("content_tagging", "TaxonomyOrg")
TaxonomyOrg.objects.get(id=-1).delete()
# TaxonomyOrg = apps.get_model("content_tagging", "TaxonomyOrg")
# TaxonomyOrg.objects.get(id=-1).delete()

Taxonomy = apps.get_model("oel_tagging", "Taxonomy")
org_taxonomy = Taxonomy(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
from django.db import migrations


def mark_language_taxonomy_as_all_orgs(apps, _schema_editor):
"""
Associates the system defined taxonomy Language (id=-1) to all orgs.
"""
TaxonomyOrg = apps.get_model("content_tagging", "TaxonomyOrg")
TaxonomyOrg.objects.update_or_create(taxonomy_id=-1, defaults={"org": None})


def revert_mark_language_taxonomy_as_all_orgs(apps, _schema_editor):
"""
Deletes association of system defined taxonomy Language (id=-1) to all orgs.
"""
TaxonomyOrg = apps.get_model("content_tagging", "TaxonomyOrg")
TaxonomyOrg.objects.get(taxonomy_id=-1, org=None).delete()


class Migration(migrations.Migration):
dependencies = [
('content_tagging', '0001_squashed'),
("oel_tagging", "0012_language_taxonomy"),
]

operations = [
migrations.RunPython(mark_language_taxonomy_as_all_orgs, revert_mark_language_taxonomy_as_all_orgs),
]
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,7 @@ def check_taxonomy(
name,
description=None,
enabled=True,
required=False,
allow_multiple=False,
allow_multiple=True,
allow_free_text=False,
system_defined=False,
visible_to_authors=True,
Expand All @@ -47,7 +46,6 @@ def check_taxonomy(
assert data["name"] == name
assert data["description"] == description
assert data["enabled"] == enabled
assert data["required"] == required
assert data["allow_multiple"] == allow_multiple
assert data["allow_free_text"] == allow_free_text
assert data["system_defined"] == system_defined
Expand Down Expand Up @@ -350,7 +348,6 @@ def test_create_taxonomy(self, user_attr, expected_status):
"name": "taxonomy_data",
"description": "This is a description",
"enabled": True,
"required": True,
"allow_multiple": True,
}

Expand Down Expand Up @@ -444,7 +441,6 @@ def test_update_taxonomy(self, user_attr, taxonomy_attr, expected_status):
"name": "new name",
"description": taxonomy.description,
"enabled": taxonomy.enabled,
"required": taxonomy.required,
},
)

Expand Down Expand Up @@ -540,7 +536,6 @@ def test_patch_taxonomy(self, user_attr, taxonomy_attr, expected_status):
"name": "new name",
"description": taxonomy.description,
"enabled": taxonomy.enabled,
"required": taxonomy.required,
},
)

Expand Down Expand Up @@ -668,13 +663,13 @@ def setUp(self):
)

self.multiple_taxonomy = Taxonomy.objects.create(name="Multiple Taxonomy", allow_multiple=True)
self.required_taxonomy = Taxonomy.objects.create(name="Required Taxonomy", required=True)
self.single_value_taxonomy = Taxonomy.objects.create(name="Required Taxonomy", allow_multiple=False)
for i in range(20):
# Valid ObjectTags
Tag.objects.create(taxonomy=self.tA1, value=f"Tag {i}")
Tag.objects.create(taxonomy=self.tA2, value=f"Tag {i}")
Tag.objects.create(taxonomy=self.multiple_taxonomy, value=f"Tag {i}")
Tag.objects.create(taxonomy=self.required_taxonomy, value=f"Tag {i}")
Tag.objects.create(taxonomy=self.single_value_taxonomy, value=f"Tag {i}")

self.open_taxonomy = Taxonomy.objects.create(name="Enabled Free-Text Taxonomy", allow_free_text=True)

Expand All @@ -685,7 +680,7 @@ def setUp(self):
rel_type=TaxonomyOrg.RelType.OWNER,
)
TaxonomyOrg.objects.create(
taxonomy=self.required_taxonomy,
taxonomy=self.single_value_taxonomy,
org=self.orgA,
rel_type=TaxonomyOrg.RelType.OWNER,
)
Expand Down
45 changes: 30 additions & 15 deletions openedx/core/djangoapps/content_tagging/tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,53 @@

from unittest.mock import patch

from django.core.management import call_command
from django.test import override_settings
from edx_toggles.toggles.testutils import override_waffle_flag
from openedx_tagging.core.tagging.models import LanguageTaxonomy, Tag, Taxonomy
from openedx_tagging.core.tagging.models import Tag, Taxonomy
from organizations.models import Organization

from common.djangoapps.student.tests.factories import UserFactory
from openedx.core.djangolib.testing.utils import skip_unless_cms
from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase

from .. import api
from ..models import TaxonomyOrg
from ..models.base import TaxonomyOrg
from ..toggles import CONTENT_TAGGING_AUTO
from ..types import ContentKey

LANGUAGE_TAXONOMY_ID = -1


class LanguageTaxonomyTestMixin:
"""
Mixin for test cases that expect the Language System Taxonomy to exist.
"""

def setUp(self):
"""
When pytest runs, it creates the database by inspecting models, not by
running migrations. So data created by our migrations is not present.
In particular, the Language Taxonomy is not present. So this mixin will
create the taxonomy, simulating the effect of the following migrations:
1. openedx_tagging.core.tagging.migrations.0012_language_taxonomy
2. content_tagging.migrations.0007_system_defined_org_2
"""
super().setUp()
Taxonomy.objects.get_or_create(id=-1, defaults={
"name": "Languages",
"description": "Languages that are enabled on this system.",
"enabled": True,
"allow_multiple": False,
"allow_free_text": False,
"visible_to_authors": True,
"_taxonomy_class": "openedx_tagging.core.tagging.models.system_defined.LanguageTaxonomy",
})
TaxonomyOrg.objects.get_or_create(taxonomy_id=-1, defaults={"org": None})


@skip_unless_cms # Auto-tagging is only available in the CMS
@override_waffle_flag(CONTENT_TAGGING_AUTO, active=True)
class TestAutoTagging(ModuleStoreTestCase):
class TestAutoTagging(LanguageTaxonomyTestMixin, ModuleStoreTestCase):
"""
Test if the Course and XBlock tags are automatically created
"""
Expand All @@ -51,17 +77,6 @@ def _check_tag(self, object_key: ContentKey, taxonomy_id: int, value: str | None

return True

@classmethod
def setUpClass(cls):
# Run fixtures to create the system defined tags
call_command("loaddata", "--app=oel_tagging", "language_taxonomy.yaml")

# Enable Language taxonomy for all orgs
language_taxonomy = LanguageTaxonomy.objects.get(id=LANGUAGE_TAXONOMY_ID)
TaxonomyOrg.objects.create(taxonomy=language_taxonomy, org=None)

super().setUpClass()

def setUp(self):
super().setUp()
# Create user
Expand Down
2 changes: 1 addition & 1 deletion requirements/constraints.txt
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ libsass==0.10.0
click==8.1.6

# pinning this version to avoid updates while the library is being developed
openedx-learning==0.2.0
openedx-learning==0.2.3

# lti-consumer-xblock 9.6.2 contains a breaking change that makes
# existing custom parameter configurations unusable.
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -783,7 +783,7 @@ openedx-filters==1.6.0
# via
# -r requirements/edx/kernel.in
# lti-consumer-xblock
openedx-learning==0.2.0
openedx-learning==0.2.3
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/kernel.in
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/development.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1316,7 +1316,7 @@ openedx-filters==1.6.0
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
# lti-consumer-xblock
openedx-learning==0.2.0
openedx-learning==0.2.3
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/doc.txt
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/doc.txt
Original file line number Diff line number Diff line change
Expand Up @@ -923,7 +923,7 @@ openedx-filters==1.6.0
# via
# -r requirements/edx/base.txt
# lti-consumer-xblock
openedx-learning==0.2.0
openedx-learning==0.2.3
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/testing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -990,7 +990,7 @@ openedx-filters==1.6.0
# via
# -r requirements/edx/base.txt
# lti-consumer-xblock
openedx-learning==0.2.0
openedx-learning==0.2.3
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
Expand Down
Loading