Skip to content

Conversation

@snewcomer
Copy link
Contributor

What's in this PR?

password reset controller finding AuthToken.

@begedin should I also change the AuthToken model value to token?

References

Fixes #731

Progress on: #498

@snewcomer
Copy link
Contributor Author

Wondering if we can just look up the token or should look up the token/user in the AuthToken model.

@joshsmith
Copy link
Contributor

This is looking pretty good. Where did tests go down?

Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

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

Looks good, @snewcomer

There is no need to change the property name in the model itself. My suggestion was just about having the public API interface be clear to the outside client. The model is internal to the API and should be clearer to the dev, so auth_token.value will make more sense that way. This is good to go.

@begedin
Copy link
Contributor

begedin commented Mar 15, 2017

@joshsmith Looks like tests going down is coveralls being coveralls. It reports them going down in files untouched by this PR.

@snewcomer You'll have to rebase and rerun migrations to deal with the structure.sql conflict, but this is good to merge after.

@snewcomer snewcomer force-pushed the reset-password-route branch from 2e99fcf to 37217d2 Compare March 15, 2017 18:50
@snewcomer
Copy link
Contributor Author

Going to squash and merge!

@snewcomer snewcomer merged commit 7426c45 into develop Mar 15, 2017
@snewcomer snewcomer deleted the reset-password-route branch March 15, 2017 19:04
@joshsmith
Copy link
Contributor

🎉 yay!

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