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

bradenmacdonald · 2023-10-06T18:18:31Z

Description

This is a minor update of the content tagging app to simplify it in correspondence with openedx/openedx-learning#91

It removes the required field which we don't really have a use for, and it makes the language taxonomy to be managed by a migration with auto-created tags, rather than imported one time via a fixture.

Testing instructions

For now there is little ability to manually test, so just make sure the tests are passing.

Deadline

None

Other information

Private-ref: FAL-3477

openedx-webhooks · 2023-10-06T18:18:37Z

Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

rpenido · 2023-10-09T18:50:37Z

Hi @bradenmacdonald!
We need to update openedx-learning in edx-platform to finish the PRs below:

Let me know if you need a hand to make the test pass to unblock this PR!
CC @pomegranited

bradenmacdonald · 2023-10-09T22:04:53Z

@rpenido Yes, I know but I'm having some trouble with the tests actually. It seems that the LanguageTaxonomy created by a data migration is not available during the test run, and I can't figure out why.

bradenmacdonald · 2023-10-09T23:04:52Z

@rpenido OK, I figured out the problem actually. Should have this PR unblocked soon.

bradenmacdonald · 2023-10-09T23:27:08Z

openedx/core/djangoapps/content_tagging/migrations/0001_squashed.py

+        ('content_tagging', '0003_system_defined_fixture'),
+        ('content_tagging', '0004_system_defined_org'),
+        ('content_tagging', '0005_auto_20230830_1517'),
+        ('content_tagging', '0006_simplify_models'),


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.

rpenido · 2023-10-09T23:45:14Z

@rpenido OK, I figured out the problem actually. Should have this PR unblocked soon.

I didn't want to put more pressure than you already had! Happy that you found the issue and also squashed the migrations!

Let us know if you need something!

bradenmacdonald · 2023-10-10T00:01:28Z

@pomegranited @Agrendalath Could you please review this PR? I'd like to get it merged tomorrow so it can make the Quince cutoff and since it's blocking other work; sorry for the short notice. I can ask other reviewers if your plate is full.

pomegranited

I've left 3 recommended changes and one question, but this is good to go once they've been applied.

👍

I tested this:
- Started with the migrations in the master branch (content_tagging.0006_simplify_models, oel_tagging.0010_cleanups)
- Switched to this branch and applied my suggested change (see inline comments).
- Installed the updated openedx-learning==0.2.1
- Applied the migrations and checked the results.
- Rolled back the new data migration: ./manage.py lms migrate content_tagging 0001_squashed
- Re-applied all migrations, and checked the results again.
I read through the code
~~I checked for accessibility issues~~ N/A
Includes documentation -- thank you for the explanatory docstrings!
Commit structure follows OEP-0051

pomegranited · 2023-10-10T04:02:46Z

openedx/core/djangoapps/content_tagging/migrations/0003_system_defined_fixture.py

-    # 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


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.

openedx/core/djangoapps/content_tagging/migrations/0007_system_defined_org_2.py

openedx/core/djangoapps/content_tagging/tests/test_tasks.py

pomegranited · 2023-10-10T06:42:22Z

openedx/core/djangoapps/content_tagging/migrations/0001_squashed.py

+    initial = True
+
+    dependencies = [
+        ("oel_tagging", "0012_language_taxonomy"),


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:

Zero-ed the migrations oel_tagging and content_tagging, and ensured all the tables for these apps were dropped.

Checked out edx-platform master (b353019), and installed openedx-learning v0.2.0

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

Checked out this branch and installed the branch from Squash Tagging Migrations [FC-0030] openedx-learning#95.

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'.

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

Tested rollback, which worked OK¹
$ ./manage.py lms migrate content_tagging zero $ ./manage.py lms migrate oel_tagging zero

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

¹ 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.

Co-authored-by: Jillian <[email protected]>

bradenmacdonald · 2023-10-10T07:03:22Z

@pomegranited Thank you for the quick and excellent review! I tried to incorporate your changes :)

pomegranited · 2023-10-10T07:05:29Z

Yep, your changes look great, thank you @bradenmacdonald !

Agrendalath

👍

I tested this: checked the changes, verified that the CI is passing
I read through the code
I checked for accessibility issues: n/a
Includes documentation: n/a
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

openedx-webhooks · 2023-10-10T17:44:14Z

@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2023-10-10T19:36:25Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-10-10T19:58:07Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2023-10-10T19:58:07Z

