diff --git a/.github/workflows/reusable-tests.yml b/.github/workflows/reusable-tests.yml index 6ca3675e..3e813274 100644 --- a/.github/workflows/reusable-tests.yml +++ b/.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: diff --git a/home/service/details.py b/home/service/details.py index 05ab1735..8f7933ee 100644 --- a/home/service/details.py +++ b/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 + ), } diff --git a/locale/en/LC_MESSAGES/django.po b/locale/en/LC_MESSAGES/django.po index c38af4e9..657ef797 100644 --- a/locale/en/LC_MESSAGES/django.po +++ b/locale/en/LC_MESSAGES/django.po @@ -353,9 +353,9 @@ 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 @@ -363,19 +363,24 @@ 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 diff --git a/templates/details_base.html b/templates/details_base.html index 8500cdce..711f1eee 100644 --- a/templates/details_base.html +++ b/templates/details_base.html @@ -90,7 +90,7 @@

- {% 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%}
diff --git a/templates/partial/contact_info.html b/templates/partial/contact_info.html index 0cd4b8bb..f296d311 100644 --- a/templates/partial/contact_info.html +++ b/templates/partial/contact_info.html @@ -1,34 +1,33 @@ {% load i18n %}
-

{% translate "IAO or Data Owner" %}

-

- {{ data_owner }} -

-
-
-

{% translate "Contact email for access requests" %}

+

{% translate "Access requirements" %}

- {{ data_owner_email|urlize }} + {% if access_requirements %} + {% if is_access_url %} + {% translate "Click link for access information (opens in new tab)" %}
+ {% else %} +

{{ access_requirements }}

+ {% endif %} + {% else %} +

{% translate "Processing the data might require permission from IAO or Data owner." %}

+ + {% endif %}

{% if slack_channel.dc_slack_channel_url %}
-

{% translate "Contact channels for access requests" %}

+

{% translate "Contact channels for questions" %}

- Slack channel: {{ slack_channel.dc_slack_channel_name }} (opens in new tab) + Slack channel: {{ slack_channel.dc_slack_channel_name }} (opens in new tab)

{% endif %}
-

{% translate "Access requirements" %}

+

{% translate "IAO or Data Owner" %}

- {% if access_requirements %} - {{ access_requirements }} - {% else %} - {% translate "Processing the data might require permission from IAO or Data owner." %} - {% endif %} + {{ data_owner_email|urlize }}

diff --git a/tests/home/service/test_details.py b/tests/home/service/test_details.py index b22d0745..33486935 100644 --- a/tests/home/service/test_details.py +++ b/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 diff --git a/tests/selenium/conftest.py b/tests/selenium/conftest.py index ebaa89e1..4ee88b08 100644 --- a/tests/selenium/conftest.py +++ b/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): diff --git a/tests/selenium/test_details_contact_contents.py b/tests/selenium/test_details_contact_contents.py new file mode 100644 index 00000000..9e7c8a22 --- /dev/null +++ b/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" + ) diff --git a/tests/selenium/test_primary_nav.py b/tests/selenium/test_primary_nav.py index 46fefb2c..62439e4a 100644 --- a/tests/selenium/test_primary_nav.py +++ b/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): diff --git a/tests/selenium/test_search_and_browse_from_homepage.py b/tests/selenium/test_search_and_browse_from_homepage.py index 5d2ad827..1bc8ba4e 100644 --- a/tests/selenium/test_search_and_browse_from_homepage.py +++ b/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):