From f445be34b91325e28db6a2ae7f18f17deb5e84db Mon Sep 17 00:00:00 2001 From: Chris Hopkins Date: Fri, 13 Dec 2024 09:21:25 +0000 Subject: [PATCH 1/4] remove logic to read and update the consent service for a contact --- datahub/company/serializers.py | 72 +------ datahub/company/tasks/__init__.py | 2 - datahub/company/tasks/contact.py | 57 ----- .../test_contact_detail_serializer.py | 201 +----------------- .../company/test/tasks/test_contact_task.py | 178 ---------------- datahub/company/test/test_contact_views.py | 106 --------- datahub/company/views.py | 21 +- 7 files changed, 13 insertions(+), 624 deletions(-) diff --git a/datahub/company/serializers.py b/datahub/company/serializers.py index 322397b13..40c6b6767 100644 --- a/datahub/company/serializers.py +++ b/datahub/company/serializers.py @@ -6,7 +6,6 @@ from django.conf import settings from django.db import models, transaction -from django.utils.timezone import now from django.utils.translation import gettext_lazy from rest_framework import serializers @@ -30,13 +29,12 @@ OneListCoreTeamMember, OneListTier, ) -from datahub.company.tasks.contact import schedule_update_contact_consent + from datahub.company.validators import ( has_no_invalid_company_number_characters, has_uk_establishment_number_prefix, validate_team_member_max_count, ) -from datahub.core.api_client import get_zipkin_headers from datahub.core.constants import Country from datahub.core.constants import HeadquarterType from datahub.core.serializers import ( @@ -300,74 +298,6 @@ def to_representation(self, value): return representation -class ContactDetailSerializer(ContactSerializer): - """ - This is the same as the ContactSerializer except it includes - accepts_dit_email_marketing in the fields. Only 3 endpoints will use this serialiser - """ - - accepts_dit_email_marketing = ConsentMarketingField(source='*', required=False) - - class Meta(ContactSerializer.Meta): - fields = ContactSerializer.Meta.fields + ('accepts_dit_email_marketing',) - - def _notify_consent_service(self, validated_data): - """ - Trigger the update_contact_consent task with the current version - of `validated_data`. The actual enqueuing of the task happens in the - on_commit hook so it won't actually notify the consent service unless - the database transaction was successful. - """ - if 'accepts_dit_email_marketing' not in validated_data: - # If no consent value in request body - return - # Remove the accepts_dit_email_marketing from validated_data - accepts_dit_email_marketing = validated_data.pop('accepts_dit_email_marketing') - # If consent value in POST, notify - combiner = DataCombiner(self.instance, validated_data) - request = self.context.get('request', None) - transaction.on_commit( - lambda: schedule_update_contact_consent( - combiner.get_value('email'), - accepts_dit_email_marketing, - kwargs={ - 'modified_at': now().isoformat(), - 'zipkin_headers': get_zipkin_headers(request), - }, - ), - ) - - @transaction.atomic - def create(self, validated_data): - """ - Create a new instance using this serializer - - :return: created instance - """ - self._notify_consent_service(validated_data) - return super().create(validated_data) - - @transaction.atomic - def update(self, instance, validated_data): - """ - Update the given instance with validated_data - - :return: updated instance - """ - self._notify_consent_service(validated_data) - return super().update(instance, validated_data) - - -class ContactDetailV4Serializer(ContactV4Serializer, ContactDetailSerializer): - """ - This is the same as the ContactSerializer except it includes - accepts_dit_email_marketing in the fields. - """ - - class Meta(ContactV4Serializer.Meta): - fields = ContactV4Serializer.Meta.fields + ('accepts_dit_email_marketing',) - - class CompanyExportCountrySerializer(serializers.ModelSerializer): """ Export country serializer holding `Country` and its status. diff --git a/datahub/company/tasks/__init__.py b/datahub/company/tasks/__init__.py index 3d8a76ae6..8df0f8c5d 100644 --- a/datahub/company/tasks/__init__.py +++ b/datahub/company/tasks/__init__.py @@ -2,11 +2,9 @@ from datahub.company.tasks.contact import ( automatic_contact_archive, - update_contact_consent, ) __all__ = ( 'automatic_adviser_deactivate', 'automatic_contact_archive', - 'update_contact_consent', ) diff --git a/datahub/company/tasks/contact.py b/datahub/company/tasks/contact.py index f85ed468e..3cf6ad152 100644 --- a/datahub/company/tasks/contact.py +++ b/datahub/company/tasks/contact.py @@ -5,22 +5,17 @@ from typing import List import environ -import requests from dateutil import parser from django.conf import settings -from django.core.exceptions import ImproperlyConfigured from django.utils import timezone from django_pglocks import advisory_lock from smart_open import open -from datahub.company import consent from datahub.company.models import Contact from datahub.core.boto3_client import get_s3_client -from datahub.core.exceptions import APIBadGatewayException from datahub.core.queues.constants import HALF_DAY_IN_SECONDS -from datahub.core.queues.errors import RetryError from datahub.core.queues.job_scheduler import job_scheduler from datahub.core.queues.scheduler import LONG_RUNNING_QUEUE from datahub.core.realtime_messaging import send_realtime_message @@ -59,58 +54,6 @@ def _automatic_contact_archive(limit=1000, simulate=False): return contacts_to_be_archived.count() -def schedule_update_contact_consent( - email_address, - accepts_dit_email_marketing, - modified_at=None, - **kwargs, -): - job = job_scheduler( - function=update_contact_consent, - function_args=( - email_address, - accepts_dit_email_marketing, - modified_at, - ), - function_kwargs=kwargs, - max_retries=5, - retry_backoff=30, - ) - logger.info( - f'Task {job.id} update_contact_consent', - ) - return job - - -def update_contact_consent( - email_address, - accepts_dit_email_marketing, - modified_at=None, - **kwargs, -) -> bool: - """ - Update consent preferences. - """ - try: - consent.update_consent( - email_address, - accepts_dit_email_marketing, - modified_at=modified_at, - **kwargs, - ) - return True - except requests.exceptions.RequestException as request_error: - logger.warning('Retrying updating contact consent') - raise RetryError(request_error) - except (APIBadGatewayException, ImproperlyConfigured, Exception) as exec_info: - logger.warning( - 'Unable to update contact consent', - exc_info=exec_info, - stack_info=True, - ) - return False - - def schedule_automatic_contact_archive(limit=1000, simulate=False): job = job_scheduler( function=automatic_contact_archive, diff --git a/datahub/company/test/serializers/test_contact_detail_serializer.py b/datahub/company/test/serializers/test_contact_detail_serializer.py index bf8862696..5e7f5ffa7 100644 --- a/datahub/company/test/serializers/test_contact_detail_serializer.py +++ b/datahub/company/test/serializers/test_contact_detail_serializer.py @@ -1,210 +1,31 @@ from unittest.mock import Mock import pytest -from django.conf import settings from freezegun import freeze_time -from datahub.company.consent import CONSENT_SERVICE_PERSON_PATH_LOOKUP -from datahub.company.constants import ( - CONSENT_SERVICE_EMAIL_CONSENT_TYPE, -) -from datahub.company.serializers import ContactDetailSerializer, ContactDetailV4Serializer -from datahub.company.test.factories import CompanyFactory, ContactFactory +from datahub.company.serializers import ContactV4Serializer +from datahub.company.test.factories import CompanyFactory from datahub.core import constants -from datahub.core.test_utils import ( - HawkMockJSONResponse, -) # mark the whole module for db use pytestmark = pytest.mark.django_db FROZEN_TIME = '2020-03-13T14:21:24.367265+00:00' -request = Mock(headers={ - 'x-b3-traceid': '123', - 'x-b3-spanid': '456', -}) - - -@pytest.fixture -def update_contact_task_mock(monkeypatch): - mock_schedule_update_contact_consent = Mock() - monkeypatch.setattr( - 'datahub.company.serializers.schedule_update_contact_consent', - mock_schedule_update_contact_consent, - ) - yield mock_schedule_update_contact_consent - - -@freeze_time(FROZEN_TIME) -class ContactSerializerBase: - """ - Tests for the Contact Serializer. Checking that update / create notify the - consent service correctly. - """ - - serializer = None - - def _make_contact(self): - contact = ContactFactory() - return contact - - def test_serializer_update_call_task(self, update_contact_task_mock, synchronous_on_commit): - """ - Ensure that consent service RQ task is called when serializer.update - is called if accepts_dit_email_marketing is True. - """ - contact = self._make_contact() - serialized_contact = self.serializer( - instance=contact, - context={'request': request}, - ) - serialized_contact.update(serialized_contact.instance, { - 'email': 'bar@foo.com', - 'accepts_dit_email_marketing': True, - }) - update_contact_task_mock.assert_called_once_with( - 'bar@foo.com', - True, - kwargs={ - 'modified_at': FROZEN_TIME, - 'zipkin_headers': request.headers, - }, - ) - - def test_serializer_update_partial_call_task( - self, - update_contact_task_mock, - synchronous_on_commit, - ): - """ - Ensure that consent service RQ task is called when serializer.update - is called with partial data if accepts_dit_email_marketing is True. - """ - contact = self._make_contact() - serialized_contact = self.serializer(instance=contact, partial=True) - serialized_contact.update(serialized_contact.instance, { - 'accepts_dit_email_marketing': True, - }) - update_contact_task_mock.assert_called_once_with( - serialized_contact.instance.email, - True, - kwargs={'modified_at': FROZEN_TIME, 'zipkin_headers': {}}, - ) - - def test_serializer_update_partial_not_call_task( - self, - update_contact_task_mock, - synchronous_on_commit, - ): - """ - Ensure that consent service RQ task is not called when serializer.update - is called with partial data but `accepts_dit_email_marketing` is missing. - """ - contact = self._make_contact() - serialized_contact = self.serializer(instance=contact, partial=True) - data = { - 'last_name': 'Nelson1', - } - serialized_contact.update(serialized_contact.instance, data) - - assert not update_contact_task_mock.called - - def test_serializer_create_calls_task(self, update_contact_task_mock, synchronous_on_commit): - """ - Ensure that consent service RQ task is called when serializer.create - is called. - """ - company = CompanyFactory() - data = { - 'title': { - 'id': constants.Title.admiral_of_the_fleet.value.id, - }, - 'first_name': 'Oratio', - 'last_name': 'Nelson', - 'job_title': 'Head of Sales', - 'company': { - 'id': str(company.pk), - }, - 'email': 'foo@bar.com', - 'primary': True, - 'full_telephone_number': '+44123456789', - 'address_same_as_company': False, - 'address_1': 'Foo st.', - 'address_2': 'adr 2', - 'address_town': 'London', - 'address_county': 'London', - 'address_country': { - 'id': constants.Country.united_kingdom.value.id, - }, - 'address_postcode': 'SW1A1AA', - 'notes': 'lorem ipsum', - 'accepts_dit_email_marketing': True, - } - serialized_contact = self.serializer(data=data, context={'request': request}) - serialized_contact.is_valid(raise_exception=True) - serialized_contact.create(serialized_contact.validated_data) - update_contact_task_mock.assert_called_once_with( - data['email'], - data['accepts_dit_email_marketing'], - kwargs={ - 'modified_at': FROZEN_TIME, - 'zipkin_headers': request.headers, - }, - ) - - @pytest.mark.parametrize('accepts_marketing', (True, False)) - def test_marketing_field_populated_by_consent_service( - self, - requests_mock, - accepts_marketing, - ): - """ - Test accepts_dit_email_marketing field is populated by the consent service. - """ - contact = self._make_contact() - hawk_response = HawkMockJSONResponse( - api_id=settings.COMPANY_MATCHING_HAWK_ID, - api_key=settings.COMPANY_MATCHING_HAWK_KEY, - response={ - 'results': [{ - 'email': contact.email, - 'consents': [ - CONSENT_SERVICE_EMAIL_CONSENT_TYPE, - ] if accepts_marketing else [], - }], - }, - ) - requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' - f'{CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - status_code=200, - text=hawk_response, - ) - contact_serialized = self.serializer(instance=contact) - assert contact_serialized.data['accepts_dit_email_marketing'] is accepts_marketing - assert requests_mock.call_count == 1 - - -@freeze_time(FROZEN_TIME) -class TestContactV3Serializer(ContactSerializerBase): - """ - Tests for the Contact V3 Serializer. Checking that update / create notify the - consent service correctly. - """ - - serializer = ContactDetailSerializer +request = Mock( + headers={ + 'x-b3-traceid': '123', + 'x-b3-spanid': '456', + } +) @freeze_time(FROZEN_TIME) -class TestContactV4Serializer(ContactSerializerBase): +class TestContactV4Serializer: """ - Tests for the Contact V4 Serializer. Checking that update / create notify the - consent service correctly. + Tests for the Contact V4 Serializer. """ - serializer = ContactDetailV4Serializer - @pytest.mark.parametrize( 'country_id, expected_response, is_valid, address_area', ( @@ -273,7 +94,7 @@ def test_area_required_validation_on_respective_countries( }, 'address_area': address_area, } - contact_serializer = ContactDetailV4Serializer(data=data, context={'request': request}) + contact_serializer = ContactV4Serializer(data=data, context={'request': request}) assert contact_serializer.is_valid(raise_exception=False) is is_valid assert len(contact_serializer.errors) == len(expected_response) assert contact_serializer.errors == expected_response diff --git a/datahub/company/test/tasks/test_contact_task.py b/datahub/company/test/tasks/test_contact_task.py index 1343cc644..e98f0f870 100644 --- a/datahub/company/test/tasks/test_contact_task.py +++ b/datahub/company/test/tasks/test_contact_task.py @@ -4,7 +4,6 @@ import uuid from unittest import mock -from unittest.mock import patch import boto3 import pytest @@ -15,13 +14,10 @@ from django.utils import timezone from freezegun import freeze_time from moto import mock_aws -from requests import ConnectTimeout -from rest_framework import status from datahub.company.models.contact import Contact from datahub.company.tasks import ( automatic_contact_archive, - update_contact_consent, ) from datahub.company.tasks.contact import ( BUCKET, @@ -30,10 +26,8 @@ ingest_contact_consent_data, REGION, schedule_automatic_contact_archive, - schedule_update_contact_consent, ) from datahub.company.test.factories import CompanyFactory, ContactFactory -from datahub.core.queues.errors import RetryError from datahub.core.test_utils import HawkMockJSONResponse from datahub.ingest.models import IngestedObject from datahub.ingest.test.factories import IngestedObjectFactory @@ -48,178 +42,6 @@ def generate_hawk_response(payload): ) -@pytest.mark.django_db -class TestConsentServiceTask: - """ - tests for the task that sends email marketing consent status to the - DIT consent service / legal basis API - """ - - @override_settings( - CONSENT_SERVICE_BASE_URL=None, - ) - def test_not_configured_error( - self, - ): - """ - Test that if feature flag is enabled, but environment variables are not set - then task will throw a caught exception and no retries or updates will occur - """ - update_succeeds = update_contact_consent('example@example.com', True) - assert update_succeeds is False - - @pytest.mark.parametrize( - 'email_address, accepts_dit_email_marketing, modified_at', - ( - ('example@example.com', True, None), - ('example@example.com', False, None), - ('example@example.com', True, '2020-01-01-12:00:00Z'), - ('example@example.com', False, '2020-01-01-12:00:00Z'), - ), - ) - def test_task_makes_http_request( - self, - requests_mock, - email_address, - accepts_dit_email_marketing, - modified_at, - ): - """ - Ensure correct http request with correct payload is generated when task - executes. - """ - matcher = requests_mock.post( - '/api/v1/person/', - text=generate_hawk_response({}), - status_code=status.HTTP_201_CREATED, - ) - update_contact_consent( - email_address, - accepts_dit_email_marketing, - modified_at=modified_at, - ) - assert matcher.called_once - expected = { - 'email': email_address, - 'consents': ['email_marketing'] if accepts_dit_email_marketing else [], - } - if modified_at: - expected['modified_at'] = modified_at - - assert matcher.last_request.json() == expected - - @pytest.mark.parametrize( - 'status_code', - ( - (status.HTTP_404_NOT_FOUND), - (status.HTTP_403_FORBIDDEN), - (status.HTTP_500_INTERNAL_SERVER_ERROR), - ), - ) - def test_task_retries_on_request_exceptions( - self, - requests_mock, - status_code, - ): - """ - Test to ensure that rq receives exceptions like 5xx, 404 and then will retry based on - job_scheduler configuration - """ - matcher = requests_mock.post( - '/api/v1/person/', - text=generate_hawk_response({}), - status_code=status_code, - ) - with pytest.raises(RetryError): - update_contact_consent('example@example.com', True) - assert matcher.called_once - - @patch('datahub.company.consent.APIClient.request', side_effect=ConnectTimeout) - def test_task_retries_on_connect_timeout( - self, - mock_post, - ): - """ - Test to ensure that RQ retries on connect timeout by virtue of the exception forcing - a retry within RQ and configured settings - """ - with pytest.raises(RetryError): - update_contact_consent('example@example.com', True) - assert mock_post.called - - @patch('datahub.company.consent.APIClient.request', side_effect=Exception) - def test_task_doesnt_retry_on_other_exception( - self, - mock_post, - ): - """ - Test to ensure that RQ raises on non-requests exception - """ - update_succeeds = update_contact_consent('example@example.com', True) - assert mock_post.called - assert update_succeeds is False - - @pytest.mark.parametrize( - 'status_code', - ( - (status.HTTP_200_OK), - (status.HTTP_201_CREATED), - ), - ) - def test_update_succeeds( - self, - requests_mock, - status_code, - ): - """ - Test success occurs when update succeeds - """ - matcher = requests_mock.post( - '/api/v1/person/', - text=generate_hawk_response({}), - status_code=status_code, - ) - - update_success = update_contact_consent('example@example.com', True) - - assert matcher.called_once - assert update_success is True - - @pytest.mark.parametrize( - 'bad_email', - ( - None, - '', - ' ', - ), - ) - def test_none_or_empty_email_assigned_fails( - self, - requests_mock, - bad_email, - ): - matcher = requests_mock.post( - '/api/v1/person/', - text=generate_hawk_response({}), - status_code=status.HTTP_201_CREATED, - ) - - update_success = update_contact_consent(bad_email, False) - - assert not matcher.called_once - assert update_success is False - - def test_job_schedules_with_correct_update_contact_consent_details(self): - actual_job = schedule_update_contact_consent('example@example.com', True) - - assert actual_job is not None - assert actual_job._func_name == 'datahub.company.tasks.contact.update_contact_consent' - assert actual_job._args == ('example@example.com', True, None) - assert actual_job.retries_left == 5 - assert actual_job.retry_intervals == [30, 961, 1024, 1089, 1156] - assert actual_job.origin == 'short-running' - - @pytest.mark.django_db class TestContactArchiveTask: """ diff --git a/datahub/company/test/test_contact_views.py b/datahub/company/test/test_contact_views.py index 1ee09e7df..12e51c97c 100644 --- a/datahub/company/test/test_contact_views.py +++ b/datahub/company/test/test_contact_views.py @@ -137,7 +137,6 @@ def test_with_manual_address(self, get_consent_fixture): }, 'address_postcode': 'SW1A1AA', 'notes': 'lorem ipsum', - 'accepts_dit_email_marketing': True, 'archived': False, 'archived_by': None, 'archived_documents_url_path': '', @@ -205,7 +204,6 @@ def test_defaults(self): assert not response_data['address_country'] assert not response_data['address_postcode'] assert not response_data['notes'] - assert not response_data['accepts_dit_email_marketing'] def test_fails_with_invalid_email_address(self): """Test that fails if the email address is invalid.""" @@ -408,7 +406,6 @@ def test_with_us_manual_address(self, get_consent_fixture): }, 'address_postcode': 'SW1A1AA', 'notes': 'lorem ipsum', - 'accepts_dit_email_marketing': True, 'valid_email': True, }, ) @@ -452,7 +449,6 @@ def test_with_us_manual_address(self, get_consent_fixture): }, 'address_postcode': 'SW1A1AA', 'notes': 'lorem ipsum', - 'accepts_dit_email_marketing': True, 'archived': False, 'archived_by': None, 'archived_documents_url_path': '', @@ -573,7 +569,6 @@ def test_patch(self): }, 'address_postcode': 'SW1A1AA', 'notes': 'lorem ipsum', - 'accepts_dit_email_marketing': False, 'archived': False, 'archived_by': None, 'archived_documents_url_path': contact.archived_documents_url_path, @@ -771,7 +766,6 @@ def test_patch_area(self): }, 'address_postcode': 'SW1A1AA', 'notes': 'lorem ipsum', - 'accepts_dit_email_marketing': False, 'archived': False, 'archived_by': None, 'archived_documents_url_path': contact.archived_documents_url_path, @@ -961,7 +955,6 @@ def test_view(self): }, 'address_postcode': 'YO22 4JU', 'notes': 'lorem ipsum', - 'accepts_dit_email_marketing': False, 'archived': False, 'archived_by': None, 'archived_documents_url_path': contact.archived_documents_url_path, @@ -989,105 +982,6 @@ def test_get_contact_without_view_document_permission(self): assert response.status_code == status.HTTP_200_OK assert 'archived_documents_url_path' not in response.json() - @pytest.mark.parametrize('accepts_marketing', (True, False)) - def test_accepts_dit_email_marketing_consent_service( - self, - accepts_marketing, - requests_mock, - ): - """ - Tests accepts_dit_email_marketing field is populated from the consent service. - """ - contact = ContactFactory() - hawk_response = HawkMockJSONResponse( - api_id=settings.CONSENT_SERVICE_HAWK_ID, - api_key=settings.CONSENT_SERVICE_HAWK_KEY, - response={ - 'results': [ - { - 'email': contact.email, - 'consents': ( - [ - CONSENT_SERVICE_EMAIL_CONSENT_TYPE, - ] - if accepts_marketing - else [] - ), - }, - ], - }, - ) - requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' f'{CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - status_code=200, - text=hawk_response, - ) - api_client = self.create_api_client() - - url = reverse(f'{self.endpoint_namespace}:contact:detail', kwargs={'pk': contact.id}) - response = api_client.get(url) - - assert requests_mock.call_count == 1 - assert response.status_code == status.HTTP_200_OK - assert response.json()['accepts_dit_email_marketing'] == accepts_marketing - - @pytest.mark.parametrize( - 'response_status', - ( - status.HTTP_400_BAD_REQUEST, - status.HTTP_401_UNAUTHORIZED, - status.HTTP_403_FORBIDDEN, - status.HTTP_404_NOT_FOUND, - status.HTTP_405_METHOD_NOT_ALLOWED, - status.HTTP_500_INTERNAL_SERVER_ERROR, - ), - ) - def test_accepts_dit_email_marketing_consent_service_http_error( - self, - response_status, - requests_mock, - ): - """Tests accepts_dit_email_marketing field return false if there is an error.""" - contact = ContactFactory() - requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' f'{CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - status_code=response_status, - ) - api_client = self.create_api_client() - - url = reverse(f'{self.endpoint_namespace}:contact:detail', kwargs={'pk': contact.id}) - response = api_client.get(url) - - assert requests_mock.call_count == 1 - assert response.json()['accepts_dit_email_marketing'] is False - - @pytest.mark.parametrize( - 'exceptions', - ( - ConnectionError, - ConnectTimeout, - ReadTimeout, - ), - ) - def test_accepts_dit_email_marketing_consent_service_error( - self, - exceptions, - requests_mock, - ): - """Tests accepts_dit_email_marketing field return false if there is an error.""" - contact = ContactFactory() - requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' f'{CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - exc=exceptions, - ) - api_client = self.create_api_client() - - url = reverse(f'{self.endpoint_namespace}:contact:detail', kwargs={'pk': contact.id}) - response = api_client.get(url) - - assert requests_mock.call_count == 1 - assert response.json()['accepts_dit_email_marketing'] is False - class TestViewContactV3(ViewContactBase): """Tests for v3 view contacts endpoint""" diff --git a/datahub/company/views.py b/datahub/company/views.py index 915613f71..0daa3a384 100644 --- a/datahub/company/views.py +++ b/datahub/company/views.py @@ -1,4 +1,5 @@ """Company and related resources view sets.""" + from django.contrib.auth.models import Group, Permission from django.db import transaction from django.db.models import Exists, Prefetch, Q @@ -40,8 +41,6 @@ AssignRegionalAccountManagerSerializer, CompanyExportSerializer, CompanySerializer, - ContactDetailSerializer, - ContactDetailV4Serializer, ContactSerializer, ContactV4Serializer, ObjectiveV4Serializer, @@ -393,15 +392,6 @@ class ContactViewSet(ArchivableViewSetMixin, CoreViewSet): filterset_fields = ['company_id', 'email', 'archived'] ordering = ('-created_on',) - def get_serializer_class(self): - """ - Overwrites the built in get_serializer_class method in order - to return the ContactDetailSerializer if certain actions are called. - """ - if self.action in ('create', 'retrieve', 'partial_update'): - return ContactDetailSerializer - return super().get_serializer_class() - def get_additional_data(self, create): """Set adviser to the user on model instance creation.""" data = super().get_additional_data(create) @@ -416,15 +406,6 @@ class ContactV4ViewSet(ContactViewSet): serializer_class = ContactV4Serializer pagination_class = ContactPageSize - def get_serializer_class(self): - """ - Overwrites the built in get_serializer_class method in order - to return the ContactDetailSerializer if certain actions are called. - """ - if self.action in ('create', 'retrieve', 'partial_update'): - return ContactDetailV4Serializer - return super().get_serializer_class() - class ContactAuditViewSet(AuditViewSet): """Contact audit views.""" From b1f1dfe57477c08cb6858b0dc1cddd26f448f189 Mon Sep 17 00:00:00 2001 From: Chris Hopkins Date: Fri, 13 Dec 2024 09:40:27 +0000 Subject: [PATCH 2/4] test fixes --- .../test_contact_detail_serializer.py | 2 +- datahub/company/test/test_contact_views.py | 46 ++----------------- 2 files changed, 5 insertions(+), 43 deletions(-) diff --git a/datahub/company/test/serializers/test_contact_detail_serializer.py b/datahub/company/test/serializers/test_contact_detail_serializer.py index 5e7f5ffa7..20c7c1e16 100644 --- a/datahub/company/test/serializers/test_contact_detail_serializer.py +++ b/datahub/company/test/serializers/test_contact_detail_serializer.py @@ -16,7 +16,7 @@ headers={ 'x-b3-traceid': '123', 'x-b3-spanid': '456', - } + }, ) diff --git a/datahub/company/test/test_contact_views.py b/datahub/company/test/test_contact_views.py index 12e51c97c..bbfe9d8cb 100644 --- a/datahub/company/test/test_contact_views.py +++ b/datahub/company/test/test_contact_views.py @@ -6,15 +6,10 @@ from django.conf import settings from django.utils.timezone import now from freezegun import freeze_time -from requests.exceptions import ConnectionError, ConnectTimeout, ReadTimeout from rest_framework import status from rest_framework.reverse import reverse from reversion.models import Version -from datahub.company.consent import CONSENT_SERVICE_PERSON_PATH_LOOKUP -from datahub.company.constants import ( - CONSENT_SERVICE_EMAIL_CONSENT_TYPE, -) from datahub.company.models import Contact from datahub.company.test.factories import ArchivedContactFactory, CompanyFactory, ContactFactory from datahub.core import constants @@ -40,37 +35,16 @@ def generate_hawk_response(response): ) -@pytest.fixture -def get_consent_fixture(requests_mock): - """Mock get call to consent service.""" - yield lambda response: requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' f'{CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - status_code=200, - text=generate_hawk_response(response), - ) - - class AddContactBase(APITestMixin): """Add contact test case.""" endpoint_namespace = None @freeze_time('2017-04-18 13:25:30.986208') - def test_with_manual_address(self, get_consent_fixture): + def test_with_manual_address(self): """Test add with manual address.""" company = CompanyFactory() - get_consent_fixture( - { - 'results': [ - { - 'consents': [ - 'email_marketing', - ], - 'email': 'foo@bar.com', - }, - ], - }, - ) + url = reverse(f'{self.endpoint_namespace}:contact:list') response = self.api_client.post( url, @@ -97,7 +71,6 @@ def test_with_manual_address(self, get_consent_fixture): }, 'address_postcode': 'SW1A1AA', 'notes': 'lorem ipsum', - 'accepts_dit_email_marketing': True, }, ) @@ -360,21 +333,10 @@ class TestAddContactV4(AddContactBase): endpoint_namespace = 'api-v4' @freeze_time('2017-04-18 13:25:30.986208') - def test_with_us_manual_address(self, get_consent_fixture): + def test_with_us_manual_address(self): """Test add with manual address.""" company = CompanyFactory() - get_consent_fixture( - { - 'results': [ - { - 'consents': [ - 'email_marketing', - ], - 'email': 'foo@bar.com', - }, - ], - }, - ) + url = reverse('api-v4:contact:list') response = self.api_client.post( url, From df1f9dde110c9a4af74b491e197fe79ba471d177 Mon Sep 17 00:00:00 2001 From: Chris Hopkins Date: Fri, 13 Dec 2024 10:56:50 +0000 Subject: [PATCH 3/4] remove consent service --- config/settings/common.py | 4 - config/settings/test.py | 11 +- datahub/company/consent.py | 198 ------------- datahub/company/serializers.py | 23 -- datahub/company/test/test_consent_client.py | 291 -------------------- datahub/dataset/contact/test/test_views.py | 22 +- datahub/search/contact/test/test_views.py | 154 +++++------ datahub/search/contact/views.py | 51 ---- 8 files changed, 84 insertions(+), 670 deletions(-) delete mode 100644 datahub/company/consent.py delete mode 100644 datahub/company/test/test_consent_client.py diff --git a/config/settings/common.py b/config/settings/common.py index dd2e60c52..3814bc199 100644 --- a/config/settings/common.py +++ b/config/settings/common.py @@ -697,10 +697,6 @@ def _add_hawk_credentials(id_env_name, key_env_name, scopes): DNB_AUTOMATIC_UPDATE_LIMIT = env.int('DNB_AUTOMATIC_UPDATE_LIMIT', default=None) DNB_MAX_COMPANIES_IN_TREE_COUNT = env.int('DNB_MAX_COMPANIES_IN_TREE_COUNT', default=1000) -# Legal Basis / Consent Service -CONSENT_SERVICE_BASE_URL = env('CONSENT_SERVICE_BASE_URL', default=None) -CONSENT_SERVICE_HAWK_ID = env('CONSENT_SERVICE_HAWK_ID', default=None) -CONSENT_SERVICE_HAWK_KEY = env('CONSENT_SERVICE_HAWK_KEY', default=None) DATAHUB_SUPPORT_EMAIL_ADDRESS = env('DATAHUB_SUPPORT_EMAIL_ADDRESS', default=None) diff --git a/config/settings/test.py b/config/settings/test.py index 04b79e7bd..829dd332d 100644 --- a/config/settings/test.py +++ b/config/settings/test.py @@ -57,11 +57,7 @@ 'django.contrib.auth.hashers.MD5PasswordHasher', ] -CACHES = { - 'default': { - 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache' - } -} +CACHES = {'default': {'BACKEND': 'django.core.cache.backends.locmem.LocMemCache'}} # Stop WhiteNoise emitting warnings when running tests without running collectstatic first WHITENOISE_AUTOREFRESH = True @@ -124,7 +120,7 @@ 'aws_access_key_id': 'bar', 'aws_secret_access_key': 'baz', 'aws_region': 'eu-west-2', - } + }, } DIT_EMAIL_INGEST_BLOCKLIST = [ @@ -161,9 +157,6 @@ ADMIN_OAUTH2_CLIENT_SECRET = 'client-secret' ADMIN_OAUTH2_LOGOUT_PATH = 'http://sso-server/o/logout' -CONSENT_SERVICE_BASE_URL = 'http://consent.service/' -CONSENT_SERVICE_HAWK_ID = 'some-id' -CONSENT_SERVICE_HAWK_KEY = 'some-secret' COMPANY_MATCHING_SERVICE_BASE_URL = 'http://content.matching/' COMPANY_MATCHING_HAWK_ID = 'some-id' diff --git a/datahub/company/consent.py b/datahub/company/consent.py deleted file mode 100644 index a031f7cc6..000000000 --- a/datahub/company/consent.py +++ /dev/null @@ -1,198 +0,0 @@ -""" -A wrapper around the DIT Legal Basis service which allows for both querying -and setting email marketing consent for an email address. -""" - -import logging - -from django.conf import settings -from django.core.exceptions import ImproperlyConfigured -from requests.exceptions import HTTPError, Timeout - -from datahub.company.constants import CONSENT_SERVICE_EMAIL_CONSENT_TYPE -from datahub.core.api_client import APIClient, HawkAuth -from datahub.core.exceptions import APIBadGatewayException - -logger = logging.getLogger(__name__) - -CONSENT_SERVICE_PERSON_PATH = 'api/v1/person/' -CONSENT_SERVICE_PERSON_PATH_LOOKUP = f'{CONSENT_SERVICE_PERSON_PATH}bulk_lookup/' -CONSENT_SERVICE_CONNECT_TIMEOUT = 5.0 -CONSENT_SERVICE_READ_TIMEOUT = 30.0 - - -class ConsentAPIError(Exception): - """ - Base exception class for Consent API related errors. - """ - - -class ConsentAPIHTTPError(ConsentAPIError): - """ - Exception for all HTTP errors. - """ - - -class ConsentAPITimeoutError(ConsentAPIError): - """ - Exception for when a timeout was encountered when connecting to Consent API. - """ - - -class ConsentAPIConnectionError(ConsentAPIError): - """ - Exception for when an error was encountered when connecting to Consent API. - """ - - -class CaseInsensitiveDict(dict): - """Inherit dict class to make keys case insitive.""" - - def __setitem__(self, key, value): - """Transform key to lower case when setting an item.""" - super().__setitem__(key.lower(), value) - - def __getitem__(self, key): - """Transform key to lower case when getting an item.""" - return super().__getitem__(key.lower()) - - def get(self, key, default=False): - """Transform key to lower case when getting an item using a dict get method.""" - return super().get(key.lower(), default) - - -def _get_client(request=None): - """ - Get configured API client for the consent service, - _api_client could've just been set as a top level attribute - of this module but that makes testing harder as the settings are - loaded at `import` time rather than at run time. Which breaks test - utilities like Django's override_settings decorator which is used - to change django settings inside tests. - """ - if not all([ - settings.CONSENT_SERVICE_HAWK_ID, - settings.CONSENT_SERVICE_HAWK_KEY, - settings.CONSENT_SERVICE_BASE_URL, - ]): - raise ImproperlyConfigured( - 'CONSENT_SERVICE_* environment variables must be set, see README', - ) - - _auth = HawkAuth( - api_id=settings.CONSENT_SERVICE_HAWK_ID, - api_key=settings.CONSENT_SERVICE_HAWK_KEY, - ) - - _api_client = APIClient( - settings.CONSENT_SERVICE_BASE_URL, - auth=_auth, - default_timeout=(CONSENT_SERVICE_CONNECT_TIMEOUT, CONSENT_SERVICE_READ_TIMEOUT), - request=request, - ) - return _api_client - - -def update_consent(email_address, accepts_dit_email_marketing, modified_at=None, - **kwargs): - """ - Update marketing consent for an email address - :param email_address: email address you want to update marketing consent for - :param accepts_dit_email_marketing: True if they want marketing, False if they don't - :param modified_at: iso8601 formatted datetime with timezone, in UTC as a string - """ - if (email_address is None or len(str(email_address).strip()) == 0): - raise ValueError('email_address is a required field') - - logger.info(f'update_consent: {email_address}, {accepts_dit_email_marketing}') - - body = { - 'consents': [ - CONSENT_SERVICE_EMAIL_CONSENT_TYPE, - ] if accepts_dit_email_marketing else [], - 'email': email_address.lower(), - } - - if modified_at: - body['modified_at'] = modified_at - - headers = { - **kwargs.get('headers', {}), - **kwargs.get('zipkin_headers', {}), - } - - _get_client().request( - 'post', - CONSENT_SERVICE_PERSON_PATH, - json=body, - headers=headers, - ) - - -def get_many(emails): - """ - Bulk lookup consent for a list of emails - :param emails: List of email addresses - :return: dict of email address to consent status - - Below is a sample of the data shape. - { - "count": 1, - "next": null, - "previous": null, - "results": [ - { - "id": 1486081, - "consents": [ - "email_marketing" - ], - "modified_at": "2020-07-27T11:54:51.796358Z", - "key": "", - "email": "example@example.com", - "phone": "", - "key_type": "email", - "created_at": "2020-07-27T11:54:51.874350Z", - "current": true - } - ] - } - } - """ - api_client = _get_client() - - try: - response = api_client.request( - 'GET', - CONSENT_SERVICE_PERSON_PATH_LOOKUP, - params={'email': [email.lower() for email in emails]}, - ) - except APIBadGatewayException as exc: - logger.error(exc) - error_message = 'Encountered an error connecting to Legal Basis API' - raise ConsentAPIConnectionError(error_message) from exc - except Timeout as exc: - logger.error(exc) - error_message = 'Encountered a timeout interacting with Legal Basis API' - raise ConsentAPITimeoutError(error_message) from exc - except HTTPError as exc: - logger.error(exc) - error_message = ( - 'The Legal Basis API returned an error status: ' - f'{exc.response.status_code}', - ) - raise ConsentAPIHTTPError(error_message) from exc - - results = { - result['email']: CONSENT_SERVICE_EMAIL_CONSENT_TYPE in result['consents'] - for result in response.json()['results'] - } - return CaseInsensitiveDict(results) - - -def get_one(email): - """ - Get consent for single email address - :return: bool indicating if we have consent to send email marketing - to an address - """ - return get_many([email]).get(email, False) diff --git a/datahub/company/serializers.py b/datahub/company/serializers.py index 40c6b6767..de1fd72ba 100644 --- a/datahub/company/serializers.py +++ b/datahub/company/serializers.py @@ -9,7 +9,6 @@ from django.utils.translation import gettext_lazy from rest_framework import serializers -from datahub.company import consent from datahub.company.constants import ( BusinessTypeConstant, OneListTierID, @@ -276,28 +275,6 @@ class Meta(ContactSerializer.Meta): ] -class ConsentMarketingField(serializers.BooleanField): - """ - ConsentMarketingField will lookup consent data. - The model that this fields is used must have an email field - BooleanField is subclassed here for validation. - """ - - def to_internal_value(self, data): - """Validate boolean on incoming data.""" - return { - 'accepts_dit_email_marketing': super().to_internal_value(data), - } - - def to_representation(self, value): - """Lookup from consent service api/""" - try: - representation = consent.get_one(value.email) - except consent.ConsentAPIError: - representation = False - return representation - - class CompanyExportCountrySerializer(serializers.ModelSerializer): """ Export country serializer holding `Country` and its status. diff --git a/datahub/company/test/test_consent_client.py b/datahub/company/test/test_consent_client.py deleted file mode 100644 index d5c25a173..000000000 --- a/datahub/company/test/test_consent_client.py +++ /dev/null @@ -1,291 +0,0 @@ -import urllib.parse - -import pytest -from django.conf import settings -from requests.exceptions import ConnectionError, Timeout -from rest_framework import status - -from datahub.company import consent as consent -from datahub.company.constants import CONSENT_SERVICE_EMAIL_CONSENT_TYPE -from datahub.core.test_utils import HawkMockJSONResponse - - -def generate_hawk_response(payload): - """Mocks HAWK server validation for content.""" - return HawkMockJSONResponse( - api_id=settings.CONSENT_SERVICE_HAWK_ID, - api_key=settings.CONSENT_SERVICE_HAWK_KEY, - response=payload, - ) - - -class TestConsentClient: - """ - Test for consent service client module - """ - - @pytest.mark.parametrize('accepts_marketing', (True, False)) - def test_get_one(self, requests_mock, accepts_marketing): - """ - Try to get consent status for a single email address - """ - matcher = requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' - f'{consent.CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - text=generate_hawk_response({ - 'results': [{ - 'email': 'foo@bar.com', - 'consents': [ - CONSENT_SERVICE_EMAIL_CONSENT_TYPE, - ] if accepts_marketing else [], - }], - }), - status_code=status.HTTP_200_OK, - ) - resp = consent.get_one('foo@bar.com') - assert resp == accepts_marketing - - assert matcher.called_once - assert matcher.last_request.query == 'email=foo%40bar.com' - - @pytest.mark.parametrize('emails', ([], ['foo@bar.com'], ['bar@foo.com', 'foo@bar.com'])) - @pytest.mark.parametrize('accepts_marketing', (True, False)) - def test_get_many(self, requests_mock, accepts_marketing, emails): - """ - Try to get consent status for a list of email addresses - """ - matcher = requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' - f'{consent.CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - text=generate_hawk_response({ - 'results': [ - { - 'email': email, - 'consents': [ - CONSENT_SERVICE_EMAIL_CONSENT_TYPE, - ] if accepts_marketing else [], - } for email in emails - ], - }), - status_code=status.HTTP_200_OK, - ) - resp = consent.get_many(emails) - assert resp == {email: accepts_marketing for email in emails} - - assert matcher.called_once - assert matcher.last_request.query == urllib.parse.urlencode({'email': emails}, doseq=True) - - @pytest.mark.parametrize('accepts_marketing', (True, False)) - def test_get_one_normalises_emails(self, requests_mock, accepts_marketing): - """ - Try to get consent status for a single email address - """ - matcher = requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' - f'{consent.CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - text=generate_hawk_response({ - 'results': [{ - 'email': 'foo@bar.com', - 'consents': [ - CONSENT_SERVICE_EMAIL_CONSENT_TYPE, - ] if accepts_marketing else [], - }], - }), - status_code=status.HTTP_200_OK, - ) - resp = consent.get_one('FOO@BAR.COM') - assert resp == accepts_marketing - - assert matcher.called_once - assert matcher.last_request.query == 'email=foo%40bar.com' - - @pytest.mark.parametrize('emails', ([], ['foo@bar.com'], ['bar@foo.com', 'foo@bar.com'])) - @pytest.mark.parametrize('accepts_marketing', (True, False)) - def test_get_many_normalises_emails(self, requests_mock, accepts_marketing, emails): - """ - Try to get consent status for a list of email addresses - """ - matcher = requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' - f'{consent.CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - text=generate_hawk_response({ - 'results': [ - { - 'email': email, - 'consents': [ - CONSENT_SERVICE_EMAIL_CONSENT_TYPE, - ] if accepts_marketing else [], - } for email in emails - ], - }), - status_code=status.HTTP_200_OK, - ) - resp = consent.get_many([email.upper() for email in emails]) - assert resp == {email: accepts_marketing for email in emails} - - assert matcher.called_once - assert matcher.last_request.query == urllib.parse.urlencode({'email': emails}, doseq=True) - - @pytest.mark.parametrize('accepts_marketing', (True, False)) - def test_update(self, requests_mock, accepts_marketing): - """ - Try to update consent status - """ - matcher = requests_mock.post( - f'{settings.CONSENT_SERVICE_BASE_URL}' - f'{consent.CONSENT_SERVICE_PERSON_PATH}', - text=generate_hawk_response({ - 'consents': [ - CONSENT_SERVICE_EMAIL_CONSENT_TYPE, - ], - 'modified_at': '2020-03-12T15:33:50.907000Z', - 'email': 'foo@bar.com', - 'phone': '', - 'key_type': 'email', - }), - status_code=status.HTTP_201_CREATED, - ) - result = consent.update_consent('foo@bar.com', accepts_marketing) - assert result is None - assert matcher.called_once - - def test_forward_zipkin_headers(self, requests_mock): - """ - Forward zipkin headers from origin request to the API call - """ - headers = { - 'x-b3-traceid': '123', - 'x-b3-spanid': '456', - } - matcher = requests_mock.post( - f'{settings.CONSENT_SERVICE_BASE_URL}' - f'{consent.CONSENT_SERVICE_PERSON_PATH}', - text=generate_hawk_response({ - 'consents': [ - CONSENT_SERVICE_EMAIL_CONSENT_TYPE, - ], - 'modified_at': '2020-03-12T15:33:50.907000Z', - 'email': 'foo@bar.com', - 'phone': '', - 'key_type': 'email', - }), - status_code=status.HTTP_201_CREATED, - ) - result = consent.update_consent('foo@bar.com', False, None, - headers=headers) - assert result is None - assert headers.items() <= matcher.last_request.headers.items() - assert matcher.called_once - - @pytest.mark.parametrize( - 'response_status', - ( - status.HTTP_400_BAD_REQUEST, - status.HTTP_401_UNAUTHORIZED, - status.HTTP_403_FORBIDDEN, - status.HTTP_404_NOT_FOUND, - status.HTTP_405_METHOD_NOT_ALLOWED, - status.HTTP_500_INTERNAL_SERVER_ERROR, - status.HTTP_502_BAD_GATEWAY, - status.HTTP_503_SERVICE_UNAVAILABLE, - status.HTTP_504_GATEWAY_TIMEOUT, - ), - ) - def test_get_one_raises_exception_when_service_http_errors( - self, - requests_mock, - response_status, - ): - """ - When the Consent Service responds with a http error, It raises a HTTPError. - """ - requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' - f'{consent.CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - status_code=response_status, - ) - - with pytest.raises(consent.ConsentAPIHTTPError): - consent.get_one('foo@bar.com') - - @pytest.mark.parametrize( - 'response_status', - ( - status.HTTP_400_BAD_REQUEST, - status.HTTP_401_UNAUTHORIZED, - status.HTTP_403_FORBIDDEN, - status.HTTP_404_NOT_FOUND, - status.HTTP_405_METHOD_NOT_ALLOWED, - status.HTTP_500_INTERNAL_SERVER_ERROR, - status.HTTP_502_BAD_GATEWAY, - status.HTTP_503_SERVICE_UNAVAILABLE, - status.HTTP_504_GATEWAY_TIMEOUT, - ), - ) - def test_get_many_raises_exception_when_service_http_errors( - self, - requests_mock, - response_status, - ): - """ - When the Consent Service responds with a http error, It raises a HTTPError. - """ - requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' - f'{consent.CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - status_code=response_status, - ) - emails = ['foo1@bar.com', 'foo2@bar.com', 'foo3@bar.com'] - with pytest.raises(consent.ConsentAPIHTTPError): - consent.get_many(emails) - - @pytest.mark.parametrize( - 'exceptions', - ( - (ConnectionError, consent.ConsentAPIConnectionError), - (Timeout, consent.ConsentAPITimeoutError), - ), - ) - def test_get_many_raises_exception_on_connection_or_timeout_error( - self, - requests_mock, - exceptions, - ): - """ - When the Consent Service responds with a 4XX error, It raises - a ConnectionError or a Timeout Error - """ - (request_exception, consent_exception) = exceptions - requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' - f'{consent.CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - exc=request_exception, - ) - emails = ['foo1@bar.com', 'foo2@bar.com', 'foo3@bar.com'] - with pytest.raises(consent_exception): - consent.get_many(emails) - - @pytest.mark.parametrize( - 'exceptions', - ( - (ConnectionError, consent.ConsentAPIConnectionError), - (Timeout, consent.ConsentAPITimeoutError), - ), - ) - def test_get_one_raises_exception_on_connection_or_timeout_error( - self, - requests_mock, - exceptions, - ): - """ - When the Consent Service responds with a 4XX error, It raises - a ConnectionError or a Timeout Error - """ - (request_exception, consent_exception) = exceptions - requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' - f'{consent.CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - exc=request_exception, - ) - with pytest.raises(consent_exception): - consent.get_one('foo@bar.com') diff --git a/datahub/dataset/contact/test/test_views.py b/datahub/dataset/contact/test/test_views.py index 37f3c1def..5621d23c0 100644 --- a/datahub/dataset/contact/test/test_views.py +++ b/datahub/dataset/contact/test/test_views.py @@ -1,7 +1,5 @@ from datetime import datetime, timezone -from unittest.mock import Mock - import pytest from django.urls import reverse @@ -49,14 +47,6 @@ def get_expected_data_from_contact(contact): } -@pytest.fixture -def consent_get_many_mock(monkeypatch): - """Mocks the consent.get_many function""" - mock = Mock() - monkeypatch.setattr('datahub.company.consent.get_many', mock) - yield mock - - @pytest.mark.django_db class TestContactsDatasetViewSet(BaseDatasetViewTest): """ @@ -67,11 +57,13 @@ class TestContactsDatasetViewSet(BaseDatasetViewTest): factory = ContactFactory @pytest.mark.parametrize( - 'contact_factory', ( + 'contact_factory', + ( ArchivedContactFactory, ContactFactory, ContactWithOwnAddressFactory, - )) + ), + ) def test_success(self, data_flow_api_client, contact_factory): """Test that endpoint returns with expected data for a single order""" contact = contact_factory() @@ -97,8 +89,10 @@ def test_with_multiple_contacts(self, data_flow_api_client): assert response.status_code == status.HTTP_200_OK response_results = response.json()['results'] assert len(response_results) == 4 - expected_contact_list = sorted([contact_3, contact_4], - key=lambda item: item.pk) + [contact_1, contact_2] + expected_contact_list = sorted([contact_3, contact_4], key=lambda item: item.pk) + [ + contact_1, + contact_2, + ] for index, contact in enumerate(expected_contact_list): assert contact.email == response_results[index]['email'] diff --git a/datahub/search/contact/test/test_views.py b/datahub/search/contact/test/test_views.py index 42b902b58..e1a7fe0f0 100644 --- a/datahub/search/contact/test/test_views.py +++ b/datahub/search/contact/test/test_views.py @@ -1,4 +1,3 @@ -import urllib.parse import uuid from cgi import parse_header from csv import DictReader @@ -13,8 +12,6 @@ from rest_framework import status from rest_framework.reverse import reverse -from datahub.company.consent import CONSENT_SERVICE_PERSON_PATH_LOOKUP -from datahub.company.constants import CONSENT_SERVICE_EMAIL_CONSENT_TYPE from datahub.company.models import Contact, ContactPermission from datahub.company.test.factories import ( AdviserFactory, @@ -200,9 +197,11 @@ def test_company_sector_descends_filter( other_companies = CompanyFactory.create_batch( 3, - sector=factory.LazyFunction(lambda: random_obj_for_queryset( - SectorModel.objects.exclude(pk__in=sectors_ids), - )), + sector=factory.LazyFunction( + lambda: random_obj_for_queryset( + SectorModel.objects.exclude(pk__in=sectors_ids), + ), + ), ) ContactFactory.create_batch( 3, @@ -234,7 +233,6 @@ def test_company_sector_descends_filter( ('his', 'whiskers and tabby'), ('ers', 'whiskers and tabby'), ('1a', '1a'), - # trading names ('maine coon egyptian mau', 'whiskers and tabby'), ('maine', 'whiskers and tabby'), @@ -242,7 +240,6 @@ def test_company_sector_descends_filter( ('ine oon', 'whiskers and tabby'), ('ine mau', 'whiskers and tabby'), ('3a', '1a'), - # non-matches ('whi lorem', None), ('wh', None), @@ -253,7 +250,10 @@ def test_company_sector_descends_filter( ), ) def test_filter_by_company_name( - self, opensearch_with_collector, name_term, matched_company_name, + self, + opensearch_with_collector, + name_term, + matched_company_name, ): """Tests filtering contact by company name.""" matching_company1 = CompanyFactory( @@ -314,7 +314,8 @@ def test_search_contact_by_partial_name(self, opensearch_with_collector, setup_d assert response.data['results'][0]['first_name'] == contact.first_name @pytest.mark.parametrize( - 'archived', ( + 'archived', + ( True, False, ), @@ -363,10 +364,7 @@ def test_filter_by_created_on_exists(self, opensearch_with_collector, created_on response_data = response.json() results = response_data['results'] assert response_data['count'] == 3 - assert all( - (not result['created_on'] is None) == created_on_exists - for result in results - ) + assert all((not result['created_on'] is None) == created_on_exists for result in results) def test_search_contact_by_company_id(self, opensearch_with_collector, setup_data): """Tests filtering by company id.""" @@ -536,14 +534,12 @@ def test_user_without_permission_cannot_export(self, opensearch_with_collector, ('address_country.name', 'computed_address_country_name'), ), ) - @pytest.mark.parametrize('accepts_dit_email_marketing', (True, False)) def test_export( self, opensearch_with_collector, request_sortby, orm_ordering, requests_mock, - accepts_dit_email_marketing, ): """Test export of contact search results.""" ArchivedContactFactory() @@ -574,7 +570,8 @@ def test_export( assert response.status_code == status.HTTP_200_OK assert parse_header(response.get('Content-Type')) == ('text/csv', {'charset': 'utf-8'}) assert parse_header(response.get('Content-Disposition')) == ( - 'attachment', {'filename': 'Data Hub - Contacts - 2018-01-01-11-12-13.csv'}, + 'attachment', + {'filename': 'Data Hub - Contacts - 2018-01-01-11-12-13.csv'}, ) sorted_contacts = Contact.objects.annotate( @@ -583,81 +580,78 @@ def test_export( 'company__address_country__name', ), ).order_by( - orm_ordering, 'pk', - ) - - matcher = requests_mock.get( - f'{settings.CONSENT_SERVICE_BASE_URL}' - f'{CONSENT_SERVICE_PERSON_PATH_LOOKUP}', - text=generate_hawk_response({ - 'results': [{ - 'email': contact.email, - 'consents': [ - CONSENT_SERVICE_EMAIL_CONSENT_TYPE, - ] if accepts_dit_email_marketing else [], - } for contact in sorted_contacts], - }), - status_code=status.HTTP_200_OK, + orm_ordering, + 'pk', ) reader = DictReader(StringIO(response.getvalue().decode('utf-8-sig'))) assert reader.fieldnames == list(SearchContactExportAPIView.field_titles.values()) - - expected_row_data = format_csv_data([ - { - 'Name': contact.name, - 'Job title': contact.job_title, - 'Date created': contact.created_on, - 'Archived': contact.archived, - 'Link': f'{settings.DATAHUB_FRONTEND_URL_PREFIXES["contact"]}/{contact.pk}', - 'Company': get_attr_or_none(contact, 'company.name'), - 'Company sector': get_attr_or_none(contact, 'company.sector.name'), - 'Company link': - f'{settings.DATAHUB_FRONTEND_URL_PREFIXES["company"]}/{contact.company.pk}', - 'Company UK region': get_attr_or_none(contact, 'company.uk_region.name'), - 'Area': - (contact.company.address_area and contact.company.address_area.name) - if contact.address_same_as_company - else (contact.address_area and contact.address_area.name), - 'Country': - contact.company.address_country.name - if contact.address_same_as_company - else contact.address_country.name, - 'Postcode': - contact.company.address_postcode - if contact.address_same_as_company - else contact.address_postcode, - 'Phone number': contact.full_telephone_number, - 'Email address': contact.email, - 'Accepts DIT email marketing': accepts_dit_email_marketing, - 'Date of latest interaction': - max(contact.interactions.all(), key=attrgetter('date')).date - if contact.interactions.all() else None, - 'Teams of latest interaction': _format_interaction_team_names( - max(contact.interactions.all(), key=attrgetter('date')), - ) if contact.interactions.exists() else None, - 'Created by team': get_attr_or_none(contact, 'created_by.dit_team.name'), - } - for contact in sorted_contacts - ]) + front_end_prefixes = settings.DATAHUB_FRONTEND_URL_PREFIXES + + expected_row_data = format_csv_data( + [ + { + 'Name': contact.name, + 'Job title': contact.job_title, + 'Date created': contact.created_on, + 'Archived': contact.archived, + 'Link': f'{front_end_prefixes["contact"]}/{contact.pk}', + 'Company': get_attr_or_none(contact, 'company.name'), + 'Company sector': get_attr_or_none(contact, 'company.sector.name'), + 'Company link': f'{front_end_prefixes["company"]}/{contact.company.pk}', + 'Company UK region': get_attr_or_none(contact, 'company.uk_region.name'), + 'Area': ( + (contact.company.address_area and contact.company.address_area.name) + if contact.address_same_as_company + else (contact.address_area and contact.address_area.name) + ), + 'Country': ( + contact.company.address_country.name + if contact.address_same_as_company + else contact.address_country.name + ), + 'Postcode': ( + contact.company.address_postcode + if contact.address_same_as_company + else contact.address_postcode + ), + 'Phone number': contact.full_telephone_number, + 'Email address': contact.email, + 'Date of latest interaction': ( + max(contact.interactions.all(), key=attrgetter('date')).date + if contact.interactions.all() + else None + ), + 'Teams of latest interaction': ( + _format_interaction_team_names( + max(contact.interactions.all(), key=attrgetter('date')), + ) + if contact.interactions.exists() + else None + ), + 'Created by team': get_attr_or_none(contact, 'created_by.dit_team.name'), + } + for contact in sorted_contacts + ], + ) actual_row_data = [dict(row) for row in reader] assert len(actual_row_data) == len(expected_row_data) for index, row in enumerate(actual_row_data): assert row == expected_row_data[index] - assert matcher.call_count == 1 - assert matcher.last_request.query == urllib.parse.urlencode( - {'email': [c.email for c in sorted_contacts]}, doseq=True, - ) def _format_interaction_team_names(interaction): - names = interaction.dit_participants.values_list( - 'team__name', - flat=True, - ).order_by( - 'team__name', - ).distinct() + names = ( + interaction.dit_participants.values_list( + 'team__name', + flat=True, + ) + .order_by( + 'team__name', + ) + .distinct() + ) return ', '.join(names) diff --git a/datahub/search/contact/views.py b/datahub/search/contact/views.py index c0a78257d..94d2f0c15 100644 --- a/datahub/search/contact/views.py +++ b/datahub/search/contact/views.py @@ -3,7 +3,6 @@ from django.db.models import Case, CharField, Max, When from django.db.models.functions import Cast -from datahub.company import consent from datahub.company.models import Contact as DBContact from datahub.core.query_utils import ( get_aggregate_subquery, @@ -12,7 +11,6 @@ get_string_agg_subquery, get_top_related_expression_subquery, ) -from datahub.core.utils import slice_iterable_into_chunks from datahub.interaction.models import Interaction as DBInteraction from datahub.metadata.query_utils import get_sector_name_subquery from datahub.search.contact import ContactSearchApp @@ -139,56 +137,7 @@ def _is_valid_email(self, value): 'computed_postcode': 'Postcode', 'full_telephone_number': 'Phone number', 'email': 'Email address', - 'accepts_dit_email_marketing': 'Accepts DIT email marketing', 'date_of_latest_interaction': 'Date of latest interaction', 'teams_of_latest_interaction': 'Teams of latest interaction', 'created_by__dit_team__name': 'Created by team', } - - def _add_consent_response(self, rows): - """ - Transforms iterable to add user consent from the consent service. - - The consent lookup makes an external API call to return consent. - For perfromance reasons the consent amount is limited by consent_page_size. - Due to this limitaion the iterable are sliced into chunks requesting consent for 100 rows - at a time. - """ - # Slice iterable into chunks - row_chunks = slice_iterable_into_chunks(rows, self.consent_page_size) - for chunk in row_chunks: - """ - Loop over the chunks and extract the email and item. - Save the item because the iterator cannot be used twice. - """ - rows = list(chunk) - # Peform constent lookup on emails POST request - consent_lookups = consent.get_many( - [row['email'] for row in rows if self._is_valid_email(row['email'])], - ) - for row in rows: - # Assign contact consent boolean to accepts_dit_email_marketing - # and yield modified result. - row['accepts_dit_email_marketing'] = consent_lookups.get(row['email'], False) - yield row - - def _get_rows(self, ids, search_ordering): - """ - Get row queryset for constent service. - - This populates accepts_dit_email_marketing field from the consent service and - removes the accepts_dit_email_marketing from the field query because the field is not in - the db. - """ - db_ordering = self._translate_search_ordering_to_django_ordering(search_ordering) - field_titles = self.field_titles.copy() - del field_titles['accepts_dit_email_marketing'] - rows = self.queryset.filter( - pk__in=ids, - ).order_by( - *db_ordering, - ).values( - *field_titles, - ).iterator() - - return self._add_consent_response(rows) From 2c0064faa833b39bd826cdfc8c3185e2ff74f7b5 Mon Sep 17 00:00:00 2001 From: Chris Hopkins Date: Fri, 20 Dec 2024 09:09:49 +0000 Subject: [PATCH 4/4] Remove the redundant env vars from readme --- README.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/README.md b/README.md index 62e922f91..449f5a791 100644 --- a/README.md +++ b/README.md @@ -380,9 +380,6 @@ Data Hub API can run on any Heroku-style platform. Configuration is performed vi | `DEFAULT_BUCKET_AWS_ACCESS_KEY_ID` | No | Used as part of [boto3 auto-configuration](http://boto3.readthedocs.io/en/latest/guide/configuration.html#configuring-credentials). | | `DEFAULT_BUCKET_AWS_DEFAULT_REGION` | No | [Default region used by boto3.](http://boto3.readthedocs.io/en/latest/guide/configuration.html#environment-variable-configuration) | | `DEFAULT_BUCKET_AWS_SECRET_ACCESS_KEY` | No | Used as part of [boto3 auto-configuration](http://boto3.readthedocs.io/en/latest/guide/configuration.html#configuring-credentials). | -| `CONSENT_SERVICE_BASE_URL` | No | The base url of the consent service, to post email consent preferences to (default=None). | -| `CONSENT_SERVICE_HAWK_ID` | No | The hawk id to use when making a request to the consent service (default=None). | -| `CONSENT_SERVICE_HAWK_KEY` | No | The hawk key to use when making a request to the consent service (default=None). | | `CSRF_COOKIE_HTTPONLY` | No | Whether to use HttpOnly flag on the CSRF cookie (default=False). | | `CSRF_COOKIE_SECURE` | No | Whether to use a secure cookie for the CSRF cookie (default=False). | | `DATA_FLOW_API_ACCESS_KEY_ID` | No | A non-secret access key ID, corresponding to `DATA_FLOW_API_SECRET_ACCESS_KEY`. The holder of the secret key can access the omis-dataset endpoint by Hawk authentication. |