feat: send course/block tags with the CourseOverview and XBlock sinks #41

pomegranited · 2024-05-01T07:11:37Z

Tags are stored on the course_block_json/block_data_json, serialized as a list of "tag name=tag value" strings.

If the course/block has no tags, its json "tags" value in the json will be null.

Testing instructions

Setup:

Install this branch in your tutor dev/local environment, e.g

tutor config save -a OPENEDX_EXTRA_PIP_REQUIREMENTS="git+https://github.com/openedx/platform-plugin-aspects.git@jill/sync-tags-on-course-sink#egg=platform_plugin_aspects"
tutor dev launch -I

Enable tagging and use of the Course Authoring MFE by enabling these waffle flags for "everyone":

contentstore.new_studio_mfe.use_tagging_taxonomy_list_page
contentstore.new_studio_mfe.use_new_home_page
contentstore.new_studio_mfe.use_new_course_outline_page
contentstore.new_studio_mfe.use_new_unit_page

Set up some sample taxonomies, orgs, and courses.

E.g. I have mounted edx-platform, and so can do this:

cd edx-platform
git clone https://github.com/open-craft/taxonomy-sample-data
cd taxonomy-sample-data

Update generate.py to provide a valid USER_EMAIL and TAXONOMY_SAMPLE_PATH, e.g

--- a/generate.py
+++ b/generate.py
@@ -200,12 +200,12 @@ def import_tarfile_in_course(tarfile_path, course_key, user_id):

 User = get_user_model()

-USER_EMAIL = "[email protected]"
+USER_EMAIL = "[email protected]"

 user = User.objects.get(email=USER_EMAIL)

 # Set to path where repo was cloned, eg: /edx/src/taxonomy-sample-data
-TAXONOMY_SAMPLE_PATH = "/openedx/taxonomy-sample-data"
+TAXONOMY_SAMPLE_PATH = "/openedx/edx-platform/taxonomy-sample-data"

Run the script in the CMS shell:

tutor dev run cms bash
app@2e5e9f0f4f4e:~/edx-platform$ ./manage.py cms shell < taxonomy-sample-data/generate.py

Testing:

Navigate to the Course Authoring MFE and locate a tagged course, e.g.

http://apps.local.edly.io:2001/course-authoring/course/course-v1:SampleTaxonomyOrg1+STC1+2023_1
Navigate to a unit in this course, and click Manage Tags in the right-hand sidebar to Edit Tags and add tags to one or more units.
Publish this course.
Visit Superset as a superuser.

Check that the course_overviews sink contains tag data in its course_data_json, e.g.

{
   "advertised_start": null, 
   "announcement": null,
   "lowest_passing_grade": 0.5,
   "invitation_only": false,
   "max_student_enrollments_allowed": null,
   "effort": null,
   "enable_proctored_exams": false,
   "entrance_exam_enabled": false,
   "external_id": null,
   "language": "en",
   "tags": "['HierarchicalTaxonomy=hierarchical taxonomy tag 2.5.14', 'HierarchicalTaxonomy=hierarchical taxonomy tag 3.13.28', 'HierarchicalTaxonomy=hierarchical taxonomy tag 3.6.45', 'ESDC Skills and Competencies=Building and Construction', 'Lightcast Open Skills Taxonomy=Stream Processing', 'FlatTaxonomy=flat taxonomy tag 2440']",
}

Check that the course_blocks sink contains tag data in its block_data_json, e.g.

{
   "course": "STC1",
   "run": "2023_1",
   "block_type": "about",
   "detached": 1,
   "graded": 0,
   "completion_mode": "",
   "section": 2,
   "subsection": 1,
   "unit": 5,
   "tags": "['ESDC Skills and Competencies=Endurance', 'ESDC Skills and Competencies=Physical Strength Abilities']",
}

Author Notes & Concerns

~~There's a bug in the Course Authoring MFE that prevents the Manage Tags UI from saving tags for units (step 2 under Testing). Branch that fixes this: TBD.~~ fixed by Bump openedx-learning to support tagging with multiple taxonomies at once [FC-0036] edx-platform#34490.
The Manage Tags UI shows a tag count that includes explicit + implicit tags. When tags from a hierarchical taxonomy are serialized for the course/block, only the explicit "leaf" tags are stored, so the counts will differ. 6055922 ensures both implicit and explicit tags are serialized to ClickHouse.
Do we need to update the aspects migrations to add a tags field to the materialized views like we do for other block_data_json fields?

Merge checklist:
Check off if complete or not applicable:

Version bumped
Changelog record added
Documentation updated (not only docstrings)
Fixup commits are squashed away
Unit tests added/updated
Manual testing instructions provided
Noted any: Concerns, dependencies, migration issues, deadlines, tickets

Tags are stored on the course_block_json/block_data_json, serialized as a list of "tag name=tag value" strings. If the course/block has no tags, its json "tags" value will be null.

openedx-webhooks · 2024-05-01T07:11:41Z

Thanks for the pull request, @pomegranited! 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.

github-actions · 2024-05-01T07:12:48Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
platform_plugin_aspects
utils.py
platform_plugin_aspects/sinks
course_overview_sink.py
serializers.py
Project Total

_{This report was generated by python-coverage-comment-action}

bmtcril · 2024-05-01T20:25:04Z

Awesome, I hope to have time to test this tomorrow. From the examples it looks like hierarchical taxonomies don't attach each parent tag in the hierarchy, just the leaf node. Is that correct? I imagine people will want to be able to "drill up" as it were, which we wouldn't be able to do on the reporting side without having all of the taxonomies imported.

pomegranited · 2024-05-01T22:43:23Z

@bmtcril No worries -- I can ensure that the "implicit" parent tags are also exported here.

I'd also appreciate your feedback on how the tags are being stored in Clickhouse.. is that flat list of "taxonomy name=tag value" OK? It's repetitive, in that the "taxonomy name" is repeated for each tag, but I figured it would be easier to query on than some nested structure.

Unfortunately the tagging API doesn't have a single-query mechanism for returning all explicit and implicit tags for all blocks in a course, so we need to query once per block.

pomegranited · 2024-05-05T23:46:49Z

@bmtcril CC @Ian2012 This is ready for review now -- I've tested it end to end in my tutor dev stack, and it's working now.

Do we need to update the aspects migrations to add a tags field to the materialized views like we do for other block_data_json fields?

Open question :)

Ian2012 · 2024-05-06T14:28:01Z

@pomegranited There is no need to do a migration as all MVs are managed in DBT, however that shouldn't be a concern here

Ian2012 · 2024-05-06T14:38:31Z

platform_plugin_aspects/utils.py

+def _get_object_tags(usage_key):  # pragma: no cover
+    """
+    Wrap the Open edX tagging API method get_object_tags.
+    """
+    # pylint: disable=import-outside-toplevel,import-error
+    from openedx.core.djangoapps.content_tagging.api import get_object_tags
+
+    return get_object_tags(object_id=str(usage_key))


We should assume this could fail in case of changes to the location of the function or in the case it doesn't exist (older versions than Redwood)

The function get_model from the utils.py module is a good pattern to follow on those cases, and we should assume an import can always be None and handle those cases. Is the operators and developers function to configure it according to their custom developments and backports.

We can rename it to something more useful.

bmtcril · 2024-05-06T14:40:54Z

I was able to test this locally, thanks for the excellent directions! I think that having a dictionary for the output makes a little more sense. Aside from the storage I'm already worried that someone will name a tag with an "=" in it or something someday. We're already parsing out the JSON from there, a little more hopefully won't hurt.

I also wrapped the platform import in a try, since this may be used in earlier versions than Redwood. We haven't tested that in a while, however, and probably should.

bmtcril · 2024-05-06T14:44:45Z

I also agree that we don't need a migration at this point, since we're just capturing the data for v1. We can add dbt models for it when we know more about how we want to query these.

bmtcril · 2024-05-06T14:50:42Z

Screenshots of the test...

And the resulting JSON in ClickHouse:

bmtcril

