Continue Reading Phase 1: Merge 'Active Loans' with 'Loan History' on 'My Books' page - #13281

@Sadashii have you run this locally and visually QA'd? As is implemented, it's a bit confusing because it still says, "My Loans (20)"; we may want to switch to Continue Reading.

It would be useful to have screenshots attached to the PR from testing and also any questions you have or feedback you would like.

In particular, we'll want to get feedback from @lokesh and the community on the rest of the UI (e.g. the return button iconography, color, etc)

i18n'd text overflows (e.g. german)

• Remove the total count number

PR #13281 — Rebase + verification (Ada, on behalf of Mek)

@Sadashii — this branch was 72 commits behind master and CONFLICTING, so with Mek's authorization I rebased it onto current master and pushed the result, then pushed three fixes on top. Details below so nothing here is a surprise.

The rebase

All 7 of your commits are preserved: same author, same author dates, same messages, in the same order. Exactly one file conflicted — openlibrary/templates/account/mybooks.html — where master's unified icon system (#12955) had rewritten the chevron <img> inside the same "Loan History" sidebar <li> your commit deletes. Resolution was the deletion, i.e. your change, applied to master's version of that block. The only line-level difference across the whole rebase is that the removed line is now master's $:macros.icon("chevron-right", ...) spelling instead of the old <img ...>.

I checked this by merging master into your original branch independently and comparing: both routes produce an identical tree. Your pre-rebase head (4707d60) is unchanged in history terms and remains recoverable.

Fixes pushed on top

89ebd40 — ReturnForm.html analytics attribute was HTML-escaped. The new analytics_track parameter built the whole attribute as one string:

$('data-ol-link-track="' + analytics_track + '"' if analytics_track else '')

$(...) escapes in Templetor, so the quotes rendered as " and the attribute came out unquoted. A browser parses the value as "LoansAndHistory|ReturnClick" — literal quote characters included — and ol.analytics.js:65 splits it on | into category "LoansAndHistory and action ReturnClick". That silently corrupted both events this branch adds (LoansAndHistory|ReturnClick on the loans table, LoanStatus|ReturnClick on book details and search results), so the tracking commit wasn't landing usable data.

This is the same pattern removed from four templates in 4285eb85 (#13186) — worth knowing about generally, since the escaped quotes are inert text rather than real delimiters, so a value containing a space can also inject a new attribute. Switched to the idiom established there (an inline $if with a static, template-source quote) and added regression coverage to test_link_track_attribute_escaping.py that exercises the real macro. Confirmed the new test fails against the previous version and passes against the fix.

fe25aae — account/loan_history.html was orphaned. Folding the list into /account/loans removed the only render["account/loan_history"] call, but the template stayed on disk. Master has since added openlibrary/tests/test_unused_templates.py, which fails on it — so this was going to block CI regardless of the rebase. Deleted it; its markup is superseded by the new Loan History section in loans.html, and the include_ratings/StarRatings branch was already dead (the old call site never passed ratings). messages.pot is regenerated accordingly — no strings added or removed, only source references moved.

597217d — loan-history fetch is now gated on ownership. render_template() guarded with if mb.me:, which only asks whether someone is signed in, then fetched history for mb.username — the profile in the URL, not the session. Since get_loan_history_data() resolves S3 credentials for whichever username it is handed, a signed-in visitor to /people/<someone-else>/books triggered a borrow-history lookup against that account. Nothing was shown to the visitor — mybooks.html gates both carousels behind $if owners_page: — but that guard is in the view layer, so the data layer was one template change away from a problem. MyBooksTemplate already exposes the right flag (mb.is_my_page), so this uses it. It also removes an external archive.org round-trip from every profile page view. Only the fetch this branch introduced is gated; the pre-existing get_loans_of_user(mb.me.key) call is untouched.

Verification

Docker, on the rebased branch: 5745 passed, 18 skipped. The one remaining failure, test_home.py::TestHomeTemplates::test_home_template, reproduces on clean origin/master — pre-existing, unrelated to this PR. test_unused_templates.py passes (it errors in a git-worktree checkout for an unrelated tooling reason; verified with the real gitdir mounted). Your own 14 tests all pass, and the null-safety cases for the IA history payload are a genuinely good addition. pre-commit clean including mypy. Logged in locally and confirmed /account/loans and the My Books page both render 200 with no errors, all three sections present, sidebar and breadcrumb consolidated to a single "Loans & History", and no stale /account/loan-history links.

Not fixed — worth your call

• Three <h1>s on /account/loans. "Active Loan(s) (N)", "Books You're Waiting For", and "Loan History" are all <h1 class="details-title">; only the middle one pre-existed. The <section> wrappers in 4707d60 don't resolve it — the HTML5 outline algorithm was never implemented by browsers or assistive tech, so all three still announce as top-level headings. <h2> for the section headings would fix it (WCAG 1.3.1). Related: the new return <button> correctly has both title and aria-label, but its inline <svg> could use aria-hidden="true", and <span style="opacity: 0.9;"> on "Return via ADE" is worth a contrast check.
• get_loan_history_data() on the My Books landing page is a synchronous archive.org call on a page that previously made none, wrapped in a bare except Exception: with no logging — so an IA outage degrades the carousel silently. Worth logging, and possibly caching.
• total_results for the loans carousel changed meaning: it was len(myloans), now len(merged_books) (active loans plus up to 25 history items) while only 18 render ([:18]). Currently invisible because mybooks.html suppresses the count for key == "loans", but the value no longer matches what's displayed.
• Minor: RESULTS_PER_PAGE at account.py:65 is now dead (logic moved to lending.py), and for _ in range(5) in mybooks.py shadows the module-level gettext _ for that function's scope — harmless today, but a TypeError waiting for whoever adds a _() call there.

Overall

Nice piece of work — the merge logic, the active-over-returned ranking, and the redirect handling all hold up under test. The two blockers are fixed and pushed; the rest above is advisory and yours to take or leave. Human review and merge remain with Mek.