diff --git a/inbox/actions/backends/generic.py b/inbox/actions/backends/generic.py index caa962789..a9f1541c2 100644 --- a/inbox/actions/backends/generic.py +++ b/inbox/actions/backends/generic.py @@ -111,8 +111,12 @@ def remote_move( # type: ignore[no-untyped-def] for folder_name, uids in uids_for_message.items(): crispin_client.select_folder_if_necessary(folder_name, uidvalidity_cb) - crispin_client.conn.copy(uids, destination) - crispin_client.delete_uids(uids) + + if crispin_client.move_supported(): + crispin_client.conn.move(uids, destination) + else: + crispin_client.conn.copy(uids, destination) + crispin_client.delete_uids(uids) def remote_create_folder( # type: ignore[no-untyped-def] diff --git a/inbox/crispin.py b/inbox/crispin.py index 89c41e29a..c9bd94534 100644 --- a/inbox/crispin.py +++ b/inbox/crispin.py @@ -874,6 +874,10 @@ def idle_supported(self) -> bool: return b"IDLE" in self.conn.capabilities() + def move_supported(self) -> bool: + interruptible_threading.check_interrupted() + return b"MOVE" in self.conn.capabilities() + def search_uids(self, criteria: list[str]) -> Iterable[int]: """ Find UIDs in this folder matching the criteria. See diff --git a/inbox/util/testutils.py b/inbox/util/testutils.py index c04016958..8b6849ca1 100644 --- a/inbox/util/testutils.py +++ b/inbox/util/testutils.py @@ -263,8 +263,19 @@ def copy( # type: ignore[no-untyped-def] self, matching_uids, folder_name ) -> None: """ - Note: _moves_ one or more messages from the currently selected folder - to folder_name + Copy one or more messages from the currently selected folder. + + Note: Also deletes from source to simulate existing test expectations. + """ + for u in matching_uids: + self._data[folder_name][u] = self._data[self.selected_folder][u] + self.delete_messages(matching_uids) + + def move( # type: ignore[no-untyped-def] + self, matching_uids, folder_name + ) -> None: + """ + Atomically move one or more messages to folder_name (RFC 6851). """ for u in matching_uids: self._data[folder_name][u] = self._data[self.selected_folder][u] diff --git a/pytest.ini b/pytest.ini index 5a2ea1d7e..2886d3a49 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,4 +1,5 @@ [pytest] +testpaths = tests norecursedirs = inbox tests/imap/network tests/data timeout = 60 diff --git a/tests/imap/test_actions.py b/tests/imap/test_actions.py index 064e4b527..9c0a4cf41 100644 --- a/tests/imap/test_actions.py +++ b/tests/imap/test_actions.py @@ -13,6 +13,7 @@ delete_label, mark_starred, mark_unread, + move, save_draft, update_draft, update_folder, @@ -275,3 +276,58 @@ def test_failed_event_creation( assert all(a.status == "failed" for a in q) service.stop() + + +def test_move_uses_imap_move_when_supported( + db, default_account, message, folder, mock_imapclient +) -> None: + """Test that IMAP MOVE command is used when the server supports it.""" + mock_imapclient.add_folder_data(folder.name, {}) + mock_imapclient.add_folder_data("Archive", {}) + mock_imapclient.capabilities = mock.Mock( + return_value=[b"IMAP4rev1", b"MOVE"] + ) + mock_imapclient.move = mock.Mock() + mock_imapclient.copy = mock.Mock() + mock_imapclient.delete_messages = mock.Mock() + add_fake_imapuid(db.session, default_account.id, message, folder, 42) + + with writable_connection_pool(default_account.id).get() as crispin_client: + move( + crispin_client, + default_account.id, + message.id, + {"destination": "Archive"}, + ) + + mock_imapclient.move.assert_called_once_with([42], "Archive") + mock_imapclient.copy.assert_not_called() + + +def test_move_falls_back_to_copy_delete_when_move_not_supported( + db, default_account, message, folder, mock_imapclient +) -> None: + """Test fallback to COPY+DELETE when MOVE is not supported.""" + mock_imapclient.add_folder_data(folder.name, {}) + mock_imapclient.add_folder_data("Archive", {}) + mock_imapclient.capabilities = mock.Mock(return_value=[b"IMAP4rev1"]) + mock_imapclient.move = mock.Mock() + mock_imapclient.copy = mock.Mock() + mock_imapclient.delete_messages = mock.Mock() + mock_imapclient.expunge = mock.Mock() + add_fake_imapuid(db.session, default_account.id, message, folder, 42) + + with writable_connection_pool(default_account.id).get() as crispin_client: + move( + crispin_client, + default_account.id, + message.id, + {"destination": "Archive"}, + ) + + mock_imapclient.move.assert_not_called() + mock_imapclient.copy.assert_called_once_with([42], "Archive") + # delete_uids converts UIDs to strings before calling delete_messages + mock_imapclient.delete_messages.assert_called_once_with( + ["42"], silent=True + )