From 55d7e846d84c09fa206aeebdf716fe65dbab37d3 Mon Sep 17 00:00:00 2001 From: Marie PUPO JEAMMET Date: Thu, 19 Sep 2024 18:47:08 +0200 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F(serializers)=20move=20dimail?= =?UTF-8?q?=20calls=20to=20serializers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit we move all business logic from model to serializer. all API calls (direct and from front) will keep on triggering expected 3rd party calls while admin actions will uniquely trigger modifications in our database. --- CHANGELOG.md | 9 +- .../mailbox_manager/api/serializers.py | 9 ++ src/backend/mailbox_manager/factories.py | 38 ------ src/backend/mailbox_manager/models.py | 9 +- .../mailboxes/test_api_mailboxes_create.py | 58 +++++++++ .../tests/test_models_mailboxes.py | 116 ------------------ src/backend/mailbox_manager/utils/dimail.py | 10 +- 7 files changed, 81 insertions(+), 168 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d23030..27373fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,8 +11,13 @@ and this project adheres to ### Added -- ✨(domains) add endpoint to list and retrieve domain accesses @sdemagny #404 -- 🍱(dev) embark dimail-api as container by @mjeammet #366 +- ✨(domains) add endpoint to list and retrieve domain accesses #404 +- 🍱(dev) embark dimail-api as container #366 + + +### Changed + +- ♻️(serializers) move business logic to serializers #414 ## [1.1.0] - 2024-09-10 diff --git a/src/backend/mailbox_manager/api/serializers.py b/src/backend/mailbox_manager/api/serializers.py index 5b8686e..60372df 100644 --- a/src/backend/mailbox_manager/api/serializers.py +++ b/src/backend/mailbox_manager/api/serializers.py @@ -5,6 +5,7 @@ from rest_framework import serializers from core.api.serializers import UserSerializer from mailbox_manager import enums, models +from mailbox_manager.utils.dimail import DimailAPIClient class MailboxSerializer(serializers.ModelSerializer): @@ -16,6 +17,14 @@ class MailboxSerializer(serializers.ModelSerializer): # everything is actually read-only as we do not allow update for now read_only_fields = ["id"] + def create(self, validated_data): + """ + Override create function to fire a request on mailbox creation. + """ + client = DimailAPIClient() + client.send_mailbox_request(validated_data) + return models.Mailbox.objects.create(**validated_data) + class MailDomainSerializer(serializers.ModelSerializer): """Serialize mail domain.""" diff --git a/src/backend/mailbox_manager/factories.py b/src/backend/mailbox_manager/factories.py index a866d14..48ababb 100644 --- a/src/backend/mailbox_manager/factories.py +++ b/src/backend/mailbox_manager/factories.py @@ -2,14 +2,10 @@ Mailbox manager application factories """ -import re - from django.utils.text import slugify import factory.fuzzy -import responses from faker import Faker -from rest_framework import status from core import factories as core_factories from core import models as core_models @@ -79,37 +75,3 @@ class MailboxFactory(factory.django.DjangoModelFactory): ) domain = factory.SubFactory(MailDomainEnabledFactory) secondary_email = factory.Faker("email") - - @classmethod - def _create(cls, model_class, *args, use_mock=True, **kwargs): - domain = kwargs["domain"] - if use_mock and isinstance(domain, models.MailDomain): - with responses.RequestsMock() as rsps: - # Ensure successful response using "responses": - rsps.add( - rsps.GET, - re.compile(r".*/token/"), - body='{"access_token": "domain_owner_token"}', - status=status.HTTP_200_OK, - content_type="application/json", - ) - rsps.add( - rsps.POST, - re.compile( - rf".*/domains/{domain.name}/mailboxes/{kwargs['local_part']}" - ), - body=str( - { - "email": f"{kwargs['local_part']}@{domain.name}", - "password": "newpass", - "uuid": "uuid", - } - ), - status=status.HTTP_201_CREATED, - content_type="application/json", - ) - - result = super()._create(model_class, *args, **kwargs) - else: - result = super()._create(model_class, *args, **kwargs) - return result diff --git a/src/backend/mailbox_manager/models.py b/src/backend/mailbox_manager/models.py index 882244c..20f6959 100644 --- a/src/backend/mailbox_manager/models.py +++ b/src/backend/mailbox_manager/models.py @@ -4,14 +4,13 @@ Declare and configure the models for the People additional application : mailbox from django.conf import settings from django.core import exceptions, validators -from django.db import models, transaction +from django.db import models from django.utils.text import slugify from django.utils.translation import gettext_lazy as _ from core.models import BaseModel from mailbox_manager.enums import MailDomainRoleChoices, MailDomainStatusChoices -from mailbox_manager.utils.dimail import DimailAPIClient class MailDomain(BaseModel): @@ -155,16 +154,12 @@ class Mailbox(BaseModel): def save(self, *args, **kwargs): """ - Override save function to fire a request on mailbox creation. Modification is forbidden for now. """ self.full_clean() if self._state.adding: - with transaction.atomic(): - client = DimailAPIClient() - client.send_mailbox_request(self) - return super().save(*args, **kwargs) + return super().save(*args, **kwargs) # Update is not implemented for now raise NotImplementedError() diff --git a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_create.py b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_create.py index c31da87..d4d932d 100644 --- a/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_create.py +++ b/src/backend/mailbox_manager/tests/api/mailboxes/test_api_mailboxes_create.py @@ -4,6 +4,8 @@ Unit tests for the mailbox API import json import re +from logging import Logger +from unittest import mock from django.test.utils import override_settings @@ -488,3 +490,59 @@ def test_api_mailboxes__handling_dimail_unexpected_error(): "detail": "Unexpected response from dimail: {'details': 'Internal server error'}" } assert not models.Mailbox.objects.exists() + + +@mock.patch.object(Logger, "error") +@mock.patch.object(Logger, "info") +def test_api_mailboxes__send_correct_logger_infos(mock_info, mock_error): + """ + Upon requesting mailbox creation, things are correctly logged + """ + access = factories.MailDomainAccessFactory(role=enums.MailDomainRoleChoices.OWNER) + + client = APIClient() + client.force_login(access.user) + mailbox_data = serializers.MailboxSerializer( + factories.MailboxFactory.build(domain=access.domain) + ).data + + with responses.RequestsMock() as rsps: + # Ensure successful response using "responses": + rsps.add( + rsps.GET, + re.compile(r".*/token/"), + body='{"access_token": "domain_owner_token"}', + status=status.HTTP_200_OK, + content_type="application/json", + ) + rsps.add( + rsps.POST, + re.compile(rf".*/domains/{access.domain.name}/mailboxes/"), + body=str( + { + "email": f"{mailbox_data['local_part']}@{access.domain.name}", + "password": "newpass", + "uuid": "uuid", + } + ), + status=status.HTTP_201_CREATED, + content_type="application/json", + ) + + response = client.post( + f"/api/v1.0/mail-domains/{access.domain.slug}/mailboxes/", + mailbox_data, + format="json", + ) + assert response.status_code == status.HTTP_201_CREATED + + # Logger + assert not mock_error.called + assert mock_info.call_count == 3 + assert mock_info.call_args_list[0][0] == ( + "Token succesfully granted by mail-provisioning API.", + ) + assert mock_info.call_args_list[1][0] == ( + "Mailbox successfully created on domain %s", + access.domain.name, + ) diff --git a/src/backend/mailbox_manager/tests/test_models_mailboxes.py b/src/backend/mailbox_manager/tests/test_models_mailboxes.py index c1527d1..91ece88 100644 --- a/src/backend/mailbox_manager/tests/test_models_mailboxes.py +++ b/src/backend/mailbox_manager/tests/test_models_mailboxes.py @@ -2,20 +2,12 @@ Unit tests for the mailbox model """ -import json -import re -from logging import Logger -from unittest import mock - from django.core import exceptions from django.test.utils import override_settings import pytest -import responses -from rest_framework import status from mailbox_manager import enums, factories, models -from mailbox_manager.api import serializers pytestmark = pytest.mark.django_db @@ -158,111 +150,3 @@ def test_models_mailboxes__dimail_no_credentials(): match="Please configure MAIL_PROVISIONING_API_CREDENTIALS before creating any mailbox.", ): factories.MailboxFactory(domain=domain) - - -@override_settings(MAIL_PROVISIONING_API_CREDENTIALS="wrongCredentials") -def test_models_mailboxes__dimail_token_permissions_denied(): - """ - Our API should raise a clear "Permission denied" error - if dimail returns a permission denied on /token/ endpoint. - """ - - domain = factories.MailDomainEnabledFactory() - - with responses.RequestsMock() as rsps: - # Ensure successful response by scim provider using "responses": - rsps.add( - rsps.GET, - re.compile(r".*/token/"), - body='{"detail": "Permission denied"}', - status=status.HTTP_403_FORBIDDEN, - content_type="application/json", - ) - - with pytest.raises( - exceptions.PermissionDenied, - match="Token denied. Please check your MAIL_PROVISIONING_API_CREDENTIALS.", - ): - mailbox = factories.MailboxFactory(use_mock=False, domain=domain) - # Payload sent to mailbox provider - payload = json.loads(rsps.calls[1].request.body) - assert payload == { - "displayName": f"{mailbox.first_name} {mailbox.last_name}", - "givenName": mailbox.first_name, - "surName": mailbox.last_name, - } - - -@mock.patch.object(Logger, "error") -@mock.patch.object(Logger, "info") -def test_models_mailboxes__create_mailbox_success(mock_info, mock_error): - """Creating a mailbox sends the expected information and get expected response before saving.""" - domain = factories.MailDomainEnabledFactory() - - # generate mailbox data before mailbox, to mock responses - mailbox_data = serializers.MailboxSerializer( - factories.MailboxFactory.build(domain=domain) - ).data - - with responses.RequestsMock() as rsps: - # Ensure successful response using "responses": - rsps.add( - rsps.GET, - re.compile(r".*/token/"), - body='{"access_token": "domain_owner_token"}', - status=status.HTTP_200_OK, - content_type="application/json", - ) - rsps.add( - rsps.POST, - re.compile(rf".*/domains/{domain.name}/mailboxes/"), - body=str( - { - "email": f"{mailbox_data['local_part']}@{domain.name}", - "password": "newpass", - "uuid": "uuid", - } - ), - status=status.HTTP_201_CREATED, - content_type="application/json", - ) - - mailbox = factories.MailboxFactory( - use_mock=False, local_part=mailbox_data["local_part"], domain=domain - ) - - # Check headers - headers = rsps.calls[1].request.headers - assert headers["Content-Type"] == "application/json" - - # Payload sent to mailbox provider - payload = json.loads(rsps.calls[1].request.body) - assert payload == { - "displayName": f"{mailbox.first_name} {mailbox.last_name}", - "givenName": mailbox.first_name, - "surName": mailbox.last_name, - } - - # Logger - assert not mock_error.called - assert mock_info.call_count == 2 - assert mock_info.call_args_list[0][0] == ( - "Token succesfully granted by mail-provisioning API.", - ) - assert mock_info.call_args_list[1][0] == ( - "Mailbox successfully created on domain %s", - domain.name, - ) - assert mock_info.call_args_list[1][1] == ( - { - "extra": { - "response": str( - { - "email": f"{mailbox.local_part}@{domain.name}", - "password": "newpass", - "uuid": "uuid", - } - ) - } - } - ) diff --git a/src/backend/mailbox_manager/utils/dimail.py b/src/backend/mailbox_manager/utils/dimail.py index a1e88b7..60d260c 100644 --- a/src/backend/mailbox_manager/utils/dimail.py +++ b/src/backend/mailbox_manager/utils/dimail.py @@ -64,15 +64,15 @@ class DimailAPIClient: """Send a CREATE mailbox request to mail provisioning API.""" payload = { - "givenName": mailbox.first_name, - "surName": mailbox.last_name, - "displayName": f"{mailbox.first_name} {mailbox.last_name}", + "givenName": mailbox["first_name"], + "surName": mailbox["last_name"], + "displayName": f"{mailbox['first_name']} {mailbox['last_name']}", } headers = self.get_headers() try: response = session.post( - f"{self.API_URL}/domains/{mailbox.domain}/mailboxes/{mailbox.local_part}/", + f"{self.API_URL}/domains/{mailbox['domain']}/mailboxes/{mailbox['local_part']}/", json=payload, headers=headers, verify=True, @@ -93,7 +93,7 @@ class DimailAPIClient: # In the meantime, we log mailbox info (including password !) logger.info( "Mailbox successfully created on domain %s", - mailbox.domain.name, + str(mailbox["domain"]), extra=extra, ) return response