Merge pull request #716 from nationalarchives/feature/cool-uris

Redesign URL structure to be cleaner and more consistent

ds_caselaw_editor_ui/static/js/src/app.js

@@ -66,9 +66,9 @@ $(".judgments-list__judgment-assign-form").on("submit", function (event) {
     success: function (data) {
       const assigned_to = data["assigned_to"];
       loading.replaceWith(
-        "<a aria-busy='false' href='/edit?judgment_uri=" +
+        "<a aria-busy='false' href='/" +
           encodeURIComponent(uri) +
-          "#assigned_to'>" +
+          "/edit#assigned_to'>" +
           assigned_to +
           "</a>"
       );

ds_caselaw_editor_ui/templates/includes/judgment_text_toolbar.html

@@ -11,10 +11,10 @@
     <div class="judgment-toolbar__edit">
 
       {% if context.judgment.is_editable %}
-        <a class="judgment-toolbar__button{% if not context.judgment_content %} judgment-toolbar__button-selected{% endif %}" href="{% url 'edit' %}?judgment_uri={{context.judgment_uri}}">
+        <a class="judgment-toolbar__button{% if not context.judgment_content %} judgment-toolbar__button-selected{% endif %}" href="{% url 'edit-judgment' context.judgment.uri %}">
           {% translate "judgment.edit_this_judgment" %}
         </a>
-        <a class="judgment-toolbar__button{% if context.judgment_content %} judgment-toolbar__button-selected{% endif %}" href="{% url 'detail' %}?judgment_uri={{context.judgment_uri}}">
+        <a class="judgment-toolbar__button{% if context.judgment_content %} judgment-toolbar__button-selected{% endif %}" href="{% url 'full-text-html' context.judgment_uri %}">
           {% translate "judgment.view_this_judgment" %}
         </a>
       {% else %}

ds_caselaw_editor_ui/templates/includes/judgments_list_item.html

@@ -4,7 +4,7 @@
 <li class="judgments-list__judgment">
   <div class="judgments-list__judgment-details">
     <div class="judgments-list__judgment-details-name">
-      <a href="{% url 'detail' %}?judgment_uri={{item.uri}}">
+      <a href="{% url 'full-text-html' item.uri %}">
         {% if not item.is_failure %}
           {{ item.name | default:"[Untitled Judgment]"}}
         {% else %}
@@ -95,7 +95,7 @@
         </span>
         <span class="judgments-list__judgment-details-meta-value">
           {% if item.meta.assigned_to %}
-            <a href="{% url 'edit' %}?judgment_uri={{item.uri}}#assigned_to">{{ item.meta.assigned_to }}</a>
+            <a href="{% url 'edit-judgment' item.uri%}#assigned_to">{{ item.meta.assigned_to }}</a>
           {% else %}
             <form action="/assign" method="post" class="judgments-list__judgment-assign-form">
               {% csrf_token %}
@@ -114,7 +114,7 @@
   </div>
   <div class="judgments-list__judgment-assigned">
     {% if item.meta.assigned_to %}
-      <a href="{% url 'edit' %}?judgment_uri={{item.uri}}#assigned_to">{{ item.meta.assigned_to }}</a>
+      <a href="{% url 'edit-judgment' item.uri %}#assigned_to">{{ item.meta.assigned_to }}</a>
     {% else %}
       <form action="/assign" method="post" class="judgments-list__judgment-assign-form">
         {% csrf_token %}

ds_caselaw_editor_ui/templates/judgment/confirm-unlock.html

@@ -16,6 +16,6 @@
       <input type="hidden" name="judgment_uri" value="{{context.judgment_uri}}">
       <button type="submit">{% translate "judgment.unlock_judgment_title" %}</button>
     </form>
-    <a href="{% url 'detail' %}?judgment_uri={{context.judgment_uri}}">{% translate "judgment.back_to_judgment" %}</a>
+    <a href="{% url 'full-text-html' context.judgment_uri %}">{% translate "judgment.back_to_judgment" %}</a>
   </div>
 {% endblock content %}

ds_caselaw_editor_ui/templates/judgment/edit.html

@@ -73,7 +73,7 @@ <h2 class="list-versions-component__header">{% translate "judgment.previous_vers
         <ul>
           {% for version in context.judgment.versions %}
             <li>
-              <a class="list-versions__version-title" href="{% url 'detail' %}?judgment_uri={{context.judgment_uri}}&version_uri={{version.uri}}">
+              <a class="list-versions__version-title" href="{% url 'full-text-html' context.judgment_uri %}?version_uri={{version.uri}}">
                 v{{ version.version }} ( {{version.uri}} )
               </a>
             </li>

judgments/models.py

@@ -177,6 +177,9 @@ def __init__(self, uri: str, api_client: Optional[MarklogicApiClient] = None):
                 use_https=settings.MARKLOGIC_USE_HTTPS,
             )
 
+        # As part of initialisation, we preload the NCN so we can generate a MarklogicResourceNotFoundError early
+        self.neutral_citation
+
     @property
     def public_uri(self) -> str:
         return "https://caselaw.nationalarchives.gov.uk/{uri}".format(uri=self.uri)
@@ -241,7 +244,7 @@ def pdf_url(self) -> str:
 
     @property
     def xml_url(self) -> str:
-        return reverse("detail_xml") + "?judgment_uri=" + self.uri
+        return reverse("full-text-xml", kwargs={"judgment_uri": self.uri})
 
     @cached_property
     def assigned_to(self) -> str:

judgments/tests/test_judgments.py

@@ -0,0 +1,63 @@
+from unittest.mock import patch
+from urllib.parse import urlencode
+
+from caselawclient.Client import MarklogicResourceNotFoundError
+from django.contrib.auth.models import User
+from django.test import TestCase
+from django.urls import reverse
+
+
+class TestJudgment(TestCase):
+    @patch(
+        "judgments.models.MarklogicApiClient.get_judgment_citation",
+        side_effect=MarklogicResourceNotFoundError(),
+    )
+    def test_judgment_html_view_not_found_response(self, mock_api_client):
+        self.client.force_login(User.objects.get_or_create(username="testuser")[0])
+        response = self.client.get("/ewca/civ/2004/63X")
+        decoded_response = response.content.decode("utf-8")
+        self.assertIn("Judgment was not found", decoded_response)
+        self.assertEqual(response.status_code, 404)
+
+    def test_judgment_html_view_redirect(self):
+        self.client.force_login(User.objects.get_or_create(username="testuser")[0])
+        response = self.client.get("/detail?judgment_uri=ewca/civ/2004/63X")
+        assert response.status_code == 302
+        assert response["Location"] == reverse(
+            "full-text-html", kwargs={"judgment_uri": "ewca/civ/2004/63X"}
+        )
+
+    def test_judgment_html_view_redirect_with_version(self):
+        self.client.force_login(User.objects.get_or_create(username="testuser")[0])
+        response = self.client.get(
+            "/detail?judgment_uri=ewca/civ/2004/63X&version_uri=ewca/civ/2004/63X_xml_versions/1-376"
+        )
+        assert response.status_code == 302
+        assert response["Location"] == (
+            reverse("full-text-html", kwargs={"judgment_uri": "ewca/civ/2004/63X"})
+            + "?"
+            + urlencode(
+                {
+                    "version_uri": "ewca/civ/2004/63X_xml_versions/1-376",
+                }
+            )
+        )
+
+    @patch(
+        "judgments.models.MarklogicApiClient.get_judgment_citation",
+        side_effect=MarklogicResourceNotFoundError(),
+    )
+    def test_judgment_xml_view_not_found_response(self, mock_api_client):
+        self.client.force_login(User.objects.get_or_create(username="testuser")[0])
+        response = self.client.get("/ewca/civ/2004/63X/xml")
+        decoded_response = response.content.decode("utf-8")
+        self.assertIn("Judgment was not found", decoded_response)
+        self.assertEqual(response.status_code, 404)
+
+    def test_judgment_xml_view_redirect(self):
+        self.client.force_login(User.objects.get_or_create(username="testuser")[0])
+        response = self.client.get("/xml?judgment_uri=ewca/civ/2004/63X")
+        assert response.status_code == 302
+        assert response["Location"] == reverse(
+            "full-text-xml", kwargs={"judgment_uri": "ewca/civ/2004/63X"}
+        )

judgments/tests/test_models_judgment.py

@@ -157,7 +157,7 @@ def test_judgment_pdf_url(self, mock_url_generator, mock_api_client):
     def test_judgment_xml_url(self, mock_api_client):
         judgment = Judgment("test/1234", mock_api_client)
 
-        assert judgment.xml_url == "/xml?judgment_uri=test/1234"
+        assert judgment.xml_url == "/test/1234/xml"
 
     def test_judgment_assigned_to(self, mock_api_client):
         mock_api_client.get_property.return_value = "testuser"

judgments/tests/test_unlock.py

@@ -3,6 +3,9 @@
 import pytest
 from django.contrib.auth.models import User
 from django.test import Client
+from django.urls import reverse
+
+from judgments.models import Judgment
 
 
 @pytest.mark.django_db
@@ -20,14 +23,19 @@ def test_break_lock_confirm_page():
 
 
 @pytest.mark.django_db
+@patch(
+    "judgments.views.unlock.Judgment",
+    autospec=Judgment,
+)
 @patch("judgments.views.unlock.api_client.break_checkout")
 @patch("judgments.views.unlock.messages")
-def test_break_lock_post(messages, break_checkout):
+def test_break_lock_post(messages, break_checkout, mock_judgment):
+    mock_judgment.return_value.uri = "ewca/civ/2023/1"
     client = Client()
     client.force_login(User.objects.get_or_create(username="testuser")[0])
 
     response = client.post("/unlock", data={"judgment_uri": "/ewca/civ/2023/1"})
-    break_checkout.assert_called_with("/ewca/civ/2023/1")
+    break_checkout.assert_called_with("ewca/civ/2023/1")
     messages.success.assert_called_with(ANY, "Judgment unlocked.")
     assert response.status_code == 302
-    assert response.url == "/edit?judgment_uri=/ewca/civ/2023/1"  # type: ignore
+    assert response.url == reverse("edit-judgment", kwargs={"judgment_uri": "ewca/civ/2023/1"})  # type: ignore

judgments/tests/tests.py

@@ -2,19 +2,12 @@
 
 from django.contrib.auth.models import User
 from django.test import TestCase
+from django.urls import reverse
 from lxml import etree
 
 from judgments.models import Judgment, SearchResult, SearchResultMeta
 
 
-class TestJudgment(TestCase):
-    def test_404_response(self):
-        response = self.client.get("/judgments/ewca/civ/2004/63X")
-        decoded_response = response.content.decode("utf-8")
-        self.assertIn("Page not found", decoded_response)
-        self.assertEqual(response.status_code, 404)
-
-
 class TestSearchResults(TestCase):
     @patch("judgments.utils.view_helpers.perform_advanced_search")
     def test_oldest(self, advanced_search):
@@ -150,11 +143,14 @@ class TestJudgmentEditor(TestCase):
         autospec=Judgment,
     )
     def test_assigned(self, mock_judgment):
+        mock_judgment.return_value.uri = "ewhc/ch/1999/1"
         mock_judgment.return_value.assigned_to = "otheruser"
         mock_judgment.return_value.versions = []
 
         User.objects.get_or_create(username="otheruser")[0]
         self.client.force_login(User.objects.get_or_create(username="testuser")[0])
-        response = self.client.get("/edit?judgment_uri=ewhc/ch/1999/1")
+        response = self.client.get(
+            reverse("edit-judgment", kwargs={"judgment_uri": "ewhc/ch/1999/1"})
+        )
 
         assert b"selected>otheruser" in response.content

judgments/urls.py

@@ -6,26 +6,33 @@
     prioritise_judgment_button,
 )
 from .views.delete import delete
-from .views.detail import detail
-from .views.detail_xml import detail_xml
-from .views.edit_judgment import EditJudgmentView
+from .views.edit_judgment import EditJudgmentView, edit_view_redirect
+from .views.full_text import html_view, html_view_redirect, xml_view, xml_view_redirect
 from .views.index import index
 from .views.results import results
 from .views.signed_asset import redirect_to_signed_asset
 from .views.unlock import unlock
 
 urlpatterns = [
-    path("edit", EditJudgmentView.as_view(), name="edit"),
-    path("detail", detail, name="detail"),
-    path("xml", detail_xml, name="detail_xml"),
+    # Home
+    path("", index, name="home"),
+    # Search
     path("results", results, name="results"),
-    # buttons that do a thing and redirect
+    # redirect to signed asset URLs
+    path("signed-asset/<path:key>", redirect_to_signed_asset, name="signed-asset"),
+    # Legacy judgment verbs
     path("delete", delete, name="delete"),
     path("unlock", unlock, name="unlock"),
     path("assign", assign_judgment_button, name="assign"),
     path("prioritise", prioritise_judgment_button, name="prioritise"),
     path("hold", hold_judgment_button, name="hold"),
-    # redirect to signed asset URLs
-    path("signed-asset/<path:key>", redirect_to_signed_asset, name="signed-asset"),
-    path("", index, name="home"),
+    # Redirects for legacy judgment URIs
+    path("edit", edit_view_redirect),
+    path("detail", html_view_redirect),
+    path("xml", xml_view_redirect),
+    # Different views on judgments
+    path("<path:judgment_uri>/edit", EditJudgmentView.as_view(), name="edit-judgment"),
+    path("<path:judgment_uri>/xml", xml_view, name="full-text-xml"),
+    # This 'bare judgment' URL must always go last
+    path("<path:judgment_uri>", html_view, name="full-text-html"),
 ]

judgments/views/edit_judgment.py

@@ -73,24 +73,17 @@ def build_jira_create_link(self, request, context):
             tdr=context["judgment"].consignment_reference,
         )
 
