feat(FCL-533): modify human identifier to rely on identifiers framework

This is instead of assuming that an NCN will always be present. As a result, this has had knock-on changes in some other places which were previously relying on this incorrect behaviour. This includes some breaking changes to method signatures, which can now return a `None` in places they previously could not. Since relying on the presence of an NCN is now considered dangerous, these methods have been marked as deprecated.

BREAKING CHANGE: Methods which were previously guaranteed to return a Neutral Citation may now return `None`.

src/caselawclient/models/documents/__init__.py

@@ -15,6 +15,7 @@
     NotSupportedOnVersion,
     OnlySupportedOnVersion,
 )
+from caselawclient.models.identifiers import Identifier
 from caselawclient.models.identifiers.fclid import FindCaseLawIdentifier, FindCaseLawIdentifierSchema
 from caselawclient.models.identifiers.unpacker import unpack_all_identifiers_from_etree
 from caselawclient.models.utilities import VersionsDict, extract_version, render_versions
@@ -171,13 +172,11 @@ def _initialise_identifiers(self) -> None:
         self.identifiers = unpack_all_identifiers_from_etree(identifiers_element_as_etree)
 
     @property
-    def best_human_identifier(self) -> Optional[str]:
-        """
-        Some identifier that is understood by legal professionals to refer to this legal event
-        that is not the name of the document.
-        Typically, this will be the neutral citation number, should it exist.
-        Should typically be overridden in subclasses.
-        """
+    def best_human_identifier(self) -> Optional[Identifier]:
+        """Return the preferred identifier for the document, providing that it is considered human readable."""
+        preferred_identifier = self.identifiers.preferred
+        if preferred_identifier and preferred_identifier.schema.human_readable:
+            return preferred_identifier
         return None
 
     @property
@@ -507,13 +506,17 @@ def force_reparse(self) -> None:
             "documentType": parser_type_noun,
             "metadata": {
                 "name": self.body.name or None,
-                "cite": self.best_human_identifier or None,
+                "cite": None,
                 "court": self.body.court or None,
                 "date": checked_date,
                 "uri": self.uri,
             },
         }
 
