-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4472 +/- ##
===========================================
+ Coverage 74.69% 74.75% +0.05%
===========================================
Files 336 338 +2
Lines 34293 34650 +357
Branches 5592 5668 +76
===========================================
+ Hits 25614 25901 +287
- Misses 7095 7149 +54
- Partials 1584 1600 +16 |
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.
looks good otherwise
@@ -118,6 +119,7 @@ class RoomVersions(object): | |||
RoomVersions.V2, | |||
RoomVersions.VDH_TEST, | |||
RoomVersions.STATE_V2_TEST, | |||
RoomVersions.V3, |
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.
We can't add V3 here yet, because doing so will allow us to accept joins/creates for V3 rooms, but won't actually implement the things that make V3 V3.
(sticking this back into the queue because I need it for Riot - sorry if I've picked the wrong person or something) |
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.
Implementation for matrix-org/matrix-spec-proposals#1804 |
"state-v2-test": "unstable", | ||
} | ||
}, | ||
"m.change_password": change_password, |
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.
err, spec wants this to be {}
at a minimum, not a boolean directly. I think we should modify the MSC to have this be the capability:
"m.change_password": {
"enabled": true
}
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.
Done
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.
Looks good, just a few nits
synapse/api/constants.py
Outdated
@@ -104,6 +104,7 @@ class ThirdPartyEntityKind(object): | |||
class RoomVersions(object): | |||
V1 = "1" | |||
V2 = "2" | |||
V3 = "3" |
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 don't think it matters, but I'm a bit surprised this is still in here.
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.
This comment is now updated due to #4515
}, | ||
} | ||
}) | ||
) |
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.
It'd be nicer if you could move this out of the defer.returnValue
, i.e. something like:
response = {
...
}
defer.returnValue((200, response))
it just makes it a lot easier to see what's going on
(200, { | ||
"capabilities": { | ||
"m.room_versions": { | ||
"default": "1", |
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.
You should pull this in from synapse.api.constants
Original proposals: * #1753 * #1804 Implementation proof: * matrix-org/synapse#4472 * matrix-org/matrix-js-sdk#830 There is one change to MSC1753 which is included in this commit. MSC1804 remains unchanged. In the original proposal, the change password capability being present was an indication that password changes were possible. It was found that this doesn't really communicate the state very well to clients in that lack of a capability (or a 404, etc) would mean that users would erroneously not be able to change their passwords. A simple boolean flag was added to assist clients in detecting this capability.
Implements MSC1753 and MSC1804.