238583 - Extend Account Manager UI to allow multiple accounts with same username and server, but different ports

Status: NEW

(MailNews Core :: Account Manager, enhancement)

Description

[Kevin Teuscher]

Add the UI capability to add multiple accounts with the same username and
server, but different ports.  This is the front-end half that goes with bug 41929

Comment 1

[Kevin Teuscher]

Attachment: Mock-up of Server Info panel in Wizard

This is a rough layout of what the new panel would look line with the optional
port specification.  The "Standard" radio will come up selected and the textbox
is disabled unless the "Other" button is selected.

Kevin

Comment 2

[Kevin Teuscher]

Attachment: Mockup of the Done panel of the wizard when the Standard port is selected

Comment 3

[Kevin Teuscher]

Attachment: Mockup of the Done panel of the wizard when another port is selected

Comment 4

[Kevin Teuscher]

I'd like to get comments and feedback on this UI approach before I go and write
code to support it.

David:  If you know of anyone else who should review this UI approach, please
let them know.

Kevin

Comment 5

[David :Bienvenu]

yes, Neil will probably want to look at it.

Comment 6

[Stefan Borggraefe]

Ok, here is some feedback.

Instead of "Standard" we should use "Default" here, because this word is used in
the Account Settings dialog in the same context.

I think the wizard should be very easy to use, because if the user fails at this
point he can't use MailNews/TB at all. I can imagine a lot of users will use
this wizard with a letter from their Internet Service Provider or Freemail
Provider at hand with the information about their new email account. Usually
such letters don't mention a port number. So the user will be in doubt what to
enter here. To avoid this problem, there should be a sentence like "If in doubt,
leave this unchanged."

A more advanced user might try to create an SSL connection by entering a default
SSL port number here (993 resp. 995). So we should either just do this
automatically for him in this situation or there should be some kind of UI to
allow the user to explicitly select that SSL should be used (bug 80919).

Comment 7

[Kevin Teuscher]

Stefan:  Thanks for the comments; I agree and have taken that other bug and will
add it to this update of the wizard.

Anyone else?

I'm no expert on the wording, so if someone has their preference, let me know now

Kevin

Comment 8

[[e-mail]]

I'll just mention that I think that, assuming that there's enough space to fit
them all in, the controls should be in the order type, hostname, secure, port.

Comment 9

[Kevin Teuscher]

(In reply to comment #8)
> I'll just mention that I think that, assuming that there's enough space to fit
> them all in, the controls should be in the order type, hostname, secure, port.

Neil:

There is no way to fit type, hostname, secure, and port all on the main wizard
panel.  What about this concept:

Leave the panel layout like it is now with type and hostname and outgoing host
but add a button near the type radio buttons that opens a dialog where the
secure and port info can be set like this (in ascii art)

                    XXXXXXXXXXXXXX
  0 Pop    0 IMAP   X  Advanced  X
                    XXXXXXXXXXXXXX

  Server Name


This is something like what is done with the CreateProfileWizard and the
SetLanguage Dialog that pops up.

Considering that bienvenu is making the wizard larger for his pop3 deferrment
UI, I don't think I want to make the wizard even larger.  Also this keeps this
extra info out of site for the typical user.  I'm open to the name of the button
-- Advanced; Extra Settings; .....

Kevin

Comment 10

[[e-mail]]

Maybe we should split the incoming and outgoing servers on to separate pages?

Comment 11

[Kevin Teuscher]

(In reply to comment #10)
> Maybe we should split the incoming and outgoing servers on to separate pages?

That may give enough room and since we are adding port capabilities to the
incoming server, adding it to the outgoing would follow.

I think that we may still want to have a button (below the hostname if we move
the outgoing server) so that the port and secure items are "out of the way" for
most users.  Do you agree?

Kevin

Comment 12

[Kevin Teuscher]

Attachment: UI patch

Here is a version that gives the ui for choosing the port and whether a secure
version of the protocol is used.  This is available to POP, IMAP and NNTP
accounts in the wizard.  There may be some more bulletproofing that I need to
do, but everything seems to work for me in testing it.

I'm still concerned about the "server" page in the mail version of this. 
Between bienvenu's additions for deferement and asking for the outgoing server,
the dialog is quite busy.  I'm not sure which way we want to go.  Neil gave his
opinion.  Is that the consensus?  mscott? bienvenu?

Comment 13

[Kevin Teuscher]

Comment on attachment 155022 [details] [diff] [review]
UI patch

Who wants to review/sr?
Neil?
bienvenu?

Comment 14

[Kevin Teuscher]

Attachment: Better Bulletproofing

This attachment moves strings out so that they can be localized and
bulletproofs the selection of the port number

Comment 15

[Kevin Teuscher]

Comment on attachment 156433 [details] [diff] [review]
Better Bulletproofing

Can I get a review/sr now?

Comment 16

[David :Bienvenu]

+  if (stype)
+  {
+    if (servtype == "pop3")
+    {
+        stype.setAttribute("value", "POP3");
+    }
+    else // We must have an imap server
+    {
+        stype.setAttribute("value", "IMAP");
+    }
+  }
this can just be:

stype.setAttribute("value",  (serverType == "pop3") ? "POP3" : "IMAP");

why this .portt var name? Does .port conflict with something?

Comment 17

[Kevin Teuscher]

(In reply to comment #16)
> +  if (stype)
> +  {
> +    if (servtype == "pop3")
> +    {
> +        stype.setAttribute("value", "POP3");
> +    }
> +    else // We must have an imap server
> +    {
> +        stype.setAttribute("value", "IMAP");
> +    }
> +  }
> this can just be:
> 
> stype.setAttribute("value",  (serverType == "pop3") ? "POP3" : "IMAP");
Done.

> 
> why this .portt var name? Does .port conflict with something?
Remanant of keeping all of my .port ids and variables separate while hashing
this out.  I have changed it to .port.

I'll attach a new patch after Neil does his review.

Kevin

Comment 18

[[e-mail]]

Comment on attachment 156433 [details] [diff] [review]
Better Bulletproofing

I'm sorry for overlooking this, I've got a number of outstanding reviews which means that things tend to slip :-(

I can't actually try this out because it is so old. However, there were some oddities that I noticed by reading through it:

It looks as if createIncomingServer needs to be extended to accept a port and secure flag.

You appear to extend the "findRealServer" function (although you use a renamed version) but there appears to be no backend code to support this.

You use new RegExp("^[0-9]{1,}$"); instead of /^\d+$/ for some reason.

You specify some new files in jar.mn but none appear to have been attached.

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 156433 [details] [diff] [review]
Better Bulletproofing

Clearing sr request per comment #18.

Comment 20

[[e-mail]]

(In reply to comment #18)
>(From update of attachment 156433 [details] [diff] [review])
> You use new RegExp("^[0-9]{1,}$"); instead of /^\d+$/ for some reason.
You could use /^[0-9]+$/ if you only want to match ASCII digits; I understand /^\d+$/ will also match Unicode digits.

Comment 21

[Jens Müller (:tessarakt)]

Why should this limited to "different ports"?

Comment 22

[:aceman]

(In reply to Jens Müller from comment #21)
> Why should this limited to "different ports"?

What do you mean?

Comment 23

[:aceman]

I understand the Account manager changes here and could pick that up from the patch (it is still needed for TB12+).
But I am not yet familiar with the Account wizard. Could we split it?

Comment 24

[Jens Müller (:tessarakt)]

(In reply to :aceman from comment #22)
> (In reply to Jens Müller from comment #21)
> > Why should this limited to "different ports"?
> 
> What do you mean?

Well, one should "Add the UI capability to add multiple accounts with the same username and server,", and the same port.

Comment 25

[:aceman]

(In reply to Jens Müller from comment #24)
> Well, one should "Add the UI capability to add multiple accounts with the
> same username and server,", and the same port.

What would that be good for? Any use case?

Comment 26

[:aceman]

Kevin Teuscher, are you still working on this? I think I would be able to finish the Account manager part.

Comment 27

[Philip Chee]

> Kevin Teuscher, are you still working on this?
That was 7+ years ago. Suggest that you grab this and run with it.

Comment 28

[:aceman]

I've noticed TB allows to set two account to the same server if the server is written in different case. I think hostnames are case insensitive so that would resolve to the same server. I'll try to fix it in this bug too.

Should I just treat the names case-insensitively or also convert the field value to lower-case so that the user sees it?

Comment 29

[WADA:World Anti-bad-Duping Agency]

Concerns in current account creation.

(i) If server name / user name is touched or changed at Server Settings, mail.server.serverN.realhostname/realuserName is created.
I think mail.server.serverN.realhostname/realuserName is better created since initial account creation.
If this enhancement is done, "key of password manager==realhostname+realuserName" is easily achieved and user's confusion will be reduced.

(ii) Duplication check should be executed on both (a) type+hostname+userName+port of all accounts and (b) type+realhostname(hostname is used if realhostname doesn't exist)+realuserName(userName is used if realuserName doesn't exist)+port of all accounts.
Bug 303542 occurs when host name or user name is changed(realhostname or realuserName is created with different value from hostname or userName), because current duplication check is (b) only. To keep uniqueness of internal server path/folder path based on hostname+userName, (a) should always be executed too.

Question on password after change by this bug.
(c) Is same password for both type+hostname+userName+PortA and type+hostname+userName+PortB? (type+hostname+userName = key of password manager)
(d) Or different password for type+hostname+userName+PortA and type+hostname+userName+PortB? (type+hostname+userName+port = key of password manager)

If actually same server, it's usually (c), but if SSL tunnel like usage, (d) is perhaps required. Because Gmail IMAP(imap.gmail.com) supports both 993(SSL/TLS) and 143(StartTLS), (c) is mandatory when port number is change at Server Settings. If (d) is used in any case, Gmail user has to enter password again even though port number change only. Will this kind of issue be resolved easily after fix of this bug?

Question on News account.
Is there no need to create server.serverN.userName for news account?
  - Password manager entry for each username and password 
    ==> single password manager entry for both username and password
    is already being processed. (may be fixed already)
  - Different news accounts with different userid on same news account
    is requested, but is not implemented yet.
Even if actual use of userName/realuserName in back end ad UI is postponed, I think it's better created in prefs.js for future enhancement.

Comment 30

[:aceman]

WADA, the password concern is probably bug 539927.

Comment 31

[:aceman]

Bug 539927 probably needs to be resolved first before we allow this in the Account manager. Otherwise users will configure it and it will not work.

Comment 32

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #31)
> Bug 539927 probably needs to be resolved first before we allow this in the
> Account manager.

I don't think Bug 539927 is right place to process following issues,
  - "internal unique identifier of an account" is lost by change of this bug.
  -  "primary/unique key of password manager's DB" is lost by change of this bug.
because that bug report is based on "multiple accounts of same server type/same server name/same username with different port" which can't occur in current Tb due to this bug.
Even though that bug's claim of "Tb currently doesn't save different password for same type/hostname/userName with different port" is absolutely correct, I believe that resolving of the issue shouldn't be executed in bug report which is based on wrong bases.

I think following issues are issues which will newly be generated by this bug's change.
- "internal unique identifier of an account" is lost by change of this bug.
  type+hostname+userName is used as internal unique identifier of this account.
    ("mailbox:// if pop3, imap:// if imap" + userName + "@" + hostname)
    ( in internal account/folder path represented in URL format.      )
    (problem of Bug 303542 is loss of uniquness of this identifier.   )
- "primary/unique key of password manager's DB" is lost by change of this bug.
  type+hostname+userName is used as primary/unique key of password manager's DB.
    ("mailbox:// if pop3, imap:// if imap" + userName + "@" + hostname)
    (user name saved in DB is actual userID used in login.)

These issues should be resolved by this bug which will surely generate new issues.

Comment 33

[Wayne Mery (:wsmwk)]

aceman, judging by comments thus far, this will not help bug 709442, correct?

Comment 34

[:aceman]

No, this bug is about less strict checks (hostname+username duplications allowed) so it may introduce more problems (like bug 539927 and bug 41929).

See bug 327812 and bug 80855 (and dependencies) for examples where we add more strict checks to avoid TB confusion.

Otherwise I am not exactly sure what bug 709442 is really about. It looks too broad an vague.

Comment 35

[WADA:World Anti-bad-Duping Agency]

Problem is that duplication check by account manager is done by following.
  type+hostname(or realhostname if exists)+userName(or realuserName if exists)
It should be used only upon server access after realhostname/realuserName is introduced.
Due to this, Bug 303542 occurs, and due to this, it's impossible to define muliple accounts of "same type/server/userID but different port" even though realhostname/realuserName is already introduced.
If "duplication check by mixture of hostname and realhostname, userName and realuserName" is removed, following is possible without breaking current uniquness of any internal unique identifier/key,
- Create account with different hostname and different userName and with any port
- Change server name in realhostname, change user ID in realuserName, change port
Tb requires uniqueness of type+hostname+userName only.
There is no need of uniqueness in type+realhostname+realuserName+port.
Even when multiple Tb accounts accessed same type+realhostname+realuserName+port,
there is no problem even if they can interfere each other,
because the interfering is same as "multiple Tb instances with same account definition on multiple PC" or "multiple Tb instances with same account definition, with -no-remote, on single PC".

To support muliple accounts of "same type/server/userID but different port", some changes are needed.

(A) Account creation
  - always generate realhostname/realuserName from requested hostname/userName.
  - strict duplication check by type+hostname+userName.
    - if no duplicate, go ahead.
    - if duplicated, duplication check by type+realhostname+realuserName+port.
      - if no duplicate, increment suffix of userName(never change realuserName)
        until unique type+hostname+userName is obtained.
        if suffix exceeded 9999, abort account creation.
        (because hostname is used for mail directory name, don't touch)
      - if duplicates is found, abort account creation.
Suffix in userName is similar to suffix in mail directory name(dir-rel) based on hostname.
Same type+realhostname+realuserName+port shouldn't be created by auto-config, because request of same type+realhostname+realuserName+port is user's mistake.

(B) Account Setting
    If realhostname/realuserName doesn't exist, always create it.
    Duplication check by type+hostname+userName in case of manual prefs.js change.
    Duplication check by type+realhostname+realuserName+port upon setting change.
    If type=non-IMAP, reject dupulicated type+realhostname+realuserName+port.
    If IMAP, ask user about intentional or not, and accept it if intentional,
    and cancel changes if by mistake.
    This is required in next case.
    Multiple Tb accounts for same IMAP account for different folder pane layout.
    - one with namespace=""
    - other with namespace="abc/def/ghi/" to avoid deep nest at folder pane.   

(C) No change is needed in internal folder URL and key of password manager, because uniqueness of type+hostname+userName is always kept. Showing realhostname & realuserName to user instead of hostname & userName is needed at password manager panel to avoid user's confusion.

In Gmail IMAP case, duplicated accounts with 993(SSL/TLS) and 143(StartTLS) are created by above, if user requests account creation with 143(StartTLS) when Gmail IMAP account with 993(SSL/TLS) already exists. However, it's user's intentional request and account deletion is possible even after user requested Gmail IMAP account creation with 143(StartTLS) by his mistake. So I believe there is no problem in Gmail IMAP case.
In Gmail IMAP case, password is prompted again by port number change only, but it's very rare, because intentional change from 993(SSL/TLS) to 143(StartTLS) is mistake or test purpose only.

Comment 36

[:aceman]

I think the Account Wizard changes are no longer needed, it is possible to set the port there today (at least in TB).

I could do the Account manager part, but we need to resolve the blocking bugs first.

Comment 38

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #36)
> but we need to resolve the blocking bugs first.

Bug 303542?

If so, both this bug and that bug can be resolved at same time.

Cause of this bug and that bug is:
  Duplication check is currently done on;
      type
    + HostName (realhostname. if realhostname is not defined, hostname)
    + UserName (realuserName. if realuserName is not defined, userName) If duplication check is done on following separately, problem is resolved.
(A) access path to an account on a server(for this bug)
    type + realhostname + realuserName + port
(B) internal path name in Tb(for Bug 303542)
    type + hostname + userName

After realhostname and realuserName was introduced, hostname and userName is merely string to generate unique internal path name in Tb.
So, if requested "type + hostname + userName" is already used by other server.serverN, using hostname=hostname+"_"+port(+suffix if required), or using hostname=user supplied unique string, is an simple way to gurantee uniqueness of (B).

For this bug's purpose, "same type+realhostname+realuserName but different number" can be silently accepted and be created.
However, in case of "user tries to define same account with different security level such as none/StartTLS/SSL(==different port)", warning on "same type+realhostname+realuserName but different port number" is better issued by checking of (A).