fix: Don't back-propagate exceptions from operations which use an `on…

…_done` callback (#836)

* fix: Pass exceptions in operations to `on_done`

* Edit comments

* ref: Move tests for `sync_with_ankihub` operation to one class

* Add test

* Add test

ankihub/gui/operations/ankihub_sync.py

@@ -1,4 +1,5 @@
 from concurrent.futures import Future
+from functools import partial
 from typing import Callable, List
 
 import aqt
@@ -74,7 +75,9 @@ def on_sync_done(future: Future) -> None:
             ),
         )
     except Exception as e:
-        on_done(future_with_exception(e))
+        # Using run_on_main prevents exceptions which occur in the callback to be backpropagated to the caller,
+        # which is what we want.
+        aqt.mw.taskman.run_on_main(partial(on_done, future_with_exception(e)))
 
 
 def _uninstall_decks_the_user_is_not_longer_subscribed_to(

ankihub/gui/operations/deck_installation.py

@@ -3,6 +3,7 @@
 import uuid
 from concurrent.futures import Future
 from datetime import datetime
+from functools import partial
 from typing import Callable, List
 
 import aqt
@@ -73,7 +74,9 @@ def on_install_done_inner(import_results: List[AnkiHubImportResult]):
             label="Downloading decks from AnkiHub",
         )
     except Exception as e:
-        on_done(future_with_exception(e))
+        # Using run_on_main prevents exceptions which occur in the callback to be backpropagated to the caller,
+        # which is what we want.
+        aqt.mw.taskman.run_on_main(partial(on_done, future_with_exception(e)))
 
 
 def _show_deck_import_summary_dialog(

ankihub/gui/operations/new_deck_subscriptions.py

@@ -1,5 +1,6 @@
 """Check if the user is subscribed to any decks that are not installed and install them if the user agrees."""
 from concurrent.futures import Future
+from functools import partial
 from typing import Callable, List, Optional
 
 import aqt
@@ -54,7 +55,9 @@ def check_and_install_new_deck_subscriptions(
         # This prevents the checkbox from being garbage collected too early
         confirmation_dialog.cleanup_cb = cleanup_cb  # type: ignore
     except Exception as e:
-        on_done(future_with_exception(e))
+        # Using run_on_main prevents exceptions which occur in the callback to be backpropagated to the caller,
+        # which is what we want.
+        aqt.mw.taskman.run_on_main(partial(on_done, future_with_exception(e)))
 
 
 def _on_button_clicked(

tests/addon/test_integration.py

@@ -735,6 +735,34 @@ def test_download_and_install_deck(
                     mock.call_count == 1
                 ), f"Mock {name} was not called once, but {mock.call_count} times"
 
+    def test_exception_is_not_backpropagated_to_caller(
+        self, anki_session_with_addon_data: AnkiSession, mock_function: MockFunction
+    ):
+        with anki_session_with_addon_data.profile_loaded():
+            # Mock a function which is called in download_install_decks to raise an exception.
+            exception_mesaage = "test exception"
+
+            def raise_exception(*args, **kwargs) -> None:
+                raise Exception(exception_mesaage)
+
+            mock_function(
+                "ankihub.gui.operations.deck_installation.aqt.mw.taskman.with_progress",
+                side_effect=raise_exception,
+            )
+
+            # Set up the on_done callback
+            future: Optional[Future] = None
+
+            def on_done(future_: Future) -> None:
+                nonlocal future
+                future = future_
+
+            # Call download_and_install_decks. This shouldn't raise an exception.
+            download_and_install_decks(ankihub_dids=[], on_done=on_done)
+
+            # Assert that the future contains the exception and that it contains the expected message.
+            assert future.exception().args[0] == exception_mesaage
+
 
 class TestCheckAndInstallNewDeckSubscriptions:
     def test_one_new_subscription(
@@ -3332,59 +3360,107 @@ def test_user_relation_gets_updated_in_deck_config(
             assert config.deck_config(ah_did).user_relation == incoming_relation
 
 
-@pytest.mark.parametrize(
-    "subscribed_to_deck",
-    [True, False],
-)
-@pytest.mark.qt_no_exception_capture
-def test_sync_uninstalls_unsubscribed_decks(
-    anki_session_with_addon_data: AnkiSession,
-    install_sample_ah_deck: InstallSampleAHDeck,
-    monkeypatch: MonkeyPatch,
-    mock_client_methods_called_during_ankihub_sync: None,
-    sync_with_ankihub: SyncWithAnkiHub,
-    subscribed_to_deck: bool,
-):
-    with anki_session_with_addon_data.profile_loaded():
-        mw = anki_session_with_addon_data.mw
+class TestSyncWithAnkiHub:
+    """Tests for the sync_with_ankihub operation."""
 
-        # Install a deck
-        anki_did, ah_did = install_sample_ah_deck()
+    @pytest.mark.parametrize(
+        "subscribed_to_deck",
+        [True, False],
+    )
+    @pytest.mark.qt_no_exception_capture
+    def test_sync_uninstalls_unsubscribed_decks(
+        self,
+        anki_session_with_addon_data: AnkiSession,
+        install_sample_ah_deck: InstallSampleAHDeck,
+        monkeypatch: MonkeyPatch,
+        mock_client_methods_called_during_ankihub_sync: None,
+        sync_with_ankihub: SyncWithAnkiHub,
+        subscribed_to_deck: bool,
+    ):
+        with anki_session_with_addon_data.profile_loaded():
+            mw = anki_session_with_addon_data.mw
 
-        # Mock client.get_deck_subscriptions to return the deck if subscribed_to_deck is True else return an empty list
-        monkeypatch.setattr(
-            AnkiHubClient,
-            "get_deck_subscriptions",
-            lambda *args, **kwargs: [DeckFactory.create(ah_did=ah_did)]
-            if subscribed_to_deck
-            else [],
-        )
+            # Install a deck
+            anki_did, ah_did = install_sample_ah_deck()
+
+            # Mock client.get_deck_subscriptions to return the deck if subscribed_to_deck is True and
+            # return an empty list otherwise
+            monkeypatch.setattr(
+                AnkiHubClient,
+                "get_deck_subscriptions",
+                lambda *args, **kwargs: [DeckFactory.create(ah_did=ah_did)]
+                if subscribed_to_deck
+                else [],
+            )
 
-        # Set a fake token so that the sync is not skipped
-        config.save_token("test_token")
+            # Set a fake token so that the sync is not skipped
+            config.save_token("test_token")
 
-        # Sync
-        sync_with_ankihub()
+            # Sync
+            sync_with_ankihub()
 
-        # Assert that the deck was uninstalled if the user is not subscribed to it,
-        # else assert that it was not uninstalled
-        assert config.deck_ids() == ([ah_did] if subscribed_to_deck else [])
-        assert ankihub_db.ankihub_deck_ids() == ([ah_did] if subscribed_to_deck else [])
+            # Assert that the deck was uninstalled if the user is not subscribed to it,
+            # else assert that it was not uninstalled
+            assert config.deck_ids() == ([ah_did] if subscribed_to_deck else [])
+            assert ankihub_db.ankihub_deck_ids() == (
+                [ah_did] if subscribed_to_deck else []
+            )
 
-        mids = [
-            mw.col.get_note(nid).mid for nid in mw.col.find_notes(f"did:{anki_did}")
-        ]
-        is_ankihub_note_type = [
-            note_type_contains_field(
-                mw.col.models.get(mid), ANKIHUB_NOTE_TYPE_FIELD_NAME
+            mids = [
+                mw.col.get_note(nid).mid for nid in mw.col.find_notes(f"did:{anki_did}")
+            ]
+            is_ankihub_note_type = [
+                note_type_contains_field(
+                    mw.col.models.get(mid), ANKIHUB_NOTE_TYPE_FIELD_NAME
+                )
+                for mid in mids
+            ]
+            assert (
+                all(is_ankihub_note_type)
+                if subscribed_to_deck
+                else not any(is_ankihub_note_type)
             )
-            for mid in mids
-        ]
-        assert (
-            all(is_ankihub_note_type)
-            if subscribed_to_deck
-            else not any(is_ankihub_note_type)
-        )
+
+    def test_sync_updates_api_version_on_last_sync(
+        self,
+        anki_session_with_addon_data: AnkiSession,
+        sync_with_ankihub: SyncWithAnkiHub,
+        mock_ankihub_sync_dependencies: None,
+    ):
+        assert config._private_config.api_version_on_last_sync is None  # sanity check
+
+        with anki_session_with_addon_data.profile_loaded():
+            sync_with_ankihub()
+
+        assert config._private_config.api_version_on_last_sync == API_VERSION
+
+    def test_exception_is_not_backpropagated_to_caller(
+        self, anki_session_with_addon_data: AnkiSession, mock_function: MockFunction
+    ):
+        with anki_session_with_addon_data.profile_loaded():
+            # Mock a client function which is called in sync_with_ankihub to raise an exception.
+            exception_mesaage = "test exception"
+
+            def raise_exception(*args, **kwargs) -> None:
+                raise Exception(exception_mesaage)
+
+            mock_function(
+                "ankihub.gui.operations.ankihub_sync.AnkiHubClient.get_deck_subscriptions",
+                side_effect=raise_exception,
+            )
+
+            # Set up the on_done callback
+            future: Optional[Future] = None
+
+            def on_done(future_: Future) -> None:
+                nonlocal future
+                future = future_
+
+            # Call sync_with_ankihub. This shouldn't raise an exception.
+            ankihub_sync.sync_with_ankihub(on_done=on_done)
+
+            # Assert that the future contains the exception and that it contains the expected message.
+            assert future.exception().args[0] == exception_mesaage
 
 
 def test_uninstalling_deck_removes_related_deck_extension_from_config(
@@ -3404,19 +3480,6 @@ def test_uninstalling_deck_removes_related_deck_extension_from_config(
         assert config.deck_extensions_ids_for_ah_did(ah_did) == []
 
 
-def test_sync_updates_api_version_on_last_sync(
-    anki_session_with_addon_data: AnkiSession,
-    sync_with_ankihub: SyncWithAnkiHub,
-    mock_ankihub_sync_dependencies: None,
-):
-    assert config._private_config.api_version_on_last_sync is None  # sanity check
-
-    with anki_session_with_addon_data.profile_loaded():
-        sync_with_ankihub()
-
-    assert config._private_config.api_version_on_last_sync == API_VERSION
-
-
 class TestAutoSync:
     def test_with_on_ankiweb_sync_config_option(
         self,