[PP-3498] Add sort option to crawlable feed#3002
Conversation
2aa4c42 to
8bb3e8f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3002 +/- ##
=======================================
Coverage 92.99% 92.99%
=======================================
Files 468 469 +1
Lines 43384 43417 +33
Branches 6034 6038 +4
=======================================
+ Hits 40344 40376 +32
Misses 1967 1967
- Partials 1073 1074 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jonathangreen
left a comment
There was a problem hiding this comment.
@dbernstein, you set the ticket to code review in JIRA, so I started reviewing it. I then noticed it's still in draft and you haven't requested a review in GitHub yet. I'd already added some comments before I realized, so I figured I'd submit a review anyway.
| from palace.manager.search.v7 import SearchV7 | ||
|
|
||
| REVISIONS = [SearchV5()] | ||
| REVISIONS = [SearchV5(), SearchV6(), SearchV7()] |
There was a problem hiding this comment.
This is going to cause us to do a complete search re-index everywhere. That is okay, but you'll have to take care when rolling this out. Since it creates a completely new index it will use 2x the storage while the complete index is happening. Depending on our storage usage in each OS cluster this might pose some issues.
Also I don't think we automatically drop the old search revision, so after you roll this out and the index is complete you will also have to go and drop the old index to free up space in the opensearch index.
You'll have to keep all this in mind while you roll out this change.
| Facets.ORDER_FACET_GROUP_NAME: [ | ||
| Facets.ORDER_LAST_UPDATE, | ||
| Facets.ORDER_LICENSE_POOL_LAST_UPDATED, | ||
| ], |
There was a problem hiding this comment.
After applying this PR. I don't see the new facet exposed in the crawlable feed, so clients have no way to discover it.
<link href="http://localhost:6500/test/crawlable?entrypoint=All&order=last_update&available=all&distributor=All&collectionName=All" rel="http://opds-spec.org/facet" title="Last Update" opds:facetGroup="Sort by" opds:activeFacet="true" defaultFacet="true"/>
<link href="http://localhost:6500/test/crawlable?entrypoint=All&order=last_update&available=all&distributor=All&collectionName=All" rel="http://opds-spec.org/facet" title="All" opds:facetGroup="Distributor" opds:activeFacet="true" defaultFacet="true"/>
<link href="http://localhost:6500/test/crawlable?entrypoint=All&order=last_update&available=all&distributor=Minotaur+Palace+Marketplace&collectionName=All" rel="http://opds-spec.org/facet" title="Minotaur Palace Marketplace" opds:facetGroup="Distributor"/>
<link href="http://localhost:6500/test/crawlable?entrypoint=All&order=last_update&available=all&distributor=Blackstone+Unlimited&collectionName=All" rel="http://opds-spec.org/facet" title="Blackstone Unlimited" opds:facetGroup="Distributor"/>
<link href="http://localhost:6500/test/crawlable?entrypoint=All&order=last_update&available=all&distributor=All&collectionName=All" rel="http://opds-spec.org/facet" title="All" opds:facetGroup="Collection Name" opds:activeFacet="true" defaultFacet="true"/>
<link href="http://localhost:6500/test/crawlable?entrypoint=All&order=last_update&available=all&distributor=All&collectionName=TEST+Minotaur+Test+Library+Palace+Marketplace" rel="http://opds-spec.org/facet" title="TEST Minotaur Test Library Palace Marketplace" opds:facetGroup="Collection Name"/>
<link href="http://localhost:6500/test/crawlable?entrypoint=All&order=last_update&available=all&distributor=All&collectionName=Blackstone+Unlimited" rel="http://opds-spec.org/facet" title="Blackstone Unlimited" opds:facetGroup="Collection Name"/>| if l["rel"] == "http://opds-spec.org/facet" | ||
| } | ||
| assert facet_groups == {"Collection Name", "Distributor"} | ||
| assert facet_groups == {"Collection Name", "Distributor", "Sort by"} |
There was a problem hiding this comment.
We also need to test that the Sort By facet group has the members we expect
f6d1f2d to
a296d48
Compare
|
@jonathangreen : thanks for the review. I took care of the missing link and added a test to confirm. I also manually tested it to make sure everything looks good. |
…se and search index. Add ability to sort search results by that field.
…ue is being populated when works are indexed.
…hanged only when the data changes. Also add support license_pool_last_updated order facet value.
7a80e0c to
e1dbb33
Compare
Description
This PR introduces a bug fix and a new feature.
Before the fix, the work.last_update_time was being updated whenever circulation data was imported, regardless of whether any data changed as a result. With this fix, we are updating that field only when a license pool's data has changed. Additionally I've added another field "last_updated" to the license pool itself. When data changes, this flag is updated. I've also added a new order facet to the crawlable search interface.
By specifying
order=license_pool_last_updatedin the crawlable url, the results will be order in reverse chronological order according to the licensepool.last_updated field. The new facet will avoid using the computationally intensive custom opensearch script that is used when ordering by the default order ("last_update"). In this case, the most recently updated licensepool associated with a work is used to determine the sort. Since the work.last_update_time is always updated with the same value as license_pool.last_updated flag when the license pool changes, the xml element (which maps to theWork.last_update_timein the feed should always reflect a timestamp that is at or after, but never before, the license pool update. (In the case that the bibliographic data is updated after the license pool, the element will reflect a date that is after the LP.last_updated.)This PR is intended to address the needs of Aspen as well as reduce load
As a side note: I confirmed that whenever availability is updated - whether by import or by checking in or out, license pool's last updated (as well as the work's) will be updated. No changes were required to fulfill that requirement.
Motivation and Context
https://ebce-lyrasis.atlassian.net/browse/PP-3498
How Has This Been Tested?
I've added tests, updated tests, and ran it locally both in my IDE as well as using compose.
Checklist