Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Recieving Room Shares #5775

Merged
merged 5 commits into from
Jul 16, 2021
Merged

Implement Recieving Room Shares #5775

merged 5 commits into from
Jul 16, 2021

Conversation

gary-kim
Copy link
Member

@gary-kim gary-kim commented Jun 15, 2021

For #5723

Also have a version that avoids adding and extra table here: https://github.com/nextcloud/spreed/tree/enh/5723%2Fadd-database-single

@nickvergessen
Copy link
Member

What is talk_federated_servers vs. talk_rooms_external

From my POV they could be part of existing tables?
An outgoing federation is just a new row in the attendees table with actorType federation and actorId <cloudid>

An incoming federation creates a room in the room table with token and the new field federation-server. Afterwards the individual users get their own records in the attendees table again.

this means the existing database queries are enough and we don't add another one when getting the users rooms or the participants in a room

@gary-kim
Copy link
Member Author

gary-kim commented Jun 15, 2021

Outgoing federation sounds good.

Incoming federations in the room table might be problematic since each new user is given a separate secret. Would it be okay to have a room per user? So if 4 users on a local server is participating in a federated room hosted on another server, it's handled as if it is 4 rooms on the local server.

We could also handle it such that each room from a remote server is a single room on the local server then the attendees table is used to keep track of the number of the token for each user that is in the room. That'd mean two new actor types, one for federated (users on another server) and one for shared (users on the local server that have gotten an incoming share).

cc @nickvergessen

@nickvergessen nickvergessen added this to the 💖 Next Major (23) milestone Jun 18, 2021
@gary-kim gary-kim force-pushed the enh/5723/add-database branch 2 times, most recently from cb64a33 to 7069c9e Compare June 23, 2021 03:54
@gary-kim gary-kim force-pushed the enh/5723/add-database branch 4 times, most recently from 286ef27 to a44c90b Compare June 25, 2021 22:43
@gary-kim gary-kim changed the title WIP: Implement Recieving Room Shares Implement Recieving Room Shares Jun 25, 2021
@gary-kim gary-kim marked this pull request as ready for review June 25, 2021 22:44
@gary-kim gary-kim force-pushed the enh/5723/add-database branch 7 times, most recently from f9d38a1 to c7ba09b Compare June 26, 2021 03:57
appinfo/routes.php Outdated Show resolved Hide resolved
@gary-kim gary-kim force-pushed the enh/5723/add-database branch 2 times, most recently from 1f3b6b2 to 994f629 Compare July 2, 2021 21:14
@nickvergessen
Copy link
Member

What was the plan with integration tests? Same PR or follow up?
I'd prefer to have it in here as there is no UI to test something.

@gary-kim
Copy link
Member Author

gary-kim commented Jul 8, 2021

This is purely receiving so it's a bit difficult to do integration tests at the moment. It's part of the share sending PR though, no worries.

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Looks good otherwise

@nickvergessen

This comment has been minimized.

gary-kim and others added 5 commits July 15, 2021 13:54
Signed-off-by: Gary Kim <[email protected]>
Signed-off-by: Gary Kim <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
@gary-kim
Copy link
Member Author

Rebased to resolve conflict

@nickvergessen nickvergessen merged commit 56e1d3d into master Jul 16, 2021
@nickvergessen nickvergessen deleted the enh/5723/add-database branch July 16, 2021 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants