front-end tests - new messages output when running

[jrgm]

Haven't looked into this much, but there are two messages that are new after the big landing of GH-2608 and friends.

1. actions called={"doAuthenticate":true}
2. request for 'post /wsapi/used_address_as_primary valid' is not mocked in resources/static/test/cases/xhr.js

[shane-tomlinson]

This second one actually points out a problem where even if used_address_as_primary returns an error, no user facing action is taken and the dialog is started as normal. See https://github.com/mozilla/browserid/blob/2ec862608909115586f5df967fe266cdcae701a9/resources/static/dialog/js/modules/dialog.js#L298

[lloyd]

@shane-tomlinson this is by design. We didn't want to block the user for a non-fatal error.

[shane-tomlinson]

@seanmonstar, @lloyd - is this the desired behavior? if used_address_as_primary fails, the dialog will continue on its merry way.

It looks like this path can be taken if either: 1) the user is not authenticated or 2) the xhr request fails.

It seems like these should be handled by two distinct paths, the first can occur if the user cancels out of the authentication process with the primary. In this case, the user should be allowed to enter in a new address.

Case 2 seems like we should show an error message.

[lloyd]

If the XHR request fails, it means we were unable to tell the server the information it needs to turn off the transition. So the user will see the screen again the next time.

If we display an error, the user will see an error, won't get to log in, and will see the transition screen again next time they try.

I think continuing regardless of whether the call succeeds is right

-- lloyd (thumb-typing)

On Dec 11, 2012, at 7:06 AM, Shane Tomlinson notifications@github.com wrote:

Case 2 seems like we should show an error message.

[shane-tomlinson]

I understand the end goal, but I am going to examine this closer - silent failures make me extremely uncomfortable.

[seanmonstar]

This is the desired effect. However, if you look at network.usedAddressAsPrimary, you'll see that it shouldn't ever make the request unless the dialog thinks its authenticated.

Reasoning was:

• If they are authenticated, then we won't call auth_with_assertion, or any other wsapi call, so we have no way of knowing if the primary was successful. So, we need to call this wsapi.
• If they aren't authenticated, this call shouldn't happen, because auth_with_assertion is needed, and that can mark the status for us.

In the case that the request fails, we definitely do not want to show an error message. This would prevent them from logging in, when error could be as benign as "Server Too Busy".

[shane-tomlinson]

@seanmonstar, @lloyd - Thanks for the explanations, I understand better now.

The way this line is currently written, a failure in /wsapi/session_context when calling user.usedAddressAsPrimary will still allow the dialog to start. This might not be a big deal because session_context will be called again by the next call that requires a CSRF token.

Should failures other than "server too busy" in used_address_as_primary stop the dialog? Another failure means something most definitely is wrong and we are potentially leaving the front end in an unknown state.

If this a silent failure is truly desired, let's make a list somewhere of calls that are allowed to fail. Otherwise, this info will be lost on future maintainers and you are going to have people like me pestering you that a call is not hooked up to an error handler.

[seanmonstar]

I can't think of a single response from used_address_as_primary that I would want the dialog to fall over. Especially if all other requests are working fine, I'd rather the user be able to finish logging in.

Would a comment at that line explaining "we want to continue even onerror" satisfy the concern?

[shane-tomlinson]

@seanmonstar - I am most interested in how this relates to #2840. As part of our cleanup soon, let's think of how to handle errors, including ones that should force error screens, and a generic way of logging errors.

w.r.t. the comment - #2853 - wanna review?

[shane-tomlinson]

@seanmonstar - I am not trying to be pain, but I am standing by my claim that an error message would have caught the problem in #2840. If "server too busy" should not stop the dialog, I am fine with that but let's whitelist that error. "User does not own the email address" is something that should show an error so the dialog fails fast and early and indicates to us, the developers, that something is not operating as expected.

[seanmonstar]

I see the concern. While it would be more noticeable to us, even in this situation, since the 400 didn't cause harm, I would hate for that failed request to have stopped someone's login attempt.