Mail account wizard (autoconfig) cannot be completed

RESOLVED FIXED in Thunderbird 38.0

Status

Thunderbird
Account Manager
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: schneija, Assigned: aceman)

Tracking

(Blocks: 1 bug)

31 Branch
Thunderbird 38.0

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dupetome][workaround: click "Advanced config" instead of "Finish"])

Attachments

(3 attachments, 2 obsolete attachments)

27.35 KB, image/png
Details
14.07 KB, image/png
Details
7.07 KB, patch
Magnus Melin
: review+
BenB
: feedback-
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
Created attachment 8467630 [details]
Screenshots.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

- opened new mail account wizard to register a second mail account 
- incoming: IMAP with SSL/TLS & normal password (unencrypted) / outgoing: SMTP (default server settings from previous mail account)
See attachment Screenshot-1.



Actual results:

When I click on "Finish", the warning dialog pops up (see attachment Screenshot-2): although it tells me I can continue with these settings, there is no way to enable the "Finish" button! (because the checkbox "I understand the risks" is NOT being displayed)
- if I go back and enter another set of SMTP settings for this mail account, the checkbox "I understand the risks" however is displayed!


Expected results:

Since this security dialog is categorized as warning, the checkbox to enable the finish button should always be displayed, regardless whether default SMTP server settings are used or not.

Updated

3 years ago
Component: Untriaged → Account Manager
Summary: Mail account wizard cannot be completed → Mail account wizard (autoconfig) cannot be completed
(Assignee)

Comment 1

3 years ago
We have several reports of this but we couldn't reproduce it yet. But with your screenshots I could reproduce it now. It seems the catch is to use one of the existing SMTP servers (probably the settings of that one matters as I got different contents of the warning with different servers).

I'll look into this now.

As a temporary workaround, instead of "Finish", click "Advanced config", then you will not get this warning.
Assignee: nobody → acelists
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [dupetome][workaround: click Advanced config]
(Assignee)

Comment 2

3 years ago
Created attachment 8565073 [details]
Settings in the Account wizard
Attachment #8467630 - Attachment is obsolete: true
(Assignee)

Comment 3

3 years ago
Created attachment 8565074 [details]
Warning that cannot be dismissed
(Assignee)

Comment 4

3 years ago
Created attachment 8565099 [details] [diff] [review]
patch

OK, so the bug seems to be that if the user immediatelly selects an existing server, the fields for outgoing transport security and password security are hidden and are left at the values of "Autodetect". Then, the warning dialog checks if "outgoing.socketType > 1" and shows the the window if yes, but only shows there is a problem with security if ".socketType == 1" (None). But an unitialized (autodetect) field has a ".socketType == 0".

If you keep the SMTP server on the "new server" menuitem and fill in all the other fields to No security and no password encription, THEN switch to use an existing server, the warning comes up using the typed in values of the fields even though they are irrelevant now as the values are not used. The warning shows the name of the potential new server and its security settings even though you want to use a different, existing server.

So I propose to skip the warning if we use an existing SMTP server as we hopefully already warned about it when it was created and the user accepted that. Also other code in the wizard shortcuts if existing server is used as nothing needs to be created in that case.

The other fix is to solve an non-existent menulist.inputField error when switching from existing SMTP server to a new one.
Attachment #8565099 - Flags: review?(mkmelin+mozilla)
Attachment #8565099 - Flags: review?(ben.bucksch)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Whiteboard: [dupetome][workaround: click Advanced config] → [dupetome][workaround: click "Advanced config" instead of "Finish"]

Comment 5

3 years ago
Comment on attachment 8565099 [details] [diff] [review]
patch

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

Nice detective work, aceman! LGTM, r=mkmelin
Attachment #8565099 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8565195 [details] [diff] [review]
patch v2

Unfortunately, the fix was not complete. If the warning dialog was shown due to incoming server insecurity the uninitialized outgoing fields were still considered. It is not very good conceptually what the open() function does the same checks that needed() was doing (duplicates logic). So I rework it so that needed() sets up the logic (marks problems) and open() just blindly obeys and shows the needed info.
Attachment #8565099 - Attachment is obsolete: true
Attachment #8565099 - Flags: review?(ben.bucksch)
Attachment #8565195 - Flags: review?(mkmelin+mozilla)
Attachment #8565195 - Flags: feedback?(ben.bucksch)

Comment 7

3 years ago
Comment on attachment 8565195 [details] [diff] [review]
patch v2

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

::: mailnews/base/prefs/content/accountcreation/emailWizard.js
@@ +1658,5 @@
>  
> +  _inSecurityBad:  0x0001,
> +  _inCertBad:      0x0010,
> +  _outSecurityBad: 0x0100,
> +  _outCertBad:     0x1000,

There's little reason for these to be anything but booleans... too much c++ lately? :)
Attachment #8565195 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 8

3 years ago
So how do you propose to do it? How do I return the 4 values as a single one? Or should I make it an object or array?
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 9

3 years ago
I intended those as constants and the real information transfer to be in the return value of needed() that could then be decoded using those constants. Do you propose to change those a "global variables" (members of the object) that are set (bool) from inside needed() and then read from open() ?

Comment 10

3 years ago
Comment on attachment 8565195 [details] [diff] [review]
patch v2

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

I see now.
Attachment #8565195 - Flags: review+

Updated

3 years ago
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 11

3 years ago
OK, thanks.
Keywords: checkin-needed

Comment 12

3 years ago
https://hg.mozilla.org/comm-central/rev/645484c48931

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0

Comment 13

3 years ago
Comment on attachment 8565195 [details] [diff] [review]
patch v2

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

I don't like the bitflags. They make the code substantially harder to read as it was before, and they don't gain anything.
Attachment #8565195 - Flags: feedback?(ben.bucksch) → feedback-

Comment 14

3 years ago
(I was on a work trip last week and couldn't respond earlier.)
> -    return !incomingOK || !outgoingOK;
> +    return incomingBad | outgoingBad;

I don't see a reason for this change, and the inversion makes it harder to read.
(Assignee)

Comment 15

3 years ago
So how do you propose to do it?
(Assignee)

Updated

3 years ago
Blocks: 1140748
(Assignee)

Comment 16

3 years ago
Spun off the possible improvements to bug 1140748.

Comment 17

3 years ago
I am still experiencing this issue as of Thurderbird 31.5.0

If you would like additional debugging details, I would be happy to provide them.

Comment 18

3 years ago
[Blocking Requested - why for this release]:
blocking-b2g: --- → 3.0?

Comment 19

3 years ago
Please don't set flags you don't understand.
blocking-b2g: 3.0? → ---

Comment 20

3 years ago
Got your attention, didn't it? Just fyi, I have a Degree in Computer Science.
(Assignee)

Comment 21

3 years ago
(In reply to Jonathan Starrett from comment #17)
> I am still experiencing this issue as of Thurderbird 31.5.0
> 
> If you would like additional debugging details, I would be happy to provide
> them.

Please try TB38. The fix was not backported to TB31.x.

Updated

2 years ago
See Also: → bug 812750
You need to log in before you can comment on or make changes to this bug.