refactor: remove ContentObjectTag model and related functions by rpenido · Pull Request #34146 · openedx/openedx-platform

rpenido · 2024-01-30T21:51:38Z

Description

This PR removes the ContentObjectTag model and some overridden tagging functions from oel_tagging.

Testing instruction

Please ensure that the tests cover the expected behavior of the refactor.

Private-ref: FAL-3610

openedx-webhooks · 2024-01-30T21:51:43Z

Thanks for the pull request, @rpenido! 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 · 2024-01-31T01:19:10Z

Hi, @pomegranited!

I removed a lot of code. Let me know what you think.

Also, we will need to override the view here to use this new tag_object implementation. Today, we are not checking if a user uses a Taxonomy from one org on objects from another.

Implementing this using the rule override would be easier, but I think it's not the best approach: it is more a business rule than a permission one (and also, superusers skip permission checks).

pomegranited · 2024-01-31T05:29:06Z

@rpenido

Also, we will need to override the view here to use this new tag_object implementation. Today, we are not checking if a user uses a Taxonomy from one org on objects from another.

Rather than overriding the view, we can change the rules to ensure that this gets checked.

oel_tagging's ObjectTagView checks oel_tagging.can_tag_object with the taxonomy + object_id.

We can override that rule here to run the base oel_tagging.can_tag_object checks, then verify that the taxonomy and object_id orgs are compatible.

pomegranited

@rpenido Yep, this is looking great. :) I've left a couple inline comments, but haven't done a full review yet, so let me know when it's ready?

openedx/core/djangoapps/content_tagging/api.py

pomegranited · 2024-01-31T05:18:29Z

openedx/core/djangoapps/content_tagging/api.py

+            object_key = UsageKey.from_string(object_id)
+        except InvalidKeyError:
+            try:
+                object_key = CourseKey.from_string(object_id)


Should use LearningContextKey (not CourseKey) and UsageKey like the previous code -- this supports content library keys too.

The LearningContextKey doesn't have the org property. It is defined in the CourseKey class:
https://github.com/openedx/opaque-keys/blob/dd532d862a51457a8f0c0f5c936fd57ee4d39fbc/opaque_keys/edx/keys.py#L43-L49

Also, the LibraryLocator inherits from CourseKey:
https://github.com/openedx/opaque-keys/blob/dd532d862a51457a8f0c0f5c936fd57ee4d39fbc/opaque_keys/edx/locator.py#L412

We are using LearningContextKey in many places, but I think we should only use CourseKey | UsageKey (because we rely on the org info to tag content). Do you think this makes sense?

PS: I didn't change it outside of the functions of this refactor.

I think we should only use CourseKey | UsageKey (because we rely on the org info to tag content)

Ah ok, agreed. I didn't realise LibraryLocator inherited from CourseKey, that's cool.

Do you think we should create a task to fix this through the code?

@rpenido I think this is too minor to warrant a task.. but if you're working on affected code, feel free to change it as you go, and cite this discussion.

rpenido · 2024-01-31T20:39:18Z

Rather than overriding the view, we can change the rules to ensure that this gets checked.

I chose that route. The only issue is that the superuser will skip that rule, but besides that, the code is much cleaner.

We were not checking this before, so the query count for tagging count went up a bit. We check TaxonomyOrgs one more time and I think we didn't have a select_related when we get a Taxonomy model.

Let me know what you think @pomegranited!

rpenido · 2024-01-31T20:52:55Z

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

-
-    def test_cannot_tag_across_orgs(self):
-        """
-        Ensure that I cannot apply tags from a taxonomy that's linked to another
-        org.
-        """
-        # This taxonomy is only linked to the "OpenedX org", so it can't be used for "Axim" content.
-        taxonomy = self.taxonomy_one_org
-        tags = [self.tag_one_org.value]
-        with self.assertRaises(ValueError) as exc:
-            api.tag_content_object(
-                object_key=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"),
-                taxonomy=taxonomy,
-                tags=tags,
-            )
-        assert "The specified Taxonomy is not enabled for the content object's org (Ax)" in str(exc.exception)
-        # But this will work fine:
-        api.tag_content_object(
-            object_key=CourseKey.from_string("course-v1:OeX+DemoX+Demo_Course"),
-            taxonomy=taxonomy,
-            tags=tags,
-        )
-        # As will this:
-        api.tag_content_object(
-            object_key=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"),
-            taxonomy=self.taxonomy_both_orgs,
-            tags=[self.tag_both_orgs.value],
-        )


This test went to the view. We don't check cross-tagging at the API level.

rpenido · 2024-01-31T20:54:33Z

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

        object_tag = getattr(self, tag_attr)
        assert self.superuser.has_perm(perm, object_tag)
-        assert self.staff.has_perm(perm, object_tag)
+        assert not self.staff.has_perm(perm, object_tag)


This is a side effect of the change. When we didn't check cross-tagging, a staff user could change objecttags from taxonomies without orgs.

I don't understand why this test changed?

Staff users are "Taxonomy Administrators", just like superusers are, so they should be able to tag objects using no-org taxonomies too.

The issue is that it is not trivial to differentiate a no-org taxonomy and a taxonomy from another org.

I fixed here: 1a4c858

But now taxonomy admins can also use a Taxonomy from orgA to tag an object from orgB (like superuser).

rpenido · 2024-01-31T21:02:06Z

This is ready for a final review @pomegranited!

pomegranited

I've requested a small change, and I have concerns about the "staff user" permissions change noted in the tests.

But other than that, this is working beautifully, so we're close! I tested this in my devstack, and was able to:

View/add/remove object tags on units (using openedx/frontend-app-authoring#815)
View/add/remove object tags on library blocks (using openedx-unsupported/frontend-app-library-authoring#400)

pomegranited · 2024-02-01T03:34:09Z

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

        object_tag = getattr(self, tag_attr)
        assert self.superuser.has_perm(perm, object_tag)
-        assert self.staff.has_perm(perm, object_tag)
+        assert not self.staff.has_perm(perm, object_tag)


I don't understand why this test changed?

Staff users are "Taxonomy Administrators", just like superusers are, so they should be able to tag objects using no-org taxonomies too.

openedx/core/djangoapps/content_tagging/rules.py

pomegranited

One nit, and a comment about the permissions change, but this looks good to merge once the nit is addressed 👍

I tested this on my devstack:
- Installed this branch on lms/cms and ran migrations
- Set up sample data using https://github.com/open-craft/taxonomy-sample-data/
- Ran openedx/frontend-app-authoring#815 to test tagging units: view, add, remove tags
- Ran openedx-unsupported/frontend-app-library-authoring#400 to test tagging libraries: view, add, remove tags
I read through the code
~~I checked for accessibility issues~~ N/A
~~Includes documentation~~ N/A
~~User-facing strings are extracted for translation~~ N/A

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

pomegranited · 2024-02-02T00:19:11Z

openedx/core/djangoapps/content_tagging/rules.py

+    # Taxonomy admins can tag any object using any taxonomy
+    if oel_tagging.is_taxonomy_admin(user):
+        return True


We can avoid the "Taxonomy admins can tag any object using any taxonomy" issue like this, however: this change bumps the query count for updating object tags up from 7 to 13!

Suggested change

# Taxonomy admins can tag any object using any taxonomy

if oel_tagging.is_taxonomy_admin(user):

return True

# Only Taxonomy admins can tag objects using a no-org taxonomy

if not perm_obj.taxonomy.taxonomyorg_set.exists():

return oel_tagging.is_taxonomy_admin(user)

So I can't actually recommend this change. But, I have a task next sprint to fix these query counts, so I'll handle making this change if needed.

Besides the query count, I wonder if it is worth making this differentiation: if adminA adds this taxonomy to an org, does this change the permission from adminB?

I'm uncomfortable with this, but I think treating no-org and cross-org the same way (allowing or blocking) would be the best path here.

I commented on the issue you created about the query count.
The staff account is probably the best-case scenario. But I'm sure that you will find what we got wrong.

rpenido · 2024-02-02T12:33:45Z

Hi @bradenmacdonald! Could you do a CC review here?

bradenmacdonald

Do we have tests that you can't tag things that aren't xblocks/courses/libraries?
If so, I think I'm good with this change. Though I did like the API where you could pass UsageKeys directly instead of having to convert them to strings all the time.

bradenmacdonald · 2024-02-08T02:57:55Z

openedx/core/djangoapps/content_tagging/migrations/0008_remove_content_object_tag.py

+
+    operations = [
+        migrations.DeleteModel(
+            name='ContentObjectTag',


There is no data stored with this model, right? This won't delete any actual database rows?

Right. Also, we weren't using it outside auto-tagging: our rest API uses the ObjectTag model (this is the main issue that motivated this PR)

bradenmacdonald · 2024-02-08T03:01:55Z

openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py

        assert response.status_code == status.HTTP_400_BAD_REQUEST

+    @ddt.data(
+        # userA and userS are staff in libraryA and can tag using enabled taxonomies


Who are userA and userS ? I see them mentioned a few times in this file, but I think those are typos or from an older version. Here I only see user, staff, and staffA.

Done: 78b3d53

openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py

bradenmacdonald · 2024-02-08T03:09:39Z

openedx/core/djangoapps/content_tagging/types.py

+from opaque_keys.edx.locator import LibraryLocatorV2

-ContentKey = Union[LearningContextKey, UsageKey]
+ContentKey = Union[LibraryLocatorV2, CourseKey, UsageKey]


What's wrong with leaving [LearningContextKey, UsageKey] ? Less specific?

We need an Org to validate the permissions.
The org property is defined in the CourseKey and the LibraryLocatorV2 for our cases. For the UsageKey, we get the org from the context_key (and we also check if the contet_key is a CourseKey or LibraryLocatorV2).

We could use LearningContext and duck typing to check if we have the org field (directly in the key or in its contex_key).

I personally prefer strict typing to make sure the use conforms with what we are implementing and testing here.

rpenido · 2024-02-08T17:32:42Z

Do we have tests that you can't tag things that aren't xblocks/courses/libraries?

I added a generic test here: 2791963

Are there some specific string keys that we can also test?
We also can't tag an object that has a UsageKey that the context_key property is not a LibraryLocatorV2 nor a CourseKey`. Is there a real key that fits this case to add a test?

Co-authored-by: Braden MacDonald <mail@bradenm.com>

openedx-webhooks · 2024-02-08T19:55:41Z

@rpenido 🎉 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 · 2024-02-08T21:07:34Z

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

edx-pipeline-bot · 2024-02-08T21:27:39Z

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

edx-pipeline-bot · 2024-02-08T21:27:39Z

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 Jan 30, 2024

rpenido force-pushed the rpenido/fal-3610-content-tag-cleanup branch 2 times, most recently from 17db766 to 944a650 Compare January 30, 2024 23:00

refactor: remove ContentObjectTag model and related functions

d72ae83

rpenido force-pushed the rpenido/fal-3610-content-tag-cleanup branch from 944a650 to d72ae83 Compare January 31, 2024 00:41

rpenido mentioned this pull request Jan 31, 2024

feat: export tagged course as csv #34091

Merged

pomegranited requested changes Jan 31, 2024

View reviewed changes

rpenido force-pushed the rpenido/fal-3610-content-tag-cleanup branch 5 times, most recently from 455105d to 252d402 Compare January 31, 2024 19:46

refactor: remove tag_object function and add cross-org check to rules

739fdb5

rpenido force-pushed the rpenido/fal-3610-content-tag-cleanup branch from 252d402 to 739fdb5 Compare January 31, 2024 20:04

rpenido commented Jan 31, 2024

View reviewed changes

rpenido marked this pull request as ready for review January 31, 2024 21:00

Merge branch 'master' into rpenido/fal-3610-content-tag-cleanup

82e9923

pomegranited requested changes Feb 1, 2024

View reviewed changes

rpenido force-pushed the rpenido/fal-3610-content-tag-cleanup branch from 2c7e806 to 2c6986e Compare February 1, 2024 17:54

fix: allow taxonomy admins use no-org taxonomies

1a4c858

rpenido force-pushed the rpenido/fal-3610-content-tag-cleanup branch from 2c6986e to 1a4c858 Compare February 1, 2024 17:56

fix: skip check if taxonomy not provided

51be568

pomegranited approved these changes Feb 2, 2024

View reviewed changes

pomegranited mentioned this pull request Feb 2, 2024

[Tagging] Reduce the number of queries used in content tagging REST API openedx/modular-learning#185

Closed

refactor: add utils module to handle opaque keys

dfb908e

rpenido force-pushed the rpenido/fal-3610-content-tag-cleanup branch from 62acf86 to dfb908e Compare February 2, 2024 17:34

rpenido added 4 commits February 6, 2024 17:58

test: add tests for tagging LibraryV2

0278ad8

fix: types

38e03ce

docs: add comments

9975a78

fix: pylint

cc6e2fd

rpenido force-pushed the rpenido/fal-3610-content-tag-cleanup branch from e2439ac to cc6e2fd Compare February 6, 2024 21:48

pomegranited requested a review from bradenmacdonald February 8, 2024 01:29

bradenmacdonald reviewed Feb 8, 2024

View reviewed changes

fix: add test to invalid object id tagging

2791963

rpenido and others added 2 commits February 8, 2024 14:38

docs: fix typo

73970f3

Co-authored-by: Braden MacDonald <mail@bradenm.com>

docs: fix test comments

78b3d53

bradenmacdonald approved these changes Feb 8, 2024

View reviewed changes

bradenmacdonald merged commit c2d91c3 into openedx:master Feb 8, 2024

bradenmacdonald deleted the rpenido/fal-3610-content-tag-cleanup branch February 8, 2024 19:55

pomegranited mentioned this pull request Feb 14, 2024

fix: prevent cross-org ObjectTags from being created open-craft/openedx-platform#633

Merged

Conversation

rpenido commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instruction

Uh oh!

openedx-webhooks commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rpenido commented Jan 31, 2024

Uh oh!

pomegranited commented Jan 31, 2024

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpenido Jan 31, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

rpenido commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

rpenido Feb 1, 2024 • 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!

rpenido commented Jan 31, 2024

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pomegranited left a comment

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!

rpenido commented Feb 2, 2024

Uh oh!

bradenmacdonald left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpenido commented Feb 8, 2024

Uh oh!

openedx-webhooks commented Feb 8, 2024

Uh oh!

rpenido commented Jan 30, 2024 •

edited

Loading

openedx-webhooks commented Jan 30, 2024 •

edited

Loading

rpenido Jan 31, 2024 •

edited

Loading

rpenido commented Jan 31, 2024 •

edited

Loading

rpenido Feb 1, 2024 •

edited

Loading