diff --git a/bin/work_reset_non_bisac_nonfiction_subjects b/bin/work_reset_non_bisac_nonfiction_subjects new file mode 100755 index 0000000000..00e84b51f1 --- /dev/null +++ b/bin/work_reset_non_bisac_nonfiction_subjects @@ -0,0 +1,11 @@ +#!/usr/bin/env python +"""Queue the reset_non_bisac_nonfiction_subjects Celery task. + +Convenience wrapper that manually dispatches the repair for BISAC subjects +stored as nonfiction in error (the `reset_non_bisac_nonfiction_subjects` Celery +task) for a worker to process. +""" + +from palace.manager.scripts.work import ResetNonBisacNonfictionSubjectsScript + +ResetNonBisacNonfictionSubjectsScript().run() diff --git a/src/palace/manager/celery/tasks/work.py b/src/palace/manager/celery/tasks/work.py index 32c10c52bf..de76d8cfc2 100644 --- a/src/palace/manager/celery/tasks/work.py +++ b/src/palace/manager/celery/tasks/work.py @@ -4,6 +4,7 @@ from sqlalchemy.orm import Session from palace.manager.celery.task import Task +from palace.manager.core.classifier.bisac import BISACClassifier from palace.manager.data_layer.policy.presentation import PresentationCalculationPolicy from palace.manager.service.celery.celery import QueueNames from palace.manager.sqlalchemy.model.classification import Classification, Subject @@ -39,6 +40,54 @@ def reclassify_null_audience_works(task: Task) -> None: session.commit() +@shared_task(queue=QueueNames.default, bind=True) +def reset_non_bisac_nonfiction_subjects(task: Task) -> None: + """Mark BISAC subjects unchecked when their stored fiction status went stale. + + A code that cannot be resolved to a canonical BISAC heading used to be + read as nonfiction by the ruleset catch-all, so those subjects carry a + fabricated fiction=False. Subjects are only re-examined when checked is + false, so repairing them means resetting that flag. + + This resets only. Re-scoring is classify_unchecked_subjects' job: the + startup task that runs this at deploy chains the two together, and the + nightly run picks up anything left over. + + Idempotent and self-selecting -- it recomputes which subjects the + classifier no longer agrees with, so a second run finds nothing to do. + That is what lets a later release re-apply it safely. + """ + with task.session() as session: + candidates = ( + session.query(Subject.id, Subject.identifier, Subject.name) + .filter( + Subject.type == Subject.BISAC, + Subject.checked == True, # noqa: E712 + Subject.fiction == False, # noqa: E712 + ) + .all() + ) + + stale_ids = [ + row.id + for row in candidates + if BISACClassifier.contradicts_stored_fiction( + row.identifier, row.name, False + ) + ] + + if stale_ids: + session.query(Subject).filter(Subject.id.in_(stale_ids)).update( + {Subject.checked: False}, synchronize_session=False + ) + session.commit() + + task.log.info( + f"Reset checked=False for {len(stale_ids)} of {len(candidates)} " + f"BISAC subjects stored as nonfiction." + ) + + @shared_task(queue=QueueNames.default, bind=True) def classify_unchecked_subjects(task: Task) -> None: """Reclassify all Works whose current classifications appear to diff --git a/src/palace/manager/core/classifier/bisac.py b/src/palace/manager/core/classifier/bisac.py index 57c19aee63..34d6f04739 100644 --- a/src/palace/manager/core/classifier/bisac.py +++ b/src/palace/manager/core/classifier/bisac.py @@ -1,5 +1,6 @@ import csv import re +from collections.abc import Callable, Sequence from frozendict import frozendict @@ -654,27 +655,130 @@ class BISACClassifier(Classifier): m(classifier.Life_Strategies, nonfiction, social_topics), ] + # The top-level headings a canonical BISAC name can begin with ("Fiction", + # "Juvenile Nonfiction", "Antiques & Collectibles", ...). + TOP_LEVEL_HEADINGS: frozenset[str] = frozenset( + Lowercased(name.split("/")[0].strip()) for name in NAMES.values() + ) | frozenset( + # Former top-level spellings of renamed categories, which distributors + # still send and bisac.csv no longer lists. They belong here so that a + # name beginning with one still reaches the catch-all rules: without + # the entry, a deprecated spelling carried by a code that does not + # resolve abstains instead of being read as nonfiction/Adult. That is + # the test for whether a new entry belongs -- not whether the rulesets + # have an Interchangeable for it, which is a separate mechanism in + # GENRE, and GENRE does not consult this set. + Lowercased(name) + for name in ( + "Mind & Spirit", + "Psychology & Psychiatry", + "Technology", + "Foreign Language Study", + "Literary Criticism & Collections", + ) + ) + + @classmethod + def _has_canonical_heading(cls, name: list[str]) -> bool: + """Does `name` begin with a real BISAC top-level heading? + + This is the premise the FICTION and AUDIENCE catch-all rules rest on, + so it is what has to be checked before running them. It holds for a + name that came from `NAMES`, and for a heading a distributor supplied + in the identifier field -- an OPDS `` + with no `label` arrives that way, since the OPDS1 extractor maps `term` + to the identifier and `label` to the name. A bare top-level heading + such as "Juvenile" is equally a heading. + + It does not hold for the fragment left over when a code cannot be + resolved ("Historical", "English literature"), which is the case these + rulesets must not be applied to. + """ + return bool(name) and name[0] in cls.TOP_LEVEL_HEADINGS + + @classmethod + def contradicts_stored_fiction( + cls, + identifier: str | None, + name: str | None, + stored_fiction: bool | None, + ) -> bool: + """Does this classifier disagree with a subject's stored fiction status? + + Subjects are only re-examined when `checked` is false, so a value + scored under superseded rules persists indefinitely. Repairs that + reset `checked` need to identify those rows, and they need to agree + with each other about which rows they are. Expressing the question + here keeps that definition in one place: a subject is stale when the + classifier, run now, does not return what is stored. + + :param identifier: The subject's identifier, as stored. + :param name: The subject's name, as stored. + :param stored_fiction: The subject's current `fiction` value. + :return: True when the classifier no longer agrees with `stored_fiction`. + """ + if not identifier and not name: + # Nothing to classify. Subject.lookup will not create such a row, + # but both columns are nullable, so do not assume. + return False + scrubbed_identifier, scrubbed_name = cls.scrub_identifier_and_name( + identifier, name + ) + return cls.is_fiction(scrubbed_identifier, scrubbed_name) is not stored_fiction + + @classmethod + def _apply_rulesets[RulesetResult]( + cls, + identifier: str | None, + name: list[str], + rulesets: Sequence[MatchingRule], + keyword_fallback: Callable[[str | None, str], RulesetResult | None], + ) -> RulesetResult | None: + """Match `name` against `rulesets`, falling back to keyword matching. + + Both the FICTION and AUDIENCE rulesets end in a catch-all that reasons + from the top-level BISAC heading -- "not filed under Fiction, therefore + nonfiction", "no juvenile heading, therefore Adult". That inference + holds for a canonical BISAC name and for nothing else, so an + unrecognized code skips the rulesets entirely and is left to the keyword + classifier, which abstains when the distributor's name carries no + signal. + + Only those two callers share this. `genre` must not skip its rulesets: + GENRE has no catch-all but does have rules that match a bare fragment, + so a subject named "Historical" is still a Historical Fiction signal + even though "Historical" is not a top-level heading. `target_age` is + left out for scope rather than correctness -- its rules key off a + juvenile first token, which only a real heading produces, so routing it + through here would be safe. + """ + # A subject with no identifier had no code that could fail to resolve, + # so there is nothing here to protect it from -- and some distributors + # classify entirely this way. Bibliotheca sends every genre as a bare + # name ("Action & Adventure", "Magic") with no code at all; gating + # those on the heading would leave its titles with no fiction evidence + # whatsoever. Whether a bare sub-heading should carry the rulesets' + # top-level inference is a real question, but a separate one. + if not identifier or cls._has_canonical_heading(name): + for ruleset in rulesets: + result = ruleset.match(*name) + if result is cls.stop: + return None + if result is not None: + return result + return keyword_fallback(identifier, "/".join(name)) + @classmethod def is_fiction(cls, identifier, name): - for ruleset in cls.FICTION: - fiction = ruleset.match(*name) - if fiction is cls.stop: - return None - if fiction is not None: - return fiction - keyword = "/".join(name) - return KeywordBasedClassifier.is_fiction(identifier, keyword) + return cls._apply_rulesets( + identifier, name, cls.FICTION, KeywordBasedClassifier.is_fiction + ) @classmethod def audience(cls, identifier, name): - for ruleset in cls.AUDIENCE: - audience = ruleset.match(*name) - if audience is cls.stop: - return None - if audience is not None: - return audience - keyword = "/".join(name) - return KeywordBasedClassifier.audience(identifier, keyword) + return cls._apply_rulesets( + identifier, name, cls.AUDIENCE, KeywordBasedClassifier.audience + ) @classmethod def target_age(cls, identifier, name): @@ -725,10 +829,14 @@ def scrub_identifier(cls, identifier): identifier = identifier.removeprefix("FB") # Some distributors (e.g. Palace Marketplace) append an "N" suffix to # standard BISAC codes (e.g. "FBJUV000000N" becomes "JUV000000N" after - # FB-stripping). Official BISAC codes always end with digits, so a - # trailing "N" is always a non-standard extension; strip it so the code - # resolves to its canonical entry. - identifier = identifier.removesuffix("N") + # FB-stripping). Strip it only when doing so produces a code we know, + # because the identifier field does not always hold a code: a heading + # can arrive there too, and stripping unconditionally would turn + # "FICTION" into "FICTIO" (likewise RELIGION, EDUCATION, DESIGN, + # TRANSPORTATION) and stop it being recognized as a heading. + stripped = identifier.removesuffix("N") + if stripped in cls.NAMES or stripped in cls.NON_STANDARD_CODE_ALIASES: + identifier = stripped # Remap any remaining non-standard codes to their canonical equivalents. identifier = cls.NON_STANDARD_CODE_ALIASES.get(identifier, identifier) if identifier in cls.NAMES: diff --git a/src/palace/manager/scripts/work.py b/src/palace/manager/scripts/work.py index 0c848b3d2b..2d33297957 100644 --- a/src/palace/manager/scripts/work.py +++ b/src/palace/manager/scripts/work.py @@ -11,6 +11,7 @@ from palace.manager.celery.tasks.work import ( classify_unchecked_subjects, reclassify_null_audience_works, + reset_non_bisac_nonfiction_subjects, ) from palace.manager.data_layer.policy.presentation import ( PresentationCalculationPolicy, @@ -252,6 +253,25 @@ class WorkOPDSScript(WorkPresentationScript): ) +class ResetNonBisacNonfictionSubjectsScript(Script): + """Manually dispatch the ``reset_non_bisac_nonfiction_subjects`` Celery task. + + The work itself happens in the Celery task; this script just queues it. It + exists so the repair can be applied again on demand, in case its reset was + consumed by old code before the new classifier was live everywhere. + + TODO: Remove this script when the ``reset_non_bisac_nonfiction_subjects`` + Celery task is removed. + """ + + def do_run(self, *args: Any, **kwargs: Any) -> None: + reset_non_bisac_nonfiction_subjects.delay() + self.log.info( + 'The "reset_non_bisac_nonfiction_subjects" task has been queued for ' + "execution. See the celery logs for details about task execution." + ) + + class ReclassifyNullAudienceWorksScript(Script): """Manually dispatch the ``reclassify_null_audience_works`` Celery task. diff --git a/src/palace/manager/sqlalchemy/model/work.py b/src/palace/manager/sqlalchemy/model/work.py index 7bf6dae00f..d65ec259d3 100644 --- a/src/palace/manager/sqlalchemy/model/work.py +++ b/src/palace/manager/sqlalchemy/model/work.py @@ -1368,6 +1368,16 @@ def calculate_quality(self, identifier_ids, default_quality=0): if new_quality != self.quality: self.quality = new_quality + @staticmethod + def _keep_known_value[T](old_value: T | None, new_value: T | None) -> T | None: + """Prefer `new_value`, but never replace a known value with None. + + Handed to the classifier as its default, this leaves an existing + determination alone when a recalculation gathers no usable + classifications, rather than erasing it. + """ + return old_value if new_value is None else new_value + def assign_genres( self, identifier_ids, @@ -1392,8 +1402,16 @@ def assign_genres( for classification in classifications: classifier.add(classification) + # Hand the retained values to the classifier as its defaults rather + # than restoring them after it has run. classify() derives the genres + # from the fiction status and the target age from the audience, so a + # value restored afterwards arrives too late to be consulted: the genre + # filter would already have run with None -- which disables it, keeping + # genres of either fiction status -- and we would then stamp the + # retained status back onto a work whose own genres now contradict it. (genre_weights, new_fiction, new_audience, target_age) = classifier.classify( - default_fiction=default_fiction, default_audience=default_audience + default_fiction=self._keep_known_value(old_fiction, default_fiction), + default_audience=self._keep_known_value(old_audience, default_audience), ) new_target_age = tuple_to_numericrange(target_age) @@ -1402,12 +1420,7 @@ def assign_genres( if new_fiction != old_fiction: self.fiction = new_fiction - # Never let a recalculation erase a known audience. If the classifier - # came back with no audience (e.g. it gathered no usable - # classifications on this pass), keep whatever we already had rather - # than writing NULL over a previously-determined audience. - if new_audience is None and old_audience is not None: - new_audience = old_audience + if new_audience != old_audience: self.audience = new_audience diff --git a/startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py b/startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py new file mode 100644 index 0000000000..d8b29c1e21 --- /dev/null +++ b/startup_tasks/2026_09_14_reclassify_non_bisac_nonfiction_subjects.py @@ -0,0 +1,54 @@ +"""Repair BISAC subjects stored as nonfiction because their code did not resolve. + +Everything on the Palace Marketplace / Feedbooks category scheme is stored with +``type='BISAC'``, including codes that are not BISAC at all -- language and +territory categories such as ``INFEN000`` ("English literature"). Those cannot +be resolved to a canonical heading, so classification used to infer nonfiction +from the distributor's name and store ``fiction=False``. The classifier no +longer does that, which leaves the stored values stale. + +Subjects are only re-examined when ``checked`` is false, so this dispatches two +steps: ``reset_non_bisac_nonfiction_subjects`` marks the stale ones unchecked, +then ``classify_unchecked_subjects`` re-scores them and recalculates their +works. The second signature is immutable so the chain does not pass the first +task's return value into it. + +Doing both here matters. The reset on its own is exposed: anything reaching +``Subject.assign_to_genre`` before the re-score consumes it, and code running +the superseded rules re-stamps ``checked=True`` with the same wrong value. +Nothing errors and nothing revisits the subject afterwards, so the repair +silently did nothing, having paid for a reindex to do it. Chaining the re-score +closes that gap to seconds rather than waiting for the nightly run. + +The timing works out. ``helpers/migrate.yml`` stops the scripts container -- +where every Celery worker and beat run -- before migrating, and starts it again +from the new image afterwards, so the worker that picks this up is necessarily +new code. + +Web containers are the remaining exposure: the deploy recycles them after the +migration step, and they can reach ``assign_to_genre`` through a presentation +recalculation. Fargate deployments are not governed by that playbook at all. A +second startup task re-applies the reset a release later, once no old code is +running anywhere. + +TODO: Remove this task once it has run on all deployments (PP-5129).""" + +from __future__ import annotations + +import logging + +from celery.canvas import Signature, chain +from sqlalchemy.orm import Session + +from palace.manager.celery.tasks.work import ( + classify_unchecked_subjects, + reset_non_bisac_nonfiction_subjects, +) +from palace.manager.service.container import Services + + +def run(services: Services, session: Session, log: logging.Logger) -> Signature | None: + return chain( + reset_non_bisac_nonfiction_subjects.s(), + classify_unchecked_subjects.si(), + ) diff --git a/tests/manager/celery/tasks/test_work.py b/tests/manager/celery/tasks/test_work.py index d7923c5fc5..3a662a7f5a 100644 --- a/tests/manager/celery/tasks/test_work.py +++ b/tests/manager/celery/tasks/test_work.py @@ -1,5 +1,7 @@ from unittest.mock import patch +import pytest + from palace.manager.celery.tasks import work as work_tasks from palace.manager.sqlalchemy.model.classification import Subject from palace.manager.sqlalchemy.model.work import Work @@ -119,3 +121,95 @@ def test_reclassify_null_audience_works( policy = call_obj[1]["policy"] assert policy.classify is True assert policy.choose_edition is False + + +@pytest.mark.parametrize( + "identifier,name", + [ + pytest.param("INFEN000", "English literature", id="language_category"), + pytest.param( + "INFENUSA", "American and Canadian literature", id="territory_category" + ), + pytest.param("FBSACT000000", "News and investigations", id="vendor_code"), + pytest.param("FSHUM000000N", "Human science", id="vendor_code_with_n_suffix"), + pytest.param("SOCO32000", None, id="malformed_code_without_name"), + pytest.param("FBZZZ000000", "Historical", id="shape_valid_but_nonexistent"), + pytest.param("FBFIC014000", "Historical", id="stale_canonical_fiction_code"), + ], +) +def test_reset_non_bisac_nonfiction_subjects( + db: DatabaseTransactionFixture, + celery_fixture: CeleryFixture, + identifier: str, + name: str | None, +) -> None: + """Subjects stored as nonfiction that the classifier no longer scores that + way are marked unchecked, so classify_unchecked_subjects re-scores them. + + Covers both codes that are not BISAC at all and codes that merely look like + one (FBZZZ000000), which a pattern-based predicate would wrongly accept. + """ + subject = db.subject(Subject.BISAC, identifier) + subject.name = name + subject.fiction = False + subject.checked = True + db.session.commit() + + work_tasks.reset_non_bisac_nonfiction_subjects.delay().wait() + db.session.expire_all() + + assert subject.checked is False + + +@pytest.mark.parametrize( + "subject_type,identifier,fiction", + [ + pytest.param(Subject.BISAC, "HIS027000", False, id="real_nonfiction_code"), + pytest.param(Subject.BISAC, "HIS000000", False, id="real_nonfiction_general"), + pytest.param( + Subject.BISAC, "HISTORY / General", False, id="nonfiction_heading" + ), + pytest.param(Subject.BISAC, "INFEN000", True, id="already_scored_fiction"), + pytest.param(Subject.BISAC, "INFEN000", None, id="already_scored_unknown"), + pytest.param(Subject.TAG, "INFEN000", False, id="not_a_bisac_subject"), + ], +) +def test_reset_non_bisac_nonfiction_subjects_leaves_everything_else_checked( + db: DatabaseTransactionFixture, + celery_fixture: CeleryFixture, + subject_type: str, + identifier: str, + fiction: bool | None, +) -> None: + """Legitimate nonfiction codes keep their value, and subjects outside the + examined set -- already scored as fiction or unknown, or not BISAC-typed -- + are not touched.""" + subject = db.subject(subject_type, identifier) + subject.fiction = fiction + subject.checked = True + db.session.commit() + + work_tasks.reset_non_bisac_nonfiction_subjects.delay().wait() + db.session.expire_all() + + assert subject.checked is True + + +def test_reset_non_bisac_nonfiction_subjects_is_idempotent( + db: DatabaseTransactionFixture, + celery_fixture: CeleryFixture, +): + """A second run finds nothing left to do and leaves the reset in place.""" + subject = db.subject(Subject.BISAC, "INFEN000") + subject.name = "English literature" + subject.fiction = False + subject.checked = True + db.session.commit() + + work_tasks.reset_non_bisac_nonfiction_subjects.delay().wait() + db.session.expire_all() + assert subject.checked is False + + work_tasks.reset_non_bisac_nonfiction_subjects.delay().wait() + db.session.expire_all() + assert subject.checked is False diff --git a/tests/manager/core/classifiers/test_bisac.py b/tests/manager/core/classifiers/test_bisac.py index c7b166ac1d..252ac57a63 100644 --- a/tests/manager/core/classifiers/test_bisac.py +++ b/tests/manager/core/classifiers/test_bisac.py @@ -1,6 +1,6 @@ import pytest -from palace.manager.core.classifier import Classifier +from palace.manager.core.classifier import Classifier, Lowercased from palace.manager.core.classifier.bisac import ( RE, BISACClassifier, @@ -392,6 +392,235 @@ def test_palace_marketplace_n_suffix_codes( assert subject.audience == Classifier.AUDIENCE_CHILDREN assert subject.fiction is True + @pytest.mark.parametrize( + "identifier,stored_name", + [ + pytest.param("INFEN000", None, id="no_name"), + pytest.param("INFEN000", "English", id="language_name"), + pytest.param("INFENUSA", "USA", id="territory_name"), + pytest.param("FBINFEN000", "English", id="fb_prefixed"), + pytest.param("FBZZZ000000", "Historical", id="partial_bisac_heading"), + ], + ) + def test_unrecognized_code_abstains( + self, identifier: str, stored_name: str | None + ) -> None: + """A code that is not BISAC carries no fiction or audience signal. + + Everything on the Feedbooks/Palace Marketplace category scheme arrives + typed as BISAC, including codes that describe language or territory + rather than subject matter. When such a code cannot be resolved, the + name we fall back on is a distributor fragment, so the rulesets' "not + filed under Fiction, therefore nonfiction" and "no juvenile heading, + therefore Adult" inferences do not apply. Abstaining keeps an + unresolvable code from voting against the codes that did resolve. + """ + subject = self._subject(identifier, stored_name) + assert subject.fiction is None + assert subject.audience is None + + @pytest.mark.parametrize( + "stored_name,expected_fiction,expected_audience", + [ + pytest.param("Science Fiction", True, None, id="fiction_signal"), + pytest.param("Nonfiction", False, None, id="nonfiction_signal"), + pytest.param( + "Juvenile Fiction", True, Classifier.AUDIENCE_CHILDREN, id="juvenile" + ), + pytest.param( + "Young Adult Fiction", + True, + Classifier.AUDIENCE_YOUNG_ADULT, + id="young_adult", + ), + ], + ) + def test_unrecognized_code_still_uses_keyword_fallback( + self, stored_name: str, expected_fiction: bool, expected_audience: str | None + ) -> None: + """Abstaining is not the same as ignoring the name. + + An unrecognized code skips the BISAC rulesets but still goes through + the keyword classifier, so a distributor name that does carry a signal + is honored -- for audience as well as fiction status. A juvenile or YA + heading keeps its audience; a name with no audience signal abstains + rather than falling to the rulesets' Adult catch-all. + """ + subject = self._subject("INFEN000", stored_name) + assert subject.fiction is expected_fiction + assert subject.audience == expected_audience + + @pytest.mark.parametrize( + "identifier,expected_fiction", + [ + pytest.param("FICTION / Horror", True, id="fiction_heading"), + pytest.param( + "FICTION / Science Fiction / Time Travel", True, id="deep_heading" + ), + pytest.param("HISTORY / General", False, id="nonfiction_heading"), + pytest.param( + "Antiques & Collectibles / Kitchenware", False, id="mixed_case_heading" + ), + # 29 of the 56 top-level headings are a single unpunctuated word, + # so a heading in the identifier field does not always look like a + # heading. "Juvenile" is the one whose full canonical name is a + # single word (JUV037020). + pytest.param("Juvenile", False, id="single_word_juvenile"), + pytest.param("Fiction", True, id="single_word_fiction"), + # Uppercase is what Boundless sends, and these end in "N" -- the + # suffix scrub_identifier strips from codes like FBJUV000000N. + pytest.param("FICTION", True, id="single_word_uppercase"), + pytest.param("RELIGION", False, id="single_word_uppercase_religion"), + pytest.param("History", False, id="single_word_history"), + pytest.param("Humor", None, id="single_word_stop_rule"), + ], + ) + def test_heading_in_identifier_field_is_matched_as_a_name( + self, identifier: str, expected_fiction: bool | None + ) -> None: + """A BISAC heading can arrive in the identifier field. + + An OPDS `` with no `label` lands that + way: the OPDS1 extractor maps `term` to the identifier and `label` to + the name. That is a heading, not a code that failed to resolve, so it + must still be matched against the rulesets -- including when the + heading is a single word and so is indistinguishable from a code by + shape alone. + """ + subject = self._subject(identifier, None) + assert subject.fiction is expected_fiction + assert subject.audience == Classifier.AUDIENCE_ADULT + + @pytest.mark.parametrize( + "name", + [ + pytest.param("Action & Adventure", id="bibliotheca_fiction_genre"), + pytest.param("Magic", id="bibliotheca_magic"), + pytest.param("Health", id="bibliotheca_nonfiction_genre"), + pytest.param("Horror", id="bare_subheading"), + ], + ) + def test_name_only_subject_still_reaches_the_rulesets(self, name: str) -> None: + """A subject with no identifier keeps the behaviour it always had. + + The heading gate exists to stop the rulesets being applied to the + fragment left behind when a code fails to resolve. A subject that never + carried a code has no such fragment, and some distributors classify + entirely this way -- Bibliotheca sends every genre as a bare name with + no code, so gating these would leave its titles with no fiction + evidence at all and a NULL fiction status on first import. + + Whether a bare sub-heading should carry the rulesets' top-level + inference is a real question, but a distributor-wide one that is not + this change's to answer. + """ + subject = self._subject("", name) + assert subject.fiction is False + assert subject.audience == Classifier.AUDIENCE_ADULT + + @pytest.mark.parametrize( + "heading", + [ + pytest.param("Fiction", id="fiction"), + pytest.param("Juvenile Nonfiction", id="two_words"), + pytest.param("Antiques & Collectibles", id="punctuated"), + pytest.param("Psychology & Psychiatry", id="deprecated_spelling"), + pytest.param( + "Literary Criticism & Collections", id="deprecated_spelling_lit_crit" + ), + ], + ) + def test_top_level_headings_recognized(self, heading: str) -> None: + """TOP_LEVEL_HEADINGS is what decides whether the rulesets apply. + + It is derived from bisac.csv, plus the former spellings of renamed + categories that are no longer in the file but that distributors still + send -- those have Interchangeable tokens in the rulesets, so the set + has to agree with them. + """ + assert Lowercased(heading) in BISACClassifier.TOP_LEVEL_HEADINGS + + @pytest.mark.parametrize( + "fragment", + [ + pytest.param("Historical", id="subheading_only"), + pytest.param("English literature", id="vendor_language_category"), + pytest.param("infen000", id="raw_code"), + ], + ) + def test_fragments_are_not_top_level_headings(self, fragment: str) -> None: + """The leftovers from an unresolvable code are not headings. + + These are exactly the values the rulesets must not be applied to: the + catch-alls would read them as "not filed under Fiction" and vote + nonfiction. + """ + assert Lowercased(fragment) not in BISACClassifier.TOP_LEVEL_HEADINGS + + @pytest.mark.parametrize( + "identifier,name,stored_fiction,expected", + [ + pytest.param( + "INFEN000", "English literature", False, True, id="vendor_code_is_stale" + ), + pytest.param( + "FBZZZ000000", "Historical", False, True, id="unreal_code_is_stale" + ), + pytest.param( + "FBFIC014000", + "Historical", + False, + True, + id="real_fiction_code_is_stale", + ), + pytest.param("HIS027000", None, False, False, id="real_nonfiction_agrees"), + pytest.param("FBFIC014000", "Historical", True, False, id="fiction_agrees"), + pytest.param( + "INFEN000", "English literature", True, False, id="keyword_agrees" + ), + pytest.param(None, None, False, False, id="nothing_to_classify"), + ], + ) + def test_contradicts_stored_fiction( + self, + identifier: str | None, + name: str | None, + stored_fiction: bool | None, + expected: bool, + ) -> None: + """The shared definition of a subject whose stored value went stale. + + Subjects are only re-examined when `checked` is false, so the repairs + that reset it need one definition of which rows are affected. Both the + migration and the re-run task ask this. + """ + assert ( + BISACClassifier.contradicts_stored_fiction(identifier, name, stored_fiction) + is expected + ) + + @pytest.mark.parametrize( + "identifier,stored_name", + [ + pytest.param("FBFIC000000", "Fiction", id="fiction_general"), + pytest.param("FBFIC014000", "Historical", id="historical"), + pytest.param("FBFIC016000", "Humorous", id="humorous"), + pytest.param("FBFIC019000", "Literary", id="literary"), + ], + ) + def test_recognized_code_unaffected_by_abstention( + self, identifier: str, stored_name: str + ) -> None: + """Codes that do resolve are classified exactly as before. + + These are the Palace Marketplace codes whose partial names + ("Historical", "Literary") would each vote nonfiction if the canonical + lookup were ever to miss. + """ + subject = self._subject(identifier, stored_name) + assert subject.fiction is True + assert subject.audience == Classifier.AUDIENCE_ADULT + @pytest.mark.parametrize( "identifier,expected", [ diff --git a/tests/manager/core/classifiers/test_classifier.py b/tests/manager/core/classifiers/test_classifier.py index 4960e09241..3973a6b212 100644 --- a/tests/manager/core/classifiers/test_classifier.py +++ b/tests/manager/core/classifiers/test_classifier.py @@ -827,6 +827,38 @@ def test_fiction_status_restricts_genre( genres = data.classifier.genres(False) assert [(nonfiction_genre.genredata, 100)] == list(genres.items()) + def test_unrecognized_bisac_codes_do_not_imply_nonfiction( + self, work_classifier_fixture: TestWorkClassifierFixture + ) -> None: + """An unresolvable BISAC code must not cast a nonfiction vote. + + Everything on the Palace Marketplace category scheme arrives typed as + BISAC, including codes that describe language or territory rather than + subject matter. Each of those used to vote nonfiction, so enough of + them could outvote the genuine fiction codes on the same book. + """ + data = work_classifier_fixture + session = data.transaction.session + source = DataSource.lookup(session, DataSource.GUTENBERG, autocreate=True) + + for code, name in [("INFEN000", "English"), ("INFENUSA", "USA")]: + data.classifier.add( + data.identifier.classify(source, Subject.BISAC, code, name, weight=1) + ) + + # No vote either way, so no determination. + assert 0 == data.classifier.fiction_weights[False] + assert 0 == data.classifier.fiction_weights[True] + assert data.classifier.fiction() is None + + # A single code that does resolve is now decisive. + data.classifier.add( + data.identifier.classify( + source, Subject.BISAC, "FBFIC014000", "Historical", weight=1 + ) + ) + assert data.classifier.fiction() is True + def test_genres_consolidated_before_classification( self, work_classifier_fixture: TestWorkClassifierFixture ): diff --git a/tests/manager/scripts/test_work.py b/tests/manager/scripts/test_work.py index 14dda5de47..58fb519207 100644 --- a/tests/manager/scripts/test_work.py +++ b/tests/manager/scripts/test_work.py @@ -10,6 +10,7 @@ from palace.manager.scripts.work import ( ReclassifyNullAudienceWorksScript, ReclassifyWorksForUncheckedSubjectsScript, + ResetNonBisacNonfictionSubjectsScript, WorkProcessingScript, ) from palace.manager.sqlalchemy.model.datasource import DataSource @@ -198,3 +199,13 @@ def test_run(self, db: DatabaseTransactionFixture): ) as task: ReclassifyNullAudienceWorksScript(db.session).run() assert task.delay.call_count == 1 + + +class TestResetNonBisacNonfictionSubjectsScript: + def test_run(self, db: DatabaseTransactionFixture): + """The script queues the reset_non_bisac_nonfiction_subjects Celery task.""" + with patch( + "palace.manager.scripts.work.reset_non_bisac_nonfiction_subjects" + ) as task: + ResetNonBisacNonfictionSubjectsScript(db.session).run() + assert task.delay.call_count == 1 diff --git a/tests/manager/sqlalchemy/model/test_work.py b/tests/manager/sqlalchemy/model/test_work.py index 96d6209c98..122029402e 100644 --- a/tests/manager/sqlalchemy/model/test_work.py +++ b/tests/manager/sqlalchemy/model/test_work.py @@ -389,6 +389,54 @@ def test_assign_genres_does_not_overwrite_audience_with_null( assert work.audience == Classifier.AUDIENCE_CHILDREN + def test_assign_genres_does_not_overwrite_fiction_with_null( + self, db: DatabaseTransactionFixture + ) -> None: + # Defensive invariant: when a recalculation pass reaches no fiction + # determination (e.g. every classification it gathered abstained), a + # previously-determined fiction status must be kept rather than + # overwritten with NULL. This matters more now that an unresolvable + # BISAC code abstains instead of voting nonfiction. + work = db.work(with_license_pool=True) + work.fiction = True + db.session.commit() + + # Force the no-evidence + null-default path directly. + work.assign_genres(work._direct_identifier_ids, default_fiction=None) + + assert work.fiction is True + + @pytest.mark.parametrize( + "stored_fiction,expected_genres", + [ + pytest.param(False, [], id="nonfiction-work-drops-the-fiction-genre"), + pytest.param(True, ["Horror"], id="fiction-work-keeps-the-fiction-genre"), + ], + ) + def test_assign_genres_filters_genres_against_the_retained_fiction_status( + self, + db: DatabaseTransactionFixture, + stored_fiction: bool, + expected_genres: list[str], + ) -> None: + # A tag contributes a genre without voting on fiction status, so a work + # whose BISAC codes all abstain reaches the genre filter with no fiction + # determination at all -- and the filter is a no-op when fiction is + # None, keeping genres of either status. The retained status therefore + # has to be in hand *before* the filter runs; restoring it afterwards + # would leave the work stamped with a status its own genres contradict. + work = db.work(with_license_pool=True) + work.fiction = stored_fiction + work.license_pools[0].identifier.classify( + DataSource.lookup(db.session, DataSource.OCLC), Subject.TAG, "Horror" + ) + db.session.commit() + + work.assign_genres(work._direct_identifier_ids, default_fiction=None) + + assert work.fiction is stored_fiction + assert sorted(genre.name for genre in work.genres) == expected_genres + def test__choose_summary(self, db: DatabaseTransactionFixture): # Test the _choose_summary helper method, called by # calculate_presentation().