diff --git a/contributions/localizedStrings.json b/contributions/localizedStrings.json index 5f2918ac..f3d281b7 100644 --- a/contributions/localizedStrings.json +++ b/contributions/localizedStrings.json @@ -232,6 +232,7 @@ "%interlinearizer_copyModal_create%": "Create copy", "%interlinearizer_copyModal_cancel%": "Cancel", "%interlinearizer_warning_pt9Sync_failed%": "The Paratext 9 data couldn't be refreshed; showing the last imported data.", + "%interlinearizer_warning_duplicateVerses%": "This book repeats one or more verse numbers. The repeated markers and their text aren't shown; fix the duplicates in the text to see them.", "%interlinearizer_modal_metadata_lastSynced_label%": "Last synced" } } diff --git a/src/__tests__/components/ContinuousView.test.tsx b/src/__tests__/components/ContinuousView.test.tsx index 678388eb..a28bb018 100644 --- a/src/__tests__/components/ContinuousView.test.tsx +++ b/src/__tests__/components/ContinuousView.test.tsx @@ -190,6 +190,7 @@ function makeBook(overrides?: Partial): Book { id: 'GEN', bookRef: 'GEN', textVersion: '1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 1:1', 'In the', [ makeWordToken('tok-0', 'In'), @@ -210,6 +211,7 @@ function makeTwoChapterBook(): Book { id: 'GEN', bookRef: 'GEN', textVersion: '1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 1:1', 'Alpha', [makeWordToken('ch1-tok-0', 'Alpha')]), makeSegment('GEN 2:1', 'Beta', [makeWordToken('ch2-tok-0', 'Beta')]), @@ -223,6 +225,7 @@ function makeSingleTokenBook(): Book { id: 'GEN', bookRef: 'GEN', textVersion: '1', + duplicateVerseIds: [], segments: [makeSegment('GEN 1:1', 'Word', [makeWordToken('tok-only', 'Word')])], }; } @@ -233,6 +236,7 @@ function makeMixedBook(): Book { id: 'GEN', bookRef: 'GEN', textVersion: '1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 1:1', 'In the', [makeWordToken('mix-tok-0', 'In')]), makeSegment('GEN 1:2', '.', [makePunctToken('mix-punct-0')]), @@ -246,6 +250,7 @@ function makeWordFreeBook(): Book { id: 'GEN', bookRef: 'GEN', textVersion: '1', + duplicateVerseIds: [], segments: [makeSegment('GEN 1:1', '...', [makePunctToken('wf-punct-0')])], }; } @@ -256,6 +261,7 @@ function makeLargeBook(count: number): Book { id: 'GEN', bookRef: 'GEN', textVersion: '1', + duplicateVerseIds: [], segments: Array.from({ length: count }, (_, i) => ({ id: `GEN 1:${i + 1}`, startRef: { book: 'GEN', chapter: 1, verse: i + 1 }, @@ -540,6 +546,7 @@ describe('ContinuousView initial render', () => { id: 'MAT', bookRef: 'MAT', textVersion: '1', + duplicateVerseIds: [], segments: [ makeSegment('MAT 1:1', 'Alpha', [makeWordToken('mat-tok-0', 'Alpha')]), makeSegment('MAT 1:2', 'Beta', [makeWordToken('mat-tok-1', 'Beta')]), @@ -566,6 +573,7 @@ describe('ContinuousView initial render', () => { id: 'MAT', bookRef: 'MAT', textVersion: '1', + duplicateVerseIds: [], segments: [ makeSegment('MAT 1:1', 'Alpha', [makeWordToken('mat-tok-0', 'Alpha')]), makeSegment('MAT 1:2', 'Beta', [makeWordToken('mat-tok-1', 'Beta')]), diff --git a/src/__tests__/components/FocusStore.test.tsx b/src/__tests__/components/FocusStore.test.tsx index f0d3203b..ebf9ecc4 100644 --- a/src/__tests__/components/FocusStore.test.tsx +++ b/src/__tests__/components/FocusStore.test.tsx @@ -29,6 +29,7 @@ function makeBook(): Book { id: 'GEN', bookRef: 'GEN', textVersion: '1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 1:1', 'In beginning', [ makeWordToken('GEN 1:1:0', 'In'), @@ -261,6 +262,7 @@ describe('FocusProvider seeding', () => { id: 'GEN', bookRef: 'GEN', textVersion: '1', + duplicateVerseIds: [], segments: [makeSegment('GEN 1:1', '', [])], }; @@ -288,6 +290,7 @@ describe('FocusProvider seeding from a child', () => { id: 'GEN', bookRef: 'GEN', textVersion: '1', + duplicateVerseIds: [], segments: [makeSegment('GEN 1:1', '', []), ...makeBook().segments.slice(1)], }; @@ -389,6 +392,7 @@ describe('FocusProvider resolution rules', () => { id: 'GEN', bookRef: 'GEN', textVersion: '2', + duplicateVerseIds: [], segments: [makeSegment('GEN 1:1', 'Anew', [makeWordToken('GEN 1:1:9', 'Anew')])], }; harness.setBook(retokenized); @@ -401,6 +405,7 @@ describe('FocusProvider resolution rules', () => { id: 'GEN', bookRef: 'GEN', textVersion: '1', + duplicateVerseIds: [], segments: [ { ...makeSegment('GEN 1:1', 'In beginning', [ @@ -429,6 +434,7 @@ describe('FocusProvider resolution rules', () => { id: 'GEN', bookRef: 'GEN', textVersion: '1', + duplicateVerseIds: [], segments: [makeSegment('GEN 1:1', '', []), ...makeBook().segments.slice(1)], }; const harness = renderFocus(noWordToken, GEN_1_1); @@ -502,6 +508,7 @@ describe('FocusProvider resolution rules', () => { id: 'MAT', bookRef: 'MAT', textVersion: '1', + duplicateVerseIds: [], segments: [makeSegment('MAT 1:1', 'Alpha', [makeWordToken('MAT 1:1:0', 'Alpha')])], }; const harness = renderFocus(makeBook(), GEN_1_1); diff --git a/src/__tests__/components/Interlinearizer.test.tsx b/src/__tests__/components/Interlinearizer.test.tsx index 178157c4..118e330a 100644 --- a/src/__tests__/components/Interlinearizer.test.tsx +++ b/src/__tests__/components/Interlinearizer.test.tsx @@ -235,7 +235,13 @@ jest.mock('../../components/modals/UnlinkPhraseConfirm', () => ({ })); /** Pre-built Book with no segments. */ -const GEN_EMPTY_BOOK: Book = { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', segments: [] }; +const GEN_EMPTY_BOOK: Book = { + id: 'GEN', + bookRef: 'GEN', + textVersion: 'v1', + segments: [], + duplicateVerseIds: [], +}; /** * Builds a GEN book with `count` single-token verses in chapter 1. Used to exercise the segment @@ -254,7 +260,7 @@ function makeLargeBook(count: number): Book { verseStarts: [{ charStart: 0, number: String(v), chapter: 1 }], }); } - return { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', segments }; + return { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', segments, duplicateVerseIds: [] }; } /** Book with two segments in GEN 1. */ @@ -262,6 +268,7 @@ const GEN_1_MULTI_BOOK: Book = { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 1:1', 'In the beginning.', [makeWordToken('GEN 1:1:0', 'In')]), makeSegment('GEN 1:2', 'And the earth.', [makeWordToken('GEN 1:2:0', 'And')]), @@ -277,6 +284,7 @@ const GEN_1_EMPTY_MIDDLE_BOOK: Book = { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 1:1', 'Alpha.', [makeWordToken('GEN 1:1:0', 'Alpha')]), makeSegment('GEN 1:2', '', []), @@ -292,6 +300,7 @@ const GEN_TWO_CHAPTER_BOOK: Book = { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [1, 2].flatMap((chapter) => [1, 2].map((verse) => makeSegment(`GEN ${chapter}:${verse}`, 'Word.', [ @@ -310,6 +319,7 @@ const GEN_SPLITTABLE_V1_BOOK: Book = { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 1:1', 'In beginning.', [ makeWordToken('GEN 1:1:0', 'In'), @@ -335,6 +345,7 @@ const GEN_SUPERSCRIPTION_BOOK: Book = { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 1:0', 'A song.', [makeWordToken('GEN 1:0:0', 'A')]), makeSegment('GEN 1:1', 'In the beginning.', [makeWordToken('GEN 1:1:0', 'In')]), @@ -1702,6 +1713,7 @@ describe('focus preservation across segmentation edits', () => { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 1:1', 'In beginning.', [ makeWordToken('GEN 1:1:0', 'In'), @@ -1899,6 +1911,7 @@ const LUK_1_1_BOOK: Book = { id: 'LUK', bookRef: 'LUK', textVersion: 'v1', + duplicateVerseIds: [], segments: [ makeSegment('LUK 1:1', 'Since many', [ makeWordToken('LUK 1:1:0', 'Since'), diff --git a/src/__tests__/components/InterlinearizerLoader.test.tsx b/src/__tests__/components/InterlinearizerLoader.test.tsx index f6406c5a..91c455a0 100644 --- a/src/__tests__/components/InterlinearizerLoader.test.tsx +++ b/src/__tests__/components/InterlinearizerLoader.test.tsx @@ -598,6 +598,7 @@ describe('InterlinearizerLoader', () => { id: 'PSA', bookRef: 'PSA', textVersion: 'v1', + duplicateVerseIds: [], segments: [makeSegment('PSA 3:0', 'A Psalm by David.', [])], }; mockBookData({ book: bookWithSuperscription }); @@ -628,6 +629,7 @@ describe('InterlinearizerLoader', () => { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 3:1', 'First verse.', []), makeSegment('GEN 3:2', 'Last verse of the chapter.', []), @@ -662,6 +664,7 @@ describe('InterlinearizerLoader', () => { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [ { id: 'GEN 4:20', @@ -705,6 +708,7 @@ describe('InterlinearizerLoader', () => { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 3:1', 'First verse.', []), makeSegment('GEN 3:3', 'Verse after the gap.', []), @@ -737,6 +741,7 @@ describe('InterlinearizerLoader', () => { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [ { id: 'GEN 3:1', @@ -2275,6 +2280,7 @@ describe('InterlinearizerLoader', () => { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 1:1', 'Alpha beta.', [ makeWordToken('GEN 1:1:0', 'Alpha'), @@ -2420,6 +2426,7 @@ describe('InterlinearizerLoader', () => { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 1:1', 'Alpha.', [makeWordToken('GEN 1:1:0', 'Alpha')]), makeSegment('GEN 1:2', '“Gamma.', [ @@ -3330,6 +3337,7 @@ const LUK_1_1_BOOK: Book = { id: 'LUK', bookRef: 'LUK', textVersion: 'v1', + duplicateVerseIds: [], segments: [ { id: 'LUK 1:1', diff --git a/src/__tests__/hooks/useInterlinearizerBookData.test.ts b/src/__tests__/hooks/useInterlinearizerBookData.test.ts index dfec6117..1b80674c 100644 --- a/src/__tests__/hooks/useInterlinearizerBookData.test.ts +++ b/src/__tests__/hooks/useInterlinearizerBookData.test.ts @@ -1,6 +1,6 @@ /// -import { logger } from '@papi/frontend'; +import papi, { logger } from '@papi/frontend'; import { useProjectData, useProjectSetting } from '@papi/frontend/react'; import { renderHook } from '@testing-library/react'; import type { Book } from 'interlinearizer'; @@ -20,6 +20,7 @@ const TEST_RAW_BOOK: RawBook = { bookCode: 'GEN', writingSystem: 'en', contentHash: 'test-hash', + duplicateVerseIds: [], verses: [ { sid: 'GEN 1:1', number: '1', text: 'In the beginning.' }, { sid: 'GEN 1:2', number: '2', text: 'And the earth.' }, @@ -32,6 +33,7 @@ const TEST_BOOK: Book = { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', + duplicateVerseIds: [], segments: [ makeSegment('GEN 1:1', 'In the beginning.', [makeWordToken('GEN 1:1:0', 'In')]), makeSegment('GEN 1:2', 'And the earth.', [makeWordToken('GEN 1:2:0', 'And')]), @@ -309,4 +311,105 @@ describe('useInterlinearizerBookData', () => { }, ); }); + + describe('duplicate verse markers', () => { + /** A tokenized book carrying the SIDs the extractor skipped as duplicates. */ + const BOOK_WITH_DUPLICATES: Book = { ...TEST_BOOK, duplicateVerseIds: ['GEN 1:1'] }; + + beforeEach(() => { + jest.mocked(logger.warn).mockImplementation(() => {}); + jest.mocked(papi.notifications.send).mockResolvedValue('notification-id'); + jest.mocked(extractBookFromUsj).mockReturnValue(TEST_RAW_BOOK); + }); + + it('still returns the book when verse markers were skipped as duplicates', () => { + jest.mocked(tokenizeBook).mockReturnValue(BOOK_WITH_DUPLICATES); + + const { result } = renderHook(() => + useInterlinearizerBookData({ projectId: 'test-project', scrRef: { ...GEN_1_1_SRC_REF } }), + ); + + expect(result.current.book).toBe(BOOK_WITH_DUPLICATES); + expect(result.current.tokenizeError).toBeUndefined(); + }); + + it('sends a warning notification naming no specific verse', () => { + jest.mocked(tokenizeBook).mockReturnValue(BOOK_WITH_DUPLICATES); + + renderHook(() => + useInterlinearizerBookData({ projectId: 'test-project', scrRef: { ...GEN_1_1_SRC_REF } }), + ); + + expect(jest.mocked(papi.notifications.send)).toHaveBeenCalledWith({ + message: '%interlinearizer_warning_duplicateVerses%', + severity: 'warning', + }); + }); + + it('logs the skipped SIDs', () => { + jest.mocked(tokenizeBook).mockReturnValue(BOOK_WITH_DUPLICATES); + + renderHook(() => + useInterlinearizerBookData({ projectId: 'test-project', scrRef: { ...GEN_1_1_SRC_REF } }), + ); + + expect(jest.mocked(logger.warn)).toHaveBeenCalledWith(expect.stringContaining('GEN 1:1')); + }); + + it('warns only once when the platform re-delivers the same book', () => { + // A fresh USJ object each render re-tokenizes into a new Book, which is what makes the + // warn-once effect re-run at all. + let usjCounter = 0; + jest.mocked(useProjectData).mockReturnValue({ + BookUSJ: () => { + usjCounter += 1; + return [{ USJ: `mock-usj-${usjCounter}` }, jest.fn(), false]; + }, + }); + jest + .mocked(tokenizeBook) + .mockImplementation(() => ({ ...BOOK_WITH_DUPLICATES, duplicateVerseIds: ['GEN 1:1'] })); + + const { rerender } = renderHook(() => + useInterlinearizerBookData({ projectId: 'test-project', scrRef: { ...GEN_1_1_SRC_REF } }), + ); + rerender(); + rerender(); + + expect(jest.mocked(papi.notifications.send)).toHaveBeenCalledTimes(1); + }); + + it('warns again when an edit adds a duplicate marker to the same book', () => { + let usjCounter = 0; + jest.mocked(useProjectData).mockReturnValue({ + BookUSJ: () => { + usjCounter += 1; + return [{ USJ: `mock-usj-${usjCounter}` }, jest.fn(), false]; + }, + }); + jest.mocked(tokenizeBook).mockReturnValue(BOOK_WITH_DUPLICATES); + + const { rerender } = renderHook(() => + useInterlinearizerBookData({ projectId: 'test-project', scrRef: { ...GEN_1_1_SRC_REF } }), + ); + + jest + .mocked(tokenizeBook) + .mockReturnValue({ ...TEST_BOOK, duplicateVerseIds: ['GEN 1:1', 'GEN 5:3'] }); + rerender(); + + expect(jest.mocked(papi.notifications.send)).toHaveBeenCalledTimes(2); + expect(jest.mocked(logger.warn)).toHaveBeenLastCalledWith(expect.stringContaining('GEN 5:3')); + }); + + it('does not warn for a book with no duplicates', () => { + jest.mocked(tokenizeBook).mockReturnValue(TEST_BOOK); + + renderHook(() => + useInterlinearizerBookData({ projectId: 'test-project', scrRef: { ...GEN_1_1_SRC_REF } }), + ); + + expect(jest.mocked(papi.notifications.send)).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/__tests__/hooks/useSegmentWindow.test.ts b/src/__tests__/hooks/useSegmentWindow.test.ts index a6e24ef5..a33af24a 100644 --- a/src/__tests__/hooks/useSegmentWindow.test.ts +++ b/src/__tests__/hooks/useSegmentWindow.test.ts @@ -44,7 +44,7 @@ function makeBook(chapter1Count: number, chapter2Count: number, book = 'GEN'): B const segments: Segment[] = []; for (let v = 1; v <= chapter1Count; v += 1) segments.push(makeSegment(1, v, book)); for (let v = 1; v <= chapter2Count; v += 1) segments.push(makeSegment(2, v, book)); - return { id: book, bookRef: book, textVersion: 'v1', segments }; + return { id: book, bookRef: book, textVersion: 'v1', segments, duplicateVerseIds: [] }; } /** @@ -276,7 +276,13 @@ describe('useSegmentWindow', () => { }, ...Array.from({ length: 8 }, (_, i) => makeSegment(1, i + 13)), ]; - const book: Book = { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', segments }; + const book: Book = { + id: 'GEN', + bookRef: 'GEN', + textVersion: 'v1', + segments, + duplicateVerseIds: [], + }; const { result } = renderSegmentWindow(book, { book: 'GEN', chapterNum: 1, verseNum: 11 }); // The merged segment sits at flat index 10, so the centered window runs [2, 19). @@ -675,7 +681,13 @@ describe('useSegmentWindow', () => { }, ...Array.from({ length: 24 }, (_, i) => makeSegment(1, i + 7)), ]; - const editedBook: Book = { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', segments: mergedTail }; + const editedBook: Book = { + id: 'GEN', + bookRef: 'GEN', + textVersion: 'v1', + segments: mergedTail, + duplicateVerseIds: [], + }; act(() => rerender({ b: editedBook, ref: scrRef, segVersion: 1 })); @@ -701,7 +713,13 @@ describe('useSegmentWindow', () => { { ...makeSegment(1, 5), id: 'GEN 1:5b' }, ...Array.from({ length: 25 }, (_, i) => makeSegment(1, i + 6)), ]; - const editedBook: Book = { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', segments: splitTail }; + const editedBook: Book = { + id: 'GEN', + bookRef: 'GEN', + textVersion: 'v1', + segments: splitTail, + duplicateVerseIds: [], + }; act(() => rerender({ b: editedBook, ref: scrRef, segVersion: 1 })); @@ -728,7 +746,13 @@ describe('useSegmentWindow', () => { }, makeSegment(1, 30), ]; - const editedBook: Book = { id: 'GEN', bookRef: 'GEN', textVersion: 'v1', segments: mergedTail }; + const editedBook: Book = { + id: 'GEN', + bookRef: 'GEN', + textVersion: 'v1', + segments: mergedTail, + duplicateVerseIds: [], + }; act(() => rerender({ b: editedBook, ref: scrRef, segVersion: 1 })); diff --git a/src/__tests__/parsers/papi/bookTokenizer.test.ts b/src/__tests__/parsers/papi/bookTokenizer.test.ts index 268bb993..ffd75600 100644 --- a/src/__tests__/parsers/papi/bookTokenizer.test.ts +++ b/src/__tests__/parsers/papi/bookTokenizer.test.ts @@ -17,6 +17,18 @@ describe('tokenizeBook', () => { expect(tokenizeBook(makeRawBook([])).segments).toEqual([]); }); + it('forwards duplicate verse SIDs onto the Book', () => { + const raw = { ...makeRawBook([{ sid: 'GEN 1:1', text: 'First.' }]) }; + raw.duplicateVerseIds = ['GEN 1:1']; + expect(tokenizeBook(raw).duplicateVerseIds).toEqual(['GEN 1:1']); + }); + + it('reports no duplicate verse SIDs when the raw book has none', () => { + expect( + tokenizeBook(makeRawBook([{ sid: 'GEN 1:1', text: 'First.' }])).duplicateVerseIds, + ).toEqual([]); + }); + it('produces one segment per verse in order', () => { const raw = makeRawBook([ { sid: 'GEN 1:1', text: 'First.' }, @@ -52,6 +64,7 @@ describe('tokenizeBook', () => { bookCode: 'PSA', writingSystem: 'en', contentHash: 'abc123', + duplicateVerseIds: [], verses: [{ sid: 'PSA 3:0', number: '0', text: 'A Psalm by David.' }], }; const { segments } = tokenizeBook(raw); diff --git a/src/__tests__/parsers/papi/usjBookExtractor.test.ts b/src/__tests__/parsers/papi/usjBookExtractor.test.ts index 9b8a64f9..79385c7a 100644 --- a/src/__tests__/parsers/papi/usjBookExtractor.test.ts +++ b/src/__tests__/parsers/papi/usjBookExtractor.test.ts @@ -391,7 +391,52 @@ describe('extractBookFromUsj', () => { ); }); - it('throws on a duplicate verse SID', () => { + /** A book whose second `GEN 1:1` marker repeats the first, with a well-formed verse after it. */ + const duplicateVerseUsj: UsjDocument = { + content: [ + { type: 'book', code: 'GEN', content: [] }, + { + type: 'para', + marker: 'p', + content: [ + { type: 'verse', sid: 'GEN 1:1' }, + 'First occurrence.', + { type: 'verse', sid: 'GEN 1:1' }, + 'Duplicate.', + { type: 'verse', sid: 'GEN 1:2' }, + 'And the earth.', + ], + }, + ], + }; + + it('keeps the first occurrence of a duplicated verse SID', () => { + const { verses } = extractBookFromUsj(duplicateVerseUsj, WS); + expect(verses).toEqual([ + { sid: 'GEN 1:1', number: '1', text: 'First occurrence.' }, + { sid: 'GEN 1:2', number: '2', text: 'And the earth.' }, + ]); + }); + + it('reports the SID of a skipped duplicate verse marker', () => { + expect(extractBookFromUsj(duplicateVerseUsj, WS).duplicateVerseIds).toEqual(['GEN 1:1']); + }); + + it('reports no duplicates for a well-formed book', () => { + const usj: UsjDocument = { + content: [ + { type: 'book', code: 'GEN', content: [] }, + { + type: 'para', + marker: 'p', + content: [{ type: 'verse', sid: 'GEN 1:1' }, 'In the beginning.'], + }, + ], + }; + expect(extractBookFromUsj(usj, WS).duplicateVerseIds).toEqual([]); + }); + + it('reports one entry per skipped marker when a SID repeats more than twice', () => { const usj: UsjDocument = { content: [ { type: 'book', code: 'GEN', content: [] }, @@ -400,14 +445,16 @@ describe('extractBookFromUsj', () => { marker: 'p', content: [ { type: 'verse', sid: 'GEN 1:1' }, - 'First occurrence.', + 'First.', { type: 'verse', sid: 'GEN 1:1' }, - 'Duplicate.', + 'Second.', + { type: 'verse', sid: 'GEN 1:1' }, + 'Third.', ], }, ], }; - expect(() => extractBookFromUsj(usj, WS)).toThrow('duplicate verse SID "GEN 1:1"'); + expect(extractBookFromUsj(usj, WS).duplicateVerseIds).toEqual(['GEN 1:1', 'GEN 1:1']); }); it('captures a d descriptive title before verse 1 as verse 0', () => { @@ -520,18 +567,44 @@ describe('extractBookFromUsj', () => { }); }); - it('throws when an explicit verse-0 marker duplicates a non-empty synthetic verse 0', () => { + it('skips an explicit verse-0 marker duplicating a non-empty synthetic verse 0', () => { const usj: UsjDocument = { content: [ { type: 'book', code: 'PSA', content: [] }, { type: 'chapter', number: '3', sid: 'PSA 3' }, // A `d` descriptive title opens a non-empty synthetic verse 0 (PSA 3:0)... { type: 'para', marker: 'd', content: ['A Psalm by David.'] }, - // ...then an explicit verse-0 marker with the same SID must be rejected as a duplicate - // rather than silently emitting two PSA 3:0 verses. + // ...then an explicit verse-0 marker with the same SID is skipped rather than emitting a + // second PSA 3:0 verse. { type: 'para', marker: 'q1', content: [{ type: 'verse', sid: 'PSA 3:0' }, 'Yahweh.'] }, ], }; - expect(() => extractBookFromUsj(usj, WS)).toThrow('duplicate verse SID "PSA 3:0"'); + const { verses, duplicateVerseIds } = extractBookFromUsj(usj, WS); + expect(verses).toEqual([{ sid: 'PSA 3:0', number: '0', text: 'A Psalm by David.' }]); + expect(duplicateVerseIds).toEqual(['PSA 3:0']); + }); + + it('does not fold a skipped duplicate marker’s text into the preceding verse', () => { + const usj: UsjDocument = { + content: [ + { type: 'book', code: 'GEN', content: [] }, + { + type: 'para', + marker: 'p', + content: [ + { type: 'verse', sid: 'GEN 1:1' }, + 'In the beginning.', + { type: 'verse', sid: 'GEN 1:2' }, + 'And the earth.', + // Repeats GEN 1:1; its text must be dropped, not appended to GEN 1:2, whose token + // offsets are already expressed against the text above. + { type: 'verse', sid: 'GEN 1:1' }, + 'Stray duplicate text.', + ], + }, + ], + }; + const { verses } = extractBookFromUsj(usj, WS); + expect(verses[1]).toEqual({ sid: 'GEN 1:2', number: '2', text: 'And the earth.' }); }); }); diff --git a/src/__tests__/services/pt9ImportService.test.ts b/src/__tests__/services/pt9ImportService.test.ts index aa46561c..e55e088a 100644 --- a/src/__tests__/services/pt9ImportService.test.ts +++ b/src/__tests__/services/pt9ImportService.test.ts @@ -217,6 +217,36 @@ describe('importPt9Project', () => { expect(__mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('no USJ for book MAT')); }); + it('imports a book that repeats a verse marker, logging the skipped SID', async () => { + const duplicated = { + content: [ + { type: 'book', code: 'MAT', content: [] }, + { type: 'chapter', number: '1', sid: 'MAT 1' }, + { + type: 'para', + marker: 'p', + content: [ + { type: 'verse', sid: 'MAT 1:1', number: '1' }, + 'hello aokaybe abe abc this is a footnote with a note تمان oj', + { type: 'verse', sid: 'MAT 1:2', number: '2' }, + 'oooo dearly', + { type: 'verse', sid: 'MAT 1:9', number: '9' }, + 'hello', + // Repeats the book's first verse marker. + { type: 'verse', sid: 'MAT 1:1', number: '1' }, + 'a repeated marker', + ], + }, + ], + }; + mockPdps({ usj: duplicated }); + + const result = await importPt9Project(token, 'src-project'); + + expect(result.outcome).toBe('imported'); + expect(__mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('MAT 1:1')); + }); + it('aborts without writing when the source has no interlinear data and no import exists', async () => { mockPdps({ manifest: {} }); diff --git a/src/__tests__/test-helpers.ts b/src/__tests__/test-helpers.ts index 5dfc7c4d..ef8ad10d 100644 --- a/src/__tests__/test-helpers.ts +++ b/src/__tests__/test-helpers.ts @@ -169,6 +169,7 @@ export const GEN_1_1_BOOK: Book = { bookRef: 'GEN', textVersion: 'v1', segments: [makeSegment('GEN 1:1', 'In the beginning.', [makeWordToken('GEN 1:1:0', 'In')])], + duplicateVerseIds: [], }; /** Minimal elevated privileges for tests (all properties optional per papi type). */ @@ -251,6 +252,7 @@ export function makeRawBook(verses: VerseSpec[]): RawBook { bookCode: verses[0]?.sid.split(' ')[0] ?? 'GEN', writingSystem: 'en', contentHash: 'abc123', + duplicateVerseIds: [], verses: verses.map(({ sid, text, number }) => ({ sid, text, diff --git a/src/hooks/useInterlinearizerBookData.ts b/src/hooks/useInterlinearizerBookData.ts index 19212efa..78e61bc1 100644 --- a/src/hooks/useInterlinearizerBookData.ts +++ b/src/hooks/useInterlinearizerBookData.ts @@ -1,4 +1,4 @@ -import { logger } from '@papi/frontend'; +import papi, { logger } from '@papi/frontend'; import { useProjectData, useProjectSetting } from '@papi/frontend/react'; import { SerializedVerseRef } from '@sillsdev/scripture'; import type { Book } from 'interlinearizer'; @@ -102,6 +102,25 @@ export default function useInterlinearizerBookData({ }); }, [tokenizeError, writingSystemTag, projectId, scrRef.book]); + // Warns once per project/book/duplicate-set. + const warnedForBookRef = useRef(undefined); + const duplicateVerseIds = book?.duplicateVerseIds; + useEffect(() => { + if (!duplicateVerseIds || duplicateVerseIds.length === 0) return; + + const bookKey = `${projectId}:${scrRef.book}:${duplicateVerseIds.join(',')}`; + if (warnedForBookRef.current === bookKey) return; + warnedForBookRef.current = bookKey; + + logger.warn( + `Interlinearizer: book ${scrRef.book} in project ${projectId} repeats ${duplicateVerseIds.length} verse marker(s); their text is not shown: ${duplicateVerseIds.join(', ')}`, + ); + papi.notifications + .send({ message: '%interlinearizer_warning_duplicateVerses%', severity: 'warning' }) + /* v8 ignore next -- a failed warning must not break the load that succeeded */ + .catch(() => {}); + }, [duplicateVerseIds, projectId, scrRef.book]); + let bookError: string | undefined; if (isPlatformError(bookResult)) { bookError = bookResult.message; diff --git a/src/parsers/papi/bookTokenizer.ts b/src/parsers/papi/bookTokenizer.ts index fc4e08ff..eae30dec 100644 --- a/src/parsers/papi/bookTokenizer.ts +++ b/src/parsers/papi/bookTokenizer.ts @@ -132,5 +132,6 @@ export function tokenizeBook(rawBook: RawBook): Book { bookRef: rawBook.bookCode, textVersion: rawBook.contentHash, segments, + duplicateVerseIds: rawBook.duplicateVerseIds, }; } diff --git a/src/parsers/papi/usjBookExtractor.ts b/src/parsers/papi/usjBookExtractor.ts index 78dcff4a..235431d1 100644 --- a/src/parsers/papi/usjBookExtractor.ts +++ b/src/parsers/papi/usjBookExtractor.ts @@ -28,6 +28,11 @@ export interface RawBook { contentHash: string; /** Verse entries in document order, one per USJ `verse` marker. */ verses: RawVerse[]; + /** + * SIDs of verse markers dropped because an earlier marker already claimed that SID, in encounter + * order and repeated once per dropped marker. Empty for a well-formed book. + */ + duplicateVerseIds: string[]; } // --------------------------------------------------------------------------- @@ -109,8 +114,10 @@ const HEADING_PARA_MARKERS = new Set([ interface TraversalState { /** 3-letter book code captured from the `book` marker (e.g. `"GEN"`). */ bookCode: string; - /** Verse SIDs seen so far; used to reject duplicates. */ + /** Verse SIDs seen so far; used to detect duplicates. */ seenVerseIds: Set; + /** SIDs of verse markers skipped as duplicates, in encounter order. */ + duplicateVerseIds: string[]; /** The verse currently being accumulated; `undefined` when outside a verse scope. */ currentVerse: { sid: string; number: string; text: string } | undefined; /** @@ -186,14 +193,18 @@ function verseNumberFromSid(sid: string): string { * verse number is the marker's verbatim `number` attribute, or the sid-derived verse portion when * the marker omits it. * + * A marker whose SID an earlier marker already claimed opens no verse scope, so its text is + * discarded rather than folded into the preceding verse. + * * @throws {SyntaxError} If the `verse` node is missing its required `sid` attribute. - * @throws {SyntaxError} If the `verse` SID has already been seen (duplicate verse SID). */ function handleVerseNode(node: UsjNode, state: TraversalState): void { closeCurrentVerse(state); if (!node.sid) throw new SyntaxError('Invalid USJ: verse marker missing required sid attribute'); - if (state.seenVerseIds.has(node.sid)) - throw new SyntaxError(`Invalid USJ: duplicate verse SID "${node.sid}"`); + if (state.seenVerseIds.has(node.sid)) { + state.duplicateVerseIds.push(node.sid); + return; + } state.seenVerseIds.add(node.sid); state.currentVerse = { sid: node.sid, @@ -235,7 +246,7 @@ const NODE_HANDLERS: Partial { @@ -295,15 +306,18 @@ function fnv1a32(s: string): string { * superscription) — is captured as a synthetic verse-0 `RawVerse` with SID `" :0"`, * but only when it has text. * + * A `verse` marker repeating a SID an earlier marker already claimed is skipped rather than fatal, + * so a book with duplicate verses still extracts. + * * @throws {SyntaxError} If no `book` marker with a `code` attribute is found in the document. * @throws {SyntaxError} If a `verse` marker is missing its required `sid` attribute. - * @throws {SyntaxError} If a duplicate `verse` SID is encountered. */ export function extractBookFromUsj(usj: UsjDocument, writingSystem: string): RawBook { const contentHash = fnv1a32(stableStringify(usj.content)); const state: TraversalState = { bookCode: '', seenVerseIds: new Set(), + duplicateVerseIds: [], currentVerse: undefined, currentVerseIsSynthetic: false, verses: [], @@ -321,5 +335,6 @@ export function extractBookFromUsj(usj: UsjDocument, writingSystem: string): Raw writingSystem, contentHash, verses: state.verses, + duplicateVerseIds: state.duplicateVerseIds, }; } diff --git a/src/services/pt9ImportService.ts b/src/services/pt9ImportService.ts index 1141aecb..d07fed52 100644 --- a/src/services/pt9ImportService.ts +++ b/src/services/pt9ImportService.ts @@ -60,7 +60,7 @@ async function getWritingSystem(sourceProjectId: string): Promise { * localized values, resolved at import time. * * A book the source project has no USJ for is skipped and counted in the report rather than failing - * the import. + * the import. A book that repeats a verse marker imports without that marker's text. * * @throws {Error} If the source project has no Paratext 9 interlinear data and no earlier import * exists - nothing is created for an empty source. @@ -106,7 +106,13 @@ export async function importPt9Project( ); return []; } - return [tokenizeBook(extractBookFromUsj(usj, writingSystem))]; + const book = tokenizeBook(extractBookFromUsj(usj, writingSystem)); + if (book.duplicateVerseIds.length > 0) { + logger.warn( + `Interlinearizer: book ${bookId} in project ${sourceProjectId} repeats ${book.duplicateVerseIds.length} verse marker(s); their text is excluded from the import: ${book.duplicateVerseIds.join(', ')}`, + ); + } + return [book]; }), ) ).flat(); diff --git a/src/types/interlinearizer.d.ts b/src/types/interlinearizer.d.ts index b0039aa6..61c96e49 100644 --- a/src/types/interlinearizer.d.ts +++ b/src/types/interlinearizer.d.ts @@ -546,6 +546,13 @@ declare module 'interlinearizer' { /** Ordered segments that compose this book. */ segments: Segment[]; + + /** + * SIDs of verse markers the source repeated, which are skipped rather than segmented because + * the SID is the segment identity analyses join on. Empty for a well-formed book; when + * non-empty, the book is missing those markers' text. + */ + duplicateVerseIds: string[]; } /**