Skip to content

System defined taxonomies - #32869

Merged
bradenmacdonald merged 14 commits into
openedx:masterfrom
open-craft:chris/smart-system-defined-taxonomies
Aug 2, 2023
Merged

System defined taxonomies#32869
bradenmacdonald merged 14 commits into
openedx:masterfrom
open-craft:chris/smart-system-defined-taxonomies

Conversation

@ChrisChV

@ChrisChV ChrisChV commented Jul 27, 2023

Copy link
Copy Markdown
Contributor

Description

Add the object tags for the system defined taxonomies that will be used in the platform. In addition, a migration has been created that loads the fixtures that create the System defined taxonomies

Testing instructions

  • Get the master openedx/devstack up and running.
  • Pull this branch into edx-platform
  • Install dependencies and update migrations by running paver install_prereqs in the LMS shell (from devstack dir, run make lms-shell) and CMS shell (make studio-shell).
  • Run the new migrations to create the System defined Taxonomies by running ./manage.py lms migrate in the LMS shell.
  • Open a shell with ./manage.py lms shell
  • Check the new created System-defined taxonomies:
>>> from openedx_tagging.core.tagging.models import Taxonomy
>>> Taxonomy.objects.filter(id__lt=0)
<QuerySet [<ContentOrganizationTaxonomy> (-3) Content Authors, <ContentAuthorTaxonomy> (-2) Organizations, <ContentLanguageTaxonomy> (-1) Languages]>
>>> lang = Taxonomy.objects.get(id=-1)
>>> lang
<ContentLanguageTaxonomy> (-1) Languages
>>> lang.tag_set.all()
  • Revert the migration with ./manage.py lms migrate content_tagging 0001
  • Check that System-defined taxonomies are deleted:
>>> from openedx_tagging.core.tagging.models import Taxonomy, Tag
>>> Taxonomy.objects.filter(id__lt=0)
<QuerySet []>
>>> Tag.objects.all()
<QuerySet []>

Deadline

None

Other information

Relates to openedx/modular-learning#77

Depends on openedx/openedx-core#67

Data migrations added here can be easily rolled back.

@ChrisChV
ChrisChV force-pushed the chris/smart-system-defined-taxonomies branch 2 times, most recently from 27f00b4 to c88660f Compare July 27, 2023 19:03
Comment thread requirements/edx/kernel.in Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TODO: Update this when openedx/openedx-core#67 is deployed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once openedx/openedx-core#67 (comment) is done, could you update this to be:

openedx-learning<0.2

We'll do all our MVP changes under 0.1.x, and so this will mean they're picked up in the platform automatically.

Is that ok @bradenmacdonald , or do we need to bump the minor release every time too?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pomegranited Good idea, let's do that. Though on the platform side, I think you're not supposed to put any version constraints in this file at all, but rather in constraints.txt. So we can probably just leave it un-pinned for now? And keep in mind the platform dependencies only get updated once a week or so, so we'll likely need to do manual version bumps sometimes when we need to pull the newer version in sooner.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bradenmacdonald Ah cool, I'm happy with unpinned.

@pomegranited pomegranited Aug 2, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ChrisChV I've merged your openedx/openedx-core#67 and tagged it as v0.1.1.

So if you unpin openedx-learning and re-compile the requirements, it should pick up the latest version? You can use this comment (which is what I should have done when I added this requirement):

openedx-learning                    # Open edX Learning core (experimental)

@ChrisChV
ChrisChV force-pushed the chris/smart-system-defined-taxonomies branch from c88660f to d3cd671 Compare July 27, 2023 19:30
Comment thread openedx/features/content_tagging/fixtures/system_defined.yaml Outdated
Comment thread openedx/features/content_tagging/models/system_defined.py Outdated
Comment thread openedx/features/content_tagging/fixtures/system_defined.yaml Outdated

@bradenmacdonald bradenmacdonald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mostly LGTM. @pomegranited if you can confirm that you've tested this, I'll approve and merge.

Comment thread openedx/features/content_tagging/models/system_defined.py Outdated
Comment thread openedx/features/content_tagging/fixtures/system_defined.yaml Outdated
Comment thread openedx/features/content_tagging/tests/test_models.py Outdated
@ChrisChV
ChrisChV force-pushed the chris/smart-system-defined-taxonomies branch from c1ae8b1 to 60c8e6f Compare August 1, 2023 19:17
@ChrisChV
ChrisChV force-pushed the chris/smart-system-defined-taxonomies branch from 60c8e6f to 51f2fc5 Compare August 1, 2023 19:43
@pomegranited

Copy link
Copy Markdown
Contributor

Mostly LGTM. @pomegranited if you can confirm that you've tested this, I'll approve and merge.

Thanks @bradenmacdonald . I've re-tested this and it's working as expected, with one change (since the language taxonomy actually lives in oel_tagging):

* Revert the migration with `./manage.py lms migrate content_tagging 0001` and `./manage.py lms migrate oel_tagging 0004`

And thank you @ChrisChV for ensuring the data migrations could be un-applied cleanly in case we need to roll back this change.

It's good to merge once @ChrisChV gets a chance to update the requirements.

@ChrisChV

ChrisChV commented Aug 2, 2023

Copy link
Copy Markdown
Contributor Author

Thanks @pomegranited 😄

It's good to merge once @ChrisChV gets a chance to #32869 (comment).

Done 👍 It's ready to merge

@bradenmacdonald
bradenmacdonald merged commit 9bea447 into openedx:master Aug 2, 2023
@bradenmacdonald
bradenmacdonald deleted the chris/smart-system-defined-taxonomies branch August 2, 2023 19:22
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants