138018 - IMAP Shared UI: Should greyout/disable delete button, Personal Folders of Move Message menu, delete folder & compact folder for read privilege folders

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Karen Huang]

Used 04-15-14-1.0.0

Should greyout/disable delete button, delete folder & compact folder for 
msgs of read privilege folders

1) Login to an IMAP with Shared Read Privilege Folders account
2) Select the messages of those Read Privilege Folders on the thread pane.

Actual Results: Delete button, delete folder & compact folder are all enable, 
select delete & delete folder will get a "permission denied" alert. Select 
compact folder will no action.

Expected results: Should greyout/disable delete button, delete folder & compact 
folder from our client, so users won't see those alerts from the IMAP server.

Comment 1

[David :Bienvenu]

accepting for moz 1.1 (I know you're going to nominate this for nsbeta1, but
it's not going to happen :-) )

Comment 2

[Karen Huang]

OK :-)

Comment 3

[Karen Huang]

This should apply to all the buttons/items of tool bar, menu, context 
menu...etc.

Comment 4

[Karen Huang]

Adding "personal folders" of "Move Message" menu should be disable too, it 
includes:
Inbox, Sent, Trash, Draft, any this account personal created folders.

Comment 5

[Karen Huang]

I am raising this bug now -- perhaps to fix this bug earlier since if user are 
trying to MOVE INTO other user's folder, the menu is disable. 
But if users are trying to MOVE OUT messages from other user's folder to their 
personal folders, the "permission denied" displays but after I checked the 
destination folder: I saw the messages did DISPLAY on the destination folder 
which is bad. It has been treat as COPY behavior already. It didn't move out but 
did COPY IN destination folder! :-(

David, please let me know if I need to log a seperate bug for "MOVE has been 
treat as COPY into to other personal folder". So, if you can disable MOVE first 
from the other bug and leave this bug other scenarios to fix later......
Let me know whether I need to log another bug or not??

Comment 6

[David :Bienvenu]

a move is just a copy followed by a delete, and the delete fails. I don't see
any need for a new bug, and I don't really see that it's a big deal...there's no
data loss or anything.

Comment 7

[David :Bienvenu]

fix coming up.

Comment 8

[David :Bienvenu]

Attachment: proposed fix

disable the delete commands for folders we can't delete from, and move menu for
those same folders (and news folders).

Comment 9

[David :Bienvenu]

Navin, can I get a review? thx.

Comment 10

[Navin Gupta]

We could optimize this by checking for imap folder and just doing it for imap 
folder, right ?

Comment 11

[David :Bienvenu]

not sure what you mean, but it's a valid check for any kind of folder (e.g., a
read-only local folder), so I'm thinking no.

Comment 12

[Navin Gupta]

I thought we will never have readonly local folder. So why add the attribute to
nsIMsgFolder, move it up to nsIMsgImapMailFolder.

Comment 13

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

sr=sspitzer, once you addresses these issue:

review here for naving's benefit:

1) 

please override CanDeleteMessages() for news.  david says everything works 
without it, since the UI is hidden or disabled for news, but we should still do 
it, because someone might expect .canDeleteMessages to do the right thing.

2)

+      var enableMenuItem = aMessage && !isNews && msgFolder && 
msgFolder.canDeleteMessages;

even though !isNews will not be needed, bienvenu is right, it's faster this 
way, so we should leave it.

3)

I asked him, and david said cancelling news articles still works with these 
changes.

4)

return gCurrentMessageUri != null && loadedFolder && 
loadedFolder.canDeleteMessages;

can be:

return gCurrentMessageUri && loadedFolder && loadedFolder.canDeleteMessages;

Comment 14

[David :Bienvenu]

no, because we're doing it for news too. Please see new patch.

Comment 15

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

another reason, besides the !isNews issue:

putting it in nsIMsgFolder simplifies our JS (no need to QI to nsIMsgImapFolder 
and make more of our js know about imap).

Comment 16

[David :Bienvenu]

Attachment: fix with Seth's comments incorporated

Incorporate Seth's comments, and remove isNews check since it just makes the
code messier. All these CanXXX attributes are polymorphic, and should be.
Checking for isImap, isNews, etc, all over the code is ugly. Having folder
capabilities is much cleaner.

Comment 17

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

Comment on attachment 84657 [details] [diff] [review]
fix with Seth's comments incorporated

sr=sspitzer

Comment 18

[Navin Gupta]

Comment on attachment 84657 [details] [diff] [review]
fix with Seth's comments incorporated

I am not seeing any !isNews code removed but anyway r=naving

Comment 19

[David :Bienvenu]

thanks, the isNews check was added in the first patch - it doesn't show up in
the second patch, it was never checked in.

Comment 20

[David :Bienvenu]

fix checked in.

Comment 21

[[e-mail]]

Comment on attachment 84657 [details] [diff] [review]
fix with Seth's comments incorporated

Nitpick: why did mailContextMenus.js not use GetLoadedMsgFolder() as per
messageWindow.js?

Comment 22

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

this caused a regression in the context menu in the stand alone msg window.

see 

the problem that mailContextMenu uses GetSelectedFolderResource(), which 
although defined in commandglue.js, only works for 3 pane, which has a folder 
tree.

/mailnews/base/resources/content/mailContextMenus.js, line 160 -- var 
folderResource = GetSelectedFolderResource();

Comment 23

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

> Nitpick: why did mailContextMenus.js not use GetLoadedMsgFolder() as per
> messageWindow.js?

that would have prevented the regression, sorry I didn't catch it during 
reviews.

bug #148078 has the fix that switches to use GetLoadedMsgFolder()

Comment 24

[Karen Huang]

Sorry for the late response since all QA currently are focusing on branch builds 
verification unless developers request for verifying on trunk bugs in order to 
checkin to branch.
I did verify this bug on 05-31-04 trunk & today's 06-05-04-trunk that Delete 
button is still enable for IMAP Shared Read Privilege Folder. I don't know why 
Seth mentioned that this bug caused regression. 
I am reopening this bug right now.

Comment 25

[David :Bienvenu]

Karen, I don't suppose you ever tried this when it was fixed? It would be useful
to know when it was broken again.

Comment 26

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

>I don't know why Seth mentioned that this bug caused regression. 

the fix for this bug caused another bug.  see 
http://bugzilla.mozilla.org/show_bug.cgi?id=148078

Comment 27

[David :Bienvenu]

ah, I forgot to checkin one part of this fix. Unfortunately, now the trunk is
closed to checkins for 1.1, so this may not get fixed for a while unless I can
get permission to checkin the rest of my fix.

Comment 28

[Karen Huang]

Yes! David and Seth are right.
This bug did fix (disable) for Move menu item which is part of this bug (See 
#c5).....I am glad that David did fix part of this problem!

Comment 29

[Karen Huang]

And actually, from Comment #5 -- David did fix the most important part of this 
bug.....

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 84657 [details] [diff] [review]
fix with Seth's comments incorporated

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

Comment 31

[David :Bienvenu]

rest of fix checked in.

Comment 32

[Karen Huang]

Used 06-10-08-1.0.0 build

I checked this bug fix on trunk -- it is working for delete button & move menu 
item 
(Except "disable delete folder/compact folder" which can been addressed on 
bug 150743)
I will leave this bug "as is" until we will have fix on branch and verify on 
branch.

Comment 33

[Karen Huang]

Oops! Typo! Above verification is on 06-10-08-trunk build, not 1.0.0 branch 
build.

Comment 34

[esther]

qa contact change