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

Clarify how to add local redirect URIs #1004

Merged

Conversation

jfly
Copy link
Contributor

@jfly jfly commented Dec 3, 2017

One of our users was confused by the wording of this message and suggested this alternative wording, which I agree is superior.

@nbulaj
Copy link
Member

nbulaj commented Jan 26, 2018

Hi @jfly . What confusing you find in phrase "local tests"? I think it is clearly explain the purpose of native redirect uris. What do you think?

@jfly
Copy link
Contributor Author

jfly commented Jan 27, 2018

CC-ing @Luis-J-Ianez, who originally told me he found the message confusing. He can probably give more insight into this than I can.

If I were going to guess, I think the word "tests" is confusing because it makes a programmer think of test scripts, not development on localhost.

@Luis-J-Ianez
Copy link

Well, it's difficult to reproduce what my mind thought first when I read "Use blablabla for local tests". As now I understand the process, I can't reproduce the mistake, so to speak. But I remember that at first I thought the "blablabla" was a kind of alias for "localhost". In my view the new wording is more helpful.

@nbulaj
Copy link
Member

nbulaj commented Jan 28, 2018

So @jfly @Luis-J-Ianez maybe we can extend this message by adding something like "for development purposes" in order to improve understanding of the help message? What do you think?

Also can you please add a changelog entry to NEWS.md with descriptive comment of the changes and rebase to quash commits? Thanks for yo patience and work!

@nbulaj nbulaj self-assigned this Jan 28, 2018
@jfly jfly force-pushed the change-native-redirect-uri-help-text branch from 50cc79f to 22bad61 Compare January 28, 2018 17:31
@jfly jfly force-pushed the change-native-redirect-uri-help-text branch from 22bad61 to 24829fe Compare January 28, 2018 17:32
@jfly
Copy link
Contributor Author

jfly commented Jan 28, 2018

Done and done!

@nbulaj nbulaj merged commit 227852e into doorkeeper-gem:master Jan 28, 2018
@nbulaj
Copy link
Member

nbulaj commented Jan 28, 2018

Great, thanks @jfly!

@jonatanklosko
Copy link

jonatanklosko commented Jun 9, 2018

Hey, this message seems no longer accurate. As far as I can see, this behaviour relayed on a bug fixed in #976, namely: if you added urn:ietf:wg:oauth:2.0:oob in the first line of redirect_uri, followed by other URLs (e.g. localhost ones) they were not validated and thus worked like a charm. #976 makes all URLs validated, so adding urn:ietf:wg:oauth:2.0:oob has no effect on other URLs.

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

Successfully merging this pull request may close these issues.

4 participants