search

openedx / openedx-learning

GNU Affero General Public License v3.0
5 stars 10 forks source link

System defined taxonomies #67

Closed ChrisChV closed 1 year ago

ChrisChV commented 1 year ago

Description

Subclasses related to build System-defined taxonomies:

Testing instructions

Manual testing is possible but not very useful, since we don't yet have views for these models and APIs.

But you can check that the added tests cover the expected polymorphism behavior supported by this change, and that tests are still covering 100% of the openedx_tagging module.

Other information

Relates to https://github.com/openedx/modular-learning/issues/77

openedx-webhooks commented 1 year ago

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

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.

pomegranited commented 1 year ago

Thanks for applying those changes @ChrisChV ! I think this is ready for @ormsbee and/or @bradenmacdonald 's eyes now.

bradenmacdonald commented 1 year ago

One thing to think about for the future:

I think that our conceptual model may be missing a key piece: a model to track how a Taxonomy should be used.

Right now, there's no place to say "I want to use the Language Taxonomy to tag Content Objects belonging to X organization" or "I want users from org X to use OrgXTaxonomy to tag Course objects belonging to the organization" or "I want users from org Y to use the SpokenLanguage taxonomy to tag learners".

If we had a TaxonomyUseCase (?) model, it could specify who, what, why, where -> which org/users can use which taxonomy for which purpose to tag what type of objects using which ObjectTag subclass.

That would solve the weird problem now where if you use LanguageSystemTaxonomy to tag an XBlock, it gets a plain ObjectTag, but if you use an org's closed taxonomy to tag the same XBlock, it gets a ContentObjectTag. I think both should be using ContentObjectTag if they're both tagging content objects, but currently you have to specify the ObjectTag subclass at the Taxonomy level, not as part of defining a particular use case for the taxonomy.

Anyhow, I'm not saying to do it this way or to launch a major refactor. Just keep this idea in mind while we're iterating the models in the future, in case it resonates with you or inspires something.

ChrisChV commented 1 year ago

You can merge this now and remove ModelObjectTag soon in a separate PR, or remove it now, or justify why we want to keep it.

@bradenmacdonald I also agree that it should be removed. @pomegranited added it to do it in a separate PR: https://github.com/openedx/openedx-learning/pull/67#discussion_r1275584286

That would solve the weird problem now where if you use LanguageSystemTaxonomy to tag an XBlock, it gets a plain ObjectTag, but if you use an org's closed taxonomy to tag the same XBlock, it gets a ContentObjectTag. I think both should be using ContentObjectTag if they're both tagging content objects, but currently you have to specify the ObjectTag subclass at the Taxonomy level, not as part of defining a particular use case for the taxonomy.

We are currently solving this by creating subclasses for tagging content. But yes, I agree that there must be a simpler way to do it.

bradenmacdonald commented 1 year ago

We are currently solving this by creating subclasses for tagging content. But yes, I agree that there must be a simpler way to do it.

@ChrisChV OK, somehow I really didn't understand that until now. So to tag content with the (built-in, system defined) language Taxonomy, we cast it to ContentLanguageTaxonomy which is still the same taxonomy, but we now have a different "view" of that Taxonomy which is now using ContentObjectTag to tag objects? Is that right? I guess that works, and I'm fine with that for now, but it seems like a strange use of subclassing to me, as there's nothing really different about ContentLanguageTaxonomy vs LanguageTaxonomy except for what type of ObjectTag it applies, and that only affects the add_tag action, not what the overall Taxonomy is. I thought that just using tag_content_object() would be enough to apply a ContentObjectTag instead of an ObjectTag.

Like if you had a Person class and the person is signing autographs, and now you want the person to sign autographs using blue ink instead of black ink, it makes much more sense to say Person.sign_autograph(book_to_sign, using_ink=Ink.Blue) than to say Person.cast_to(BlueInkUsingPerson).sign_autograph(book_to_sign).

I have already approved this PR, but I just noticed there is no test that actually applies some system-defined tags to content objects. Shouldn't we add that in?

ChrisChV commented 1 year ago

@bradenmacdonald Thanks for your feedback!

OK, somehow I really didn't understand that until now. So to tag content with the (built-in, system defined) language Taxonomy, we cast it to ContentLanguageTaxonomy which is still the same taxonomy, but we now have a different "view" of that Taxonomy which is now using ContentObjectTag to tag objects? Is that right?

Yes, it is.

I have already approved this PR, but I just noticed there is no test that actually applies some system-defined tags to content objects. Shouldn't we add that in?

Yes, sorry, I added the system defined to the api test, that includes tag objects: 034726df9374ca549c4947b76556f41090a5a5cc

pomegranited commented 1 year ago

Sorry @ChrisChV one last change request: could you bump the version to "0.1.1"? We'll tag it after merging to produce a new release.