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

Send users a server notice about consent #3236

Merged
merged 4 commits into from May 22, 2018
Merged

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented May 18, 2018

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.

"""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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Ah

@@ -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)
Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

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.

@erikjohnston
Copy link
Member

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
@richvdh richvdh merged commit 8aeb529 into develop May 22, 2018
@richvdh richvdh deleted the rav/consent_notice branch July 10, 2018 12:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants