Skip to content

Commit

Permalink
fix: Don't attach ankihub DB for querying review data (#851)
Browse files Browse the repository at this point in the history 
* Don't attach db for querying review data

* Add test

* Edit test
  • Loading branch information
@RisingOrange
RisingOrange authored Dec 27, 2023
1 parent 7ad2168 commit 632b84c
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 89 deletions.
70 changes: 32 additions & 38 deletions ankihub/main/review_data.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,41 +7,39 @@
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


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}")

Expand All @@ -50,37 +48,33 @@ 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


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
Expand Down
117 changes: 66 additions & 51 deletions tests/addon/test_unit.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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:
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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():
Expand Down

0 comments on commit 632b84c

Please sign in to comment.