Fmd 584 contact redesign (#633)

* new design for contacts template

* add details function to test if access reqs are a url

* add a selenium test to check access requirement contact logic

* add unit tests for new details functions

* update copy file for new contact design

* update test workflow to compile messages

* change workflow compile messages command

* add step to install gettext to workflow

* try just the install

* fix other selenium tests to align to copy file

* one more test...

* better name for is_access_requirements... for context key

* and update the tests with new key

.github/workflows/reusable-tests.yml

@@ -73,6 +73,12 @@ jobs:
       - name: Install project
         run: poetry install --no-interaction --no-root
 
+      - name: Install compile messages prereqs
+        run: sudo apt-get install gettext
+
+      - name: Compile messages
+        run: make compile_messages
+
       - name: Setup Node.js
         uses: actions/setup-node@v4
         with:

home/service/details.py

@@ -3,13 +3,14 @@
 
 from data_platform_catalogue.entities import EntityRef, RelationshipType
 from data_platform_catalogue.search_types import ResultType
-from django.core.exceptions import ObjectDoesNotExist
+from django.core.exceptions import ObjectDoesNotExist, ValidationError
+from django.core.validators import URLValidator
 from django.utils.translation import gettext as _
 
 from .base import GenericService
 
 
-def _parse_parent(relationships) -> EntityRef | None:
+def _parse_parent(relationships: dict) -> EntityRef | None:
     """
     returns the EntityRef of the first parent if one exists
     """
@@ -24,6 +25,21 @@ def _parse_parent(relationships) -> EntityRef | None:
     return parent_entity
 
 
+def is_access_requirements_a_url(access_requirements) -> bool:
+    """
+    return a bool indicating if the passed access_requirements arg is a url
+    """
+    validator = URLValidator()
+
+    try:
+        validator(access_requirements)
+        is_url = True
+    except ValidationError:
+        is_url = False
+
+    return is_url
+
+
 class DatabaseDetailsService(GenericService):
     def __init__(self, urn: str):
         self.urn = urn
@@ -53,6 +69,9 @@ def _get_context(self):
             ),
             "h1_value": self.database_metadata.name,
             "is_esda": self.is_esda,
+            "is_access_requirements_a_url": is_access_requirements_a_url(
+                self.database_metadata.custom_properties.access_information.access_requirements
+            ),
         }
 
         return context
@@ -88,6 +107,9 @@ def _get_context(self):
             "h1_value": self.table_metadata.name,
             "has_lineage": self.has_lineage(),
             "lineage_url": f"{split_datahub_url.scheme}://{split_datahub_url.netloc}/dataset/{self.table_metadata.urn}/Lineage?is_lineage_mode=true&",  # noqa: E501
+            "is_access_requirements_a_url": is_access_requirements_a_url(
+                self.table_metadata.custom_properties.access_information.access_requirements
+            ),
         }
 
     def has_lineage(self) -> bool:
@@ -118,6 +140,9 @@ def _get_context(self):
             "parent_entity": self.parent_entity,
             "parent_type": ResultType.DASHBOARD.name.lower(),
             "h1_value": self.chart_metadata.name,
+            "is_access_requirements_a_url": is_access_requirements_a_url(
+                self.chart_metadata.custom_properties.access_information.access_requirements
+            ),
         }
 
 
@@ -138,4 +163,7 @@ def _get_context(self):
                 self.children,
                 key=lambda d: d.entity_ref.display_name,
             ),
+            "is_access_requirements_a_url": is_access_requirements_a_url(
+                self.dashboard_metadata.custom_properties.access_information.access_requirements
+            ),
         }

locale/en/LC_MESSAGES/django.po

@@ -353,29 +353,34 @@ msgstr ""
 "intended to be narrower in scope than an entire agency."
 
 # Heading
-#: templates/partial/contact_info.html:4
+#: templates/partial/contact_info.html:29
 msgid "IAO or Data Owner"
-msgstr "IAO or Data Owner"
+msgstr "Data owner"
 
 # Heading
 #: templates/partial/contact_info.html:10
 msgid "Contact email for access requests"
 msgstr "Contact email for access requests"
 
 # Heading
-#: templates/partial/contact_info.html:18
-msgid "Contact channels for access requests"
-msgstr "Contact channels for access requests"
+#: templates/partial/contact_info.html:21
+msgid "Contact channels for questions"
+msgstr "Contact channels for questions"
 
 # Heading
-#: templates/partial/contact_info.html:26
+#: templates/partial/contact_info.html:4
 msgid "Access requirements"
-msgstr "Access requirements"
+msgstr "Request access"
+
+# Request access link
+#: templates/partial/contact_info.html:8
+msgid "Click link for access information (opens in new tab)"
+msgstr "Click link for access information (opens in new tab)"
 
 # Default text for contact info
-#: templates/partial/contact_info.html:31
+#: templates/partial/contact_info.html:13
 msgid "Processing the data might require permission from IAO or Data owner."
