simplified validation (#91)

mobilizon_reshare/publishers/abstract.py

@@ -76,9 +76,12 @@ class AbstractPlatform(ABC, LoggerMixin, ConfLoaderMixin):
     # the second the name of its service (ie: 'facebook', 'telegram')
 
     def __repr__(self):
-        return type(self).__name__
+        return self.name
 
-    __str__ = __repr__
+    @property
+    @abstractmethod
+    def name(self):
+        pass
 
     @abstractmethod
     def _send(self, message: str):
@@ -99,13 +102,6 @@ class AbstractPlatform(ABC, LoggerMixin, ConfLoaderMixin):
     def _validate_response(self, response):
         raise NotImplementedError  # pragma: no cover
 
-    def are_credentials_valid(self) -> bool:
-        try:
-            self.validate_credentials()
-        except PublisherError:
-            return False
-        return True
-
     @abstractmethod
     def validate_credentials(self) -> None:
         """
@@ -146,14 +142,6 @@ class AbstractEventFormatter(LoggerMixin, ConfLoaderMixin):
         template_path = self.conf.msg_template_path or self.default_template_path
         return JINJA_ENV.get_template(template_path)
 
-    def is_message_valid(self, event: MobilizonEvent) -> bool:
-        # TODO: this thing swallows exception messages. It should be handled differently
-        try:
-            self.validate_message(self.get_message_from_event(event))
-        except PublisherError:
-            return False
-        return True
-
     @abstractmethod
     def validate_message(self, message: str) -> None:
         """
@@ -163,13 +151,6 @@ class AbstractEventFormatter(LoggerMixin, ConfLoaderMixin):
         """
         raise NotImplementedError  # pragma: no cover
 
-    def is_event_valid(self, event) -> bool:
-        try:
-            self.validate_event(event)
-        except PublisherError:
-            return False
-        return True
-
     def get_recap_header(self):
         template_path = (
             self.conf.recap_header_template_path
@@ -184,6 +165,9 @@ class AbstractEventFormatter(LoggerMixin, ConfLoaderMixin):
         return JINJA_ENV.get_template(template_path)
 
     def get_recap_fragment(self, event: MobilizonEvent) -> str:
+        """
+        Retrieves the fragment that describes a single event inside the event recap.
+        """
         event = self._preprocess_event(event)
         return event.format(self.get_recap_fragment_template())
 

mobilizon_reshare/publishers/coordinator.py

@@ -1,7 +1,6 @@
 import logging
 from dataclasses import dataclass
 from typing import List, Optional
-from uuid import UUID
 
 from mobilizon_reshare.models.publication import PublicationStatus
 from mobilizon_reshare.publishers import get_active_notifiers
@@ -30,13 +29,17 @@ class BasePublicationReport:
 
 @dataclass
 class EventPublicationReport(BasePublicationReport):
-    publication_id: UUID
+    publication: EventPublication
 
     def get_failure_message(self):
 
+        if not self.reason:
+            logger.error("Report of failure without reason.", exc_info=True)
+
         return (
-            f"Publication {self.publication_id } failed with status: {self.status}.\n"
-            f"Reason: {self.reason}"
+            f"Publication {self.publication.id} failed with status: {self.status}.\n"
+            f"Reason: {self.reason}\n"
+            f"Publisher: {self.publication.publisher.name}"
         )
 
 
@@ -72,9 +75,7 @@ class PublisherCoordinator:
     def _make_successful_report(self, failed_ids):
         return [
             EventPublicationReport(
-                status=PublicationStatus.COMPLETED,
-                reason="",
-                publication_id=publication.id,
+                status=PublicationStatus.COMPLETED, reason="", publication=publication,
             )
             for publication in self.publications
             if publication.id not in failed_ids
@@ -93,7 +94,7 @@ class PublisherCoordinator:
                 reports.append(
                     EventPublicationReport(
                         status=PublicationStatus.COMPLETED,
-                        publication_id=publication.id,
+                        publication=publication,
                         reason=None,
                     )
                 )
@@ -102,7 +103,7 @@ class PublisherCoordinator:
                     EventPublicationReport(
                         status=PublicationStatus.FAILED,
                         reason=str(e),
-                        publication_id=publication.id,
+                        publication=publication,
                     )
                 )
 
@@ -110,25 +111,37 @@ class PublisherCoordinator:
             publications=self.publications, reports=reports
         )
 
+    def _safe_run(self, reasons, f, *args, **kwargs):
+        try:
+            f(*args, **kwargs)
+            return reasons
+        except Exception as e:
+            logger.error(str(e))
+            return reasons + [str(e)]
+
     def _validate(self):
         errors = []
 
         for publication in self.publications:
+            reasons = []
+            reasons = self._safe_run(
+                reasons, publication.publisher.validate_credentials,
+            )
+            reasons = self._safe_run(
+                reasons, publication.formatter.validate_event, publication.event
+            )
+            reasons = self._safe_run(
+                reasons,
+                publication.formatter.validate_message,
+                publication.formatter.get_message_from_event(publication.event),
+            )
 
-            reason = []
-            if not publication.publisher.are_credentials_valid():
-                reason.append("Invalid credentials")
-            if not publication.formatter.is_event_valid(publication.event):
-                reason.append("Invalid event")
-            if not publication.formatter.is_message_valid(publication.event):
-                reason.append("Invalid message")
-
-            if len(reason) > 0:
+            if len(reasons) > 0:
                 errors.append(
                     EventPublicationReport(
                         status=PublicationStatus.FAILED,
-                        reason=", ".join(reason),
-                        publication_id=publication.id,
+                        reason=", ".join(reasons),
+                        publication=publication,
                     )
                 )
 

mobilizon_reshare/publishers/exceptions.py

@@ -32,9 +32,9 @@ class InvalidSettings(PublisherError):
     """ Publisher settings are either missing or badly configured """
 
 
-class HTTPResponseError(PublisherError):
-    """ Response received with an HTTP error status code"""
-
-
 class ZulipError(PublisherError):
     """ Publisher receives an error response from Zulip"""
+
+
+class HTTPResponseError(PublisherError):
+    """ Publisher receives a HTTP error"""

mobilizon_reshare/publishers/platforms/mastodon.py

@@ -1,6 +1,6 @@
-import pkg_resources
 from urllib.parse import urljoin
 
+import pkg_resources
 import requests
 from requests import Response
 
@@ -13,8 +13,8 @@ from mobilizon_reshare.publishers.exceptions import (
     InvalidBot,
     InvalidEvent,
     InvalidResponse,
-    PublisherError,
     HTTPResponseError,
+    InvalidMessage,
 )
 
 
@@ -40,7 +40,7 @@ class MastodonFormatter(AbstractEventFormatter):
 
     def validate_message(self, message) -> None:
         if len(message.encode("utf-8")) >= self.conf.toot_length:
-            raise PublisherError("Message is too long")
+            raise InvalidMessage("Message is too long")
 
 
 class MastodonPlatform(AbstractPlatform):
@@ -50,6 +50,7 @@ class MastodonPlatform(AbstractPlatform):
 
     _conf = ("publisher", "mastodon")
     api_uri = "api/v1/"
+    name = "mastodon"
 
     def _send(self, message: str) -> Response:
         """
@@ -58,10 +59,7 @@ class MastodonPlatform(AbstractPlatform):
         return requests.post(
             url=urljoin(self.conf.instance, self.api_uri) + "statuses",
             headers={"Authorization": f"Bearer {self.conf.token}"},
-            data={
-                "status": message,
-                "visibility": "public",
-            },
+            data={"status": message, "visibility": "public"},
         )
 
     def validate_credentials(self):
@@ -85,8 +83,7 @@ class MastodonPlatform(AbstractPlatform):
         except requests.exceptions.HTTPError as e:
             self._log_debug(str(res))
             self._log_error(
-                str(e),
-                raise_error=HTTPResponseError,
+                str(e), raise_error=HTTPResponseError,
             )
 
         try:

mobilizon_reshare/publishers/platforms/platform_mapping.py

@@ -19,6 +19,11 @@ from mobilizon_reshare.publishers.platforms.zulip import (
     ZulipNotifier,
 )
 
+"""
+This module is required to have an explicit mapping between platform names and the classes implementing those platforms.
+It could be refactored in a different pattern but this way makes it more explicit and linear. Eventually this could be
+turned into a plugin system with a plugin for each platform."""
+
 name_to_publisher_class = {
     "mastodon": MastodonPublisher,
     "telegram": TelegramPublisher,

mobilizon_reshare/publishers/platforms/telegram.py

@@ -14,8 +14,7 @@ from mobilizon_reshare.publishers.exceptions import (
     InvalidBot,
     InvalidEvent,
     InvalidResponse,
-    PublisherError,
-    HTTPResponseError,
+    InvalidMessage,
 )
 
 
@@ -72,7 +71,7 @@ class TelegramFormatter(AbstractEventFormatter):
 
     def validate_message(self, message: str) -> None:
         if len(message) >= 4096:
-            raise PublisherError("Message is too long")
+            raise InvalidMessage("Message is too long")
 
     def _preprocess_event(self, event: MobilizonEvent):
         event.description = html_to_markdown(event.description)
@@ -85,6 +84,8 @@ class TelegramPlatform(AbstractPlatform):
     Telegram publisher class.
     """
 
+    name = "telegram"
+
     def _preprocess_message(self, message: str):
         return TelegramFormatter.escape_message(message)
 
@@ -109,12 +110,11 @@ class TelegramPlatform(AbstractPlatform):
 
     def _validate_response(self, res):
         try:
+
             res.raise_for_status()
         except requests.exceptions.HTTPError as e:
-            self._log_debug(str(res))
             self._log_error(
-                str(e),
-                raise_error=HTTPResponseError,
+                f"Server returned invalid data: {str(e)}", raise_error=InvalidResponse,
             )
 
         try:

mobilizon_reshare/publishers/platforms/twitter.py

@@ -47,6 +47,7 @@ class TwitterPlatform(AbstractPlatform):
     """
 
     _conf = ("publisher", "twitter")
+    name = "twitter"
 
     def _get_api(self):
 
@@ -62,7 +63,7 @@ class TwitterPlatform(AbstractPlatform):
         try:
             return self._get_api().update_status(message)
         except TweepyException as e:
-            raise PublisherError from e
+            raise PublisherError(e.args[0])
 
     def validate_credentials(self):
         if not self._get_api().verify_credentials():

mobilizon_reshare/publishers/platforms/zulip.py

@@ -14,7 +14,7 @@ from mobilizon_reshare.publishers.exceptions import (
     InvalidEvent,
     InvalidResponse,
     ZulipError,
-    PublisherError,
+    InvalidMessage,
 )
 
 
@@ -40,7 +40,7 @@ class ZulipFormatter(AbstractEventFormatter):
 
     def validate_message(self, message) -> None:
         if len(message.encode("utf-8")) >= 10000:
-            raise PublisherError("Message is too long")
+            raise InvalidMessage("Message is too long")
 
     def _preprocess_event(self, event: MobilizonEvent):
         event.description = html_to_markdown(event.description)
@@ -56,6 +56,7 @@ class ZulipPlatform(AbstractPlatform):
     _conf = ("publisher", "zulip")
 
     api_uri = "https://zulip.twc-italia.org/api/v1/"
+    name = "zulip"
 
     def _send_private(self, message: str) -> Response:
         """

mobilizon_reshare/storage/query.py

@@ -201,7 +201,7 @@ async def save_publication_report(
     publications: Dict[UUID, Publication],
 ) -> None:
     for publication_report in coordinator_report.reports:
-        publication_id = publication_report.publication_id
+        publication_id = publication_report.publication.id
         publications[publication_id].status = publication_report.status
         publications[publication_id].reason = publication_report.reason
         publications[publication_id].timestamp = arrow.now().datetime

tests/publishers/conftest.py

@@ -65,13 +65,13 @@ def message_collector():
 def mock_formatter_invalid():
     class MockFormatter(AbstractEventFormatter):
         def validate_event(self, event) -> None:
-            raise PublisherError("Invalid event")
+            raise PublisherError("Invalid event error")
 
         def get_message_from_event(self, event) -> str:
             return ""
 
         def validate_message(self, event) -> None:
-            raise PublisherError("Invalid message")
+            raise PublisherError("Invalid message error")
 
     return MockFormatter()
 
@@ -79,8 +79,10 @@ def mock_formatter_invalid():
 @pytest.fixture
 def mock_publisher_valid(message_collector):
     class MockPublisher(AbstractPlatform):
+        name = "mock"
+
         def _send(self, message):
-            message_collector.collect_message(message)
+            message_collector.append(message)
 
         def _validate_response(self, response):
             pass
@@ -94,15 +96,17 @@ def mock_publisher_valid(message_collector):
 @pytest.fixture
 def mock_publisher_invalid(message_collector):
     class MockPublisher(AbstractPlatform):
-        def _send(self, message):
 
-            message_collector.collect_message(message)
+        name = "mock"
+
+        def _send(self, message):
+            message_collector.append(message)
 
         def _validate_response(self, response):
-            return InvalidResponse("error")
+            return InvalidResponse("response error")
 
         def validate_credentials(self) -> None:
-            raise PublisherError("error")
+            raise PublisherError("credentials error")
 
     return MockPublisher()
 
@@ -110,8 +114,11 @@ def mock_publisher_invalid(message_collector):
 @pytest.fixture
 def mock_publisher_invalid_response(message_collector):
     class MockPublisher(AbstractPlatform):
+
+        name = "mock"
+
         def _send(self, message):
-            message_collector.collect_message(message)
+            message_collector.append(message)
 
         def _validate_response(self, response):
             raise InvalidResponse("Invalid response")

tests/publishers/test_abstract_validators.py

@@ -1,14 +0,0 @@
-def test_is_event_valid(mock_formatter_valid, event):
-    assert mock_formatter_valid.is_event_valid(event)
-
-
-def test_is_event_valid_false(mock_formatter_invalid, event):
-    assert not mock_formatter_invalid.is_event_valid(event)
-
-
-def test_is_message_valid(mock_formatter_valid, event):
-    assert mock_formatter_valid.is_message_valid(event)
-
-
-def test_is_message_valid_false(mock_formatter_invalid, event):
-    assert not mock_formatter_invalid.is_message_valid(event)

tests/publishers/test_coordinator.py

@@ -33,7 +33,7 @@ def test_publication_report_successful(statuses, successful):
     reports = []
     for i, status in enumerate(statuses):
         reports.append(
-            EventPublicationReport(reason=None, publication_id=None, status=status)
+            EventPublicationReport(reason=None, publication=None, status=status)
         )
     assert (
         PublisherCoordinatorReport(publications=[], reports=reports).successful
@@ -116,7 +116,7 @@ async def test_publication_coordinator_run_failure(
     assert not report.successful
     assert (
         list(report.reports)[0].reason
-        == "Invalid credentials, Invalid event, Invalid message"
+        == "credentials error, Invalid event error, Invalid message error"
     )
 
 
@@ -142,7 +142,9 @@ async def test_notifier_coordinator_publication_failed(mock_publisher_valid):
     report = EventPublicationReport(
         status=PublicationStatus.FAILED,
         reason="some failure",
-        publication_id=UUID(int=1),
+        publication=EventPublication(
+            id=UUID(int=4), publisher=mock_publisher_valid, formatter=None, event=None
+        ),
     )
     coordinator = PublicationFailureNotifiersCoordinator(
         report, [mock_publisher_valid, mock_publisher_valid]

tests/publishers/test_mastodon.py

@@ -5,6 +5,7 @@ from mobilizon_reshare.publishers.exceptions import (
     InvalidEvent,
     InvalidResponse,
     HTTPResponseError,
+    InvalidMessage,
 )
 from mobilizon_reshare.publishers.platforms.mastodon import (
     MastodonFormatter,
@@ -15,13 +16,16 @@ from mobilizon_reshare.publishers.platforms.mastodon import (
 def test_message_length_success(event):
     message = "a" * 200
     event.name = message
-    assert MastodonFormatter().is_message_valid(event)
+    message = MastodonFormatter().get_message_from_event(event)
+    MastodonFormatter().validate_message(message)
 
 
 def test_message_length_failure(event):
     message = "a" * 500
     event.name = message
-    assert not MastodonFormatter().is_message_valid(event)
+    message = MastodonFormatter().get_message_from_event(event)
+    with pytest.raises(InvalidMessage):
+        MastodonFormatter().validate_message(message)
 
 
 def test_event_validation(event):

tests/publishers/test_telegram.py

@@ -1,7 +1,11 @@
 import pytest
 import requests
 
-from mobilizon_reshare.publishers.exceptions import InvalidEvent, InvalidResponse, HTTPResponseError
+from mobilizon_reshare.publishers.exceptions import (
+    InvalidEvent,
+    InvalidResponse,
+    InvalidMessage,
+)
 from mobilizon_reshare.publishers.platforms.telegram import (
     TelegramFormatter,
     TelegramPublisher,
@@ -11,13 +15,22 @@ from mobilizon_reshare.publishers.platforms.telegram import (
 def test_message_length_success(event):
     message = "a" * 500
     event.description = message
-    assert TelegramFormatter().is_message_valid(event)
+    assert (
+        TelegramFormatter().validate_message(
+            TelegramFormatter().get_message_from_event(event)
+        )
+        is None
+    )
 
 
 def test_message_length_failure(event):
     message = "a" * 10000
     event.description = message
-    assert not TelegramFormatter().is_message_valid(event)
+
+    with pytest.raises(InvalidMessage):
+        TelegramFormatter().validate_message(
+            TelegramFormatter().get_message_from_event(event)
+        )
 
 
 @pytest.mark.parametrize(
@@ -68,7 +81,7 @@ def test_validate_response_invalid_request():
     response = requests.Response()
     response.status_code = 400
     response._content = b"""{"error":true}"""
-    with pytest.raises(HTTPResponseError) as e:
+    with pytest.raises(InvalidResponse) as e:
 
         TelegramPublisher()._validate_response(response)
 

tests/publishers/test_zulip.py

@@ -13,6 +13,7 @@ from mobilizon_reshare.publishers.exceptions import (
     InvalidEvent,
     InvalidResponse,
     ZulipError,
+    InvalidMessage,
 )
 from mobilizon_reshare.publishers.platforms.zulip import ZulipFormatter, ZulipPublisher
 from mobilizon_reshare.storage.query import (
@@ -139,7 +140,7 @@ async def test_zulip_publishr_failure_invalid_credentials(
         )
     ).run()
     assert report.reports[0].status == PublicationStatus.FAILED
-    assert report.reports[0].reason == "Invalid credentials"
+    assert report.reports[0].reason == "403 Error - Your credentials are not valid!"
 
 
 @pytest.mark.asyncio
@@ -170,13 +171,21 @@ def test_event_validation(event):
 def test_message_length_success(event):
     message = "a" * 500
     event.description = message
-    assert ZulipFormatter().is_message_valid(event)
+    assert (
+        ZulipFormatter().validate_message(
+            ZulipFormatter().get_message_from_event(event)
+        )
+        is None
+    )
 
 
 def test_message_length_failure(event):
     message = "a" * 10000
     event.description = message
-    assert not ZulipFormatter().is_message_valid(event)
+    with pytest.raises(InvalidMessage):
+        ZulipFormatter().validate_message(
+            ZulipFormatter().get_message_from_event(event)
+        )
 
 
 def test_validate_response():

tests/storage/test_update.py

@@ -7,6 +7,7 @@ import pytest
 from mobilizon_reshare.event.event import MobilizonEvent, EventPublicationStatus
 from mobilizon_reshare.models.publication import PublicationStatus, Publication
 from mobilizon_reshare.models.publisher import Publisher
+from mobilizon_reshare.publishers.abstract import EventPublication
 from mobilizon_reshare.publishers.coordinator import (
     PublisherCoordinatorReport,
     EventPublicationReport,
@@ -73,12 +74,16 @@ async def test_update_publishers(
                     EventPublicationReport(
                         status=PublicationStatus.FAILED,
                         reason="Invalid credentials",
-                        publication_id=UUID(int=3),
+                        publication=EventPublication(
+                            id=UUID(int=3), formatter=None, event=None, publisher=None
+                        ),
                     ),
                     EventPublicationReport(
                         status=PublicationStatus.COMPLETED,
                         reason="",
-                        publication_id=UUID(int=4),
+                        publication=EventPublication(
+                            id=UUID(int=4), formatter=None, event=None, publisher=None
+                        ),
                     ),
                 ],
             ),