Skip to content
This repository has been archived by the owner. It is now read-only.

fix(client): Fix the confirm_reset_password screen polling logic & tests. #3083

Merged
merged 1 commit into from Sep 24, 2015

Conversation

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Sep 21, 2015

There were several problems:

  1. Because promises are async, setTimeout could be called after the module was
    torn down, which caused completion logic to be run after it was expected.
  2. The timer-mixin called clearAllTimeouts without a context when used as
    a destroy handler, which meant outstanding timeouts were not cleared on
    view destroy.
  3. The unit tests had a lot of cross test pollution. New dependencies are set
    up for each test now.
  4. The logic did not take into account that the login message could arrive
    from the other tab before, during, or after the XHR request that checks
    whether the user has verified. There were no tests for all of these cases
    either.

This fix takes a new approach to polling:

When the user attempts a password reset, we have no idea whether the
user is going to verify in the same browser. The only way we know if
the user verified in this browser is if a login message is received
from the inter-tab channel.

Because we have no idea if the user will verify in this browser,
assume they will not. Start polling the server to see if the user has
verified. If the server eventually reports the user has successfully
reset their password, assume the user has completed in a different
browser. In this case the best we can do is ask the user to sign in
again. Once the user has entered their updated creds, we can then
notify the browser.

If a complete_reset_password_tab_open message is received, hooray,
the user has opened the password reset link in this browser. At this
point we can assume the user will complete verification in this
browser. It's not 100% certain the user will complete, but most
likely. Stop polling the server. The server poll is no longer needed,
and in fact makes things more complex. Instead, wait for the login
message that will arrive once the user finishes the password reset.

Once the login message has arrived, notify the browser. BOOM.

fixes #2483

@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Sep 21, 2015

Replaces #3081.

@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Sep 21, 2015

@vladikoff - r?

This scheme does away with the problematic timeout completely.

@@ -252,7 +252,7 @@ define([
.end();
},

'password reset, verify same browser': function () {

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Sep 21, 2015
Author Member

I changed all of the password reset=>reset password because we had a mixture of the two and it made running the tests with grep a pain.

@shane-tomlinson shane-tomlinson force-pushed the issue-2483-fix-reset-password branch from c3f7b5e to 42a7d0d Sep 21, 2015
…sts.

There were several problems:

1. Because promises are async, setTimeout could be called after the module was
   torn down, which caused completion logic to be run after it was expected.
1. The timer-mixin called clearAllTimeouts without a context when used as
   a `destroy` handler, which meant outstanding timeouts were not cleared on
   view destroy.
1. The unit tests had a lot of cross test pollution. New dependencies are set
   up for each test now.
1. The logic did not take into account that the `login` message could arrive
   from the other tab before, during, or after the XHR request that checks
   whether the user has verified. There were no tests for all of these cases
   either.

This fix takes a new approach to polling:

When the user attempts a password reset, we have no idea whether the
user is going to verify in the same browser. The only way we know if
the user verified in this browser is if a `login` message is received
from the inter-tab channel.

Because we have no idea if the user will verify in this browser,
assume they will not. Start polling the server to see if the user has
verified. If the server eventually reports the user has successfully
reset their password, assume the user has completed in a different
browser. In this case the best we can do is ask the user to sign in
again. Once the user has entered their updated creds, we can then
notify the browser.

If a `complete_reset_password_tab_open` message is received, hooray,
the user has opened the password reset link in this browser. At this
point we can assume the user will complete verification in this
browser. It's not 100% certain the user will complete, but most
likely. Stop polling the server. The server poll is no longer needed,
and in fact makes things more complex. Instead, wait for the `login`
message that will arrive once the user finishes the password reset.

Once the `login` message has arrived, notify the browser. BOOM.

fixes #2483
@shane-tomlinson shane-tomlinson force-pushed the issue-2483-fix-reset-password branch from 42a7d0d to 6cd7dc1 Sep 21, 2015
* @param {string} name
* @param {string} key
*/
off: function (name, key) {

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Sep 21, 2015
Author Member

This was changed from callback=>key since I realized that is what crosstab wants. I wonder if we should do the conversion to take a callback here instead of in the inter-tab-channel-mixin so the inter-tab-channel acts like all the other pub-sub mechanisms we use.

@rfk rfk added this to the FxA-0: quality milestone Sep 21, 2015
vladikoff added a commit that referenced this pull request Sep 24, 2015
fix(client): Fix the confirm_reset_password screen polling logic & tests. r=vladikoff
@vladikoff vladikoff merged commit 0c435dd into master Sep 24, 2015
4 checks passed
4 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 98.942%
Details
@vladikoff
Copy link
Contributor

@vladikoff vladikoff commented Sep 24, 2015

Let's see how this performs on TeamCity! 🎌

@vladikoff vladikoff deleted the issue-2483-fix-reset-password branch Sep 24, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants