Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Write some tests for the email pusher #4095

Merged
merged 17 commits into from
Oct 30, 2018
8 changes: 4 additions & 4 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,21 +33,21 @@ matrix:
env: TOX_ENV="pep8,check_isort"

- python: 2.7
env: TOX_ENV=py27
env: TOX_ENV=py27 TRIAL_FLAGS="-j 2"

- python: 2.7
env: TOX_ENV=py27-old
env: TOX_ENV=py27-old TRIAL_FLAGS="-j 2"

- python: 2.7
env: TOX_ENV=py27-postgres TRIAL_FLAGS="-j 4"
services:
- postgresql

- python: 3.5
env: TOX_ENV=py35
env: TOX_ENV=py35 TRIAL_FLAGS="-j 2"

- python: 3.6
env: TOX_ENV=py36
env: TOX_ENV=py36 TRIAL_FLAGS="-j 2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use -j 2 here rather than -j 4 like we do for the postgres tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it, -j 2 is faster for SQLite, which has less I/O blocking and therefore is approximate to the speed of your CPU. -j 4 is better for Postgres which has a lot more I/O blocking (mostly creating the database).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Possibly a comment to that effect would be useful)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


- python: 3.6
env: TOX_ENV=py36-postgres TRIAL_FLAGS="-j 4"
Expand Down
1 change: 1 addition & 0 deletions changelog.d/4095.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix exceptions when using the email mailer on Python 3.
5 changes: 4 additions & 1 deletion synapse/push/emailpusher.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,10 @@ def on_stop(self):
self.timed_call = None

def on_new_notifications(self, min_stream_ordering, max_stream_ordering):
self.max_stream_ordering = max(max_stream_ordering, self.max_stream_ordering)
if self.max_stream_ordering:
self.max_stream_ordering = max(max_stream_ordering, self.max_stream_ordering)
else:
self.max_stream_ordering = max_stream_ordering
self._start_processing()

def on_new_receipts(self, min_stream_id, max_stream_id):
Expand Down
9 changes: 4 additions & 5 deletions synapse/push/mailer.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import jinja2

from twisted.internet import defer
from twisted.mail.smtp import sendmail

from synapse.api.constants import EventTypes
from synapse.api.errors import StoreError
Expand Down Expand Up @@ -191,11 +190,11 @@ def _fetch_room_state(room_id):
multipart_msg.attach(html_part)

logger.info("Sending email push notification to %s" % email_address)
# logger.debug(html_text)

