Skip to content

Redirect DAISY pages to Archive item pages#13049

Merged
mekarpeles merged 2 commits into
internetarchive:masterfrom
nathanpixodeo:9439/fix/daisy-archive-redirect
Jul 21, 2026
Merged

Redirect DAISY pages to Archive item pages#13049
mekarpeles merged 2 commits into
internetarchive:masterfrom
nathanpixodeo:9439/fix/daisy-archive-redirect

Conversation

@nathanpixodeo

Copy link
Copy Markdown
Contributor

Refs #9439

Fixes the stale /books/.../daisy flow by redirecting editions with an ocaid to their Internet Archive item page instead of rendering the old page with a guessed _daisy.zip download URL.

Technical

  • Adds a redirect from the DAISY route to https://archive.org/details/{ocaid} when an edition has an Archive identifier.
  • URL-escapes the Archive identifier path segment before redirecting.
  • Keeps the existing no-ocaid fallback page behavior.

Testing

  • python -m py_compile openlibrary/plugins/upstream/addbook.py openlibrary/plugins/upstream/tests/test_addbook.py
  • python -m ruff check openlibrary/plugins/upstream/addbook.py openlibrary/plugins/upstream/tests/test_addbook.py
  • Attempted targeted pytest locally, but this machine only has Python 3.13 while Open Library requires Python 3.14; pytest import fails under 3.13 before reaching this test because annotations in openlibrary/core/helpers.py are evaluated differently.

Screenshot

Not applicable; this is a redirect behavior change.

Stakeholders

@mekarpeles

@openlibrary-bot

Copy link
Copy Markdown
Collaborator

Thank you @nathanpixodeo for submitting this issue! Welcome to Open Library — this is your first contribution! 🎉

🤖 Copilot has been assigned for an initial review.

A reviewer must first be assigned. There are currently 104 open non-draft PRs of equal or higher priority ahead of yours.

PR triage checklist (maintainers / Richy)
  • PR description — not empty; explains what the change does and how to verify it
  • References an issue — PR body contains #9439
  • Commit history clean — 1 clean commit with meaningful message
  • CI passing — pre-commit.ci pending
  • Test cases present — 50 lines added to tests/test_addbook.py covering the redirect behavior
  • Proof of testing — PR body documents py_compile and ruff check passes; explains pytest environment constraint

Note

This comment was automatically generated by PAM, Open Library's Project AI Manager. PAM provides status visibility, performs basic project management functions, and gives actionable feedback so contributors aren't left waiting.

@github-actions github-actions Bot added the Needs: Response Issues which require feedback from lead label Jun 26, 2026
@mekarpeles mekarpeles removed the Needs: Response Issues which require feedback from lead label Jul 4, 2026
@mekarpeles mekarpeles self-assigned this Jul 6, 2026
@openlibrary-bot

Copy link
Copy Markdown
Collaborator

Verification (Ada, on behalf of Mek): Checked out this branch in an isolated worktree + Docker (Python 3.14, matching CI) and independently verified every claim in the PR body and diff.

Result: The fix works as described and I found no regressions. One thing worth a deliberate decision: the redirect status code (details below).

Details:

  1. The two new tests actually pass — the author flagged they could only run py_compile/ruff locally (Python 3.13 vs. required 3.14). I ran them for real under 3.14:
    test_redirects_to_archive_item_for_edition_with_ocaid PASSED
    test_redirect_escapes_archive_item_identifier PASSED
    
  2. CI has not actually run these tests either — current status rollup on this PR is only labeler, AccessLint, pre-commit.ci; there's no python_tests entry. So until now, nobody had confirmed these tests pass.
  3. Full test_addbook.py suite: 16 passed, 2 pre-existing failures in TestMakeWork (test_make_author_adds_the_correct_key, test_make_work_does_indeed_make_a_work) — unrelated to this change. I confirmed these two fail identically on master before this PR's commit (that test class never sets up web.ctx.site/the site ContextVar, unlike the new TestDaisyPage class, which does it correctly in setup_method). Not a regression from this PR.
  4. Live behavior, real dev-DB editions (not just unit tests):
    • /books/OL24982896M/Pioneers/daisy (edition with ocaid=pioneersfrontier00macd) → 303 See OtherLocation: https://archive.org/details/pioneersfrontier00macd. Confirmed correct.
    • /books/OL4M/…/daisy (edition with no ocaid) → 200 OK, same old fallback page renders unchanged ("There isn't a DAISY file available for…").
  5. Escaping claim: confirmed the exact urllib.parse.quote call the PR added produces https://archive.org/details/test%20item%2Farchive for an ocaid of test item/archive — matches the PR's own test exactly. Note: I wasn't able to persist a new dev-DB edition with a special-character ocaid to drive this end-to-end over real HTTP (fresh dev DB has no working admin login/auth-token path for scripted writes — a known local-dev-tooling gap, unrelated to this PR). Instead I exercised the real imported addbook.daisy handler's actual code directly against a stand-in object with that ocaid, confirming the real running code (not a reimplementation) produces the correctly-escaped URL.
  6. Redirect semantics worth a deliberate call: web.seeother issues a real 303 See Other (confirmed via curl). This is intentionally a permanent behavior change — any edition with an ocaid will now always redirect here, not just this one response — so a 301 Moved Permanently (or 308, though method-preservation doesn't matter for a GET-only route) would signal that more accurately to caches and crawlers than a 303 does. That said, 303/web.seeother is what this codebase already uses for other key-based redirects nearby (e.g. the /cover redirect in code.py), so it's consistent with existing convention. Flagging for a conscious choice rather than treating it as a defect.

No blockers found. Nice fix — small, well-tested once actually run, and it removes a genuinely stale/broken guessed-URL page.

@mekarpeles
mekarpeles merged commit 74010c6 into internetarchive:master Jul 21, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants