fix(aladin): use ItemLookUp for ISBN search instead of keyword lookup#470
Conversation
ISBN queries were sent to ItemSearch.aspx as keywords, missing ~24% of valid Korean ISBNs. Route ISBN searches through ItemLookUp.aspx with ItemIdType=ISBN13 (13-digit) or ISBN (10-digit), falling back to keyword search for unrecognised formats. Closes bookorbit#437
github-actions
Bot
added
the
server
📝 WalkthroughWalkthrough
Aladin ISBN Lookup Routing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/src/modules/metadata-fetch/providers/aladin/aladin.provider.test.ts (1)
144-181: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the normalization contract, not just the routed endpoint.
Line 150 and Line 165 only exercise already-normalized ISBNs, so a regression in the hyphen/space stripping path would still pass. Please add at least one hyphenated/spaced ISBN case and assert
toHaveBeenCalledTimes(1)so these tests also fail if the provider accidentally hits bothItemLookUp.aspxandItemSearch.aspx.Example hardening
- const result = await provider.search({ isbn: '9788912345678' }); + const result = await provider.search({ isbn: '978-89-1234-567-8' }); expect(global.fetch).toHaveBeenCalledWith(expect.stringContaining('ItemLookUp.aspx'), expect.any(Object)); expect(global.fetch).toHaveBeenCalledWith(expect.stringContaining('ItemId=9788912345678'), expect.any(Object)); expect(global.fetch).toHaveBeenCalledWith(expect.stringContaining('ItemIdType=ISBN13'), expect.any(Object)); + expect(global.fetch).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/modules/metadata-fetch/providers/aladin/aladin.provider.test.ts` around lines 144 - 181, The Aladin provider search tests currently verify routing for already-normalized ISBNs only, so they miss regressions in the normalization path. Update the relevant `provider.search` cases in `aladin.provider.test.ts` to include at least one hyphenated or spaced ISBN input and assert `global.fetch` is called exactly once, while still checking the expected `ItemLookUp.aspx` or `ItemSearch.aspx` URL. This should harden the `search` contract so a bug that triggers both lookup and fallback search is caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/modules/metadata-fetch/providers/aladin/aladin.provider.ts`:
- Around line 18-26: The ISBN-10 detection in getIsbnItemIdType only accepts an
uppercase X, so valid lowercase x inputs are missed and fall back to keyword
search. Update the ISBN-10 regex in getIsbnItemIdType to accept both X and x
after normalizeIsbn, keeping the behavior for ISBN13 unchanged and preserving
the existing normalization flow in normalizeIsbn.
---
Nitpick comments:
In `@server/src/modules/metadata-fetch/providers/aladin/aladin.provider.test.ts`:
- Around line 144-181: The Aladin provider search tests currently verify routing
for already-normalized ISBNs only, so they miss regressions in the normalization
path. Update the relevant `provider.search` cases in `aladin.provider.test.ts`
to include at least one hyphenated or spaced ISBN input and assert
`global.fetch` is called exactly once, while still checking the expected
`ItemLookUp.aspx` or `ItemSearch.aspx` URL. This should harden the `search`
contract so a bug that triggers both lookup and fallback search is caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 210b3d0d-5d44-43fa-99ae-5b85c1668571
📒 Files selected for processing (2)
server/src/modules/metadata-fetch/providers/aladin/aladin.provider.test.tsserver/src/modules/metadata-fetch/providers/aladin/aladin.provider.ts
| function normalizeIsbn(isbn: string): string { | ||
| return isbn.replace(/[-\s]/g, ''); | ||
| } | ||
|
|
||
| function getIsbnItemIdType(isbn: string): 'ISBN13' | 'ISBN' | null { | ||
| const normalized = normalizeIsbn(isbn); | ||
| if (/^\d{13}$/.test(normalized)) return 'ISBN13'; | ||
| if (/^\d{9}[\dX]$/.test(normalized)) return 'ISBN'; | ||
| return null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Accept lowercase x in ISBN-10 normalization.
Line 25 only recognizes uppercase X, so valid inputs like 0-8044-2957-x now miss the new lookup path and fall back to keyword search.
Proposed fix
function normalizeIsbn(isbn: string): string {
- return isbn.replace(/[-\s]/g, '');
+ return isbn.replace(/[-\s]/g, '').toUpperCase();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function normalizeIsbn(isbn: string): string { | |
| return isbn.replace(/[-\s]/g, ''); | |
| } | |
| function getIsbnItemIdType(isbn: string): 'ISBN13' | 'ISBN' | null { | |
| const normalized = normalizeIsbn(isbn); | |
| if (/^\d{13}$/.test(normalized)) return 'ISBN13'; | |
| if (/^\d{9}[\dX]$/.test(normalized)) return 'ISBN'; | |
| return null; | |
| function normalizeIsbn(isbn: string): string { | |
| return isbn.replace(/[-\s]/g, '').toUpperCase(); | |
| } | |
| function getIsbnItemIdType(isbn: string): 'ISBN13' | 'ISBN' | null { | |
| const normalized = normalizeIsbn(isbn); | |
| if (/^\d{13}$/.test(normalized)) return 'ISBN13'; | |
| if (/^\d{9}[\dX]$/.test(normalized)) return 'ISBN'; | |
| return null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/modules/metadata-fetch/providers/aladin/aladin.provider.ts` around
lines 18 - 26, The ISBN-10 detection in getIsbnItemIdType only accepts an
uppercase X, so valid lowercase x inputs are missed and fall back to keyword
search. Update the ISBN-10 regex in getIsbnItemIdType to accept both X and x
after normalizeIsbn, keeping the behavior for ISBN13 unchanged and preserving
the existing normalization flow in normalizeIsbn.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
🎉 This PR is included in version 2.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
github-actions
Bot
added
the
released
2 participants
What does this PR do?
Closes #437
The Aladin provider's
search()was sending ISBNs toItemSearch.aspxas a keyword query (QueryType: 'Keyword'). This endpoint misses ~24% of valid Korean ISBNs thatItemLookUp.aspxreturns reliably. This PR routes ISBN searches throughItemLookUp.aspxwith the correctItemIdType(ISBN13 for 13-digit, ISBN for 10-digit), mirroring the pattern used by the Goodreads provider's ISBN-specific endpoint.How did you test this?
vitest run src/modules/metadata-fetch/providers/aladin/— 26 tests passpnpm run typecheck— passes (server + client + types)Screenshots
N/A — backend-only change.
Anything non-obvious in the diff?
buildLookupUrl()signature changed to accept an optionalitemIdTypeparameter (default'ItemId'). The existinglookupItem()path is unaffected — onlysearch()benefits from the new ISBN routing.normalizeIsbn()strips hyphens/spaces before sending, since ISBNs in the wild often contain them.Checklist
Summary by CodeRabbit
Bug Fixes
Tests