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

Add configuration for dropping read receipts #10177

Closed
wants to merge 13 commits into from
Closed

Add configuration for dropping read receipts #10177

wants to merge 13 commits into from

Conversation

EdGeraghty
Copy link

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.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

@EdGeraghty
Copy link
Author

Signed-off-by: Ed Geraghty ed@geraghty.london

@EdGeraghty EdGeraghty mentioned this pull request Jun 14, 2021
Open
@erikjohnston erikjohnston requested a review from a team June 15, 2021 09:24
@clokep clokep changed the title Edgeraghty/read receipts Add configuration for dropping read receipts Jun 15, 2021
richvdh
richvdh suggested changes Jun 17, 2021
View reviewed changes
changelog.d/10177.feature Outdated Show resolved Hide resolved
synapse/server.py Outdated Show resolved Hide resolved
synapse/handlers/receipts.py Outdated Show resolved Hide resolved
synapse/config/server.py Show resolved Hide resolved
synapse/handlers/read_marker.py Outdated Show resolved Hide resolved
synapse/handlers/receipts.py Outdated Show resolved Hide resolved
@richvdh richvdh added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jun 17, 2021
Ed Geraghty and others added 12 commits June 17, 2021 16:42
Add and expose send_read_receipts setting to our homeserver.yaml
a653499 
Default to `true` since this is the expected behaviour
Stop sending read receipts if they're disabled in our homeserver.yaml
b1d8153
Add to changelog.d
f6de65c 
Signed-off-by: Ed Geraghty <ed@geraghty.london>
Update 10177.feature
99c121b
Update sample_config.yaml with the new read receipt setting
74e99ae
Expose & use whether to send read receipts to our ReceiptEventSource
809d193
@richvdh
Update changelog.d/10177.feature
a7be8ad 
No need to explicitly include PR number

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Remove redundant (unreachable) code
83a80c8
No need for an accessor here, just access directly
113c92d
Expand the description in the config
0f26fc0
Don't stop sending read markers
9a03f88
Make sure pushers are notified of new receipts
abf2b55
@EdGeraghty EdGeraghty marked this pull request as draft June 17, 2021 15:45
@EdGeraghty EdGeraghty marked this pull request as ready for review June 17, 2021 15:55
@ShadowJonathan
Copy link
Contributor

This PR doesn't look to be dropping read receipts as they arrive, so they're still processed and stored in the database, is that intentional?

@EdGeraghty
Copy link
Author

EdGeraghty commented Jun 17, 2021 •
edited
Loading

Isn't storing/processing an individuals' read receipt required to make sure we don't end up with sticky "unread" notifications in the client?

@EdGeraghty EdGeraghty requested a review from richvdh June 17, 2021 16:50
richvdh
richvdh reviewed Jun 18, 2021
View reviewed changes
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Am I right in understanding that this means we will still send read receipts over federation? Is that deliberate? It seems a very curious choice that we would share read receipts with users on other servers, but not our own users.

Another thing: it looks like this means that we won't share read receipts between a user's own devices, so when the user switches device, he won't know where he's read up to.

@richvdh
Copy link
Member

richvdh commented Jun 18, 2021

Isn't storing/processing an individuals' read receipt required to make sure we don't end up with sticky "unread" notifications in the client?

yes.

@EdGeraghty
Copy link
Author

Am I right in understanding that this means we will still send read receipts over federation? Is that deliberate?

No it's not. I'm guessing I've removed one too many returns?

Another thing: it looks like this means that we won't share read receipts between a user's own devices, so when the user switches device, he won't know where he's read up to.

Which is literally the one behaviour I wasn't trying to stop.

I'll keep headbutting this wall for a little while...

@EdGeraghty EdGeraghty marked this pull request as draft June 19, 2021 10:47
@anoadragon453
Copy link
Member

Note that support for read receipt privacy at the per-account level in Matrix is currently being added: matrix-org/matrix-spec-proposals#2285, and an implementation has already landed in Synapse: #10413

Which may be enough to solve the original problem proposed here?

@EdGeraghty
Copy link
Author

yep! Glad someone's done the hard bit for me :)

@EdGeraghty EdGeraghty closed this Aug 18, 2021
@EdGeraghty EdGeraghty deleted the edgeraghty/read-receipts branch August 18, 2021 18:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@richvdh richvdh richvdh requested changes

Assignees
No one assigned
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@EdGeraghty @ShadowJonathan @richvdh @anoadragon453