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

add user_directory #1096

Merged
merged 7 commits into from
Dec 29, 2017
Merged

add user_directory #1096

merged 7 commits into from
Dec 29, 2017

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Dec 17, 2017

No description provided.

@turt2live
Copy link
Member

operationId is missing in /user_directory/search, verb post

@ara4n ara4n requested a review from richvdh December 18, 2017 00:40
@richvdh richvdh self-assigned this Dec 18, 2017
@richvdh
Copy link
Member

richvdh commented Dec 18, 2017

I think this fixes #953

@@ -0,0 +1,93 @@
# Copyright 2016 OpenMarket Ltd
Copy link
Member

Choose a reason for hiding this comment

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

it's not 2016, and you don't work for openmarket. (you may be after New Vector Ltd or Michael Telatynski, depending which hat you feel like wearing)

Copy link
Member Author

Choose a reason for hiding this comment

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

the woes of Copying entire files and my IDE collapsing the header and never actually seeing this

# limitations under the License.
swagger: '2.0'
info:
title: "Matrix Client-Server Profile API"
Copy link
Member

Choose a reason for hiding this comment

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

not the profile api

@@ -76,6 +76,9 @@ r0.3.0
- ``GET /media/{version}/preview_url``
(`#1064 <https://github.com/matrix-org/matrix-doc/pull/1064>`_).

- ``POST /user_directory/search``
Copy link
Member

Choose a reason for hiding this comment

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

Needs to go under 'unreleased changes', please.

$ref: definitions/security.yaml
paths:
"/user_directory/search":
post:
Copy link
Member

Choose a reason for hiding this comment

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

as Travis says, this is missing its operationId.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

required: ["search_term"]
responses:
200:
description: The results of the paginated search.
Copy link
Member

Choose a reason for hiding this comment

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

erm, "the paginated results of the search", surely. Or probably just "The results of the search".

example: "foo"
limit:
type: number
description: The maximum number of results to return
Copy link
Member

Choose a reason for hiding this comment

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

if this is optional, what's the behaviour when it's omitted?

post:
summary: Searches the user directory.
description: |-
This API paginates over search results of the user directory.
Copy link
Member

Choose a reason for hiding this comment

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

I think this emphasises the pagination too strongly. Not least because aiui pagination isn't currently supported. How about "Performs a server-side search over all users registered on the server".

Copy link
Member

Choose a reason for hiding this comment

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

could you also give some details about how the search works. Is it case-sensitive? Does it search displayname, mxid, both?

items:
title: User
type: object
properties:
Copy link
Member

Choose a reason for hiding this comment

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

is user_id not required here?

type: object
required: ["results", "limited"]
properties:
results:
Copy link
Member

Choose a reason for hiding this comment

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

do we know anything about the ordering of the results?

Copy link
Member Author

Choose a reason for hiding this comment

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

vaguely, seems implementation specific

@@ -1335,6 +1335,13 @@ Listing rooms

{{list_public_rooms_cs_http_api}}


Users
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be worth merging this and profiles into a generic user data section.

@t3chguy
Copy link
Member Author

t3chguy commented Dec 18, 2017

back to you @richvdh

This API paginates over search results of the user directory.
This API performs a server-side search over all users registered on the server.
Searches MXID and displayname case-insesitively for users that you share a room with or that are in public rooms.
operationId: postUserDirectorySearch
Copy link
Member

Choose a reason for hiding this comment

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

why postUserDirectorySearch? seems inconsistent with everything else.

@@ -31,7 +31,9 @@ paths:
post:
summary: Searches the user directory.
description: |-
This API paginates over search results of the user directory.
This API performs a server-side search over all users registered on the server.
Searches MXID and displayname case-insesitively for users that you share a room with or that are in public rooms.
Copy link
Member

Choose a reason for hiding this comment

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

insesitively

@@ -31,7 +31,9 @@ paths:
post:
summary: Searches the user directory.
description: |-
This API paginates over search results of the user directory.
This API performs a server-side search over all users registered on the server.
Searches MXID and displayname case-insesitively for users that you share a room with or that are in public rooms.
Copy link
Member

Choose a reason for hiding this comment

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

full sentence here please.

@@ -46,12 +48,12 @@ paths:
example: "foo"
limit:
type: number
description: The maximum number of results to return
description: The maximum number of results to return (10 if omitted), with a maximum of 50
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that the maximum here needs speccing, and if it does, it probably belongs in a note in the api description rather than that of the summary.

@@ -46,12 +48,12 @@ paths:
example: "foo"
limit:
type: number
description: The maximum number of results to return
description: The maximum number of results to return (10 if omitted), with a maximum of 50
Copy link
Member

Choose a reason for hiding this comment

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

suggest you use 'Defaults to 10' instead of (10 if omitted).

rav@fred:~/work/matrix-doc (master $%=)$ grep -ir 'if omitted' api/client-server/ | wc -l
1
rav@fred:~/work/matrix-doc (master $%=)$ grep -ir 'Defaults to' api/client-server/ | wc -l
6

@@ -46,12 +48,12 @@ paths:
example: "foo"
limit:
type: number
description: The maximum number of results to return
description: The maximum number of results to return (10 if omitted), with a maximum of 50
Copy link
Member

Choose a reason for hiding this comment

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

we seem to be using full-stops on these descriptions in general

@@ -31,7 +31,9 @@ paths:
post:
summary: Searches the user directory.
description: |-
This API paginates over search results of the user directory.
This API performs a server-side search over all users registered on the server.
Searches MXID and displayname case-insesitively for users that you share a room with or that are in public rooms.
Copy link
Member

Choose a reason for hiding this comment

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

MXID isn't actually used very much in the spec (and where it is, I think it's a bug). Suggest 'user ID'.

properties:
user_id:
type: string
example: "@foo:bar.com"
description: The MXID of the user.
Copy link
Member

Choose a reason for hiding this comment

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

"The user's matrix user ID"


{{users_cs_http_api}}


Profiles
--------
~~~~~~~~
Copy link
Member

Choose a reason for hiding this comment

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

"Events on Change of Profile Information" is no longer included under "Profiles", which seems odd

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

\o/ perfect

@richvdh richvdh merged commit 73118b6 into matrix-org:master Dec 29, 2017
@richvdh
Copy link
Member

richvdh commented Feb 20, 2018

This was added to synapse in matrix-org/synapse#2252, ftr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants