If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

trunk: Undo msg moved from local folder to an imap folder is broken for non UIDPLUS servers

NEW
Unassigned

Status

MailNews Core
Backend
15 years ago
3 years ago

People

(Reporter: Navin Gupta, Unassigned)

Tracking

(Blocks: 2 bugs, {imap-interop})

Trunk
x86
Windows 2000
imap-interop
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

15 years ago
The fix for bug 129495 caused this regression because it prevented
SetAppendMsgUid needed for undo.
(Reporter)

Updated

15 years ago
Status: NEW → ASSIGNED
Keywords: regression
Summary: Undo moving a local msg to imap folder is broken → Undo msg moved from local folder to an imap folder is broken
(Reporter)

Updated

15 years ago
Summary: Undo msg moved from local folder to an imap folder is broken → trunk: Undo msg moved from local folder to an imap folder is broken

Comment 1

15 years ago
actually, I don't believe that was working even without that change. The message
is removed from the source, but not from the destination imap folder, with or
w/o my change. I'll go verify it on the branch, but that was my experience.
(Reporter)

Comment 2

15 years ago
This code is never executed because dstKeyArray size is always zero. 

  if (m_dstKeyArray.GetSize() > 0)
  {
    nsCOMPtr<nsIMsgFolder> dstFolder = do_QueryReferent(m_dstFolder, &rv);
    if (NS_FAILED(rv) || !dstFolder) return rv;
    
    nsCOMPtr<nsIUrlListener> dstListener;
    
    dstListener = do_QueryInterface(dstFolder, &rv);
    if (NS_FAILED(rv)) return rv;
    // ** make sire we are in the selected state; use lite select folder
    // so we won't hit preformace hard
    rv = imapService->LiteSelectFolder(m_eventQueue, dstFolder,
      dstListener, nsnull);
    if (NS_FAILED(rv)) return rv;
    rv = imapService->AddMessageFlags(m_eventQueue, dstFolder,
      dstListener, nsnull,
      m_dstMsgIdString.get(),
      kImapMsgDeletedFlag,
      m_idsAreUids);
  }
(Reporter)

Comment 3

15 years ago
If I back out your fix locally on trunk it fixes the problem for me.

Comment 4

15 years ago
OK, well, please read the comments in the other bug - we can't go back to the
way it worked before - that approach was ridiculously slow.

Updated

15 years ago
QA Contact: gayatri → meehansqa
(Reporter)

Comment 5

15 years ago
How else are we going to know the new key of the msg we appended. We cannot just
go and search for new key when the user tries to do undo because that can happen
anytime and new hdrs could come into that folder. The only way not to slow down
copy is to not execute that code when we are copying sent mail, probably we
would have to add another imapAction nsIImapUrl::CopyingSentMail or something. 

Comment 6

15 years ago
no, as I thought I said in my other comment, you can search by message-id, which
should be unique.

Comment 7

15 years ago
also, we shouldn't just fix it for copying to the sent folder. The way the code
was before, if you copied 100 messages from local to imap, we would issue a
select and 100 searches based on message-id trying to find the appended
messages, JUST IN CASE the user undid the move/copy. That price should obviously
only be paid in the highly unlikely event that the user undoes the move/copy.

Comment 8

15 years ago
Created attachment 94774 [details] [diff] [review]
backing out part of fix that caused this regression

OK, I've rearranged the code so that everything works or doesn't work like it
did before. This was working for UIDPLUS servers and not working for non
UIDPLUS servers. With this patch, we'll be back where we were in terms of it
working, but w/o the horrible performance penalty on non UIDPLUS servers. This
has r/sr =sspitzer/

Comment 9

15 years ago
changing summary, removing regression keyword, adding interop, since it works
like it did before now.  It is, however, still broken on non UIDPLUS servers,
like the UW server. The Netscape server supports UIDPLUS, but we should really
work on all the servers that don't support UIDPLUS.
Keywords: regression → interop
Summary: trunk: Undo msg moved from local folder to an imap folder is broken → trunk: Undo msg moved from local folder to an imap folder is broken for non UIDPLUS servers
(Reporter)

Comment 10

15 years ago
Actually I was working on adding some search code in imapUndo. I mean creating
search session..... and searching for message-id. Let me see how it goes. I will
add a patch if it works. Also I will be looking into passing m_srcMsgIdString
instead of m_dstMsgIdString.get() here (from comment #2) with m_idsAreUids as
false. I don't know if it will work.


    rv = imapService->AddMessageFlags(m_eventQueue, dstFolder,
      dstListener, nsnull,
      m_dstMsgIdString.get(),
      kImapMsgDeletedFlag,
      m_idsAreUids);

Comment 11

15 years ago
yes, you still need to do that, I think. It's only in the case of UIDPLUS, where
we know right away what the UID of the newly appended message will be, that we
don't need to issue a search later. It will make life a little more complicated
in that the UNDO object will need to know if it already knows the UID, or if it
needs to search based on the string message-id to find the UID.
(Reporter)

Comment 12

15 years ago
How is this not a regression anymore ? It was working for non UIDPLUS servers
before. 
Keywords: interop → regression

Comment 13

15 years ago
No, it was not. If you can show me an imap protocol log with a non-uid plus imap
server with a 1.0 build where it worked, then I'm wrong, but my tests showed
that it did not work - the msg was not deleted from the destination imap server.
In any case, as it stands now, it's an interop bug because it only occurs on
certain imap servers.
Keywords: regression → interop
(Reporter)

Comment 14

15 years ago
why 1.0 build ? I am saying this is a trunk regression. Do you have a uw-imap
server I can test on ?

Comment 15

15 years ago
Are you saying it worked on the trunk, but not in Mozilla 1.0?  I didn't realize
it had been fixed on the trunk, but not in 1.0 - can you point me at a trunk
checkin that made it work on the trunk? I was not aware of that, sorry. 

Comment 16

15 years ago
Instead of 1.0, I should have said "1.1 build, or any trunk build before my
checkin that introduced the regression".
(Reporter)

Comment 17

15 years ago
I am unable to get such a server for testing. I had problems in setting up accts. 
Anyway, if you think it was not working before, then it is ok.

Comment 18

15 years ago
sorry, forgot - valjean.mcom.com is a UW server - Seth maintains it.
mass re-assign.
Assignee: naving → sspitzer
Status: ASSIGNED → NEW
Product: MailNews → Core

Comment 20

13 years ago
Possibly related: bug 59688.
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Blocks: 427007
Filter on "Nobody_NScomTLD_20080620"
QA Contact: meehansqa → backend
(Assignee)

Updated

9 years ago
Product: Core → MailNews Core
Blocks: 467841
See Also: → bug 59688
See Also: → bug 626469
You need to log in before you can comment on or make changes to this bug.