-
Notifications
You must be signed in to change notification settings - Fork 112
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
Remove all helper_method
usage from IDV controllers
#1340
Conversation
**WHY**: Better to include in view model (refactor)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| #{idv_params[:address2]}<br> | ||
| #{idv_params[:city]}, #{idv_params[:state]} #{idv_params[:zipcode]} | ||
| #{@idv_params[:address1]}<br> | ||
- if @idv_params[:address2].present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related but not, we do some very similar formatting + logic on the home/profile page to support addresses, I'm going to file a ticket to see if we can . refactor that into a partial?
@@ -7,7 +7,7 @@ | |||
before do | |||
user = build_stubbed(:user, :signed_up) | |||
allow(view).to receive(:current_user).and_return(user) | |||
allow(view).to receive(:idv_params).and_return( | |||
@idv_params = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it more common to use assign(:idv_params, ...)
? Does it matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooh maybe it is? I am not sure. Looks like we are doing both right now. which do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we had to use assigns, but since this works, and other tests do the same, not worth worrying about on this PR.
WHY: Better to include in view model (refactor)