1593122 - [autoconfig] Status area goes blank while running guess config

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Ben Bucksch (:BenB)]

Reproduction:

1. Open Thunderbird account creation dialog
2. Enter [e-mail] as email address (must be domain "outlook.co.uk", not a domain that is in the ISPDB)
3. Continue

Actual result:

1. First, you'll see the status messages how the different autoconfig methods are being tried.
2. Then, the status area goes completely blank, for several seconds. (During this time, TB tries its "guess config" heuristic of trying common server names.) As a user, you're confused whether that's a bug, or whether you should start over, or what.
3. Then, the warning "failed to find the settings" appears and you see the manual config mode.

Expected result:
Result step 2 should not exist. Instead:

1. First, you'll see the status messages how the different autoconfig methods are being tried. The "guess config" is just the last line of the autoconfig method statuses.
2. Then, the warning "failed to find the settings" appears and you see the manual config mode.

Comment 1

[Alessandro Castellani [:aleca]]

This happens in 60 and 68 as well, it seems it always behaved like this, which indeed is weird and need to be fixed.
It also shows the "Your Login:" field.
Not a regression, but definitely need to be fixed.

Comment 2

[Ben Bucksch (:BenB)]

What's even more weird is that it works for [e-mail] . It fails reproducibly for [e-mail].

On the Dev Tools console, I see "UI mode start" before the guess config attempts happen. This is the bug, but I don't know why we enter the UI mode "start".

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: 1593122-email-wizard.patch

This is a possible solution to the problem.
If the Exchange username is required, the wizard shows that field and keeps the autoconfig message visible, until we find or don't find a configuration for it.

Is this clear enough?
I'm not sure if this workflow is intuitive and simply showing the "Your Login:" field is understandable enough.
Also, is it worth continuing to search for the configuration if we don't have the proper login info?
Maybe we should stop that and show a more eloquent message to the user when showing the login field.

Comment 4

[Ben Bucksch (:BenB)]

is it worth continuing to search for the configuration if we don't have the proper login info?

Yes, it is. For Office365, if the user has legacy protocols and password-based auth disabled, not even AutoDiscover will work, because (sadly) it needs a password. However, for Office365 domains, we can find the config from ISPDB. Then, after the user confirmed, Owl can then open a login window and we're logged in. So, the ISPDB allows us to jump over this hole, in this particular Office365 case.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9105829 [details] [diff] [review]
1593122-email-wizard.patch

Review of attachment 9105829 [details] [diff] [review]:
-----------------------------------------------------------------

This is an improvement. I agree the "Your login" shouldn't be shows as early as it is. Now it's apparently shown at the time we go off searching for configs. We should only show that if we have reason to think we need it.

Comment 6

[Ben Bucksch (:BenB)]

Comment on attachment 9105829 [details] [diff] [review]
1593122-email-wizard.patch

As mentioned above, I don't think this is the right fix, and might well break stuff that works now. But I have to look into it more. Please don't land this yet.

Comment 7

[Jorg K (CEST = GMT+2)]

Certainly ;-)

Comment 8

[Alessandro Castellani [:aleca]]

I doubt this patch can break something as it doesn't affect anything other than keeping the autoconfig message visible.
The real issue here, as pointed out by Magnus, is the lack of proper feedback to the user when we need/ask for the login on an exchange account.

Anyway, I'll put this on the backburner until there's more clarity on what to do.

Comment 9

[Alessandro Castellani [:aleca]]

The issue reported on this bug is purely from a UX point of view, which is the config message disappearing while there's still a check running in the background.
The submitted patch fixes the issue and doesn't change anything in the currently running code.

I think we should land this as it improves the usability in this scenario, and then open a follow up bug to fix any AutoDiscovery issue there might be.

Comment 10

[Ben Bucksch (:BenB)]

Hi aleca, sorry for the lack of response. I had to put out fires at other places. I hope to get to the review in the next 1-3 days.

Comment 11

[Ben Bucksch (:BenB)]

The problem is that the config process needs the login, and the server doesn't use the email address as username. So, the login field is required even to find the right config.

I tested this and this patch doesn't work as expected for the intended use case. Try [e-mail] . You will see a config from the email provider, but that config is broken and not the right one. And the "Your login:" field appears at the same time, but it's very easy to miss. This config result distracts user focus away from the "Login" input field. But that field needs to be filled out in order to get the correct config from the server. Given that another config result shows, the user will likely miss that input field. That's what the start mode line was for.

Worse, the "Done" button is enabled. Even if the user enters a username, there's no way to restart the config process. That is another reason why we need to go back to the start mode.

Looking more at the code, there's a comment a few lines up saying: Must call error callback in any case to stop the discover mode.. That's the actual bug. This doesn't work as intended. The discover mode is not stopped on all errors (because errors from config methods are completely normal), but only when CancelledException is thrown. Furthermore, the old code took only the exception from the first config method into consideration. I fixed both, and now it works.

Comment 12

[Ben Bucksch (:BenB)]

Attachment: Fix UI state when username field is needed, v2

Comment 13

[Alessandro Castellani [:aleca]]

Comment on attachment 9110393 [details] [diff] [review]
Fix UI state when username field is needed, v2

Review of attachment 9110393 [details] [diff] [review]:
-----------------------------------------------------------------

I think the approach of going back to the start mode to emphasize the new Login field is good, but it doesn't feel complete yet.
We should prompt the user with an explanation why the discovery mode was interrupted and what's that new field we're showing.

Also, if after it fails you click the manual Config button, it shows the message "Looking up configuration..." with the spinner.
That should be cleared out, or updated with a proper string.

Comment 14

[Ben Bucksch (:BenB)]

Hey aleca, thanks for the positive feedback. Would you like to iterate based on my patch and build on it? If so, feel free to do so.

Comment 15

[Ben Bucksch (:BenB)]

Probably a simple way to fix both birds with one line of code is to display a status message with your explanation when we go to show the username field. It would probably also override the "Looking up configuration..." message.

Comment 16

[Alessandro Castellani [:aleca]]

Yes, I can implement the message based on your patch, no problem.
What do you think about this?

"A Username is required to complete the login process."
Very short and easy to understand.

Comment 17

[Ben Bucksch (:BenB)]

"A Username is required to complete the login process."

That's good - simple and precise, yes. I like it! :-) 2 nits corrected:
"A username is required to complete the configuration process."

Comment 18

[Alessandro Castellani [:aleca]]

Attachment: 1593122-autoconfig-username.patch

This should do it.

Comment 19

[Ben Bucksch (:BenB)]

Comment on attachment 9110931 [details] [diff] [review]
1593122-autoconfig-username.patch

Review of attachment 9110931 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with these minor changes

