Skip to content

Commit

Permalink
🐛(dimail) fix unexpected status_code for proper debug
Browse files Browse the repository at this point in the history
Remove duplicate and catch errors more gracefully. Fixes tests accordingly.
  • Loading branch information
mjeammet authored and Laurent Bossavit committed Nov 5, 2024
1 parent f9ae0fd commit 3617ab6
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 15 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ and this project adheres to
- 🔇(backend) remove Sentry duplicated warning/errors
- 👷(ci) add sharding e2e tests #467

### Fixed

- 🐛(dimail) fix unexpected status_code for proper debug #454

## [1.4.1] - 2024-10-23

### Fixed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

import pytest
import responses
from requests.exceptions import HTTPError
from rest_framework import status
from rest_framework.test import APIClient

Expand Down Expand Up @@ -500,8 +501,7 @@ def test_api_mailboxes__user_unrelated_to_domain():
assert not models.Mailbox.objects.exists()


@mock.patch.object(Logger, "error")
def test_api_mailboxes__handling_dimail_unexpected_error(mock_error):
def test_api_mailboxes__handling_dimail_unexpected_error(caplog):
"""
API should raise a clear error when dimail returns an unexpected response.
"""
Expand All @@ -526,12 +526,12 @@ def test_api_mailboxes__handling_dimail_unexpected_error(mock_error):
rsps.add(
rsps.POST,
re.compile(rf".*/domains/{access.domain.name}/mailboxes/"),
body='{"details": "Internal server error"}',
body='{"detail": "Internal server error"}',
status=status.HTTP_500_INTERNAL_SERVER_ERROR,
content_type="application/json",
)

with pytest.raises(SystemError):
with pytest.raises(HTTPError):
response = client.post(
f"/api/v1.0/mail-domains/{access.domain.slug}/mailboxes/",
mailbox_data,
Expand All @@ -544,10 +544,9 @@ def test_api_mailboxes__handling_dimail_unexpected_error(mock_error):
assert not models.Mailbox.objects.exists()

# Check error logger was called
assert mock_error.called
assert mock_error.call_args_list[1][0] == (
'Unexpected response from dimail: 500 {"details": "Internal server error"}',
)
assert len(caplog.records) == 2 # 1 + new empty info. To be investigated
assert caplog.records[0].levelname == "ERROR"
assert caplog.records[0].message == "[DIMAIL] 500: Internal server error"


@mock.patch.object(Logger, "error")
Expand Down
17 changes: 10 additions & 7 deletions src/backend/mailbox_manager/utils/dimail.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""A minimalist client to synchronize with mailbox provisioning API."""

import json
import smtplib
from logging import getLogger

Expand All @@ -10,6 +11,7 @@
from django.utils.translation import gettext_lazy as _

import requests
from requests.exceptions import HTTPError
from rest_framework import status
from urllib3.util import Retry

Expand Down Expand Up @@ -152,14 +154,15 @@ def send_mailbox_request(self, mailbox, user_sub=None):

def pass_dimail_unexpected_response(self, response):
"""Raise error when encountering an unexpected error in dimail."""
error_content = response.content.decode("utf-8")
error_content = json.loads(response.content.decode("utf-8").replace("'", '"'))

logger.error(
"[DIMAIL] unexpected error : %s %s", response.status_code, error_content
)
raise SystemError(
f"Unexpected response from dimail: {response.status_code} {error_content}"
)
# catch error just to add failing interop name
try:
response.raise_for_status()
except HTTPError as error:
raise HTTPError(
f"[DIMAIL] {response.status_code}: {error_content['detail']}"
) from error

def send_new_mailbox_notification(self, recipient, mailbox_data):
"""
Expand Down

0 comments on commit 3617ab6

Please sign in to comment.