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

feat: System defined taxonomies #551

Conversation

ChrisChV
Copy link
Member

@ChrisChV ChrisChV commented Jul 12, 2023

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 open-craft/openedx-learning#3

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 b30e79a to 5a8580f Compare July 12, 2023 22:53
Copy link
Member

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

Hi @ChrisChV , I've requested some changes here, but as with open-craft/openedx-learning#3 (review), feel free to wait until upstream have weighed in on openedx#32661 before addressing them.

@pomegranited pomegranited force-pushed the jill/add-content-tagging-smart-objecttags branch from d780cc7 to 180a29c Compare July 18, 2023 07:23
@ChrisChV ChrisChV force-pushed the chris/smart-system-defined-taxonomies branch 4 times, most recently from d3a045e to ecd8e07 Compare July 22, 2023 19:37
Copy link
Member

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

Hi @ChrisChV, this is looking so much better. I've suggested some changes with #560, and raised a similar issue with the rules and taxonomy.cast() -- but once these are addressed, this should be ready for upstream :)

allow_multiple: false
allow_free_text: false
visible_to_authors: true
_taxonomy_class: openedx.features.content_tagging.models.ContentLanguageTaxonomy
Copy link
Member

Choose a reason for hiding this comment

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

Oh bummer.. i don't see a better way to do this right now, but we had hopes that our "system-defined language taxonomy" could be reused by other tagging applications, e.g. "people tags". But I think we'll need to deal with that when we come to it.

@@ -118,7 +118,7 @@ openedx-events>=3.1.0 # Open edX Events from Hooks Extension Frame
openedx-filters # Open edX Filters from Hooks Extension Framework (OEP-50)
# FIXME Use pip package once created
# ref https://github.com/openedx/openedx-learning/pull/62
git+https://github.com/open-craft/openedx-learning.git@jill/smarter-object-tags-rebase#egg=openedx-learning==0.1
git+https://github.com/open-craft/openedx-learning.git@chris/system-defined-taxonomies#egg=openedx-learning
Copy link
Member

Choose a reason for hiding this comment

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

TODO: merge and tag openedx/openedx-learning#67, then update this dependency.

Copy link
Member

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

👍 awesome, looks ready to merge to me, @ChrisChV !

What do you think @bradenmacdonald ?

  • I tested this using the PR test instructions and ran the automated tests.
  • I read through the code and checked that the tests cover all the expected functionality.
  • I checked for accessibility issues N/A
  • Includes documentation -- code comments are great
  • Commit structure follows OEP-0051

@pomegranited pomegranited force-pushed the jill/add-content-tagging-smart-objecttags branch 2 times, most recently from 1a3c38f to 6206316 Compare July 26, 2023 05:14
AhtishamShahid and others added 17 commits July 26, 2023 14:19
* fix: use student role for zoom in case of global staff.

* fix: added request cache to avoid duplicate db calls.
This PR implements much of the static assets rework ADR [1], including:

* `npm run build[-dev]`, and its subcommands,
* `npm run webpack[-dev]` and
* `npm run compile-sass[-dev]`.

This is backwards-compatible. `paver update_assets` should not be affected.
The new command warns that it is "experimental" for a few reasons:

* `npm run build` will fail in the webpack phase unless you first
run  `xmodule_assets`. This will be changed soon [2].

* We have tested the new build, but not quite so thoroughly that we'd
recommend it as the production default yet. Once the xmodule_assets
work lands, we'll share this on the forums so early adopters can try it
out.

