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.
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
This should apply to all the buttons/items of tool bar, menu, context menu...etc.
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
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
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.
fix coming up.
Severity: major → normal
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).
Navin, can I get a review? thx.
We could optimize this by checking for imap folder and just doing it for imap folder, right ?
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.
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;
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).
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 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+
thanks, the isNews check was added in the first patch - it doesn't show up in the second patch, it was never checked in.
fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
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]
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 → ---
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
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.
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!
And actually, from Comment #5 -- David did fix the most important part of this bug.....
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+
rest of fix checked in.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago → 17 years ago
Resolution: --- → FIXED
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.
Oops! Typo! Above verification is on 06-10-08-trunk build, not 1.0.0 branch build.
qa contact change
QA Contact: huang → esther
You need to log in before you can comment on or make changes to this bug.