::: mail/components/accountcreation/content/emailWizard.js
@@ +700,5 @@
>            if (
>              e.code == 401 ||
>              (allErrors && allErrors.find(e => e.code == 401))
>            ) {
> +            // Auth failed, ask user for username.

NIT: // Auth failed. -> Ask user for username.
(Situation and reaction are different things.)

@@ +705,3 @@
>              _show("usernameRow");
>              this.switchToMode("start");
> +            this.showErrorStatus("username_required");

Try whether `this.stopSpinner("username_required")` works. This shows the message without being an error.

Move `_show("status-area");` before this line, after switchToMode().

Comment 20

[Ben Bucksch (:BenB)]

Try whether this.stopSpinner("username_required") works.

If for some reason that doesn't work, then showErrorStatus() is fine, too.

Comment 21

[Alessandro Castellani [:aleca]]

Attachment: With message, v3

Thanks for the feedback.
We need to use the showErrorStatus() method otherwise the message area will show a green checkmark, which is misleading.
This message will also appear if the user adds an incorrect username, so it makes sense being an error.

Comment 22

[Ben Bucksch (:BenB)]

Comment on attachment 9110943 [details] [diff] [review]
With message, v3

Thanks, aleca. r=BenB

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 9110943 [details] [diff] [review]
With message, v3

Review of attachment 9110943 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/en-US/chrome/messenger/accountCreation.properties
@@ +103,5 @@
>  
>  confirmAdvancedConfigTitle=Confirm Advanced Configuration
>  confirmAdvancedConfigText=This dialog will be closed and an account with the current settings will be created, even if the configuration is incorrect. Do you want to proceed?
> +
> +username_required=A username is required to complete the configuration process

I don't think this is quite correct. All we know is there was an auth failure, which could mean wrong email, wrong password, missing password, or that a separate username is required, or (after the patch) that the separate username is incorrect, or still missing, or correct but requiring AD domain, or has wrong AD domain. Maybe something else too. 

For the uni-heidelberg.de case, bug 1599029 will have to be addressed before we can test this.

Comment 24

[Ben Bucksch (:BenB)]

Magnus, this fix is correct. Please see bug 1599029 comment 5.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 9110943 [details] [diff] [review]
With message, v3

Review of attachment 9110943 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/en-US/chrome/messenger/accountCreation.properties
@@ +103,5 @@
>  
>  confirmAdvancedConfigTitle=Confirm Advanced Configuration
>  confirmAdvancedConfigText=This dialog will be closed and an account with the current settings will be created, even if the configuration is incorrect. Do you want to proceed?
> +
> +username_required=A username is required to complete the configuration process

If you want that backported to TB 68, you'd better find a solution without string changes.

Comment 26

[Magnus Melin [:mkmelin]]

Indeed, but I have a feeling trying to get by without proper error feedback messages is what got this into the current state to begin with.

Comment 27

[Ben Bucksch (:BenB)]

Not a problem. For TB 68, we can just remove the line this.showErrorStatus("username_required"); and the string. That was just nice sugar added. Just showing the username field on its own is sufficient.

Comment 28

[Ben Bucksch (:BenB)]

There is no error at this point yet. We just detected that we don't have enough information. For most servers, the email address matches the login username, but not so for Exchange servers, and we detect this specific situation and ask the user to enter the username.
In fact, this is similar to the other 3 fields.

Even if the user entered a username and the login fails, it's still most likely that the username form is wrong (because there are so many variants how to write the username, and they are different on each system) and not the password (which is always the same, and he can even display it with the little icon button that aleca helpfully added).

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 9110943 [details] [diff] [review]
With message, v3

Review of attachment 9110943 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/accountcreation/content/emailWizard.js
@@ +705,5 @@
>              // Ask user for username.
>              _show("usernameRow");
>              this.switchToMode("start");
> +            _show("status-area");
> +            this.showErrorStatus("username_required");

In general the patch looks like it would do almost the right thing. I think bug 1598861, and bug 1599029 should land first so that the whole process makes some sense (and doesn't bitrot each other).

What IS wrong, is the error message. I suggest we make it say:

The automatic configuration process could not be completed since there was a problem with the credentials you provided. Please check they are correct. If they are correct, please enter your username, since in that case it's likely that you need to use a separate username for login. This username is usually your AD login name with or without the domain. 

We also need a separate message for the case where you already DID enter the login name but autodicovery still failed with 401. I suggest:

The automatic configuration process could not be completed. There is still problem with the your entered credentials. Please check the username and password.

For 68? Either skip the message or just have it hard coded to English...

Comment 30

[Ben Bucksch (:BenB)]

In general the patch looks like it would do almost the right thing.

Thanks.

I think bug 1598861, and bug 1599029 should land first so that the whole process makes some sense (and doesn't bitrot each other).

In bug 1599029, it's not even clear what happens, and the issue there is clouded by the bug here, so this here needs to land first.

This here is a clear bug, and has a fix since quite a while, and the fix is correct.

What IS wrong, is the error message.

The error message is correct, for the given situation. We know that this is an Exchange server, and their standards configuration uses usernames different from the email address, so the username is the problem in almost all cases. We know the user just needs to enter the username.

You might argue that if the user did enter a username and we still get an error, then we can infer that the password might be wrong. But in reality, there are so many different forms of usernames, and there is only one password, that even if the user entered a username, and login fails, the most likely reasons is still the username and not the password. Please see bug 1599029 comment 33 for experiences from the field, where admin cites evidence and actual logs that people needs several attempts to get the username right, even with hints what the right form is.

So, I maintain that the error message is exactly right. It was created by aleca with input from me.

For 68? Either skip the message

Agreed.

Comment 31

[Ben Bucksch (:BenB)]

I like the error message, I think it's helpful exactly as it is, but if that helps the landing, I suggest remove the message for now. It wasn't there before and is not required to fix the bug. It's just additional niceness.

Comment 32

[Magnus Melin [:mkmelin]]

(In reply to Ben Bucksch (:BenB) from comment #30)

The error message is correct, for the given situation. We know that this is an Exchange server, and their standards configuration uses usernames different from the email address, so the username is the problem in almost all cases. We know the user just needs to enter the username.

Please look at what I wrote. The username might be there already.
Anyway, just re-read what you wrote and apply it to context of O365 (and the other free MS account). There is no username needed. Just the email. 401 == wrong credentials, or other trouble.

Comment 33

[Ben Bucksch (:BenB)]

Please look at what I wrote. The username might be there already.

I saw what you wrote. See bug 1599029 comment 37 (where you wrote this).

just re-read what you wrote and apply it to context of O365 (and the other free MS account). There is no username needed. Just the email. 401 == wrong credentials, or other trouble.

Yes, I can see your point for this case. Let me think about that.

Comment 34

[Ben Bucksch (:BenB)]

just re-read what you wrote and apply it to context of O365 (and the other free MS account). There is no username needed. Just the email. 401 == wrong credentials, or other trouble.

Yes, I can see your point for this case. Let me think about that.

I've just tried this, and it worked just fine for me. With this patch here, and an Office365 account, the ISPDB result was used, and no username field appeared, no matter whether the correct password or a wrong password is used.

Comment 35

[Ben Bucksch (:BenB)]

Attachment: Remove error message, v3b

This is the same as the last patch, but without status message. We simply show the username field.

Comment 36

[Ben Bucksch (:BenB)]

Attachment: Screenshot of patch v3b

Comment 37

[Magnus Melin [:mkmelin]]

Comment on attachment 9113012 [details] [diff] [review]
Remove error message, v3b

Review of attachment 9113012 [details] [diff] [review]:
-----------------------------------------------------------------

Well, it fixes the "blank" problem, but it's pretty bad UX not to get any feedback on what happened. I guess we can add that in another patch so we can get this part in for 68

::: mail/components/accountcreation/content/emailWizard.js
@@ +626,4 @@
>          self.stopSpinner(call.foundMsg);
>          self.foundConfig(config);
>        },
> +      function(e, allErrors) {

we should really change this to be function(errors)

... since e is always the first of the errors, and allErrors is always there
https://searchfox.org/comm-central/rev/2ef473ed7f9517c953008bae3de732820ea9c5e8/mail/components/accountcreation/content/util.js#490

@@ +696,2 @@
>          call.successCallback(),
>          (e, allErrors) => {

same here

@@ +700,3 @@
>            if (
>              e.code == 401 ||
>              (allErrors && allErrors.find(e => e.code == 401))

these two lines should just be

errors.some(e => e.code == 401)

Comment 38

[Ben Bucksch (:BenB)]

we should really change this to be function(errors)

The signature of all errorCallbacks is that the first parameter is the exception. That's the rule for all functions here. You cannot just suddenly change the type. That would confuse everybody and surely lead to bugs.

But I can remove the allErrors check, yes. That makes the code nicer.

Comment 39

[Ben Bucksch (:BenB)]

Attachment: Without error message, v5

Comment 40

[Ben Bucksch (:BenB)]

Attachment: Without error message, v6

During self-review, I saw that I could use some() instead of find(). Fixed.

Comment 41

[Ben Bucksch (:BenB)]

Attachment: Without error message, v7

Use existing onStartOver() function. Makes it cleaner.

Compare also bug 1600954, which goes along with this patch.

Comment 42

[Alessandro Castellani [:aleca]]

Comment on attachment 9113013 [details]
Screenshot of patch v3b

This is exactly what we have right now, isn't it?
Except we're enabling the manual config button.

It's super weird but I guess if we can't push a new string into 68, this is good enough.

Comment 43

[Alessandro Castellani [:aleca]]

Comment on attachment 9113238 [details] [diff] [review]
Without error message, v7

It's better if you ping Magnus for this reviews.
I'm giving a ui-review+ since that's all we can do for 68 without introducing a new string.

Comment 44

[Magnus Melin [:mkmelin]]

Comment on attachment 9113238 [details] [diff] [review]
Without error message, v7

Review of attachment 9113238 [details] [diff] [review]:
-----------------------------------------------------------------

Ben, please fix you .hgrc file to have a proper user.

Comment 45

[Magnus Melin [:mkmelin]]

Going to land it, but let's keep open to add a proper message too while we wouldn't backport that.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2b0257c0f794
"[autoconfig] Status area goes blank while running guess config" [r=mkmelin, ui-r=alessandro]

Comment 47

[Jorg K (CEST = GMT+2)]

Any chance to land patches with correct commit messages in the future? :-( :-(

Comment 48

[Ben Bucksch (:BenB)]

@Magnus: Thanks for having landed this!

Comment 49

[Jorg K (CEST = GMT+2)]

Please submit a second part to re-introduce the message. Alex called the current behaviour "super weird" in comment #42.

Comment 50

[Jorg K (CEST = GMT+2)]

Attachment: 1593122-login-box-message.patch

Here's the missing error message. I tried setting up [e-mail] with incorrect credentials and that gives an appropriate message. So I think this text is correct. I'm open to a better wording.

I don't understand why I have to do this during my leave, though :-( :-( :-(

Comment 51

[Alessandro Castellani [:aleca]]

Comment on attachment 9113908 [details] [diff] [review]
1593122-login-box-message.patch

Review of attachment 9113908 [details] [diff] [review]:
-----------------------------------------------------------------

I was going to do this this afternoon, but you're faster than me even on vacation.
Anyway, ui-r+ from me as showing the status error is what I wanted since the beginning.

I'll leave the wording decision to you and Magnus.

Comment 52

[Magnus Melin [:mkmelin]]

Like I said earlier, there are two cases so I don't see why we wouldn't display the different cases differently. I'll attach a patch and screenshots

Comment 53

[Magnus Melin [:mkmelin]]

Attachment: 1593122-login-box-message.patch (with two cases separate)

Comment 54

[Magnus Melin [:mkmelin]]

Attachment: creds-incomplete.png

Comment 55

[Magnus Melin [:mkmelin]]

Attachment: creds-stillwrong.png

Comment 56

[Jorg K (CEST = GMT+2)]

Comment on attachment 9113908 [details] [diff] [review]
1593122-login-box-message.patch

Sure, Magnus approach is better.

Comment 57

[Jorg K (CEST = GMT+2)]

Comment on attachment 9113922 [details] [diff] [review]
1593122-login-box-message.patch  (with two cases separate)

Review of attachment 9113922 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/en-US/chrome/messenger/accountCreation.properties
@@ +103,5 @@
>  
>  confirmAdvancedConfigTitle=Confirm Advanced Configuration
>  confirmAdvancedConfigText=This dialog will be closed and an account with the current settings will be created, even if the configuration is incorrect. Do you want to proceed?
> +
> +credentials_incomplete=The automatic configuration process could not be completed since there was a problem with the email or password you provided. If they are correct, please enter your username, since in that case it's likely that you need to use a separate username for login. This username is usually your AD login name with or without the domain.

No abbreviations, please. s/it's/it is/. What's AD? Active directory? Wouldn't it just be "domain login name"? This is very wordy, any chance to make this shorter? How about:
The automatic configuration process could not be completed. Please check your password, and if correct, additionally provide your domain login.

Comment 58

[Magnus Melin [:mkmelin]]

It's slightly wordy, but we do have plenty of space. For AD, yes that is active directory. In reality, I do think that is what almost everyone is using as the "domain". That is, you will have the username be "AD\example" - so that's why I wouldn't write out fully what it is. I should add a l10n note about it though.

Comment 59

[Ben Bucksch (:BenB)]

Remember that we target end users who are not in the computer business. They don't know what "Active Directory" or "AD" is.

However, they do know what their "Windows login" or "Windows domain login" is. An example is also critically important, to show the format of the username, and the example should be as concrete as possible, e.g. "COMPANY\you".

I concur with Alex' comment 16 that the message should be as short as possible. More explanations are worse. Just say what we expect from the user, and give a very concrete example of the username form.

I suggest:
"A username is required to complete the configuration process. This is typically your Windows domain login, e.g. "COMPANY\you". Please also verify your password by clicking on the eye icon."

This is concrete, with instructions. It's still longer than I would like.

Contents:

• The standard configuration of an on-premise Exchange server is to use the Windows domain login as username, and the email address does not work. So, the primary message here should be to enter the username, and not that the login failed.
• I am adding a concrete example of the username, because the concrete form and spelling of the username - e.g. "" vs. "/" vs "." is what causes most of the login failures.
• For the rare case that the password is wrong, I added the phrase.
• I don't think we need to mention to check the email address, because that's going to be obvious, as it's displayed and right next to the password. If the user checks the password, they will also see when the email address right above it is wrong.

Comment 60

[Jorg K (CEST = GMT+2)]

Ben: Magus suggested to have two different strings. What you think about that?

First:
A username is required to complete the configuration process. This is typically your Windows domain login, e.g. "COMPANY\you". Please also verify your password by clicking on the eye icon.

Second:
Please verify your username and password by clicking on the eye icon.

Comment 61

[Magnus Melin [:mkmelin]]

Attachment: 1593122-login-box-message.patch

Ignoring the password wrong case doesn't make any sense since that's easily even half the times especially if you have a more complex password.
Anyway, I've made some adjustments to make the message more clear. All the domain names I ever saw were actually named AD so I think it's reasonable to assume it's a very common domain name. (Probably the default in some setup?) If something else is actually used, the placeholder text would still guide you right.

Comment 62

[Ben Bucksch (:BenB)]

Remember that the standard config of on-premise Exchange servers (which is what we're talking about here) is to use a username completely different from the email address. That's not a user error, but all users will get this prompt.

For on-premise Exchange, it's typically the Windows domain login, e.g. EXAMPLE\mjola .

In my first hand experience, and that's substantiated with logs from admin (see bug 1599029 comment 33), users have a really hard time to find the right form of username, because there are so many different forms of username for the same user, depending on the system used. It could be:

• mjola
• michael.jola
• [e-mail] or
• EXAMPLE\mjola

Good luck finding the right one. You can't really know for sure.

OTOH, there is typically only a single password in all cases (and the user can verify it easily with the eye button).

In reality, many companies have various different forms of usernames for various systems, and most users have no clue what the right username form is in this particular case. If the username was obvious, then I would agree with you. But the reality is different.

So, no, I do not think that "password wrong" is a typical case. Even if the user entered a username, and we fail the second time, it's still most likely the username that's wrong and not the password.

If indeed the username was wrong, and the user thinks that he needs to use a different password, and enters a different, valid password, we not only created a headache and setup pain for the end user, we also made him give up a potentially valuable password to the wrong server. That's a security problem, in addition to user setup pain.

More scientifically, you need to fix one variable at a time. If you ask the user to perpetuate two variables at the same time, you multiply the attempts required.

I don't mind having 2 prompts, but even the second prompt needs to primarily encourage users to find the right username and not the password. The latter would be actively damaging.

Comment 63

[Alessandro Castellani [:aleca]]

Comment on attachment 9114516 [details] [diff] [review]
1593122-login-box-message.patch

Review of attachment 9114516 [details] [diff] [review]:
-----------------------------------------------------------------

Very wordy, but I guess is necessary.
This looks good to me.

We could potentially have the last sentence of the first error message as a tooltip for the (?) of the login field.
"Windows domain login with or without the domain (e.g. johndoe or AD\\johndoe)".

Comment 64

[Ben Bucksch (:BenB)]

The tooltip wouldn't be prominent enough. It's exactly that part that is the most important and needs to be in the user's face.
It should also be included in the second error message, otherwise it would be gone after the first attempt.

FWIW, the username example should appear as "ACME\johndoe" (single backslash). "ACME" would be nice reference to Coyote and Road Runner, and doesn't need to be translated, but it's less cryptic than "AD" as example.

Comment 65

[Ben Bucksch (:BenB)]

Attachment: Same as Magnus' patch, with different error messages

I think the last patch of Magnus is good, but the error messages are not really good yet.
Rationale: See comment 62.

So, as first error message, I used the one from Aleca in patch 3, plus a concrete example to help the user, with the most likely form of the username.
As second error message, I also ask the user to double-check the password. And expand more on the different forms of username. I kept the concrete examples in there, because that's exactly the info that the user needs to find the right format of username. (I don't ask to check the email address, because that's too obvious, and if it's really wrong, the user will see it once he checks the password.)

I think that captures all concerns, including:

• Magnus: ask user to check password, once we have a username
• Ben: there are many different username forms and it often needs several attempts to find the right one, see bug 1599029 comment 33
• Aleca: short and concise message, to the point

Comment 66

[Ben Bucksch (:BenB)]

Attachment: Same as Magnus' patch, with different error messages, v11

Comment 67

[Magnus Melin [:mkmelin]]

Comment on attachment 9114796 [details] [diff] [review]
Same as Magnus' patch, with different error messages, v11

Review of attachment 9114796 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry but this is wrong as previously described.

Comment 68

[Ben Bucksch (:BenB)]

You wrote:

this is wrong as previously described

Are you referring to your statement "password wrong case ... that's easily even half the times"? I explained in comment 62 why that is not the true here in this case. I also gave real world data, specifically for this case here, to back that up.

Sorry, I don't understand why my patch is "wrong", given that I captured your concern to ask the user to check the password.
Your text does not consider the reality of Exchange servers.

I really felt like the patch captured everybody's concerns.

Comment 69

[Jorg K (CEST = GMT+2)]

How long do you want to go around in circles here leaving the product broken in the meantime? In bug 683809 for example we're up to comment #129 and the thing has been blocked for more then three years now.

Personally, I think that both suggestions presented here s*ck. Magnus message is way too long and AD active directory is a M$ technology that has little to do with the concept of a domain in the M$ sense, at least that was the case when I was on M$ decades ago. I don't know what's wrong with <!ENTITY usernameEx.placeholder "YOURDOMAIN\yourusername"> or just using domain.

How about stopping r- each others patches superseding them when all we need is to agree on a text in a COMMENT before turning this into a patch. It's really sad that I can't enjoy my hard earned PTO and need to intervene on Kindergarten stuff like this :-( :-( :-(

Comment 70

[Jorg K (CEST = GMT+2)]

Comment on attachment 9114516 [details] [diff] [review]
1593122-login-box-message.patch

Too long, AD is confusing, and johndoe is equally confusing for non-IT knowledgable people who haven't met John Doe. 

Look at the current placeholders we have:
https://searchfox.org/comm-central/source/mail/locales/en-US/chrome/messenger/accountCreation.dtd
There is no johndoe.

Comment 71

[Jorg K (CEST = GMT+2)]

So let's compare the two suggestions without having to open up patches.

Magnus:
credentials_incomplete=Due to authentication failure the automatic configuration process could not be completed. If the email and password you entered are correct, it is likely that you need to use a separate username for logging in. This username is usually your Windows domain login with or without the domain (e.g. johndoe or AD\johndoe).
credentials_wrong=Authentication failed. There is still a problem with the entered credentials. Please check the username and password.

Ben:
credentials_incomplete=A username is required. This is usually your Windows domain login (e.g. ACME\johndoe).
credentials_wrong=Please check the password using the eye button, and ask which format of username you need for your Exchange email account. This is usually your Windows domain login with or without the domain (e.g. johndoe or ACME\johndoe).

What needs to be done:
First message: Ask for domain user name and make sure password is correct.
Second message: Make sure both are correct.

Jörg:
Please provide a username, usually your Windows domain login in the format COMPANY\yourname. This may not be required if your password was entered incorrectly which you can verify by clicking on the eye icon.

Second:
Please verify your username and password by clicking on the eye icon.

Comment 72

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+1) (PTO to 15th Dec 2019, sporadically reading bugmail) from comment #69)

AD active directory is a M$ technology that has little to do with the concept of a domain in the M$ sense

Eh, AD is exactly what the domain is in the MS sense, in this context. It's NOT the domain as in the internet domain name, it's the naming of the Active Directory domain, which can be anything.

I'm not going to spend more time on this bug now. The patch already has r, and ui-r so I'm just going to land it so we can move on with more important business.

For the wording you suggested, asking the user to click somewhere is... odd UI to say the least. The eye icon may change over time or even by changing theme used.

Comment 73

[Jorg K (CEST = GMT+2)]

Maybe you want to read up on that "Active Directory" really is: https://en.wikipedia.org/wiki/Active_Directory.

In the olden days a MS domain controller existed before the advent of Active Directory, but it appears that the latter has now subsumed the former.

If you want to force your patch into the system, at least consider changing the (e.g. johndoe or AD\johndoe) stuff to this:
This username is usually your Windows domain login with or without the domain (for example: yourname or DOMAIN\yourname).
Please leave johndoe out of user-facing strings:
https://searchfox.org/comm-central/search?q=johndoe&case=false&regexp=false&path=.dtd

BTW, formally your patch doesn't have an r+ but an r- from a Thunderbird peer so I can't see any legitimacy in what you announced to do.

Comment 74

[Magnus Melin [:mkmelin]]

"yourname" is not en example since nobody would ever have that as a username. John Doe on the other hand is a common English idiom.

Comment 75

[Jorg K (CEST = GMT+2)]

Comment on attachment 9114516 [details] [diff] [review]
1593122-login-box-message.patch

Just make sure no one complains for not using Jane Doe :-( - https://en.wikipedia.org/wiki/John_Doe

I'm sure that people using English as a second language may not understand this.

The patch **MUST NOT** land (or I will back it out personally) since it has only a negative review. It is also clearly **MISSING** the localiser notes.

You will need to explain that johndoe will need to be translated into something like maxmustermann in German for example (https://de.wikipedia.org/wiki/Mustermann), and that AD stands for Active Directory, which also needs to be translated. I'm still convinced that is a really bad choice, you should minimise abbreviations in user facing strings, and that simply DOMAIN would be the better choice.

Comment 76

[Magnus Melin [:mkmelin]]

You'd have no reason to back it out. The patch is submitted by the module owner, It has ui-review from the UX lead, and the previous iteration (adding an if-else) was previous reviewed to the extent such a two line change needs extensive review.

But sure, I can add a localization note, and change it to jane.
AD is an example and should not really be translated. Even non-English users are likely to use that.

Comment 77

[Jorg K (CEST = GMT+2)]

Well, your English derailed here:

... , and the previous iteration (adding an if-else) was previous reviewed to the extent such a two line change needs extensive review.

Please point me to the attachment which according to you had positive review. The only one with r+ is the one without the strings. Are you referring to attachment 9110943 [details] [diff] [review] which has r- from you?

Comment 78

[Magnus Melin [:mkmelin]]

Attachment: 1593122-login-box-message.patch

Alright, feel free to review the if-else then. But you already agreed in comment 56.

Comment 79

[Jorg K (CEST = GMT+2)]

Comment on attachment 9114837 [details] [diff] [review]
1593122-login-box-message.patch

Review of attachment 9114837 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/en-US/chrome/messenger/accountCreation.properties
@@ +103,5 @@
>  
>  confirmAdvancedConfigTitle=Confirm Advanced Configuration
>  confirmAdvancedConfigText=This dialog will be closed and an account with the current settings will be created, even if the configuration is incorrect. Do you want to proceed?
> +
> +# LOCALIZATION NOTE(credentials_incomplete): The reference to "janedoe" (Jane Doe) is the name of an example person. You wiil want to translate it to whatever example persons would be named in your language. In the example, AD is the name of the Window domain, and this should usually not be translated.

Clearly this needs review since the module owner doesn't appear to be able to do his own proof reading.
Mistyped are: wiil and Window.

Comment 80

[Magnus Melin [:mkmelin]]

Attachment: 1593122-login-box-message.patch

Fixed. r for the if-else please, per popular demand

Comment 81

[Alessandro Castellani [:aleca]]

Comment on attachment 9114864 [details] [diff] [review]
1593122-login-box-message.patch

Review of attachment 9114864 [details] [diff] [review]:
-----------------------------------------------------------------

Apologies for proposing some copy changes, but I think we should reduce the verbosity of these messages as much as possible, without altering the context.

Since English is not my first language, feel free to disregard these suggestions.
This is a r+ for me, with or without my proposed copy edits, since it solve the issue of not telling the user what the problem is, and it shows a proper feedback, even if a bit long.

::: mail/locales/en-US/chrome/messenger/accountCreation.properties
@@ +104,5 @@
>  confirmAdvancedConfigTitle=Confirm Advanced Configuration
>  confirmAdvancedConfigText=This dialog will be closed and an account with the current settings will be created, even if the configuration is incorrect. Do you want to proceed?
> +
> +# LOCALIZATION NOTE(credentials_incomplete): The reference to "janedoe" (Jane Doe) is the name of an example person. You will want to translate it to whatever example persons would be named in your language. In the example, AD is the name of the Windows domain, and this should usually not be translated.
> +credentials_incomplete=Due to authentication failure the automatic configuration process could not be completed. If the email and password you entered are correct, it is likely that you need to use a separate username for logging in. This username is usually your Windows domain login with or without the domain (e.g. janedoe or AD\\janedoe).

I'm sorry to propose another change, but I think we can shrink this down and keeping the same core message.
"Authentication failed. The entered credentials might be incorrect or a separate username might be required for logging in. This username is usually your Windows domain login with or without the domain (e.g. janedoe or AD\\janedoe)."

@@ +105,5 @@
>  confirmAdvancedConfigText=This dialog will be closed and an account with the current settings will be created, even if the configuration is incorrect. Do you want to proceed?
> +
> +# LOCALIZATION NOTE(credentials_incomplete): The reference to "janedoe" (Jane Doe) is the name of an example person. You will want to translate it to whatever example persons would be named in your language. In the example, AD is the name of the Windows domain, and this should usually not be translated.
> +credentials_incomplete=Due to authentication failure the automatic configuration process could not be completed. If the email and password you entered are correct, it is likely that you need to use a separate username for logging in. This username is usually your Windows domain login with or without the domain (e.g. janedoe or AD\\janedoe).
> +credentials_wrong=Authentication failed. There is still a problem with the entered credentials. Please check the username and password.

This is a bit of a repetition, since the authentication failed even after typing the Login username, we can safely assume any of the typed credentials might be incorrect.
What about:
"Authentication failed. The entered credentials might be incorrect."

Comment 82

[Jorg K (CEST = GMT+2)]

While we're here, can we please lose the abbreviation "e.g." and use "for example":
https://searchfox.org/comm-central/search?q=for+example&case=false&regexp=false&path=AccountWizard.dtd

Comment 83

[Magnus Melin [:mkmelin]]

(In reply to Alessandro Castellani (:aleca) from comment #81)

I'm sorry to propose another change, but I think we can shrink this down and
keeping the same core message.
"Authentication failed. The entered credentials might be incorrect or a
separate username might be required for logging in. This username is usually
your Windows domain login with or without the domain (e.g. janedoe or
AD\janedoe)."

Sound ok, but I'll change it slightly to have the sentence say either or instead.
Will also update to "for instance"

This is a bit of a repetition, since the authentication failed even after
typing the Login username, we can safely assume any of the typed credentials
might be incorrect.
What about:
"Authentication failed. The entered credentials might be incorrect."

Here it makes sense to specifically call out username and password since once we have the username, what the email is doesn't matter (it's not used then).

Comment 84

[Magnus Melin [:mkmelin]]

Attachment: 1593122-login-box-message.patch

Comment 85

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/df3dbb04dd384e4c8fd7a7e01f28bde758646bb2
Follow-up - show message guiding users what to do when exchange autodiscover authentication failed. r=aleca, ui-r=aleca
DONTBUILD

Pulsebot on strike today? Anyway, thanks for the string changes.

Comment 86

[Jorg K (CEST = GMT+2)]

Comment on attachment 9113238 [details] [diff] [review]
Without error message, v7

You should take the first part without the string changes to beta.

Comment 87

[Geoff Lankow (:darktrojan)]

72 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/65cb9a77ffecf5e7423c40f6e8ebb69b035c9849

Comment 88

[Jorg K (CEST = GMT+2)]

Comment on attachment 9113238 [details] [diff] [review]
Without error message, v7

Needed for TB 68.4 as a dependent of bug 1600041.

Comment 89

[Geoff Lankow (:darktrojan)]

68.4:
https://hg.mozilla.org/releases/comm-esr68/rev/02d9914009c2fbc2d9add584385d08df5127e1fc

Comment 90

[Magnus Melin [:mkmelin]]

Attachment: bug1593122_68_status_area.patch (64 uplift fix)

Unfortunately I notice https://hg.mozilla.org/releases/comm-esr68/rev/02d9914009c2fbc2d9add584385d08df5127e1fc would have needed an adjustment since the id on 68 is status_area instead of status-area

I'm rebuilding locally with this to verify it works.

Comment 91

[Magnus Melin [:mkmelin]]

Comment on attachment 9122859 [details] [diff] [review]
bug1593122_68_status_area.patch (64 uplift fix)

Works

Comment 93

[Magnus Melin [:mkmelin]]

Rob, please land attachment 9122859 [details] [diff] [review] on comm-esr68. It's not landed elsewhere because it's a follow-up to the esr68 fix already landed earlier.

Comment 94

[Jorg K (CEST = GMT+2)]

And when you do, put 68.5 in the comment and set the status-thunderbird_esr68 accordingly. That was also not done correctly in bug 1609607 and bug 1608539 and maybe elsewhere. Without that information, it will be hard to tell in which dot release things got fixed.

By the looks of it, setting status-thunderbird_esr68 was also not set on any of the bugs here and earlier :-(
https://treeherder.mozilla.org/#/jobs?repo=comm-esr68&revision=72e751b740f18dcceb4f3ed53849129929be24c7
Please talk to whoever handles releases these days.

Comment 95

[Rob Lemley [:rjl]]

Thunderbird 68.5.0:
https://hg.mozilla.org/releases/comm-esr68/rev/7cdcdb0a1ea2