-msgstr "Processing the data might require permission from IAO or Data owner."
+msgstr "Processing the data might require permission from the Data owner."
 
 # Contact us link on error pages
 #: templates/partial/contact_team.html:4

templates/details_base.html

@@ -90,7 +90,7 @@ <h3 class="govuk-heading-s govuk-!-margin-top-3">
       </div>
     </div>
     <div class="govuk-grid-column-one-third">
-      {% include "partial/contact_info.html" with data_owner=entity.governance.data_owner.display_name data_owner_email=entity.governance.data_owner.email slack_channel=entity.custom_properties.further_information access_requirements=entity.custom_properties.access_information.access_requirements%}
+      {% include "partial/contact_info.html" with data_owner=entity.governance.data_owner.display_name data_owner_email=entity.governance.data_owner.email slack_channel=entity.custom_properties.further_information access_requirements=entity.custom_properties.access_information.access_requirements is_access_url=is_access_requirements_a_url%}
     </div>
   </div>
 

templates/partial/contact_info.html

@@ -1,34 +1,33 @@
 {% load i18n %}
 
 <div class="govuk-body">
-  <h2 class="govuk-heading-s govuk-!-margin-bottom-1">{% translate "IAO or Data Owner" %}</h2>
-  <p class="govuk-body">
-    {{ data_owner }}
-  </p>
-</div>
-<div class="govuk-body">
-  <h2 class="govuk-heading-s govuk-!-margin-bottom-1">{% translate "Contact email for access requests" %}</h2>
+  <h2 class="govuk-heading-s govuk-!-margin-bottom-1">{% translate "Access requirements" %}</h2>
   <p class="govuk-body">
-    {{ data_owner_email|urlize }}
+    {% if access_requirements %}
+      {% if is_access_url %}
+        <a id="request-access" href="{{ access_requirements }}" class="govuk-link" rel="noreferrer noopener" target="_blank">{% translate "Click link for access information (opens in new tab)" %}</a><br>
+      {% else %}
+        <p id="request-access">{{ access_requirements }} </p>
+      {% endif %}
+    {% else %}
+      <p id="request-access"> {% translate "Processing the data might require permission from IAO or Data owner." %} </p>
+
+    {% endif %}
   </p>
 </div>
 <!-- placeholder until we have more contact channels than just slack -->
 {% if slack_channel.dc_slack_channel_url %}
   <div class="govuk-body">
-    <h2 class="govuk-heading-s govuk-!-margin-bottom-1">{% translate "Contact channels for access requests" %}</h2>
+    <h2 class="govuk-heading-s govuk-!-margin-bottom-1">{% translate "Contact channels for questions" %}</h2>
     <p class="govuk-body">
         <!-- This should become a list of populated contact channels -->
-      Slack channel: <a href="{{ slack_channel.dc_slack_channel_url }}" target="_blank">{{ slack_channel.dc_slack_channel_name }} (opens in new tab)</a>
+      Slack channel: <a href="{{ slack_channel.dc_slack_channel_url }}" class="govuk-link" rel="noreferrer noopener" target="_blank">{{ slack_channel.dc_slack_channel_name }} (opens in new tab)</a>
     </p>
   </div>
 {% endif %}
 <div class="govuk-body">
-  <h2 class="govuk-heading-s govuk-!-margin-bottom-1">{% translate "Access requirements" %}</h2>
+  <h2 class="govuk-heading-s govuk-!-margin-bottom-1">{% translate "IAO or Data Owner" %}</h2>
   <p class="govuk-body">
-    {% if access_requirements %}
-      {{ access_requirements }}
-    {% else %}
-    {% translate "Processing the data might require permission from IAO or Data owner." %}
-    {% endif %}
+    {{ data_owner_email|urlize }}
   </p>
 </div>

tests/home/service/test_details.py

@@ -1,3 +1,4 @@
+import pytest
 from data_platform_catalogue.entities import (
     AccessInformation,
     Chart,
@@ -18,6 +19,8 @@
     DashboardDetailsService,
     DatabaseDetailsService,
     DatasetDetailsService,
+    _parse_parent,
+    is_access_requirements_a_url,
 )
 from tests.conftest import (
     generate_dashboard_metadata,
@@ -26,6 +29,63 @@
 )
 
 
