Commit 05da7ae5 authored by Robert Knight's avatar Robert Knight

Extract export file parsing out of ImportAnnotations component

Extract the reading and parsing of Hypothesis JSON files out of the
ImportAnnotations component and into a separate module. This allows for it to be
more directly tested, and also may help to fix test flakiness in
ImportAnnotations by replacing the file "reading" with a fake that takes a more
deterministic amount of time (one tick).

May fix https://github.com/hypothesis/client/issues/5746.
parent 37f7683b
import { Button, CardActions, Select } from '@hypothesis/frontend-shared';
import { useCallback, useEffect, useId, useMemo, useState } from 'preact/hooks';
import { isObject } from '../../../shared/is-object';
import { readJSONFile } from '../../../shared/read-json-file';
import type { APIAnnotationData } from '../../../types/api';
import { annotationDisplayName } from '../../helpers/annotation-user';
import { readExportFile } from '../../helpers/import';
import { withServices } from '../../service-context';
import type { ExportContent } from '../../services/annotations-exporter';
import type { ImportAnnotationsService } from '../../services/import-annotations';
import { useSidebarStore } from '../../store';
import FileInput from './FileInput';
import LoadingSpinner from './LoadingSpinner';
/**
* Parse a file generated by the annotation exporter and return the extracted
* annotations.
*/
async function parseExportContent(file: File): Promise<APIAnnotationData[]> {
let json;
try {
json = await readJSONFile(file);
} catch (err) {
throw new Error('Not a valid JSON file');
}
// Perform some very rudimentary validation of the file content, just enough
// to catch issues where the user picked the wrong kind of JSON file.
if (!isObject(json) || !Array.isArray((json as any).annotations)) {
throw new Error('Not a valid Hypothesis JSON file');
}
return (json as ExportContent).annotations;
}
type UserAnnotationCount = {
userid: string;
displayName: string;
......@@ -112,7 +89,7 @@ function ImportAnnotations({
setAnnotations(null);
setError(null);
setSelectedUser(null);
parseExportContent(file)
readExportFile(file)
.then(annotations => {
setAnnotations(annotations);
......
......@@ -6,9 +6,12 @@ import ImportAnnotations, { $imports } from '../ImportAnnotations';
describe('ImportAnnotations', () => {
let fakeImportAnnotationsService;
let fakeReadExportFile;
let fakeStore;
beforeEach(() => {
fakeReadExportFile = sinon.stub().rejects(new Error('Failed to read file'));
fakeImportAnnotationsService = {
import: sinon.stub().resolves([]),
};
......@@ -21,6 +24,7 @@ describe('ImportAnnotations', () => {
};
$imports.$mock({
'../../helpers/import': { readExportFile: fakeReadExportFile },
'../../store': { useSidebarStore: () => fakeStore },
});
});
......@@ -49,16 +53,18 @@ describe('ImportAnnotations', () => {
return wrapper.find('button[data-testid="import-button"]');
}
function selectFile(wrapper, data) {
function selectFile(wrapper, readResult) {
const fileInput = wrapper.find('input[type="file"]');
const fileContent = typeof data === 'string' ? data : JSON.stringify(data);
const file = new File([fileContent], 'export.json');
const file = new File(['dummy content'], 'export.json');
const transfer = new DataTransfer();
transfer.items.add(file);
fileInput.getDOMNode().files = transfer.files;
// `HTMLInputElement.files` can be assigned, but is a `FileList`, which
// can't be constructed, so we just stub the property instead.
Object.defineProperty(fileInput.getDOMNode(), 'files', {
get: () => [file],
});
if (readResult instanceof Error) {
fakeReadExportFile.withArgs(file).rejects(readResult);
} else {
fakeReadExportFile.withArgs(file).resolves(readResult);
}
fileInput.simulate('change');
}
......@@ -71,24 +77,22 @@ describe('ImportAnnotations', () => {
it('displays user list when a valid file is selected', async () => {
const wrapper = createImportAnnotations();
selectFile(wrapper, {
annotations: [
{
user: 'acct:john@example.com',
user_info: {
display_name: 'John Smith',
},
text: 'Test annotation',
selectFile(wrapper, [
{
user: 'acct:john@example.com',
user_info: {
display_name: 'John Smith',
},
{
user: 'acct:brian@example.com',
user_info: {
display_name: 'Brian Smith',
},
text: 'Test annotation',
text: 'Test annotation',
},
{
user: 'acct:brian@example.com',
user_info: {
display_name: 'Brian Smith',
},
],
});
text: 'Test annotation',
},
]);
const userList = await waitForElement(wrapper, 'Select');
const users = userList.find('option');
......@@ -101,29 +105,17 @@ describe('ImportAnnotations', () => {
assert.equal(users.at(2).text(), 'John Smith (1)');
});
// TODO - Check handling of errors when reading file fails
[
{
content: 'foobar',
reason: 'Not a valid JSON file',
},
{
content: {},
reason: 'Not a valid Hypothesis JSON file',
},
].forEach(({ content, reason }) => {
it('displays error if file is invalid', async () => {
const wrapper = createImportAnnotations();
selectFile(wrapper, content);
const error = await waitForElement(wrapper, '[data-testid="error-info"]');
assert.equal(
error.text(),
`Unable to find annotations to import: ${reason}`,
);
});
it('displays error if file is invalid', async () => {
const wrapper = createImportAnnotations();
const reason = 'Not a valid Hypothesis JSON file';
selectFile(wrapper, new Error(reason));
const error = await waitForElement(wrapper, '[data-testid="error-info"]');
assert.equal(
error.text(),
`Unable to find annotations to import: ${reason}`,
);
});
it('selects user matching logged in user if found', async () => {
......@@ -137,7 +129,7 @@ describe('ImportAnnotations', () => {
text: 'Test annotation',
},
];
selectFile(wrapper, { annotations });
selectFile(wrapper, annotations);
const userList = await waitForElement(wrapper, 'Select');
assert.equal(userList.getDOMNode().value, 'acct:john@example.com');
......@@ -155,7 +147,7 @@ describe('ImportAnnotations', () => {
text: 'Test annotation',
},
];
selectFile(wrapper, { annotations });
selectFile(wrapper, annotations);
const userList = await waitForElement(wrapper, 'Select');
assert.equal(userList.getDOMNode().value, '');
......@@ -180,7 +172,7 @@ describe('ImportAnnotations', () => {
},
];
selectFile(wrapper, { annotations });
selectFile(wrapper, annotations);
const userList = await waitForElement(wrapper, 'select');
userList.getDOMNode().value = 'acct:brian@example.com';
......
import { isObject } from '../../shared/is-object';
import { readJSONFile } from '../../shared/read-json-file';
import type { APIAnnotationData } from '../../types/api';
import type { ExportContent } from '../services/annotations-exporter';
/**
* Parse a file generated by the annotation exporter and return the extracted
* annotations.
*/
export async function readExportFile(file: File): Promise<APIAnnotationData[]> {
let json;
try {
json = await readJSONFile(file);
} catch (err) {
throw new Error('Not a valid JSON file');
}
// Perform some very rudimentary validation of the file content, just enough
// to catch issues where the user picked the wrong kind of JSON file.
if (!isObject(json) || !Array.isArray((json as any).annotations)) {
throw new Error('Not a valid Hypothesis JSON file');
}
return (json as ExportContent).annotations;
}
import { readExportFile } from '../import';
function createFile(content, name = 'example.json') {
if (typeof content === 'object') {
content = JSON.stringify(content);
}
return new File([content], name);
}
describe('readExportFile', () => {
it('throws error if file is not JSON', async () => {
let error;
try {
await readExportFile(createFile('foo bar'));
} catch (err) {
error = err;
}
assert.instanceOf(error, Error);
assert.equal(error.message, 'Not a valid JSON file');
});
[
// Top-level is not an object
[],
123,
// Missing `annotations` field
{},
// Invalid `annotations` field
{ annotations: 'not an array' },
].forEach(content => {
it('throws error if validation fails', async () => {
let error;
try {
await readExportFile(createFile(content));
} catch (err) {
error = err;
}
assert.instanceOf(error, Error);
assert.equal(error.message, 'Not a valid Hypothesis JSON file');
});
});
it('returns annotations from the file', async () => {
const annotations = [{ id: 'abc' }, { id: 'def' }];
const file = createFile({ annotations });
const parsedAnnotations = await readExportFile(file);
assert.deepEqual(parsedAnnotations, annotations);
});
});
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