Fix VerseRefs not working as expected with check ranges

[lyonsil]

I thought updating deserialize would have been enough, but sometimes the check aggregator was also seeing VerseRef values where instanceof VerseRef was false. In those cases it seemed that getting BBBCCC was not working as expected some calls to VerseRef constructors were not running as expected, so I needed to update setActiveRanges, too.

---

This change is 

[irahopkinson]

Thanks for this, it with one block and some questions.

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lyonsil)

---

lib/platform-bible-utils/src/serialization.test.ts line 36 at r1 (raw file):

  it('should use the reviver function if provided', () => {
    const json = '{"a":1,"b":2}';
    const reviver = (key: string, value: unknown) => (key === 'b' ? undefined : value);

In the equivalent test for the serializer, I like that it checks that the replacer function runs before the null/undefined replacer. The equivalent here would be use null for key 'b' in this reviver.

---

lib/platform-bible-utils/src/serialization.test.ts line 62 at r1 (raw file):

    const json = '{"a":1,"b":{"book":"GEN","chapterNum":1,"verseNum":1}}';
    const result = deserialize(json);
    expect(result).toEqual({ a: 1, b: new VerseRef('GEN', '1', '1') });

BTW here I'm guessing expect(result.b instanceof VerseRef).toBe(false), right, even though that is not what we want? If not then I'm wondering what the issue is in check-aggregator.service.ts?

---

extensions/src/platform-scripture/src/checks/check-aggregator.service.ts line 143 at r1 (raw file):

          Canon.bookIdToNumber(start.book),
          start.chapterNum,
          start.verseNum,

BTW do we care that this won't work if range.start is a range, sequence, or segment, i.e. range.start.verse !== range.start.verseNum.toString()? I'm guessing ranges will only ever contain a single reference so we don't care.

[lyonsil]

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @irahopkinson)

---

extensions/src/platform-scripture/src/checks/check-aggregator.service.ts line 143 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW do we care that this won't work if range.start is a range, sequence, or segment, i.e. range.start.verse !== range.start.verseNum.toString()? I'm guessing ranges will only ever contain a single reference so we don't care.

In this case I don't expect us to care. The UI generating the ranges I think is always specifying 0 or 1. I included it here because:

1. SerializedVerseRef says verseNum is a mandatory field, so I'm assuming it will always be there.
2. The constructor requires something. (I guess I could have just passed a constant value, but due to 1 I didn't really think about it.)
3. I know the range-related code in the check aggregator is only looking at the book/chapter level.

I think all this code will end up being ripped out as part of #1355, anyway, so I'm not worried about it.

---

lib/platform-bible-utils/src/serialization.test.ts line 36 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

In the equivalent test for the serializer, I like that it checks that the replacer function runs before the null/undefined replacer. The equivalent here would be use null for key 'b' in this reviver.

Makes sense - I updated the test.

---

lib/platform-bible-utils/src/serialization.test.ts line 62 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW here I'm guessing expect(result.b instanceof VerseRef).toBe(false), right, even though that is not what we want? If not then I'm wondering what the issue is in check-aggregator.service.ts?

TJ and I looked at this in more depth this morning. The issue is coming up because the VerseRef constructor uses instanceof in some cases. We're really going to want to do #1355, I think, in addition to updating our guidance to point people to only pass along non-class, serializable objects through PAPI calls.

[irahopkinson]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved