135993 - MDN: If sent folder changes in mid stream, Receipts still get sent to the old sent folder until quit/restart

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[grylchan]

Using 2002040503 commercial trunk on NT 4.0
I assume the behavior is across all OS's but
just tested under Windows.

Seth asked what happend if the sent folder changed in midstream
does MDN also adjust? the answer is no.

You have to quit/restart, in order for the Return Receipt to
be filtered to the correct folder. The copy of the original
mesg adjusts dynamically and is filtered to the correct sent folder
when the pref in Copies&Folders is changed but the same can't
be said for mdn.

Can't test local folders due to bug 135987.

Steps to reproduce:
1.create a imap/pop account
2.Make sure sent folder is under your imap/pop acnt
  Use either pref: 'Sent on <accnt>' or 'Other: sent on <acnt>'
3.Set either Global or Customized MDN:
   Check the box 'When sending mesgs, always request a Return Receipt'
   When Receipt arrives: radio button Move it to my Sent Folder
4.Compose/send mesg
5.Send a Return Receipt back
6.Notice RR stored in your account's Sent folder
7.Change the Location of sent folder to some other folder
  (don't use local folders)
8.Compose/send a mesg
9.Notice origial copy of mesg adjusts and stores it the new
  sent folder
10.Get mesg and send a RR back
result: RR still gets stored in the old sent folder
expected: RR to get stored in new sent folder

If you quit/restart, then it works as expected
                   
Below is my test situation:

imap1 - old sent folder was imap sent folder
       changed the new sent folder to be my pop sent folder

pop - old sent folder was the pop sent folder
      changed the new sent folder to be a folder called 'foo' located
      in my pop acnt
imap2 -old sent folder was imap sent folder
       changed the new sent folder to be another imap folder under this acnt
       (Place a copy in other : test on imap2)

result
Copy of orig mesgs stored in the correct sent folder

imap1 - RR still received in the Imap sent folder
pop - RR still received in the Pop sent folder
imap2 - RR still received in the Imap sent folder

Quit/Restart

result
imap1 - RR now received in inbox (i think because imap doesn't work under
       Local folders, see bug 135987, and a pop account in reality
       is local folder)
pop - RR Now received in my folder called foo
imap2 - RR Now received in my folder called test under my acnt

Comment 1

[Jeff Tsai]

Attachment: Patch ready for review

Upon changing Sent folder pref we simply remove the temporary return receipt
filter. Newer temporary return receipt filter will then be regenerated the next
time when receiving new messages.

Comment 2

[Jeff Tsai]

Attachment: Patch addresses both 135993 & 135987; please review

When changing Sent folder pref we have to clear the identity's server mdn
filter not the previous Sent folder's hosting server.

Comment 3

[David :Bienvenu]

"if Sent folder pref ever get changed" - this should read "if Sent folder pref
is changed"

could you put these statements on a single line:
+            {
+              nsCOMPtr<nsISupports> aSup =
+                getter_AddRefs(retval->ElementAt(0));
+              nsCOMPtr<nsIMsgIncomingServer> server =
+                do_QueryInterface(aSup,&rv);
like so: 
nsCOMPtr<nsISupports> aSup = getter_AddRefs(retval->ElementAt(0));
nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(aSup,&rv);
It's much more readable.
Actually, better yet, I believe you can do this in one line with QueryElementAt.

You don't need to use NS_NAMED_LITERAL_STRING if you're only going to reference
the string once - just use NS_LITERAL_STRING in place.

+    NS_NAMED_LITERAL_STRING(internalReturnReceiptFilterName, 
+                            "mozilla-temporary-internal-MDN-receipt-filter");

Comment 4

[Jeff Tsai]

Attachment: New patch addresses David's comment

It replaces the previous one. Fixes both 135993 & 135987.

Comment 5

[David :Bienvenu]

Comment on attachment 78440 [details] [diff] [review]
New patch addresses David's comment

r=bienvenu

Comment 6

[Jeff Tsai]

JF or Seth, can you sr this one? Thanks,

Comment 7

[Jeff Tsai]

The fix also fixes bug 135987.

Comment 8

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

some comments:

1)
     
+        // setting up new fcc_folder; time to clear the mdn filter
+        nsCOMPtr<nsIMsgAccountManager> accountManager = 
+          do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv);
+        if (NS_SUCCEEDED(rv))
+        {
+          nsCOMPtr<nsISupportsArray> retval; 
+          rv = accountManager->GetServersForIdentity(this,
+                                                     getter_AddRefs(retval));
+          if (NS_SUCCEEDED(rv))
+          {
+            PRUint32 cnt = 0;
+            retval->Count(&cnt);
+            if (cnt > 0)
+            {
+              nsCOMPtr<nsISupports> aSup = getter_AddRefs(retval->ElementAt(0));
+              nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(aSup,&rv);
+              if (NS_SUCCEEDED(rv))
+                server->ClearTemporaryReturnReceiptsFilter();
+            }
+          }
+        }

