Closed Bug 201469 Opened 22 years ago Closed 22 years ago

Add smtp server's username field to Account Wizard

Categories

(SeaMonkey :: MailNews: Account Configuration, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: iannbugzilla, Assigned: iannbugzilla)

References

Details

Attachments

(1 file, 10 obsolete files)

Spun off from thoughts on bug 107953 Currently Account Wizard sets smtp server's username to be the same as the incoming server's username if the hostnames are the same or the username part of the email address if the hostnames are different. If this field is added we might still want to autofill the smtp server's username the same way we do for incoming server's username (defaults to username part of the email address) but if you overtype the incoming username and the incoming and smtp servers' hostnames are the same we should autofill with that username.
Taking bug
Assignee: racham → ian
Attached patch Patch v1.0 (obsolete) — Splinter Review
First pass at patch to implement smtp username field
Accepting bug and adding Seth Spitzer to cc
Status: NEW → ASSIGNED
Adding Neil to cc so he can cast his eye over this patch
Attachment #121091 - Flags: review?(neil)
Attachment #121091 - Flags: review?(neil)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Basically the same as patch v1.0 just with some added comments to explain what's going on in some of the new code.
Attachment #121091 - Attachment is obsolete: true
Attachment #121187 - Flags: review?(neil)
Comment on attachment 121187 [details] [diff] [review] Patch v1.1 These nested boxes look like a lame hack. Just have three boxes and show the one you want (presumably you would write a new function hideShowLoginSettings).
Attachment #121187 - Flags: review?(neil) → review-
Attached patch Patch v1.2 (obsolete) — Splinter Review
Updated patch taking on board Neil's comments
Attachment #121187 - Attachment is obsolete: true
Attached patch Patch v1.2a (obsolete) — Splinter Review
Same as patch 1.2 except updated against trunk to reflect recent changes to AccountWizard.js
Attachment #121461 - Attachment is obsolete: true
Attachment #121702 - Attachment is obsolete: true
Attachment #125583 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 125583 [details] [diff] [review] Updated patch against current trunk >+function hideShowLoginSettings(aEle, bEle, cEle) { >+ >+ document.getElementById("loginSet"+aEle).removeAttribute("hidden"); >+ document.getElementById("loginSet"+bEle).setAttribute("hidden", "true"); >+ document.getElementById("loginSet"+cEle).setAttribute("hidden", "true"); >+ >+} Quick nit for now: use .hidden = true; or .hidden = false;
Attachment #125583 - Attachment is obsolete: true
Attachment #125583 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Patch v1.2c (obsolete) — Splinter Review
Taking on board Neil's comment
Attachment #126646 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 126646 [details] [diff] [review] Patch v1.2c I was offline when I reviewed this so I hope that I can remember... When you create an account for the first time, then get to the smtp page, then go back and change your username, then go forward, the smtp page still displays the old username. Here are some other nits: >+ while (smtpStatic.hasChildNodes()) >+ smtpStatic.removeChild(smtpStatic.firstChild); >+ var staticTextNode = document.createTextNode(staticText); >+ smtpStatic.appendChild(staticTextNode); I know you've only copied this code but you can in fact change the nodeValue of a text node, which should be simpler than the above code. >+ var smtpusername = document.getElementById("smtpusername").value; >+ if (smtpusername == "") { >+ smtpusername = username; >+ } >+ > setPageData(pageData, "login", "username", username); >+ setPageData(pageData, "login", "smtpusername", smtpusername); JS has a nifty || function which comes in useful here, you can remove the above if and use smtpusername || username here instead. >+ if (smtpServer && smtpServer.hostname && smtpServer.username && >+ smtpServer.hostname != "" && smtpServer.redirectorType == null) { Copying again. if && smtpServer.hostname && succeeds then it will also be != "" so that check is unnecessary. >+function hideShowLoginSettings(aEle, bEle, cEle) { >+ >+ document.getElementById("loginSet"+aEle).hidden = "false"; >+ document.getElementById("loginSet"+bEle).hidden = "true"; >+ document.getElementById("loginSet"+cEle).hidden = "true"; .hidden is actually a boolean i.e. no "s
Attachment #126646 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch Patch v1.3 (obsolete) — Splinter Review
Taken on board all review comments from comment#12 and also fixed bug in patch found by Neil, fixed another similar bug. This was where, if you intially set incoming/outgoing servers to be different and also the usernames and then went back within the wizard and made the incoming/outgoing servers the same, it remembered the outgoing username - it now forgets it.
Attachment #126646 - Attachment is obsolete: true
Attachment #128046 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 128046 [details] [diff] [review] Patch v1.3 Something that I think I forgot from my last review - if you create a new account and specify identical servers, then change your mind and go back and specify different identical servers, then the user name page will still display the old outgoing server. Also, I don't think you have to worry about what you set the smtpNameInput.value to if you have a default SMTP server.
Attachment #128046 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Patch v1.3a (obsolete) — Splinter Review
Problem identified in comment#14 has been fixed by rewriting the function modifyStaticText to make use of data elements.
Attachment #128046 - Attachment is obsolete: true
Attachment #128100 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #128100 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Patch v1.3 (obsolete) — Splinter Review
Eliminates use of data elements by adding attributes to description elements and using them, has the side effect of reducing number of getElementById usages. Done some tidying up of trailing spaces and using braces when they're not needed.
Attachment #128100 - Attachment is obsolete: true
Attachment #128543 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 128543 [details] [diff] [review] Patch v1.3 Almost there... >+ <description id="smtpStaticText1" style="width: 200px;" prefix="&haveSmtp1a.label;" >+ suffix="&haveSmtp1b.label;">*</description> It's typical to name entities after the attribute, i.e. haveSmtp1.prefix and haveSmtp1.suffix etc. It might be worth making a localization note to explain what's going on. A couple of style nits: Put spaces after commas e.g. here: >+ hideShowLoginSettings(2,1,3); Put spaces around + e.g. here >+ document.getElementById("loginSet"+aEle).hidden = false; >+ document.getElementById("loginSet"+bEle).hidden = true; >+ document.getElementById("loginSet"+cEle).hidden = true;
Attachment #128543 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch Patch v1.3c (obsolete) — Splinter Review
Tidy up as requested in comment#17
Attachment #128543 - Attachment is obsolete: true
Attachment #128573 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 128573 [details] [diff] [review] Patch v1.3c r=me, but please fix these style nits: I would say existing style is for >+ try >+ { >+ smtpServer = parent.smtpService.defaultServer; >+ } >+ catch(ex){} try { } catch (ex) {} (existing style doesn't apply to spaces, use this style) >+ } >+ else >+ { } else { etc. i.e. { should be on the same line as try/else/if
Attachment #128573 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attached patch Patch v1.3dSplinter Review
Taking on board tweaks to style
Attachment #128573 - Attachment is obsolete: true
Comment on attachment 128597 [details] [diff] [review] Patch v1.3d Carrying forward r= from Neil
Attachment #128597 - Flags: superreview?(sspitzer)
Attachment #128597 - Flags: review+
Attachment #128597 - Flags: superreview?(sspitzer) → superreview?(bienvenu)
Comment on attachment 128597 [details] [diff] [review] Patch v1.3d sr=bienvenu
Attachment #128597 - Flags: superreview?(bienvenu) → superreview+
Checked in by timeless, thank you
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: