Add view model for ForgotPassword#show #1354
Conversation
**WHY**: Now we can have 1 ivar in the view instead of 3. Also no conditionals.
|
this security warning seems like a false positive to me -- anyone else? |
|
+1 I think "render path contains a parameter value" is definitely a false positive |
| session[:email] = 'test@example.com' | ||
|
|
||
| get :show | ||
|
|
||
| expect(session[:email]).to be_nil |
There was a problem hiding this comment.
I think this is still a valid assertion? Since the .delete was moved into the view model?
There was a problem hiding this comment.
yes, but now that the logic was re-shuffled it will only happen if we render the view (view.model.email is called).
Since rendering the views makes the test slower, I added a spec around session deletion in the unit test for the view model.
| @@ -0,0 +1,26 @@ | |||
| class ForgotPasswordShow | |||
| def initialize(params:, session:) | |||
There was a problem hiding this comment.
params and session only use one key each...what do you think of making those the params?
ForgotPasswordShow.new(resend: params[:resend], email: session.delete(:email))There was a problem hiding this comment.
I find it just as easy / easier to access those within the model itself. What do you see as the benefits or passing the parsed vals instead?
There was a problem hiding this comment.
I like that the view model "knows less", it's just dealing with the direct values. For example, why does the view model mutate the session currently? Seems weird to me.
However I don't think we have an established pattern here so I'm also fine leaving it as is.
There was a problem hiding this comment.
Updated at 5c73e6c
I am now passing the param[:resend] value instead of all params. I kept the session stuff as-is, because I liked having the deletion part happen in a method rather than in the controller. Easier to test that way. 🍇
**WHY**: It is OK to render a partial based on a param's presence
WHY: Now we can have 1 ivar in the view instead of 3. Also no
conditionals.