160586 - IMAP folder moved to ~/<folder> instead of ~/mail/<folder> when moving from sub-folder

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Ehren Wilson]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.1b) Gecko/20020721
BuildID:    2002072104

Using both Mozilla 1.0 and 1.1b I tried dragging an IMAP mailbox from a
subdirectory to the root mail directory.  Actual mailbox file is moved from
~/mail/<subdir>/<mailbox> to ~/<mailbox>.  Should have moved to
~/mail/<mailbox>.  .mailboxlist lists file as simply <mailbox> instread of
~/mail/<mailbox> as well.

Reproducible: Always
Steps to Reproduce:
1. set IMAP server directory to "~/mail/"
2. drag a subscribed mailbox into root directory
3. get Alert "The current command did not succeed. The mail server responded:
SELECT failed: Can't open mailbox ~/mail/<folder>: no such mailbox. 

Actual Results:  File is moved into ~/<mailbox> instead of ~/mail/<mailbox>. 
.mailboxlist has entry for <mailbox> not for ~/mail/<mailbox>.  Used shell
access to move back and correct entries in .mailboxlist

Expected Results:  should have moved file to ~/mail/<mailbox> and listed
~/mail/<mailbox> in .mailboxlist

not sure if this affects any other imap servers than UW imap.  Also the
behaviour is fine when moving between sub-directories, only occurs when moving
to root folder

Comment 1

[jonathan.courtney]

I can reproduce this bug using 1.3a on Mac OS X 10.2.2 using Sun's IMAP server.

Comment 2

[David :Bienvenu]

confirming. cc'ing Henry Jia.

Comment 3

[philip zhao]

I will look into this one soon.

Comment 4

[philip zhao]

Attachment: patch for test and review

In nsImapUrl::CreateServerDestinationFolderPathString, mozilla should always
use AllocateServerPath() but should not set requested
DestinationFolderPathString to "" directly when we move a folder to root.

And in nsImapUrl::AddOnlineDirIfNeccessary, we should also give a check if the
onlineMailboxName is "".

Comment 5

[philip zhao]

Attachment: a cleaned patch for review

Pls review. Thanks. :)

Comment 6

[David :Bienvenu]

instead of +                           if ( !PL_strcmp(onlineMailboxName,"") )
can't you just use *onlineMailboxName?

it's less code, and faster.

Comment 7

[philip zhao]

Attachment: patch for review

Thanks David,I change it.Please r=? if it's all right.Thanks.

Comment 8

[David :Bienvenu]

Comment on attachment 114063 [details] [diff] [review]
patch for review

r=bienvenu

Comment 9

[Henry Jia]

It should be !*onlineMailboxName.

Comment 10

[philip zhao]

Attachment: correct patch for review

My falt. Sorry,David&Henry. 
This is the correct patch. It runs.
Pls r=?&sr=?

Comment 11

[Henry Jia]

Comment on attachment 114072 [details] [diff] [review]
correct patch for review

Carry David's r= here. It's just a very small change between the two patches.

sr=Henry

Require for approval 1.3. This is a small patch.

Comment 12

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

assuming I'm understanding the code (and your patch), instead of:

  if ( onlineDirWithDelimiter.Last() != delimiter )
    onlineDirWithDelimiter += delimiter;
  if ( !*onlineMailboxName )
    onlineDirWithDelimiter.SetLength(onlineDirWithDelimiter.Length()-1);

I think this would be better:

  if ( onlineDirWithDelimiter.Last() != delimiter && *onlineMailboxName)
    onlineDirWithDelimiter += delimiter;

Why add the delimiter and then remove it, right?

re-assign to [e-mail]

Comment 13

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

philip, let me know if you need help testing on another server (other than UW IMAP).

we'd want to verify that this code does the right thing on non-UW IMAP servers.

Comment 14

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

Comment on attachment 114072 [details] [diff] [review]
correct patch for review

clearing approval request until I hear back from philip or henry about the
change I suggested and if they need help testing on non-uw imap servers.

Comment 15

[philip zhao]

Attachment: patch with sspitzer's suggestion

You're right,sspitzer. I changed it. Please see if it's well then. And I tested
this patch on iPlanet mail server , it works well. Please give me your Cyrus
server account and Courier server account, I will test them soon then. Thx. :)

Comment 16

[Henry Jia]

But onlineDirWithDelimiter may already have the delimiter before the code goes
to if ( onlineDirWithDelimiter.Last() != delimiter ), i.e. it's 'INBOX.'. We
still need to remove the delimiter if onlineMailboxName is "". Right?

Comment 17

[philip zhao]

Yes,henry. I test again. If the IMAP directory is set "INBOX." like, the drag of
a subfolder to root will still error with attachment 114292 [details] [diff] [review]. Then we will turn
to attachment 114072 [details] [diff] [review]. And look for sspitzer's approval? Thanks sspitzer.

Comment 18

[philip zhao]

Comment on attachment 114072 [details] [diff] [review]
correct patch for review

=> not obsolete.

Comment 19

[philip zhao]

Comment on attachment 114292 [details] [diff] [review]
patch with sspitzer's suggestion

=> obsolete

Comment 20

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

I've got access to UW, Cyrus and Netscape Messaging Server 4.15
I'll try out your patch tomorrow, and let you know.

Comment 21

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

Comment on attachment 114072 [details] [diff] [review]
correct patch for review

sr=sspitzer

stepping through the code, if (!m_destinationCanonicalFolderPathSubString),
AllocateServerPath() will still do the right thing.

I tested on UW, Cyrus, and Netscape Messaging.

my only nit would be to fix
nsImapUrl::CreateServerDestinationFolderPathString()

and remove 

nsresult rv = NS_OK;

and do:

nsresult rv = 
AllocateServerPath(...);

Comment 22

[philip zhao]

Thanks sspitzer! 

While, still wait for an approval. :)

Comment 23

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

Attachment: patch, with minor nit cleanup

Comment 24

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

Comment on attachment 115322 [details] [diff] [review]
patch, with minor nit cleanup

r/sr=henry.jia,sspitzer

Comment 25

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

fixed on the trunk.  not fixed on the 1.3 branch yet.

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 115322 [details] [diff] [review]
patch, with minor nit cleanup

a=asa (on behalf of drivers) for checkins to 1.3

Comment 27

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

before I land on 1.3, I'll wait for this to be verified.

moving to 1.4 alpha, so there is no confusion as to when it was fixed.

if I land in 1.3 final, I'll change the TM back to 1.3 final.

Comment 28

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

has approval for 1.3 final, but I'll wait until the bug is verified.

Comment 29

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

I back ported this fix to the 1.3 branch.