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 2 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", "0012_language_taxonomy"),
Copy link
Contributor

@pomegranited pomegranited Oct 10, 2023

Choose a reason for hiding this comment

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

Ach.. I'm afraid this line caused an issue when I was testing openedx/openedx-learning#95 :(

Changing it to this fixes the issue, but means we need to merge/tag openedx/openedx-learning#95 first:

Suggested change
("oel_tagging", "0012_language_taxonomy"),
("oel_tagging", "0001_squashed"),

Here's the steps I took:

  1. Zero-ed the migrations oel_tagging and content_tagging, and ensured all the tables for these apps were dropped.
  2. Checked out edx-platform master (b353019), and installed openedx-learning v0.2.0
  3. Ran migrations, which applied fine:
    $ ./manage.py lms migrate
    Running migrations:
      Applying oel_tagging.0001_initial... OK
      Applying oel_tagging.0002_auto_20230718_2026... OK
      Applying oel_tagging.0003_auto_20230721_1238... OK
      Applying oel_tagging.0004_auto_20230723_2001... OK
      Applying oel_tagging.0005_language_taxonomy...Installed 185 object(s) from 1 fixture(s) OK
      Applying content_tagging.0001_initial... OK
      Applying content_tagging.0002_system_defined_taxonomies... OK
      Applying content_tagging.0003_system_defined_fixture... OK
      Applying content_tagging.0004_system_defined_org... OK
      Applying content_tagging.0005_auto_20230830_1517... OK
      Applying content_tagging.0006_simplify_models... OK
      Applying oel_tagging.0006_alter_objecttag_unique_together... OK
      Applying oel_tagging.0006_auto_20230802_1631... OK
      Applying oel_tagging.0007_tag_import_task_log_null_fix... OK
      Applying oel_tagging.0008_taxonomy_description_not_null... OK
      Applying oel_tagging.0009_alter_objecttag_object_id... OK
      Applying oel_tagging.0010_cleanups... OK
    
  4. Checked out this branch and installed the branch from Squash Tagging Migrations [FC-0030] openedx-learning#95.
  5. Applied migrations, which errored out:
    django.db.migrations.exceptions.InconsistentMigrationHistory: Migration content_tagging.0001_squashed is applied before its dependency oel_tagging.0012_language_taxonomy on database 'default'.
    
  6. Made the above recommended change, and was able to proceed with applying the migrations:
    $ ./manage.py lms migrate
    Running migrations:
      Applying oel_tagging.0011_remove_required... OK
      Applying oel_tagging.0012_language_taxonomy... OK
      Applying content_tagging.0007_system_defined_org_2... OK
    
  7. Tested rollback, which worked OK1
    $ ./manage.py lms migrate content_tagging zero
    $ ./manage.py lms migrate oel_tagging zero
    
  8. Tested applying the squashed migrations, also worked fine:
    $ ./manage.py lms migrate
    Running migrations:
      Applying oel_tagging.0001_squashed... OK
      Applying oel_tagging.0012_language_taxonomy... OK
      Applying content_tagging.0001_squashed... OK
      Applying content_tagging.0007_system_defined_org_2... OK   
    

1 I always get errors about the named indexes (content_tag_taxonom_70d60b_idx, content_tag_taxonom_b04dd1_idx, oel_tagging_taxonom_5e948c_idx, etc.) when rolling back migrations ("Cannot drop index 'whatever_idx': needed in a foreign key constraint"). I've seen this before these migrations were squashed, and I don't know why it happens. To get around it, I comment out the AddIndex and AlterUniqueTogether lines in the squashed migrations before rolling back.

('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.create(taxonomy_id=-1, org=None)
bradenmacdonald marked this conversation as resolved.
Show resolved Hide resolved


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.create(taxonomy_id=-1, org=None)
bradenmacdonald marked this conversation as resolved.
Show resolved Hide resolved


@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.1

# 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.1
# 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.1
# 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.1
# 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.1
# via
# -c requirements/edx/../constraints.txt
# -r requirements/edx/base.txt
Expand Down
Loading