2060750 - Comment in nsImapMailFolder::CopyFolder wrongly says the UI forbids copying a folder within one IMAP account

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Nico D.]

Steps to reproduce:

The comment above the folder copy path in nsImapMailFolder::CopyFolder gives a reason for that code path which is not true:

  // !sameServer OR it's a copy. Unit tests expect a successful folder
  // copy within the same IMAP server even though the UI forbids copy and
  // only allows moves inside the same server. folderCopier, set below,
  // handles the folder copy within an IMAP server (needed by unit tests) and
  // the folder move or copy from another account or server into an IMAP
  // account/server. The folder move from another account is "impure" since
  // just the messages are moved and the source folder remains in place.
  RefPtr<nsImapFolderCopyState> folderCopier = new nsImapFolderCopyState(

The user interface does allow copying a folder inside one IMAP account, in two ways: dragging the folder in the folder pane with Ctrl held down, and the "Copy To" submenu of the folder context menu. Neither restricts source and destination to different accounts: in mail/base/content/about3Pane.js the drop effect becomes "copy" whenever the system drop effect is "copy" for any folder drag, and folderPaneContext-copyMenu is shown for a folder guarded only by "not a special folder" and "account is online", which then calls copyFolder(..., isMove = false, ...).

Suggest to drop the claim that the UI forbids a copy, and with it "(needed by unit
tests)": no unit test copies a folder inside one IMAP account. Every copy in
mailnews/imap/test/unit uses a local folder as the source, and every call
between two folders of one account is a move. The branch exists because the UI
offers that copy.

The condition itself is fine and should stay. It is a dispatch, not a guard: a move inside one account becomes a server-side RENAME, everything else goes through nsImapFolderCopyState. What is worth keeping is that the branch handles a copy inside one account as well as a move or a copy coming from another account, and the last sentence about a move from another account being "impure" is accurate.

Link to the code, search the file for "UI forbids copy":
https://searchfox.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#7339

Comment 1

[Magnus Melin [:mkmelin]]

Want to submit a patch?

Comment 2

[Nico D.]

Yeah, I'll take care of it if no one snatches it up first. Unfortunately, it's not uncommon for me to stumble upon other issues while working on a bug, and I'll log those as bugs or tasks in Bugzilla - if I can't find an existing issue - so they don't get lost.

My goal is definitely to work through my own findings as well

Comment 3

[Nico D.]

Attachment: Bug 2060750 - Fix the comment about copying a folder within one IMAP account and cover that copy with a test. r=#thunderbird-back-end-reviewers

The comment claimed that the UI forbids that copy and that the branch below
only exists for the unit tests. Neither is true: the folder pane offers the
copy through the "Copy To" menu and through a copy-drag, and no test in
mailnews/imap/test/unit copies a folder within one account - every copy there
has a local source. The new test closes that gap. It fails if the dispatch
stops telling a move apart from a copy, because the source folder is then
renamed away instead of staying in place.

While here, drop the null check on srcFolder in the move branch. It cannot
fail: NS_ENSURE_ARG_POINTER returns earlier in the function, IsOnSameServer
rejects a null source as well, and the same block dereferences srcFolder
unconditionally a few lines further down.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/700824fe6f7a
Fix the comment about copying a folder within one IMAP account and cover that copy with a test. r=mkmelin