106534 - loop over all identities on sending unsent messages, or fix how we find the unsent message folder

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P1)

Description

[(not reading, please use [e-mail] instead)]

nsMsgSendLater::GetUnsentMessagesFolder() takes an identity and calls
LocalMessageFolder()
which calls
GetUnsentMessagesFolder()
which calls
GetFoldersWithFlag(MSG_FOLDER_FLAG_QUEUE,..)

I need to investigate, and see what happens if I try to send unsent message from
an account without an identity.

loop over all identities on sending unsent messages, or fix how we find the
unsent message folder

this might be related to why you're unable to send migrated unsent messages if
you migrating them from 4.x pop account.

Comment 1

[scottputterman]

p1 for investigation and if it's the cause of not migrating the unsent messages
folder.

Comment 2

[scottputterman]

reassigning to Cavin.

Comment 3

[Cavin Song]

I couldn't reproduce the "unable to send migrated unsent messages if you migrate 
them from 4.x pop account".  All msgs got sent fine. However, for each msg I got 
the following error msg:

  Couldn't open Sent Mail folder. Please verify that your Mail preference
  are correct.

and this is caused by that 4.x creates the following FCC: header for each of the 
unsent msg in the unsent message folder:

  FCC: /C|/Program Files/Netscape/Users/alldefaults_pop/mail/Sent

Since the destination sent folder is set we simply use it (which causes problems 
when we try to find it in rdf).

This is the only problem I see so far.

Comment 4

[Cavin Song]

Unsent msgs in migrated IMAP accounts has the same problem. In summary, the FCC: 
headers created by 4.x are the following:

 "/C|/Program Files/Netscape/Users/alldefaults_pop/mail/Sent" (for POP)
 "IMAP://nsmail-2"                                            (for IMAP)

In 6.x the valid folder urls are:

 "mailbox://[e-mail]/Sent" (for POP)
 "imap://[e-mail]/Sent"    (for IMAP)

One way to fix the problem is during migration where we know how to translate 
4.x folder urls to 6.x. But this is tedious because we need to parse the unsent 
msg folder and handle it specially.  Another way to do it is to make sure that 
the fcc folder url is valid during sending.

Comment 5

[Cavin Song]

Attachment: Fix copying unsent msgs to sent folder problem.

Comment 6

[Cavin Song]

The fix is to make sure the fcc folder uri is valid and exists. If not then the 
configured one is used.

Can I get a review from ducarroz and sspitzer? Thanks.

Comment 7

[(not reading, please use [e-mail] instead)]

Comment on attachment 66564 [details] [diff] [review]
Fix copying unsent msgs to sent folder problem.

the patch looks good.  I could have sworn this code already lived somewhere
else.
check with bhuvan and / or naving.  I think it has been written before,
but that might have been in js.  It seems like a common thing, not specific to
compose.

research that, and fix three minor style nits, and sr=sspitzer

here are my 3 nits:

waterson pointed out to me that this makes debugging hard, since you can't put
a break on the return.

>+  if (NS_FAILED(rv)) return PR_FALSE;

try this instead:

if (NS_FAILED(rv))
  return PR_FALSE;

>+  if (NS_FAILED(rv)) return PR_FALSE;

same thing.

a style suggestion:

>+  nsCOMPtr <nsIMsgFolder> thisFolder;
>+  thisFolder = do_QueryInterface(resource, &rv);
>+  if (!thisFolder || NS_FAILED(rv))
>+    return PR_FALSE;
>+
>+  // Parent doesn't exist means that this folder doesn't exist.
>+  nsCOMPtr<nsIMsgFolder> parentFolder;
>+  rv = thisFolder->GetParentMsgFolder(getter_AddRefs(parentFolder));
>+  if (NS_SUCCEEDED(rv) && parentFolder)
>+    return PR_TRUE;
>+  else
>+    return PR_FALSE;
>+}

Comment 8

[Jean-Francois Ducarroz]

Comment on attachment 66564 [details] [diff] [review]
Fix copying unsent msgs to sent folder problem.

The code looks good but I would rather see the function IsValidFolderURI in
nsMsgCompUtils.cpp or event better in mailnews/base/utils

Comment 9

[Navin Gupta]

you can just do GetParent() instead of doing GetParentMsgFolder() because
I don't see any uses of parentMsgFolder. 

Comment 10

[Cavin Song]

Attachment: Moved new routine to compose utils and incorporated comments.

Comment 11

[David :Bienvenu]

+  if (NS_SUCCEEDED(rv) && parentFolder)
+    return PR_TRUE;
+  else
+    return PR_FALSE;

personally, I think this should be "return NS_SUCCEEDED(rv) && parentFolder" -
that generates the least code.

I agree with JF that this would best be in base/utils, not comp utils, since I
imagine other code could use this.

Comment 12

[Cavin Song]

