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

Refactor verify namespaces/urls to idv #2149

Merged
merged 14 commits into from
May 10, 2018
Merged

Conversation

davemcorwin
Copy link
Contributor

WHY
Some Idv related items were namespaced under verify while others were under idv. This PR refactors all to be under the idv namespace so all are consistent while differentiating from some routes that begin with verify but are not part of the Idv flow.

URLs have also been changed from verify/ to idv/, if this is not desired, we can remove that commit and keep the name change internally.

This PR is rather large, the commits can be looked at individually to make reviewing a bit easier. There is only refactoring here, no functionality is (should be) changed.

@monfresh
Copy link
Contributor

monfresh commented May 9, 2018

Thanks for thinking about this. If we want consistency, we want to rename IdV to Verify, not the other way around. We originally renamed idv to verify to be more user-friendly. See #844

@davemcorwin
Copy link
Contributor Author

hmmmmm, thats informative. @jmhooper and I had spoken about this before so I thought it was something that would be helpful. We don't need to rehash the previous discussion and I can change everything to verify if thats what makes the most sense. I do think it can be a bit confusing with other routes named verify_password, verify_personal_key, verify_profile_phone, verify_account though.

@jmhooper
Copy link
Member

jmhooper commented May 9, 2018

We could also change only the name of the routes to /verify/*, which would be the only part of this that is user facing. I prefer that b/c, as Dave noted, we use the "verify" prefix for a lot of things related to auth so it doesn't signal "identity verification" well.

@davemcorwin
Copy link
Contributor Author

On behalf of @andrewhughey , he prefers we keep the verify urls as well. In that case, do we want to leave the internal namespace as Idv?

@jmhooper
Copy link
Member

jmhooper commented May 9, 2018

I prefer Idv internally. I know that a few people have been tripped up by the verify namespace in the past.

@jmhooper
Copy link
Member

@monfresh: Do you have any objections to using Idv internally and naming the routes to /verify/*?

@monfresh
Copy link
Contributor

No objections your honor.

@jmhooper
Copy link
Member

A disappointing day to realize the :gavel: emoji is custom for our Slack and can't be used here 😞

@davemcorwin
Copy link
Contributor Author

@monfresh @jmhooper This is rdy, urls reverted to verify

Copy link
Member

@jmhooper jmhooper left a comment

Choose a reason for hiding this comment

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

💯

@davemcorwin davemcorwin merged commit 1f0c35c into master May 10, 2018
@davemcorwin davemcorwin deleted the dmc-refactor-verify-to-idv branch May 10, 2018 17:45
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.

3 participants