Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PP-1914] Remove collection facet from feed. #2182

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/palace/manager/api/lanes.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Expand Down Expand Up @@ -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,
)
Expand Down Expand Up @@ -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,
)
Expand Down Expand Up @@ -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,
}
Expand Down
16 changes: 0 additions & 16 deletions src/palace/manager/core/facets.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -63,23 +54,20 @@ 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,
}

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

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"
Expand All @@ -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
Expand All @@ -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],
}
Expand 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,
}
Expand Down
30 changes: 1 addition & 29 deletions src/palace/manager/integration/configuration/library.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
8 changes: 0 additions & 8 deletions src/palace/manager/search/external_search.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = []
Expand Down
64 changes: 1 addition & 63 deletions src/palace/manager/sqlalchemy/model/lane.py
Original file line number Diff line number Diff line change
Expand Up @@ -396,18 +396,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)
Expand Down Expand Up @@ -443,15 +431,13 @@ 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,
}

return dict(
order=order,
availability=availability,
collection=collection,
distributor=distributor,
collection_name=collection_name,
enabled_facets=enabled,
Expand Down Expand Up @@ -483,7 +469,6 @@ def from_request(
def __init__(
self,
library,
collection,
availability,
order,
distributor,
Expand All @@ -496,17 +481,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
)
Expand All @@ -531,7 +510,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(
Expand All @@ -549,7 +527,6 @@ def __init__(

def navigate(
self,
collection=None,
availability=None,
order=None,
entrypoint=None,
Expand All @@ -559,7 +536,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,
Expand All @@ -575,8 +551,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:
Expand All @@ -596,7 +570,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,
]
Expand All @@ -607,7 +580,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,
):
Expand All @@ -626,7 +598,6 @@ def facet_groups(self):
(
order_facets,
availability_facets,
collection_facets,
distributor_facets,
collection_name_facets,
) = self.enabled_facets
Expand Down Expand Up @@ -674,23 +645,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
Expand Down Expand Up @@ -733,7 +687,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:
Expand Down Expand Up @@ -797,16 +750,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


Expand Down Expand Up @@ -1011,7 +954,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)
Expand Down Expand Up @@ -1043,13 +985,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

Expand Down Expand Up @@ -2971,7 +2910,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,
Expand Down
4 changes: 2 additions & 2 deletions tests/manager/api/controller/test_work.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -868,7 +868,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.
Expand Down
Loading