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
FAL-3449 - Taxonomy view management apis [WIP] #4
FAL-3449 - Taxonomy view management apis [WIP] #4
Conversation
bc75e61
to
a6b0bf8
Compare
a7661bb
to
0807949
Compare
8a45e8a
to
a0fedd3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rpenido -- This is a good start, but I think it can be leaner.
Please see my suggestions, and feel free to push back if you disagree.
9188fe4
to
107c1f6
Compare
Thank you for the review, @pomegranited! The code is a lot cleaner now! |
8e420c3
to
963841c
Compare
…vior (openedx#62) Adds support for custom Taxonomy subclasses to be stored against a Taxonomy, to be used by the python API when instantiating and returning Taxonomies. Also adds minimal support for ObjectTag subclasses. However, these are not stored against the ObjectTag instances; they can be instantiated by the Taxonomy subclasses if and when needed. Related: * docs: updates decisions to reflect this change * feat: adds api.get_taxonomy, which returns a Taxonomy cast to its subclass, when set * refactor: adds _check_taxonomy, _check_tag, and _check_object methods to the Taxonomy class, which can be overridden by subclasses when validating ObjectTags Added to support system-defined Taxonomies: * feat: adds un-editable Taxonomy.system_defined field so that system taxonomies can store this field and ensure no one edits them. * feat: adds Taxonomy.visible_to_authors, which is needed for fully automated tagging. Cleanup changes: * fix: updates Tag model to cascade delete if the Taxonomy or parent Tag is deleted. * style: adds missing type annotations to rules and python API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpenido Getting closer :) Few more changes and then it'll be perfect.
And good news -- since openedx#62 is merged, now, you can prepare your PR against main. Feel free to do that when you're happy with what's here, and you've addressed my remaining changes?
|
||
def has_permission(self, request, view): | ||
# Workaround to allow 'retrieve' view to pass through the | ||
# method GET permission check. Actual object permission check | ||
# is done in the get_object() method. | ||
if view.action == "retrieve": | ||
return bool(request.user and request.user.is_authenticated) | ||
else: | ||
return super().has_permission(request, view) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see from Jira that you had a question about permissions, but I don't see it on the PR?
Basically, I'm suggesting that we don't need this method at all, and so the view permissions will use the rules we've established, namely that that only staff
(or superuser
s) should be allowed to view taxonomies, or do anything else with them.
Normal users cannot even view them, but that's what this method allows, so it should be removed, and any failing tests updated accordingly.
def has_permission(self, request, view): | |
# Workaround to allow 'retrieve' view to pass through the | |
# method GET permission check. Actual object permission check | |
# is done in the get_object() method. | |
if view.action == "retrieve": | |
return bool(request.user and request.user.is_authenticated) | |
else: | |
return super().has_permission(request, view) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rule (the one you linked) allows ANY user to view an {enabled=true}
taxonomy. That's why I'm confused. If we want to ensure that only staff should be allowed to view taxonomies, then we should rewrite the rule as follows:
@rules.predicate
def can_view_taxonomy(user: User, taxonomy: Taxonomy = None) -> bool:
"""
Only taxonomy admins can view a taxonomy.
"""
return is_taxonomy_admin(user)
And this test (line 95):
openedx-learning/tests/openedx_tagging/core/tagging/test_rules.py
Lines 82 to 96 in 963841c
@ddt.data( | |
True, | |
False, | |
) | |
def test_view_taxonomy_enabled(self, enabled): | |
"""Anyone can see enabled taxonomies, but learners cannot see disabled taxonomies""" | |
self.taxonomy.enabled = enabled | |
assert self.superuser.has_perm("oel_tagging.view_taxonomy") | |
assert self.superuser.has_perm("oel_tagging.view_taxonomy", self.taxonomy) | |
assert self.staff.has_perm("oel_tagging.view_taxonomy") | |
assert self.staff.has_perm("oel_tagging.view_taxonomy", self.taxonomy) | |
assert not self.learner.has_perm("oel_tagging.view_taxonomy") | |
assert ( | |
self.learner.has_perm("oel_tagging.view_taxonomy", self.taxonomy) == enabled | |
) |
Sorry for asking for more clarification. Am I misunderstanding you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rule (the one you linked) allows ANY user to view an {enabled=true} taxonomy. That's why I'm confused.
Ah yes, I spoke incorrectly here.. My apologies! Logged-in users should be able to list taxonomies, and so they need non-object view perms.
So instead of hacking around this with has_permission
, let's fix that rule:
diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py
index 052dfdf..4c506a3 100644
--- a/openedx_tagging/core/tagging/rules.py
+++ b/openedx_tagging/core/tagging/rules.py
@@ -19,7 +19,7 @@ def can_view_taxonomy(user: User, taxonomy: Taxonomy = None) -> bool:
Anyone can view an enabled taxonomy,
but only taxonomy admins can view a disabled taxonomy.
"""
- return (taxonomy and taxonomy.enabled) or is_taxonomy_admin(user)
+ return (not taxonomy or taxonomy.enabled) or is_taxonomy_admin(user)
@rules.predicate
diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py
index 577c594..415ae19 100644
--- a/tests/openedx_tagging/core/tagging/test_rules.py
+++ b/tests/openedx_tagging/core/tagging/test_rules.py
@@ -90,7 +90,7 @@ class TestRulesTagging(TestTagTaxonomyMixin, TestCase):
assert self.superuser.has_perm("oel_tagging.view_taxonomy", self.taxonomy)
assert self.staff.has_perm("oel_tagging.view_taxonomy")
assert self.staff.has_perm("oel_tagging.view_taxonomy", self.taxonomy)
- assert not self.learner.has_perm("oel_tagging.view_taxonomy")
+ assert self.learner.has_perm("oel_tagging.view_taxonomy")
assert (
self.learner.has_perm("oel_tagging.view_taxonomy", self.taxonomy) == enabled
)
diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py
index b714015..e282580 100644
--- a/tests/openedx_tagging/core/tagging/test_views.py
+++ b/tests/openedx_tagging/core/tagging/test_views.py
@@ -86,7 +86,7 @@ class TestTaxonomyViewSet(APITestCase):
@ddt.data(
(None, status.HTTP_403_FORBIDDEN),
- ("user", status.HTTP_403_FORBIDDEN),
+ ("user", status.HTTP_200_OK),
("staff", status.HTTP_200_OK),
)
@ddt.unpack
…x#66) AWS Aurora does not support COMPRESSED in its MySQL implementation: https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/AuroraMySQL.Migrating.RDSMySQL.Import.html
adfcbbd
to
7c93cd2
Compare
2b6fd16
to
1b1b59a
Compare
Closing in favor of openedx#63 |
No description provided.