diff --git a/backend/alembic/versions/f6a7b8c9d0e1_add_mail_source_import_details.py b/backend/alembic/versions/f6a7b8c9d0e1_add_mail_source_import_details.py new file mode 100644 index 0000000..616a9c1 --- /dev/null +++ b/backend/alembic/versions/f6a7b8c9d0e1_add_mail_source_import_details.py @@ -0,0 +1,27 @@ +"""add mail source import details + +Revision ID: f6a7b8c9d0e1 +Revises: e5f6a7b8c9d0 +Create Date: 2026-05-22 20:05:00.000000 + +""" +from typing import Sequence, Union + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "f6a7b8c9d0e1" +down_revision: Union[str, Sequence[str], None] = "e5f6a7b8c9d0" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + """Add a sanitized details payload to import history rows.""" + op.add_column("mail_source_imports", sa.Column("details", sa.Text(), nullable=True)) + + +def downgrade() -> None: + """Remove import details payloads.""" + op.drop_column("mail_source_imports", "details") diff --git a/backend/app/api/api_v1/endpoints/mail_sources.py b/backend/app/api/api_v1/endpoints/mail_sources.py index e1847de..785b093 100644 --- a/backend/app/api/api_v1/endpoints/mail_sources.py +++ b/backend/app/api/api_v1/endpoints/mail_sources.py @@ -121,6 +121,7 @@ class MailSourceImportResponse(BaseModel): error_count: int new_domains: List[str] errors: List[str] + details: List[Dict[str, str]] started_at: datetime finished_at: datetime created_at: datetime @@ -183,6 +184,24 @@ def _decode_json_list(value: Optional[str]) -> List[str]: return [str(item) for item in decoded] +def _decode_json_details(value: Optional[str]) -> List[Dict[str, str]]: + """Decode a sanitized JSON detail list stored on import history rows.""" + if not value: + return [] + try: + decoded = json.loads(value) + except (json.JSONDecodeError, TypeError): + return [] + if not isinstance(decoded, list): + return [] + + details: List[Dict[str, str]] = [] + for item in decoded: + if isinstance(item, dict): + details.append({str(key): str(val) for key, val in item.items()}) + return details + + def _import_to_response(row: MailSourceImport) -> MailSourceImportResponse: """Convert an import-history ORM row to an API response.""" return MailSourceImportResponse( @@ -196,6 +215,7 @@ def _import_to_response(row: MailSourceImport) -> MailSourceImportResponse: error_count=row.error_count, new_domains=_decode_json_list(row.new_domains), errors=_decode_json_list(row.errors), + details=_decode_json_details(row.details), started_at=row.started_at, finished_at=row.finished_at, created_at=row.created_at, diff --git a/backend/app/models/mail_source_import.py b/backend/app/models/mail_source_import.py index c9911b8..a7fa8d4 100644 --- a/backend/app/models/mail_source_import.py +++ b/backend/app/models/mail_source_import.py @@ -24,6 +24,7 @@ class MailSourceImport(Base): new_domains = Column(Text, nullable=True) errors = Column(Text, nullable=True) + details = Column(Text, nullable=True) started_at = Column(DateTime, nullable=False, default=datetime.utcnow, index=True) finished_at = Column(DateTime, nullable=False, default=datetime.utcnow, index=True) diff --git a/backend/app/services/gmail_client.py b/backend/app/services/gmail_client.py index 63cf941..7dd9396 100644 --- a/backend/app/services/gmail_client.py +++ b/backend/app/services/gmail_client.py @@ -214,6 +214,7 @@ class GmailClient: "new_domains": [], "errors": [], "new_ingested_ids": [], + "details": [], } try: @@ -232,6 +233,12 @@ class GmailClient: for msg_id in message_ids: if msg_id in self.already_ingested_ids: + self._append_detail( + stats, + status="skipped", + reason="already_ingested_message", + message_id=msg_id, + ) continue stats["processed"] += 1 @@ -292,6 +299,11 @@ class GmailClient: return ids + @staticmethod + def _append_detail(stats: dict, **detail: str) -> None: + """Append a compact attachment/message outcome to the import stats.""" + stats.setdefault("details", []).append({key: value for key, value in detail.items() if value}) + def _process_message(self, service, msg_id: str, stats: dict) -> int: """ Download a Gmail message and process any DMARC-report attachments. @@ -305,11 +317,17 @@ class GmailClient: except HttpError as exc: logger.error("Gmail API: failed to fetch message %s: %s", msg_id, exc) stats["errors"].append(f"Failed to fetch message {msg_id}") + self._append_detail( + stats, + status="error", + reason="message_fetch_failed", + message_id=msg_id, + ) return 0 raw_bytes = base64.urlsafe_b64decode(msg_data.get("raw", "")) msg = email.message_from_bytes(raw_bytes) - return self._process_attachments(msg, stats) + return self._process_attachments(msg, stats, message_id=msg_id) @staticmethod def _decode_part_filename(part: email.message.Message) -> str: @@ -352,33 +370,81 @@ class GmailClient: self.report_store.add_report(report) return True - def _process_attachments(self, msg: email.message.Message, stats: dict) -> int: + def _process_attachments( + self, + msg: email.message.Message, + stats: dict, + message_id: Optional[str] = None, + ) -> int: """Walk a parsed email message and extract DMARC report attachments.""" reports_found = 0 for part in msg.walk(): filename = self._decode_part_filename(part) - if not filename or not self._is_dmarc_attachment(filename): + if not filename: continue disposition = part.get_content_disposition() if disposition not in ("attachment", None): continue + if not self._is_dmarc_attachment(filename): + self._append_detail( + stats, + status="skipped", + reason="unsupported_attachment", + message_id=message_id, + filename=filename, + ) + continue + content = part.get_payload(decode=True) if not content: + self._append_detail( + stats, + status="skipped", + reason="empty_attachment", + message_id=message_id, + filename=filename, + ) continue try: report = DMARCParser.parse_file(content, filename) + domain = str(report.get("domain", "unknown")) + report_id = str(report.get("report_id", "")) if self._store_report_if_new(report): stats["reports_found"] += 1 reports_found += 1 + self._append_detail( + stats, + status="imported", + message_id=message_id, + filename=filename, + domain=domain, + report_id=report_id, + ) else: stats["duplicate_reports"] = stats.get("duplicate_reports", 0) + 1 + self._append_detail( + stats, + status="duplicate", + message_id=message_id, + filename=filename, + domain=domain, + report_id=report_id, + ) except Exception as exc: # pylint: disable=broad-exception-caught logger.error("Failed to parse DMARC attachment %s: %s", filename, exc) stats["errors"].append(f"Failed to parse {filename}: {exc}") + self._append_detail( + stats, + status="error", + reason="parse_failed", + message_id=message_id, + filename=filename, + error=str(exc), + ) return reports_found diff --git a/backend/app/services/imap_client.py b/backend/app/services/imap_client.py index 34d68f1..71f3222 100644 --- a/backend/app/services/imap_client.py +++ b/backend/app/services/imap_client.py @@ -140,17 +140,24 @@ class IMAPClient: def _process_single_email(self, mail, email_id: bytes, stats: dict) -> None: """Fetch, parse, and store DMARC attachments from one email message.""" + message_id = email_id.decode("utf-8", errors="replace") try: status, msg_data = mail.fetch(email_id, "(RFC822)") if status != "OK": logger.error("Error fetching email ID %s", email_id) + self._append_detail( + stats, + status="error", + reason="message_fetch_failed", + message_id=message_id, + ) return raw_email = msg_data[0][1] msg = email.message_from_bytes(raw_email) if self._is_dmarc_report_email(msg): - reports_found = self._process_attachments(msg, stats) + reports_found = self._process_attachments(msg, stats, message_id=message_id) stats["reports_found"] += reports_found # Mark email as read (and optionally delete) @@ -163,6 +170,13 @@ class IMAPClient: error_msg = f"Error processing email ID {email_id}: {str(e)}" logger.error(error_msg) stats["errors"].append(error_msg) + self._append_detail( + stats, + status="error", + reason="message_processing_failed", + message_id=message_id, + error=str(e), + ) def fetch_reports(self, days: int = 7) -> Dict[str, Any]: """ @@ -185,6 +199,7 @@ class IMAPClient: "duplicate_reports": 0, "new_domains": [], "errors": [], + "details": [], } try: @@ -334,12 +349,7 @@ class IMAPClient: filename = self._decode_email_header(filename) # Check file extension - if ( - filename.lower().endswith(".xml") - or filename.lower().endswith(".zip") - or filename.lower().endswith(".gz") - or filename.lower().endswith(".gzip") - ): + if self._is_dmarc_filename(filename): return True # Check content type @@ -355,7 +365,115 @@ class IMAPClient: return False - def _process_attachments(self, msg: email.message.Message, stats: dict | None = None) -> int: + @staticmethod + def _is_dmarc_filename(filename: str) -> bool: + lower = filename.lower() + return ( + lower.endswith(".xml") + or lower.endswith(".zip") + or lower.endswith(".gz") + or lower.endswith(".gzip") + ) + + @staticmethod + def _append_detail(stats: dict | None, **detail: str) -> None: + """Append a compact attachment/message outcome to the import stats.""" + if stats is None: + return + stats.setdefault("details", []).append( + {key: value for key, value in detail.items() if value} + ) + + def _store_report_if_new( + self, + report: Dict[str, Any], + *, + filename: str, + stats: dict | None, + message_id: str | None, + ) -> bool: + domain = report.get("domain", "unknown") + report_id = report.get("report_id", "") + if report_id and ( + self.report_store.has_report(domain, report_id) + or (self.db is not None and report_exists(self.db, domain, report_id)) + ): + logger.info("Skipping duplicate DMARC report %s for %s", report_id, domain) + if stats is not None: + stats["duplicate_reports"] = stats.get("duplicate_reports", 0) + 1 + self._append_detail( + stats, + status="duplicate", + message_id=message_id, + filename=filename, + domain=str(domain), + report_id=str(report_id), + ) + return False + + if self.db is not None: + save_parsed_report(self.db, report) + self.report_store.add_report(report) + self._append_detail( + stats, + status="imported", + message_id=message_id, + filename=filename, + domain=str(domain), + report_id=str(report_id), + ) + return True + + def _process_dmarc_attachment( + self, + part: email.message.Message, + *, + filename: str, + stats: dict | None, + message_id: str | None, + ) -> bool: + try: + content = part.get_payload(decode=True) + if not content: + self._append_detail( + stats, + status="skipped", + reason="empty_attachment", + message_id=message_id, + filename=filename, + ) + return False + + report = DMARCParser.parse_file(content, filename) + stored = self._store_report_if_new( + report, + filename=filename, + stats=stats, + message_id=message_id, + ) + if stored: + logger.info("Successfully processed DMARC report: %s", filename) + return stored + except Exception as exc: # pylint: disable=broad-exception-caught + logger.error("Error processing attachment %s: %s", filename, str(exc)) + if stats is not None: + stats.setdefault("errors", []).append(f"Failed to parse {filename}: {exc}") + self._append_detail( + stats, + status="error", + reason="parse_failed", + message_id=message_id, + filename=filename, + error=str(exc), + ) + return False + + def _process_attachments( + self, + msg: email.message.Message, + stats: dict | None = None, + message_id: str | None = None, + ) -> int: """ Process email attachments that might be DMARC reports @@ -368,57 +486,30 @@ class IMAPClient: reports_found = 0 for part in msg.walk(): - content_disposition = part.get_content_disposition() + if part.get_content_disposition() != "attachment": + continue - if content_disposition == "attachment": - filename = part.get_filename() - if filename: - # Decode filename if needed - filename = self._decode_email_header(filename) + filename = part.get_filename() + if not filename: + continue - # Check if it's a likely DMARC report file - if ( - filename.lower().endswith(".xml") - or filename.lower().endswith(".zip") - or filename.lower().endswith(".gz") - or filename.lower().endswith(".gzip") - ): + filename = self._decode_email_header(filename) + if not self._is_dmarc_filename(filename): + self._append_detail( + stats, + status="skipped", + reason="unsupported_attachment", + message_id=message_id, + filename=filename, + ) + continue - try: - # Get attachment content - content = part.get_payload(decode=True) - - # Parse the DMARC report - report = DMARCParser.parse_file(content, filename) - - domain = report.get("domain", "unknown") - report_id = report.get("report_id", "") - if report_id and ( - self.report_store.has_report(domain, report_id) - or ( - self.db is not None - and report_exists(self.db, domain, report_id) - ) - ): - logger.info( - "Skipping duplicate DMARC report %s for %s", - report_id, - domain, - ) - if stats is not None: - stats["duplicate_reports"] = ( - stats.get("duplicate_reports", 0) + 1 - ) - continue - - # Add the report to the store - if self.db is not None: - save_parsed_report(self.db, report) - self.report_store.add_report(report) - - reports_found += 1 - logger.info("Successfully processed DMARC report: %s", filename) - except Exception as e: # pylint: disable=broad-exception-caught - logger.error("Error processing attachment %s: %s", filename, str(e)) + if self._process_dmarc_attachment( + part, + filename=filename, + stats=stats, + message_id=message_id, + ): + reports_found += 1 return reports_found diff --git a/backend/app/services/import_history.py b/backend/app/services/import_history.py index bac1df6..12112a7 100644 --- a/backend/app/services/import_history.py +++ b/backend/app/services/import_history.py @@ -9,13 +9,24 @@ from app.models.mail_source_import import MailSourceImport MAX_STORED_ERRORS = 10 MAX_ERROR_LENGTH = 500 +MAX_STORED_DETAILS = 50 +MAX_DETAIL_VALUE_LENGTH = 300 +DETAIL_FIELDS = { + "status", + "reason", + "message_id", + "filename", + "domain", + "report_id", + "error", +} def _sanitize_error(value: object) -> str: """Return a compact, log-safe error string for storage and UI display.""" text = str(value).replace("\r", "").replace("\n", " ").strip() if len(text) > MAX_ERROR_LENGTH: - return text[: MAX_ERROR_LENGTH - 1] + "..." + return text[: MAX_ERROR_LENGTH - 3] + "..." return text @@ -23,6 +34,27 @@ def _json_list(values: Optional[Iterable[Any]]) -> str: return json.dumps([str(value) for value in values or []]) +def _sanitize_detail_value(value: object) -> str: + text = _sanitize_error(value) + if len(text) > MAX_DETAIL_VALUE_LENGTH: + return text[: MAX_DETAIL_VALUE_LENGTH - 3] + "..." + return text + + +def _json_details(values: Optional[Iterable[Any]]) -> str: + details = [] + for value in list(values or [])[:MAX_STORED_DETAILS]: + if not isinstance(value, dict): + continue + entry = {} + for key in DETAIL_FIELDS: + if key in value and value[key] not in (None, ""): + entry[key] = _sanitize_detail_value(value[key]) + if entry: + details.append(entry) + return json.dumps(details) + + def record_import_attempt( db: Session, source: MailSource, @@ -47,6 +79,7 @@ def record_import_attempt( error_count=len(result_errors), new_domains=_json_list(results.get("new_domains", [])), errors=json.dumps(errors), + details=_json_details(results.get("details", [])), started_at=started_at, finished_at=datetime.utcnow(), ) diff --git a/backend/app/templates/mail_sources.html b/backend/app/templates/mail_sources.html index 34cf85a..100293f 100644 --- a/backend/app/templates/mail_sources.html +++ b/backend/app/templates/mail_sources.html @@ -178,6 +178,7 @@ Duplicates New Domains Errors + Details @@ -207,6 +208,32 @@ None + + + + @@ -579,6 +606,17 @@ function mailSourcesApp() { return value && value.length ? value.join(', ') : '—'; }, + formatDetailsSummary(details) { + const counts = details.reduce((acc, detail) => { + const key = detail.status || 'unknown'; + acc[key] = (acc[key] || 0) + 1; + return acc; + }, {}); + return Object.entries(counts) + .map(([key, count]) => `${count} ${key}`) + .join(', '); + }, + statusBadgeClass(status) { if (status === 'success') return 'badge-success'; if (status === 'warning') return 'badge-warning'; @@ -586,6 +624,13 @@ function mailSourcesApp() { return 'badge-outline'; }, + detailBadgeClass(status) { + if (status === 'imported') return 'badge-success'; + if (status === 'duplicate' || status === 'skipped') return 'badge-warning'; + if (status === 'error') return 'badge-error'; + return 'badge-outline'; + }, + openAddForm() { this.editingId = null; this.gmailConnected = false; diff --git a/backend/app/tests/test_gmail_client.py b/backend/app/tests/test_gmail_client.py index 724aab3..f9e6dfd 100644 --- a/backend/app/tests/test_gmail_client.py +++ b/backend/app/tests/test_gmail_client.py @@ -439,6 +439,8 @@ class TestProcessMessage: assert count == 0 assert len(stats["errors"]) == 1 assert "bad-id" in stats["errors"][0] + assert stats["details"][0]["status"] == "error" + assert stats["details"][0]["message_id"] == "bad-id" # =========================================================================== @@ -463,9 +465,36 @@ class TestProcessAttachments: raw = _make_raw_email([{"filename": "photo.png", "content": b"\x89PNG"}]) msg = email_mod.message_from_bytes(raw) stats = {"reports_found": 0, "errors": []} - count = client._process_attachments(msg, stats) + count = client._process_attachments(msg, stats, message_id="msg-1") assert count == 0 assert stats["reports_found"] == 0 + assert stats["details"] == [ + { + "status": "skipped", + "reason": "unsupported_attachment", + "message_id": "msg-1", + "filename": "photo.png", + } + ] + + def test_inline_dmarc_attachment_is_skipped(self): + client = _make_client() + raw = _make_raw_email( + [ + { + "filename": "report.xml", + "content": SAMPLE_XML.encode(), + "disposition": "inline", + } + ] + ) + msg = email_mod.message_from_bytes(raw) + stats = {"reports_found": 0, "errors": []} + + count = client._process_attachments(msg, stats) + + assert count == 0 + assert stats.get("details") is None def test_google_style_zip_attachment_is_parsed(self): """A Google DMARC ZIP attachment is parsed and counted.""" @@ -485,6 +514,9 @@ class TestProcessAttachments: assert count == 1 assert stats["reports_found"] == 1 + assert stats["details"][0]["status"] == "imported" + assert stats["details"][0]["filename"].endswith(".zip") + assert stats["details"][0]["report_id"] == "123456789" assert "example.com" in client.report_store.get_domains() def test_google_style_zip_attachment_is_persisted(self, db_session): @@ -524,6 +556,8 @@ class TestProcessAttachments: assert client._process_attachments(msg, first_stats) == 1 assert client._process_attachments(msg, second_stats) == 0 + assert second_stats["details"][0]["status"] == "duplicate" + assert second_stats["details"][0]["report_id"] == "123456789" assert client.report_store.get_domain_summary("example.com")["reports_processed"] == 1 def test_dmarc_attachment_with_empty_content_skipped(self): @@ -537,6 +571,8 @@ class TestProcessAttachments: # Empty payload → `get_payload(decode=True)` returns b"" which is # falsy, so the attachment is skipped assert count == 0 + assert stats["details"][0]["status"] == "skipped" + assert stats["details"][0]["reason"] == "empty_attachment" def test_parse_exception_adds_error_and_continues(self): """A parse error should be recorded in stats but not raise.""" @@ -567,6 +603,9 @@ class TestProcessAttachments: assert len(stats["errors"]) == 1 assert "bad.xml" in stats["errors"][0] + assert stats["details"][0]["status"] == "error" + assert stats["details"][0]["filename"] == "bad.xml" + assert stats["details"][1]["status"] == "imported" assert count == 1 # second attachment still parsed @@ -622,6 +661,9 @@ class TestFetchReports: call_args = mock_proc.call_args_list[0][0] assert call_args[1] == "id2" assert result["processed"] == 1 + assert result["details"][0]["status"] == "skipped" + assert result["details"][0]["reason"] == "already_ingested_message" + assert result["details"][0]["message_id"] == "id1" def test_tracks_new_ingested_ids(self): client = _make_client() diff --git a/backend/app/tests/test_imap_client.py b/backend/app/tests/test_imap_client.py index 602c9d7..61a5e19 100644 --- a/backend/app/tests/test_imap_client.py +++ b/backend/app/tests/test_imap_client.py @@ -427,8 +427,12 @@ class TestProcessAttachments: msg = email.message_from_bytes( _make_email_with_attachment("report.xml", MINIMAL_DMARC_XML, "application/xml") ) - count = client._process_attachments(msg) + stats = {"processed": 0, "reports_found": 0, "errors": []} + count = client._process_attachments(msg, stats, message_id="1") assert count == 1 + assert stats["details"][0]["status"] == "imported" + assert stats["details"][0]["message_id"] == "1" + assert stats["details"][0]["report_id"] == "abc-123" def test_processes_zip_attachment(self): client = self._make_client() @@ -450,12 +454,73 @@ class TestProcessAttachments: assert count == 1 assert db_session.query(DMARCReport).filter_by(report_id="abc-123").count() == 1 + def test_duplicate_report_adds_detail(self): + client = self._make_client() + msg = email.message_from_bytes( + _make_email_with_attachment("report.xml", MINIMAL_DMARC_XML, "application/xml") + ) + first_stats = {"processed": 0, "reports_found": 0, "errors": []} + second_stats = {"processed": 0, "reports_found": 0, "errors": []} + + assert client._process_attachments(msg, first_stats) == 1 + assert client._process_attachments(msg, second_stats) == 0 + assert second_stats["details"][0]["status"] == "duplicate" + assert second_stats["details"][0]["report_id"] == "abc-123" + def test_bad_attachment_does_not_raise(self): client = self._make_client() msg = email.message_from_bytes(_make_email_with_attachment("report.xml", b"not xml at all")) # Should not raise; just returns 0 - count = client._process_attachments(msg) + stats = {"processed": 0, "reports_found": 0, "errors": []} + count = client._process_attachments(msg, stats) assert count == 0 + assert stats["errors"] + assert stats["details"][0]["status"] == "error" + assert stats["details"][0]["filename"] == "report.xml" + + def test_empty_attachment_adds_detail(self): + client = self._make_client() + msg = email.message_from_bytes(_make_email_with_attachment("report.xml", b"")) + stats = {"processed": 0, "reports_found": 0, "errors": []} + + count = client._process_attachments(msg, stats) + + assert count == 0 + assert stats["details"][0]["status"] == "skipped" + assert stats["details"][0]["reason"] == "empty_attachment" + + def test_unsupported_attachment_adds_detail(self): + client = self._make_client() + msg = email.message_from_bytes( + _make_email_with_attachment("notes.txt", b"not xml", "text/plain") + ) + stats = {"processed": 0, "reports_found": 0, "errors": []} + + count = client._process_attachments(msg, stats, message_id="2") + + assert count == 0 + assert stats["details"] == [ + { + "status": "skipped", + "reason": "unsupported_attachment", + "message_id": "2", + "filename": "notes.txt", + } + ] + + def test_attachment_without_filename_is_skipped(self): + client = self._make_client() + msg = MIMEMultipart() + msg.attach(MIMEText("body")) + part = MIMEApplication(MINIMAL_DMARC_XML) + part["Content-Disposition"] = "attachment" + msg.attach(part) + stats = {"processed": 0, "reports_found": 0, "errors": []} + + count = client._process_attachments(msg, stats) + + assert count == 0 + assert stats.get("details") is None def test_no_attachments_returns_zero(self): client = self._make_client() @@ -497,6 +562,7 @@ class TestProcessSingleEmail: assert stats["processed"] == 1 assert stats["reports_found"] == 1 + assert stats["details"][0]["status"] == "imported" def test_fetch_error_skips_email(self): client = self._make_client() @@ -507,6 +573,8 @@ class TestProcessSingleEmail: client._process_single_email(mock_mail, b"1", stats) assert stats["processed"] == 0 + assert stats["details"][0]["status"] == "error" + assert stats["details"][0]["reason"] == "message_fetch_failed" def test_exception_adds_to_errors(self): client = self._make_client() @@ -517,6 +585,7 @@ class TestProcessSingleEmail: client._process_single_email(mock_mail, b"1", stats) assert len(stats["errors"]) == 1 + assert stats["details"][0]["reason"] == "message_processing_failed" def test_marks_deleted_when_flag_set(self): client = self._make_client() @@ -599,6 +668,7 @@ class TestFetchReports: assert result["success"] is True assert result["reports_found"] >= 1 + assert result["details"][0]["status"] == "imported" def test_connection_error_returns_failure(self): client = self._make_client() diff --git a/backend/app/tests/test_mail_sources.py b/backend/app/tests/test_mail_sources.py index 69b220f..1c6d87c 100644 --- a/backend/app/tests/test_mail_sources.py +++ b/backend/app/tests/test_mail_sources.py @@ -3,6 +3,8 @@ Tests for MailSource model and mail-sources API endpoints. """ import asyncio +import json +from datetime import datetime from unittest.mock import MagicMock, patch from urllib.parse import parse_qs, urlparse @@ -12,6 +14,7 @@ from sqlalchemy.orm import Session from app.models.mail_source import MailSource from app.models.mail_source_import import MailSourceImport +from app.services.import_history import record_import_attempt class TestMailSourceModel: @@ -95,6 +98,7 @@ class TestMailSourceImportModel: error_count=1, new_domains='["example.com"]', errors='["bad attachment"]', + details='[{"status": "imported", "filename": "report.xml"}]', ) db_session.add(row) db_session.commit() @@ -103,8 +107,51 @@ class TestMailSourceImportModel: assert row.id is not None assert row.mail_source_id == source.id assert row.duplicate_reports == 1 + assert '"imported"' in row.details assert row.mail_source.name == "History Source" + def test_record_import_attempt_sanitizes_details(self, db_session: Session): + source = MailSource(name="Detail Sanitizer", method="IMAP") + db_session.add(source) + db_session.commit() + db_session.refresh(source) + + attempt = record_import_attempt( + db_session, + source, + { + "success": True, + "errors": ["x" * 600], + "details": [ + "skip-me", + {"status": "imported", "filename": "a" * 400, "ignored": "secret"}, + ], + }, + started_at=datetime.utcnow(), + trigger="manual", + ) + + details = json.loads(attempt.details) + errors = json.loads(attempt.errors) + assert len(errors[0]) == 500 + assert details[0]["status"] == "imported" + assert len(details[0]["filename"]) == 300 + assert "ignored" not in details[0] + + +class TestImportHistoryDecoding: + """Unit tests for import-history JSON decoding helpers.""" + + def test_decode_details_handles_empty_and_malformed_values(self): + from app.api.api_v1.endpoints.mail_sources import _decode_json_details + + assert _decode_json_details(None) == [] + assert _decode_json_details("not-json") == [] + assert _decode_json_details('{"not": "a list"}') == [] + assert _decode_json_details('["skip", {"status": "imported", "report_id": 123}]') == [ + {"status": "imported", "report_id": "123"} + ] + class TestMailSourcesAPI: """Integration tests for /api/v1/mail-sources endpoints (no auth).""" @@ -267,6 +314,7 @@ class TestMailSourcesAPIAuthed: error_count=1, new_domains='["example.com"]', errors='["sanitized error"]', + details='[{"status": "duplicate", "report_id": "abc-123"}]', ) ) db_session.commit() @@ -281,6 +329,7 @@ class TestMailSourcesAPIAuthed: assert data[0]["duplicate_reports"] == 1 assert data[0]["new_domains"] == ["example.com"] assert data[0]["errors"] == ["sanitized error"] + assert data[0]["details"] == [{"status": "duplicate", "report_id": "abc-123"}] def test_list_import_history_handles_malformed_json( self, authed_client: TestClient, db_session: Session @@ -297,6 +346,7 @@ class TestMailSourcesAPIAuthed: status="warning", new_domains="not-json", errors='{"not": "a list"}', + details='{"not": "a list"}', ) ) db_session.commit() @@ -306,6 +356,7 @@ class TestMailSourcesAPIAuthed: assert resp.status_code == 200 assert resp.json()[0]["new_domains"] == [] assert resp.json()[0]["errors"] == [] + assert resp.json()[0]["details"] == [] def test_list_import_history_unknown_source_returns_404(self, authed_client: TestClient): resp = authed_client.get("/api/v1/mail-sources/99999/imports") @@ -764,6 +815,7 @@ class TestManualSourceFetchEndpoint: "duplicate_reports": 1, "new_domains": ["example.com"], "errors": ["bad attachment\nwith newline"], + "details": [{"status": "error", "filename": "bad.xml"}], } with ( @@ -780,6 +832,10 @@ class TestManualSourceFetchEndpoint: assert data["duplicate_reports"] == 1 assert data["error_count"] == 1 assert "bad attachment with newline" in caplog.text + history_resp = authed_client.get(f"/api/v1/mail-sources/{source_id}/imports") + assert history_resp.json()[0]["details"] == [ + {"status": "error", "filename": "bad.xml"} + ] mock_imap.fetch_reports.assert_called_once_with(days=30) def test_fetch_gmail_source(self, authed_client: TestClient, db_session: Session): @@ -807,6 +863,7 @@ class TestManualSourceFetchEndpoint: "new_domains": [], "errors": [], "new_ingested_ids": ["id1"], + "details": [{"status": "imported", "report_id": "abc-123"}], } mock_gmail.get_refreshed_tokens.return_value = None diff --git a/docs/development/roadmap.md b/docs/development/roadmap.md index d1696a1..b5900b9 100644 --- a/docs/development/roadmap.md +++ b/docs/development/roadmap.md @@ -28,6 +28,7 @@ Recently improved: - Report/domain API reads can hydrate the dashboard projection from persisted data after restart. - Mail source import history is visible in the Mail Sources UI. - Individual mail sources can be manually imported from the Mail Sources UI. +- Import-history rows include sanitized per-attachment outcomes and imported report IDs. - The current Alpine-based UI is allowed by CSP and renders dynamic tables in real browsers. Implementation note: @@ -38,9 +39,8 @@ Implementation note: Objective: make mailbox imports auditable and make report totals trustworthy. Priority tasks: -- Expand import attempts with message ID, source, attachment filename, outcome, and sanitized error details. +- Add mailbox search controls for date-range backfills. - Report duplicate skips separately from parse failures. -- Add backfill controls for Gmail and IMAP sources. - Improve source rollups so a source IP tracks pass/fail counts over time. Quality bar: diff --git a/docs/milestones.md b/docs/milestones.md index 75e8fa0..3cc882e 100644 --- a/docs/milestones.md +++ b/docs/milestones.md @@ -63,10 +63,10 @@ Recently delivered: - Uploaded, Gmail-imported, and IMAP-imported reports are now persisted to report/record tables and can be reloaded into report/domain views. - Mail source import history is now visible from the Mail Sources UI. - A single mail source can be manually imported from the Mail Sources UI, with the result recorded in import history. +- Import history now includes per-attachment details for imported reports, duplicates, parse errors, unsupported attachments, and imported report IDs. - The current Alpine-based UI can run under the configured CSP, so dynamic tables render in real browsers. Next tasks: -- Add per-import result details: skipped duplicates, parse failures, unsupported attachments, and imported report IDs. - Add mailbox search controls for date range/backfill without requiring code changes. - Improve source aggregation so each sender IP keeps pass/fail totals instead of only the latest result. diff --git a/docs/todo.md b/docs/todo.md index 3028cba..7022883 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -92,6 +92,7 @@ This file tracks the specific implementation tasks for each milestone of the DMA - [x] Count duplicate skips separately from parse failures - [x] Show recent import history in the Mail Sources UI - [x] Add manual import trigger per mail source +- [x] Add per-import result details for duplicates, parse failures, unsupported attachments, and imported report IDs - [ ] Add retry/backfill controls per mail source ## Milestone 3: Database Integration