Commit 75fe5e81 authored by Barry Warsaw's avatar Barry Warsaw

Merge branch 'abompard/73'

Large performance improvement in SubscriptionService.find_members(),
along with a refactoring which will allow future improvements, many new
tests, and a bug fix.  Minor branch cleanups by Barry.
parents 9c9d0f01 8b13f610
Pipeline #486886 failed with stage
......@@ -47,11 +47,12 @@ from mailman.interfaces.subscriptions import ISubscriptionService, TokenOwner
from mailman.interfaces.user import IUser
from mailman.interfaces.usermanager import IUserManager
from mailman.interfaces.workflow import IWorkflowStateManager
from mailman.model.address import Address
from mailman.model.member import Member
from mailman.model.user import User
from mailman.utilities.datetime import now
from mailman.utilities.i18n import make
from operator import attrgetter
from sqlalchemy import and_, or_
from zope.component import getUtility
from zope.event import notify
from zope.interface import implementer
......@@ -60,16 +61,6 @@ from zope.interface import implementer
log = logging.getLogger('mailman.subscribe')
def _membership_sort_key(member):
"""Sort function for find_members().
The members are sorted first by unique list id, then by subscribed email
address, then by role.
"""
return (member.list_id, member.address.email, member.role.value)
class WhichSubscriber(Enum):
address = 1
user = 2
......@@ -385,39 +376,39 @@ class SubscriptionService:
# If `subscriber` is a user id, then we'll search for all addresses
# which are controlled by the user, otherwise we'll just search for
# the given address.
user_manager = getUtility(IUserManager)
if subscriber is None and list_id is None and role is None:
return []
order = (Member.list_id, Address.email, Member.role)
# Querying for the subscriber is the most complicated part, because
# the parameter can either be an email address or a user id.
query = []
# the parameter can either be an email address or a user id. Start by
# building two queries, one joined on the member's address, and one
# joined on the member's user. Add the resulting email address to the
# selected values to be able to sort on it later on.
q_address = store.query(Member, Address.email).join(Member._address)
q_user = store.query(Member, Address.email).join(Member._user)
if subscriber is not None:
if isinstance(subscriber, str):
# subscriber is an email address.
address = user_manager.get_address(subscriber)
user = user_manager.get_user(subscriber)
# This probably could be made more efficient.
if address is None or user is None:
return []
query.append(or_(Member.address_id == address.id,
Member.user_id == user.id))
q_address = q_address.filter(
Address.email == subscriber.lower())
q_user = q_user.join(User.addresses).filter(
Address.email == subscriber.lower())
else:
# subscriber is a user id.
user = user_manager.get_user_by_id(subscriber)
address_ids = list(address.id for address in user.addresses
if address.id is not None)
if len(address_ids) == 0 or user is None:
return []
query.append(or_(Member.user_id == user.id,
Member.address_id.in_(address_ids)))
# Calculate the rest of the query expression, which will get And'd
# with the Or clause above (if there is one).
q_address = q_address.join(Address.user).filter(
User._user_id == subscriber)
q_user = q_user.join(User._preferred_address).filter(
User._user_id == subscriber)
# Add additional filters to both queries.
if list_id is not None:
query.append(Member.list_id == list_id)
q_address = q_address.filter(Member.list_id == list_id)
q_user = q_user.filter(Member.list_id == list_id)
if role is not None:
query.append(Member.role == role)
results = store.query(Member).filter(and_(*query))
return sorted(results, key=_membership_sort_key)
q_address = q_address.filter(Member.role == role)
q_user = q_user.filter(Member.role == role)
# Do a UNION of the two queries, sort the result and generate Members.
query = q_address.union(q_user).order_by(*order).from_self(Member)
return query.all()
def __iter__(self):
for member in self.get_members():
......
......@@ -27,9 +27,9 @@ import unittest
from mailman.app.lifecycle import create_list
from mailman.app.subscriptions import SubscriptionWorkflow
from mailman.interfaces.bans import IBanManager
from mailman.interfaces.member import MembershipIsBannedError
from mailman.interfaces.member import MemberRole, MembershipIsBannedError
from mailman.interfaces.pending import IPendings
from mailman.interfaces.subscriptions import TokenOwner
from mailman.interfaces.subscriptions import ISubscriptionService, TokenOwner
from mailman.testing.helpers import LogFileMark, get_queue_messages
from mailman.testing.layers import ConfigLayer
from mailman.interfaces.mailinglist import SubscriptionPolicy
......@@ -641,3 +641,203 @@ approval:
self.assertIsNone(workflow.token)
self.assertEqual(workflow.token_owner, TokenOwner.no_one)
self.assertEqual(workflow.member.address, anne)
class TestSubscriptionService(unittest.TestCase):
layer = ConfigLayer
def setUp(self):
self._mlist = create_list('[email protected]')
self._mlist.admin_immed_notify = False
self._user_manager = getUtility(IUserManager)
self._service = getUtility(ISubscriptionService)
def test_find_member_address_no_user(self):
# Find address-based memberships when no user is linked to the address.
address = self._user_manager.create_address(
'[email protected]', 'Anne Address')
self._mlist.subscribe(address)
members = self._service.find_members('[email protected]')
self.assertEqual(len(members), 1)
self.assertEqual(members[0].address, address)
def test_find_member_address_with_user(self):
# Find address-based memberships when a user is linked to the address.
user = self._user_manager.create_user(
'[email protected]', 'Anne User')
address = user.addresses[0]
address.verified_on = now()
user.preferred_address = address
# Subscribe the address.
self._mlist.subscribe(address)
members = self._service.find_members('[email protected]')
self.assertEqual(len(members), 1)
self.assertEqual(members[0].user, user)
def test_find_member_user(self):
# Find user-based memberships by address.
user = self._user_manager.create_user(
'[email protected]', 'Anne User')
address = user.addresses[0]
address.verified_on = now()
user.preferred_address = address
# Subscribe the user.
self._mlist.subscribe(user)
members = self._service.find_members('[email protected]')
self.assertEqual(len(members), 1)
self.assertEqual(members[0].user, user)
def test_find_member_user_secondary_address(self):
# Find user-based memberships using a secondary address.
user = self._user_manager.create_user(
'[email protected]', 'Anne User')
address = user.addresses[0]
address.verified_on = now()
user.preferred_address = address
# Create a secondary address.
address_2 = self._user_manager.create_address(
'[email protected]', 'Anne User 2')
address_2.user = user
# Subscribe the user.
self._mlist.subscribe(user)
# Search for the secondary address.
members = self._service.find_members('[email protected]')
self.assertEqual(len(members), 1)
self.assertEqual(members[0].user, user)
def test_wont_find_member_secondary_address(self):
# A user is subscribed with one of their address, and a search is
# performed on another of their addresses. This is not supported; the
# subscription is not returned.
user = self._user_manager.create_user(
'[email protected]', 'Anne User')
address = user.addresses[0]
address.verified_on = now()
user.preferred_address = address
# Create a secondary address.
address_2 = self._user_manager.create_address(
'[email protected]', 'Anne User 2')
address_2.verified_on = now()
address_2.user = user
# Subscribe the secondary address.
self._mlist.subscribe(address_2)
# Search for the primary address.
members = self._service.find_members('[email protected]')
self.assertEqual(len(members), 0)
def test_find_member_user_id(self):
# Find user-based memberships by user_id.
user = self._user_manager.create_user(
'[email protected]', 'Anne User')
address = user.addresses[0]
address.verified_on = now()
user.preferred_address = address
# Subscribe the user.
self._mlist.subscribe(user)
members = self._service.find_members(user.user_id)
self.assertEqual(len(members), 1)
self.assertEqual(members[0].user, user)
def test_find_member_user_id_controlled_addresses(self):
# Find address-based memberships by user_id when a secondary address is
# subscribed.
user = self._user_manager.create_user(
'[email protected]', 'Anne User')
address = user.addresses[0]
address.verified_on = now()
user.preferred_address = address
# Create a secondary address.
address_2 = self._user_manager.create_address(
'[email protected]', 'Anne User 2')
address_2.verified_on = now()
address_2.user = user
# Create a third address.
address_3 = self._user_manager.create_address(
'[email protected]', 'Anne User 3')
address_3.verified_on = now()
address_3.user = user
# Subscribe the secondary address only.
self._mlist.subscribe(address_2)
members = self._service.find_members(user.user_id)
self.assertEqual(len(members), 1)
self.assertEqual(members[0].address, address_2)
def test_find_member_sorting(self):
# Check that the memberships are properly sorted.
user = self._user_manager.create_user(
'[email protected]', 'Anne User')
address = user.addresses[0]
address.verified_on = now()
user.preferred_address = address
# Create a secondary address.
address_2 = self._user_manager.create_address(
'[email protected]', 'Anne User 2')
address_2.verified_on = now()
address_2.user = user
# Create a third address.
address_3 = self._user_manager.create_address(
'[email protected]', 'Anne User 3')
address_3.verified_on = now()
address_3.user = user
# Create three lists.
mlist1 = create_list('[email protected]')
mlist1.admin_immed_notify = False
mlist2 = create_list('[email protected]')
mlist2.admin_immed_notify = False
mlist3 = create_list('[email protected]')
mlist3.admin_immed_notify = False
# Subscribe the addresses in random order
# https://www.xkcd.com/221/
mlist3.subscribe(address_3, MemberRole.moderator)
mlist3.subscribe(address_3, MemberRole.owner)
mlist3.subscribe(address_3, MemberRole.member)
mlist3.subscribe(address_2, MemberRole.member)
mlist3.subscribe(address_2, MemberRole.owner)
mlist3.subscribe(address_2, MemberRole.moderator)
mlist3.subscribe(address, MemberRole.owner)
mlist3.subscribe(address, MemberRole.member)
mlist3.subscribe(address, MemberRole.moderator)
mlist2.subscribe(address_2, MemberRole.moderator)
mlist2.subscribe(address_2, MemberRole.member)
mlist2.subscribe(address_2, MemberRole.owner)
mlist2.subscribe(address_3, MemberRole.moderator)
mlist2.subscribe(address_3, MemberRole.member)
mlist2.subscribe(address_3, MemberRole.owner)
mlist2.subscribe(address, MemberRole.owner)
mlist2.subscribe(address, MemberRole.moderator)
mlist2.subscribe(address, MemberRole.member)
mlist1.subscribe(address_2, MemberRole.moderator)
mlist1.subscribe(address, MemberRole.member)
mlist1.subscribe(address_3, MemberRole.owner)
# The results should be sorted first by list id, then by address, then
# by member role.
members = self._service.find_members(user.user_id)
self.assertEqual(len(members), 21)
self.assertListEqual(
[(m.list_id.partition('.')[0],
m.address.email.partition('@')[0],
m.role)
for m in members],
[('test1', 'anne1', MemberRole.member),
('test1', 'anne2', MemberRole.moderator),
('test1', 'anne3', MemberRole.owner),
('test2', 'anne1', MemberRole.member),
('test2', 'anne1', MemberRole.owner),
('test2', 'anne1', MemberRole.moderator),
('test2', 'anne2', MemberRole.member),
('test2', 'anne2', MemberRole.owner),
('test2', 'anne2', MemberRole.moderator),
('test2', 'anne3', MemberRole.member),
('test2', 'anne3', MemberRole.owner),
('test2', 'anne3', MemberRole.moderator),
('test3', 'anne1', MemberRole.member),
('test3', 'anne1', MemberRole.owner),
('test3', 'anne1', MemberRole.moderator),
('test3', 'anne2', MemberRole.member),
('test3', 'anne2', MemberRole.owner),
('test3', 'anne2', MemberRole.moderator),
('test3', 'anne3', MemberRole.member),
('test3', 'anne3', MemberRole.owner),
('test3', 'anne3', MemberRole.moderator),
])
......@@ -52,6 +52,8 @@ Bugs
subcommand extensions. Given by Aurélien Bompard. (Closes: #168)
* Don't traceback if a nonexistent message-id is deleted from the message
store. Given by Aurélien Bompard, tweaked by Barry Warsaw. (Closes: #167)
* Fix a bug in ``SubscriptionService.find_members()`` when searching for a
subscribed address that is not linked to a user. Given by Aurélien Bompard.
Configuration
-------------
......@@ -122,6 +124,8 @@ Other
* Improvements in importing Mailman 2.1 lists, given by Aurélien Bompard.
* The ``prototype`` archiver is not web accessible so it does not have a
``list_url`` or permalink. Given by Aurélien Bompard.
* Large performance improvement in ``SubscriptionService.find_members()``.
Given by Aurélien Bompard.
3.0.0 -- "Show Don't Tell"
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment