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

RESOLVED FIXED in mozilla1.1alpha

Status

RESOLVED FIXED
17 years ago
14 years ago

People

(Reporter: huang, Assigned: Bienvenu)

Tracking

Trunk
mozilla1.1alpha

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [don't check this into the branch with out the fix in bug #148078])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
accepting for moz 1.1 (I know you're going to nominate this for nsbeta1, but
it's not going to happen :-) )
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1alpha
(Reporter)

Comment 2

17 years ago
OK :-)
(Reporter)

Updated

17 years ago
QA Contact: olgam → huang
(Reporter)

Comment 3

17 years ago
This should apply to all the buttons/items of tool bar, menu, context 
menu...etc.
(Reporter)

Comment 4

17 years ago
Adding "personal folders" of "Move Message" menu should be disable too, it 
includes:
Inbox, Sent, Trash, Draft, any this account personal created folders.
OS: Windows 2000 → All
Hardware: PC → All
Summary: IMAP Shared UI: Should greyout/disable delete button, delete folder & compact folder for msgs of read privilege folders → IMAP Shared UI: Should greyout/disable delete button, Personal Folders of Move Message menu, delete folder & compact folder for read privilege folders
(Reporter)

Comment 5

17 years ago
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??
Severity: normal → major
(Assignee)

Comment 6

17 years ago
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.
(Reporter)

Updated

17 years ago
Blocks: 112096
(Assignee)

Comment 7

17 years ago
fix coming up.
Severity: major → normal
(Assignee)

Comment 8

17 years ago
Created attachment 84654 [details] [diff] [review]
proposed fix

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

Comment 9

17 years ago
Navin, can I get a review? thx.

Comment 10

17 years ago
We could optimize this by checking for imap folder and just doing it for imap 
folder, right ?
(Assignee)

Comment 11

17 years ago
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

17 years ago
I thought we will never have readonly local folder. So why add the attribute to
nsIMsgFolder, move it up to nsIMsgImapMailFolder.
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;
(Assignee)

Comment 14

17 years ago
no, because we're doing it for news too. Please see new patch.
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).
(Assignee)

Comment 16

17 years ago
Created attachment 84657 [details] [diff] [review]
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.
Attachment #84654 - Attachment is obsolete: true
Comment on attachment 84657 [details] [diff] [review]
fix with Seth's comments incorporated

sr=sspitzer
Attachment #84657 - Flags: superreview+

Comment 18

17 years ago
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
Attachment #84657 - Flags: review+
(Assignee)

Comment 19

17 years ago
thanks, the isNews check was added in the first patch - it doesn't show up in
the second patch, it was never checked in.
(Assignee)

Comment 20

17 years ago
fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 21

17 years ago
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?
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();
Whiteboard: [don't check this into the branch with out the fix in bug #]
> 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()
Whiteboard: [don't check this into the branch with out the fix in bug #] → [don't check this into the branch with out the fix in bug #148078]
(Reporter)

Comment 24

17 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

17 years ago
Karen, I don't suppose you ever tried this when it was fixed? It would be useful
to know when it was broken again.
>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
(Assignee)

Comment 27

17 years ago
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.
(Reporter)

Comment 28

17 years ago
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!
(Reporter)

Comment 29

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

Comment 30

17 years ago
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
Attachment #84657 - Flags: approval+
(Assignee)

Comment 31

17 years ago
rest of fix checked in.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
(Reporter)

Comment 32

17 years ago
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.
(Reporter)

Comment 33

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

Comment 34

16 years ago
qa contact change
QA Contact: huang → esther
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.