Properly countdown time to account unlocking by el-mapache · Pull Request #772 · 18F/identity-idp

el-mapache · 2016-11-25T21:55:27Z

Why:
The time remaining displayed on the Account Locked screen never changes,
leading to a poor user experience

How:
Refactor JS to make the countdown and hours-minutes-seconds formatting
reusable

**Why**: The time remaining displayed on the Account Locked screen never changes, leading to a poor user experience **How**: Refactor JS to make the countdown and hours-minutes-seconds formatting reusable

el-mapache · 2016-11-25T22:00:26Z

If someone knows how to render the nonced_javascript_tag in slim that would be awesome! Couldn't get it to work outside of erb.

zachmargolis · 2016-11-25T22:14:03Z

app/assets/javascripts/app/hms-formatter.js

+  var remainingSeconds = parseInt(seconds % 60, 10);
+
+  var displayMinutes = minutes == 0 ? '' :
+    minutes + ' minute' + (minutes !== 1 ? 's' : '') + ' and ';


I see this didn't really change in the diff, but I'm now seeing this is not i18n-friendly. I'll make an issue to track i18n-ing this

zachmargolis · 2016-11-25T22:18:05Z

config/locales/devise/en.yml

        Your account is temporarily locked because you have entered the
        one-time passcode incorrectly too many times.
-      please_try_again: Please try again in %{time_remaining}.
+      please_try_again: Please try again in <span id="countdown">%{time_remaining}</span>.


If we rename this key please_try_again_html we don't need to manually .html_safe this in the template.

Also, I consider it a best practie to keep HTML markup outside of i18n (because of human- and machine-introduced bugs that appear in translation pipelines) so I would recommend creating the span in the template:

<%= t('devise.two_factor_authentication.please_try_again_html', time_remaining: content_tag(:span, decorated_user.lockout_time_remaining_in_words)) %>

That seems cleaner. I saw markup in another translation file and figured it was OK to put there.
I can remove the tag from that file as well, if you don't think it's outside the scope of this PR.

Yeah I'm 100% ok with fixing that translation in the other file, just didn't want to distract you accidentally

**Why**: CodeClimate doesnt like errors

**Why**: Cleaner separation of concerns

**Why**: When the account locked countdown reaches zero, the signed in modal flashes briefly, and then reloads the page. This produces an unattractive UI state to the user **How**: uses `user_fully_authenticated` helper method instead of `current_user`

**Why**: Makes shared javascript functions available under correct namespace

brendansudol · 2016-11-28T17:44:23Z

app/assets/javascripts/app/hms-formatter.js

+  return `${seconds} ${pluralize('second', seconds)}`;
+}
+
+root.hmsFormatter = function(milliseconds) {


why do we want these methods attached to our LoginGov object (vs. just a function that we export / import within the js modules that need it)?

as in export default hmsFormatter at the bottom of this file and then import hmsFormatter from './hms-formatter' in the other js file(s) that need it

using import filename from 'file' didn't work, it gave me a compilation error. Specifically, they didn't work in the .js.erb files.I think we at least need the countdownTimer and autoLogout functions in the LoginGov namespace on the window object, if not the hmsFormatter

brendansudol · 2016-11-28T17:46:50Z

app/assets/javascripts/application.js

@@ -1,3 +1,6 @@
+import 'app/autoLogout';
+import 'app/hms-formatter';
+import 'app/countdownTimer';


we should probably be consistent with our js filenames (dashes vs camelcase)

yep, you are right, I payed attention for the formatter but named on autopilot for the other two 🙁

brendansudol · 2016-11-28T17:47:00Z

app/assets/javascripts/application.js

@@ -1,3 +1,6 @@
+import 'app/autoLogout';


we should probably be consistent with our js filenames (dashes vs camelcase)

jessieay

Just a few comments from me! 🎉

jessieay · 2016-11-28T17:38:18Z

app/views/session_timeout/_ping.js.erb

  if (show_warning & !el) {
    cntnr.insertAdjacentHTML('afterbegin', warning_info);
-    initTimer(warning);
+    LoginGov.countdownTimer('#countdown', warning);


so much better! as the last person who touched this js, I really appreciate the refactor

jessieay · 2016-11-28T17:42:50Z

app/views/two_factor_authentication/shared/max_login_attempts_reached.html.erb

+</p>
+<p>
+  <%= t('devise.two_factor_authentication.please_try_again_html',
+  time_remaining: content_tag(:span, lockout_time_in_words, id: 'countdown')) %>


The upside of this (vs having the HTML in the i18n string itself):

feels more railsy

easier to read

Downside:

when just looking at the translation itself (please_try_again_html), it is not obvious that there is HTML used in the translation. Need to look at where it is called.

Because of this downside, I think we've mostly been including the HTML directly in I18n translation. At least that was the case the last time I checked.

What do you think?

I do agree with the downside, I originally had the html directly in the locale file. However, per @zachmargolis comment above, I do think that it is a little cleaner to have the html live in the view.

Do we, strictly speaking, need to know that the translation includes HTML?

We do not, but if I were buzzing around in the locale files and saw an _html translation without HTML, I might be tempted to refactor that and remove the _html, which would break this.

I am fine with keeping as-is and will try to move in this direction when I am using html-ified translations in the future in this proj.

Yeah that part does require one to know that the _html automatically calls html_safe on the output, but rails is already so full of magic what's one more 😬

Yes, that is the magic ✨ -- what I meant to convey is that, as someone who does know this magic, if I saw a translation without HTML but with _html, I might want to refactor to remove _html since it would not be necessary.

ahhh I see! The ✨ was not known to me. 😢

jessieay · 2016-11-28T17:43:36Z

app/views/two_factor_authentication/shared/max_login_attempts_reached.html.erb

@@ -0,0 +1,18 @@
+<% lockout_time_in_words = decorated_user.lockout_time_remaining_in_words %>


I am not a huge fan of declaring vars in views like this (because I would expect this to be a helper method and woud look for it by grepping for def lockout_time_in_words), but this may be the style of the rest of the app

I used a variable here because the view originally had the decorated_user.lockout_time_remaining_in_words sitting directly in the markup taking up a bunch of characters 😄. I saw variables in other views, and since I'm still learning how you all write rails code, I just went with that. I certainly don't mind making a helper method in this case if that's the style we're should to be adhering to!

I don't think a helper method is needed, but I do think that using that veryyyy long decorated_user.lockout_time_remaining_in_words directly in the markup would be fine by me!

brendansudol · 2016-11-28T17:49:39Z

app/assets/javascripts/app/countdownTimer.js

+  if (!countdownTarget) return;
+
+  (function tick() {
+    countdownTarget.innerHTML = window.LoginGov.hmsFormatter(remaining);


can window.LoginGov.hmsFormatter(remaining); now become root.hmsFormatter(remaining)?

brendansudol

left a few questions/comments

**Why**: because comments

brendansudol

thanks, this is looking good. i have one more thought / recommendation around organization. it might be nice / cleaner to have these utility functions separated from the binding of them to the LoginGov object -- what are your thoughts on this? https://gist.github.com/brendansudol/feef1dc02c764bd4a01379fad905b93e (making the functions a bit more self contained and consolidating the root = window.LoginGov bizness to one place)

brendansudol · 2016-11-28T21:22:37Z

ps, i'm also happy to do this reorg in a new PR too if you'd like to get this merged in

el-mapache · 2016-11-28T21:33:43Z

Yeah proposed changed seem fine

el-mapache · 2016-11-28T21:45:07Z

@brendansudol updated!

**Why**: keeps code neater

brendansudol

👍 💥

feedback addressed!

* Properly coountdown time to account unlocking **Why**: The time remaining displayed on the Account Locked screen never changes, leading to a poor user experience **How**: Refactor JS to make the countdown and hours-minutes-seconds formatting reusable * Fixes CodeClimate errors **Why**: CodeClimate doesnt like errors * Removes markup from translation files **Why**: Cleaner separation of concerns * Only show signed in modal after 2fa code entered **Why**: When the account locked countdown reaches zero, the signed in modal flashes briefly, and then reloads the page. This produces an unattractive UI state to the user **How**: uses `user_fully_authenticated` helper method instead of `current_user` * Ensure LoginGov object is properly set on window **Why**: Makes shared javascript functions available under correct namespace * Tweaks to JS files per brendans comments **Why**: because comments * Organizes javascript files **Why**: keeps code neater

monfresh · 2016-12-01T23:21:34Z

app/views/layouts/application.html.slim


  #session-timeout-cntnr
-  - if current_user
+  - if user_fully_authenticated?


This change means that a user who signs in with email and password and sits on the 2FA delivery page will never auto time out. The consequence is that if I leave my computer and phone unattended, someone can sign in to my account without needing to know my email and password. That's probably an edge case, but still something to think about and decide whether or not this is what we want.

The solution to keeping the user on the account locked screen is to reverse the order of these 2 lines: https://github.com/18F/identity-idp/blob/master/app/controllers/concerns/two_factor_authenticatable.rb#L16-L18

The reason is that application.html.slim wasn't called after signing the user out, and therefore the JS was still active. By reversing the order, we force a new call to the layout template, which then sees that a current user does not exist, and therefore does not trigger the timeout JS.

I will submit a PR shortly.

Disregard my first comment about not timing out. I forgot that Devise will time you out regardless of any JS we have. The JS is just for the modal to appear, but I think we do want it to appear during 2FA, right?

Properly coountdown time to account unlocking

3ebab02

**Why**: The time remaining displayed on the Account Locked screen never changes, leading to a poor user experience **How**: Refactor JS to make the countdown and hours-minutes-seconds formatting reusable

el-mapache added status - help wanted status - work in progress labels Nov 25, 2016

zachmargolis previously requested changes Nov 25, 2016

View reviewed changes

el-mapache added 2 commits November 25, 2016 17:31

Fixes CodeClimate errors

5267d09

**Why**: CodeClimate doesnt like errors

Removes markup from translation files

1ae4ece

**Why**: Cleaner separation of concerns

el-mapache force-pushed the ab-account-lock-countdown branch from a663540 to 1ae4ece Compare November 28, 2016 13:31

el-mapache removed the status - help wanted label Nov 28, 2016

el-mapache added status - ready for review and removed status - work in progress labels Nov 28, 2016

el-mapache force-pushed the ab-account-lock-countdown branch from 35157ad to 52f1a2b Compare November 28, 2016 14:37

el-mapache changed the title ~~Properly coountdown time to account unlocking~~ Properly countdown time to account unlocking Nov 28, 2016

el-mapache force-pushed the ab-account-lock-countdown branch from 52f1a2b to 7a5366f Compare November 28, 2016 15:36

Ensure LoginGov object is properly set on window

2c43600

**Why**: Makes shared javascript functions available under correct namespace

el-mapache force-pushed the ab-account-lock-countdown branch from 7a5366f to 2c43600 Compare November 28, 2016 16:56

el-mapache mentioned this pull request Nov 28, 2016

Attempts to resolve CSP violations #762

Merged

brendansudol reviewed Nov 28, 2016

View reviewed changes

jessieay reviewed Nov 28, 2016

View reviewed changes

brendansudol reviewed Nov 28, 2016

View reviewed changes

brendansudol suggested changes Nov 28, 2016

View reviewed changes

Tweaks to JS files per brendans comments

619237f

**Why**: because comments

brendansudol suggested changes Nov 28, 2016

View reviewed changes

el-mapache force-pushed the ab-account-lock-countdown branch from f1c830d to a74e1bb Compare November 28, 2016 21:51

Organizes javascript files

a83a870

**Why**: keeps code neater

el-mapache force-pushed the ab-account-lock-countdown branch from a74e1bb to a83a870 Compare November 28, 2016 22:07

brendansudol approved these changes Nov 29, 2016

View reviewed changes

brendansudol merged commit 7b4175d into master Nov 29, 2016

brendansudol deleted the ab-account-lock-countdown branch November 29, 2016 04:26

monfresh reviewed Dec 1, 2016

View reviewed changes

monfresh mentioned this pull request Dec 2, 2016

Allow user to stay on account locked page #804

Merged

el-mapache self-assigned this Jan 10, 2017

		@@ -0,0 +1,18 @@
		<% lockout_time_in_words = decorated_user.lockout_time_remaining_in_words %>

Conversation

el-mapache commented Nov 25, 2016

Uh oh!

el-mapache commented Nov 25, 2016 • 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!

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 Nov 28, 2016 • 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!

el-mapache Nov 28, 2016 • 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!

jessieay left a comment

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!

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!

brendansudol left a comment

Choose a reason for hiding this comment

Uh oh!

brendansudol left a comment • 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!

brendansudol commented Nov 28, 2016

Uh oh!

el-mapache commented Nov 28, 2016

Uh oh!

el-mapache commented Nov 28, 2016

Uh oh!

brendansudol left a comment

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!

Reviewers

Assignees

Labels

el-mapache commented Nov 25, 2016 •

edited

Loading

el-mapache Nov 28, 2016 •

edited

Loading

el-mapache Nov 28, 2016 •

edited

Loading

brendansudol left a comment •

edited

Loading