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 getThirdpartyUser to base api #589

Merged
merged 2 commits into from
Dec 18, 2018

Conversation

johbo
Copy link
Contributor

@johbo johbo commented Dec 31, 2017

I investigated options to build a new bridge for XMPP and found that this API method would be very useful for it. My assumption is that it could be used to enhance the invite dialog in a way so that it also understands protocols and external IDs, similar to the way how the room directory currently works.

Signed-off-by: Johannes Bornhold johannes@bornhold.name

Signed-off-by: Johannes Bornhold <johannes@bornhold.name>
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Otherwise this looks fine, thanks!


return this._http.authedRequestWithPrefix(
undefined, "GET", path, params, undefined,
httpApi.PREFIX_UNSTABLE,
Copy link
Member

Choose a reason for hiding this comment

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

This is in the v2 API now so if we're adding it, we should probably add it using the stable endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the same be tweaked for getThirdpartyProtocols ? I assume they go together.

Copy link
Member

Choose a reason for hiding this comment

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

Good question - since these are existing APIs, we should probably leave them using unstable for a while as not all servers will have updated to expose the APIs on the stable path yet.

@dbkr
Copy link
Member

dbkr commented Jan 3, 2018

Also please note you need to do the DCO signoff as per https://github.com/matrix-org/matrix-js-sdk/blob/master/CONTRIBUTING.rst

@johbo
Copy link
Contributor Author

johbo commented Jan 3, 2018

Had the sign off already in the commit. I've added it into the PR comment as well. Does it work this way?

@dbkr
Copy link
Member

dbkr commented Jan 3, 2018

oh, I missed that, either is fine (although if it's on the commits it should technically be on all of them)

@Half-Shot
Copy link
Contributor

out of interest, what's holding this up?

@jryans
Copy link
Collaborator

jryans commented Dec 18, 2018

This appears ready to merge to me. It has been reviewed and comments were replied to. We have sign off on 1 of 2 commits and the PR as a whole, which seems like sufficient coverage.

@jryans jryans merged commit 26893b9 into matrix-org:develop Dec 18, 2018
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.

4 participants