substituting /static/images for /images in all urls -fixes 12208#12352
substituting /static/images for /images in all urls -fixes 12208#12352jack-wines wants to merge 8 commits intointernetarchive:masterfrom
Conversation
|
Thank you for your contribution, @jack-wines — and welcome to Open Library! 🤖 Copilot has been assigned for an initial review. @RayBB is assigned to this PR and currently has:
PR triage checklist (maintainers / Pam)
Note This comment was automatically generated by Pam, Open Library's Project AI Manager, on behalf of @mekarpeles. Pam is designed to provide status visibility, perform basic project management functions and relevant codebase research, and provide actionable feedback so contributors aren't left waiting. |
There was a problem hiding this comment.
Pull request overview
Refactors Open Library UI, templates, and tests to consistently reference static image assets via /static/images instead of /images, addressing environment-specific routing differences that caused image 404s in testing (Closes #12208).
Changes:
- Updated many templates/macros and JS fallbacks to use
/static/images/...for icons, loaders, and placeholder covers. - Updated test fixtures/expectations to match the new
/static/images/...URLs. - Updated static assets referencing (e.g.,
manifest.json,status-500.html) to point at/static/images/....
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/js/sample-html/lists-test-data.js | Updates default cover URL in sample markup data to /static/images. |
| tests/unit/js/lists.test.js | Updates unit test cover URL expectations to /static/images. |
| static/status-500.html | Updates 500 error page logo to /static/images. |
| static/manifest.json | Updates PWA screenshot URLs to /static/images. |
| static/css/components/generic-dropper.css | Updates dropper icon URL (but currently reintroduces /images in one rule). |
| openlibrary/templates/type/list/embed.html | Updates logo and default cover URL to /static/images. |
| openlibrary/templates/type/author/view.html | Updates loader gif URL to /static/images. |
| openlibrary/templates/type/author/edit.html | Updates help icon URL to /static/images. |
| openlibrary/templates/search/sort_options.html | Updates sort icon URL to /static/images. |
| openlibrary/templates/lists/snippet.html | Updates default cover URL to /static/images. |
| openlibrary/templates/lists/showcase.html | Updates default cover URL to /static/images. |
| openlibrary/templates/lists/preview.html | Updates default cover URL to /static/images. |
| openlibrary/templates/lists/list_follow.html | Updates default cover URL to /static/images. |
| openlibrary/templates/lists/home.html | Updates default cover URL to /static/images. |
| openlibrary/templates/covers/book_cover.html | Updates placeholder book cover + srcset URLs to /static/images. |
| openlibrary/templates/covers/book_cover_small.html | Updates small placeholder cover URLs to /static/images. |
| openlibrary/templates/covers/author_photo.html | Updates placeholder author image URL to /static/images. |
| openlibrary/templates/books/edition-sort.html | Updates fallback cover URL to /static/images. |
| openlibrary/templates/books/edit/edition.html | Updates dimensions image URL to /static/images. |
| openlibrary/templates/account/not_verified.html | Updates alert background icon URL to /static/images. |
| openlibrary/plugins/upstream/tests/test_addbook.py | Updates expected cover_url to /static/images. |
| openlibrary/plugins/upstream/code.py | Changes dev redirect handler route (currently results in incorrect redirect target). |
| openlibrary/plugins/upstream/addbook.py | Updates make_work fallback cover_url to /static/images. |
| openlibrary/plugins/openlibrary/lists.py | Updates list API cover_url fallback to /static/images. |
| openlibrary/plugins/openlibrary/js/my-books/MyBooksDropper/ReadingLists.js | Updates default cover URL constant to /static/images. |
| openlibrary/plugins/openlibrary/js/lists/ShowcaseItem.js | Updates default cover URL constant to /static/images. |
| openlibrary/plugins/openlibrary/js/covers.js | Updates author placeholder image URL to /static/images. |
| openlibrary/macros/SearchResultsWork.html | Updates default cover URL to /static/images. |
| openlibrary/macros/LoadingIndicator.html | Updates loader gif URL to /static/images. |
| openlibrary/macros/FulltextSearchSuggestionItem.html | Updates default cover URL to /static/images. |
| openlibrary/macros/CoverImage.html | Updates default cover URL to /static/images. |
/images remains in the ngnix config, on the off chance that I missed a couple
10c506b to
97b5811
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Put it on testing and it resolves the issue. I think this is meaningful improvement. Tagging @lokesh just for a quick sanity check in case he has concerns. |
| logger = logging.getLogger('openlibrary.plugins.upstream.code') | ||
|
|
||
|
|
||
| # Note: This is done in web_nginx.conf on production ; this endpoint is |
There was a problem hiding this comment.
This PR removes the shim for dev environments, which is a good idea, but there are quite a few image references remaining that need to have their url updated to include the /static/ folder or they will break on dev.
It might appear they are still working when you test locally, but try clearing site data and trying again.
All the image urls that need fixing still are in CSS files.
There was a problem hiding this comment.
oh good call! I'll clear cache and take a second look.
|
The failing test is in a file that wasn't changed. It passes locally. In the meantime there's a merge conflict with the master branch, which ran linting on the js and added semicolons where there weren't previously. I'll pull and rebase. |
Closes #12208
refactor, the ui and tests no longer call /images, only /static/images
Technical
Different environments route /images slightly differently, some redirect, others substitute entirely. The original bug was an image only 404ing in the testing environment.
/images remains in the ngnix config, on the off chance that I missed a couple
Testing
deploy to testing environment, check nothings hits /images, and nothing hitting /static fails
Screenshot
Stakeholders
@RayBB