From 98dd91056005542b38daad3cbb2cfa14d967eea4 Mon Sep 17 00:00:00 2001 From: LavMatt Date: Mon, 5 Aug 2024 20:27:59 +0100 Subject: [PATCH 01/13] new design for contacts template --- templates/details_base.html | 2 +- templates/partial/contact_info.html | 31 ++++++++++++++--------------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/templates/details_base.html b/templates/details_base.html index 8500cdce..1a48ccea 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_url%}
diff --git a/templates/partial/contact_info.html b/templates/partial/contact_info.html index 0cd4b8bb..7b3eabf7 100644 --- a/templates/partial/contact_info.html +++ b/templates/partial/contact_info.html @@ -1,34 +1,33 @@ {% load i18n %}
-

{% translate "IAO or Data Owner" %}

+

Request access

- {{ data_owner }} -

-
-
-

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

-

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

{{ access_requirements }}

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

Processing the data might require permission from the Data owner.

+ + {% endif %}

{% if slack_channel.dc_slack_channel_url %}
-

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

+

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" %}

+

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 }}

From 7c493ecd0821275d7219af041b9ecdc1bcea449c Mon Sep 17 00:00:00 2001 From: LavMatt Date: Mon, 5 Aug 2024 20:28:45 +0100 Subject: [PATCH 02/13] add details function to test if access reqs are a url --- home/service/details.py | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/home/service/details.py b/home/service/details.py index 05ab1735..6c978aee 100644 --- a/home/service/details.py +++ b/home/service/details.py @@ -3,7 +3,8 @@ 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 @@ -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_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_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_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_url": is_access_requirements_a_url( + self.dashboard_metadata.custom_properties.access_information.access_requirements + ), } From 3624191bbf7d62026b3cbeef4c8cc87dc00d9052 Mon Sep 17 00:00:00 2001 From: LavMatt Date: Mon, 5 Aug 2024 20:31:02 +0100 Subject: [PATCH 03/13] add a selenium test to check access requirement contact logic --- tests/selenium/conftest.py | 3 + .../selenium/test_details_contact_contents.py | 79 +++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 tests/selenium/test_details_contact_contents.py 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" + ) From fb2eb7fa392980df6978f2b7a709a666914de700 Mon Sep 17 00:00:00 2001 From: LavMatt Date: Tue, 6 Aug 2024 09:07:10 +0100 Subject: [PATCH 04/13] add unit tests for new details functions --- home/service/details.py | 2 +- tests/home/service/test_details.py | 61 ++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/home/service/details.py b/home/service/details.py index 6c978aee..77733885 100644 --- a/home/service/details.py +++ b/home/service/details.py @@ -10,7 +10,7 @@ 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 """ diff --git a/tests/home/service/test_details.py b/tests/home/service/test_details.py index b22d0745..440a60a2 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_url": False, } assert context == expected From 461fe571670f89ce71be69c041cea9dea57aec5a Mon Sep 17 00:00:00 2001 From: LavMatt Date: Tue, 6 Aug 2024 15:53:30 +0100 Subject: [PATCH 05/13] update copy file for new contact design --- locale/en/LC_MESSAGES/django.po | 23 ++++++++++++++--------- templates/partial/contact_info.html | 10 +++++----- 2 files changed, 19 insertions(+), 14 deletions(-) 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/partial/contact_info.html b/templates/partial/contact_info.html index 7b3eabf7..f296d311 100644 --- a/templates/partial/contact_info.html +++ b/templates/partial/contact_info.html @@ -1,16 +1,16 @@ {% load i18n %}
-

Request access

+

{% translate "Access requirements" %}

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

{{ access_requirements }}

{% endif %} {% else %} -

Processing the data might require permission from the Data owner.

+

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

{% endif %}

@@ -18,7 +18,7 @@

Request access

{% if slack_channel.dc_slack_channel_url %}
-

Contact channels for questions

+

{% translate "Contact channels for questions" %}

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

Contact channels for questio

{% endif %}
-

Data owner

+

{% translate "IAO or Data Owner" %}

{{ data_owner_email|urlize }}

From dfe3d23d0d1a68372724f08e683481512dcce1f7 Mon Sep 17 00:00:00 2001 From: LavMatt Date: Tue, 6 Aug 2024 16:12:19 +0100 Subject: [PATCH 06/13] update test workflow to compile messages --- .github/workflows/reusable-tests.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/reusable-tests.yml b/.github/workflows/reusable-tests.yml index 6ca3675e..23bc114b 100644 --- a/.github/workflows/reusable-tests.yml +++ b/.github/workflows/reusable-tests.yml @@ -73,6 +73,9 @@ jobs: - name: Install project run: poetry install --no-interaction --no-root + - name: compile messages + run: make compile_messages + - name: Setup Node.js uses: actions/setup-node@v4 with: From 95ba0676a222539942bcbd66138cecbda4d5b9eb Mon Sep 17 00:00:00 2001 From: LavMatt Date: Tue, 6 Aug 2024 16:14:46 +0100 Subject: [PATCH 07/13] change workflow compile messages command --- .github/workflows/reusable-tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/reusable-tests.yml b/.github/workflows/reusable-tests.yml index 23bc114b..e6b446dd 100644 --- a/.github/workflows/reusable-tests.yml +++ b/.github/workflows/reusable-tests.yml @@ -74,7 +74,7 @@ jobs: run: poetry install --no-interaction --no-root - name: compile messages - run: make compile_messages + run: poetry run python manage.py compilemessages - name: Setup Node.js uses: actions/setup-node@v4 From 17a9bc29d91cb32f6d92e954697652b42bb9dd71 Mon Sep 17 00:00:00 2001 From: LavMatt Date: Tue, 6 Aug 2024 16:19:59 +0100 Subject: [PATCH 08/13] add step to install gettext to workflow --- .github/workflows/reusable-tests.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/reusable-tests.yml b/.github/workflows/reusable-tests.yml index e6b446dd..498206fd 100644 --- a/.github/workflows/reusable-tests.yml +++ b/.github/workflows/reusable-tests.yml @@ -73,8 +73,13 @@ jobs: - name: Install project run: poetry install --no-interaction --no-root - - name: compile messages - run: poetry run python manage.py compilemessages + - name: Install compile messages reqs + run: | + apt-get update + sudo apt-get install gettext + + - name: Compile messages + run: make compile_messages - name: Setup Node.js uses: actions/setup-node@v4 From 8beb0fa76a17a5ffe85679e33b9f58f785b987a9 Mon Sep 17 00:00:00 2001 From: LavMatt Date: Tue, 6 Aug 2024 16:23:36 +0100 Subject: [PATCH 09/13] try just the install --- .github/workflows/reusable-tests.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/workflows/reusable-tests.yml b/.github/workflows/reusable-tests.yml index 498206fd..3e813274 100644 --- a/.github/workflows/reusable-tests.yml +++ b/.github/workflows/reusable-tests.yml @@ -73,10 +73,8 @@ jobs: - name: Install project run: poetry install --no-interaction --no-root - - name: Install compile messages reqs - run: | - apt-get update - sudo apt-get install gettext + - name: Install compile messages prereqs + run: sudo apt-get install gettext - name: Compile messages run: make compile_messages From 221b8abd8776a554b28e1cc392aac8033a0880bb Mon Sep 17 00:00:00 2001 From: LavMatt Date: Tue, 6 Aug 2024 16:26:19 +0100 Subject: [PATCH 10/13] fix other selenium tests to align to copy file --- tests/selenium/test_search_and_browse_from_homepage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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): From 47aafe18544f9a59469648029b0cd708e80a25f3 Mon Sep 17 00:00:00 2001 From: LavMatt Date: Tue, 6 Aug 2024 16:30:26 +0100 Subject: [PATCH 11/13] one more test... --- tests/selenium/test_primary_nav.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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): From 46f54b8ed27c9c50e0feba85c6ccb56bfc90d93a Mon Sep 17 00:00:00 2001 From: LavMatt Date: Tue, 6 Aug 2024 17:22:58 +0100 Subject: [PATCH 12/13] better name for is_access_requirements... for context key --- home/service/details.py | 8 ++++---- templates/details_base.html | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/home/service/details.py b/home/service/details.py index 77733885..8f7933ee 100644 --- a/home/service/details.py +++ b/home/service/details.py @@ -69,7 +69,7 @@ def _get_context(self): ), "h1_value": self.database_metadata.name, "is_esda": self.is_esda, - "is_access_requirements_url": is_access_requirements_a_url( + "is_access_requirements_a_url": is_access_requirements_a_url( self.database_metadata.custom_properties.access_information.access_requirements ), } @@ -107,7 +107,7 @@ 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_url": is_access_requirements_a_url( + "is_access_requirements_a_url": is_access_requirements_a_url( self.table_metadata.custom_properties.access_information.access_requirements ), } @@ -140,7 +140,7 @@ def _get_context(self): "parent_entity": self.parent_entity, "parent_type": ResultType.DASHBOARD.name.lower(), "h1_value": self.chart_metadata.name, - "is_access_requirements_url": is_access_requirements_a_url( + "is_access_requirements_a_url": is_access_requirements_a_url( self.chart_metadata.custom_properties.access_information.access_requirements ), } @@ -163,7 +163,7 @@ def _get_context(self): self.children, key=lambda d: d.entity_ref.display_name, ), - "is_access_requirements_url": is_access_requirements_a_url( + "is_access_requirements_a_url": is_access_requirements_a_url( self.dashboard_metadata.custom_properties.access_information.access_requirements ), } diff --git a/templates/details_base.html b/templates/details_base.html index 1a48ccea..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 is_access_url=is_access_requirements_url%} + {% 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%}
From 6e3e2903327c9421d16b0919f1a7242e0f8e4cf3 Mon Sep 17 00:00:00 2001 From: LavMatt Date: Tue, 6 Aug 2024 17:51:20 +0100 Subject: [PATCH 13/13] and update the tests with new key --- tests/home/service/test_details.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/home/service/test_details.py b/tests/home/service/test_details.py index 440a60a2..33486935 100644 --- a/tests/home/service/test_details.py +++ b/tests/home/service/test_details.py @@ -216,7 +216,7 @@ def test_get_context(self, mock_catalogue): "parent_entity": None, "parent_type": "dashboard", "h1_value": "test", - "is_access_requirements_url": False, + "is_access_requirements_a_url": False, } assert context == expected