From a999c647af0fbbcc082596caf5f6e9411f7d61d7 Mon Sep 17 00:00:00 2001 From: Sweta Yadav <106366788+swetayadav1@users.noreply.github.com> Date: Thu, 2 Nov 2023 19:15:46 +0530 Subject: [PATCH] NXDRIVE-2711: Show that upload is still alive for very large files (#4227) NXDRIVE-2711: Show thta upload is still alive for very large file Co-authored-by: swetayadav1 --- docs/changes/5.3.3.md | 5 +-- nxdrive/client/uploader/__init__.py | 18 ++++++---- nxdrive/dao/engine.py | 7 ++++ nxdrive/data/i18n/i18n.json | 1 + nxdrive/data/qml/TransferItem.qml | 8 +++++ nxdrive/engine/activity.py | 19 ++++++++-- nxdrive/gui/view.py | 7 ++++ tests/functional/test_view.py | 10 ------ tests/unit/conftest.py | 55 +++++++++++++++++++++++++++-- tests/unit/test_action.py | 3 ++ tests/unit/test_client_uploader.py | 50 ++++++++++++++++++++++++++ tests/unit/test_engine_dao.py | 23 ++++++++++-- tests/unit/test_utils.py | 6 ++-- tests/unit/test_view.py | 37 +++++++++++++++++++ 14 files changed, 221 insertions(+), 28 deletions(-) delete mode 100644 tests/functional/test_view.py create mode 100644 tests/unit/test_client_uploader.py create mode 100644 tests/unit/test_view.py diff --git a/docs/changes/5.3.3.md b/docs/changes/5.3.3.md index 98cf2bbdf5..3f85fecd63 100644 --- a/docs/changes/5.3.3.md +++ b/docs/changes/5.3.3.md @@ -12,7 +12,7 @@ Release date: `2023-xx-xx` ### Direct Transfer -- [NXDRIVE-2](https://jira.nuxeo.com/browse/NXDRIVE-2): +- [NXDRIVE-2711](https://jira.nuxeo.com/browse/NXDRIVE-2711): Show that upload is still alive for very large files ## GUI @@ -83,6 +83,7 @@ Release date: `2023-xx-xx` - Upgraded `typing-extensions` from 4.0.1 to 4.7.1 - Upgraded `vulture` from 2.3 to 2.9.1 - Upgraded `wcwidth` from 0.2.5 to 0.2.6 + ## Technical Changes -- +- Added `finalizing_status` attribute in LinkingAction class diff --git a/nxdrive/client/uploader/__init__.py b/nxdrive/client/uploader/__init__.py index 812fa719e7..601d90db8c 100644 --- a/nxdrive/client/uploader/__init__.py +++ b/nxdrive/client/uploader/__init__.py @@ -416,7 +416,7 @@ def _link_blob_to_doc( self._set_transfer_status(transfer, TransferStatus.ONGOING) raise exc - def link_blob_to_doc( # type: ignore[return] + def link_blob_to_doc( self, command: str, transfer: Upload, @@ -451,15 +451,19 @@ def link_blob_to_doc( # type: ignore[return] kwargs["headers"] = headers try: doc_type = kwargs.get("doc_type", "") - if transfer.is_direct_transfer and doc_type and doc_type != "": - res = self._transfer_docType_file(transfer, headers, doc_type) - else: - res = self._transfer_autoType_file(command, blob, kwargs) - - return res + return ( + self._transfer_docType_file(transfer, headers, doc_type) + if transfer.is_direct_transfer and doc_type and doc_type != "" + else self._transfer_autoType_file(command, blob, kwargs) + ) except Exception as exc: err = f"Error while linking blob to doc: {exc!r}" log.warning(err) + action.finalizing_status = "Error" + if "TCPKeepAliveHTTPSConnectionPool" not in str(exc): + transfer.request_uid = str(uuid4()) + self.dao.update_upload_requestid(transfer) + raise exc finally: action.finish_action() diff --git a/nxdrive/dao/engine.py b/nxdrive/dao/engine.py index 38705b6795..9f52a18d90 100644 --- a/nxdrive/dao/engine.py +++ b/nxdrive/dao/engine.py @@ -2381,6 +2381,13 @@ def update_upload(self, upload: Upload, /) -> None: sql = "UPDATE Uploads SET batch = ? WHERE uid = ?" c.execute(sql, (json.dumps(batch), upload.uid)) + def update_upload_requestid(self, upload: Upload, /) -> None: + """In case of error during linking, update request_uid for upload""" + with self.lock: + c = self._get_write_connection().cursor() + sql = "UPDATE Uploads SET request_uid = ? WHERE uid = ?" + c.execute(sql, (upload.request_uid, upload.uid)) + def pause_transfer( self, nature: str, diff --git a/nxdrive/data/i18n/i18n.json b/nxdrive/data/i18n/i18n.json index 379740df5f..f65f8eecf5 100644 --- a/nxdrive/data/i18n/i18n.json +++ b/nxdrive/data/i18n/i18n.json @@ -106,6 +106,7 @@ "DIRECT_TRANSFER_DETAILS": "[%1%] %2 of %3", "DIRECT_TRANSFER_END": "Transfer done: \"%1\"", "DIRECT_TRANSFER_ERROR": "Transfer error: \"%1\"", + "DIRECT_TRANSFER_FINALIZING_ERROR": "An error occurred during the transfer, it will resume shortly.", "DIRECT_TRANSFER_NO_ACCOUNT": "Cannot use the Direct Transfer feature with no account, aborting.", "DIRECT_TRANSFER_NOT_ALLOWED": "Direct Transfer of \"%1\" is not allowed for synced files.", "DIRECT_TRANSFER_NOT_ENABLED": "The Direct Transfer feature is not enabled.", diff --git a/nxdrive/data/qml/TransferItem.qml b/nxdrive/data/qml/TransferItem.qml index eb030950c1..cd1d79fafe 100644 --- a/nxdrive/data/qml/TransferItem.qml +++ b/nxdrive/data/qml/TransferItem.qml @@ -77,5 +77,13 @@ Rectangle { } } } + + ScaledText { + text: qsTr("DIRECT_TRANSFER_FINALIZING_ERROR") + tl.tr + color: secondaryText + visible: finalizing && finalizing_status + Layout.leftMargin: icon.width + 5 + font.pointSize: point_size * 0.8 + } } } diff --git a/nxdrive/engine/activity.py b/nxdrive/engine/activity.py index d43e459869..b8713cefc3 100644 --- a/nxdrive/engine/activity.py +++ b/nxdrive/engine/activity.py @@ -54,8 +54,7 @@ def get_current_action(*, thread_id: int = None) -> Optional["Action"]: @staticmethod def finish_action() -> None: - action = Action.actions.pop(current_thread_id(), None) - if action: + if action := Action.actions.pop(current_thread_id(), None): action.finish() def finish(self) -> None: @@ -149,6 +148,15 @@ def progress(self, value: float, /) -> None: self.progressing.emit(self) + @property + def finalizing_status(self) -> str: + return self._finalizing_status + + @finalizing_status.setter + def finalizing_status(self, value: str, /) -> None: + self._finalizing_status = value + self.progressing.emit(self) + def get_percent(self) -> float: if self.size < 0 or (self.empty and not self.uploaded): return 0.0 @@ -257,6 +265,13 @@ def __init__( doc_pair=doc_pair, ) self.progress = size + self.finalizing_status = "" + + def export(self) -> Dict[str, Any]: + return { + **super().export(), + "finalizing_status": self.finalizing_status, + } def tooltip(doing: str): # type: ignore diff --git a/nxdrive/gui/view.py b/nxdrive/gui/view.py index 98882c34c8..b0fdbceec9 100755 --- a/nxdrive/gui/view.py +++ b/nxdrive/gui/view.py @@ -273,6 +273,7 @@ class DirectTransferModel(QAbstractListModel): REMOTE_PARENT_REF = qt.UserRole + 10 SHADOW = qt.UserRole + 11 # Tell the interface if the row should be visible or not DOC_PAIR = qt.UserRole + 12 + FINALIZING_STATUS = qt.UserRole + 13 def __init__(self, translate: Callable, /, *, parent: QObject = None) -> None: super().__init__(parent) @@ -291,6 +292,7 @@ def __init__(self, translate: Callable, /, *, parent: QObject = None) -> None: self.REMOTE_PARENT_REF: b"remote_parent_ref", self.SHADOW: b"shadow", self.DOC_PAIR: b"doc_pair", + self.FINALIZING_STATUS: b"finalizing_status", } # Pretty print self.psize = partial(sizeof_fmt, suffix=self.tr("BYTE_ABBREV")) @@ -353,6 +355,8 @@ def data(self, index: QModelIndex, role: int, /) -> Any: return self.psize(row["filesize"]) if role == self.TRANSFERRED: return self.psize(row["filesize"] * row["progress"] / 100) + if role == self.FINALIZING_STATUS: + return row.get("finalizing_status") return row[self.names[role].decode()] def setData(self, index: QModelIndex, value: Any, /, *, role: int = None) -> None: @@ -375,6 +379,9 @@ def set_progress(self, action: Dict[str, Any], /) -> None: self.setData(idx, action["progress"], role=self.TRANSFERRED) if action["action_type"] == "Linking": self.setData(idx, True, role=self.FINALIZING) + self.setData( + idx, action["finalizing_status"], role=self.FINALIZING_STATUS + ) def add_item(self, parent: QModelIndex, n_item: Dict[str, Any], /) -> None: """Add an item to existing list.""" diff --git a/tests/functional/test_view.py b/tests/functional/test_view.py deleted file mode 100644 index 8ac1897b1f..0000000000 --- a/tests/functional/test_view.py +++ /dev/null @@ -1,10 +0,0 @@ -from nxdrive.gui.view import FileModel - - -def test_foldersDialog(): - def func(): - return True - - file_model = FileModel(func) - returned_val = file_model.add_files([{"key": "val"}]) - assert not returned_val diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 44a037bf51..5ee49abf9f 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -1,19 +1,23 @@ import os import shutil import time -from typing import Optional +from typing import Any, Callable, Optional from uuid import uuid4 import pytest from nxdrive.client.remote_client import Remote +from nxdrive.constants import TransferStatus from nxdrive.dao.engine import EngineDAO from nxdrive.dao.manager import ManagerDAO from nxdrive.engine.engine import Engine from nxdrive.engine.processor import Processor +from nxdrive.gui.view import DirectTransferModel from nxdrive.manager import Manager -from nxdrive.objects import DocPair +from nxdrive.objects import DocPair, Upload from nxdrive.osi import AbstractOSIntegration +from nxdrive.qt import constants as qt +from nxdrive.qt.imports import QObject from nxdrive.updater.darwin import Updater from nxdrive.utils import normalized_path @@ -141,6 +145,13 @@ def __init__(self, tmp_path): super().__init__(self, final_app) +class MockDirectTransferModel(DirectTransferModel): + def __init__( + self, translate: Callable[..., Any], /, *, parent: QObject = None + ) -> None: + super().__init__(translate, parent=parent) + + @pytest.fixture() def engine_dao(tmp_path): dao = MockEngineDAO @@ -188,3 +199,43 @@ def processor(engine, engine_dao): processor.remote = Remote processor.dao = engine_dao return processor + + +@pytest.fixture() +def upload(): + upload = Upload + upload.path = "/tmp" + upload.status = TransferStatus.ONGOING + upload.engine = f"{engine}" + upload.is_direct_edit = False + upload.is_direct_transfer = True + upload.filesize = "23.0" + upload.batch = {"batchID": f"{str(uuid4())}"} + upload.chunk_size = "345" + upload.remote_parent_path = "/tmp/remote_path" + upload.remote_parent_ref = "/tmp/remote_path_ref" + upload.doc_pair = "test_file" + upload.request_uid = str(uuid4()) + return upload + + +@pytest.fixture() +def direct_transfer_model(): + direct_transfer_model = MockDirectTransferModel + direct_transfer_model.FINALIZING_STATUS = qt.UserRole + 13 + direct_transfer_model.items = [ + { + "uid": 1, + "name": "a.txt", + "filesize": 142936511610, + "status": "", + "engine": "51a2c2dc641311ee87fb...bfc0ec09fa", + "progress": 100.0, + "doc_pair": 1, + "remote_parent_path": "/default-domain/User...TestFolder", + "remote_parent_ref": "7b7886ea-5ad9-460d-8...1607ea0081", + "shadow": True, + "finalizing": True, + } + ] + return direct_transfer_model diff --git a/tests/unit/test_action.py b/tests/unit/test_action.py index fbc7d1ecdd..1bff8b34bc 100644 --- a/tests/unit/test_action.py +++ b/tests/unit/test_action.py @@ -244,6 +244,9 @@ def test_finalization_action(tmp): action = LinkingAction(filepath, filepath.stat().st_size) assert action.type == "Linking" + action.finalizing_status = "Error occurred while linking" + details = action.export() + assert details["finalizing_status"] == "Error occurred while linking" Action.finish_action() assert action.finished diff --git a/tests/unit/test_client_uploader.py b/tests/unit/test_client_uploader.py new file mode 100644 index 0000000000..f1a2dff404 --- /dev/null +++ b/tests/unit/test_client_uploader.py @@ -0,0 +1,50 @@ +from unittest.mock import Mock +from uuid import uuid4 + +import pytest +import requests +from nuxeo.models import FileBlob + +from nxdrive.client.remote_client import Remote +from nxdrive.client.uploader import BaseUploader + + +@pytest.fixture +def baseuploader(): + remote = Remote + remote.dao = Mock() + return BaseUploader(remote) + + +def test_link_blob_to_doc(baseuploader, upload, tmp_path, monkeypatch): + """Test system network and server side exception handling while linking blob to document""" + file = tmp_path / f"{uuid4()}.txt" + file.write_bytes(b"content") + + def mock_transfer_autoType_file(*args, **kwargs): + raise requests.exceptions.RequestException("Connection Error") + + monkeypatch.setattr( + baseuploader, "_transfer_autoType_file", mock_transfer_autoType_file + ) + + # server side exceptions + with pytest.raises(requests.exceptions.RequestException): + baseuploader.link_blob_to_doc( + "Filemanager.Import", upload, FileBlob(str(file)), False + ) + + def mock_transfer_autoType_file(*args, **kwargs): + raise requests.exceptions.RequestException( + "TCPKeepAliveHTTPSConnectionPool: Connection Error" + ) + + monkeypatch.setattr( + baseuploader, "_transfer_autoType_file", mock_transfer_autoType_file + ) + + # system network disconnect + with pytest.raises(requests.exceptions.RequestException): + baseuploader.link_blob_to_doc( + "Filemanager.Import", upload, FileBlob(str(file)), False + ) diff --git a/tests/unit/test_engine_dao.py b/tests/unit/test_engine_dao.py index 9cba64f50a..1b2a07c2eb 100644 --- a/tests/unit/test_engine_dao.py +++ b/tests/unit/test_engine_dao.py @@ -1,8 +1,9 @@ import os import sqlite3 from datetime import datetime +from multiprocessing import RLock from pathlib import Path -from unittest.mock import patch +from unittest.mock import Mock, patch from uuid import uuid4 from nxdrive.constants import TransferStatus @@ -408,7 +409,7 @@ def test_migration_db_v10(engine_dao): """Verify Downloads after migration from v9 to v10.""" with engine_dao("engine_migration_10.db") as dao: downloads = list(dao.get_downloads()) - assert len(downloads) == 0 + assert not downloads states = list(dao.get_states_from_partial_local(Path())) assert len(states) == 4 @@ -608,3 +609,21 @@ def test_migration_interface(): assert not interface.downgrade(cursor) assert not interface.previous_version assert not interface.version + + +def test_update_upload_requestid(engine_dao, upload): + """Test to save upload and update reuqest_uid of existing row""" + engine_dao.lock = RLock() + with engine_dao("engine_migration_18.db") as dao: + engine_dao.directTransferUpdated = Mock() + # Save New upload + engine_dao.save_upload(dao, upload) + + assert upload.uid + + previous_request_id = upload.request_uid + upload.request_uid = str(uuid4()) + # Update request_uid of existing record + engine_dao.update_upload_requestid(dao, upload) + + assert previous_request_id != upload.request_uid diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 2e9133e5ca..5c0d96cc95 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -413,7 +413,7 @@ def test_request_verify_ca_bundle_file(caplog, tmp_path): # Save the certificate for the first time caplog.clear() cert = nxdrive.utils.requests_verify(ca_bundle, False) - path = "" if type(cert) == bool else cert + path = "" if isinstance(bool, type(cert)) else cert final_certificate = Path(path) records = [line.message for line in caplog.records] assert len(records) == 3 @@ -451,7 +451,7 @@ def test_request_verify_ca_bundle_file_is_str(caplog, tmp_path): # Save the certificate for the first time caplog.clear() cert = nxdrive.utils.requests_verify(ca_bundle, False) - path = "" if type(cert) == bool else cert + path = "" if isinstance(bool, type(cert)) else cert final_certificate = Path(path) records = [line.message for line in caplog.records] assert len(records) == 3 @@ -494,7 +494,7 @@ def test_request_verify_ca_bundle_file_mimic_updates(caplog, tmp_path): # Save the certificate for the first time caplog.clear() cert = nxdrive.utils.requests_verify(ca_bundle, False) - path = "" if type(cert) == bool else cert + path = "" if isinstance(bool, type(cert)) else cert final_certificate_1 = Path(path) records = [line.message for line in caplog.records] assert len(records) == 3 diff --git a/tests/unit/test_view.py b/tests/unit/test_view.py new file mode 100644 index 0000000000..f5d78674fe --- /dev/null +++ b/tests/unit/test_view.py @@ -0,0 +1,37 @@ +from unittest.mock import Mock + +from nxdrive.gui.view import FileModel +from nxdrive.qt.imports import QModelIndex + + +def test_foldersDialog(): + def func(): + return True + + file_model = FileModel(func) + returned_val = file_model.add_files([{"key": "val"}]) + assert not returned_val + + +def test_set_progress(direct_transfer_model): + """Test the finalize state after 100% progress""" + action = { + "engine": "51a2c2dc641311ee87fb...bfc0ec09fa", + "doc_pair": 1, + "progress": "100", + "action_type": "Linking", + "finalizing_status": "Finalize the status", + } + + direct_transfer_model.createIndex = Mock(return_value=1) + direct_transfer_model.setData = Mock() + direct_transfer_model.set_progress(direct_transfer_model, action) + + +def test_data(direct_transfer_model): + """Test get row data as per role""" + index = QModelIndex + index.row = Mock(return_value=0) + direct_transfer_model.data( + direct_transfer_model, index, direct_transfer_model.FINALIZING_STATUS + )