Conversation
zachmargolis
requested review from
el-mapache,
hursey013,
jessieay and
monfresh
jessieay
left a comment
jessieay
left a comment
There was a problem hiding this comment.
Nice one! Two comments for your consideration re: controller / method naming, but you can take or leave that advice as you see fit. 🚿
There was a problem hiding this comment.
rails controllers are usually plural -- thoughts on calling this AccountsController ?
There was a problem hiding this comment.
Yeah that seems reasonable, I'll pluralize this (and all the partials that go with it 🙈 )
There was a problem hiding this comment.
if we called this AccountsController, we'd probably want this to be show rather than index
There was a problem hiding this comment.
Good idea! On it
There was a problem hiding this comment.
I just remebered Rails' support for singular resources: WDYT of AccountController#show:
http://guides.rubyonrails.org/routing.html#singular-resources
There was a problem hiding this comment.
although I think what that is saying is that you still have a plural controller but just name the route as get 'profile', to: 'users#show' ?
There was a problem hiding this comment.
ah you're right, for some reason I thought it was saying the controller would be singularly named, but I looked more closely. keeping plural controller!
spec/requests/redirects_spec.rb
Outdated
There was a problem hiding this comment.
niiiice -- will we want to remove this redirect at some point ?
There was a problem hiding this comment.
Since we have an app in production now that people may have linked to...I think we want to keep this as long as possible? Maybe we can check back in 6 months and see if it was ever used?
**Why**: Matches the copy already on the page
zachmargolis
force-pushed
the
margolis-rename-to-account
branch
from
a5cda68 to
2c16b0b
Compare
2 participants
Why: Matches the copy already on the page
Open question is if we want to rename
/profile/verifyand/profile/reactivate, for the time being I left them, and we can do that in another PR if we want.Makes these things the same: