Initial Group Server #2352
Initial Group Server #2352
Conversation
erikjohnston
force-pushed
the
erikj/group_server_split
branch
from
d6dba81 to
b8ca494
Compare
synapse/groups/attestations.py
Outdated
| elif get_domain_from_id(user_id) == self.server_name: | ||
| server_name = get_domain_from_id(group_id) | ||
| else: | ||
| raise Exception("Expected eitehr group_id or user_id to be local") |
synapse/groups/attestations.py
Outdated
|
|
||
| @defer.inlineCallbacks | ||
| def verify_attestation(self, attestation, group_id, user_id, server_name=None): | ||
| """Verifies that the given attestation matches the given paramaters. |
| group_id TEXT NOT NULL, | ||
| user_id TEXT NOT NULL, | ||
| valid_until_ms BIGINT NOT NULL, | ||
| attestation TEXT NOT NULL |
There was a problem hiding this comment.
Is this json? Should some indication of this either as a comment or as a suffix on the column?
| CREATE TABLE group_rooms ( | ||
| group_id TEXT NOT NULL, | ||
| room_id TEXT NOT NULL, | ||
| is_public BOOLEAN NOT NULL |
There was a problem hiding this comment.
What does "is_public" mean? Does it mean the group is public? or does it mean the room is public? or does it mean something else?
| group_id TEXT NOT NULL, | ||
| user_id TEXT NOT NULL, | ||
| is_admin BOOLEAN NOT NULL, | ||
| is_public BOOLEAN NOT NULL |
There was a problem hiding this comment.
What does "is_public" mean? Does it mean the group is public? or does it mean the room is public? or does it mean something else?
|
|
||
| CREATE TABLE groups ( | ||
| group_id TEXT NOT NULL, | ||
| name TEXT, |
There was a problem hiding this comment.
Should "name" be called "display_name"?
There was a problem hiding this comment.
Why? We don't call room names display names?
There was a problem hiding this comment.
Because it makes it easier to tell that it is just a display name. In my opinion we don't have a strong enough convention of using "name" only when we are talking about display names that I can infer that it is a display name just because it is called "name". So I think it either needs to be called "display_name" or it needs a comment to explain what it is.
There was a problem hiding this comment.
The only time we use display name is for users... but I guess i can change it. It feels like a very odd thing to be confused about the difference between a display_name and a name (its not like its called ID or anything), especially given we've already set a precedence that we use "name" for things like this (e.g. room name)
There was a problem hiding this comment.
Eh, leave it as is if you insist, but I'd like to see a comment explaining what it is :)
synapse/groups/attestations.py
Outdated
|
|
||
| DEFAULT_ATTESTATION_LENGTH_MS = 3 * 24 * 60 * 60 * 1000 | ||
| MIN_ATTESTATION_LENGTH_MS = 1 * 60 * 60 * 1000 | ||
| UPDATE_ATTESTATION_TIME_MS = 1 * 24 * 60 * 60 * 1000 |
There was a problem hiding this comment.
Could you add some comments to explain what these numbers mean and what the effect of increasing or decreasing the would be?
synapse/groups/groups_server.py
Outdated
| @defer.inlineCallbacks | ||
| def get_rooms_in_group(self, group_id, requester_user_id): | ||
| """Get the rooms in group as seen by requester_user_id | ||
| """ |
There was a problem hiding this comment.
Could you add a something to docstring to document the order the rooms are returned in?
| }) | ||
|
|
||
| @defer.inlineCallbacks | ||
| def add_room(self, group_id, requester_user_id, room_id, content): |
There was a problem hiding this comment.
Is this called anywhere?
| group_id TEXT NOT NULL, | ||
| user_id TEXT NOT NULL, | ||
| is_admin BOOLEAN NOT NULL, -- whether the users membership can be seen by everyone | ||
| is_public BOOLEAN NOT NULL |
There was a problem hiding this comment.
Umm. Did you put the comment on the wrong line?
No description provided.