Merge pull request #255 from MycroftAI/bugfix/send-email

Fix email sending logic

Dockerfile

@@ -104,6 +104,7 @@ ENV PANTACOR_API_TOKEN pantacor-token
 ENV PANTACOR_API_BASE_URL pantacor.test.url
 ENV PYTHONPATH=$PYTHONPATH:/opt/selene/selene-backend/api/public
 ENV GOOGLE_STT_KEY $google_stt_key
+ENV SENDGRID_API_KEY test_sendgrid_key
 ENV WOLFRAM_ALPHA_KEY $wolfram_alpha_key
 ENV WOLFRAM_ALPHA_URL https://api.wolframalpha.com
 COPY api/public api/public

api/public/public_api/endpoints/device_email.py

@@ -16,21 +16,20 @@
 #
 # You should have received a copy of the GNU Affero General Public License
 # along with this program. If not, see <https://www.gnu.org/licenses/>.
-
-import json
-import os
-import smtplib
-from email.message import EmailMessage
+"""Device API endpoint to send an email as specified by the device."""
 from http import HTTPStatus
 
 from schematics import Model
 from schematics.types import StringType
 
 from selene.api import PublicEndpoint
 from selene.data.account import AccountRepository
+from selene.util.email import EmailMessage, SeleneMailer
 
 
 class SendEmail(Model):
+    """Data model of the incoming PUT request."""
+
     title = StringType(required=True)
     sender = StringType(required=True)
     body = StringType(required=True)
@@ -39,37 +38,27 @@ class SendEmail(Model):
 class DeviceEmailEndpoint(PublicEndpoint):
     """Endpoint to send an email to the account associated to a device"""
 
-    def __init__(self):
-        super(DeviceEmailEndpoint, self).__init__()
-
     def put(self, device_id):
+        """Handle an HTTP PUT request."""
         self._authenticate(device_id)
-        payload = json.loads(self.request.data)
-        send_email = SendEmail(payload)
-        send_email.validate()
-
+        self._validate_request()
         account = AccountRepository(self.db).get_account_by_device_id(device_id)
+        self._send_message(account)
+
+        return "", HTTPStatus.OK
 
-        if account:
-            message = EmailMessage()
-            message['Subject'] = str(send_email.title)
-            message['From'] = str(send_email.sender)
-            message.set_content(str(send_email.body))
-            message['To'] = account.email_address
-            self._send_email(message)
-            response = '', HTTPStatus.OK
-        else:
-            response = '', HTTPStatus.NO_CONTENT
-        return response
+    def _validate_request(self):
+        """Validate that the request is well-formed."""
+        send_email = SendEmail(self.request.json)
+        send_email.validate()
 
-    def _send_email(self, message: EmailMessage):
-        email_client = self.config.get('EMAIL_CLIENT')
-        if email_client is None:
-            host = os.environ['EMAIL_SERVICE_HOST']
-            port = os.environ['EMAIL_SERVICE_PORT']
-            user = os.environ['EMAIL_SERVICE_USER']
-            password = os.environ['EMAIL_SERVICE_PASSWORD']
-            email_client = smtplib.SMTP(host, port)
-            email_client.login(user, password)
-        email_client.send_message(message)
-        email_client.quit()
+    def _send_message(self, account):
+        """Send an email to the account that owns the device that requested it."""
+        message = EmailMessage(
+            recipient=account.email_address,
+            sender=self.request.json["sender"],
+            subject=self.request.json["title"],
+            body=self.request.json["body"],
+        )
+        mailer = SeleneMailer(message)
+        mailer.send()

api/public/tests/features/device_email.feature

@@ -1,11 +1,13 @@
-Feature: Send email to a to the account that owns a device
-  Test the email endpoint
+Feature: Device requests email sent to the account holder
+  Some skills have the ability to send email upon request.  One example of
+  this is the support skill, which emails device diagnostics.
 
-  Scenario: an email payload is passed to the email endpoint
-    When an email message is sent to the email endpoint
-    Then an email should be sent to the user's account that owns the device
+  Scenario: Email sent to account holder
+    When a user interaction with a device causes an email to be sent
+    Then the request will be successful
+    And an email should be sent to the account that owns the device
     And the device's last contact time is updated
 
-  Scenario: an email payload is passed to the the email endpoint using a not allowed device
-    When the email endpoint is called by a not allowed device
-    Then 401 status code should be returned by the email endpoint
+  Scenario: Email request sent by unauthorized device
+    When an unpaired or unauthenticated device attempts to send an email
+    Then the request will fail with an unauthorized error

api/public/tests/features/steps/device_email.py

@@ -16,59 +16,79 @@
 #
 # You should have received a copy of the GNU Affero General Public License
 # along with this program. If not, see <https://www.gnu.org/licenses/>.
-
+"""Step functions for the device API call to send an email."""
 import json
 import uuid
-from http import HTTPStatus
 from unittest.mock import patch, MagicMock
 
-from behave import when, then
-from hamcrest import assert_that, equal_to
+from behave import when, then  # pylint: disable=no-name-in-module
 
-email_request = dict(
-    title='this is a test',
-    sender='[email protected]',
-    body='body message'
-)
 
+@when("a user interaction with a device causes an email to be sent")
+def send_email(context):
+    """Call the email endpoint to send an email as specified by a device.
 
-@when('an email message is sent to the email endpoint')
-@patch('smtplib.SMTP')
-def send_email(context, email_client):
-    context.client_config['EMAIL_CLIENT'] = email_client
-    login = context.device_login
-    device_id = login['uuid']
-    access_token = login['accessToken']
-    context.email_response = context.client.put(
-        '/v1/device/{uuid}/message'.format(uuid=device_id),
-        data=json.dumps(email_request),
-        content_type='application_json',
-        headers=dict(Authorization='Bearer {token}'.format(token=access_token))
-    )
+    The SendGrid call to send email is mocked out because we trust that the library
+    is coded to correctly send email via sendgrid.  That, and testing sent emails is
+    a real pain in the arse.
+    """
+    with patch("selene.util.email.email.Mail") as message_patch:
+        context.message_patch = message_patch
+        with patch("selene.util.email.email.SendGridAPIClient") as sendgrid_patch:
+            sendgrid_patch.return_value, post_mock = _define_sendgrid_mock()
+            context.sendgrid_patch = sendgrid_patch
+            context.post_mock = post_mock
+            _call_email_endpoint(context)
 
 
-@then('an email should be sent to the user\'s account that owns the device')
-def validate_response(context):
-    response = context.email_response
-    assert_that(response.status_code, equal_to(HTTPStatus.OK))
-    email_client: MagicMock = context.client_config['EMAIL_CLIENT']
-    email_client.send_message.assert_called()
+@when("an unpaired or unauthenticated device attempts to send an email")
+def send_email_invalid_device(context):
+    """Call the email endpoint with an invalid device ID to test authentication."""
+    _call_email_endpoint(context, device_id=str(uuid.uuid4()))
+
 
+def _define_sendgrid_mock():
+    """Go through some insane hoops to setup the correct Mock for the Sendgrid code."""
+    post_return_value = MagicMock(name="post_return")
+    post_return_value.status_code = 200
+    post_mock = MagicMock(name="post_mock")
+    post_mock.return_value = post_return_value
+    send_mock = MagicMock(name="send_mock")
+    send_mock.post = post_mock
+    mail_mock = MagicMock(name="mail_mock")
+    mail_mock.send = send_mock
+    client_mock = MagicMock(name="client_mock")
+    client_mock.mail = mail_mock
+    sendgrid_mock = MagicMock(name="sendgrid_mock")
+    sendgrid_mock.client = client_mock
 
