Refactor email summary generation.

[clokep]

Split out of #9257.

This is reviewable commit-by-commit. For the first commit you likely want to disable whitespace.

It has two changes:

• The second commit fixes a case where no summary text was returned.
• The use of messages_from_person vs. messages_from_person_and_others was tweaked slightly to be based on whether there was 1 sender or multiple senders, not based on if there was 1 room or multiple rooms.

[erikjohnston]

This changes the behaviour of the summary returned to be based on the number of senders, instead of based on the number of rooms.

Is that what we want? I'm failing to grok really what that means, as I'd have naively thought it'd be weird to have message from one sender, but in different rooms, look like they were from the same room?

[clokep]

This changes the behaviour of the summary returned to be based on the number of senders, instead of based on the number of rooms.

Is that what we want? I'm failing to grok really what that means, as I'd have naively thought it'd be weird to have message from one sender, but in different rooms, look like they were from the same room?

I don't know, but I think it previously didn't make sense.

In the case of a single room with multiple notifications and no room name we used (call this Case A):

synapse/synapse/config/emailconfig.py

Line 37 in fa50e4b

"messages_from_person": "[%(app)s] You have messages on %(app)s from %(person)s...",

In the case of multiple rooms and the room causing the notification to be unnamed we used (call this Case B):

synapse/synapse/config/emailconfig.py

Line 40 in fa50e4b

"messages_from_person_and_others": "[%(app)s] You have messages on %(app)s from %(person)s and others...",

In both cases the person is from descriptor_from_member_events:

synapse/synapse/push/presentable_names.py

Lines 168 to 193 in 6f7417c

	def descriptor_from_member_events(member_events: Iterable[EventBase]) -> str:
	"""Get a description of the room based on the member events.
	Args:
	member_events: The events of a room.
	Returns:
	The room description
	"""
	member_events = list(member_events)
	if len(member_events) == 0:
	return "nobody"
	elif len(member_events) == 1:
	return name_from_member_event(member_events[0])
	elif len(member_events) == 2:
	return "%s and %s" % (
	name_from_member_event(member_events[0]),
	name_from_member_event(member_events[1]),
	)
	else:
	return "%s and %d others" % (
	name_from_member_event(member_events[0]),
	len(member_events) - 1,
	)

So for Case A this was generally OK, but for Case B this could end up with:

You have messages on FooApp from Bob and 3 others and others...

So the idea is that for both of them we'll use the number of senders, which will end up with reasonable messages, I hope! This change is all in 6f33c1e I think.

[erikjohnston]

Oh, OK. For some reason I was blanking that this was just for the summary, rather than the full email...

[clokep]

Yeah, this is only for the summaries! And the changed cases are only in the case of the room not having a name, which I don't think is common? 🤷