Merge pull request #9203 from kiendang/noti

Check email credentials on switching on `ServerSettings.notifications_enabled`

packages/syft/src/syft/service/notifier/notifier_service.py

@@ -67,24 +67,16 @@ def user_settings(
             app=notifications[NOTIFIERS.APP],
         )
 
-    def set_notifier_active_to_true(self, context: AuthedServiceContext) -> SyftSuccess:
+    def _set_notifier(self, context: AuthedServiceContext, active: bool) -> SyftSuccess:
         notifier = self.stash.get(credentials=context.credentials).unwrap(
             public_message="Notifier settings not found."
         )
-        notifier.active = True
-        self.stash.update(credentials=context.credentials, settings=notifier).unwrap()
-        return SyftSuccess(message="notifier.active set to true.")
 
-    def set_notifier_active_to_false(
-        self, context: AuthedServiceContext
-    ) -> SyftSuccess:
-        """
-        Essentially a duplicate of turn_off method.
-        """
-        notifier = self.stash.get(credentials=context.credentials).unwrap()
-        notifier.active = False
+        notifier.active = active
         self.stash.update(credentials=context.credentials, settings=notifier).unwrap()
-        return SyftSuccess(message="notifier.active set to false.")
+
+        active_s = "active" if active else "inactive"
+        return SyftSuccess(message=f"Notifier set to {active_s}")
 
     @as_result(SyftException)
     def turn_on(

packages/syft/src/syft/service/settings/settings_service.py

@@ -1,5 +1,6 @@
 # stdlib
 from string import Template
+from typing import cast
 
 # relative
 from ...abstract_server import ServerSideType
@@ -9,6 +10,7 @@
 from ...store.document_store_errors import StashException
 from ...types.errors import SyftException
 from ...types.result import as_result
+from ...types.syft_metaclass import Empty
 from ...util.assets import load_png_base64
 from ...util.experimental_flags import flags
 from ...util.misc_objs import HTMLObject
@@ -20,6 +22,7 @@
 from ..context import AuthedServiceContext
 from ..context import UnauthedServiceContext
 from ..notifier.notifier_enums import EMAIL_TYPES
+from ..notifier.notifier_service import NotifierService
 from ..response import SyftSuccess
 from ..service import AbstractService
 from ..service import service_method
@@ -31,6 +34,14 @@
 from .settings import ServerSettingsUpdate
 from .settings_stash import SettingsStash
 
+# for testing purpose
+_NOTIFICATIONS_ENABLED_WIHOUT_CREDENTIALS_ERROR = (
+    "Failed to enable notification. "
+    "Email credentials are invalid or have not been set. "
+    "Please use `enable_notifications` from `user_service` "
+    "to set the correct email credentials."
+)
+
 
 @serializable(canonical_name="SettingsService", version=1)
 class SettingsService(AbstractService):
@@ -118,23 +129,44 @@ def _update(
                 context.credentials, settings=new_settings
             ).unwrap()
 
-            notifier_service = context.server.get_service("notifierservice")
+            notifier_service = cast(
+                NotifierService, context.server.get_service("notifierservice")
+            )
 
             # If notifications_enabled is present in the update, we need to update the notifier settings
-            if settings.notifications_enabled is True:
-                if not notifier_service.settings(context):
+            if settings.notifications_enabled is not Empty:  # type: ignore[comparison-overlap]
+                notifier_settings_res = notifier_service.settings(context)
+                if (
+                    not notifier_settings_res.is_ok()
+                    or (notifier_settings := notifier_settings_res.ok()) is None
+                ):
                     raise SyftException(
-                        public_smessage="Create notification settings using enable_notifications from user_service"
+                        public_message=(
+                            "Notification has not been enabled. "
+                            "Please use `enable_notifications` from `user_service`."
+                        )
+                    )
+
+                if settings.notifications_enabled and (
+                    not (
+                        notifier_settings.email_username
+                        and notifier_settings.email_password
                     )
-                notifier_service = context.server.get_service("notifierservice")
-                notifier_service.set_notifier_active_to_true(context)
-            elif settings.notifications_enabled is False:
-                if not notifier_service.settings(context):
+                    or not notifier_settings.validate_email_credentials(
+                        notifier_settings.email_username,
+                        notifier_settings.email_password,
+                        notifier_settings.email_server,
+                        notifier_settings.email_port,
+                    )
+                ):
                     raise SyftException(
-                        public_message="Create notification settings using enable_notifications from user_service"
+                        public_message=_NOTIFICATIONS_ENABLED_WIHOUT_CREDENTIALS_ERROR
                     )
-                notifier_service = context.server.get_service("notifierservice")
-                notifier_service.set_notifier_active_to_false(context)
+
+                notifier_service.set_notifier(
+                    context, active=settings.notifications_enabled
+                )
+
             return update_result
         else:
             raise NotFoundException(public_message="Server settings not found")

packages/syft/tests/syft/settings/settings_service_test.py

@@ -23,6 +23,9 @@
 from syft.service.service import _SIGNATURE_ERROR_MESSAGE
 from syft.service.settings.settings import ServerSettings
 from syft.service.settings.settings import ServerSettingsUpdate
+from syft.service.settings.settings_service import (
+    _NOTIFICATIONS_ENABLED_WIHOUT_CREDENTIALS_ERROR,
+)
 from syft.service.settings.settings_service import SettingsService
 from syft.service.settings.settings_stash import SettingsStash
 from syft.service.user.user import UserPrivateKey
@@ -462,3 +465,12 @@ def test_invalid_args_error_message(root_datasite_client: DatasiteClient) -> Non
     settings = root_datasite_client.api.services.settings.get()
     assert settings.name == update_args["name"]
     assert settings.organization == update_args["organization"]
+
+
+def test_notifications_enabled_without_emails_credentials_not_allowed(
+    root_datasite_client: DatasiteClient,
+) -> None:
+    with pytest.raises(SyftException) as exc:
+        root_datasite_client.api.services.settings.update(notifications_enabled=True)
+
+    assert _NOTIFICATIONS_ENABLED_WIHOUT_CREDENTIALS_ERROR in exc.value.public_message