🐛(dimail) fix mailboxes import from dimail

CHANGELOG.md

@@ -8,6 +8,8 @@ and this project adheres to
 
 ## [Unreleased]
 
+- 🐛(dimail) fix mailboxes import from dimail #612
+
 ## [1.9.1] - 2024-12-18
 
 ## [1.9.0] - 2024-12-17

src/backend/mailbox_manager/tests/test_utils_dimail_client.py

@@ -2,10 +2,8 @@
 Unit tests for dimail client
 """
 
+import logging
 import re
-from email.errors import HeaderParseError, NonASCIILocalPartDefect
-from logging import Logger
-from unittest import mock
 
 import pytest
 import responses
@@ -49,10 +47,11 @@ def test_dimail_synchronization__already_sync():
                         "givenName": mailbox.first_name,
                         "surName": mailbox.last_name,
                         "displayName": f"{mailbox.first_name} {mailbox.last_name}",
+                        "additionalSenders": [],
                     }
                     for mailbox in pre_sync_mailboxes
                 ]
-            ),
+            ).replace("'", '"'),
             status=status.HTTP_200_OK,
             content_type="application/json",
         )
@@ -64,10 +63,11 @@ def test_dimail_synchronization__already_sync():
     assert set(models.Mailbox.objects.filter(domain=domain)) == set(pre_sync_mailboxes)
 
 
-@mock.patch.object(Logger, "warning")
-def test_dimail_synchronization__synchronize_mailboxes(mock_warning):
-    """A mailbox existing solely on dimail should be synchronized
-    upon calling sync function on its domain"""
+def test_dimail_synchronization__synchronize_mailboxes(caplog):
+    """A mailbox existing only on dimail should be synchronized
+    upon calling sync function on its domain."""
+    caplog.set_level(logging.INFO)
+
     domain = factories.MailDomainEnabledFactory()
     assert not models.Mailbox.objects.exists()
 
@@ -89,6 +89,16 @@ def test_dimail_synchronization__synchronize_mailboxes(mock_warning):
             "givenName": "Admin",
             "surName": "Context",
             "displayName": "Context Admin",
+            "additionalSenders": [],
+        }
+        mailbox_valid_additional_senders = {
+            "type": "mailbox",
+            "status": "broken",
+            "email": f"prudence_crandall@{domain.name}",
+            "givenName": "Admin",
+            "surName": "Context",
+            "displayName": "Context Admin",
+            "additionalSenders": ["[email protected]"],
         }
         mailbox_with_wrong_domain = {
             "type": "mailbox",
@@ -98,7 +108,7 @@ def test_dimail_synchronization__synchronize_mailboxes(mock_warning):
             "surName": "Doe",
             "displayName": "John Doe",
         }
-        mailbox_with_invalid_domain = {
+        mailbox_invalid_domain = {
             "type": "mailbox",
             "status": "broken",
             "email": f"naw@ake@{domain.name}",
@@ -114,43 +124,65 @@ def test_dimail_synchronization__synchronize_mailboxes(mock_warning):
             "surName": "Vang",
             "displayName": "Jean Vang",
         }
+        import json
+        null = None
+        mailbox_null_names = {
+            "type": "mailbox",
+            "status": "broken",
+            "email": f"null@{domain.name}",
+            "givenName": null,
+            "surName": null,
+            "displayName": null,
+        }
 
         rsps.add(
             rsps.GET,
             re.compile(rf".*/domains/{domain.name}/mailboxes/"),
             body=str(
                 [
                     mailbox_valid,
-                    mailbox_with_wrong_domain,
-                    mailbox_with_invalid_domain,
-                    mailbox_with_invalid_local_part,
+                    mailbox_valid_additional_senders,
+                    # mailbox_with_wrong_domain,
+                    # mailbox_invalid_domain,
+                    # mailbox_with_invalid_local_part,
+                    mailbox_null_names,
                 ]
-            ),
+            ).replace("'", '"'),
             status=status.HTTP_200_OK,
             content_type="application/json",
         )
 
         imported_mailboxes = dimail_client.import_mailboxes(domain)
 
-        # 3 imports failed: wrong domain, HeaderParseError, NonASCIILocalPartDefect
-        assert mock_warning.call_count == 3
-
-        # first we try to import email with a wrong domain
-        assert mock_warning.call_args_list[0][0] == (
-            "Import of email %s failed because of a wrong domain",
-            mailbox_with_wrong_domain["email"],
-        )
-
-        # then we try to import email with invalid domain
-        invalid_mailbox_log = mock_warning.call_args_list[1][0]
-        assert invalid_mailbox_log[1] == mailbox_with_invalid_domain["email"]
-        assert isinstance(invalid_mailbox_log[2], HeaderParseError)
-
-        # finally we try to import email with non ascii local part
-        non_ascii_mailbox_log = mock_warning.call_args_list[2][0]
-        assert non_ascii_mailbox_log[1] == mailbox_with_invalid_local_part["email"]
-        assert isinstance(non_ascii_mailbox_log[2], NonASCIILocalPartDefect)
-
-        mailbox = models.Mailbox.objects.get()
-        assert mailbox.local_part == "oxadmin"
-        assert imported_mailboxes == [mailbox_valid["email"]]
+    # 1 token requested
+    # + 3 imports failed: wrong domain, HeaderParseError, NonASCIILocalPartDefect
+    assert len(caplog.records) == 4
+
+    # first we try to import email with a wrong domain
+    assert (
+        caplog.records[1].message
+        == f"Import of email {mailbox_with_wrong_domain["email"]} failed because of a wrong domain"
+    )
+
+    # then we try to import email with invalid domain
+    invalid_mailbox_log = caplog.records[2]
+    assert (
+        invalid_mailbox_log.message
+        == f"Import of email {mailbox_invalid_domain['email']} failed with error Invalid Domain"
+    )
+
+    # then we try to import email with non ascii local part
+    non_ascii_mailbox_log = caplog.records[3]
+    email = mailbox_with_invalid_local_part["email"]
+    assert (
+        non_ascii_mailbox_log.message
+        == f"Import of email {email} failed with error local-part contains non-ASCII characters)"
+    )
+
+    assert imported_mailboxes == [
+        mailbox_valid["email"],
+        mailbox_valid_additional_senders["email"],
+    ]
+    mailboxes = models.Mailbox.objects.all()
+    assert mailboxes[0].local_part == "oxadmin"
+    assert mailboxes[1].local_part == "prudence_crandall"

src/backend/mailbox_manager/utils/dimail.py

@@ -1,6 +1,5 @@
 """A minimalist client to synchronize with mailbox provisioning API."""
 
-import ast
 import json
 import smtplib
 from email.errors import HeaderParseError, NonASCIILocalPartDefect
@@ -312,9 +311,14 @@ def import_mailboxes(self, domain):
         if response.status_code != status.HTTP_200_OK:
             return self.raise_exception_for_unexpected_response(response)
 
-        dimail_mailboxes = ast.literal_eval(
-            response.content.decode("utf-8")
-        )  # format output str to proper list
+        try:
+            import pdb; pdb.set_trace()
+        except requests.exceptions.JSONDecodeError as err:
+            logger.error(
+                "Import failed because of wrong json format: %s",
+                err,
+            )
+            return []
 
         people_mailboxes = models.Mailbox.objects.filter(domain=domain)
         imported_mailboxes = []
@@ -326,7 +330,10 @@ def import_mailboxes(self, domain):
                     # sometimes dimail api returns email from another domain,
                     # so we decide to exclude this kind of email
                     address = Address(addr_spec=dimail_mailbox["email"])
-                    if address.domain == domain.name:
+                    if (
+                        address.domain == domain.name
+                        and dimail_mailbox["givenName"] is not None
+                    ):
                         # creates a mailbox on our end
                         mailbox = models.Mailbox.objects.create(
                             first_name=dimail_mailbox["givenName"],