Conversation
…te into s7evink/server-notices
kegsay
left a comment
kegsay
left a comment
There was a problem hiding this comment.
Mostly LGTM - how has this been tested?
setup/config/config_test.go
Outdated
| username: metrics | ||
| password: metrics | ||
| server_notices: | ||
| local_part: "server" |
There was a problem hiding this comment.
Shouldn't this be starting with _? That generally gets reserved by the HS as per AS API docs:
Exclusive user and alias namespaces should begin with an underscore after the sigil to avoid collisions with other users on the homeserver.
setup/config/config_global.go
Outdated
|
|
||
| func (c *ServerNotices) Defaults(generate bool) { | ||
| if generate { | ||
| c.LocalPart = "server" |
| }, accData); err != nil { | ||
| return nil, fmt.Errorf("unable to query account data") | ||
| } | ||
| roomData := accData.RoomAccountData[req.RoomID] |
There was a problem hiding this comment.
I don't think there is a guarantee that roomData is always non-nil? Don't you need to ok it?
clientapi/routing/server_notices.go
Outdated
| // create a new room for the user | ||
| if len(commonRooms) == 0 { | ||
| powerLevelContent := eventutil.InitialPowerLevelsContent(senderUserID) | ||
| powerLevelContent.Users[r.UserID] = -10 |
There was a problem hiding this comment.
Not 0? Please quote synapse for things like this.
| ) (*userapi.Device, error) { | ||
| var accRes userapi.PerformAccountCreationResponse | ||
| // create account if it doesn't exist | ||
| err := userAPI.PerformAccountCreation(ctx, &userapi.PerformAccountCreationRequest{ |
There was a problem hiding this comment.
This will be called every time this endpoint is invoked which surely isn't what's intended?
There was a problem hiding this comment.
Right, forgot about moving this to routing.go.
Only add routes if sever notices are enabled
Check that we actually got roomData
Since there's no spec for this, mostly manually with a local sqlite database and running curl with the required data. |
|
Hmm, we can do better than that I think. I'll see if I can hook up admin user support in Complement and then we can actually test this in CI. |
|
I'll work to add admin support in Complement today so we can test then merge this. |
|
Tests can now be added to Complement. |
…te into s7evink/server-notices
Enable server notices for CI Return same values as Synapse
| return util.JSONResponse{ | ||
| Code: leaveRes.Code, | ||
| JSON: jsonerror.Forbidden(leaveRes.Message), | ||
| JSON: jsonerror.LeaveServerNoticeError(), |
There was a problem hiding this comment.
There's no guarantee that this is because of server notices though?
There was a problem hiding this comment.
The PerformLeaveResponse struct just got the Code in 2e1e0c81a19830cbf2055a6d15c5d7424c2d0087, so right now it should be guaranteed.
There was a problem hiding this comment.
That's not very extensible or kind to the next person who wants to add an additional code :) it would be nicer to just proxy back the JSON blob to send back so instead of Message string having Message interface{} which is JSON serialisable should work.
kegsay
left a comment
kegsay
left a comment
There was a problem hiding this comment.
This is looking fantastic, and the passing tests gives me much confidence this PR does what we think it should do. Excellent work!
2 participants
This adds the unspecced server notices.
Synapse Docs:
admin api server_notices.md (for the request)
server_notices (for server side implementation details)
Due to not having the history visibility yet, a message is sent, but the user can't see it after joining the room. After joining, subsequent messages show up correctly.