This LGTM in the current state, but some of the changes are mine so I'll wait for a thumb or more feedback from @Ian2012 as I think I addressed his comments with my changes?

Ian2012 · 2024-05-06T15:10:40Z

@bmtcril Yes, you addressed my concerns here. Just one more comment (not a blocker), does it makes sense to have the nested structured saved?

Instead of

{
   "tags": {
      "x": [
        "y", "z"
       ]
   }
}

use:

{
   "tags": {
      "x": [
        { "y": {"z": null}}
       ]
   }
}

It would be way harder to query this, just wanted to make sure we are not losing any context here.

bmtcril · 2024-05-06T17:11:11Z

@Ian2012 that's a good point, but I think we probably don't need that complexity here. When it comes time to start reporting on these we'll need to make a decision about whether to import the whole set of tags to Aspects as well, and the hierarchy can come with it at that point. Otherwise I think this is sufficient to support the filtering cases we've talked about so far. 🤞

openedx-webhooks · 2024-05-06T17:13:55Z

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

feat: send course/block tags with the CourseOverview and XBlock sinks

9583bc3

Tags are stored on the course_block_json/block_data_json, serialized as a list of "tag name=tag value" strings. If the course/block has no tags, its json "tags" value will be null.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 1, 2024

pomegranited added 3 commits May 2, 2024 09:11

feat: include implicit tags in course and block json

6055922

Unfortunately the tagging API doesn't have a single-query mechanism for returning all explicit and implicit tags for all blocks in a course, so we need to query once per block.

fix: object tag "parent" tags must be accessed through their "tag"

d6e7013

Merge branch 'main' into jill/sync-tags-on-course-sink

195b0cb

pomegranited marked this pull request as ready for review May 5, 2024 23:45

pomegranited requested review from bmtcril and Ian2012 May 5, 2024 23:45

refactor: Make tag output a dict

30103e6

Ian2012 reviewed May 6, 2024

View reviewed changes

style: Linting fixes

3249924

bmtcril approved these changes May 6, 2024

View reviewed changes

Ian2012 approved these changes May 6, 2024

View reviewed changes

bmtcril merged commit 925210b into main May 6, 2024
10 checks passed

bmtcril deleted the jill/sync-tags-on-course-sink branch May 6, 2024 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: send course/block tags with the CourseOverview and XBlock sinks #41

feat: send course/block tags with the CourseOverview and XBlock sinks #41

pomegranited commented May 1, 2024 •

edited

Loading

openedx-webhooks commented May 1, 2024 •

edited

Loading

github-actions bot commented May 1, 2024 •

edited

Loading

bmtcril commented May 1, 2024

pomegranited commented May 1, 2024

pomegranited commented May 5, 2024

Ian2012 commented May 6, 2024

Ian2012 May 6, 2024

bmtcril commented May 6, 2024

bmtcril commented May 6, 2024

bmtcril commented May 6, 2024

bmtcril left a comment

Ian2012 commented May 6, 2024

bmtcril commented May 6, 2024

openedx-webhooks commented May 6, 2024

feat: send course/block tags with the CourseOverview and XBlock sinks #41

feat: send course/block tags with the CourseOverview and XBlock sinks #41

Conversation

pomegranited commented May 1, 2024 • edited Loading

openedx-webhooks commented May 1, 2024 • edited Loading

github-actions bot commented May 1, 2024 • edited Loading

Coverage report

bmtcril commented May 1, 2024

pomegranited commented May 1, 2024

pomegranited commented May 5, 2024

Ian2012 commented May 6, 2024

Ian2012 May 6, 2024

Choose a reason for hiding this comment

bmtcril commented May 6, 2024

bmtcril commented May 6, 2024

bmtcril commented May 6, 2024

bmtcril left a comment

Choose a reason for hiding this comment

Ian2012 commented May 6, 2024

bmtcril commented May 6, 2024

openedx-webhooks commented May 6, 2024

pomegranited commented May 1, 2024 •

edited

Loading

openedx-webhooks commented May 1, 2024 •

edited

Loading

github-actions bot commented May 1, 2024 •

edited

Loading