Feat/documents biblio author sort - #1742
Conversation
…ographic data retrieval
|
Is this PR related to an existing issue? |
ClemRz
left a comment
There was a problem hiding this comment.
Suggestions (Should Consider)
-
[assets/swaggerV1.yaml ~L4805] The advanced-search
sortdescription listsdataQualityas an entity-specific field but doesn't mentionauthorsSort. Since this field's whole purpose is a sortable author key and it silently returns 400 for non-document entities, it's worth documenting alongsidedataQuality:Entity-specific sort fields: - entrances: "dataQuality:asc" or "dataQuality:desc" (sort by data quality score) - documents: "authorsSort:asc" or "authorsSort:desc" (sort by alphabetical-first author)
The second
sortdescription block (around line 4893) could also benefit from the same addition for completeness. -
[api/utils/computeDocumentAuthorsSort.js] This is a pure, side-effect-free function — exactly the kind the tech stack targets with
fast-checkproperty-based tests. The current test suite covers concrete examples well, but a few properties are worth encoding as invariants:- Normalization idempotency:
computeDocumentAuthorsSort([x])should equalcomputeDocumentAuthorsSort([normalizeName(x)])for any name. - Sentinel ordering: for any non-empty input,
computeDocumentAuthorsSort([x]) < EMPTY_AUTHORS_SORT_KEY(the sentinel always sorts last). - Pool commutativity: the result must not depend on whether a name is in the
personNamesororganizationNamesargument — swapping them should produce the same output.
These invariants would catch regressions in
normalizeNameor the pool-merge logic that hand-picked examples might miss. - Normalization idempotency:
Nitpicks (Optional)
-
[test/integration/2_utils/computeDocumentAuthorsSort.test.js:1] The
/* eslint-disable func-names */directive at the top is a no-op here — there are nofunctionkeyword expressions in this file (all blocks use arrow functions). It appears to have been copied from the.property.test.jsfiles in the same directory, where it's needed becausefc.assertcallbacks usefunction. Safe to remove. -
[api/services/DocumentService.js:499] Minor code quality note:
const [parents] = await Promise.all([...])destructures only the first element, leaving the second (NameService.setNamesresult) unnamed. This is intentional sincesetNamesmutates in place, but it can look like a bug to a new reader. A comment like// setNames mutates grottos in place; result is intentionally unusedwould make the intent immediately obvious without needing to trace into the service.
ClemRz
left a comment
There was a problem hiding this comment.
Issues (Must Fix)
- [api/controllers/v1/caver/delete.js]
toCavernow callstoCitationDocumentfor documents (changed inconverters.js), butcaver/delete.jspasses the caver directly fromCaverService.getCaver()without theDocumentService.getDocumentsForCitation()enrichment step thatcaver/find.jscorrectly adds. The documents at that point only have the basic Waterline.populate('documents', ...)fields — noauthors,authorsOrganization,identifierType,editor, orlibrary.toCitationDocumentwon't crash (toList returns[]for missing fields), but the delete response will silently return documents with empty citation arrays, which is now a schema contract violation sinceCitationDocumentis the advertised shape. Either add the enrichment step before thetoCavercall:or revert thecaver.documents = await DocumentService.getDocumentsForCitation( caver.documents.map((d) => d.id) );
delete.jsresponse to use a converter that matches the actual data shape (toSimpleDocument).
Suggestions (Should Consider)
-
[test/integration/4_routes/Search/] There are no route-level tests for the new
authorsSortentity guard. The analogousdataQualityguard has a dedicated test file (advanced-search-data-quality-sort.test.js) with cases for valid use (documents), invalid use on a non-document entity (expecting 400), and multi-field sort combinations. Adding the equivalent forauthorsSortwould lock in the 400 behaviour and catch any future refactors toENTITY_SPECIFIC_SORT_FIELDS. The existing test pattern (sinon stub onSearchService.collectionSearch+ supertest) can be reused directly. -
[assets/swaggerV1.yaml —
SimpleOrganizationschema] The newSimpleOrganizationschema has anamefield typed asstring. However,toSimpleOrganizationinconverters.jsdelegates togetMainName(source), which can returnnullwhen no name is available. Consider addingnullable: trueto thenameproperty to avoid clients misinterpreting a null as a missing field:name: type: string nullable: true
-
[api/services/DocumentService.js:503 —
getDocumentsForCitation] The JSDoc says "Same asgetDocuments()" but the new method also fetchesidentifierType,editor, andlibrary, whichgetDocuments()does not. The comment is accurate in spirit but may mislead future maintainers who look at both methods side by side. A small amendment like "LikegetDocuments(), but also populates citation fields (identifierType,authors,authorsOrganization,editor,library) and resolves parent descriptions and grotto names." would make the distinction explicit.
Nitpicks (Optional)
-
[test/integration/2_utils/computeDocumentAuthorsSort.property.test.js:1]
/* eslint-disable func-names */is needed here because the test body usesfunction()(forthis.timeout()). This is correct and intentional — no action needed. It's also worth noting this is a good example of the documented exception to the arrow-function rule in the steering file. -
[api/services/DocumentService.js:540 —
getDocumentsForCitation]for...ofon documents (a plain array coming fromTDocument.find(...)) is fine, but the ESLint config restrictsfor...in. This isfor...ofso it's clean — just confirming it won't trip the linter. -
[api/dbSync/entities/document.js:198]
row.authors?.map(...)uses optional chaining, but the join always setsrow.authors(to an empty array if no matches).row.authorsOrganizationhas the same guarantee. The?.is harmless but slightly misleading since the arrays can never beundefinedat that point. Consistent with the existingrow.cave?.[0]pattern in the file though, so feel free to leave as-is.
🤔 What
Add the ability to sort bibliography (document) search results by author, both persons and organizations.
authorsSorton thedocumentsTypesense collectionsort=authorsSort:asc/:descforentity=documentsj_document_grotto_author) are now indexed and searchable — previously they weren't indexed at all🤷♂️ Why
Typesense cannot sort on array fields, and a document's authors are stored as
string[](authors.nickname) split across two relations — persons (j_document_caver_author) and organizations (j_document_grotto_author). There was no scalar field to sort on, so biblio results couldn't be ordered by author, hurting the browsing UX. This adds the denormalized sort key the index needs.🔍 How
api/utils/computeDocumentAuthorsSort.js— computes a single scalar key = the alphabetically-first author name across both persons and organizations, normalized (diacritics stripped via NFKD, lowercased, whitespace collapsed) so Typesense's byte-order sort matches human A→Z. Authorless documents get a~sentinel that sorts last on ascending order.api/dbSync/entities/document.js) — added{ name: 'authorsSort', type: 'string', optional: true, sort: true }, plus aj_document_grotto_authorjoin so org names feed the key. The key is computed per row during the full reindex.api/services/DocumentService.js) —updateInSearchrecomputesauthorsSortvia the same helper on every upsert, so incremental edits match the reindexed baseline.advanced-search.js) —authorsSortis scoped toentity=documents; using it elsewhere returns a clean400instead of a raw Typesense error. Sorting itself flows through the existing genericsortparam — no new wiring.dataQualitycomputed-sort-field pattern already in the codebase.Production actions required
Important
This PR changes the Typesense schema. The new
authorsSortfield is not populated until a full search reindex runs in each environment. Until then, sorting by author returns empty/unsorted results.This uses an alphabetical-first-author approximation, not true bibliographic ordering. Accepted trade-offs:
_text_matchtiebreaker, not their second author.nickname(may start with a first name), not a structured surname.Future upgrade path: add an ordinal column to the author join tables and key on the ranked-first name. Only the helper's name-selection changes — schema, query wiring, and the front-end contract stay identical.
🧪 Testing
test/integration/2_utils/computeDocumentAuthorsSort.test.js(alphabetical-first, diacritics, whitespace, null/blank handling, empty-authors sentinel).node scripts/resync-search.js. Verified against the local dev stack — 38 documents reindexed,authorsSortpresent in the live schema, andsort_by=authorsSort:asc/:descreturn correctly ordered results.📸 Previews
N/A — backend-only change. Front-end will attach screenshots of the enabled "sort by author" column.