From b6090d2c928ed88473f8e5bc27ea07ab9cc7653c Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 2 Dec 2024 11:17:16 -0800 Subject: [PATCH 1/5] Bump pyfakefs from 5.7.1 to 5.7.2 (#2203) Bumps [pyfakefs](https://github.com/pytest-dev/pyfakefs) from 5.7.1 to 5.7.2. - [Release notes](https://github.com/pytest-dev/pyfakefs/releases) - [Changelog](https://github.com/pytest-dev/pyfakefs/blob/main/CHANGES.md) - [Commits](https://github.com/pytest-dev/pyfakefs/compare/v5.7.1...v5.7.2) --- updated-dependencies: - dependency-name: pyfakefs dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- poetry.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/poetry.lock b/poetry.lock index 37ca6cfd5..2e7403f96 100644 --- a/poetry.lock +++ b/poetry.lock @@ -3489,13 +3489,13 @@ yaml = ["pyyaml (>=6.0.1)"] [[package]] name = "pyfakefs" -version = "5.7.1" +version = "5.7.2" description = "pyfakefs implements a fake file system that mocks the Python file system modules." optional = false python-versions = ">=3.7" files = [ - {file = "pyfakefs-5.7.1-py3-none-any.whl", hash = "sha256:6503ffe7f401701cf974b502311f926da2b0657a72244a6ba36e985ceb3dd783"}, - {file = "pyfakefs-5.7.1.tar.gz", hash = "sha256:24774c632f3b67ea26fd56b08115ba7c339d5cd65655410bca8572d73a1ae9a4"}, + {file = "pyfakefs-5.7.2-py3-none-any.whl", hash = "sha256:e1527b0e8e4b33be52f0b024ca1deb269c73eecd68457c6b0bf608d6dab12ebd"}, + {file = "pyfakefs-5.7.2.tar.gz", hash = "sha256:40da84175c5af8d9c4f3b31800b8edc4af1e74a212671dd658b21cc881c60000"}, ] [[package]] From c2af3a405a7c4f7fa604b1272a33a7bb0607d31c Mon Sep 17 00:00:00 2001 From: dbernstein Date: Mon, 2 Dec 2024 11:40:32 -0800 Subject: [PATCH 2/5] [PP-1914] Remove collection facet from feed. (#2182) --- src/palace/manager/api/lanes.py | 4 - src/palace/manager/core/facets.py | 16 -- .../integration/configuration/library.py | 30 +--- src/palace/manager/search/external_search.py | 8 - src/palace/manager/sqlalchemy/model/lane.py | 64 +------- tests/manager/api/controller/test_work.py | 4 +- tests/manager/api/test_lanes.py | 24 +-- tests/manager/feed/test_library_annotator.py | 4 +- .../feed/test_opds_acquisition_feed.py | 48 +----- tests/manager/search/test_external_search.py | 52 +------ tests/manager/sqlalchemy/model/test_lane.py | 145 +++--------------- .../manager/sqlalchemy/model/test_library.py | 2 - 12 files changed, 44 insertions(+), 357 deletions(-) diff --git a/src/palace/manager/api/lanes.py b/src/palace/manager/api/lanes.py index c071bc189..ecf861f10 100644 --- a/src/palace/manager/api/lanes.py +++ b/src/palace/manager/api/lanes.py @@ -1087,7 +1087,6 @@ def overview_facets(self, _db, facets): # the best recommendations are in the front. return Facets.default( self.get_library(_db), - collection=facets.COLLECTION_FULL, availability=facets.AVAILABLE_ALL, entrypoint=facets.entrypoint, ) @@ -1157,7 +1156,6 @@ def overview_facets(self, _db, facets): """ return SeriesFacets.default( self.get_library(_db), - collection=facets.COLLECTION_FULL, availability=facets.AVAILABLE_ALL, entrypoint=facets.entrypoint, ) @@ -1225,7 +1223,6 @@ def overview_facets(self, _db, facets): """ return ContributorFacets.default( self.get_library(_db), - collection=facets.COLLECTION_FULL, availability=facets.AVAILABLE_ALL, entrypoint=facets.entrypoint, ) @@ -1354,7 +1351,6 @@ class CrawlableFacets(Facets): SETTINGS = { Facets.ORDER_FACET_GROUP_NAME: Facets.ORDER_LAST_UPDATE, Facets.AVAILABILITY_FACET_GROUP_NAME: Facets.AVAILABLE_ALL, - Facets.COLLECTION_FACET_GROUP_NAME: Facets.COLLECTION_FULL, Facets.DISTRIBUTOR_FACETS_GROUP_NAME: Facets.DISTRIBUTOR_ALL, Facets.COLLECTION_NAME_FACETS_GROUP_NAME: Facets.COLLECTION_NAME_ALL, } diff --git a/src/palace/manager/core/facets.py b/src/palace/manager/core/facets.py index 692967fca..b61b8f2b6 100644 --- a/src/palace/manager/core/facets.py +++ b/src/palace/manager/core/facets.py @@ -8,15 +8,6 @@ class FacetConstants: ENTRY_POINT_REL = "http://librarysimplified.org/terms/rel/entrypoint" ENTRY_POINT_FACET_GROUP_NAME = "entrypoint" - # Subset the collection, roughly, by quality. - COLLECTION_FACET_GROUP_NAME = "collection" - COLLECTION_FULL = "full" - COLLECTION_FEATURED = "featured" - COLLECTION_FACETS = [ - COLLECTION_FULL, - COLLECTION_FEATURED, - ] - # Subset the collection by availability. AVAILABILITY_FACET_GROUP_NAME = "available" AVAILABLE_NOW = "now" @@ -63,7 +54,6 @@ class FacetConstants: COLLECTION_NAME_ALL = "All" FACETS_BY_GROUP = { - COLLECTION_FACET_GROUP_NAME: COLLECTION_FACETS, AVAILABILITY_FACET_GROUP_NAME: AVAILABILITY_FACETS, ORDER_FACET_GROUP_NAME: ORDER_FACETS, } @@ -71,7 +61,6 @@ class FacetConstants: GROUP_DISPLAY_TITLES = { ORDER_FACET_GROUP_NAME: _("Sort by"), AVAILABILITY_FACET_GROUP_NAME: _("Availability"), - COLLECTION_FACET_GROUP_NAME: _("Collection"), DISTRIBUTOR_FACETS_GROUP_NAME: _("Distributor"), COLLECTION_NAME_FACETS_GROUP_NAME: _("Collection Name"), } @@ -79,7 +68,6 @@ class FacetConstants: GROUP_DESCRIPTIONS = { ORDER_FACET_GROUP_NAME: _("Allow patrons to sort by"), AVAILABILITY_FACET_GROUP_NAME: _("Allow patrons to filter availability to"), - COLLECTION_FACET_GROUP_NAME: _("Allow patrons to filter collection to"), DISTRIBUTOR_FACETS_GROUP_NAME: _("Allow patrons to filter by distributor"), COLLECTION_NAME_FACETS_GROUP_NAME: _( "Allow patrons to filter by collection name" @@ -96,8 +84,6 @@ class FacetConstants: AVAILABLE_NOW: _("Available now"), AVAILABLE_ALL: _("All"), AVAILABLE_OPEN_ACCESS: _("Yours to keep"), - COLLECTION_FULL: _("Everything"), - COLLECTION_FEATURED: _("Popular Books"), } # For titles generated based on some runtime value @@ -115,7 +101,6 @@ class FacetConstants: AVAILABLE_NOW, AVAILABLE_OPEN_ACCESS, ], - COLLECTION_FACET_GROUP_NAME: [COLLECTION_FULL, COLLECTION_FEATURED], DISTRIBUTOR_FACETS_GROUP_NAME: [DISTRIBUTOR_ALL], COLLECTION_NAME_FACETS_GROUP_NAME: [COLLECTION_NAME_ALL], } @@ -125,7 +110,6 @@ class FacetConstants: DEFAULT_FACET = { ORDER_FACET_GROUP_NAME: ORDER_AUTHOR, AVAILABILITY_FACET_GROUP_NAME: AVAILABLE_ALL, - COLLECTION_FACET_GROUP_NAME: COLLECTION_FULL, DISTRIBUTOR_FACETS_GROUP_NAME: DISTRIBUTOR_ALL, COLLECTION_NAME_FACETS_GROUP_NAME: COLLECTION_NAME_ALL, } diff --git a/src/palace/manager/integration/configuration/library.py b/src/palace/manager/integration/configuration/library.py index 0e6bf3866..ef8434631 100644 --- a/src/palace/manager/integration/configuration/library.py +++ b/src/palace/manager/integration/configuration/library.py @@ -234,35 +234,7 @@ class LibrarySettings(BaseSettings): skip=True, ), ) - facets_enabled_collection: list[str] = FormField( - FacetConstants.DEFAULT_ENABLED_FACETS[ - FacetConstants.COLLECTION_FACET_GROUP_NAME - ], - form=LibraryConfFormItem( - label="Allow patrons to filter collection to", - type=ConfigurationFormItemType.MENU, - options={ - facet: FacetConstants.FACET_DISPLAY_TITLES[facet] - for facet in FacetConstants.COLLECTION_FACETS - }, - category="Lanes & Filters", - paired="facets_default_collection", - level=Level.SYS_ADMIN_OR_MANAGER, - ), - ) - facets_default_collection: str = FormField( - FacetConstants.COLLECTION_FULL, - form=LibraryConfFormItem( - label="Default Collection", - type=ConfigurationFormItemType.SELECT, - options={ - facet: FacetConstants.FACET_DISPLAY_TITLES[facet] - for facet in FacetConstants.COLLECTION_FACETS - }, - category="Lanes & Filters", - skip=True, - ), - ) + library_description: str | None = FormField( None, form=LibraryConfFormItem( diff --git a/src/palace/manager/search/external_search.py b/src/palace/manager/search/external_search.py index 7c326350f..014168148 100644 --- a/src/palace/manager/search/external_search.py +++ b/src/palace/manager/search/external_search.py @@ -1832,14 +1832,6 @@ def build(self, _chain_filters=None): Bool(must=[not_open_access, licensed, not_available]) ) - if self.subcollection == FacetConstants.COLLECTION_FEATURED: - # Exclude books with a quality of less than the library's - # minimum featured quality. - range_query = self._match_range( - "quality", "gte", self.minimum_featured_quality - ) - f = chain(f, Bool(must=range_query)) - if self.identifiers: # Check every identifier for a match. clauses = [] diff --git a/src/palace/manager/sqlalchemy/model/lane.py b/src/palace/manager/sqlalchemy/model/lane.py index 73a87897e..3acc81cf7 100644 --- a/src/palace/manager/sqlalchemy/model/lane.py +++ b/src/palace/manager/sqlalchemy/model/lane.py @@ -395,18 +395,6 @@ def _values_from_request( 400, ) - g = Facets.COLLECTION_FACET_GROUP_NAME - collection = get_argument(g, cls.default_facet(config, g)) - collection_facets = cls.available_facets(config, g) - if collection and not collection in collection_facets: - return INVALID_INPUT.detailed( - _( - "I don't understand what '%(collection)s' refers to.", - collection=collection, - ), - 400, - ) - g = Facets.DISTRIBUTOR_FACETS_GROUP_NAME distributor = get_argument(g, cls.default_facet(config, g)) distributor_facets = cls.available_facets(config, g) @@ -442,7 +430,6 @@ def _values_from_request( enabled = { Facets.ORDER_FACET_GROUP_NAME: order_facets, Facets.AVAILABILITY_FACET_GROUP_NAME: availability_facets, - Facets.COLLECTION_FACET_GROUP_NAME: collection_facets, Facets.DISTRIBUTOR_FACETS_GROUP_NAME: distributor_facets, Facets.COLLECTION_NAME_FACETS_GROUP_NAME: collection_name_facets, } @@ -450,7 +437,6 @@ def _values_from_request( return dict( order=order, availability=availability, - collection=collection, distributor=distributor, collection_name=collection_name, enabled_facets=enabled, @@ -482,7 +468,6 @@ def from_request( def __init__( self, library, - collection, availability, order, distributor, @@ -495,17 +480,11 @@ def __init__( ): """Constructor. - :param collection: This is not a Collection object; it's a value for - the 'collection' facet, e.g. 'main' or 'featured'. - :param entrypoint: An EntryPoint class. The 'entry point' facet group is configured on a per-WorkList basis rather than a per-library basis. """ super().__init__(entrypoint, entrypoint_is_default, **constructor_kwargs) - collection = collection or self.default_facet( - library, self.COLLECTION_FACET_GROUP_NAME - ) availability = availability or self.default_facet( library, self.AVAILABILITY_FACET_GROUP_NAME ) @@ -530,7 +509,6 @@ def __init__( availability = self.AVAILABLE_NOW self.library = library - self.collection = collection self.availability = availability self.order = order self.distributor = distributor or self.default_facet( @@ -548,7 +526,6 @@ def __init__( def navigate( self, - collection=None, availability=None, order=None, entrypoint=None, @@ -558,7 +535,6 @@ def navigate( """Create a slightly different Facets object from this one.""" return self.__class__( library=self.library, - collection=collection or self.collection, availability=availability or self.availability, order=order or self.order, distributor=distributor or self.distributor, @@ -574,8 +550,6 @@ def items(self): yield (self.ORDER_FACET_GROUP_NAME, self.order) if self.availability: yield (self.AVAILABILITY_FACET_GROUP_NAME, self.availability) - if self.collection: - yield (self.COLLECTION_FACET_GROUP_NAME, self.collection) if self.distributor: yield (self.DISTRIBUTOR_FACETS_GROUP_NAME, self.distributor) if self.collection_name: @@ -595,7 +569,6 @@ def enabled_facets(self): facet_types = [ self.ORDER_FACET_GROUP_NAME, self.AVAILABILITY_FACET_GROUP_NAME, - self.COLLECTION_FACET_GROUP_NAME, self.DISTRIBUTOR_FACETS_GROUP_NAME, self.COLLECTION_NAME_FACETS_GROUP_NAME, ] @@ -606,7 +579,6 @@ def enabled_facets(self): for group_name in ( Facets.ORDER_FACET_GROUP_NAME, Facets.AVAILABILITY_FACET_GROUP_NAME, - Facets.COLLECTION_FACET_GROUP_NAME, Facets.DISTRIBUTOR_FACETS_GROUP_NAME, Facets.COLLECTION_NAME_FACETS_GROUP_NAME, ): @@ -625,7 +597,6 @@ def facet_groups(self): ( order_facets, availability_facets, - collection_facets, distributor_facets, collection_name_facets, ) = self.enabled_facets @@ -673,23 +644,6 @@ def dy(new_value): for facet in availability_facets: yield dy(facet) - # Next, the collection facets. - def dy(new_value): - group = self.COLLECTION_FACET_GROUP_NAME - current_value = self.collection - facets = self.navigate(collection=new_value) - return ( - group, - new_value, - facets, - new_value == current_value, - is_default_facet(facets, new_value, group), - ) - - if len(collection_facets) > 1: - for facet in collection_facets: - yield dy(facet) - if len(distributor_facets) > 1: for facet in distributor_facets: group = self.DISTRIBUTOR_FACETS_GROUP_NAME @@ -732,7 +686,6 @@ def modify_search_filter(self, filter): ) filter.availability = self.availability - filter.subcollection = self.collection # We can only have distributor and collection name facets if we have a library if self.library: @@ -796,16 +749,6 @@ def modify_database_query(self, _db, qu): qu = qu.filter(availability_clause) - if self.collection == self.COLLECTION_FULL: - # Include everything. - pass - elif self.collection == self.COLLECTION_FEATURED: - # Exclude books with a quality of less than the library's - # minimum featured quality. - qu = qu.filter( - Work.quality >= self.library.settings.minimum_featured_quality - ) - return qu @@ -1010,7 +953,6 @@ def __init__(self, **kwargs): # usage, a Library will be provided via # SearchFacets.from_request. kwargs.setdefault("library", None) - kwargs.setdefault("collection", None) kwargs.setdefault("availability", None) kwargs.setdefault("distributor", None) kwargs.setdefault("collection_name", None) @@ -1042,13 +984,10 @@ def __init__(self, **kwargs): def default_facet(cls, ignore, group_name): """The default facet settings for SearchFacets are hard-coded. - By default, we will search the full collection and all + By default, we will search all availabilities, and order by match quality rather than any bibliographic field. """ - if group_name == cls.COLLECTION_FACET_GROUP_NAME: - return cls.COLLECTION_FULL - if group_name == cls.AVAILABILITY_FACET_GROUP_NAME: return cls.AVAILABLE_ALL @@ -2977,7 +2916,6 @@ def update_size(self, _db, search_engine: ExternalSearchIndex): for entrypoint in EntryPoint.ENTRY_POINTS: facets = DatabaseBackedFacets( library, - FacetConstants.COLLECTION_FULL, FacetConstants.AVAILABLE_ALL, order=FacetConstants.ORDER_WORK_ID, distributor=FacetConstants.DISTRIBUTOR_ALL, diff --git a/tests/manager/api/controller/test_work.py b/tests/manager/api/controller/test_work.py index 4a92038de..710504ba8 100644 --- a/tests/manager/api/controller/test_work.py +++ b/tests/manager/api/controller/test_work.py @@ -138,7 +138,7 @@ def test_contributor(self, work_fixture: WorkFixture): facet_links = [ link for link in links if link["rel"] == "http://opds-spec.org/facet" ] - assert 10 == len(facet_links) + assert 8 == len(facet_links) # At this point we don't want to generate real feeds anymore. # We can't do a real end-to-end test without setting up a real @@ -870,7 +870,7 @@ def test_series(self, work_fixture: WorkFixture): facet_links = [ link for link in links if link["rel"] == "http://opds-spec.org/facet" ] - assert 11 == len(facet_links) + assert 9 == len(facet_links) # The facet link we care most about is the default sort order, # put into place by SeriesFacets. diff --git a/tests/manager/api/test_lanes.py b/tests/manager/api/test_lanes.py index 5dbfd4014..30ef2ed33 100644 --- a/tests/manager/api/test_lanes.py +++ b/tests/manager/api/test_lanes.py @@ -736,7 +736,6 @@ def test_overview_facets(self, lane_fixture: LaneFixture): ) overview = lane.overview_facets(lane_fixture.db.session, featured) assert isinstance(overview, Facets) - assert Facets.COLLECTION_FULL == overview.collection assert Facets.AVAILABLE_ALL == overview.availability assert Facets.ORDER_AUTHOR == overview.order @@ -814,7 +813,6 @@ def test_overview_facets(self, lane_fixture: LaneFixture): lane = SeriesLane(lane_fixture.db.default_library(), "Alrighty Then") overview = lane.overview_facets(lane_fixture.db.session, featured) assert isinstance(overview, SeriesFacets) - assert Facets.COLLECTION_FULL == overview.collection assert Facets.AVAILABLE_ALL == overview.availability assert Facets.ORDER_SERIES_POSITION == overview.order @@ -902,7 +900,6 @@ def test_overview_facets(self, lane_fixture: LaneFixture): ) overview = lane.overview_facets(lane_fixture.db.session, featured) assert isinstance(overview, ContributorFacets) - assert Facets.COLLECTION_FULL == overview.collection assert Facets.AVAILABLE_ALL == overview.availability assert Facets.ORDER_TITLE == overview.order @@ -913,7 +910,6 @@ def test_overview_facets(self, lane_fixture: LaneFixture): class TestCrawlableFacets: def test_default(self, db: DatabaseTransactionFixture): facets = CrawlableFacets.default(db.default_library()) - assert facets.collection == CrawlableFacets.COLLECTION_FULL assert facets.availability == CrawlableFacets.AVAILABLE_ALL assert facets.order == CrawlableFacets.ORDER_LAST_UPDATE assert facets.order_ascending is False @@ -921,13 +917,12 @@ def test_default(self, db: DatabaseTransactionFixture): [ order, availability, - collection, distributor, collectionName, ] = facets.enabled_facets # The default facets are the only ones enabled. - for facet in [order, availability, collection]: + for facet in [order, availability]: assert len(facet) == 1 # Except for distributor and collectionName, which have the default @@ -940,7 +935,6 @@ def test_default(self, db: DatabaseTransactionFixture): [ (Facets.ORDER_FACET_GROUP_NAME, Facets.ORDER_LAST_UPDATE), (Facets.AVAILABILITY_FACET_GROUP_NAME, Facets.AVAILABLE_ALL), - (Facets.COLLECTION_FACET_GROUP_NAME, Facets.COLLECTION_FULL), (Facets.DISTRIBUTOR_FACETS_GROUP_NAME, Facets.DISTRIBUTOR_ALL), (Facets.COLLECTION_NAME_FACETS_GROUP_NAME, Facets.COLLECTION_NAME_ALL), ], @@ -953,7 +947,6 @@ def test_available_none(self, group_name: str, expected: list[str]) -> None: [ (Facets.ORDER_FACET_GROUP_NAME, [Facets.ORDER_LAST_UPDATE]), (Facets.AVAILABILITY_FACET_GROUP_NAME, [Facets.AVAILABLE_ALL]), - (Facets.COLLECTION_FACET_GROUP_NAME, [Facets.COLLECTION_FULL]), (Facets.DISTRIBUTOR_FACETS_GROUP_NAME, [Facets.DISTRIBUTOR_ALL, "foo"]), ( Facets.COLLECTION_NAME_FACETS_GROUP_NAME, @@ -1126,13 +1119,9 @@ def test_default_facet(self, db: DatabaseTransactionFixture): # For other facet groups, the class defers to the Facets # superclass. (But this doesn't matter because it's not relevant # to the creation of jackpot feeds.) - for group in ( - Facets.COLLECTION_FACET_GROUP_NAME, - Facets.ORDER_FACET_GROUP_NAME, - ): - assert m(db.default_library(), group) == Facets.default_facet( - db.default_library(), group - ) + assert m( + db.default_library(), Facets.ORDER_FACET_GROUP_NAME + ) == Facets.default_facet(db.default_library(), Facets.ORDER_FACET_GROUP_NAME) def test_available_facets(self, db: DatabaseTransactionFixture): # A JackpotFacets object always has the same availability @@ -1150,10 +1139,7 @@ def test_available_facets(self, db: DatabaseTransactionFixture): # For other facet groups, the class defers to the Facets # superclass. (But this doesn't matter because it's not relevant # to the creation of jackpot feeds.) - for group in ( - Facets.COLLECTION_FACET_GROUP_NAME, - Facets.ORDER_FACET_GROUP_NAME, - ): + for group in (Facets.ORDER_FACET_GROUP_NAME,): assert m(db.default_library(), group) == Facets.available_facets( db.default_library(), group ) diff --git a/tests/manager/feed/test_library_annotator.py b/tests/manager/feed/test_library_annotator.py index c0276960c..18f21e9cf 100644 --- a/tests/manager/feed/test_library_annotator.py +++ b/tests/manager/feed/test_library_annotator.py @@ -464,17 +464,15 @@ def test_search_url(self, annotator_fixture: LibraryAnnotatorFixture): def test_facet_url(self, annotator_fixture: LibraryAnnotatorFixture): # A regular Lane. facets = Facets.default( - annotator_fixture.db.default_library(), collection="main" + annotator_fixture.db.default_library(), ) facet_url = annotator_fixture.annotator.facet_url(facets) - assert "collection=main" in facet_url assert str(annotator_fixture.lane.id) in facet_url # A QueryGeneratedLane. annotator_fixture.annotator.lane = annotator_fixture.contributor_lane facet_url_contributor = annotator_fixture.annotator.facet_url(facets) - assert "collection=main" in facet_url_contributor assert annotator_fixture.contributor_lane.ROUTE in facet_url_contributor assert ( annotator_fixture.contributor_lane.contributor_key in facet_url_contributor diff --git a/tests/manager/feed/test_opds_acquisition_feed.py b/tests/manager/feed/test_opds_acquisition_feed.py index 1948a46ac..6070ed375 100644 --- a/tests/manager/feed/test_opds_acquisition_feed.py +++ b/tests/manager/feed/test_opds_acquisition_feed.py @@ -850,33 +850,6 @@ def facet_groups(self): bunch that don't (which will be ignored). """ - # Real facet group, real facet - yield ( - Facets.COLLECTION_FACET_GROUP_NAME, - Facets.COLLECTION_FULL, - "try the featured collection instead", - True, - False, - ) - - # Real facet group, nonexistent facet - yield ( - Facets.COLLECTION_FACET_GROUP_NAME, - "no such facet", - "this facet does not exist", - True, - False, - ) - - # Nonexistent facet group, real facet - yield ( - "no such group", - Facets.COLLECTION_FULL, - "this facet exists but it's in a nonexistent group", - True, - False, - ) - # Nonexistent facet group, nonexistent facet yield ( "no such group", @@ -897,20 +870,11 @@ def facet_link(cls, url, facet_title, group_title, selected, is_default): annotator = MockAnnotator() facets = MockFacets() - # The only 5-tuple yielded by facet_groups was passed on to us. - # The link was run through MockAnnotator.facet_url(), - # and the human-readable titles were found using lookups. - # - # The other three 5-tuples were ignored since we don't know + # The 5-tuples were ignored since we don't know # how to generate human-readable titles for them. - [[url, facet, group, selected, is_default]] = MockFeed.facet_links( - annotator, facets - ) - assert "url: try the featured collection instead" == url - assert Facets.FACET_DISPLAY_TITLES[Facets.COLLECTION_FULL] == facet - assert Facets.GROUP_DISPLAY_TITLES[Facets.COLLECTION_FACET_GROUP_NAME] == group - assert selected - assert not is_default + links = MockFeed.facet_links(annotator, facets) + + assert len([x for x in links]) == 0 def test_active_loans_for_with_holds( self, @@ -1134,7 +1098,7 @@ def run(wl=None, facets=None, pagination=None): # The make_link function that was passed in calls # TestAnnotator.feed_url() when passed an EntryPoint. The # Facets object's other facet groups are propagated in this URL. - first_page_url = "http://wl/?available=all&collection=full&collectionName=All&distributor=All&entrypoint=Book&order=author" + first_page_url = "http://wl/?available=all&collectionName=All&distributor=All&entrypoint=Book&order=author" assert first_page_url == make_link(EbooksEntryPoint) # Pagination information is not propagated through entry point links @@ -1206,7 +1170,7 @@ def mock_search(self, *args, **kwargs): # The make_link function that was passed in calls # TestAnnotator.search_url() when passed an EntryPoint. - first_page_url = "http://localhost/default/search/?entrypoint=Book&order=relevance&available=all&collection=full&search_type=default" + first_page_url = "http://localhost/default/search/?entrypoint=Book&order=relevance&available=all&search_type=default" with app.test_request_context("/"): assert first_page_url == make_link(EbooksEntryPoint) diff --git a/tests/manager/search/test_external_search.py b/tests/manager/search/test_external_search.py index 38398828d..cc0671124 100644 --- a/tests/manager/search/test_external_search.py +++ b/tests/manager/search/test_external_search.py @@ -940,7 +940,6 @@ def pages(worklist): facets = Facets( transaction.default_library(), None, - None, order=Facets.ORDER_TITLE, distributor=None, collection_name=None, @@ -988,7 +987,6 @@ def pages(worklist): facets = SearchFacets by_author_facet = facets( library=transaction.default_library(), - collection=facets.COLLECTION_FULL, availability=facets.AVAILABLE_ALL, order=facets.ORDER_AUTHOR, ) @@ -996,7 +994,6 @@ def pages(worklist): by_title_facet = facets( library=transaction.default_library(), - collection=facets.COLLECTION_FULL, availability=facets.AVAILABLE_ALL, order=facets.ORDER_TITLE, ) @@ -1124,11 +1121,10 @@ def test_facet_filtering(self, end_to_end_search_fixture: EndToEndSearchFixture) data = self._populate_works(fixture) fixture.populate_search_index() - def expect(availability, collection, works): + def expect(availability, works): facets = Facets( transaction.default_library(), availability, - collection, order=Facets.ORDER_TITLE, distributor=None, collection_name=None, @@ -1137,7 +1133,6 @@ def expect(availability, collection, works): # Get all the books in alphabetical order by title. expect( - Facets.COLLECTION_FULL, Facets.AVAILABLE_ALL, [ data.becoming, @@ -1149,28 +1144,19 @@ def expect(availability, collection, works): # Show only works that can be borrowed right now. expect( - Facets.COLLECTION_FULL, Facets.AVAILABLE_NOW, [data.horse, data.moby, data.duck], ) # Show only works that can *not* be borrowed right now. - expect(Facets.COLLECTION_FULL, Facets.AVAILABLE_NOT_NOW, [data.becoming]) + expect(Facets.AVAILABLE_NOT_NOW, [data.becoming]) # Show only open-access works. expect( - Facets.COLLECTION_FULL, Facets.AVAILABLE_OPEN_ACCESS, [data.horse, data.moby], ) - # Show only featured-quality works. - expect( - Facets.COLLECTION_FEATURED, - Facets.AVAILABLE_ALL, - [data.becoming, data.moby], - ) - class TestSearchOrderData: a1: LicensePool @@ -1401,7 +1387,6 @@ def assert_order(sort_field, order, **filter_kwargs): expect = fixture.expect_results facets = Facets( transaction.default_library(), - Facets.COLLECTION_FULL, Facets.AVAILABLE_ALL, order=sort_field, distributor=None, @@ -2524,25 +2509,8 @@ def from_facets(*args, **kwargs): # Return the rest to be verified in a test-specific way. return built - # When using the 'featured' collection... - built = from_facets(Facets.COLLECTION_FEATURED, None, None, None, None) - - # There is no nested filter. - assert [] == built.nested_filter_calls - - # A non-nested filter is applied on the 'quality' field. - [quality_filter] = built._query.filter - quality_range = Filter._match_range( - "quality", - "gte", - db.default_library().settings.minimum_featured_quality, - ) - assert Q("bool", must=[quality_range], must_not=[RESEARCH]) == quality_filter - # When using the AVAILABLE_OPEN_ACCESS availability restriction... - built = from_facets( - Facets.COLLECTION_FULL, Facets.AVAILABLE_OPEN_ACCESS, None, None, None - ) + built = from_facets(Facets.AVAILABLE_OPEN_ACCESS, None, None, None) # An additional nested filter is applied. [available_now] = built.nested_filter_calls @@ -2555,9 +2523,7 @@ def from_facets(*args, **kwargs): assert nested_filter.to_dict() == {"bool": {"filter": [open_access]}} # When using the AVAILABLE_NOW restriction... - built = from_facets( - Facets.COLLECTION_FULL, Facets.AVAILABLE_NOW, None, None, None - ) + built = from_facets(Facets.AVAILABLE_NOW, None, None, None) # An additional nested filter is applied. [available_now] = built.nested_filter_calls @@ -2582,9 +2548,7 @@ def from_facets(*args, **kwargs): } # When using the AVAILABLE_NOT_NOW restriction... - built = from_facets( - Facets.COLLECTION_FULL, Facets.AVAILABLE_NOT_NOW, None, None, None - ) + built = from_facets(Facets.AVAILABLE_NOT_NOW, None, None, None) # An additional nested filter is applied. [not_available_now] = built.nested_filter_calls @@ -2608,7 +2572,6 @@ def from_facets(*args, **kwargs): # Distributor builds _id = DataSource.lookup(db.session, DataSource.OVERDRIVE).id built = from_facets( - Facets.COLLECTION_FULL, Facets.AVAILABLE_ALL, None, DataSource.OVERDRIVE, @@ -2622,9 +2585,7 @@ def from_facets(*args, **kwargs): # Collection Name builds collection = db.default_collection() - built = from_facets( - Facets.COLLECTION_FULL, Facets.AVAILABLE_ALL, None, None, collection.name - ) + built = from_facets(Facets.AVAILABLE_ALL, None, None, collection.name) [collection_only] = built.nested_filter_calls nested_filter = collection_only["query"] assert nested_filter.to_dict() == { @@ -2645,7 +2606,6 @@ def from_facets(*args, **kwargs): # used to convert it to appropriate Opensearch syntax, and # the MockSearch object is modified appropriately. built = from_facets( - None, None, order=Facets.ORDER_AUTHOR, distributor=None, diff --git a/tests/manager/sqlalchemy/model/test_lane.py b/tests/manager/sqlalchemy/model/test_lane.py index eba50eddd..63b23081d 100644 --- a/tests/manager/sqlalchemy/model/test_lane.py +++ b/tests/manager/sqlalchemy/model/test_lane.py @@ -309,7 +309,6 @@ def test_facet_groups(self, db: DatabaseTransactionFixture): db.default_collection().data_source = DataSource.AMAZON # type: ignore[assignment] facets = Facets( db.default_library(), - Facets.COLLECTION_FULL, Facets.AVAILABLE_ALL, Facets.ORDER_TITLE, Facets.DISTRIBUTOR_ALL, @@ -318,15 +317,14 @@ def test_facet_groups(self, db: DatabaseTransactionFixture): all_groups = list(facets.facet_groups) # By default, there are 10 facet transitions: two groups of three - # and one group of two and 2 datasource groups and 2 for collection names - assert 12 == len(all_groups) + # and 2 datasource groups and 2 for collection names + assert 10 == len(all_groups) - # available=all, collection=full, and order=title are the selected + # available=all and order=title are the selected # facets. selected = sorted(x[:2] for x in all_groups if x[-2] == True) assert [ ("available", "all"), - ("collection", "full"), ("collectionName", "All"), ("distributor", "All"), ("order", "title"), @@ -335,20 +333,16 @@ def test_facet_groups(self, db: DatabaseTransactionFixture): # Distributor and CollectionName facets are generated at runtime, they are not a setting value test_enabled_facets = { Facets.ORDER_FACET_GROUP_NAME: [Facets.ORDER_WORK_ID, Facets.ORDER_TITLE], - Facets.COLLECTION_FACET_GROUP_NAME: [Facets.COLLECTION_FEATURED], Facets.AVAILABILITY_FACET_GROUP_NAME: [Facets.AVAILABLE_ALL], } test_default_facets = { Facets.ORDER_FACET_GROUP_NAME: Facets.ORDER_TITLE, - Facets.COLLECTION_FACET_GROUP_NAME: Facets.COLLECTION_FEATURED, Facets.AVAILABILITY_FACET_GROUP_NAME: Facets.AVAILABLE_ALL, } library = db.default_library() self._configure_facets(library, test_enabled_facets, test_default_facets) - facets = Facets( - db.default_library(), None, None, Facets.ORDER_TITLE, None, None - ) + facets = Facets(db.default_library(), None, Facets.ORDER_TITLE, None, None) all_groups = list(facets.facet_groups) # We have disabled almost all the facets, so the list of # facet transitions includes only two items. @@ -435,7 +429,6 @@ def test_default_availability( # facet. test_enabled_facets = { Facets.ORDER_FACET_GROUP_NAME: [Facets.ORDER_WORK_ID], - Facets.COLLECTION_FACET_GROUP_NAME: [Facets.COLLECTION_FULL], Facets.AVAILABILITY_FACET_GROUP_NAME: [ Facets.AVAILABLE_ALL, Facets.AVAILABLE_NOW, @@ -443,7 +436,6 @@ def test_default_availability( } test_default_facets = { Facets.ORDER_FACET_GROUP_NAME: Facets.ORDER_TITLE, - Facets.COLLECTION_FACET_GROUP_NAME: Facets.COLLECTION_FULL, Facets.AVAILABILITY_FACET_GROUP_NAME: Facets.AVAILABLE_ALL, } library = db.default_library() @@ -475,7 +467,6 @@ def test_facets_can_be_enabled_at_initialization( Facets.ORDER_TITLE, Facets.ORDER_AUTHOR, ], - Facets.COLLECTION_FACET_GROUP_NAME: [Facets.COLLECTION_FULL], Facets.AVAILABILITY_FACET_GROUP_NAME: [Facets.AVAILABLE_OPEN_ACCESS], } library = db.default_library() @@ -485,7 +476,6 @@ def test_facets_can_be_enabled_at_initialization( # no matter the Configuration. facets = Facets( db.default_library(), - Facets.COLLECTION_FULL, Facets.AVAILABLE_OPEN_ACCESS, Facets.ORDER_TITLE, Facets.DISTRIBUTOR_ALL, @@ -502,13 +492,11 @@ def test_facets_dont_need_a_library(self): Facets.ORDER_TITLE, Facets.ORDER_AUTHOR, ], - Facets.COLLECTION_FACET_GROUP_NAME: [Facets.COLLECTION_FULL], Facets.AVAILABILITY_FACET_GROUP_NAME: [Facets.AVAILABLE_OPEN_ACCESS], } facets = Facets( None, - Facets.COLLECTION_FULL, Facets.AVAILABLE_OPEN_ACCESS, Facets.ORDER_TITLE, Facets.DISTRIBUTOR_ALL, @@ -525,7 +513,6 @@ def test_items(self, db: DatabaseTransactionFixture): """ facets = Facets( db.default_library(), - Facets.COLLECTION_FULL, Facets.AVAILABLE_ALL, Facets.ORDER_TITLE, Facets.DISTRIBUTOR_ALL, @@ -534,7 +521,6 @@ def test_items(self, db: DatabaseTransactionFixture): ) assert [ ("available", Facets.AVAILABLE_ALL), - ("collection", Facets.COLLECTION_FULL), ("collectionName", Facets.COLLECTION_NAME_ALL), ("distributor", Facets.DISTRIBUTOR_ALL), ("entrypoint", AudiobooksEntryPoint.INTERNAL_NAME), @@ -546,7 +532,6 @@ def test_default_order_ascending(self, db: DatabaseTransactionFixture): for order in (Facets.ORDER_TITLE, Facets.ORDER_AUTHOR): f = Facets( db.default_library(), - collection=Facets.COLLECTION_FULL, availability=Facets.AVAILABLE_ALL, order=order, distributor=Facets.DISTRIBUTOR_ALL, @@ -562,7 +547,6 @@ def test_default_order_ascending(self, db: DatabaseTransactionFixture): for order in Facets.ORDER_DESCENDING_BY_DEFAULT: f = Facets( db.default_library(), - collection=Facets.COLLECTION_FULL, availability=Facets.AVAILABLE_ALL, order=order, distributor=Facets.DISTRIBUTOR_ALL, @@ -579,7 +563,6 @@ def test_navigate(self, db: DatabaseTransactionFixture): ebooks = EbooksEntryPoint f = Facets( db.default_library(), - F.COLLECTION_FULL, F.AVAILABLE_ALL, F.ORDER_TITLE, Facets.DISTRIBUTOR_ALL, @@ -587,16 +570,7 @@ def test_navigate(self, db: DatabaseTransactionFixture): entrypoint=ebooks, ) - different_collection = f.navigate(collection=F.COLLECTION_FEATURED) - assert F.COLLECTION_FEATURED == different_collection.collection - assert F.AVAILABLE_ALL == different_collection.availability - assert F.ORDER_TITLE == different_collection.order - assert F.DISTRIBUTOR_ALL == different_collection.distributor - assert F.COLLECTION_NAME_ALL == different_collection.collection_name - assert ebooks == different_collection.entrypoint - different_availability = f.navigate(availability=F.AVAILABLE_NOW) - assert F.COLLECTION_FULL == different_availability.collection assert F.AVAILABLE_NOW == different_availability.availability assert F.ORDER_TITLE == different_availability.order assert F.DISTRIBUTOR_ALL == different_availability.distributor @@ -604,7 +578,6 @@ def test_navigate(self, db: DatabaseTransactionFixture): assert ebooks == different_availability.entrypoint different_order = f.navigate(order=F.ORDER_AUTHOR) - assert F.COLLECTION_FULL == different_order.collection assert F.AVAILABLE_ALL == different_order.availability assert F.ORDER_AUTHOR == different_order.order assert F.DISTRIBUTOR_ALL == different_order.distributor @@ -613,7 +586,6 @@ def test_navigate(self, db: DatabaseTransactionFixture): audiobooks = AudiobooksEntryPoint different_entrypoint = f.navigate(entrypoint=audiobooks) - assert F.COLLECTION_FULL == different_entrypoint.collection assert F.AVAILABLE_ALL == different_entrypoint.availability assert F.ORDER_TITLE == different_entrypoint.order assert F.DISTRIBUTOR_ALL == different_entrypoint.distributor @@ -621,14 +593,12 @@ def test_navigate(self, db: DatabaseTransactionFixture): assert audiobooks == different_entrypoint.entrypoint different_distributor = f.navigate(distributor=DataSource.AMAZON) - assert F.COLLECTION_FULL == different_distributor.collection assert F.AVAILABLE_ALL == different_distributor.availability assert F.ORDER_TITLE == different_distributor.order assert F.COLLECTION_NAME_ALL == different_distributor.collection_name assert DataSource.AMAZON == different_distributor.distributor different_collection_name = f.navigate(collection_name="Collection Name") - assert F.COLLECTION_FULL == different_collection_name.collection assert F.AVAILABLE_ALL == different_collection_name.availability assert F.ORDER_TITLE == different_collection_name.order assert F.DISTRIBUTOR_ALL == different_collection_name.distributor @@ -652,7 +622,6 @@ def test_from_request( # Valid object using the default settings. default_order = config.default_facet(Facets.ORDER_FACET_GROUP_NAME) - default_collection = config.default_facet(Facets.COLLECTION_FACET_GROUP_NAME) default_availability = config.default_facet( Facets.AVAILABILITY_FACET_GROUP_NAME ) @@ -660,7 +629,6 @@ def test_from_request( headers: dict = {} facets = m(library, library, args.get, headers.get, worklist) assert default_order == facets.order - assert default_collection == facets.collection assert default_availability == facets.availability assert library == facets.library @@ -671,13 +639,11 @@ def test_from_request( # Valid object using non-default settings. args = dict( order=Facets.ORDER_TITLE, - collection=Facets.COLLECTION_FULL, available=Facets.AVAILABLE_OPEN_ACCESS, entrypoint=EbooksEntryPoint.INTERNAL_NAME, ) facets = m(library, library, args.get, headers.get, worklist) assert Facets.ORDER_TITLE == facets.order - assert Facets.COLLECTION_FULL == facets.collection assert Facets.AVAILABLE_OPEN_ACCESS == facets.availability assert library == facets.library assert EbooksEntryPoint == facets.entrypoint @@ -700,15 +666,6 @@ def test_from_request( == invalid_availability.detail ) - # Invalid collection - args = dict(collection="no such collection") - invalid_collection = m(library, library, args.get, headers.get, None) - assert INVALID_INPUT.uri == invalid_collection.uri - assert ( - "I don't understand what 'no such collection' refers to." - == invalid_collection.detail - ) - def test_from_request_gets_available_facets_through_hook_methods( self, db: DatabaseTransactionFixture ): @@ -724,7 +681,6 @@ class Mock(Facets): mock_enabled = dict( order=[Facets.ORDER_TITLE], available=[Facets.AVAILABLE_OPEN_ACCESS], - collection=[Facets.COLLECTION_FULL], distributor=[Facets.DISTRIBUTOR_ALL], collectionName=[Facets.COLLECTION_NAME_ALL], ) @@ -745,15 +701,13 @@ def default_facet(cls, config, facet_group_name): ( order, available, - collection, distributor, collection_name, ) = Mock.available_facets_calls # available_facets was called three times, to ask the Mock class what it thinks - # the options for order, availability, and collection should be. + # the options for order and availability should be. assert (library, "order") == order assert (library, "available") == available - assert (library, "collection") == collection assert (library, "distributor") == distributor assert (library, "collectionName") == collection_name @@ -762,13 +716,11 @@ def default_facet(cls, config, facet_group_name): ( order_d, available_d, - collection_d, distributor_d, collection_name_d, ) = Mock.default_facet_calls assert (library, "order") == order_d assert (library, "available") == available_d - assert (library, "collection") == collection_d assert (library, "distributor") == distributor_d assert (library, "collectionName") == collection_name_d @@ -781,7 +733,6 @@ def default_facet(cls, config, facet_group_name): # The current values came from the defaults provided by default_facet(). assert Facets.ORDER_TITLE == result.order assert Facets.AVAILABLE_OPEN_ACCESS == result.availability - assert Facets.COLLECTION_FULL == result.collection assert Facets.DISTRIBUTOR_ALL == result.distributor assert Facets.COLLECTION_NAME_ALL == result.collection_name @@ -793,7 +744,6 @@ def test_modify_search_filter(self, db: DatabaseTransactionFixture): None, None, None, - None, entrypoint=AudiobooksEntryPoint, ) filter = Filter() @@ -803,7 +753,6 @@ def test_modify_search_filter(self, db: DatabaseTransactionFixture): # Now test the subclass behavior. facets = Facets( db.default_library(), - "some collection", "some availability", order=Facets.ORDER_ADDED_TO_COLLECTION, distributor=DataSource.OVERDRIVE, @@ -821,7 +770,6 @@ def test_modify_search_filter(self, db: DatabaseTransactionFixture): # Availability and collection and distributor are propagated with no # validation. assert "some availability" == filter.availability - assert "some collection" == filter.subcollection assert [ DataSource.lookup(db.session, DataSource.OVERDRIVE).id ] == filter.license_datasources @@ -836,13 +784,13 @@ def test_modify_search_filter(self, db: DatabaseTransactionFixture): # Specifying an invalid sort order doesn't cause a crash, but you # don't get a sort order. - facets = Facets(db.default_library(), None, None, "invalid order", None, None) + facets = Facets(db.default_library(), None, "invalid order", None, None) filter = Filter() facets.modify_search_filter(filter) assert None == filter.order facets = Facets( - db.default_library(), None, None, None, None, db.default_collection().name + db.default_library(), None, None, None, db.default_collection().name ) filter = Filter() facets.modify_search_filter(filter) @@ -888,46 +836,25 @@ def test_modify_database_query(self, db: DatabaseTransactionFixture): ), (Facets.AVAILABLE_NOT_NOW, [not_available]), ]: - facets = Facets(db.default_library(), None, availability, None, None, None) + facets = Facets(db.default_library(), availability, None, None, None) modified = facets.modify_database_query(db.session, qu) assert (availability, sorted(x.title for x in modified)) == ( availability, sorted(x.title for x in expect), ) - # Setting the 'featured' collection includes only known - # high-quality works. - for collection, expect in [ - ( - Facets.COLLECTION_FULL, - [open_access, available, unlimited_access], - ), - (Facets.COLLECTION_FEATURED, [open_access]), - ]: - facets = Facets( - db.default_library(), collection, Facets.AVAILABLE_NOW, None, None, None - ) - modified = facets.modify_database_query(db.session, qu) - assert (collection, sorted(x.title for x in modified)) == ( - collection, - sorted(x.title for x in expect), - ) - class TestDefaultSortOrderFacets: def _check_other_groups_not_changed(self, cls, config: Library): - # Verify that nothing has changed for the collection or - # availability facet groups. - for group_name in ( - Facets.COLLECTION_FACET_GROUP_NAME, - Facets.AVAILABILITY_FACET_GROUP_NAME, - ): - assert Facets.available_facets(config, group_name) == cls.available_facets( - config, group_name - ) - assert Facets.default_facet(config, group_name) == cls.default_facet( - config, group_name - ) + # Verify that nothing has changed for the + # availability facet group. + group_name = Facets.AVAILABILITY_FACET_GROUP_NAME + assert Facets.available_facets(config, group_name) == cls.available_facets( + config, group_name + ) + assert Facets.default_facet(config, group_name) == cls.default_facet( + config, group_name + ) def test_sort_order_rearrangement(self, db: DatabaseTransactionFixture): config = db.default_library() @@ -1003,29 +930,16 @@ def test_available_facets(self, db: DatabaseTransactionFixture): for order in f2_orders: assert order in f1_orders and order in f2.ORDER_FACET_TO_DATABASE_FIELD - # The rules for collection and availability are the same. - for group in ( - FacetConstants.COLLECTION_FACET_GROUP_NAME, - FacetConstants.AVAILABILITY_FACET_GROUP_NAME, - ): - assert f1.available_facets( - db.default_library(), group - ) == f2.available_facets(db.default_library(), group) - def test_default_facets(self, db: DatabaseTransactionFixture): # If the configured default sort order is not available, # DatabaseBackedFacets chooses the first enabled sort order. f1 = Facets f2 = DatabaseBackedFacets - # The rules for collection and availability are the same. - for group in ( - FacetConstants.COLLECTION_FACET_GROUP_NAME, - FacetConstants.AVAILABILITY_FACET_GROUP_NAME, - ): - assert f1.default_facet(db.default_library(), group) == f2.default_facet( - db.default_library(), group - ) + group = FacetConstants.AVAILABILITY_FACET_GROUP_NAME + assert f1.default_facet(db.default_library(), group) == f2.default_facet( + db.default_library(), group + ) # In this bizarre library, the default sort order is 'time # added to collection' -- an order not supported by @@ -1066,7 +980,6 @@ def test_order_by(self, db: DatabaseTransactionFixture): def order(facet, ascending=None): f = DatabaseBackedFacets( db.default_library(), - collection=Facets.COLLECTION_FULL, availability=Facets.AVAILABLE_ALL, order=facet, distributor=None, @@ -1148,13 +1061,10 @@ def test_modify_database_query( qu = DatabaseBackedWorkList.base_query(db.session) def facetify( - collection=Facets.COLLECTION_FULL, available=Facets.AVAILABLE_ALL, order=Facets.ORDER_TITLE, ): - f = DatabaseBackedFacets( - db.default_library(), collection, available, order, None, None - ) + f = DatabaseBackedFacets(db.default_library(), available, order, None, None) return f.modify_database_query(db.session, qu) # When holds are allowed, we can find all works by asking @@ -1187,13 +1097,6 @@ def facetify( assert licensed_low not in open_access assert unlimited_access_high not in open_access - # If we restrict to the featured collection we lose the two - # low-quality books. - featured_collection = facetify(collection=Facets.COLLECTION_FEATURED) - assert 3 == featured_collection.count() - assert open_access_low not in featured_collection - assert licensed_low not in featured_collection - # Try some different orderings to verify that order_by() # is called and used properly. title_order = facetify(order=Facets.ORDER_TITLE) @@ -1519,7 +1422,6 @@ def test_items(self): ("entrypoint", EverythingEntryPoint.INTERNAL_NAME), (Facets.ORDER_FACET_GROUP_NAME, SearchFacets.ORDER_BY_RELEVANCE), (Facets.AVAILABILITY_FACET_GROUP_NAME, Facets.AVAILABLE_ALL), - (Facets.COLLECTION_FACET_GROUP_NAME, Facets.COLLECTION_FULL), ("media", Edition.BOOK_MEDIUM), ("min_score", "123"), ("search_type", "default"), @@ -2332,7 +2234,6 @@ def works_for_hits(self, _db, work_ids, facets=None): facets = Facets( db.default_library(), None, - None, order=Facets.ORDER_TITLE, distributor=None, collection_name=None, @@ -2978,7 +2879,6 @@ def test_works_from_database_end_to_end(self, db: DatabaseTransactionFixture): # are returned. facets = DatabaseBackedFacets( db.default_library(), - collection=Facets.COLLECTION_FULL, availability=Facets.AVAILABLE_ALL, order=Facets.ORDER_TITLE, distributor=None, @@ -4406,7 +4306,6 @@ def database_facets(self, library: Library | None = None) -> DatabaseBackedFacet library = library or self.db.default_library() return DatabaseBackedFacets( library, - collection=Facets.COLLECTION_FULL, availability=Facets.AVAILABLE_ALL, order=Facets.ORDER_TITLE, distributor=None, diff --git a/tests/manager/sqlalchemy/model/test_library.py b/tests/manager/sqlalchemy/model/test_library.py index 2f557b838..ef68f0358 100644 --- a/tests/manager/sqlalchemy/model/test_library.py +++ b/tests/manager/sqlalchemy/model/test_library.py @@ -164,8 +164,6 @@ def test_explain(self, db: DatabaseTransactionFixture): facets_default_order='author' facets_enabled_available='['all', 'now', 'always']' facets_default_available='all' -facets_enabled_collection='['full', 'featured']' -facets_default_collection='full' help_web='http://library.com/support' default_notification_email_address='noreply@thepalaceproject.org' color_scheme='blue' From be1b50eac46a5065a1fc3d9b16e7f9bf1ea4522d Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 2 Dec 2024 15:40:42 -0800 Subject: [PATCH 3/5] Bump pytest from 8.3.3 to 8.3.4 (#2202) Bumps [pytest](https://github.com/pytest-dev/pytest) from 8.3.3 to 8.3.4. - [Release notes](https://github.com/pytest-dev/pytest/releases) - [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst) - [Commits](https://github.com/pytest-dev/pytest/compare/8.3.3...8.3.4) --- updated-dependencies: - dependency-name: pytest dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- poetry.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/poetry.lock b/poetry.lock index 2e7403f96..03338a91b 100644 --- a/poetry.lock +++ b/poetry.lock @@ -3692,13 +3692,13 @@ files = [ [[package]] name = "pytest" -version = "8.3.3" +version = "8.3.4" description = "pytest: simple powerful testing with Python" optional = false python-versions = ">=3.8" files = [ - {file = "pytest-8.3.3-py3-none-any.whl", hash = "sha256:a6853c7375b2663155079443d2e45de913a911a11d669df02a50814944db57b2"}, - {file = "pytest-8.3.3.tar.gz", hash = "sha256:70b98107bd648308a7952b06e6ca9a50bc660be218d53c257cc1fc94fda10181"}, + {file = "pytest-8.3.4-py3-none-any.whl", hash = "sha256:50e16d954148559c9a74109af1eaf0c945ba2d8f30f0a3d3335edde19788b6f6"}, + {file = "pytest-8.3.4.tar.gz", hash = "sha256:965370d062bce11e73868e0335abac31b4d3de0e82f4007408d242b4f8610761"}, ] [package.dependencies] From 5f769ad753abe545b41276e61145fe54bee5307d Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Tue, 3 Dec 2024 07:00:05 -0400 Subject: [PATCH 4/5] Check for problem detail return (#2199) --- .../manager/api/controller/opds_feed.py | 5 +++- .../manager/api/controller/test_crawlfeed.py | 23 +++++++++++++++++-- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/palace/manager/api/controller/opds_feed.py b/src/palace/manager/api/controller/opds_feed.py index f13f67fa0..b331fd725 100644 --- a/src/palace/manager/api/controller/opds_feed.py +++ b/src/palace/manager/api/controller/opds_feed.py @@ -34,6 +34,7 @@ SearchFacets, WorkList, ) +from palace.manager.util.flask_util import OPDSFeedResponse from palace.manager.util.problem_detail import ProblemDetail @@ -243,7 +244,7 @@ def crawlable_list_feed(self, list_name): def _crawlable_feed( self, title, url, worklist, annotator=None, feed_class=OPDSAcquisitionFeed - ): + ) -> OPDSFeedResponse | ProblemDetail: """Helper method to create a crawlable feed. :param title: The title to use for the feed. @@ -270,6 +271,8 @@ def _crawlable_feed( worklist=worklist, base_class=CrawlableFacets, ) + if isinstance(facets, ProblemDetail): + return facets annotator = annotator or self.manager.annotator(worklist, facets=facets) return feed_class.page( diff --git a/tests/manager/api/controller/test_crawlfeed.py b/tests/manager/api/controller/test_crawlfeed.py index 80385c9f4..df3d07f0a 100644 --- a/tests/manager/api/controller/test_crawlfeed.py +++ b/tests/manager/api/controller/test_crawlfeed.py @@ -1,7 +1,7 @@ import json from contextlib import contextmanager from typing import Any -from unittest.mock import MagicMock +from unittest.mock import MagicMock, create_autospec import feedparser from flask import url_for @@ -13,7 +13,11 @@ CrawlableFacets, DynamicLane, ) -from palace.manager.api.problem_details import NO_SUCH_COLLECTION, NO_SUCH_LIST +from palace.manager.api.problem_details import ( + NO_SUCH_COLLECTION, + NO_SUCH_LANE, + NO_SUCH_LIST, +) from palace.manager.core.problem_details import INVALID_INPUT from palace.manager.feed.acquisition import OPDSAcquisitionFeed from palace.manager.feed.annotator.circulation import CirculationManagerAnnotator @@ -226,6 +230,21 @@ def works(self, _db, facets, pagination, *args, **kwargs): title="Lane title", url="Lane URL", worklist=mock_lane, feed_class=MockFeed ) + # Lane that cannot be accessed -> problem detail + inaccessible_lane = MockLane() + inaccessible_lane.accessible_to = create_autospec( + mock_lane.accessible_to, return_value=False + ) + with circulation_fixture.request_context_with_library("/"): + response = circulation_fixture.manager.opds_feeds._crawlable_feed( + title="Lane title", + url="Lane URL", + worklist=inaccessible_lane, + feed_class=MockFeed, + ) + assert isinstance(response, ProblemDetail) + assert NO_SUCH_LANE.uri == response.uri + # Bad pagination data -> problem detail with circulation_fixture.app.test_request_context("/?size=a"): response = circulation_fixture.manager.opds_feeds._crawlable_feed( From e7ae3643d27371d7a1800d763ab56a0cac0b4ee5 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Tue, 3 Dec 2024 07:00:38 -0400 Subject: [PATCH 5/5] Filter work summary to remove non-xml safe characters (PP-1969) (#2198) --- ...1ef9aa_remove_unsafe_characters_summary.py | 32 +++++++++++++++++++ src/palace/manager/sqlalchemy/model/work.py | 18 +++++++++-- .../manager/sqlalchemy/model/test_edition.py | 22 ++++++++++--- 3 files changed, 64 insertions(+), 8 deletions(-) create mode 100644 alembic/versions/20241127_c3458e1ef9aa_remove_unsafe_characters_summary.py diff --git a/alembic/versions/20241127_c3458e1ef9aa_remove_unsafe_characters_summary.py b/alembic/versions/20241127_c3458e1ef9aa_remove_unsafe_characters_summary.py new file mode 100644 index 000000000..05101d74f --- /dev/null +++ b/alembic/versions/20241127_c3458e1ef9aa_remove_unsafe_characters_summary.py @@ -0,0 +1,32 @@ +"""Remove unsafe characters summary + +Revision ID: c3458e1ef9aa +Revises: 272da5f400de +Create Date: 2024-11-27 20:32:41.431147+00:00 + +""" + +from alembic import op + +# revision identifiers, used by Alembic. +revision = "c3458e1ef9aa" +down_revision = "272da5f400de" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # Remove any characters that are not XML safe from the summary_text field. The code has been + # updated to filter out these characters, but this cleans up any existing data. + # https://www.postgresql.org/docs/current/functions-matching.html#FUNCTIONS-POSIX-REGEXP + op.execute( + "UPDATE works SET summary_text = regexp_replace(" + " summary_text, '[^\u0020-\uD7FF\u0009\u000A\u000D\uE000-\uFFFD\U00010000-\U0010FFFF]+', '', 'g'" + ") WHERE " + "summary_text ~ '[^\u0020-\uD7FF\u0009\u000A\u000D\uE000-\uFFFD\U00010000-\U0010FFFF]'" + ) + + +def downgrade() -> None: + # No need to do anything on downgrade. + pass diff --git a/src/palace/manager/sqlalchemy/model/work.py b/src/palace/manager/sqlalchemy/model/work.py index 9c0cc4263..b3e0fdbc9 100644 --- a/src/palace/manager/sqlalchemy/model/work.py +++ b/src/palace/manager/sqlalchemy/model/work.py @@ -2,11 +2,13 @@ from __future__ import annotations +import re import sys from collections import Counter from collections.abc import Sequence from datetime import date, datetime from decimal import Decimal +from functools import cache from typing import TYPE_CHECKING, Any, cast import opensearchpy @@ -609,11 +611,21 @@ def merge_into(self, other_work): other_work.calculate_presentation() - def set_summary(self, resource): + @classmethod + @cache + def _xml_text_sanitization_regex(cls) -> re.Pattern[str]: + # Source: https://stackoverflow.com/questions/8733233/filtering-out-certain-bytes-in-python + return re.compile( + r"[^\u0020-\uD7FF\u0009\u000A\u000D\uE000-\uFFFD\U00010000-\U0010FFFF]+" + ) + + def set_summary(self, resource: Resource) -> None: self.summary = resource - # TODO: clean up the content if resource and resource.representation: - self.summary_text = resource.representation.unicode_content + # Make sure that the summary text only contains characters that are XML compatible. + self.summary_text = self._xml_text_sanitization_regex().sub( + "", resource.representation.unicode_content + ) else: self.summary_text = "" WorkCoverageRecord.add_for(self, operation=WorkCoverageRecord.SUMMARY_OPERATION) diff --git a/tests/manager/sqlalchemy/model/test_edition.py b/tests/manager/sqlalchemy/model/test_edition.py index a2d7cd4ec..0674b60a5 100644 --- a/tests/manager/sqlalchemy/model/test_edition.py +++ b/tests/manager/sqlalchemy/model/test_edition.py @@ -403,20 +403,32 @@ def test_set_summary(self, db: DatabaseTransactionFixture): work = db.work(presentation_edition=e) overdrive = DataSource.lookup(db.session, DataSource.OVERDRIVE) - # Set the work's summmary. + # Set the work's summary. l1, new = pool.add_link( Hyperlink.DESCRIPTION, None, overdrive, "text/plain", "F" ) work.set_summary(l1.resource) - assert l1.resource == work.summary - assert "F" == work.summary_text + assert work.summary == l1.resource + assert work.summary_text == "F" + + # Set the work's summary to a string that contains characters that cannot be + # represented in XML. + l2, new = pool.add_link( + Hyperlink.DESCRIPTION, + None, + overdrive, + "text/plain", + "\u0000💣ü🔥\u0001\u000C", + ) + work.set_summary(l2.resource) + assert work.summary_text == "💣ü🔥" # Remove the summary. work.set_summary(None) - assert None == work.summary - assert "" == work.summary_text + assert work.summary is None + assert work.summary_text == "" def test_calculate_evaluate_summary_quality_with_privileged_data_sources( self, db: DatabaseTransactionFixture