Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
Initial Group Server #2352
Conversation
erikjohnston
added some commits
Jul 10, 2017
Some checks were not successful
All checks have failed
All checks have failed
erikjohnston
assigned
NegativeMjark
Jul 11, 2017
| + 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") |
| + | ||
| + @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 |
NegativeMjark
Jul 11, 2017
Contributor
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 |
NegativeMjark
Jul 11, 2017
Contributor
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 |
NegativeMjark
Jul 11, 2017
Contributor
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, |
NegativeMjark
Jul 11, 2017
Contributor
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.
erikjohnston
Jul 11, 2017
Owner
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)
NegativeMjark
Jul 11, 2017
Contributor
Eh, leave it as is if you insist, but I'd like to see a comment explaining what it is :)
| + | ||
| +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 |
NegativeMjark
Jul 11, 2017
Contributor
Could you add some comments to explain what these numbers mean and what the effect of increasing or decreasing the would be?
| + @defer.inlineCallbacks | ||
| + def get_rooms_in_group(self, group_id, requester_user_id): | ||
| + """Get the rooms in group as seen by requester_user_id | ||
| + """ |
NegativeMjark
Jul 11, 2017
Contributor
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): |
erikjohnston
added some commits
Jul 11, 2017
Some checks were not successful
Some checks were not successful
| + 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 |
Some checks were not successful
NegativeMjark
approved these changes
Jul 11, 2017
LGTM apart from commenting on the purpose of the "name" column.
Some checks were not successful
erikjohnston commentedJul 10, 2017
No description provided.