Documents Tagging architecture decisions by pomegranited · Pull Request #55 · openedx/openedx-core

pomegranited · 2023-06-01T07:16:57Z

This PR documents the decisions that arose from openedx/modular-learning#60.

It also builds on #54 and enforces ADR 7. Tagging app structure by moving the tagging app to a separate package outside of openedx_learning, called openedx_tagging. The models in this app will be altered and aligned with the other ADRs as part of subsequent PRs.

Testing instructions

When running the automated tests, check that the openedx_tagging test runs.
Check the added ADRs for clarity and correctness.

Supporting information

Closes openedx/modular-learning#60

to openedx_tagging.core.tagging as per ADR 0007.

openedx-webhooks · 2023-06-01T07:17:02Z

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.

ChrisChV · 2023-06-01T17:34:09Z

@pomegranited Great work! 👍

I think we need also to specify how will the link with the contents be handled. I have seen that ContenTag is mentioned in the ADRs, but I feel that it has to be specified more. Specify that we have to create subclasses of ObjectTag to link the content, like ContentTag or the class to link the learning core content.

bradenmacdonald · 2023-06-01T18:17:55Z

+
+Permission #1 requires no external access, so can be enforced by the ``openedx_tagging`` app.
+
+But because permissions #2 + #3 require access to the edx-platform CMS model `CourseCreator`_, this access can only be enforced in Studio, and so will live under `cms.djangoapps.tagging` along with the ``ContentTag`` class. Tagging MVP must work for libraries v1, v2 and courses created in Studio, and so tying these permissions to Studio is reasonable for the MVP.


Per OEP-9, these three rules that you listed above can be defined using rules... with #1 defined in openedx_tagging and #2 and #3 defined in Studio. I think that what that gets us is that any application can then use the standard Django API to query things like user.has_perm('openedx_tagging.edit_taxonomy', taxonomy) and get the right result without needing to be aware of how those permissions are defined or even where they are defined (in Studio).

That would be really cool @bradenmacdonald , but can django-rules handle chaining multiple rule definitions together?

Ref rules.RuleSet:

add_rule raises a KeyError if another rule by that name already exists.

set_rule sets the rule, regardless of whether one already exists

But I don't see how to combine them?

@pomegranited Isn't combining rules what you mean? It supports combining rules with "and", "or", "not", etc.

@bradenmacdonald Yes, we can combine rules logically and explicitly -- what I meant was, there's no magic provided by django or django-rules to chain these calls to set_rule.

It's totally doable, it just takes a little care. For example:

openedx_tagging calls set_rule('openedx_tagging.edit_taxonomy', can_edit_taxonomy) where can_edit_taxonomy checks e.g user.is_staff | user.is_superuser. This is fine, openedx_tagging can stand alone.

openedx.features.tagging.SystemTaxonomy might want to call set_rule('openedx_tagging.edit_taxonomy', can_edit_taxonomy) too, where its can_edit_taxonomy disallows editing for all SystemTaxonomy no matter who you are.

openedx.features.tagging.OrgTaxonomy might want to call set_rule('openedx_tagging.edit_taxonomy', can_edit_taxonomy) too, where its can_edit_taxonomy checks openedx_tagging.can_edit_taxonomy and the user's rights against the OrgTaxonomy.org.

But they can't all pile on and call set_rule, because the most recently called set_rule wins.

So to make sure user.has_perm('openedx_tagging.edit_taxonomy', taxonomy) returns the right thing, the openedx.features.tagging rules must be aware of its different Taxonomy subclasses, and explicitly combine with the openedx_tagging rules where needed.

@pomegranited Ah, I see. Yeah it's too bad there's not a nice API for combining those. maybe we can add a lightweight way to do that?

@bradenmacdonald Sure, will see.. I'll start by trying to use consistent convention, and if it ends up being abstractable into something we can reuse, that would be awesome :) Thanks for pointing me to OEP-9 here, and clarifying the end goal of these permission rules.

bradenmacdonald · 2023-06-01T18:22:01Z

+Instance
+~~~~~~~~
+
+A Taxonomy can be enabled/disabled for all contexts using its ``enabled`` flag.


I think this needs to be clarified. It sounds like this is saying "the taxonomy can be enabled for all contexts (forced on for all courses, regardless of other settings) when enabled=True or disabled for all contexts (forced off for all courses, regardless of other settings) when enabled=False". We either need a third state (can be selectively enabled/disabled at other levels when this is set to None), or make this into either a "global disable" or "global enable" boolean, but not both. I hope that makes sense.

Yep, that makes sense @bradenmacdonald . Does this look clearer? https://github.com/open-craft/openedx-learning/blob/jill/tagging-data-arch-adr/docs/decisions/0010-taxonomy-enable-context.rst

bradenmacdonald · 2023-06-01T18:26:01Z

+    """
+
+    # Content id to which this tag is associated
+    content_id = models.CharField(max_length=255)


I think some namespacing spec is required here.

What if I want to tag a user, which has a numeric ID? Should we specify the format for this field? Perhaps that it's always an "Opaque Key" serialized as a string, or that if it's not an opaque key, it's a similarly prefixed string like "user:15" ?

Note that this model is going to change substantially when we start implementation -- I just included it here because #54 adds it, and I needed to move it under openedx_tagging.

But re your comment: I'd rather add a short string content_type field than prefix the content_id -- or in terms of the ObjectTag class that will ultimately be the base class for ContentTag, I'd rather use object_type and object_id fields.

@bradenmacdonald Thoughts on ^?

Yep, that sounds good! As long as it's namespaced in some way.

also fixes up some link typos in ADR 10

pomegranited · 2023-06-07T06:36:54Z

Thank you for your feedback @ChrisChV @bradenmacdonald -- I've addressed what I can from your comments, and from watching the meeting on 6 June. @ormsbee did you have any comments or changes needed here?

I guess I'm also not sure how much of the data model document needs to be enshrined in ADRs, and how much can be done when we do the implementation.. Is there anything missing that should be in here?

ormsbee

Minor question about taxonomy enablement. It doesn't need to block merging.

ormsbee · 2023-06-08T17:53:04Z

+-------------+-----------------------------+--------------------------+-------------------------------+
+| True        | False                       | True                     | False                         |
+-------------+-----------------------------+--------------------------+-------------------------------+
+| True        | True                        | False                    | False                         |


If the course hasn't enabled all taxonomies, but it has enabled this specific one, shouldn't it be shown for the course?

I think this doesn't allow enabling taxonomies at the course level (yet)... I misunderstood this at first too, but I think tax.enabled_for(course.org) means tax.enabled_for(org) (where org comes from the course), not tax.enabled_for(course | org) as I first read this.

bradenmacdonald · 2023-06-08T23:22:16Z

Thanks. I like how this is shaping up!

pomegranited · 2023-06-12T01:49:46Z

@bradenmacdonald @ormsbee Thanks for your comments! I'm pretty pleased with it too :)

If you're happy with what's here, could one of you approve and squash+merge?
Or if there's more that needs to be in here, please let me know? I'm starting implementation this week.

pomegranited · 2023-06-13T00:38:04Z

Thanks for your 👍 @bradenmacdonald :) I don't have merge rights here, so could you or @ormsbee merge if it's ready? Thank you!

openedx-webhooks · 2023-06-13T18:55:54Z

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

ChrisChV and others added 4 commits May 28, 2023 17:04

feat: Tagging app created

fec6472

docs: Nit on tagging app readme

c3446a5

docs: adds ADRs arising from the taxonomy data architecture decisions

c5fae57

refactor: moves openedx_learning.contrib.tagging

2182b55

to openedx_tagging.core.tagging as per ADR 0007.

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

pomegranited force-pushed the jill/tagging-data-arch-adr branch from f7837f5 to 2182b55 Compare June 1, 2023 07:19

bradenmacdonald reviewed Jun 1, 2023

View reviewed changes

docs: clarifies when taxonomies are enabled in different contexts

9f38046

pomegranited force-pushed the jill/tagging-data-arch-adr branch from 184962d to 9f38046 Compare June 7, 2023 01:05

pomegranited added 2 commits June 7, 2023 13:01

docs: ref OEP-9 for permissions

f91eca0

docs: adds ObjectTag, and updates model inheritance plan

f51515d

also fixes up some link typos in ADR 10

docs: updated to reflect "multiple orgs per taxonomy" decision on doc

152b8fc

ormsbee reviewed Jun 8, 2023

View reviewed changes

docs: fixed nit

9aa6e98

ChrisChV mentioned this pull request Jun 9, 2023

feat: Tagging Django App setup #54

Closed

bradenmacdonald approved these changes Jun 12, 2023

View reviewed changes

ChrisChV mentioned this pull request Jun 13, 2023

System-defined taxonomies architecture decisions #56

Merged

pomegranited mentioned this pull request Jun 13, 2023

Adds Taxonomy, Tag, ObjectTag models and APIs #57

Merged

3 tasks

ormsbee merged commit f1b8579 into openedx:main Jun 13, 2023

pomegranited deleted the jill/tagging-data-arch-adr branch June 14, 2023 00:11

ormsbee mentioned this pull request Mar 24, 2025

add vscode pytest settings open-craft/openedx-learning#19

Closed

ormsbee mentioned this pull request Sep 4, 2025

version bump 0.28.0 open-craft/openedx-learning#20

Closed


		Permission #1 requires no external access, so can be enforced by the ``openedx_tagging`` app.

		But because permissions #2 + #3 require access to the edx-platform CMS model `CourseCreator`_, this access can only be enforced in Studio, and so will live under `cms.djangoapps.tagging` along with the ``ContentTag`` class. Tagging MVP must work for libraries v1, v2 and courses created in Studio, and so tying these permissions to Studio is reasonable for the MVP.

Uh oh!

Conversation

pomegranited commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Jun 1, 2023

Uh oh!

ChrisChV commented Jun 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomegranited Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomegranited commented Jun 7, 2023

Uh oh!

ormsbee left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bradenmacdonald commented Jun 8, 2023

Uh oh!

pomegranited commented Jun 12, 2023

Uh oh!

pomegranited commented Jun 13, 2023

Uh oh!

openedx-webhooks commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pomegranited commented Jun 1, 2023 •

edited

Loading

pomegranited Jun 7, 2023 •

edited

Loading

ormsbee left a comment •

edited

Loading