Closed Bug 120485 Opened 23 years ago Closed 22 years ago

Can't delete \NoSelect folders on IMAP server cascadely

Categories

(MailNews Core :: Networking: IMAP, defect)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.0.1

People

(Reporter: Henry.Jia, Assigned: Henry.Jia)

Details

Attachments

(2 files, 3 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.4) Gecko/20011207
BuildID:    2002011510

Mozilla can't delete folders on IMAP server cascadely. From the communication
and the code, I'm sure that mozilla didn't delete these folders successfully.
Test Enviroment: mozilla nightly build 2002011510, mozilla 0.9.7 on solaris & win2k

Reproducible: Always
Steps to Reproduce:
1. Clear the default settings 'Edit'->'Mail & Newsgroup Account
Settings...'->'Server Settings'->'Advanced...'->'Server supports folders that
contain sub-folders and messages' for IMAP server that supports folders that can
contain either sub-folders or messages
2. Create folders that can only contain sub-folders cascadely
3. Delete the toppest parent folder
4. Mozilla shows it's OK. But in fact, when you refresh the folder list of the
mail window, you'll see the deleted folders.

Actual Results:  The folders can't be deleted cascadely.

Expected Results:  The folders should be deleted cascadely. And from the code,
mozilla tries to do this.
mscott, I'm trying to solve this bug, can this bug assigned to me.
reassigning.
Assignee: mscott → Henry.Jia
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Attached patch suggested patch (obsolete) — Splinter Review
I test this patch on solaris & win2k professional. I test IMAP 4 and IMAP 4 Rev
1 servers(including UW server).
Attached patch new suggested fix (obsolete) — Splinter Review
I made a stupid mistake last time. Now, I give the correct patch.
mscott@netcape.com:  Would you review the patch for Henry please?  Thanks.
I'm sorry I haven't done this yet. I started Friday and then accidentally killed
the window I was typing in =). I'll finish this on Monday for sure. 
Comment on attachment 68508 [details] [diff] [review]
new suggested fix

Thanks for the patch Henry. A couple comments, most of them are very minor
nits.

1) we tend to not use the notation where you put a 'b' before booleans, a 'p'
before pointers, etc. In the case of method arguments like bDeleteMyself we do
encourage using an 'a' for argument.

i.e.
DeleteSubFolders(const char * aMailboxName, aDeleteSelf);

instead of using myself, how about self: aDeleteSelf. myself makes it sound
like we are deleting a person =). 

2) small typo in your comment for DeleteSubFolders on: "dicide". the last part
of the comment should be "and provide feedback...."

3) Can you move the comment for DeleteSubFolders above the method instead of
below it?

4) instead of using PL_strlen, people are trying to convert existing code to
just strlen. This is a recent change you wouldn't know about with out me
telling you. For more details: Bug #124536

5) I'd get rid of variables like iLen and pSelectedMailboxDir and
bMyselfInTheSubFolderList and just rename them to things like length,
selectedMailboxDir, etc.

6) PL_strcpy can now be just strcpy

7) I'd express equalties with zero as:
!PL_strcmp(currentName, selectedMailbox)

8) instead of bMyselfInTheSubfolderList
how about folderInSubfolderList
instead of bMyselfDelted how about folderDeleted

9) Instead of using PL_strcmp, use nsCRT::strcmp

Thanks for the patch!
In addition to Scott's comments, I have some comments as well:

+    PR_FREEIF(pSelectedMailboxDir);

This can just be PR_Free(pSelectedMailboxDir) since PR_Free checks for null - 
PR_FREEIF checks for null, and also sets pSelectedMailboxDir to null, which I
don't think you need, so it's just extra code. Or, you could use an
nsCAutoString, which will get cleaned up automatically.

Also, the variable named "b" - that's not a useful name for a variable. In fact,
can't you just pass in PR_FALSE directly and get rid of the variable completely?
according to the bug 124536
sccott's comments 9 may changed to 

9) Instead of using PL_strcmp, use strcmp directly.

Thanks, mscott, bienvenu, margaret and jerry. I've revised the patch according
to your suggestions.
Bienvenu, since the argument 'aDeleteSelf' in the function DeleteSubFolders is
not a general parameter, it's a reference parameter and wants to return
something, so we can't pass PR_FALSE directly to the function.
Keywords: patch, review
Comment on attachment 71624 [details] [diff] [review]
the revised patch

one small nit, should we check for an allocation failure:
selectedMailboxDir = (char *)PR_MALLOC(length+2);

r=mscott

You still need to get an sr= on this. I'd suggest bienvenu.
Attachment #71624 - Flags: review+
Comment on attachment 71624 [details] [diff] [review]
the revised patch

sr=bienvenu
Attachment #71624 - Flags: superreview+
Should I give a new patch according to mscott's comment 2002-03-05 or the patch 
that has passed r= and sr= will be checked in?
Another thing is that I want to obsolete the patch(attachment 67368 [details] [diff] [review]) and the 
patch(attachment 68508 [details] [diff] [review]), but I don't have the right. Anyone can obsolete them 
to make it clear? How can I get this right? Thanks.
Attachment #67368 - Attachment is obsolete: true
Attachment #68508 - Attachment is obsolete: true
just add a check for a failed memory allocation. No need to post a new patch.
Your good to go. 

Looks like someone already obsoleted those 2 patches for you. 
Unfortunately, Henry does not have checkin privileges yet.  In order for someone
to check in for him, I think it is better for him to add a new patch with the
check.  With that, I guess he will get automatic approval based on
mscott@netscape.com's comment.  It would be nice if someone can check it in for
him after he has provided the patch.  I am afraid that I may not have the time
to do so until next week.  Thanks, Margaret
Attached patch the patch for checkin (obsolete) — Splinter Review
According to Margaret & mscott's comments, I give this new patch. Just add a
malloc check. Can anyone reviewed it and checked in? Thanks very much.
Make the check more strict. Compare to attachment #71624 [details] [diff] [review], two checks are added.

1. use
	if( selectedMailboxDir )    // only do the intelligent test if there is
enough memory
	{
	    strcpy(selectedMailboxDir, selectedMailbox);
	    selectedMailboxDir[length] = onlineDirSeparator;
	    selectedMailboxDir[length+1] = '\0';
	    PRInt32 i;
	    for( i=0; i<numberToDelete && !folderInSubfolderList; i++ )
	    {
		char *currentName = (char *) m_deletableChildren->ElementAt(i);

		if( !strcmp(currentName, selectedMailbox) ||
!strcmp(currentName, selectedMailboxDir) )
		    folderInSubfolderList = PR_TRUE;
	    }
	}
instead of
	    strcpy(selectedMailboxDir, selectedMailbox);
	    selectedMailboxDir[length] = onlineDirSeparator;
	    selectedMailboxDir[length+1] = '\0';
	    PRInt32 i;
	    for( i=0; i<numberToDelete && !folderInSubfolderList; i++ )
	    {
		char *currentName = (char *) m_deletableChildren->ElementAt(i);

		if( !strcmp(currentName, selectedMailbox) ||
!strcmp(currentName, selectedMailboxDir) )
		    folderInSubfolderList = PR_TRUE;
	    }

2. use 
if( selectedMailboxDir && !strcmp(selectedMailboxDir, longestName) )	// just
myself
instead of 
if( !strcmp(selectedMailboxDir, longestName) )	// just myself

This should be final patch. Checkin wanted.
Attachment #75908 - Attachment is obsolete: true
Comment on attachment 77400 [details] [diff] [review]
the patch for checkin

r=bienvenu on last patch
Attachment #77400 - Flags: review+
Nominating (but I don't know if this will be accepted - it's a bit late)
Keywords: nsbeta1
Not too late, the patch is still fresh. The related files on trunk haven't been
changed these days.
Comment on attachment 77400 [details] [diff] [review]
the patch for checkin

I thought I already reviewed this back in February. It sure looks familiar.
Sorry if you were waiting long.

sr=mscott
Attachment #77400 - Flags: superreview+
Thanks, mscott and bienvenu.

a= and checkin wanted
Keywords: approval
Fix checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment on attachment 77400 [details] [diff] [review]
the patch for checkin

a=shaver for 1.0 branch.
Attachment #77400 - Flags: approval+
Target Milestone: --- → mozilla1.0.1
Adding keyword mozilla1.0.1
Keywords: mozilla1.0.1
please checkin to the 1.0.1 branch ASAP. once there, remove the mozilla1.0.1+
keyword and add the fixed1.0.1 keyword.
checked in to 1.0 branch.
if this is really in the 1.0 branch we should change the mozilla1.0.1+ keyword
into  a fixed1.0.0 keyword so QA will know to verify this on the branch. 
Verified on all the platforms that we can delete \NoSelect folders cascadely:
--------------------------------------------------------------------------------
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 53 list "" "28_test1/*"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST 
(\NoSelect) "/" 28_test1/
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST 
(\NoSelect) "/" 28_test1/28_test2
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST 
(\NoSelect) "/" 28_test1/28_test2/28_test3
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 53 OK LIST 
completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 54 list "" 
"28_test1/28_test2/28_test3/*"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST 
(\NoSelect) "/" 28_test1/28_test2/28_test3/
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 54 OK LIST 
completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 55 delete 
"28_test1/28_test2/28_test3/"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 55 OK DELETE 
completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 56 unsubscribe 
"28_test1/28_test2/28_test3/"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 56 OK 
UNSUBSCRIBE completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 57 list "" 
"28_test1/28_test2/*"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: * LIST 
(\NoSelect) "/" 28_test1/28_test2/
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 57 OK LIST 
completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 58 delete "28_test1/28_test2/"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 58 OK DELETE 
completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 59 unsubscribe 
"28_test1/28_test2/"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 59 OK 
UNSUBSCRIBE completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 60 delete "28_test1/"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 60 OK DELETE 
completed
1292[26e7cd0]: valjean.nscp.aoltw.net:A:SendData: 61 unsubscribe "28_test1/"
1292[26e7cd0]: valjean.nscp.aoltw.net:A:CreateNewLineFromSocket: 61 OK 
UNSUBSCRIBE completed
--------------------------------------------------------------------------------
Verified on Windows 2000 06-27-08-1.0 branch build
Verified on Linux 06-27-06-1.0 branch build
Verified on Mac 06-27-05-1.0 branch build

Changing keywords from "fixed1.0.0" to "verified1.0.1".
Summary: Can't delete folders on IMAP server cascadely → Can't delete \NoSelect folders on IMAP server cascadely
Verified.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: