-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Send users a server notice about consent #3236
Conversation
404cc1f
to
8a807ff
Compare
"""Called when the user performs a sync operation. | ||
|
||
This is only called when /sync (or /events) is called on the synapse | ||
master. In a deployment with synchrotrons, on_user_ip is called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good reason then to have the two code paths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, except that I couldn't think of a name for the method that would cover both bases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah
synapse/handlers/presence.py
Outdated
@@ -428,6 +433,9 @@ def user_syncing(self, user_id, affect_presence=True): | |||
last_user_sync_ts=self.clock.time_msec(), | |||
)]) | |||
|
|||
# send any outstanding server notices to the user. | |||
yield self._server_notices_sender.on_user_syncing(user_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this here rather than in the sync handler? It feels odd to have this indirection, especially since I can see someone commenting out the presence handler in the sync code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason it's here is that it takes care of the fact that we don't want to run it on the workers. Agreed that it feels odd though. Alternative solutions very welcome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be tempted to move it to sync code and then make it noop, possibly by having a dummy notice sender on workers. But not sure how worth it that is.
Is something not missing a check here to make sure its only run on master worker? |
we're going to use it for the version we require too.
When a user first syncs, we will send them a server notice asking them to consent to the privacy policy if they have not already done so.
turns out we need to reuse this, so it's better in the config class.
... and have the sync endpoints call it directly rather than obsure indirection via PresenceHandler
1d6b076
to
8810685
Compare
When a user first syncs, we will send them a server notice asking them to consent to the privacy policy if they have not already done so.