Remove system-defined taxonomies, replacing with "read_only" flag - #654
Remove system-defined taxonomies, replacing with "read_only" flag#654bradenmacdonald wants to merge 4 commits into
Conversation
|
Thanks for the pull request, @bradenmacdonald! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
ef4a798 to
dfc6079
Compare
Co-Authored-By: Claude <noreply@anthropic.com>
dfc6079 to
13ea459
Compare
|
@ormsbee (CC @mgwozdz-unicon @jesperhodge): when you have some time, could you please take a look at this PR and the corresponding platform PR? |
|
I'll start by sharing Claude Code Review:
On deletion, there's a related TaxonomyOrg row for taxonomy_id=-1 (owned by content_tagging). Does deleting the taxonomy also remove that row? A test that runs the real migration against a database (rather than the pytest-built schema) would confirm it. |
|
Thanks @jesperhodge !
Good point! Here it is: openedx/frontend-app-authoring#3183
Fixed in 4e5e09d
Yes, it does. |
jesperhodge
left a comment
There was a problem hiding this comment.
Great work! Thanks so much for this, that removes a lot of cruft in front of the Competency work. The PR looks really good, I tested with the frontend PR briefly and it seems to work fine.
I'd love my suggestion for the TagPermissionsMixin addressed if possible (reject it if you don't agree), but I'm not going to block the PR on that minor detail.
| """ | ||
| Keeps a reference to the view, so `get_can_add` can use it to check object-level permissions. | ||
| """ | ||
| self.view = view |
There was a problem hiding this comment.
Suggestion: self.view = view here is only ever read for .get_taxonomy(), and only by TagPermissionsMixin — TaxonomyPagination never uses it. Might it simplify this by capturing the taxonomy directly there instead, and drop view/self.view from the base mixin?
class TagPermissionsMixin(CanAddPermissionMixin):
def paginate_queryset(self, queryset, request, view=None):
self._taxonomy = view.get_taxonomy() if view else None
return super().paginate_queryset(queryset, request, view=view)
def get_can_add(self, _instance=None) -> bool | None:
return super().get_can_add(Tag(taxonomy=self._taxonomy) if self._taxonomy else None)
| self.system_taxonomy = Taxonomy.objects.get(name="System defined taxonomy") | ||
| self.language_taxonomy = LanguageTaxonomy.objects.get(name="Languages") | ||
| self.user_taxonomy = Taxonomy.objects.get(name="User Authors").cast() | ||
| self.system_taxonomy = Taxonomy.objects.get(name="Read Only Taxonomy") |
There was a problem hiding this comment.
Non-blocking nit: I suggest dropping the system_taxonomy language to avoid confusion later.
| self.system_taxonomy = Taxonomy.objects.get(name="Read Only Taxonomy") | |
| self.read_only_taxonomy = Taxonomy.objects.get(name="Read Only Taxonomy") |
jesperhodge
left a comment
There was a problem hiding this comment.
See my comment in the other PR: openedx/openedx-platform#38895 (comment)
Might make sense to add a little change here?
Implements #634
read_onlyflag on the base Taxonomy classcast()functionalityobject_tag_classoverride. We only useObjectTag.