-
Notifications
You must be signed in to change notification settings - Fork 4
Search fixes #3193
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
base: main
Are you sure you want to change the base?
Search fixes #3193
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import os | ||
|
|
||
| from metrics.api.settings import ROOT_LEVEL_BASE_DIR | ||
|
|
||
| DATA_UPLOAD_MAX_NUMBER_FIELDS = None | ||
|
|
||
| DEBUG = True | ||
|
|
||
| DATABASES = { | ||
| "default": { | ||
| "TIME_ZONE": "Europe/London", | ||
| "ENGINE": "django.db.backends.sqlite3", | ||
| "NAME": os.path.join(ROOT_LEVEL_BASE_DIR, "test.sqlite3"), | ||
| }, | ||
| "test": { | ||
| "TIME_ZONE": "Europe/London", | ||
| "ENGINE": "django.db.backends.sqlite3", | ||
| "NAME": os.path.join(ROOT_LEVEL_BASE_DIR, "test.sqlite3"), | ||
| }, | ||
| } | ||
|
|
||
| INTERNAL_IPS = ["127.0.0.1"] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,3 +13,5 @@ | |
| ) | ||
|
|
||
| from .root_view import PublicAPIRootViewV2 | ||
|
|
||
| from .search import SearchView | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| from drf_spectacular.utils import extend_schema | ||
| from rest_framework.request import Request | ||
| from rest_framework.response import Response | ||
| from rest_framework.views import APIView | ||
| from wagtail.api.v2.serializers import get_serializer_class | ||
| from wagtail.models import Page | ||
|
|
||
| from cms.topic.models import TopicPage | ||
| from public_api.version_02.views.base import PUBLIC_API_TAG | ||
|
|
||
|
|
||
| @extend_schema(tags=[PUBLIC_API_TAG]) | ||
| class SearchView(APIView): | ||
| """This endpoint provides search results and could in the future provide | ||
| autocomplete suggestions etc. | ||
|
|
||
| """ | ||
|
|
||
| def get(self, request: Request): | ||
| search = request.GET.get("search") | ||
| limit = int(request.GET.get("limit", 0)) | ||
| fields = request.GET.get("fields", ["title", "slug"]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my understanding is that fields would be returned as a string. i would recommend not setting the default as a list but rather splitting the atm it seems like we're trying to handle |
||
| meta = request.GET.get("meta", ["id"]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above |
||
| topic_results = TopicPage.objects.all().search(search) | ||
| if not limit or topic_results.count() < limit: | ||
| # TODO: go get more | ||
|
Check warning on line 26 in public_api/version_02/views/search.py
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we may not need the extra |
||
| # results = queryset | ||
| results = topic_results | ||
| else: | ||
| results = topic_results[0:limit] | ||
| print(f"AIDAN: returning {results}") | ||
| serialized = get_serializer_class(Page, fields, meta)(results, many=True) | ||
| return Response(serialized.data) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import factory | ||
|
|
||
| from cms.common.models import CommonPage | ||
|
|
||
|
|
||
| class CommonPageFactory(factory.django.DjangoModelFactory): | ||
| """ | ||
| Factory for creating `CommonPage` instances for tests | ||
| """ | ||
|
|
||
| class Meta: | ||
| model = CommonPage |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import factory | ||
|
|
||
| from cms.topic.models import TopicPage | ||
|
|
||
|
|
||
| class TopicPageFactory(factory.django.DjangoModelFactory): | ||
| """ | ||
| Factory for creating `Topic` instances for tests | ||
| """ | ||
|
|
||
| class Meta: | ||
| model = TopicPage |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| from http import HTTPStatus | ||
| import os | ||
| import pytest | ||
|
|
||
| from rest_framework.test import RequestsClient | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| class TestSearchAPIView: | ||
|
|
||
| @property | ||
| def path(self) -> str: | ||
| return "/api/public/search/v1" | ||
|
|
||
| @property | ||
| def target_domain(self) -> str: | ||
| return os.environ.get("PUBLIC_API_TEST_DOMAIN", "http://testserver") | ||
|
|
||
| def test_search_finds_topics_only(self, topics, other_pages): | ||
| """ | ||
| Given a string that matches all topic pages | ||
| When the API is called with that query and a limit of 5 | ||
| Then the results will include only topic pages | ||
| """ | ||
|
|
||
| # Given | ||
| limit = len(topics) | ||
| search = "topic" # All topics have a title with topic in them | ||
| query = f"search={search}&limit={limit}" | ||
|
|
||
| # When | ||
| client = RequestsClient() | ||
| url = f"{self.target_domain}{self.path}?{query}" | ||
| response: Response = client.get(url) | ||
|
|
||
| # Then | ||
| assert response.status_code == HTTPStatus.OK | ||
| response_data: list[dict] = response.json() | ||
| assert limit == len(response_data) | ||
| for page in topics: | ||
| target = {"title": page.title, "slug": page.slug} | ||
| assert response_data.index(target) > -1 | ||
|
|
||
| def test_search_finds_topics_and_pages(self, topics, other_pages): | ||
| """ | ||
| Given a string that matches 2 topic page | ||
| When the API is called with that query and no limit | ||
| Then the results will include the matching topic pages and others with the topics first | ||
| """ | ||
|
|
||
| # Given | ||
| search = "rare" # All topics have a title, only even ones have rare in it | ||
| query = f"search={search}" | ||
| expected_results = [t for t in topics if "rare" in t.title] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it be best to use Django's |
||
|
|
||
| # When | ||
| client = RequestsClient() | ||
| url = f"{self.target_domain}{self.path}?{query}" | ||
| response: Response = client.get(url) | ||
|
|
||
| # Then | ||
| assert response.status_code == HTTPStatus.OK | ||
| response_data: list[dict] = response.json() | ||
| assert len(expected_results) == len(response_data) | ||
| for page in expected_results: | ||
| target = {"title": page.title, "slug": page.slug} | ||
| assert response_data.index(target) > -1 | ||
|
|
||
| def test_search_doesnt_find_unpublish_pages(self, topics, other_pages): | ||
|
Check warning on line 69 in tests/integration/public_api/v2/views/test_search.py
|
||
| """ | ||
| Given a string that matches 2 topic page | ||
| When the API is called with that query and no limit | ||
| Then the results will include the matching topic pages and others with the topics first | ||
| """ | ||
|
|
||
| # Given | ||
| search = "rare" # All topics have a title, only even ones have rare in it | ||
| query = f"search={search}" | ||
| expected_results = [t for t in topics if "rare" in t.title] | ||
|
|
||
| # When | ||
| client = RequestsClient() | ||
| url = f"{self.target_domain}{self.path}?{query}" | ||
| response: Response = client.get(url) | ||
|
|
||
| # Then | ||
| assert response.status_code == HTTPStatus.OK | ||
| response_data: list[dict] = response.json() | ||
| assert len(expected_results) == len(response_data) | ||
| for page in expected_results: | ||
| target = {"title": page.title, "slug": page.slug} | ||
| assert response_data.index(target) > -1 | ||
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.
since this feields are exact replica, could you create it as a variable then reuse it