diff --git a/ankihub/gui/errors.py b/ankihub/gui/errors.py index c2237b211..17ad4a2bb 100644 --- a/ankihub/gui/errors.py +++ b/ankihub/gui/errors.py @@ -7,7 +7,6 @@ import time import traceback import zipfile -from concurrent.futures import Future from pathlib import Path from sqlite3 import OperationalError from textwrap import dedent @@ -46,6 +45,7 @@ ) from .deck_updater import NotLoggedInError from .error_dialog import ErrorDialog +from .operations import AddonQueryOp from .utils import ( ask_user, check_and_prompt_for_updates_on_main_window, @@ -92,7 +92,7 @@ def report_exception_and_upload_logs( def upload_logs_in_background( - on_done: Optional[Callable[[Future], None]] = None, hide_username=False + on_done: Optional[Callable[[str], None]] = None, hide_username=False ) -> str: """Upload the logs to S3 in the background. Returns the S3 key of the uploaded logs.""" @@ -103,19 +103,18 @@ def upload_logs_in_background( user_name = config.user() if not hide_username else checksum(config.user())[:5] key = f"ankihub_addon_logs_{user_name}_{int(time.time())}.log" - if on_done is not None: - aqt.mw.taskman.run_in_background( - task=lambda: _upload_logs(key), on_done=on_done - ) - else: - aqt.mw.taskman.run_in_background( - task=lambda: _upload_logs(key), on_done=_on_upload_logs_done - ) + AddonQueryOp( + parent=aqt.mw, + op=lambda _: _upload_logs(key), + success=on_done if on_done is not None else lambda _: None, + ).failure(_on_upload_logs_failure).without_collection().run_in_background() return key -def upload_logs_and_data_in_background(on_done: Callable[[Future], None] = None) -> str: +def upload_logs_and_data_in_background( + on_done: Optional[Callable[[str], None]] = None +) -> str: """Upload the data dir and logs to S3 in the background. Returns the S3 key of the uploaded file.""" @@ -125,9 +124,11 @@ def upload_logs_and_data_in_background(on_done: Callable[[Future], None] = None) user_name_hash = checksum(config.user())[:5] key = f"ankihub_addon_debug_info_{user_name_hash}_{int(time.time())}.zip" - aqt.mw.taskman.run_in_background( - task=lambda: _upload_logs_and_data_in_background(key), on_done=on_done - ) + AddonQueryOp( + parent=aqt.mw, + op=lambda _: _upload_logs_and_data_in_background(key), + success=on_done if on_done is not None else lambda _: None, + ).failure(_on_upload_logs_failure).without_collection().run_in_background() return key @@ -577,20 +578,18 @@ def _upload_logs(key: str) -> str: raise e -def _on_upload_logs_done(future: Future) -> None: - try: - future.result() - except AnkiHubHTTPError as e: +def _on_upload_logs_failure(exc: Exception) -> None: + if isinstance(exc, AnkiHubHTTPError): from .errors import OUTDATED_CLIENT_ERROR_REASON # Don't report outdated client errors that happen when uploading logs, # because they are handled by the add-on when they happen in other places # and we don't want to see them in Sentry. - if e.response.status_code == 401 or ( - e.response.status_code == 406 - and e.response.reason == OUTDATED_CLIENT_ERROR_REASON + if exc.response.status_code == 401 or ( + exc.response.status_code == 406 + and exc.response.reason == OUTDATED_CLIENT_ERROR_REASON ): return - _report_exception(e) - except Exception as e: - _report_exception(e) + _report_exception(exc) + else: + _report_exception(exc) diff --git a/ankihub/gui/media_sync.py b/ankihub/gui/media_sync.py index 857108798..e09b3efbb 100644 --- a/ankihub/gui/media_sync.py +++ b/ankihub/gui/media_sync.py @@ -1,5 +1,4 @@ import uuid -from concurrent.futures import Future from datetime import datetime from functools import cached_property from pathlib import Path @@ -13,6 +12,7 @@ from ..ankihub_client.models import DeckMedia from ..db import ankihub_db from ..settings import config +from .operations import AddonQueryOp class _AnkiHubMediaSync: @@ -44,10 +44,15 @@ def start_media_download(self): self._download_in_progress = True self.refresh_sync_status_text() - aqt.mw.taskman.run_in_background( - self._update_deck_media_and_download_missing_media, - on_done=self._on_download_finished, - ) + def on_failure(exception: Exception) -> None: + self._download_in_progress = False + raise exception + + AddonQueryOp( + parent=aqt.mw, + op=lambda _: self._update_deck_media_and_download_missing_media(), + success=self._on_download_finished, + ).failure(on_failure).without_collection().run_in_background() def start_media_upload( self, @@ -60,12 +65,18 @@ def start_media_upload( self.refresh_sync_status_text() media_paths = self._media_paths_for_media_names(media_names) - aqt.mw.taskman.run_in_background( - lambda: self._client.upload_media(media_paths, ankihub_did), - on_done=lambda future: self._on_upload_finished( - future, ankihub_deck_id=ankihub_did, on_success=on_success + + def on_failure(exception: Exception) -> None: + self._amount_uploads_in_progress -= 1 + raise exception + + AddonQueryOp( + parent=aqt.mw, + op=lambda _: self._client.upload_media(media_paths, ankihub_did), + success=lambda _: self._on_upload_finished( + ankihub_deck_id=ankihub_did, on_success=on_success ), - ) + ).failure(on_failure).without_collection().run_in_background() def stop_background_threads(self): """Stop all media sync operations.""" @@ -91,12 +102,10 @@ def _media_paths_for_media_names(self, media_names: Iterable[str]) -> Set[Path]: def _on_upload_finished( self, - future: Future, ankihub_deck_id: uuid.UUID, on_success: Optional[Callable[[], None]] = None, ): self._amount_uploads_in_progress -= 1 - future.result() LOGGER.info("Uploaded media to AnkiHub.") self.refresh_sync_status_text() @@ -155,9 +164,8 @@ def _missing_media_for_ah_deck(self, ah_did: uuid.UUID) -> List[str]: ] return result - def _on_download_finished(self, future: Future) -> None: + def _on_download_finished(self, _: None) -> None: self._download_in_progress = False - future.result() self.refresh_sync_status_text() def _refresh_media_download_status_inner(self): diff --git a/ankihub/gui/menu.py b/ankihub/gui/menu.py index 50d2de8e6..93ac52cf1 100644 --- a/ankihub/gui/menu.py +++ b/ankihub/gui/menu.py @@ -249,13 +249,12 @@ def _upload_logs_and_data_action() -> None: upload_logs_and_data_in_background(on_done=_on_logs_uploaded) -def _on_logs_uploaded(future: Future): +def _on_logs_uploaded(log_file_name: str) -> None: aqt.mw.progress.finish() - log_file_name = future.result() LogUploadResultDialog(log_file_name=log_file_name).exec() -def _ankihub_login_setup(parent: QMenu): +def _ankihub_login_setup(parent: QMenu) -> None: sign_in_button = QAction("🔑 Sign into AnkiHub", aqt.mw) qconnect(sign_in_button.triggered, AnkiHubLogin.display_login) parent.addAction(sign_in_button) diff --git a/ankihub/gui/operations/__init__.py b/ankihub/gui/operations/__init__.py index e69de29bb..4815c5ea1 100644 --- a/ankihub/gui/operations/__init__.py +++ b/ankihub/gui/operations/__init__.py @@ -0,0 +1,31 @@ +from aqt.operations import QueryOp + +from ...settings import ANKI_MINOR + + +class AddonQueryOp(QueryOp): + """A subclass of aqt.operations.QueryOp that is tailored for the AnkiHub add-on.""" + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + # The default behavior of the QueryOp class on a failure is to show the exception to the user. + # However we want to raise the exception so that our central error handler can handle it. + self._failure = _on_failure + + def without_collection(self): + """The QueryOp class doesn't have a without_collection method on Anki versions before 23.10. + We are setting this to be a no-op for backwards compatibility. + It's fine for this to be a no-op, because without_collection is used to allow + background tasks to run in parallel. On previous Anki versions background tasks were + already running in parallel, so there is no need to do anything. + This way we can use the same code for all Anki versions. + """ + if ANKI_MINOR < 231000: + return self + + return super().without_collection() + + +def _on_failure(exception: Exception) -> None: + raise exception diff --git a/ankihub/gui/operations/deck_creation.py b/ankihub/gui/operations/deck_creation.py index 58864a084..95538e0bb 100644 --- a/ankihub/gui/operations/deck_creation.py +++ b/ankihub/gui/operations/deck_creation.py @@ -2,7 +2,6 @@ import aqt from aqt import QCheckBox, QMessageBox -from aqt.operations import QueryOp from aqt.studydeck import StudyDeck from aqt.utils import showInfo, tooltip @@ -14,6 +13,7 @@ from ...main.subdecks import SUBDECK_TAG from ...settings import config, url_view_deck from ..media_sync import media_sync +from ..operations import AddonQueryOp from ..utils import ask_user @@ -132,13 +132,13 @@ def on_success(deck_creation_result: DeckCreationResult) -> None: f"{deck_url}" ) - def on_failure(exc: Exception): + def on_failure(exc: Exception) -> None: aqt.mw.progress.finish() raise exc - op = QueryOp( + op = AddonQueryOp( parent=aqt.mw, - op=lambda col: create_ankihub_deck( + op=lambda _: create_ankihub_deck( deck_name, private=private, add_subdeck_tags=add_subdeck_tags, diff --git a/ankihub/gui/optional_tag_suggestion_dialog.py b/ankihub/gui/optional_tag_suggestion_dialog.py index 212e925ae..ddcf6802e 100644 --- a/ankihub/gui/optional_tag_suggestion_dialog.py +++ b/ankihub/gui/optional_tag_suggestion_dialog.py @@ -1,6 +1,6 @@ """Dialog for suggesting optional tags for notes.""" from concurrent.futures import Future -from typing import Sequence +from typing import List, Sequence import aqt from anki.notes import NoteId @@ -20,7 +20,9 @@ from .. import LOGGER from ..ankihub_client import AnkiHubHTTPError +from ..ankihub_client.models import TagGroupValidationResponse from ..main.optional_tag_suggestions import OptionalTagsSuggestionHelper +from .operations import AddonQueryOp from .utils import show_error_dialog @@ -144,19 +146,20 @@ def _setup_tag_group_list(self): ) item.setToolTip("Validating...") - def _validate_tag_groups_and_update_ui(self): - aqt.mw.taskman.run_in_background( - task=self._validate_tag_groups_in_background, - on_done=self._on_validate_tag_groups_finished, - ) + def _validate_tag_groups_and_update_ui(self) -> None: + AddonQueryOp( + parent=self, + op=lambda _: self._validate_tag_groups(), + success=self._on_validate_tag_groups_finished, + ).without_collection().run_in_background() - def _validate_tag_groups_in_background(self): + def _validate_tag_groups(self) -> List[TagGroupValidationResponse]: result = self._optional_tags_helper.prevalidate() return result - def _on_validate_tag_groups_finished(self, future: Future): - tag_group_validation_responses = future.result() - + def _on_validate_tag_groups_finished( + self, tag_group_validation_responses: List[TagGroupValidationResponse] + ) -> None: self._valid_tag_groups = [ response.tag_group_name for response in tag_group_validation_responses diff --git a/tests/addon/test_unit.py b/tests/addon/test_unit.py index 90194b0f4..9d6544726 100644 --- a/tests/addon/test_unit.py +++ b/tests/addon/test_unit.py @@ -43,6 +43,7 @@ # has to be set before importing ankihub os.environ["SKIP_INIT"] = "1" +from ankihub.addon_ankihub_client import AddonAnkiHubClient from ankihub.ankihub_client import ( AnkiHubClient, AnkiHubHTTPError, @@ -60,7 +61,9 @@ _contains_path_to_this_addon, _normalize_url, _try_handle_exception, + upload_logs_in_background, ) +from ankihub.gui.media_sync import media_sync from ankihub.gui.menu import AnkiHubLogin from ankihub.gui.operations import deck_creation from ankihub.gui.operations.deck_creation import ( @@ -108,7 +111,7 @@ mids_of_notes, retain_nids_with_ah_note_type, ) -from ankihub.settings import ANKIWEB_ID +from ankihub.settings import ANKIWEB_ID, log_file_path @pytest.fixture @@ -162,6 +165,56 @@ def test_update_media_names_on_notes( assert '' in " ".join(notes[2].fields) +class TestMediaSyncMediaDownload: + def test_with_exception(self, mock_function: MockFunction, qtbot: QtBot): + def raise_exception() -> None: + raise Exception("test") + + update_and_download_mock = mock_function( + media_sync, + "_update_deck_media_and_download_missing_media", + side_effect=raise_exception, + ) + + with qtbot.captureExceptions() as exceptions: + media_sync.start_media_download() + qtbot.wait(500) + + # Assert that _download_in_progress was set to False and the exception was raised + assert not media_sync._download_in_progress + assert len(exceptions) == 1 + update_and_download_mock.assert_called_once() # sanity check + + +class TestMediaSyncMediaUpload: + def test_with_exception( + self, + anki_session_with_addon_data: AnkiSession, + mock_function: MockFunction, + qtbot: QtBot, + next_deterministic_uuid, + ): + with anki_session_with_addon_data.profile_loaded(): + + def raise_exception() -> None: + raise Exception("test") + + upload_media_mock = mock_function( + media_sync._client, + "upload_media", + side_effect=raise_exception, + ) + + with qtbot.captureExceptions() as exceptions: + media_sync.start_media_upload([], next_deterministic_uuid()) + qtbot.wait(500) + + # Assert that _amount_uploads_in_progress was was reset to 0 and the exception was raised + assert media_sync._amount_uploads_in_progress == 0 + assert len(exceptions) == 1 + upload_media_mock.assert_called_once() # sanity check + + def test_lowest_level_common_ancestor_deck_name(): deck_names = [ @@ -1434,6 +1487,62 @@ def _mock_ask_user_to_return_false( importlib.reload(errors) +class TestUploadLogs: + def test_basic( + self, + qtbot: QtBot, + mock_function: MockFunction, + ): + on_done_mock = Mock() + upload_logs_mock = mock_function(AddonAnkiHubClient, "upload_logs") + upload_logs_in_background(on_done=on_done_mock) + + qtbot.wait_until(lambda: on_done_mock.called) + + upload_logs_mock.assert_called_once() + assert upload_logs_mock.call_args[1]["file"] == log_file_path() + + @pytest.mark.parametrize( + "exception, expected_report_exception_called", + [ + # The exception should not be reported for these two specific cases + (AnkiHubHTTPError(response=Mock(status_code=401)), False), + ( + AnkiHubHTTPError( + response=Mock(status_code=406, reason=OUTDATED_CLIENT_ERROR_REASON) + ), + False, + ), + # The exception should be reported in all other cases + (AnkiHubHTTPError(response=Mock(status_code=500)), True), + (Exception("test"), True), + ], + ) + def test_with_exception( + self, + qtbot: QtBot, + mock_function: MockFunction, + exception: Exception, + expected_report_exception_called: bool, + ): + def raise_exception(*args, **kwargs) -> None: + raise exception + + on_done_mock = Mock() + upload_logs_mock = mock_function( + AddonAnkiHubClient, "upload_logs", side_effect=raise_exception + ) + report_exception_mock = mock_function(errors, "_report_exception") + upload_logs_in_background(on_done=on_done_mock) + + qtbot.wait(500) + + upload_logs_mock.assert_called_once() + on_done_mock.assert_not_called() + + assert report_exception_mock.called == expected_report_exception_called + + class TestRateLimitedDecorator: def test_rate_limited_decorator(self): # Create a counter to keep track of how many times foo is executed