After creating news account through clicking news URL Account Wizard is broken (skips account type selection, bound to invalid news account)
Categories
(SeaMonkey :: MailNews: Account Configuration, defect)
Tracking
(seamonkey2.39 affected)
Tracking | Status | |
---|---|---|
seamonkey2.39 | --- | affected |
People
(Reporter: InvisibleSmiley, Assigned: frg, NeedInfo)
References
()
Details
(Keywords: relnote)
Attachments
(4 files, 3 obsolete files)
(deleted),
application/octet-stream
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
iannbugzilla
:
feedback+
|
Details | Diff | Splinter Review |
Comment 3•15 years ago
|
||
Comment 4•15 years ago
|
||
Comment 6•15 years ago
|
||
Comment 7•15 years ago
|
||
Reporter | ||
Updated•14 years ago
|
Reporter | ||
Comment 8•14 years ago
|
||
Comment 11•12 years ago
|
||
Comment 12•12 years ago
|
||
Comment 14•11 years ago
|
||
Comment 15•9 years ago
|
||
Comment 16•9 years ago
|
||
Reporter | ||
Comment 17•9 years ago
|
||
Updated•9 years ago
|
Assignee | ||
Comment 20•9 years ago
|
||
Assignee | ||
Comment 21•9 years ago
|
||
Comment 22•9 years ago
|
||
Comment 24•9 years ago
|
||
Assignee | ||
Comment 25•9 years ago
|
||
Assignee | ||
Comment 26•8 years ago
|
||
Assignee | ||
Comment 27•8 years ago
|
||
Comment 28•8 years ago
|
||
Comment 29•8 years ago
|
||
Assignee | ||
Comment 31•6 years ago
|
||
Comment 32•6 years ago
|
||
Assignee | ||
Comment 33•6 years ago
|
||
Comment 34•6 years ago
|
||
Assignee | ||
Comment 36•6 years ago
|
||
Comment 37•6 years ago
|
||
Comment 38•6 years ago
|
||
Comment 39•6 years ago
|
||
Comment 40•6 years ago
|
||
Comment 41•6 years ago
|
||
Assignee | ||
Comment 42•6 years ago
|
||
Comment 43•6 years ago
|
||
Comment 44•6 years ago
|
||
Comment 45•6 years ago
|
||
Comment 46•6 years ago
|
||
Comment 47•6 years ago
|
||
Updated•6 years ago
|
Comment 48•5 years ago
|
||
Yes I think the news account creation is the same in TB and SM so the patch is needed in TB too, as was confirmed by Alfred and Jorg.
But it is an ugly cludge and needs to be marked as such in the code (it seems it is already) so that we remove it later.
We then need to find the real reason why this happens. If the server is actually usable (after removing the .valid pref), then it shouldn't get the .valid=false flag from the wizard. Or if it is not usable (finished seting up), then it should be finished (a news server without subscribed newsgroups) should be fine. Or it should be deleted as a whole upon clicking Cancel.
I think there was a report of an empty identity being attached to the account and then also comment 47, which may actually have the same cause.
So we do not really know if the created server is fully usable, we just hack away the .valid pref.
There are nice STR in comment 45. Do we know which code runs in step 4 and creates the news account automatically? Then, is the subscribe dialog the standalone one, you can open any time to subscribe to more newsgroups? Or is is part of the "other account" creation wizard?
Assignee | ||
Comment 49•5 years ago
|
||
I will see that I update the patch. Sorry for the inactivity here. Mostly did firefighting and trying to get the next release out.
And aceman is right. It is a cludge. Should be reproducible if you just cancel create an account at a certain stage.
Updated•4 years ago
|
Comment 51•3 years ago
|
||
Just some ideas after the tracking this issue (with current SeaMonkey code).
When a new account is created through the clicking news URL, nsNntpService::CreateNewsAccount is called. Since it does not know what exactly should be set for "FullName" and "Email" in this case, it leaves these parameters undefined, and set the Valid flag to false:
// the identity isn't filled in, so it is not valid.
rv = (*aServer)->SetValid(false);
if (NS_FAILED(rv)) return rv;
Probably it follows some "ancient" logic, when in a case of impossibility to completely fill in all the required data "right now", a certain flag is set in order to complete the creation of this account later.
So on its next run, AccountWizard sees that some account has "false" valid flag and immediately invites the user to complete (or fix) this account. To do this, onAccountWizardLoad() calls checkForInvalidAccounts().
The follow-up process is currently broken.
First, user follows "identitypage", "newsserver", "accnamepage" steps. Both "newsserver" and "accnamepage" show the same current value of the correspond news server name, but both do not allow user to go further because of the disabled Next button. The button is disabled, because the code wrongly assumes that such a server and such an account already exist.
The problem is AccountExists() (for "newsserevr") and accountNameExists() (for "accnamepage") ignore the "valid" flag when searching, and interprete the matched data as referring to another valid server.
The following changes fix this:
--- comm/mailnews/base/prefs/content/AccountWizard.js.orig 2021-07-19 01:02:40.194695831 +0300
+++ comm/mailnews/base/prefs/content/AccountWizard.js 2021-07-19 01:03:25.146412465 +0300
@@ -708,7 +708,8 @@ function setDefaultCopiesAndFoldersPrefs
function AccountExists(userName, hostName, serverType)
{
- return MailServices.accounts.findRealServer(userName, hostName, serverType, 0);
+ let server = MailServices.accounts.findRealServer(userName, hostName, serverType, 0);
+ return server && server.valid != false;
}
function getFirstInvalidAccount()
and
--- comm/mailnews/base/prefs/content/amUtils.js.orig 2021-07-19 01:05:23.732664849 +0300
+++ comm/mailnews/base/prefs/content/amUtils.js 2021-07-19 01:05:47.846512850 +0300
@@ -198,6 +198,7 @@ function accountNameExists(aAccountName,
Ci.nsIMsgAccount))
{
if (account.key != aAccountKey && account.incomingServer &&
+ account.incomingServer.valid != false &&
aAccountName == account.incomingServer.prettyName) {
return true;
}
(It is unlikely that this introduces the ability to create a "duplicate server" somewhere, since it requires the user to run AccountWizard (to manually fill such duplicate data), but then it will first force the user to complete the non-valid account anyway.)
Second, when the account data is completed such a way, that account becomes valid, but the valid flag remains unchanged (ie. "false").
To fix this, the following change is needed for FinishAccount() function:
--- comm/mailnews/base/prefs/content/AccountWizard.js.orig 2021-07-19 01:24:03.807566601 +0300
+++ comm/mailnews/base/prefs/content/AccountWizard.js 2021-07-19 01:24:28.850407172 +0300
@@ -178,6 +178,8 @@ function FinishAccount()
// we might be simply finishing another account
if (!gCurrentAccount)
gCurrentAccount = createAccount(accountData);
+ else
+ gCurrentAccount.incomingServer.valid = true;
// transfer all attributes from the accountdata
finishAccount(gCurrentAccount, accountData);
(Note, createAccount() normally manipulates the valid flag and finally set it to true as well).
Applying these fixes, we restore that "ancient" account completion algorithm. Such news accounts will be created with valid="false" as for now, but the next time the AccountWizard is run, the user will be able to complete them propely, and the flag will disappear.
The same is true for accounts that have ever had this flag set in the past and remains set until now (they will be fixed this way too).
Comment 52•3 years ago
|
||
The changes above restore the account completion algorithm.
But in general, the whole path of processing such news links does not look as the best way.
It looks strange a bit to have such a delay between the account creation and its follow-up fixing sometime later in an arbitrary time. It seems that initially such a fixing performed without a delay, but just at the next start of the messenger.
When the messenger starts, say standard SeaMonkey's 3 pane window, OnLoadMessenger() runs verifyAccounts(), which has a code fragment regarding invalid accounts:
var accountCount = accounts.length;
var invalidAccounts = getInvalidAccounts(accounts);
if (invalidAccounts.length > 0 && invalidAccounts.length == accountCount) {
prefillAccount = invalidAccounts[0];
}
so when ALL the account are invalid, the AccountWizard will be run. (And this happens after the clicking news URL when there are no any accounts yet, ie. new profile etc.).
But a bit below there is a comment:
// prefillAccount is non-null if there is at least one invalid account.
that follows a different logic, namely:
--- accountUtils.js.orig 2021-07-19 02:54:35.037675140 +0300
+++ accountUtils.js 2021-07-19 02:54:40.176742599 +0300
@@ -96,7 +96,7 @@ function verifyAccounts(wizardCallback,
// as long as we have some accounts, we're fine.
var accountCount = accounts.length;
var invalidAccounts = getInvalidAccounts(accounts);
- if (invalidAccounts.length > 0 && invalidAccounts.length == accountCount) {
+ if (invalidAccounts.length > 0) {
prefillAccount = invalidAccounts[0];
}
Perhaps sometime in the past, the code was created exactly like this, and the AccountWizard was run whenever there was "at least one invalid account" at the messenger startup.
With such a change, AccountWizard will invite user to fix the invalid account at any messenger start, including the start performed after the clicking on the news URL. This way there will be no any delay, since the user fixes things immediately after the clicking news URL.
OTOH, the logic of the news links clicking implies some easy and fast operations, and to cause the user to completely fill the account data here looks a bit out of place. Besides this, users are already accustomed that this is not required, and things work OK without this (until they try to create a new account).
So the idea to run AccountWizard at any messenger startup when non-valid accounts present is questionable.
Comment 53•3 years ago
|
||
A better idea is to not set the false valid flag at all in nsNntpService::CreateNewsAccount().
It will lead to the same situation as after the recommended work-around is done (ie. manually drop "mail.server.serverX.valid" prefs). Things work, but the FullName and Email parameters remains unset. The user will only notice this when will try to "send" something to the newsgroup. But then an email from the first valid account will be chosen by default for the From address.
Some users complain that such address substitution (when an address from another account is substituted instead of an empty one) is inconvenient and fraught with the loss of private data. The user can in a hurry send a message with the private From address, but he would prefer to hide it from spammers in such a public area as newsgroups.
So a slightly better way is to fill some fake data, following the way how it was done for AccountWizard.js:onCancel() -- ie. set FullName just to the empty string, and set Email to the same fake value of "user@domain.invalid".
The change is:
--- comm/mailnews/news/src/nsNntpService.cpp.orig 2020-02-18 02:36:20.000000000 +0300
+++ comm/mailnews/news/src/nsNntpService.cpp 2021-07-19 03:48:05.804759761 +0300
@@ -948,9 +948,12 @@ nsNntpService::CreateNewsAccount(const c
rv = identity->SetComposeHtml(false);
NS_ENSURE_SUCCESS(rv,rv);
- // the identity isn't filled in, so it is not valid.
- rv = (*aServer)->SetValid(false);
- if (NS_FAILED(rv)) return rv;
+ // Use fake data for now (user can change it in AccountWizard whenever want)
+ rv = identity->SetEmail(NS_LITERAL_CSTRING("user@domain.invalid"));
+ NS_ENSURE_SUCCESS(rv,rv);
+
+ rv = identity->SetFullName(NS_LITERAL_STRING(""));
+ NS_ENSURE_SUCCESS(rv,rv);
// hook them together
rv = account->SetIncomingServer(*aServer);
With this change the false valid flag is not set anymore. (And solution from comment #51 is not even needed if there are no such half-broken accounts in the past).
When the user post something ti the newsgroup, the From field is "user@domain.invalid". Then he/she either send it as is (for rare posting etc.), or choose an email address from another account, or run AccountWizard to fill the email properly in a case of frequent posts.
Comment 54•3 years ago
|
||
Personally, I lean towards both of the comment #51 and comment #53 solutions together. I will try it at the downstream level (Fedora Linux).
Certainly it could be fine if anybody more skilled in these areas prepare all the ideas and provide some final patches etc. (for both SeaMonkey and Thunderbird).
Comment 55•3 years ago
|
||
The change in behaviour described in comment 52 - the wizard only showing up to complete invalid accounts when there are no valid accounts - appears to have been introduced in bug 476135.
Comment 56•3 years ago
|
||
Yep, it was bug #476135, and it was somewhat of a kludge. Seeing that something was going wrong, they just postponed its effect for later.
But this means that the initial algorithm ran the AccountWizard whenever any account was found to be not valid. Then user fix (or remove) it. And it seems to be the reason to consider comment 52 to implement (ie. the same as to revert bug #476135).
When user is invited to fix non-valid account that way, he sees an "identity" page, and it is not completely obvious for him that it is about an uncompleted news account. He can guess it if the AcountWizard appears right after the clicking on the news URL, but he will be confused, if it appears later (either when add a new account, as for now, or even when start messenger, as with comment 52).
Fortunately, when user clicks Cancel on such "identity" page, onCancel() fills the fake data (comment 53 just took this idea from here exactly) and then run FinishAccounts(), which now works properly since fixed by comment 51. The result will be the same as proposed in comment 53.
Given that, perhaps comment 52 idea is no longer so questionable...
Comment 57•3 years ago
|
||
Fedora Linux patch for now, comment 51 plus comment 53 .
Updated•3 years ago
|
Description
•