Pass the s3 cookie into get_loan_history_data (#14001)

* Pass the s3 cookie into get_loan_history_data

/account/loan-history.json 500'd under FastAPI because
get_loan_history_data read web.cookies(), which needs a web.py request.
Callers now pass the cookie in, as fastapi/borrow.py already does, and a
missing account raises AccountNotFoundError (404 at each route) instead
of web.py's render.notfound.

Closes #13959

* Trim loan history docstring and redundant tests

The route test in test_account_loans.py already covers the cookie being
passed through without web.cookies(), so drop the heavily mocked
duplicate in test_lending.py and a repeated assertion.

---------

Co-authored-by: Raymond Berger <RayBB@users.noreply.github.com>

Author: cipoklean

openlibrary/core/lending.py

@@ -895,7 +895,11 @@ def get_lending_state(doc, user=None, check_loan_status=False) -> str:
 RESULTS_PER_PAGE: int = 25
 
 
-def get_loan_history_data(username: str, page: int) -> dict:
+class AccountNotFoundError(LookupError):
+    """No Open Library account exists for the requested username."""
+
+
+def get_loan_history_data(username: str, page: int, s3_cookie: str | None) -> dict:
     """Fetch loan history data for a user.
 
     This will use a patron's S3 keys to query the IA loan history API,
@@ -906,13 +910,16 @@ def get_loan_history_data(username: str, page: int) -> dict:
     includes items that are not in Open Library, and displaying only IA
     items creates pagination and navigation issues. For further discussion,
     see https://github.com/internetarchive/openlibrary/pull/8375.
-    """
-    from infogami.utils.view import render
 
+    `s3_cookie` is the patron's encrypted `s3` cookie. Callers pass it in
+    because FastAPI requests have no web.py context to read it from.
+
+    Raises AccountNotFoundError when `username` has no Open Library account.
+    """
     if not OpenLibraryAccount.get_by_username(username):
-        raise render.notfound("Account not found for %s" % username, create=False)
+        raise AccountNotFoundError(f"Account not found for {username}")
 
-    s3_keys = parse_s3_cookie(web.cookies().get("s3"))
+    s3_keys = parse_s3_cookie(s3_cookie)
     limit = RESULTS_PER_PAGE
     offset = page * limit - limit
 

openlibrary/fastapi/account.py

@@ -21,6 +21,7 @@
 from openlibrary.core.auth import ExpiredTokenError, HMACToken, MissingKeyError
 from openlibrary.core.env import get_ol_env
 from openlibrary.core.follows import PubSub
+from openlibrary.core.lending import AccountNotFoundError
 from openlibrary.fastapi.auth import (
     AuthenticatedUser,
     get_authenticated_user,
@@ -220,11 +221,14 @@ def account_loans_json(
 
 @router.get("/account/loan-history.json", response_model=AccountLoanHistoryResponse)
 def account_loan_history_json(
+    request: Request,
     _: Annotated[AuthenticatedUser, Depends(require_authenticated_user)],
     page: Annotated[int, Query(ge=1)] = 1,
 ) -> dict:
     try:
-        return legacy_account.get_account_loan_history_json(accounts.get_current_user(), page)
+        return legacy_account.get_account_loan_history_json(accounts.get_current_user(), page, s3_cookie=request.cookies.get("s3"))
+    except AccountNotFoundError as e:
+        raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e))
     except Exception:
         if get_ol_env().LOCAL_DEV:
             raise HTTPException(

openlibrary/plugins/upstream/account.py

@@ -43,7 +43,7 @@
 from openlibrary.core.carousels import get_carousel_data
 from openlibrary.core.db import get_db  # noqa: TID251
 from openlibrary.core.follows import PubSub
-from openlibrary.core.lending import get_loan_history_data
+from openlibrary.core.lending import AccountNotFoundError, get_loan_history_data
 from openlibrary.core.observations import Observations
 from openlibrary.core.ratings import Ratings
 from openlibrary.i18n import gettext as _
@@ -1256,9 +1256,9 @@ def get_account_loans_json(user: User) -> dict[str, Any]:
     return {"loans": loans}
 
 
-def get_account_loan_history_json(user: User, page: int) -> dict[str, Any]:
+def get_account_loan_history_json(user: User, page: int, s3_cookie: str | None) -> dict[str, Any]:
     username = user["key"].split("/")[-1]
-    loan_history_data = dict(get_loan_history_data(username, page=page))
+    loan_history_data = dict(get_loan_history_data(username, page=page, s3_cookie=s3_cookie))
     # Ensure all `docs` are `dicts`, as some are `Edition`s.
     loan_history_data["docs"] = [loan.dict() if not isinstance(loan, dict) else loan for loan in loan_history_data["docs"]]
     return {"loans_history": loan_history_data}
@@ -1293,7 +1293,10 @@ def GET(self):
         provider = lenny.provider_loans(username)
         docs = list(docs) + provider.loans
 
-        loan_history_data = get_loan_history_data(username, page=page)
+        try:
+            loan_history_data = get_loan_history_data(username, page=page, s3_cookie=web.cookies().get("s3"))
+        except AccountNotFoundError as e:
+            raise render.notfound(str(e), create=False)
 
         staff_picks = get_carousel_data(carousels=("staff_picks",))["staff_picks"]
 
@@ -1346,8 +1349,12 @@ def GET(self):
         i = web.input(page=1)
         page = int(i.page)
         user = accounts.get_current_user()
+        try:
+            data = get_account_loan_history_json(user, page, s3_cookie=web.cookies().get("s3"))
+        except AccountNotFoundError as e:
+            raise render.notfound(str(e), create=False)
         web.header("Content-Type", "application/json")
-        return delegate.RawText(json.dumps(get_account_loan_history_json(user, page)))
+        return delegate.RawText(json.dumps(data))
 
 
 class account_waitlist(delegate.page):

openlibrary/plugins/upstream/mybooks.py

@@ -140,7 +140,7 @@ def render_template(self, mb: MyBooksTemplate) -> TemplateResult:
             history_books = []
             if mb.is_my_page:
                 try:
-                    history_data = get_loan_history_data(mb.username, page=1)
+                    history_data = get_loan_history_data(mb.username, page=1, s3_cookie=web.cookies().get("s3"))
                     history_books = [doc for doc in history_data.get("docs", []) if not doc.get("ia_only")]
                 except Exception:
                     # Deliberately non-fatal: My Books must still render its

openlibrary/tests/core/test_lending.py

@@ -248,12 +248,11 @@ def test_returns_empty_history_when_patron_has_no_s3_keys(self):
         response.json.return_value = {"history": {"items": []}}
         with (
             patch.object(lending.OpenLibraryAccount, "get_by_username", return_value=Mock()),
-            patch("openlibrary.core.lending.web.cookies", return_value={"s3": "irrelevant"}),
             patch("openlibrary.core.lending.parse_s3_cookie", return_value=None),
             patch("openlibrary.core.lending.s3_loan_api", return_value=response) as mock_api,
             patch("openlibrary.core.lending.get_items_and_add_availability", return_value={}),
         ):
-            result = lending.get_loan_history_data("someuser", page=1)
+            result = lending.get_loan_history_data("someuser", page=1, s3_cookie="irrelevant")
 
         # Must short-circuit: calling the real s3_loan_api with None keys raises
         # TypeError on `s3_keys | kwargs`, which 500s /account/loans.
@@ -273,12 +272,21 @@ def test_still_queries_ia_when_s3_keys_are_present(self):
         response.json.return_value = {"history": {"items": []}}
         with (
             patch.object(lending.OpenLibraryAccount, "get_by_username", return_value=Mock()),
-            patch("openlibrary.core.lending.web.cookies", return_value={"s3": "irrelevant"}),
             patch("openlibrary.core.lending.parse_s3_cookie", return_value={"access": "a", "secret": "s"}),
             patch("openlibrary.core.lending.s3_loan_api", return_value=response) as mock_api,
             patch("openlibrary.core.lending.get_items_and_add_availability", return_value={}),
         ):
-            result = lending.get_loan_history_data("someuser", page=1)
+            result = lending.get_loan_history_data("someuser", page=1, s3_cookie="irrelevant")
 
         mock_api.assert_called_once()
         assert result["docs"] == []
+
+    def test_unknown_account_raises_account_not_found(self):
+        with (
+            patch.object(lending.OpenLibraryAccount, "get_by_username", return_value=None),
+            patch("openlibrary.core.lending.s3_loan_api") as mock_api,
+            pytest.raises(lending.AccountNotFoundError),
+        ):
+            lending.get_loan_history_data("nobody", page=1, s3_cookie="irrelevant")
+
+        mock_api.assert_not_called()

openlibrary/tests/fastapi/test_account_loans.py

@@ -82,6 +82,36 @@ def test_loan_history_json_serializes_docs_and_forwards_page(self, fastapi_clien
         assert get_data.called
         assert get_data.call_args.kwargs["page"] == page
 
+    def test_loan_history_json_passes_request_s3_cookie(self, fastapi_client, mock_authenticated_user):
+        """Regression for #13959: the route used to 500 with
+        AttributeError: 'ThreadedDict' object has no attribute 'env' because
+        get_loan_history_data() read web.cookies() on a FastAPI worker thread."""
+        response = MagicMock()
+        response.json.return_value = {"history": {"items": []}}
+
+        with (
+            patch("openlibrary.fastapi.account.accounts.get_current_user", return_value=FakeLegacyUser()),
+            patch("openlibrary.core.lending.OpenLibraryAccount.get_by_username", return_value=MagicMock()),
+            patch("openlibrary.core.lending.parse_s3_cookie", return_value={"access": "a", "secret": "s"}) as parse_s3_cookie,
+            patch("openlibrary.core.lending.s3_loan_api", return_value=response),
+            patch("openlibrary.core.lending.get_items_and_add_availability", return_value={}),
+        ):
+            fastapi_client.cookies.set("s3", "a%3As")
+            result = fastapi_client.get("/account/loan-history.json")
+
+        assert result.status_code == 200
+        assert result.json() == {"loans_history": {"docs": [], "show_next": False, "limit": 25, "page": 1}}
+        parse_s3_cookie.assert_called_once_with("a%3As")
+
+    def test_loan_history_json_returns_404_when_account_missing(self, fastapi_client, mock_authenticated_user):
+        with (
+            patch("openlibrary.fastapi.account.accounts.get_current_user", return_value=FakeLegacyUser()),
+            patch("openlibrary.core.lending.OpenLibraryAccount.get_by_username", return_value=None),
+        ):
+            result = fastapi_client.get("/account/loan-history.json")
+
+        assert result.status_code == 404
+
     def test_loan_history_json_rejects_invalid_page(self, fastapi_client, mock_authenticated_user):
         response = fastapi_client.get("/account/loan-history.json?page=0")
 
@@ -147,12 +177,11 @@ def test_get_loan_history_data_handles_missing_or_null_history(self, mock_respon
 
         with (
             patch("openlibrary.core.lending.OpenLibraryAccount.get_by_username", return_value=mock_account),
-            patch("openlibrary.core.lending.web.cookies", return_value={"s3": "irrelevant"}),
             patch("openlibrary.core.lending.parse_s3_cookie", return_value={"access": "acc", "secret": "sec"}),
             patch("openlibrary.core.lending.s3_loan_api", return_value=mock_response) as mock_s3_loan_api,
             patch("openlibrary.core.lending.get_items_and_add_availability", return_value={}) as mock_availability,
         ):
-            result = get_loan_history_data(username="testuser", page=1)
+            result = get_loan_history_data(username="testuser", page=1, s3_cookie="irrelevant")
 
             assert result == {
                 "docs": [],

openlibrary/tests/fastapi/test_mybooks_integration.py

@@ -10,10 +10,20 @@
 
 from unittest.mock import MagicMock, patch
 
+import pytest
+
 from openlibrary.plugins.upstream.mybooks import MyBooksTemplate, mybooks_home
 from openlibrary.utils.request_context import RequestContextVars, req_context
 
 
+@pytest.fixture(autouse=True)
+def _s3_cookie():
+    """render_template is a web.py handler and reads the patron's `s3` cookie
+    with web.cookies(), which needs a web.py request these tests don't build."""
+    with patch("openlibrary.plugins.upstream.mybooks.web.cookies", return_value={"s3": "test-s3"}):
+        yield
+
+
 def _req_context():
     req_context.set(
         RequestContextVars(

openlibrary/tests/fastapi/test_unified_loans_carousel.py

@@ -1,9 +1,19 @@
 from unittest.mock import MagicMock, patch
 
+import pytest
+
 from openlibrary.plugins.upstream.mybooks import MyBooksTemplate, mybooks_home
 from openlibrary.utils.request_context import RequestContextVars, req_context
 
 
+@pytest.fixture(autouse=True)
+def _s3_cookie():
+    """render_template is a web.py handler and reads the patron's `s3` cookie
+    with web.cookies(), which needs a web.py request these tests don't build."""
+    with patch("openlibrary.plugins.upstream.mybooks.web.cookies", return_value={"s3": "test-s3"}):
+        yield
+
+
 def test_unified_loans_carousel_merges_active_and_history():
     # Setup req_context to prevent LookupError
     req_context.set(RequestContextVars(x_forwarded_for=None, user_agent=None, lang="en", solr_editions=True, print_disabled=False, is_bot=False))
@@ -237,5 +247,4 @@ def test_loan_history_still_fetched_on_own_profile():
     mb.username = "viewer"
     _run_render(mb, mock_history_data)
 
-    mock_history_data.assert_called_once()
-    assert mock_history_data.call_args.args[0] == "viewer"
+    mock_history_data.assert_called_once_with("viewer", page=1, s3_cookie="test-s3")