Skip to content

feat: add taxonomy api pagination #69

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.

Sign up for GitHub

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

Merged
pomegranited merged 4 commits into from
Aug 14, 2023
Merged

feat: add taxonomy api pagination #69

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d3d0e6
feat: add taxonomy api pagination
rpenido Aug 2, 2023
e45f791
docs: add page and page_size parameters in doc
rpenido Aug 3, 2023
dd1b2de
style: fix formating
rpenido Aug 3, 2023
0139163
refactor: set default pagination
rpenido Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions openedx_tagging/core/tagging/rest_api/v1/pagination.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
"""
Tagging API Pagination
"""
from rest_framework.pagination import PageNumberPagination


class TaggingPagination(PageNumberPagination):
page_size_query_param = "page_size"
page_size = 100
max_page_size = 1000

@bradenmacdonald bradenmacdonald Aug 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe you should just use edx_rest_framework_extensions.paginators.DefaultPagination instead.

The platform sets it as a global default for all APIs - maybe we can do so here too?

@rpenido rpenido Aug 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @bradenmacdonald!

I believe you should just use edx_rest_framework_extensions.paginators.DefaultPagination instead.

In a discussion with @pomegranited open-craft#4 (comment), we talked that we shouldn't import edx-platform here. I think I misunderstood it and avoided importing other apps from edx in general. If this is not a problem, we should use this default pagination class.

The platform sets it as a global default for all APIs - maybe we can do so here too?

I think we can. Note that we have another rest_api in this app that will also be affected (not implemented, thou):

https://github.com/openedx/openedx-learning/blob/fb1fd0860f4873aae9250901a6af75a5f86384cc/projects/urls.py#L11

If this is fine, we can set a default for all APIs.

@pomegranited pomegranited Aug 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks for that recommendation @bradenmacdonald , I didn't know that existed.

@rpenido Yes please add this dependency and use edx_rest_framework_extensions.paginators.DefaultPagination as our default for this library instead of creating our own class here.

It's ok to just use it directly, and keep its page_size and max_page_size too -- we can adjust these later if we need them in the Tagging UI.

@bradenmacdonald bradenmacdonald Aug 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pomegranited I think you pinged the wrong person ;)

@pomegranited pomegranited Aug 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oops! Sorry for the noise @robrap!

7 changes: 5 additions & 2 deletions openedx_tagging/core/tagging/rest_api/v1/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@
get_taxonomy,
get_taxonomies,
)
from .serializers import TaxonomyListQueryParamsSerializer, TaxonomySerializer
from .pagination import TaggingPagination
from .permissions import TaxonomyObjectPermissions
from .serializers import TaxonomyListQueryParamsSerializer, TaxonomySerializer


class TaxonomyView(ModelViewSet):
Expand All @@ -19,6 +20,8 @@ class TaxonomyView(ModelViewSet):

**List Query Parameters**
* enabled (optional) - Filter by enabled status. Valid values: true, false, 1, 0, "true", "false", "1"
* page (optional) - Page number (default: 1)
* page_size (optional) - Number of items per page (default: 100)

**List Example Requests**
GET api/tagging/v1/taxonomy - Get all taxonomies
Expand Down Expand Up @@ -59,7 +62,6 @@ class TaxonomyView(ModelViewSet):
"allow_free_text": True,
}


**Create Query Returns**
* 201 - Success
* 403 - Permission denied
Expand Down Expand Up @@ -107,6 +109,7 @@ class TaxonomyView(ModelViewSet):

"""

pagination_class = TaggingPagination
Comment thread
pomegranited marked this conversation as resolved.
Outdated
serializer_class = TaxonomySerializer
permission_classes = [TaxonomyObjectPermissions]

Expand Down
82 changes: 49 additions & 33 deletions tests/openedx_tagging/core/tagging/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,16 @@
from django.contrib.auth import get_user_model
from rest_framework import status
from rest_framework.test import APITestCase
from urllib.parse import urlparse, parse_qs

from openedx_tagging.core.tagging.models import Taxonomy
from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy

User = get_user_model()

TAXONOMY_LIST_URL = '/tagging/rest_api/v1/taxonomies/'
TAXONOMY_DETAIL_URL = '/tagging/rest_api/v1/taxonomies/{pk}/'
TAXONOMY_LIST_URL = "/tagging/rest_api/v1/taxonomies/"
TAXONOMY_DETAIL_URL = "/tagging/rest_api/v1/taxonomies/{pk}/"


def check_taxonomy(
data,
Expand Down Expand Up @@ -85,7 +87,7 @@ def test_list_taxonomy_queryparams(self, enabled, expected_status, expected_coun
# If we were able to list the taxonomies, check that we got the expected number back
# We take into account the Language Taxonomy that is created by the system in a migration
if status.is_success(expected_status):
assert len(response.data) == expected_count
assert len(response.data["results"]) == expected_count

@ddt.data(
(None, status.HTTP_403_FORBIDDEN),
Expand All @@ -103,14 +105,42 @@ def test_list_taxonomy(self, user_attr, expected_status):
response = self.client.get(url)
assert response.status_code == expected_status

def test_list_taxonomy_pagination(self):
url = TAXONOMY_LIST_URL
Taxonomy.objects.create(name="T1", enabled=True).save()
Taxonomy.objects.create(name="T2", enabled=True).save()
Taxonomy.objects.create(name="T3", enabled=False).save()
Taxonomy.objects.create(name="T4", enabled=False).save()
Taxonomy.objects.create(name="T5", enabled=False).save()

self.client.force_authenticate(user=self.staff)

query_params = {"page_size": 2, "page": 2}
response = self.client.get(url, query_params, format="json")

assert response.status_code == status.HTTP_200_OK

self.assertEqual(set(t["name"] for t in response.data["results"]), set(("T2", "T3")))
parsed_url = urlparse(response.data["next"])

next_page = parse_qs(parsed_url.query).get("page", [None])[0]
assert next_page == "3"

def test_list_invalid_page(self):
url = TAXONOMY_LIST_URL

self.client.force_authenticate(user=self.user)

query_params = {"page": 123123}

response = self.client.get(url, query_params, format="json")

assert response.status_code == status.HTTP_404_NOT_FOUND

@ddt.data(
(None, {"enabled": True}, status.HTTP_403_FORBIDDEN),
(None, {"enabled": False}, status.HTTP_403_FORBIDDEN),
(
"user",
{"enabled": True},
status.HTTP_200_OK,
),
("user", {"enabled": True}, status.HTTP_200_OK),
("user", {"enabled": False}, status.HTTP_404_NOT_FOUND),
("staff", {"enabled": True}, status.HTTP_200_OK),
("staff", {"enabled": False}, status.HTTP_200_OK),
Expand Down Expand Up @@ -237,22 +267,18 @@ def test_update_taxonomy(self, user_attr, expected_status):
(True, status.HTTP_403_FORBIDDEN),
)
@ddt.unpack
def test_update_taxonomy_system_defined(
self, system_defined, expected_status
):
'''
def test_update_taxonomy_system_defined(self, system_defined, expected_status):
"""
Test that we can't update system_defined field
'''
"""
taxonomy = Taxonomy.objects.create(name="test system taxonomy")
if system_defined:
taxonomy.taxonomy_class = SystemDefinedTaxonomy
taxonomy.save()
url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk)

self.client.force_authenticate(user=self.staff)
response = self.client.put(
url, {"name": "new name"}, format="json"
)
response = self.client.put(url, {"name": "new name"}, format="json")
assert response.status_code == expected_status

def test_update_taxonomy_404(self):
Expand All @@ -269,9 +295,7 @@ def test_update_taxonomy_404(self):
)
@ddt.unpack
def test_patch_taxonomy(self, user_attr, expected_status):
taxonomy = Taxonomy.objects.create(
name="test patch taxonomy", enabled=False, required=True
)
taxonomy = Taxonomy.objects.create(name="test patch taxonomy", enabled=False, required=True)
taxonomy.save()

url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk)
Expand All @@ -280,9 +304,7 @@ def test_patch_taxonomy(self, user_attr, expected_status):
user = getattr(self, user_attr)
self.client.force_authenticate(user=user)

response = self.client.patch(
url, {"name": "new name", "required": False}, format="json"
)
response = self.client.patch(url, {"name": "new name", "required": False}, format="json")
assert response.status_code == expected_status

# If we were able to update the taxonomy, check if the name changed
Expand All @@ -303,24 +325,18 @@ def test_patch_taxonomy(self, user_attr, expected_status):
(True, status.HTTP_403_FORBIDDEN),
)
@ddt.unpack
def test_patch_taxonomy_system_defined(
self, system_defined, expected_status
):
'''
def test_patch_taxonomy_system_defined(self, system_defined, expected_status):
"""
Test that we can't patch system_defined field
'''
taxonomy = Taxonomy.objects.create(
name="test system taxonomy"
)
"""
taxonomy = Taxonomy.objects.create(name="test system taxonomy")
if system_defined:
taxonomy.taxonomy_class = SystemDefinedTaxonomy
taxonomy.save()
url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk)

self.client.force_authenticate(user=self.staff)
response = self.client.patch(
url, {"name": "New name"}, format="json"
)
response = self.client.patch(url, {"name": "New name"}, format="json")
assert response.status_code == expected_status

def test_patch_taxonomy_404(self):
Expand Down