Add a config option to change whether unread push notification counts are per-message or per-room

[anoadragon453]

This PR adds a new config option to the push section of the homeserver config, group_unread_count_by_room. By default Synapse will group push notifications by room (so if you have 1000 unread messages, if they lie in 55 rooms, you'll see an unread count on your phone of 55).

However, it is also useful to be able to send out the true count of unread messages if desired. If group_unread_count_by_room is set to false, then with the above example, one would see an unread count of 1000 (email anyone?).

The actual config option and logic is very simple. The bulk of this PR is the tests, which are a little verbose. There also did not seem to be able existing tests for unread count, so there was quite a bit to add. More explanation in the commits.

Reviewable commit-by-commit. Based on #8818

[clokep]

I think this needs a rebase / merge of develop after #8818.

[clokep]

Seems overall good! A few minor comments.

[clokep]

Having a test helper like this take a boolean is a bit odd, instead I think it would be clearer to take an expected_count argument.

[anoadragon453]

That's true, and definitely generally the case. I think though in this case _test_push_unread_count isn't meant to be used as a helper. It's really only intended for saving on code duplication and running the above two tests.

Perhaps I could rename it to something more clear?

[clokep]

isn't meant to be used as a helper. It's really only intended for saving on code duplication and running the above two tests.

Sure, but abstracting via forking the code that way is a bit odd IMO.

Another option is to do the final assertions in the callers instead of in _test_push_unread_count?

[anoadragon453]

My worry is mainly with anybody extending these tests in the future (which may not be a necessary worry and one that can be a bridge crossed when we get to it!), but my line of thinking is currently:

• With expected_count we're toggling on a single variable when multiple may need to be changed in the future. A boolean for group_by_room would then be simpler.
• Finalizing the code at the end in each function would require possible variables such as event IDs, room IDs etc to be returned by _test_push_unread_count to the calling function, which could be something else that gets a bit messy in the future.

However, I'm probably just making a mountain out of a molehill here, and taking your initial suggestion will likely be just fine for the current requirements of these tests.

[clokep]

Finalizing the code at the end in each function would require possible variables such as event IDs, room IDs etc to be returned by _test_push_unread_count to the calling function, which could be something else that gets a bit messy in the future.

I don't follow this, I'm suggesting doing something like this:

def test_push_unread_count_group_by_room(self):
    self._test_push_unread_count()

   # This push should still only contain an unread count of 1 (for 1 unread room)
   self.assertEqual(
        self.push_attempts[5][2]["notification"]["counts"]["unread"], 1
   )

[anoadragon453]

Oh, well I meant in terms of wanting to do more requests, however I forgot that everything we need to assert is stored in self.push_attempts anyways huh!

That's quite elegant to check in a separate function. I think I'll give that a shot.

[anoadragon453]

Done in c05b7ed!

stale