Prevent duplicate push notifications for room reads

[lukasdenk]

Fixes #11704.

Make a POST to /rooms/<room_id>/receipt/m.read/<event_id> only trigger a push notification when the count of unread messages is different to the one in the last successfully sent push.

Signed-off by: Lukas Denk, [email protected]

@reivilibre:

• As I believe, every message sent to a room triggers a push notification to be sent. So is there actually a scenario in which a POST to /rooms/<room_id>/receipt/m.read/<event_id> will still trigger a push now?
• I have not written an extra test since test_push_unread_count_message_count will now fail in case a POST to /rooms/<room_id>/receipt/m.read/<event_id> erroneaously causes a push notification to be sent.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[lukasdenk]

Made WIP because I still need to change sth small

[reivilibre]

Looks good, thank you — just worth checking the tests are still testing what they're meant to test

[reivilibre]

there's a comment just above 'This will actually trigger a new notification to be sent out so that even if the user does not receive another message, their unread count goes down'...

It doesn't make much sense to me though because it was already at zero right before.

I would say 'I think it may be worth removing the comment', but this test is trying to test that pushes go out when you read something. As it stands, we're not testing that anymore.
Do you think you can adapt the test so that it will actually get the chance to send a push after a read receipt?

The test currently looks like:

• send message → send count of 0 (because we haven't got a read receipt in the room yet)
• add read receipt → send count of 0 (because we sent a read receipt) — your changes stop this from happening, so we don't actually end up testing that read receipts do anything
• send message → send count of 1

I wonder if it should look like this:

• send message → send count of 0 (because we haven't got a read receipt in the room yet)
• add read receipt → make sure nothing new has been sent (that's still a valuable piece of test information) — do it with an assert on len(push_attempts)
• send message → send count of 1
• add read receipt (to the second message) → make sure we send a count of 0
• send message → send count of 1

[lukasdenk]

Ok, I think I will start working on that issue by the beginning of next week.

[lukasdenk]

@reivilibre: I still refactored the test a bit. If you do not like it, the commit before preserves
the old coding style.

[reivilibre]

This is good, thanks: almost there!

[reivilibre]

Thanks for this one! I know it will be appreciated!

[MatMaul]

Thanks a lot @lukasdenk !