+@pytest.mark.parametrize(
+    "input, expected_output",
+    [
+        (
+            {
+                RelationshipType.PARENT: [
+                    EntitySummary(
+                        entity_ref=EntityRef(urn="urn:li:db", display_name="db"),
+                        description="test",
+                        entity_type="database",
+                        tags=[],
+                    )
+                ]
+            },
+            EntityRef(urn="urn:li:db", display_name="db"),
+        ),
+        ({}, None),
+        (
+            {
+                RelationshipType.DATA_LINEAGE: [
+                    EntitySummary(
+                        entity_ref=EntityRef(urn="urn:li:db", display_name="db"),
+                        description="test",
+                        entity_type="database",
+                        tags=[],
+                    )
+                ]
+            },
+            None,
+        ),
+    ],
+)
+def test_parse_parent(input, expected_output):
+    result = _parse_parent(input)
+    assert result == expected_output
+
+
+@pytest.mark.parametrize(
+    "input, expected_output",
+    [
+        ("122", False),
+        ("https://test.gov.uk", True),
+        ("https://test.gov.uk/data/#readme", True),
+        ("http://test.co.uk", True),
+        ("ftp.example.com/how-to-access.txt", False),
+        ("Just some instructions", False),
+        ("", False),
+        (123, False),
+        (None, False),
+        (["https://test.gov.uk"], False),
+    ],
+)
+def test_is_access_requirements_a_url(input, expected_output):
+    result = is_access_requirements_a_url(input)
+    assert result == expected_output
+
+
 class TestDatasetDetailsService:
     def test_get_context_contains_table_metadata(self, dataset_with_parent):
         service = DatasetDetailsService(dataset_with_parent["urn"])
@@ -156,6 +216,7 @@ def test_get_context(self, mock_catalogue):
             "parent_entity": None,
             "parent_type": "dashboard",
             "h1_value": "test",
+            "is_access_requirements_a_url": False,
         }
 
         assert context == expected

tests/selenium/conftest.py

@@ -84,6 +84,9 @@ def database_tables(self):
     def table_link(self):
         return self.selenium.find_element(By.LINK_TEXT, "Table details")
 
+    def request_access(self):
+        return self.selenium.find_element(By.ID, "request-access")
+
 
 class TableDetailsPage(Page):
     def column_descriptions(self):

tests/selenium/test_details_contact_contents.py

@@ -0,0 +1,79 @@
+import pytest
+from data_platform_catalogue.entities import AccessInformation, CustomEntityProperties
+
+from tests.conftest import (
+    generate_database_metadata,
+    mock_get_database_details_response,
+)
+
+
+@pytest.mark.slow
+class TestDetailsPageContactDetails:
+    """
+    Given I am in a details page
+    When I view the content of the contact information
+    Then I should be presented with corrently formated information. A link
+    if the request access section is a url, the given free text or if nothing
+    given the default text for request access.
+    """
+
+    @pytest.fixture(autouse=True)
+    def setup(
+        self,
+        live_server,
+        selenium,
+        details_database_page,
+    ):
+        self.selenium = selenium
+        self.live_server_url = live_server.url
+        self.details_database_page = details_database_page
+
+    @pytest.mark.parametrize(
+        "access_reqs, expected_text, expected_tag",
+        [
+            (
+                "https://place-to-get-your-access.com",
+                "Click link for access information (opens in new tab)",
+                "a",
+            ),
+            (
+                "To access these data you need to seek permission from the data owner by email",
+                "To access these data you need to seek permission from the data owner by email",
+                "p",
+            ),
+            (
+                "",
+                "Processing the data might require permission from the Data owner.",
+                "p",
+            ),
+        ],
+    )
+    def test_access_requirements_content(
+        self, mock_catalogue, access_reqs, expected_text, expected_tag
+    ):
+        """
+        test that what is displayed in the request action section of contacts is what we expect
+        e.g.
+        1 - a sole link given is rendered as a hyperlink with standard link text
+        2 - some other specific free text held in the access_requirements custom property is
+        shown as given
+        3 - where no access_requirements custom property exists default to the standrd line
+        """
+        test_database = generate_database_metadata(
+            custom_properties=CustomEntityProperties(
+                access_information=AccessInformation(access_requirements=access_reqs)
+            )
+        )
+        mock_get_database_details_response(mock_catalogue, test_database)
+
+        self.start_on_the_details_page()
+
+        request_access_metadata = self.details_database_page.request_access()
+
+        assert request_access_metadata.text == expected_text
+        assert request_access_metadata.tag_name == expected_tag
+
+    def start_on_the_details_page(self):
+        self.selenium.get(
+            f"{self.live_server_url}/details/database/urn:li:container:test"
+        )

tests/selenium/test_primary_nav.py

@@ -37,7 +37,7 @@ def start_on_the_home_page(self):
         assert self.selenium.title in self.page_titles
         heading_text = self.home_page.primary_heading().text
 
-        assert heading_text == "Find MOJ data"
+        assert heading_text == "Find MoJ data"
         assert self.selenium.title.split("-")[0].strip() == "Home"
 
     def click_on_the_glossary_link(self):

tests/selenium/test_search_and_browse_from_homepage.py

@@ -62,7 +62,7 @@ def start_on_the_home_page(self):
         assert self.selenium.title in self.page_titles
         heading_text = self.home_page.primary_heading().text
 
-        assert heading_text == "Find MOJ data"
+        assert heading_text == "Find MoJ data"
         assert self.selenium.title.split("-")[0].strip() == "Home"
 
     def verify_i_am_on_the_search_page(self):