yield sendmail(
yield self.hs.get_sendmail()(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally try and pull out dependencies in the constructor, so that things fail at startup if there is a problem with initialising in dependencies

(I thought I made this comment, but GH forgot it :/)

self.hs.config.email_smtp_host,
raw_from, raw_to, multipart_msg.as_string(),
raw_from, raw_to, multipart_msg.as_string().encode('utf8'),
reactor=self.hs.get_reactor(),
port=self.hs.config.email_smtp_port,
requireAuthentication=self.hs.config.email_smtp_user is not None,
username=self.hs.config.email_smtp_user,
Expand Down Expand Up @@ -333,7 +332,7 @@ def make_summary_text(self, notifs_by_room, room_state_ids,
notif_events, user_id, reason):
if len(notifs_by_room) == 1:
# Only one room has new stuff
room_id = notifs_by_room.keys()[0]
room_id = list(notifs_by_room.keys())[0]

# If the room has some kind of name, use it, but we don't
# want the generated-from-names one here otherwise we'll
Expand Down
7 changes: 7 additions & 0 deletions synapse/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,13 @@ def get_reactor(self):
"""
return self._reactor

def get_sendmail(self):
if hasattr(self, "_sendmail"):
return self._sendmail
else:
from twisted.mail.smtp import sendmail
return sendmail
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the import up to the top please?

Also, I'd be tempted to instead add sendmail to DEPENDENCIES and define:

def build_sendmail(self):
    return sendmail

Which bring it inline with other dependencies. To override in the tests its enough to explicitly define sendmail on the HS object, which can be done by passing sendmail kwarg to setup_test_homeserver


def get_ip_from_request(self, request):
# X-Forwarded-For is handled by our custom request type.
return request.getClientIP()
Expand Down
Empty file added tests/push/__init__.py
Empty file.
150 changes: 150 additions & 0 deletions tests/push/test_email.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
# -*- coding: utf-8 -*-
# Copyright 2018 New Vector
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import os

import pkg_resources

from twisted.internet.defer import Deferred

from synapse.rest.client.v1 import admin, login, room

from tests.unittest import HomeserverTestCase

try:
from synapse.push.mailer import load_jinja2_templates
except Exception:
load_jinja2_templates = None


class EmailPusherTests(HomeserverTestCase):

skip = "No Jinja installed" if not load_jinja2_templates else None
servlets = [
admin.register_servlets,
room.register_servlets,
login.register_servlets,
]
user_id = True
hijack_auth = False

def make_homeserver(self, reactor, clock):

config = self.default_config()
config.email_enable_notifs = True
config.start_pushers = True

config.email_template_dir = os.path.abspath(
pkg_resources.resource_filename('synapse', 'res/templates')
)
config.email_notif_template_html = "notif_mail.html"
config.email_notif_template_text = "notif_mail.txt"
config.email_smtp_host = "127.0.0.1"
config.email_smtp_port = 20
config.require_transport_security = False
config.email_smtp_user = None
config.email_app_name = "Matrix"
config.email_notif_from = "[email protected]"

hs = self.setup_test_homeserver(config=config)

return hs

def test_sends_email(self):

# List[Tuple[Deferred, args, kwargs]]
email_attempts = []

def sendmail(*args, **kwargs):
d = Deferred()
email_attempts.append((d, args, kwargs))
return d

self.hs._sendmail = sendmail

# Register the user who gets notified
user_id = self.register_user("user", "pass")
access_token = self.login("user", "pass")

# Register the user who sends the message
other_user_id = self.register_user("otheruser", "pass")
other_access_token = self.login("otheruser", "pass")

# Register the pusher
user_tuple = self.get_success(
self.hs.get_datastore().get_user_by_access_token(access_token)
)
token_id = user_tuple["token_id"]

self.get_success(
self.hs.get_pusherpool().add_pusher(
user_id=user_id,
access_token=token_id,
kind="email",
app_id="m.email",
app_display_name="Email Notifications",
device_display_name="[email protected]",
pushkey="[email protected]",
lang=None,
data={},
)
)

# Create a room
room = self.helper.create_room_as(user_id, tok=access_token)

# Invite the other person
self.helper.invite(room=room, src=user_id, tok=access_token, targ=other_user_id)

# The other user joins
self.helper.join(room=room, user=other_user_id, tok=other_access_token)

# The other user sends some messages
self.helper.send(room, body="Hi!", tok=other_access_token)
self.helper.send(room, body="There!", tok=other_access_token)

# Get the stream ordering before it gets sent
pushers = self.get_success(
self.hs.get_datastore().get_pushers_by(dict(user_name=user_id))
)
self.assertEqual(len(pushers), 1)
last_stream_ordering = pushers[0]["last_stream_ordering"]

# Advance time a bit, so the pusher will register something has happened
self.pump(100)

# It hasn't succeeded yet, so the stream ordering shouldn't have moved
pushers = self.get_success(
self.hs.get_datastore().get_pushers_by(dict(user_name=user_id))
)
self.assertEqual(len(pushers), 1)
self.assertEqual(last_stream_ordering, pushers[0]["last_stream_ordering"])

# One email was attempted to be sent
self.assertEqual(len(email_attempts), 1)

# Make the email succeed
email_attempts[0][0].callback(True)
self.pump()

# One email was attempted to be sent
self.assertEqual(len(email_attempts), 1)

# The stream ordering has increased
pushers = self.get_success(
self.hs.get_datastore().get_pushers_by(dict(user_name=user_id))
)
self.assertEqual(len(pushers), 1)
self.assertTrue(pushers[0]["last_stream_ordering"] > last_stream_ordering)
4 changes: 3 additions & 1 deletion tests/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,9 @@ def make_request(method, path, content=b"", access_token=None, request=SynapseRe
req.content = BytesIO(content)

if access_token:
req.requestHeaders.addRawHeader(b"Authorization", b"Bearer " + access_token)
req.requestHeaders.addRawHeader(
b"Authorization", b"Bearer " + access_token.encode('ascii')
)

if content:
req.requestHeaders.addRawHeader(b"Content-Type", b"application/json")
Expand Down
2 changes: 1 addition & 1 deletion tests/test_mau.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ def create_user(self, localpart):

def do_sync_for_user(self, token):
request, channel = make_request(
"GET", "/sync", access_token=token.encode('ascii')
"GET", "/sync", access_token=token
)
render(request, self.resource, self.reactor)

Expand Down
9 changes: 8 additions & 1 deletion tests/unittest.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,13 @@ def DEBUG(target):
return target


def INFO(target):
"""A decorator to set the .loglevel attribute to logging.INFO.
Can apply to either a TestCase or an individual test method."""
target.loglevel = logging.INFO
return target


class HomeserverTestCase(TestCase):
"""
A base TestCase that reduces boilerplate for HomeServer-using test cases.
Expand Down Expand Up @@ -373,5 +380,5 @@ def login(self, username, password, device_id=None):
self.render(request)
self.assertEqual(channel.code, 200)

access_token = channel.json_body["access_token"].encode('ascii')
access_token = channel.json_body["access_token"]
return access_token