FAL-3449 - Taxonomy view management apis [WIP]

[rpenido]

No description provided.

[pomegranited]

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.

[rpenido]

Thank you for the review, @pomegranited! The code is a lot cleaner now!

[pomegranited]

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

[pomegranited]

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 superusers) 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.

Suggested change

	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)

[rpenido]

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?

[pomegranited]

@rpenido

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

[rpenido]

Closing in favor of openedx#63