-@when('the email endpoint is called by a not allowed device')
-@patch('smtplib.SMTP')
-def send_email_invalid_device(context, email_client):
-    context.client_config['EMAIL_CLIENT'] = email_client
-    context.email_invalid_response = context.client.put(
-        '/v1/device/{uuid}/email'.format(uuid=str(uuid.uuid4())),
-        data=json.dumps(email_request),
-        content_type='application_json'
+    return sendgrid_mock, post_mock
+
+
+def _call_email_endpoint(context, device_id=None):
+    """Build the request to the email endpoint and issue a call."""
+    if device_id is None:
+        login = context.device_login
+        request_device_id = login["uuid"]
+        request_headers = dict(Authorization=f"Bearer {login['accessToken']}")
+    else:
+        request_device_id = device_id
+        request_headers = dict(Authorization=f"Bearer thisisadummybearertoken")
+    request_data = dict(
+        title="this is a test", sender="[email protected]", body="body message"
+    )
+    context.response = context.client.put(
+        f"/v1/device/{request_device_id}/message",
+        data=json.dumps(request_data),
+        content_type="application/json",
+        headers=request_headers,
     )
 
 
-@then('401 status code should be returned by the email endpoint')
-def validate_response_invalid_device(context):
-    response = context.email_invalid_response
-    assert_that(response.status_code, equal_to(HTTPStatus.UNAUTHORIZED))
-    email_client: MagicMock = context.client_config['EMAIL_CLIENT']
-    email_client.send_message.assert_not_called()
+@then("an email should be sent to the account that owns the device")
+def validate_response(context):
+    """Validate that the SendGrid API was called as expected."""
+    sendgrid_patch = context.sendgrid_patch
+    sendgrid_patch.assert_called_with(api_key="test_sendgrid_key")
+    post_mock = context.post_mock
+    post_mock.assert_called_with(request_body=context.message_patch().get())

db/mycroft/geography_schema/delete_duplicate_cities.sql

@@ -1 +1,5 @@
-DELETE FROM geography.city WHERE id IN %(city_ids)s
+DELETE FROM
+    geography.city
+WHERE
+    id IN %(city_ids)s
+    and (population is null or population != %(max_population)s)

db/mycroft/geography_schema/get_duplicated_cities.sql

@@ -4,6 +4,7 @@ WITH duplicated_cities AS (
         c.name AS country_name,
         r.name AS region_name,
         y.name AS city_name,
+        max(population) as max_population,
         array_agg(y.id::text) as city_ids
     FROM
         geography.city y

db/scripts/remove_duplicate_cities.py

@@ -69,7 +69,10 @@ def get_device_geographies(cursor, city):
     request = DatabaseRequest(sql, args)
     result = cursor.select_all(request)
     if result:
-        print(f"found {len(result)} device geographies for {city['city_name']}")
+        print(
+            f"found {len(result)} device geographies for city: {city['city_name']}; "
+            f"region: {city['region_name']}; country: {city['country_name']}"
+        )
 
     return result
 
@@ -125,20 +128,28 @@ def check_account_defaults_for_dup_cities(cursor, duplicate_cities):
     return account_defaults_found
 
 
-def delete_duplicates(cursor, duplicate_cities):
+def delete_duplicates(cursor, city, used_cities):
     """Once all the checks are done, we can delete the rows from the database.
 
     Remove the first ID from the list so that one of the rows remains.
     """
     deleted_rows = 0
     geography_dir = Path(MYCROFT_DB_DIR).joinpath("geography_schema")
     sql = get_sql_from_file(str(geography_dir.joinpath("delete_duplicate_cities.sql")))
-    for city in duplicate_cities:
-        cities_to_delete = city["city_ids"][1:]
-        args = dict(city_ids=tuple(cities_to_delete))
-        request = DatabaseRequest(sql, args)
-        result = cursor.delete(request)
-        deleted_rows += result
+    if used_cities:
+        sql += " and id not in %(used_cities)s"
+        args = dict(
+            city_ids=tuple(city["city_ids"]),
+            max_population=city["max_population"],
+            used_cities=tuple(used_cities),
+        )
+    else:
+        args = dict(
+            city_ids=tuple(city["city_ids"]), max_population=city["max_population"],
+        )
+    request = DatabaseRequest(sql, args)
+    result = cursor.delete(request)
+    deleted_rows += result
 
     print(f"Deleted {deleted_rows} from the geography.city table")
 
@@ -147,16 +158,21 @@ def main():
     """Make it so."""
     cursor = get_cursor()
     duplicate_cities = get_duplicate_cities(cursor)
-    device_geographies_found = check_device_geography_for_dup_cities(
-        cursor, duplicate_cities
-    )
     account_defaults_found = check_account_defaults_for_dup_cities(
         cursor, duplicate_cities
     )
-    if device_geographies_found or account_defaults_found:
+    if account_defaults_found:
         print("Great, now you need to write more code!")
     else:
-        delete_duplicates(cursor, duplicate_cities)
+        for city in duplicate_cities:
+            device_geographies = get_device_geographies(cursor, city)
+            if not device_geographies:
+                delete_duplicates(cursor, city, [])
+            else:
+                print(device_geographies)
+                used_cities = [geo["city_id"] for geo in device_geographies]
+                delete_duplicates(cursor, city, used_cities)
+                break
 
 
 if __name__ == "__main__":