Some style comments:

a) instead of nesting deeper and deeper with if (NS_SUCCEEDED(rv)), bail out on
unexpected errors.
It makes the code easier to read and follow.

for example, if this fails, we are in a world of hurt and it would be worth an
assertion:

+        nsCOMPtr<nsIMsgAccountManager> accountManager = 
+          do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv);


+        // setting up new fcc_folder; time to clear the mdn filter
+        nsCOMPtr<nsIMsgAccountManager> accountManager = 
+          do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv);
+        NS_ENSURE_SUCCESS(rv,rv);

assuming that returning at this point is acceptable.  In my opinion, if we fail
to get the account manager, something very bad, so returning is acceptable.

b)

+          nsCOMPtr<nsISupportsArray> retval; 
+          rv = accountManager->GetServersForIdentity(this,
+                                                     getter_AddRefs(retval));

retval?  How about:

nsCOMPtr<nsISupportsArray> servers;

c)


+              nsCOMPtr<nsISupports> aSup = getter_AddRefs(retval->ElementAt(0));

aFoo is the notation for arguments.  since this is a local comptr, do "supports"
or something instead.

+              nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(aSup,&rv);
+              if (NS_SUCCEEDED(rv))

d)

+                server->ClearTemporaryReturnReceiptsFilter();

do we care about rv from this call?

2)

+NS_IMETHODIMP
+nsMsgIncomingServer::ClearTemporaryReturnReceiptsFilter()
+{
+  nsresult rv; // remove mdn filter iff it exists
+  if (mFilterList)
+  {
+    nsCOMPtr<nsIMsgFilter> mdnFilter;
+    rv = mFilterList->GetFilterNamed(
+        NS_LITERAL_STRING("mozilla-temporary-internal-MDN-receipt-filter").get(),
+        getter_AddRefs(mdnFilter));
+    if (mdnFilter)
+      rv = mFilterList->RemoveFilter(mdnFilter);
+  }
+  return NS_OK;
+}

a) your rv can be declared inside the "if (mFilterListBlock)"

b) what about the rv from the GetFilterNamed() call and the RemoveFilter() call?

something like:

+NS_IMETHODIMP
+nsMsgIncomingServer::ClearTemporaryReturnReceiptsFilter()
+{
+  // remove mdn filter iff it exists
+  if (mFilterList)
+  {
+    nsCOMPtr<nsIMsgFilter> mdnFilter;
+    nsresult rv = mFilterList->GetFilterNamed(
+        NS_LITERAL_STRING("mozilla-temporary-internal-MDN-receipt-filter").get(),
+        getter_AddRefs(mdnFilter));
+    if (NS_SUCCEEDED(rv) && mdnFilter)
+      return mFilterList->RemoveFilter(mdnFilter);
+  }
+  return NS_OK;
+} 

Comment 9

[Jeff Tsai]

Attachment: Patch for final review

Patch addresses Seth's comments.

Comment 10

[Jeff Tsai]

Adding nsbeta1+ and [adt2] since this fix bug 135987 which has nsbeta1 and
[adt2] status.

Comment 11

[Cavin Song]

Reassigning.

Comment 12

[Cavin Song]

Attachment: Made Jeff's patch work on the trunk.

Tested a few scenarios too.

Comment 13

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

Comment on attachment 114143 [details] [diff] [review]
Made Jeff's patch work on the trunk.

thanks for re-testing jefft's patch.

instead of

+    if (actionTargetFolderUri.Length() > 0)

do something like

if (!actionTargetFolderUri.IsEmpty()) 

no need for a new patch.

Comment 14

[Cavin Song]

Fix checked in.

Comment 15

[Boris Zbarsky [:bzbarsky]]

For future reference,

nsCOMPtr<nsISupports> supports = getter_AddRefs(servers->ElementAt(0));
nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(supports,&rv);

is less leak-prone as:

nsCOMPtr<nsIMsgIncomingServer> server = do_QueryElementAt(servers, 0, &rv);

(At one point I went through mailnews and eliminated most uses of ElementAt in
favor of do_QueryElementAt, fixing a few leaks in the process, since as I said
ElementAt is leak-prone; it'd be good not to reintroduce the more leak-prone usage.)

Comment 16

[Ninoschka Baca]

Trunk build 2003-02-25: Mac 10.1.5
Trunk build 2003-03-03: WinXP
Verified Fixed. Changed it to another folder on the account, then als changed to
a local folder.