* The commands lack some top-level documentation. Once they stabilize more,
we'll add a section to the README that explains how and when to use `npm run
build` and its subcommands and its env vars.

* `npm run watch` is not yet implemented.

References:
1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst
2. openedx#32685

Part of: openedx#31604
* fix: allow missing authOrgId

---------

Co-authored-by: Leangseu Kim <[email protected]>
…enedx#32480)

As part of the static asset build, JS modules for most built-in XBlocks were
unnecessarily copied from the original locations (under xmodule/js and
common/static/js) to a git-ignored location (under common/static/xmodule), and
then included into the Webpack builld via
common/static/xmodule/webpack.xmodule.config.js.

With this commit, we stop copying the JS modules. Instead, we have
common/static/xmodule/webpack.xmodule.config.js just reference the original
source under xmodule/js and common/static/js.

This lets us us radically simplify the xmodule/static_content.py build script.
It also sets the stage for the next change, in which we will check
webpack.xmodule.config.js into the repository, and delete
xmodule/static_content.py entirely.

common/static/xmodule/webpack.xmodule.config.js before:

    module.exports = {
        "entry": {
            "AboutBlockDisplay": [
                "./common/static/xmodule/modules/js/000-b82f6c436159f6bc7ca2513e29e82503.js",
                "./common/static/xmodule/modules/js/001-3ed86006526f75d6c844739193a84c11.js",
                "./common/static/xmodule/modules/js/002-3918b2d4f383c04fed8227cc9f523d6e.js",
                "./common/static/xmodule/modules/js/003-b3206f2283964743c4772b9d72c67d64.js",
                "./common/static/xmodule/modules/js/004-274b8109ca3426c2a6fde9ec2c56e969.js",
                "./common/static/xmodule/modules/js/005-26caba6f71877f63a7dd4f6796109bf6.js"
            ],
            "AboutBlockEditor": [
                "./common/static/xmodule/descriptors/js/000-b82f6c436159f6bc7ca2513e29e82503.js",
                "./common/static/xmodule/descriptors/js/001-19c4723cecaa5a5a46b8566b3544e732.js"
            ],
            // etc
        }
    };

common/static/xmodule/webpack.xmodule.config.js after:

    module.exports = {
        "entry": {
            "AboutBlockDisplay": [
                "./xmodule/js/src/xmodule.js",
                "./xmodule/js/src/html/display.js",
                "./xmodule/js/src/javascript_loader.js",
                "./xmodule/js/src/collapsible.js",
                "./xmodule/js/src/html/imageModal.js",
                "./xmodule/js/common_static/js/vendor/draggabilly.js"
            ],
            "AboutBlockEditor": [
                "./xmodule/js/src/xmodule.js",
                "./xmodule/js/src/html/edit.js"
            ],
            // etc
        }
    };

Part of: openedx#32481
* refactor: moves is_content_creator

from cms.djangoapps.contentstore.helpers to common.djangoapps.student.auth

* feat: adds content tagging app

Adds models and APIs to support tagging content objects (e.g. XBlocks,
content libraries) by content authors. Content tags can be thought of as
"name:value" fields, though underneath they are a bit more complicated.

* adds dependency on openedx-learning<=0.1.0
* adds tagging app to LMS and CMS
* adds content tagging models, api, rules, admin, and tests.
* content taxonomies and tags can be maintained per organization by
  content creators for that organization.
…2686)

* fix: push docker multi-arch images

---------

Co-authored-by: Salman Nawaz <[email protected]>
* fix: include new assets.txt file in make upgrade
* test: update click version to resolve upgrade job failure
* chore: Updating Python Requirements (openedx#32861)
Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
jajjibhai008 and others added 7 commits July 27, 2023 18:56
…x-enterprise-34f9fc4

feat: Upgrade Python dependency edx-enterprise
…ts` (openedx#32685)

The Webpack configuration file for built-in XBlock JS used to be
generated at build time and git-ignored. It lived at
common/static/xmodule/webpack.xmodule.config.js. It was generated
because the JS that it referred to was also generated at build-time, and
the filenames of those JS modules were not static.

Now that its contents have been made entirely static [1], there is no
reason we need to continue generating this Webpack configuration file.
So, we check it into edx-platform under the name
./webpack.builtinblocks.config.js. We choose to put it in the repo's
root directory because the paths contained in the config file are
relative to the repo's root.

This allows us to behead both the xmodule/static_content.py
(`xmodule_assets`) script andthe  `process_xmodule_assets` paver task, a
major step in removing the need for Python in the edx-platform asset
build [2]. It also allows us to delete the `HTMLSnippet` class and all
associated attributes, which were exclusively used by
xmodule/static_content.py..

We leave `xmodule_assets` and  `process_xmodule_assets` in as stubs for
now in order to avoid breaking external code (like Tutor) which calls
Paver; the entire pavelib/assets.py function will be eventually removed
soon anyway [3]. Further, to avoid extraneous refactoring, we keep one
method of `HTMLSnippet` around on a few of its former subclasses:
`get_html`. This method was originally part of the XModule framework;
now, it is left over on a few classes as a simple internal helper
method.

References:
1. openedx#32480
2. openedx#31800
3. openedx#31895

Part of: openedx#32481
@bradenmacdonald
Copy link
Member

@ChrisChV could you please rebase this PR or merge master into it, then ping me for a look?

@ChrisChV ChrisChV force-pushed the chris/smart-system-defined-taxonomies branch from 6a4a19a to 9dd2a2a Compare July 27, 2023 17:36
@ChrisChV
Copy link
Member Author

Closed in favor of openedx#32869

@ChrisChV ChrisChV closed this Jul 27, 2023
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.