Ab addtl pkey formatting by el-mapache · Pull Request #1152 · 18F/identity-idp

el-mapache · 2017-02-28T18:40:08Z

There is a discrepency between how errors are displayed due to how we process the personal key on the profile reactivation screen and the personal key otp screen:

reactivation:

otp screen:

We will want to standardize this one way or the other, I'm not sure what direction we are going with regards to errors on form elements vs the entire form, so I'll probably open up an issue. Or maybe just having a banner alert is better in this case, since the fields will always be either all valid or all invalid?

**Why**: dry up the tests and encapsulate the recovery code entry logic

**Why**: Multiple views need the ability to enter a recovery code, so we break it out. The `ReactivateProfileController` needed to be aware of `recovery_code` as an array

zachmargolis · 2017-02-28T19:24:57Z

app/views/users/reactivate_profile/index.html.slim

 p.mt-tiny.mb0 = t('forms.reactivate_profile.instructions')
 = simple_form_for(@reactivate_profile_form, url: reactivate_profile_path,
  html: { autocomplete: 'off', method: :post, role: 'form' }) do |f|
+  - field_name = "#{f.object_name}[#{:recovery_code}][]"


the #{:recovery_code} is a little redudant...it can just be recovery_code, like:

field_name = "#{f.object_name}[recovery_code][]"

zachmargolis · 2017-02-28T19:25:26Z

app/views/partials/personal_key/_entry_fields.html.slim

+.mb3
+  label.block.bold#personal-key-label
+    = t('simple_form.required.html') + t('forms.two_factor.recovery_code')
+  - Figaro.env.recovery_code_length.to_i.times do |_|


same feedback as last time, can we drop the block argument here? or is that a slim thing? ex:

times.do

zachmargolis · 2017-02-28T19:28:21Z

app/controllers/users/reactivate_profile_controller.rb

+        permit({:recovery_code => []}, :password).
+        tap do |obj|
+          code = obj[:recovery_code]
+          obj[:recovery_code] = code.join(' ') unless obj[:recovery_code].nil?


We can probably just move the code.join(' ') inside the ReactivateProfileForm object?

Also I bet we could change the permit line to remove the extra curly braces:

permit(:password, recovery_code: [])

It seemed easier to do it here, since there didn't seem a good place to manipulate the recovery code. I guess after the super, maybe?

The second comment here is odd to me, why does the order of the keys matter to rails? For example, if I put what I was trying initially: permit(recovery_code: [], :password), I get an error.

Ok well if not ordering, let's at least use the new-style hash syntax?

permit({ recovery_code: [] }, :password)

Also, I just made a test case, I believe that the syntax permit(:password, recovery_code: []) should definitely work:

params = ActionController::Parameters.new(reactivate_profile_form: { password: 'foobar', recovery_code: %w(a b c d) }) => {"reactivate_profile_form"=>{"password"=>"foobar", "recovery_code"=>["a", "b", "c", "d"]}} params[:reactivate_profile_form].permit(:password, recovery_code: []) => {"password"=>"foobar", "recovery_code"=>["a", "b", "c", "d"]}

So, my question wasn't whether or not it would work, but rather if you knew why rails accepted the new hash syntax when placed after the :password, but not before.

zachmargolis · 2017-02-28T19:30:34Z

app/views/users/reactivate_profile/index.html.slim

  = f.error :base
-  = f.input :recovery_code, required: true
+  = f.error :recovery_code
+  = render 'partials/personal_key/entry_fields', field_name: field_name


I would move this partial to the shared directory, such as app/views/shared/_personal_key_fields.html.slim or something, since we put a lot of shared partials there already

zachmargolis · 2017-03-02T14:31:54Z

app/forms/reactivate_profile_form.rb

this is not a destructuring assignment like it is in JS, the reason recovery_code works is because it's after the super call and it's using the attr_accessor-defined method. I would just leave this as attrs = {}

I get that, but we do want a fallback in the case of recovery_code being nil, correct? In the show action we call @reactivate_profile_form = ReactivateProfileForm.new(current_user), which obviously doesn't include a recovery code.

Is it better practice to supply an explicit argument vs rely on a default param in the function signature?

cool, I see that I just need to specify that in the method.

Ah, I understand the goal now. I still think it should be attrs = {} because you could pass in a hash with password but not recovery_code and then recovery code would still be nil, not an array.

I think this would be clearer with like a attrs[:recovery_code] ||= [] in the first line before we call super, that will make sure we don't have nil

zachmargolis · 2017-03-02T14:48:23Z

app/inputs/array_input.rb

What is this used in?

Nevermind, I see the as: :array

**Why**: Needed simple form hooks to be able to have multiple text inputs described by a single label, with discrete error classes on each

**Why**: To leverage simple form

zachmargolis · 2017-03-02T19:45:28Z

app/forms/reactivate_profile_form.rb

  attr_reader :user

  def initialize(user, attrs = {})
+    options = { recovery_code: [] }.merge(attrs)


I'd recommend attrs[:recovery_code] ||= {} rather than a merge like this, the case I'm imagining is that somehow, we get passted in { recovery_code: nil } which will get through here and cause an error when trying to join below

zachmargolis

LGTM!

el-mapache · 2017-03-02T19:56:04Z

thanks!!

**Why**: In case `recovery_code: nil` is passed in

* Add helper method to specs for recovery code entry **Why**: dry up the tests and encapsulate the recovery code entry logic * Add partial + update reactivate_profile controller **Why**: Multiple views need the ability to enter a recovery code, so we break it out. The `ReactivateProfileController` needed to be aware of `recovery_code` as an array * Fix specs, add simple_form ext, code clean up **Why**: Needed simple form hooks to be able to have multiple text inputs described by a single label, with discrete error classes on each * Make the RecoveryCodeForm behave more like a model **Why**: To leverage simple form * Directly set the attrs object **Why**: In case `recovery_code: nil` is passed in

el-mapache added 2 commits February 28, 2017 10:07

Add helper method to specs for recovery code entry

3489911

**Why**: dry up the tests and encapsulate the recovery code entry logic

Add partial + update reactivate_profile controller

a7fe38d

**Why**: Multiple views need the ability to enter a recovery code, so we break it out. The `ReactivateProfileController` needed to be aware of `recovery_code` as an array

el-mapache added the status - work in progress label Feb 28, 2017

el-mapache self-assigned this Feb 28, 2017

zachmargolis reviewed Feb 28, 2017

View reviewed changes

el-mapache force-pushed the ab-addtl-pkey-formatting branch from 1e94e59 to 59c00d4 Compare March 2, 2017 14:29

zachmargolis reviewed Mar 2, 2017

View reviewed changes

Fix specs, add simple_form ext, code clean up

d49bbac

**Why**: Needed simple form hooks to be able to have multiple text inputs described by a single label, with discrete error classes on each

el-mapache force-pushed the ab-addtl-pkey-formatting branch from 59c00d4 to d49bbac Compare March 2, 2017 17:07

Make the RecoveryCodeForm behave more like a model

aafc8af

**Why**: To leverage simple form

el-mapache added status - ready for review and removed status - work in progress labels Mar 2, 2017

zachmargolis reviewed Mar 2, 2017

View reviewed changes

zachmargolis approved these changes Mar 2, 2017

View reviewed changes

Directly set the attrs object

0e6a0cc

**Why**: In case `recovery_code: nil` is passed in

el-mapache force-pushed the ab-addtl-pkey-formatting branch from 0af0496 to 0e6a0cc Compare March 2, 2017 20:02

el-mapache merged commit c942a67 into master Mar 2, 2017

el-mapache deleted the ab-addtl-pkey-formatting branch March 2, 2017 20:14

Conversation

el-mapache commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

el-mapache Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis Mar 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

el-mapache Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

el-mapache commented Mar 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

el-mapache commented Feb 28, 2017 •

edited

Loading

el-mapache Feb 28, 2017 •

edited

Loading

zachmargolis Mar 1, 2017 •

edited

Loading

el-mapache Mar 2, 2017 •

edited

Loading