From 46284bef0f39959e2dfa444bde329a03732753f1 Mon Sep 17 00:00:00 2001 From: Hettie Street <43522239+HettieS@users.noreply.github.com> Date: Thu, 12 Dec 2024 09:27:57 +0000 Subject: [PATCH] EL-1902: 404 page post rebase (#387) * EL-1902: Handle 404s * Amend code post rebase --- fala/apps/adviser/forms.py | 1 - fala/apps/adviser/laa_laa_paginator.py | 5 +- fala/apps/adviser/models.py | 130 +++++++++--------- .../tests/test_fala_views_and_functions.py | 11 ++ .../tests/test_other_regions_playwright.py | 4 +- .../tests/test_pagination_playwright.py | 2 +- .../test_results_page_journeys_playwright.py | 10 +- .../test_search_page_journeys_playwright.py | 2 +- fala/apps/laalaa/api.py | 15 +- fala/playwright/setup.py | 8 +- 10 files changed, 114 insertions(+), 74 deletions(-) diff --git a/fala/apps/adviser/forms.py b/fala/apps/adviser/forms.py index 92c2119d..e167a228 100644 --- a/fala/apps/adviser/forms.py +++ b/fala/apps/adviser/forms.py @@ -73,7 +73,6 @@ def perform_search(form, postcode, categories, page): ) if "error" in data: form.add_error("postcode", data["error"]) - return {} return data except laalaa.LaaLaaError: form.add_error("postcode", "%s %s" % (_("Error looking up legal advisers."), _("Please try again later."))) diff --git a/fala/apps/adviser/laa_laa_paginator.py b/fala/apps/adviser/laa_laa_paginator.py index 6ff3c80a..08215e47 100644 --- a/fala/apps/adviser/laa_laa_paginator.py +++ b/fala/apps/adviser/laa_laa_paginator.py @@ -1,3 +1,6 @@ +import math + + class LaaLaaPaginator(object): def __init__(self, count, page_size, window_size, current_page): self._window_size = window_size @@ -5,7 +8,7 @@ def __init__(self, count, page_size, window_size, current_page): self._current_page = 1 else: self._current_page = current_page - self._very_last_page = count // page_size + 1 + self._very_last_page = math.ceil(count / page_size) @property def page_range(self): diff --git a/fala/apps/adviser/models.py b/fala/apps/adviser/models.py index 2c8a8d5f..d011f0a2 100644 --- a/fala/apps/adviser/models.py +++ b/fala/apps/adviser/models.py @@ -1,5 +1,6 @@ from django.db import models from django.utils import timezone +from django.http import Http404 from .regions import Region from .laa_laa_paginator import LaaLaaPaginator import urllib @@ -29,69 +30,72 @@ def get_queryset(self): return self._data.get("results", None) def get_context_data(self): - pages = LaaLaaPaginator(self._data["count"], 10, 3, self._form.current_page) - current_page = pages.current_page() - params = { - "postcode": self._form.cleaned_data["postcode"], - "name": self._form.cleaned_data["name"], - } - categories = self._form.cleaned_data["categories"] - - # create list of tuples which can be passed to urlencode for pagination links - category_tuples = [("categories", c) for c in categories] - - def item_for(page_num): - if len(categories) > 0: - page_params = {"page": page_num} - href = ( - "/search?" - + urllib.parse.urlencode({**page_params, **params}) - + "&" - + urllib.parse.urlencode(category_tuples) - ) - else: - page_params = {"page": page_num} - href = "/search?" + urllib.parse.urlencode({**page_params, **params}) - - return {"number": page_num, "current": self._form.current_page == page_num, "href": href} - - pagination = {"items": [item_for(page_num) for page_num in pages.page_range]} - - if current_page.has_previous(): - if len(categories) > 0: - page_params = {"page": current_page.previous_page_number()} - prev_link = ( - "/search?" - + urllib.parse.urlencode({**page_params, **params}) - + "&" - + urllib.parse.urlencode(category_tuples) - ) - else: - page_params = {"page": current_page.previous_page_number()} - prev_link = "/search?" + urllib.parse.urlencode({**page_params, **params}) - pagination["previous"] = {"href": prev_link} - - if current_page.has_next(): - if len(categories) > 0: - page_params = {"page": current_page.next_page_number()} - href = ( - "/search?" - + urllib.parse.urlencode({**page_params, **params}) - + "&" - + urllib.parse.urlencode(category_tuples) - ) - else: - page_params = {"page": current_page.next_page_number()} - href = "/search?" + urllib.parse.urlencode({**page_params, **params}) - pagination["next"] = {"href": href} - - return { - "form": self._form, - "data": self._data, - "params": params, - "FEATURE_FLAG_SURVEY_MONKEY": settings.FEATURE_FLAG_SURVEY_MONKEY, - "pagination": pagination, - } + if "Invalid page" in self._data.get("error", []): + raise Http404("Page not found") + else: + pages = LaaLaaPaginator(self._data["count"], 10, 3, self._form.current_page) + current_page = pages.current_page() + params = { + "postcode": self._form.cleaned_data["postcode"], + "name": self._form.cleaned_data["name"], + } + categories = self._form.cleaned_data["categories"] + + # create list of tuples which can be passed to urlencode for pagination links + category_tuples = [("categories", c) for c in categories] + + def item_for(page_num): + if len(categories) > 0: + page_params = {"page": page_num} + href = ( + "/search?" + + urllib.parse.urlencode({**page_params, **params}) + + "&" + + urllib.parse.urlencode(category_tuples) + ) + else: + page_params = {"page": page_num} + href = "/search?" + urllib.parse.urlencode({**page_params, **params}) + + return {"number": page_num, "current": self._form.current_page == page_num, "href": href} + + pagination = {"items": [item_for(page_num) for page_num in pages.page_range]} + + if current_page.has_previous(): + if len(categories) > 0: + page_params = {"page": current_page.previous_page_number()} + prev_link = ( + "/search?" + + urllib.parse.urlencode({**page_params, **params}) + + "&" + + urllib.parse.urlencode(category_tuples) + ) + else: + page_params = {"page": current_page.previous_page_number()} + prev_link = "/search?" + urllib.parse.urlencode({**page_params, **params}) + pagination["previous"] = {"href": prev_link} + + if current_page.has_next(): + if len(categories) > 0: + page_params = {"page": current_page.next_page_number()} + href = ( + "/search?" + + urllib.parse.urlencode({**page_params, **params}) + + "&" + + urllib.parse.urlencode(category_tuples) + ) + else: + page_params = {"page": current_page.next_page_number()} + href = "/search?" + urllib.parse.urlencode({**page_params, **params}) + pagination["next"] = {"href": href} + + return { + "form": self._form, + "data": self._data, + "params": params, + "FEATURE_FLAG_SURVEY_MONKEY": settings.FEATURE_FLAG_SURVEY_MONKEY, + "pagination": pagination if len(pagination["items"]) > 1 else {}, + } class OtherJurisdictionState(object): diff --git a/fala/apps/adviser/tests/test_fala_views_and_functions.py b/fala/apps/adviser/tests/test_fala_views_and_functions.py index 05df6f18..edef84bd 100644 --- a/fala/apps/adviser/tests/test_fala_views_and_functions.py +++ b/fala/apps/adviser/tests/test_fala_views_and_functions.py @@ -248,3 +248,14 @@ class AccessibilityViewTest(SimpleTestCase): def test_shows_new_accessibility_statement(self): response = self.client.get(self.url) self.assertContains(response, "Accessibility statement for Find a legal advisor") + + +class ErrorPageTest(SimpleTestCase): + client = Client() + url = reverse("search") + + def test_raises_404_when_page_number_does_not_exist( + self, + ): + response = self.client.get(self.url, {"postcode": "SE11", "page": 500}) + self.assertEqual(response.status_code, 404) diff --git a/fala/apps/adviser/tests/test_other_regions_playwright.py b/fala/apps/adviser/tests/test_other_regions_playwright.py index 341b9cc4..9dd7c36e 100644 --- a/fala/apps/adviser/tests/test_other_regions_playwright.py +++ b/fala/apps/adviser/tests/test_other_regions_playwright.py @@ -4,7 +4,7 @@ class OtherRegionsTest(PlaywrightTestSetup): def test_jersey(self): - results_page = self.visit_results_page("JE1") + results_page = self.visit_results_page(postcode="JE1") expect(results_page.h1).to_have_text("The postcode JE1 is in Jersey") search_page = results_page.change_search() # We don't preserve Jersey postcodes search term, by design, as we don't want residents from Jersey to use our service @@ -13,7 +13,7 @@ def test_jersey(self): def test_scotland_with_persistant_search_and_categories(self): checkboxes = ["Family mediation", "Clinical Negligence"] - results_page = self.visit_results_page("TD13", checkboxes) + results_page = self.visit_results_page(postcode="TD13", checkbox_labels=checkboxes) expect(results_page.item_from_text("Legal Aid in Scotland")).to_be_visible() search_page = results_page.change_search() diff --git a/fala/apps/adviser/tests/test_pagination_playwright.py b/fala/apps/adviser/tests/test_pagination_playwright.py index b4c9f61d..40b53112 100644 --- a/fala/apps/adviser/tests/test_pagination_playwright.py +++ b/fala/apps/adviser/tests/test_pagination_playwright.py @@ -7,7 +7,7 @@ class PaginationResults(PlaywrightTestSetup): def test_results_pagination(self): checkboxes = ["Crime"] - page = self.visit_results_page("M25 3JF", checkboxes) + page = self.visit_results_page(postcode="M25 3JF", checkbox_labels=checkboxes) expect(page.item_from_text("results in order of closeness to M25 3JF.")).to_be_visible() first_page_result_count = page.result_count.inner_text() expect(page.pagination_link_title).to_be_visible() diff --git a/fala/apps/adviser/tests/test_results_page_journeys_playwright.py b/fala/apps/adviser/tests/test_results_page_journeys_playwright.py index 8ac4250f..0a161090 100644 --- a/fala/apps/adviser/tests/test_results_page_journeys_playwright.py +++ b/fala/apps/adviser/tests/test_results_page_journeys_playwright.py @@ -4,7 +4,15 @@ class ResultPageEndToEndJourneys(PlaywrightTestSetup): def test_change_search(self): - results_page = self.visit_results_page("SE11") + results_page = self.visit_results_page(postcode="SE11") expect(results_page.change_search_grey_box).to_have_text("Postcode: SE11") search_page = results_page.change_search() expect(search_page.h1).to_have_text("Find a legal aid adviser or family mediator") + + def test_pagination_does_not_appear_when_there_is_one_page(self): + results_page = self.visit_results_page(postcode="W1J5BF", organisation="charles") + expect(results_page.pagination_link_title).not_to_be_visible() + + def test_pagination_does_appears_when_there_is_more_than_one_page(self): + results_page = self.visit_results_page(postcode="SE11") + expect(results_page.pagination_link_title).to_be_visible() diff --git a/fala/apps/adviser/tests/test_search_page_journeys_playwright.py b/fala/apps/adviser/tests/test_search_page_journeys_playwright.py index 775585cc..7fda9b3f 100644 --- a/fala/apps/adviser/tests/test_search_page_journeys_playwright.py +++ b/fala/apps/adviser/tests/test_search_page_journeys_playwright.py @@ -36,7 +36,7 @@ def test_postcode_search_journey(self): ] for postcode in test_cases: with self.subTest(postcode=postcode): - page = self.visit_results_page(postcode) + page = self.visit_results_page(postcode=postcode) expect(page.h1).to_have_text("Search results") expect(page.change_search_grey_box.nth(0)).to_have_text(f"Postcode: {postcode}") expect(page.item_from_text("in order of closeness")).to_be_visible() diff --git a/fala/apps/laalaa/api.py b/fala/apps/laalaa/api.py index 2d3b3209..321705a5 100644 --- a/fala/apps/laalaa/api.py +++ b/fala/apps/laalaa/api.py @@ -41,7 +41,12 @@ def laalaa_url(**kwargs): def laalaa_search(**kwargs): try: response = requests.get(laalaa_url(**kwargs)) + response.raise_for_status() return response.json() + except requests.exceptions.HTTPError as e: + if response.status_code == 404: + return response.json() + raise LaaLaaError(e) except (requests.exceptions.RequestException, ValueError) as e: raise LaaLaaError(e) @@ -65,6 +70,10 @@ def find(postcode=None, categories=None, page=1, organisation_types=None, organi organisation_name=organisation_name, ) - data["results"] = list(map(decode_categories, data.get("results", []))) - - return data + # because of the way the current error capture and forms are set up, it is + # easier to allow the 404 errors through as a valid response and treat them separately. + if "error" in data: + return data + else: + data["results"] = list(map(decode_categories, data.get("results", []))) + return data diff --git a/fala/playwright/setup.py b/fala/playwright/setup.py index 077461d8..2b7c12c9 100644 --- a/fala/playwright/setup.py +++ b/fala/playwright/setup.py @@ -26,12 +26,18 @@ def tearDown(self): self.browser.close() super().tearDown() - def visit_results_page(self, postcode, checkbox_labels=None): + def visit_results_page(self, **kwargs): + postcode = kwargs.get("postcode") + organisation = kwargs.get("organisation", None) + checkbox_labels = kwargs.get("checkbox_labels", None) + if checkbox_labels is None: checkbox_labels = [] page = self.browser.new_page() page.goto(f"{self.live_server_url}") page.get_by_label("Postcode").fill(postcode) + if organisation: + page.get_by_label("Name of organisation you are looking for (optional)").fill(organisation) for label in checkbox_labels: page.get_by_label(label).check() page.get_by_role("button", name="Search").click()