From 632b84c5caa53d9c6cb1122f2970e675838a93a2 Mon Sep 17 00:00:00 2001 From: Jakub Fidler <31575114+RisingOrange@users.noreply.github.com> Date: Wed, 27 Dec 2023 20:21:09 +0100 Subject: [PATCH] fix: Don't attach ankihub DB for querying review data (#851) * Don't attach db for querying review data * Add test * Edit test --- ankihub/main/review_data.py | 70 ++++++++++----------- tests/addon/test_unit.py | 117 ++++++++++++++++++++---------------- 2 files changed, 98 insertions(+), 89 deletions(-) diff --git a/ankihub/main/review_data.py b/ankihub/main/review_data.py index 96217cb64..693934260 100644 --- a/ankihub/main/review_data.py +++ b/ankihub/main/review_data.py @@ -7,7 +7,7 @@ from .. import LOGGER from ..addon_ankihub_client import AddonAnkiHubClient as AnkiHubClient from ..ankihub_client import CardReviewData -from ..db import attached_ankihub_db +from ..db import ankihub_db from ..settings import config @@ -15,33 +15,31 @@ def send_review_data() -> None: """Send data about card reviews for each installed AnkiHub deck to the server. Data about decks that have not been reviewed yet will not be included.""" now = datetime.now() + card_review_data = [] + for ah_did in config.deck_ids(): + first_and_last_review_times = _get_first_and_last_review_datetime_for_ah_deck( + ah_did + ) + if first_and_last_review_times is None: + # This deck has no reviews yet + continue - with attached_ankihub_db(): - card_review_data = [] - for ah_did in config.deck_ids(): - first_and_last_review_times = ( - _get_first_and_last_review_datetime_for_ah_deck(ah_did) - ) - if first_and_last_review_times is None: - # This deck has no reviews yet - continue - - first_review_at, last_review_at = first_and_last_review_times - total_card_reviews_last_7_days = _get_review_count_for_ah_deck_since( - ah_did, now - timedelta(days=7) - ) - total_card_reviews_last_30_days = _get_review_count_for_ah_deck_since( - ah_did, now - timedelta(days=30) - ) - card_review_data.append( - CardReviewData( - ah_did=ah_did, - total_card_reviews_last_7_days=total_card_reviews_last_7_days, - total_card_reviews_last_30_days=total_card_reviews_last_30_days, - first_card_review_at=first_review_at, - last_card_review_at=last_review_at, - ) + first_review_at, last_review_at = first_and_last_review_times + total_card_reviews_last_7_days = _get_review_count_for_ah_deck_since( + ah_did, now - timedelta(days=7) + ) + total_card_reviews_last_30_days = _get_review_count_for_ah_deck_since( + ah_did, now - timedelta(days=30) + ) + card_review_data.append( + CardReviewData( + ah_did=ah_did, + total_card_reviews_last_7_days=total_card_reviews_last_7_days, + total_card_reviews_last_30_days=total_card_reviews_last_30_days, + first_card_review_at=first_review_at, + last_card_review_at=last_review_at, ) + ) LOGGER.info(f"Review data: {card_review_data}") @@ -50,19 +48,17 @@ def send_review_data() -> None: def _get_review_count_for_ah_deck_since(ah_did: uuid.UUID, since: datetime) -> int: - """Get the number of reviews (recorded in Anki's review log table) for an ankihub deck since a given time. - Requires the ankihub db to be attached to the Anki db.""" + """Get the number of reviews (recorded in Anki's review log table) for an ankihub deck since a given time.""" timestamp_ms = int(datetime.timestamp(since) * 1000) + anki_nids = ankihub_db.anki_nids_for_ankihub_deck(ah_did) result = aqt.mw.col.db.scalar( - """ + f""" SELECT COUNT(*) FROM revlog as r JOIN cards as c ON r.cid = c.id - JOIN ankihub_db.notes as ah_n ON c.nid = ah_n.anki_note_id - WHERE r.id > ? AND ah_n.ankihub_deck_id = ? + WHERE r.id > ? AND c.nid IN ({','.join(map(str, anki_nids))}) """, timestamp_ms, - str(ah_did), ) return result @@ -70,17 +66,15 @@ def _get_review_count_for_ah_deck_since(ah_did: uuid.UUID, since: datetime) -> i def _get_first_and_last_review_datetime_for_ah_deck( ah_did: uuid.UUID, ) -> Optional[Tuple[datetime, datetime]]: - """Get the date time of the first and last review (recorded in Anki's review log table) for an ankihub deck. - Requires the ankihub db to be attached to the Anki db.""" + """Get the date time of the first and last review (recorded in Anki's review log table) for an ankihub deck.""" + anki_nids = ankihub_db.anki_nids_for_ankihub_deck(ah_did) row = aqt.mw.col.db.first( - """ + f""" SELECT MIN(r.id), MAX(r.id) FROM revlog as r JOIN cards as c ON r.cid = c.id - JOIN ankihub_db.notes as ah_n ON c.nid = ah_n.anki_note_id - WHERE ah_n.ankihub_deck_id = ? + WHERE c.nid IN ({','.join(map(str, anki_nids))}) """, - str(ah_did), ) if row[0] is None: return None diff --git a/tests/addon/test_unit.py b/tests/addon/test_unit.py index 8bec8fd0f..ff8484310 100644 --- a/tests/addon/test_unit.py +++ b/tests/addon/test_unit.py @@ -59,7 +59,6 @@ SuggestionType, TagGroupValidationResponse, ) -from ankihub.db import attached_ankihub_db from ankihub.db.db import _AnkiHubDB from ankihub.db.exceptions import IntegrityError from ankihub.feature_flags import _FeatureFlags, feature_flags @@ -1899,13 +1898,13 @@ def test_review_times_relative_to_since_time( record_review_for_anki_nid( NoteId(note_info.anki_nid), now + review_delta ) - with attached_ankihub_db(): - assert ( - _get_review_count_for_ah_deck_since( - ah_did=ah_did, since=now + since_time - ) - == expected_count + + assert ( + _get_review_count_for_ah_deck_since( + ah_did=ah_did, since=now + since_time ) + == expected_count + ) def test_with_multiple_notes( self, @@ -1925,11 +1924,10 @@ def test_with_multiple_notes( ) since_time = now - timedelta(days=1) - with attached_ankihub_db(): - assert ( - _get_review_count_for_ah_deck_since(ah_did=ah_did, since=since_time) - == 2 - ) + assert ( + _get_review_count_for_ah_deck_since(ah_did=ah_did, since=since_time) + == 2 + ) def test_with_review_for_other_deck( self, @@ -1952,13 +1950,10 @@ def test_with_review_for_other_deck( # Only the review for the first deck should be counted. since_time = now - timedelta(days=1) - with attached_ankihub_db(): - assert ( - _get_review_count_for_ah_deck_since( - ah_did=ah_did_1, since=since_time - ) - == 1 - ) + assert ( + _get_review_count_for_ah_deck_since(ah_did=ah_did_1, since=since_time) + == 1 + ) class TestGetLastReviewTimeForAHDeck: @@ -1994,26 +1989,25 @@ def test_review_times_relative_to_since_time( NoteId(note_info.anki_nid), now + review_delta ) - with attached_ankihub_db(): - first_and_last_time = _get_first_and_last_review_datetime_for_ah_deck( - ah_did=ah_did - ) + first_and_last_time = _get_first_and_last_review_datetime_for_ah_deck( + ah_did=ah_did + ) - if expected_last_review_delta is not None: - first_review_time, last_review_time = first_and_last_time + if expected_last_review_delta is not None: + first_review_time, last_review_time = first_and_last_time - expected_first_review_time = now + expected_first_review_delta - assert_datetime_equal_ignore_milliseconds( - first_review_time, - expected_first_review_time, - ) + expected_first_review_time = now + expected_first_review_delta + assert_datetime_equal_ignore_milliseconds( + first_review_time, + expected_first_review_time, + ) - expected_last_review_time = now + expected_last_review_delta - assert_datetime_equal_ignore_milliseconds( - last_review_time, expected_last_review_time - ) - else: - assert first_and_last_time is None + expected_last_review_time = now + expected_last_review_delta + assert_datetime_equal_ignore_milliseconds( + last_review_time, expected_last_review_time + ) + else: + assert first_and_last_time is None def test_with_multiple_notes( self, @@ -2036,11 +2030,10 @@ def test_with_multiple_notes( NoteId(note_info_2.anki_nid), expected_last_review_time ) - with attached_ankihub_db(): - ( - first_review_time, - last_review_time, - ) = _get_first_and_last_review_datetime_for_ah_deck(ah_did=ah_did) + ( + first_review_time, + last_review_time, + ) = _get_first_and_last_review_datetime_for_ah_deck(ah_did=ah_did) assert_datetime_equal_ignore_milliseconds( first_review_time, @@ -2073,17 +2066,16 @@ def test_with_review_for_other_deck( ) # Only the review for the first deck should be considered. - with attached_ankihub_db(): - ( - first_review_time, - last_review_time, - ) = _get_first_and_last_review_datetime_for_ah_deck(ah_did=ah_did_1) + ( + first_review_time, + last_review_time, + ) = _get_first_and_last_review_datetime_for_ah_deck(ah_did=ah_did_1) - assert first_review_time == last_review_time - assert_datetime_equal_ignore_milliseconds( - first_review_time, - expected_review_time, - ) + assert first_review_time == last_review_time + assert_datetime_equal_ignore_milliseconds( + first_review_time, + expected_review_time, + ) class TestSendReviewData: @@ -2127,6 +2119,29 @@ def test_with_two_reviews_for_one_deck( card_review_data.last_card_review_at, second_review_time ) + def test_without_reviews( + self, + anki_session_with_addon_data: AnkiSession, + install_ah_deck: InstallAHDeck, + mock_function: MockFunction, + ) -> None: + with anki_session_with_addon_data.profile_loaded(): + # We install the deck so that we get coverage for the case where a deck + # has no reviews. + install_ah_deck() + + send_card_review_data_mock = mock_function( + AnkiHubClient, "send_card_review_data" + ) + + send_review_data() + + # Assert that the correct data was passed to the client method. + send_card_review_data_mock.assert_called_once() + + review_data_list = send_card_review_data_mock.call_args[0][0] + assert review_data_list == [] + def test_clear_empty_cards(anki_session_with_addon_data: AnkiSession, qtbot: QtBot): with anki_session_with_addon_data.profile_loaded():