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)
SeaMonkey
MailNews: Account Configuration
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: iannbugzilla, Assigned: iannbugzilla)
References
Details
Attachments
(1 file, 10 obsolete files)
16.60 KB,
patch
|
iannbugzilla
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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.
Accepting bug and adding Seth Spitzer to cc
Status: NEW → ASSIGNED
Attachment #121091 -
Flags: review?(neil)
Attachment #121091 -
Flags: review?(neil)
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 6•22 years ago
|
||
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-
Updated patch taking on board Neil's comments
Attachment #121187 -
Attachment is obsolete: true
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 10•22 years ago
|
||
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)
Assignee | ||
Comment 11•22 years ago
|
||
Taking on board Neil's comment
Attachment #126646 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 12•22 years ago
|
||
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-
Assignee | ||
Comment 13•22 years ago
|
||
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 14•22 years ago
|
||
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)
Assignee | ||
Comment 15•22 years ago
|
||
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)
Assignee | ||
Comment 16•22 years ago
|
||
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 17•22 years ago
|
||
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-
Assignee | ||
Comment 18•22 years ago
|
||
Tidy up as requested in comment#17
Attachment #128543 -
Attachment is obsolete: true
Attachment #128573 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 19•22 years ago
|
||
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+
Assignee | ||
Comment 20•22 years ago
|
||
Taking on board tweaks to style
Attachment #128573 -
Attachment is obsolete: true
Assignee | ||
Comment 21•22 years ago
|
||
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 22•22 years ago
|
||
Comment on attachment 128597 [details] [diff] [review]
Patch v1.3d
sr=bienvenu
Attachment #128597 -
Flags: superreview?(bienvenu) → superreview+
Assignee | ||
Comment 23•22 years ago
|
||
Checked in by timeless, thank you
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Updated•21 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•