-        editor_details_url = request.build_absolute_uri(
-            "{base_url}?{params}".format(
-                base_url=reverse("detail"),
-                params=urlencode(
-                    {
-                        "judgment_uri": context["judgment_uri"],
-                    }
-                ),
-            )
+        editor_html_url = request.build_absolute_uri(
+            reverse("full-text-html", kwargs={"judgment_uri": context["judgment_uri"]})
         )
 
-        description_string = "{editor_details_url}".format(
-            editor_details_url="""{details_url}
+        description_string = "{editor_html_url}".format(
+            editor_html_url="""{html_url}
 
 {source_name_label}: {source_name}
 {source_email_label}: {source_email}
 {consignment_ref_label}: {consignment_ref}""".format(
-                details_url=editor_details_url,
+                html_url=editor_html_url,
                 source_name_label=gettext("judgments.submitter"),
                 source_name=context["judgment"].source_name,
                 source_email_label=gettext("judgments.submitteremail"),
@@ -116,8 +109,7 @@ def render(self, request, context):
         return HttpResponse(template.render({"context": context}, request))
 
     def get(self, request, *args, **kwargs):
-        params = request.GET
-        judgment_uri = params.get("judgment_uri")
+        judgment_uri = kwargs["judgment_uri"]
         judgment = Judgment(judgment_uri)
 
         context = {"judgment_uri": judgment_uri}
@@ -177,7 +169,9 @@ def post(self, request, *args, **kwargs):
             # If judgment_uri is a `failure` URI, amend it to match new neutral citation and redirect
             if "failures" in judgment_uri and new_citation is not None:
                 new_judgment_uri = update_judgment_uri(judgment_uri, new_citation)
-                return redirect(reverse("edit") + f"?judgment_uri={new_judgment_uri}")
+                return redirect(
+                    reverse("edit-judgment", kwargs={"judgment_uri": new_judgment_uri})
+                )
 
             if published:
                 notify_status = "published"
@@ -202,4 +196,14 @@ def post(self, request, *args, **kwargs):
 
         invalidate_caches(judgment_uri)
 
-        return HttpResponseRedirect(reverse("edit") + "?judgment_uri=" + judgment_uri)
+        return HttpResponseRedirect(
+            reverse("edit-judgment", kwargs={"judgment_uri": judgment_uri})
+        )
+
+
+def edit_view_redirect(request):
+    params = request.GET
+    judgment_uri = params.get("judgment_uri", None)
+    return HttpResponseRedirect(
+        reverse("edit-judgment", kwargs={"judgment_uri": judgment_uri})
+    )