2U Release Notice: This PR has been deployed to the edX production environment.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 6, 2023

bradenmacdonald mentioned this pull request Oct 6, 2023

Remove Taxonomy.required, make allow_multiple True by default [FC-0030] openedx/openedx-learning#91

Merged

pomegranited mentioned this pull request Oct 9, 2023

Taxonomy import templates [FC-0036] open-craft/edx-platform#593

Closed

rpenido mentioned this pull request Oct 9, 2023

fix: update taxonomies permission rules #33413

Merged

1 task

bradenmacdonald force-pushed the braden/remove-required branch from e14fae3 to fbfa001 Compare October 9, 2023 23:08

bradenmacdonald added 2 commits October 9, 2023 16:20

feat: remove Taxonomy.required, make allow_multiple True by default

1ec1985

chore: squash migrations

ece987a

bradenmacdonald force-pushed the braden/remove-required branch from c02da5e to ece987a Compare October 9, 2023 23:20

bradenmacdonald commented Oct 9, 2023

View reviewed changes

bradenmacdonald mentioned this pull request Oct 10, 2023

Squash Tagging Migrations [FC-0030] openedx/openedx-learning#95

Merged

pomegranited approved these changes Oct 10, 2023

View reviewed changes

pomegranited reviewed Oct 10, 2023

View reviewed changes

bradenmacdonald and others added 2 commits October 10, 2023 00:01

fix: more robust migration

95a4e60

Co-authored-by: Jillian <[email protected]>

chore: version bump, use squashed migration

8215a5e

bradenmacdonald force-pushed the braden/remove-required branch from d4924cd to 8215a5e Compare October 10, 2023 07:02

Agrendalath approved these changes Oct 10, 2023

View reviewed changes

bradenmacdonald changed the title ~~feat: remove Taxonomy.required, make allow_multiple True by default [FC-0030]~~ Continued minor simplification of Taxonomy model, squashed migrations [FC-0030] Oct 10, 2023

bradenmacdonald changed the title ~~Continued minor simplification of Taxonomy model, squashed migrations [FC-0030]~~ Continued simplification of Taxonomy model, squashed migrations [FC-0030] Oct 10, 2023

bradenmacdonald merged commit d6e21a1 into openedx:master Oct 10, 2023
63 checks passed

bradenmacdonald mentioned this pull request Oct 11, 2023

[Quince backport] Squash Tagging/Taxonomy migrations, bump openedx-learning to 0.2.3 [FC-0030] #33476

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bradenmacdonald commented Oct 6, 2023

openedx-webhooks commented Oct 6, 2023

rpenido commented Oct 9, 2023

bradenmacdonald commented Oct 9, 2023

bradenmacdonald commented Oct 9, 2023

bradenmacdonald Oct 9, 2023

rpenido commented Oct 9, 2023

bradenmacdonald commented Oct 10, 2023

pomegranited left a comment •

edited

Loading

pomegranited Oct 10, 2023

pomegranited Oct 10, 2023 •

edited

Loading

bradenmacdonald commented Oct 10, 2023

pomegranited commented Oct 10, 2023

Agrendalath left a comment

openedx-webhooks commented Oct 10, 2023

edx-pipeline-bot commented Oct 10, 2023

edx-pipeline-bot commented Oct 10, 2023

edx-pipeline-bot commented Oct 10, 2023

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

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

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

Conversation

bradenmacdonald commented Oct 6, 2023

Description

Testing instructions

Deadline

Other information

openedx-webhooks commented Oct 6, 2023

rpenido commented Oct 9, 2023

bradenmacdonald commented Oct 9, 2023

bradenmacdonald commented Oct 9, 2023

bradenmacdonald Oct 9, 2023

Choose a reason for hiding this comment

rpenido commented Oct 9, 2023

bradenmacdonald commented Oct 10, 2023

pomegranited left a comment • edited Loading

Choose a reason for hiding this comment

pomegranited Oct 10, 2023

Choose a reason for hiding this comment

pomegranited Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

bradenmacdonald commented Oct 10, 2023

pomegranited commented Oct 10, 2023

Agrendalath left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Oct 10, 2023

edx-pipeline-bot commented Oct 10, 2023

edx-pipeline-bot commented Oct 10, 2023

edx-pipeline-bot commented Oct 10, 2023

pomegranited left a comment •

edited

Loading

pomegranited Oct 10, 2023 •

edited

Loading