Attachment: Moved the routine to msgbsutl.

Comment 13

[Cavin Song]

Attachment: Moved the routine to msgbsutl.

Comment 14

[David :Bienvenu]

Comment on attachment 66927 [details] [diff] [review]
Moved the routine to msgbsutl.

sr=bienvenu,thx

Comment 15

[Jean-Francois Ducarroz]

Comment on attachment 66927 [details] [diff] [review]
Moved the routine to msgbsutl.

R=ducarroz

Comment 16

[Cavin Song]

Fix checked in.

Comment 17

[grylchan]

Using commercial trunk:
2002030103 on NT 4.0
2002030412 on Mac 9.2.2
20022208 linux 2.2 (had problems w/current builds migrating profile)

Verified that a migrated profile w/usent mesgs gets migrated over
in current commercial trunk builds. And once you login, you can
send your unsent mesgs w/no problems and the mesgs are actually sent.
Tried both imap and pop mail accounts.

Cavin, in addiion I notice this:
-After sending my unsent mesgs, it fails to copy it to my 'sent folder' 
  either on mail acount or local folders. But If I do send later
  from commercial trunk, it does copy it. I guess when we migrate 'unsent
  mesgs' over, it doesn't know how to copy itself to sent folder.
  New bug?
- If I go back to the old 4.x profile, my unsent mesgs are still present.
  I assume not a bug?

I'll wait for your response before marking verified.

Comment 18

[Cavin Song]

> After sending my unsent mesgs, it fails to copy it to my 'sent folder' 
> either on mail acount or local folders.
>
So did you see any error dialog indicating that the sent folder could not be
opened? Or the process just went silently? I'm curious to know how do you know
it failed to copy sent msgs to sent folder.

> But If I do send later from commercial trunk, it does copy it.
>
Does normal send copy msgs to sent folder?

> If I go back to the old 4.x profile, my unsent mesgs are still present.
> I assume not a bug?
>
This is the right behavior.

Comment 19

[grylchan]

>> After sending my unsent mesgs, it fails to copy it to my 'sent folder' 
>> either on mail acount or local folders.
>>
>So did you see any error dialog indicating that the sent folder could not be
>opened? Or the process just went silently? I'm curious to know how do you know
>it failed to copy sent msgs to sent folder.

It failed silently. After sending the Unsent mesgs, I checked the Sent
megs folder (either on mail or local folders acnts) and didn't see
a copy.

> But If I do send later from commercial trunk, it does copy it.
>
>Does normal send copy msgs to sent folder?

yes. I tried doing a regular send and doing a send later/send unsent
mesgs and it succesfully copied to the sent folder.

Comment 20

[Cavin Song]

Gary, after the account is migrated can you take a look at the migrated 6.x 
unsent message folder and see what the FCC: header is set to for each message?  
See Comment #3 of an example.

Comment 21

[grylchan]

Sorry for not getting back to you earlier Cavin.

To answer your question:
If 4.x profile has sent folders dir under local act:

for my imap
/c|/Program Files/Netscape/Users/glinzilla/mail/Sent

if 4.x profile has sent folders under the mail act
iamp://linzilla.mcom.com

But since all profiles are stored under windows aplication folder
and not like 4.x is that why it isn't working?

Comment 22

[grylchan]

Reopening Bug, Discussed with Cavin and he found the problem..

Comment 23

[Cavin Song]

I see the problem and it's caused by patch of bugscape 10163. A better fix to 
the problem is to set the right FCC url when it's initialized in 
InitCompositionFields(). This way we'll always have the correct url for the 
compose/send code to deal with.

Comment 24

[Cavin Song]

Attachment: Proposed patch for the regression.

Set the right FCC url at init time.

Comment 25

[Jean-Francois Ducarroz]

Comment on attachment 74263 [details] [diff] [review]
Proposed patch for the regression.

R=ducarroz

Comment 26

[David :Bienvenu]

Comment on attachment 74263 [details] [diff] [review]
Proposed patch for the regression.

sr=bienvenu

Comment 27

[Asa Dotzler [:asa]]

Comment on attachment 74263 [details] [diff] [review]
Proposed patch for the regression.

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 28

[Cavin Song]

Fix checked in on trunk.

Comment 29

[grylchan]

Using commercial trunk builds:
2002032008 linux 2.2 
2002-03-21-08-trunk mac 9.2.2 
2002-03-21-10-trunk win 2k

Verified that a migrated profile w/unsent mesgs gets migrated over
in current commercial trunk builds. And once you login, you can
send your unsent mesgs w/no problems and the mesgs are actually sent.
Tried both imap and pop mail accounts. Also tested sending a mesg
to a newsgroup.

Tried sending via file menu, going offline then online and sending
mesg at prompt.

Also copy of the unsent mesg is NOW copied to the sent folder.

Marking as verified for Sheela since she's out.