+        ## TODO: Remove this hack around the fact that NCNs are assumed to be present for all documents' metadata, but actually different document classes may have different metadata
+        if hasattr(self, "neutral_citation"):
+            parser_instructions["metadata"]["cite"] = self.neutral_citation
+
         request_parse(
             uri=self.uri,
             reference=self.consignment_reference,

src/caselawclient/models/judgments.py

@@ -25,20 +25,17 @@ def __init__(self, uri: DocumentURIString, *args: Any, **kwargs: Any) -> None:
         super().__init__(self.document_noun, uri, *args, **kwargs)
 
     @cached_property
-    def neutral_citation(self) -> NeutralCitationString:
-        return NeutralCitationString(
-            self.body.get_xpath_match_string(
-                "/akn:akomaNtoso/akn:*/akn:meta/akn:proprietary/uk:cite/text()",
-                {
-                    "uk": "https://caselaw.nationalarchives.gov.uk/akn",
-                    "akn": "http://docs.oasis-open.org/legaldocml/ns/akn/3.0",
-                },
-            )
+    def neutral_citation(self) -> Optional[NeutralCitationString]:
+        value_in_xml = self.body.get_xpath_match_string(
+            "/akn:akomaNtoso/akn:*/akn:meta/akn:proprietary/uk:cite/text()",
+            {
+                "uk": "https://caselaw.nationalarchives.gov.uk/akn",
+                "akn": "http://docs.oasis-open.org/legaldocml/ns/akn/3.0",
+            },
         )
-
-    @property
-    def best_human_identifier(self) -> str:
-        return self.neutral_citation
+        if value_in_xml:
+            return NeutralCitationString(value_in_xml)
+        return None
 
     @cached_property
     def linked_document(self) -> Optional["PressSummary"]:

src/caselawclient/models/neutral_citation_mixin.py

@@ -1,9 +1,10 @@
 from abc import ABC, abstractmethod
 from functools import cached_property
-from typing import Any
+from typing import Any, Optional
 
 from ds_caselaw_utils import neutral_url
 from ds_caselaw_utils.types import NeutralCitationString
+from typing_extensions import deprecated
 
 
 class NeutralCitationMixin(ABC):
@@ -38,12 +39,15 @@ def __init__(self, document_noun: str, *args: Any, **kwargs: Any) -> None:
 
     @cached_property
     @abstractmethod
-    def neutral_citation(self) -> NeutralCitationString: ...
+    @deprecated("Legacy usage of NCNs is deprecated; you should be moving to the Identifiers framework")
+    def neutral_citation(self) -> Optional[NeutralCitationString]: ...
 
     @cached_property
+    @deprecated("Legacy usage of NCNs is deprecated; you should be moving to the Identifiers framework")
     def has_ncn(self) -> bool:
-        return bool(self.neutral_citation)
+        return self.neutral_citation is not None and self.neutral_citation != ""
 
     @cached_property
+    @deprecated("Legacy usage of NCNs is deprecated; you should be moving to the Identifiers framework")
     def has_valid_ncn(self) -> bool:
-        return self.has_ncn and neutral_url(self.neutral_citation) is not None
+        return self.neutral_citation is not None and neutral_url(self.neutral_citation) is not None

src/caselawclient/models/press_summaries.py

@@ -27,19 +27,16 @@ def __init__(self, uri: DocumentURIString, *args: Any, **kwargs: Any) -> None:
         super().__init__(self.document_noun, uri, *args, **kwargs)
 
     @cached_property
-    def neutral_citation(self) -> NeutralCitationString:
-        return NeutralCitationString(
-            self.body.get_xpath_match_string(
-                "/akn:akomaNtoso/akn:doc/akn:preface/akn:p/akn:neutralCitation/text()",
-                {
-                    "akn": "http://docs.oasis-open.org/legaldocml/ns/akn/3.0",
-                },
-            )
+    def neutral_citation(self) -> Optional[NeutralCitationString]:
+        value_in_xml = self.body.get_xpath_match_string(
+            "/akn:akomaNtoso/akn:doc/akn:preface/akn:p/akn:neutralCitation/text()",
+            {
+                "akn": "http://docs.oasis-open.org/legaldocml/ns/akn/3.0",
+            },
         )
-
-    @property
-    def best_human_identifier(self) -> str:
-        return self.neutral_citation
+        if value_in_xml:
+            return NeutralCitationString(value_in_xml)
+        return None
 
     @cached_property
     def linked_document(self) -> Optional[Judgment]:

tests/models/test_judgments.py

@@ -5,15 +5,17 @@
 from caselawclient.errors import DocumentNotFoundError
 from caselawclient.factories import PressSummaryFactory
 from caselawclient.models.documents import DocumentURIString
+from caselawclient.models.identifiers.neutral_citation import NeutralCitationNumber
 from caselawclient.models.judgments import Judgment
 from caselawclient.models.neutral_citation_mixin import NeutralCitationString
 
 
 class TestJudgment:
     def test_best_identifier(self, mock_api_client):
         judgment = Judgment(DocumentURIString("test/1234"), mock_api_client)
-        judgment.neutral_citation = NeutralCitationString("[2023] TEST 1234")
-        assert judgment.best_human_identifier == judgment.neutral_citation
+        document_ncn = NeutralCitationNumber(value="[2023] TEST 1234")
+        judgment.identifiers.add(document_ncn)
+        assert judgment.best_human_identifier == document_ncn
 
 
 class TestJudgmentValidation:

tests/models/test_press_summaries.py

@@ -5,15 +5,17 @@
 from caselawclient.errors import DocumentNotFoundError
 from caselawclient.factories import JudgmentFactory
 from caselawclient.models.documents import DocumentURIString
+from caselawclient.models.identifiers.neutral_citation import NeutralCitationNumber
 from caselawclient.models.neutral_citation_mixin import NeutralCitationString
 from caselawclient.models.press_summaries import PressSummary
 
 
 class TestPressSummary:
     def test_best_identifier(self, mock_api_client):
         summary = PressSummary(DocumentURIString("test/1234"), mock_api_client)
-        summary.neutral_citation = NeutralCitationString("[2023] TEST 1234")
-        assert summary.best_human_identifier == summary.neutral_citation
+        document_ncn = NeutralCitationNumber(value="[2023] TEST 1234")
+        summary.identifiers.add(document_ncn)
+        assert summary.best_human_identifier == document_ncn
 
 
 class TestPressSummaryValidation: