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

refactor: remove ContentObjectTag model and related functions #34146

Merged

Conversation

rpenido
Copy link
Contributor

@rpenido rpenido commented Jan 30, 2024

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 openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 30, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Jan 30, 2024

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 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
@rpenido rpenido force-pushed the rpenido/fal-3610-content-tag-cleanup branch from 944a650 to d72ae83 Compare January 31, 2024 00:41
@rpenido
Copy link
Contributor Author

rpenido commented Jan 31, 2024

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
Copy link
Contributor

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

Copy link
Contributor

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

@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 Outdated Show resolved Hide resolved
object_key = UsageKey.from_string(object_id)
except InvalidKeyError:
try:
object_key = CourseKey.from_string(object_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@rpenido rpenido Jan 31, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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 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
@rpenido rpenido force-pushed the rpenido/fal-3610-content-tag-cleanup branch from 252d402 to 739fdb5 Compare January 31, 2024 20:04
@rpenido
Copy link
Contributor Author

rpenido commented Jan 31, 2024

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!

Comment on lines -223 to -250

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],
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rpenido rpenido Feb 1, 2024

Choose a reason for hiding this comment

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

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 rpenido marked this pull request as ready for review January 31, 2024 21:00
@rpenido
Copy link
Contributor Author

rpenido commented Jan 31, 2024

This is ready for a final review @pomegranited!

Copy link
Contributor

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

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:

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

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

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

Comment on lines +289 to +291
# Taxonomy admins can tag any object using any taxonomy
if oel_tagging.is_taxonomy_admin(user):
return True
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

rpenido commented Feb 2, 2024

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

@rpenido rpenido force-pushed the rpenido/fal-3610-content-tag-cleanup branch from 62acf86 to dfb908e Compare February 2, 2024 17:34
@rpenido rpenido force-pushed the rpenido/fal-3610-content-tag-cleanup branch from e2439ac to cc6e2fd Compare February 6, 2024 21:48
Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

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.


operations = [
migrations.DeleteModel(
name='ContentObjectTag',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

@@ -1534,6 +1541,129 @@ def test_tag_xblock_invalid(self, user_attr, taxonomy_attr):
response = self.client.put(url, {"tags": ["invalid"]}, format="json")
assert response.status_code == status.HTTP_400_BAD_REQUEST

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: 78b3d53


ContentKey = Union[LearningContextKey, UsageKey]
ContentKey = Union[LibraryLocatorV2, CourseKey, UsageKey]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

rpenido commented Feb 8, 2024

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?

rpenido and others added 2 commits February 8, 2024 14:38
@bradenmacdonald bradenmacdonald merged commit c2d91c3 into openedx:master Feb 8, 2024
46 checks passed
@bradenmacdonald bradenmacdonald deleted the rpenido/fal-3610-content-tag-cleanup branch February 8, 2024 19:55
@openedx-webhooks
Copy link

@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
Copy link
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
Contributor

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

1 similar comment
@edx-pipeline-bot
Copy link
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
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants