Skip to content

C
coopwire-hypothesis
Commit cca16c88 authored by Robert Knight's avatar Robert Knight
Browse files
Options

Remove book_as_single_document flag and throw if book ID is missing 

 - Remove the book_as_single_document flag, as we have now enabled this
   flag for everyone in production

 - Throw if the `isbn` property is missing from the book info returned
   by VitalSource. If this ever happens, we want to fail loudly instead
   of silently capturing a URL with no book ID on it.
parent 99b816c5
Show whitespace changes
Inline Side-by-side
Showing with 37 additions and 131 deletions
src/annotator/features.js
View file @ cca16c88
......@@ -11,11 +11,7 @@ import { warnOnce } from '../shared/warn-once';
*
* @type {string[]}
*/
const annotatorFlags = [
'book_as_single_document',
'html_side_by_side',
'styled_highlight_clusters',
];
const annotatorFlags = ['html_side_by_side', 'styled_highlight_clusters'];
/**
* An observable container of feature flags.
......
src/annotator/integrations/index.js
View file @ cca16c88
......@@ -25,9 +25,7 @@ export function createIntegration(annotator) {
const vsFrameRole = vitalSourceFrameRole();
if (vsFrameRole === 'content') {
return new VitalSourceContentIntegration(document.body, {
features: annotator.features,
});
return new VitalSourceContentIntegration(document.body);
}
return new HTMLIntegration({ features: annotator.features });
......
src/annotator/integrations/test/vitalsource-test.js
View file @ cca16c88
import { delay, waitFor } from '../../../test-util/wait';
import { FeatureFlags } from '../../features';
import {
VitalSourceInjector,
VitalSourceContentIntegration,
......@@ -59,14 +58,12 @@ function resolveURL(relativeURL) {
}
describe('annotator/integrations/vitalsource', () => {
let featureFlags;
let fakeViewer;
let FakeHTMLIntegration;
let fakeHTMLIntegration;
let fakeInjectClient;
beforeEach(() => {
featureFlags = new FeatureFlags();
fakeViewer = new FakeVitalSourceViewer();
fakeHTMLIntegration = {
......@@ -281,7 +278,6 @@ describe('annotator/integrations/vitalsource', () => {
function createIntegration() {
const integration = new VitalSourceContentIntegration(document.body, {
features: featureFlags,
bookElement: fakeBookElement,
});
integrations.push(integration);
......@@ -305,11 +301,6 @@ describe('annotator/integrations/vitalsource', () => {
assert.calledWith(fakeHTMLIntegration.getAnnotatableRange, range);
});
it('asks guest to wait for feature flags before sending document info', () => {
const integration = createIntegration();
assert.isTrue(integration.waitForFeatureFlags());
});
it('asks sidebar to persist annotations after frame unloads', () => {
const integration = createIntegration();
assert.isTrue(integration.persistFrame());
......@@ -415,20 +406,6 @@ describe('annotator/integrations/vitalsource', () => {
});
describe('#getMetadata', () => {
context('when "book_as_single_document" flag is off', () => {
it('returns metadata for current page/chapter', async () => {
const integration = createIntegration();
const metadata = await integration.getMetadata();
assert.equal(metadata.title, document.title);
assert.deepEqual(metadata.link, []);
});
});
context('when "book_as_single_document" flag is on', () => {
beforeEach(() => {
featureFlags.update({ book_as_single_document: true });
});
it('returns book metadata', async () => {
const integration = createIntegration();
const metadata = await integration.getMetadata();
......@@ -436,7 +413,6 @@ describe('annotator/integrations/vitalsource', () => {
assert.deepEqual(metadata.link, []);
});
});
});
describe('#navigateToSegment', () => {
function createAnnotationWithSelector(selector) {
......@@ -526,23 +502,7 @@ describe('annotator/integrations/vitalsource', () => {
await urlChanged;
});
context('when "book_as_single_document" flag is off', () => {
it('returns book chapter URL excluding query string', async () => {
const integration = createIntegration();
const uri = await integration.uri();
const parsedURL = new URL(uri);
assert.equal(parsedURL.hostname, document.location.hostname);
assert.equal(
parsedURL.pathname,
'/books/abc/epub/OPS/xhtml/chapter_001.html'
);
assert.equal(parsedURL.search, '');
});
});
context('when "book_as_single_document" flag is on', () => {
it('returns book reader URL', async () => {
featureFlags.update({ book_as_single_document: true });
const integration = createIntegration();
const uri = await integration.uri();
assert.equal(
......@@ -550,19 +510,24 @@ describe('annotator/integrations/vitalsource', () => {
'https://bookshelf.vitalsource.com/reader/books/TEST-BOOK-ID'
);
});
it('throws if book ID is missing', async () => {
fakeBookElement.getBookInfo = () => ({
format: 'epub',
title: 'Test book title',
// `isbn` field is missing
});
it('updates when "book_as_single_document" flag changes', async () => {
const uriChanged = sinon.stub();
const integration = createIntegration();
integration.on('uriChanged', uriChanged);
const uri1 = await integration.uri();
featureFlags.update({ book_as_single_document: true });
let error;
try {
await integration.uri();
} catch (err) {
error = err;
}
assert.calledOnce(uriChanged);
const uri2 = await integration.uri();
assert.notEqual(uri1, uri2);
assert.instanceOf(error, Error);
assert.equal(error.message, 'Unable to get book ID from VitalSource');
});
});
......
src/annotator/integrations/vitalsource.ts
View file @ cca16c88
......@@ -12,7 +12,6 @@ import { injectClient } from '../hypothesis-injector';
import type {
Anchor,
AnnotationData,
FeatureFlags as IFeatureFlags,
Integration,
SegmentInfo,
SidebarLayout,
......@@ -204,7 +203,6 @@ export class VitalSourceContentIntegration
implements Integration
{
private _bookElement: MosaicBookElement;
private _features: IFeatureFlags;
private _htmlIntegration: HTMLIntegration;
private _listeners: ListenerCollection;
private _textLayer?: ImageTextLayer;
......@@ -212,16 +210,14 @@ export class VitalSourceContentIntegration
constructor(
/* istanbul ignore next - defaults are overridden in tests */
container: HTMLElement = document.body,
/* istanbul ignore next - defaults are overridden in tests */
options: {
features: IFeatureFlags;
// Test seam
bookElement?: MosaicBookElement;
}
} = {}
) {
super();
this._features = options.features;
const bookElement =
options.bookElement ?? findBookElement(window.parent.document);
if (!bookElement) {
......@@ -232,12 +228,6 @@ export class VitalSourceContentIntegration
}
this._bookElement = bookElement;
// If the book_as_single_document flag changed, this will change the
// document URI returned by this integration.
this._features.on('flagsChanged', () => {
this.emit('uriChanged');
});
const htmlFeatures = new FeatureFlags();
// Forcibly enable the side-by-side feature for VS books. This feature is
......@@ -401,7 +391,6 @@ export class VitalSourceContentIntegration
}
async getMetadata() {
if (this._bookIsSingleDocument()) {
const bookInfo = this._bookElement.getBookInfo();
return {
title: bookInfo.title,
......@@ -409,14 +398,6 @@ export class VitalSourceContentIntegration
};
}
// Return minimal metadata which includes only the information we really
// want to include.
return {
title: document.title,
link: [],
};
}
navigateToSegment(ann: AnnotationData) {
const selector = ann.target[0].selector?.find(
s => s.type === 'EPUBContentSelector'
......@@ -484,49 +465,15 @@ export class VitalSourceContentIntegration
}
async uri() {
if (this._bookIsSingleDocument()) {
const bookInfo = this._bookElement.getBookInfo();
const bookId = bookInfo.isbn;
return `https://bookshelf.vitalsource.com/reader/books/${bookId}`;
if (!bookId) {
throw new Error('Unable to get book ID from VitalSource');
}
// An example of a typical URL for the chapter content in the Bookshelf reader is:
//
// https://jigsaw.vitalsource.com/books/9781848317703/epub/OPS/xhtml/chapter_001.html#cfi=/6/10%5B;vnd.vst.idref=chap001%5D!/4
//
// Where "9781848317703" is the VitalSource book ID ("vbid"), "chapter_001.html"
// is the location of the HTML page for the current chapter within the book
// and the `#cfi` fragment identifies the scroll location.
//
// Note that this URL is typically different than what is displayed in the
// iframe's `src` attribute.
// Strip off search parameters and fragments.
const uri = new URL(document.location.href);
uri.search = '';
return uri.toString();
return `https://bookshelf.vitalsource.com/reader/books/${bookId}`;
}
async scrollToAnchor(anchor: Anchor) {
return this._htmlIntegration.scrollToAnchor(anchor);
}
/**
* Return true if the feature flag to treat books as one document is enabled,
* as opposed to treating each chapter/segment/page as a separate document.
*/
_bookIsSingleDocument(): boolean {
return this._features.flagEnabled('book_as_single_document');
}
waitForFeatureFlags() {
// The `book_as_single_document` flag changes the URI reported by this
// integration.
//
// Ask the guest to delay reporting document metadata to the sidebar until
// feature flags have been received. This ensures that the initial document
// info reported to the sidebar after a chapter navigation is consistent
// between the previous/new guest frames.
return true;
}
}
src/annotator/test/guest-test.js
View file @ cca16c88
......@@ -1451,7 +1451,7 @@ describe('Guest', () => {
assert.isFalse(sidebarRPC().call.calledWith('documentInfoChanged'));
emitSidebarEvent('featureFlagsUpdated', {
book_as_single_document: true,
some_new_feature: true,
});
await delay(0);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment