log throttle events for email sending throttles (LG-4404)

[mitchellhenke]

No description provided.

[aduth]

Definitely not a blocker, but I'd love to have a bikeshed discussion about the two indentation styles in this file and the one above:

      @register_user_email_form = RegisterUserEmailForm.new(
        recaptcha_results: validate_recaptcha,
        analytics: analytics,
      )

      user, result = RequestPasswordReset.new(email: email,
                                              request_id: request_id,
                                              analytics: analytics).perform

...and whether we have a preference to one or the other.

[mitchellhenke]

I usually do the former, and assumed rubocop would fix the latter. Changed to be consistent with the first option in c4bcc70

I'm also happy to bikeshed. The closest checks in Rubocop I could find are:

• https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Layout/ArgumentAlignment
• https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Layout/MultilineMethodCallBraceLayout
• https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Layout/MultilineMethodArgumentLineBreaks

It doesn't look like there are any options to require the first style above. The MultilineMethodCallBraceLayout has a new_line option that gets close, but isn't quite it.

[aduth]

Yeah, my preference would also be to the first, since I find it's much easier to implement and maintain and more consistent with most languages I've experience with (and that we use, e.g. JavaScript). But ultimately I care less which style it is, and more that there is a preferred style I can align to 😄

In addition to the cops you found, this one might be relevant as well:

https://docs.rubocop.org/rubocop/cops_layout.html#layoutfirstmethodargumentlinebreak

This mix of configuration seems to get the desired result in my initial testing:

Layout/ArgumentAlignment:
  Enabled: true
  EnforcedStyle: with_fixed_indentation

Layout/MultilineMethodCallBraceLayout:
  Enabled: true

Layout/FirstMethodArgumentLineBreak:
  Enabled: true

[mitchellhenke]

ah nice, that does it. Running with those options lists 558 issues, with most in the specs. Not sure on the best strategy for fixing that 😬

[aduth]

If it's auto-correctable, maybe we can do a quick mega-PR to fix them all at once?

[mitchellhenke]

I ran it, and it does auto-correct all of them, but results in some unexpected fixes:

-      params.require(:edit_phone_form).permit(:delivery_preference,
-                                              :make_default_number)
+      params.require(:edit_phone_form).permit(
+        :delivery_preference,
+        :make_default_number,
+)

-      prompt_to_confirm_phone(id: user_session[:phone_id], phone: @new_phone_form.phone,
+      prompt_to_confirm_phone(
+id: user_session[:phone_id], phone: @new_phone_form.phone,
                               selected_delivery_method: @new_phone_form.otp_delivery_preference,
-                              selected_default_number: @new_phone_form.otp_make_default_number)
+                              selected_default_number: @new_phone_form.otp_make_default_number
+)

I pushed all of the changes to a branch here: https://github.com/18F/identity-idp/compare/rubocop-multiline-method-calls?expand=1. I'd be happy to pair on this for a bit this week to see if we can iron out those with some option changes.

[aduth]

I pushed all of the changes to a branch here: https://github.com/18F/identity-idp/compare/rubocop-multiline-method-calls?expand=1. I'd be happy to pair on this for a bit this week to see if we can iron out those with some option changes.

Yeah, I'd like that 👍 I created some time tomorrow to work through this.

[zachmargolis]

Big fan of the new approach (except that weird ending paren all the way on the left site)

[aduth]

Is this doing anything?

Do we not have any coverage for throttled flows in these specs?

[mitchellhenke]

It is not but it should be since this was a note of sorts to myself to add a test. Added a test in 3d1780d

[zachmargolis]

LGTM