Closed
Bug 138018
Opened 22 years ago
Closed 22 years ago
IMAP Shared UI: Should greyout/disable delete button, Personal Folders of Move Message menu, delete folder & compact folder for read privilege folders
Categories
(SeaMonkey :: MailNews: Message Display, defect)
SeaMonkey
MailNews: Message Display
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.1alpha
People
(Reporter: huang, Assigned: Bienvenu)
References
Details
(Whiteboard: [don't check this into the branch with out the fix in bug #148078])
Attachments
(1 file, 1 obsolete file)
9.54 KB,
patch
|
naving
:
review+
sspitzer
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
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•22 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•22 years ago
|
||
OK :-)
Reporter | ||
Updated•22 years ago
|
QA Contact: olgam → huang
Reporter | ||
Comment 3•22 years ago
|
||
This should apply to all the buttons/items of tool bar, menu, context menu...etc.
Reporter | ||
Comment 4•22 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•22 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•22 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.
Assignee | ||
Comment 8•22 years ago
|
||
disable the delete commands for folders we can't delete from, and move menu for those same folders (and news folders).
Assignee | ||
Comment 9•22 years ago
|
||
Navin, can I get a review? thx.
Comment 10•22 years ago
|
||
We could optimize this by checking for imap folder and just doing it for imap folder, right ?
Assignee | ||
Comment 11•22 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•22 years ago
|
||
I thought we will never have readonly local folder. So why add the attribute to nsIMsgFolder, move it up to nsIMsgImapMailFolder.
Comment 13•22 years ago
|
||
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•22 years ago
|
||
no, because we're doing it for news too. Please see new patch.
Comment 15•22 years ago
|
||
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•22 years ago
|
||
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 17•22 years ago
|
||
Comment on attachment 84657 [details] [diff] [review] fix with Seth's comments incorporated sr=sspitzer
Attachment #84657 -
Flags: superreview+
Comment 18•22 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•22 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•22 years ago
|
||
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 21•22 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?
Comment 22•22 years ago
|
||
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 #]
Comment 23•22 years ago
|
||
> 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•22 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•22 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.
Comment 26•22 years ago
|
||
>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•22 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•22 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•22 years ago
|
||
And actually, from Comment #5 -- David did fix the most important part of this bug.....
Comment 30•22 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•22 years ago
|
||
rest of fix checked in.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 32•22 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•22 years ago
|
||
Oops! Typo! Above verification is on 06-10-08-trunk build, not 1.0.0 branch build.
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•