diff --git a/src/ContentStorer.ts b/src/ContentStorer.ts index 065aaef27..6833f2ab8 100644 --- a/src/ContentStorer.ts +++ b/src/ContentStorer.ts @@ -5,9 +5,8 @@ import { ContentFileScanner, IFileReference } from './ContentFileScanner'; import ContentManager from './ContentManager'; import Logger from './helpers/Logger'; import LibraryManager from './LibraryManager'; -import LibraryName from './LibraryName'; import TemporaryFileManager from './TemporaryFileManager'; -import { ContentId, IContentMetadata, IUser } from './types'; +import { ContentId, IContentMetadata, ILibraryName, IUser } from './types'; const log = new Logger('ContentStorer'); @@ -32,14 +31,14 @@ export default class ContentStorer { * @param contentId the contentId (can be undefined if previously unsaved) * @param parameters the parameters of teh content (= content.json) * @param metadata = content of h5p.json - * @param mainLibraryUberName use whitespace as separated (e.g. H5P.Example 1.0) + * @param mainLibraryName the library name * @param user the user who wants to save the file */ public async saveOrUpdateContent( contentId: ContentId, parameters: any, metadata: IContentMetadata, - mainLibraryUberName: string, + mainLibraryName: ILibraryName, user: IUser ): Promise { const isUpdate = contentId !== undefined; @@ -55,9 +54,7 @@ export default class ContentStorer { // were deleted in the editor by the user. const fileReferencesInNewParams = await this.contentFileScanner.scanForFiles( parameters, - LibraryName.fromUberName(mainLibraryUberName, { - useWhitespace: true - }) + mainLibraryName ); const filesToCopyFromTemporaryStorage = await this.determineFilesToCopyFromTemporaryStorage( fileReferencesInNewParams, diff --git a/src/ContentTypeCache.ts b/src/ContentTypeCache.ts index ee4a9aab7..f59982aa6 100644 --- a/src/ContentTypeCache.ts +++ b/src/ContentTypeCache.ts @@ -123,43 +123,58 @@ export default class ContentTypeCache { /** * Downloads the content type information from the H5P Hub and stores it in the storage object. - * @returns {Promise} true if update was done; false if it failed (e.g. because Hub was unreachable) + * @returns the downloaded (and saved) cache; undefined if it failed (e.g. because Hub was unreachable) */ - public async forceUpdate(): Promise { + public async forceUpdate(): Promise { log.info(`forcing update`); let cacheInHubFormat; try { cacheInHubFormat = await this.downloadContentTypesFromHub(); if (!cacheInHubFormat) { - return false; + return undefined; } } catch (error) { log.error(error); - return false; + return undefined; } const cacheInInternalFormat = cacheInHubFormat.map( ContentTypeCache.convertCacheEntryToLocalFormat ); await this.storage.save('contentTypeCache', cacheInInternalFormat); await this.storage.save('contentTypeCacheUpdate', Date.now()); - return true; + return cacheInInternalFormat; } /** * Returns the cache data. - * @param {string[]} machineNames (optional) The method only returns content type cache data for these machinen ames. + * @param {string[]} machineNames (optional) The method only returns content type cache data for these machine names. * @returns {Promise} Cached hub data in a format in which the version objects are flattened into the main object, */ public async get(...machineNames: string[]): Promise { log.info(`getting content types`); + + let cache = await this.storage.load('contentTypeCache'); + if (!cache) { + log.info( + 'ContentTypeCache was never updated before. Downloading it from the H5P Hub...' + ); + // try updating cache if it is empty for some reason + cache = await this.forceUpdate(); + // if the cache is still empty (e.g. because no connection to the H5P Hub can be established, return an empty array) + if (!cache) { + log.info( + 'ContentTypeCache could not be retrieved from H5P Hub.' + ); + return []; + } + } + if (!machineNames || machineNames.length === 0) { - return this.storage.load('contentTypeCache'); + return cache; } - return ( - await this.storage.load('contentTypeCache') - ).filter((contentType: HubContentType) => + return cache.filter((contentType: HubContentType) => machineNames.some( - (machineName: string) => machineName === contentType.machineName + machineName => machineName === contentType.machineName ) ); } @@ -218,7 +233,7 @@ export default class ContentTypeCache { const oldCache = await this.storage.load('contentTypeCache'); if (!oldCache || (await this.isOutdated())) { log.info(`update is necessary`); - return this.forceUpdate(); + return (await this.forceUpdate()) !== undefined; } log.info(`no update necessary`); return false; diff --git a/src/ContentTypeInformationRepository.ts b/src/ContentTypeInformationRepository.ts index c576d0abe..e8ff46e2d 100644 --- a/src/ContentTypeInformationRepository.ts +++ b/src/ContentTypeInformationRepository.ts @@ -53,15 +53,6 @@ export default class ContentTypeInformationRepository { public async get(user: IUser): Promise { log.info(`getting information about available content types`); let cachedHubInfo = await this.contentTypeCache.get(); - if (!cachedHubInfo) { - // try updating cache if it is empty for some reason - await this.contentTypeCache.updateIfNecessary(); - cachedHubInfo = await this.contentTypeCache.get(); - } - if (!cachedHubInfo) { - // if the H5P Hub is unreachable use empty array (so that local libraries can be added) - cachedHubInfo = []; - } cachedHubInfo = await this.addUserAndInstallationSpecificInfo( cachedHubInfo, user diff --git a/src/H5PEditor.ts b/src/H5PEditor.ts index 195defe26..6319212ef 100644 --- a/src/H5PEditor.ts +++ b/src/H5PEditor.ts @@ -151,7 +151,7 @@ export default class H5PEditor { return this.contentTypeRepository.get(user); } - public getLibraryData( + public async getLibraryData( machineName: string, majorVersion: number, minorVersion: number, @@ -165,36 +165,42 @@ export default class H5PEditor { majorVersion, minorVersion ); - return Promise.all([ + + if (!(await this.libraryManager.libraryExists(library))) { + throw new H5pError( + `Library ${LibraryName.toUberName(library)} was not found.` + ); + } + + const [ + assets, + semantics, + languageObject, + languages, + upgradeScriptPath + ] = await Promise.all([ this.loadAssets(machineName, majorVersion, minorVersion, language), this.libraryManager.loadSemantics(library), this.libraryManager.loadLanguage(library, language), this.libraryManager.listLanguages(library), this.libraryManager.getUpgradesScriptPath(library) - ]).then( - ([ - assets, - semantics, - languageObject, - languages, - upgradeScriptPath - ]) => ({ - languages, - semantics, - // tslint:disable-next-line: object-literal-sort-keys - css: assets.styles, - defaultLanguage: null, - language: languageObject, - name: machineName, - version: { - major: majorVersion, - minor: minorVersion - }, - javascript: assets.scripts, - translations: assets.translations, - upgradesScript: upgradeScriptPath // we don't check whether the path is null, as we can retur null - }) - ); + ]); + return { + languages, + semantics, + // tslint:disable-next-line: object-literal-sort-keys + css: assets.styles, + defaultLanguage: null, + language: languageObject, + name: machineName, + version: { + major: majorVersion, + minor: minorVersion + }, + javascript: assets.scripts, + translations: assets.translations, + upgradesScript: upgradeScriptPath // we don't check whether the path is null, as we can retur null + }; } /** @@ -378,9 +384,19 @@ export default class H5PEditor { log.info('saving new content'); } + // validate library name + let parsedLibraryName: ILibraryName; + try { + parsedLibraryName = LibraryName.fromUberName(mainLibraryName, { + useWhitespace: true + }); + } catch (error) { + throw new H5pError(`mainLibraryName is invalid: ${error.message}`); + } + const h5pJson: IContentMetadata = await this.generateH5PJSON( metadata, - mainLibraryName, + parsedLibraryName, this.findLibraries(parameters) ); @@ -388,7 +404,7 @@ export default class H5PEditor { contentId, parameters, h5pJson, - mainLibraryName, + parsedLibraryName, user ); return newContentId; @@ -597,17 +613,13 @@ export default class H5PEditor { private generateH5PJSON( metadata: IContentMetadata, - libraryName: string, + libraryName: ILibraryName, contentDependencies: ILibraryName[] = [] ): Promise { log.info(`generating h5p.json`); return new Promise((resolve: (value: IContentMetadata) => void) => { this.libraryManager - .loadLibrary( - LibraryName.fromUberName(libraryName, { - useWhitespace: true - }) - ) + .loadLibrary(libraryName) .then((library: InstalledLibrary) => { const h5pJson: IContentMetadata = { ...metadata, diff --git a/src/LibraryManager.ts b/src/LibraryManager.ts index 8d7c17127..3e17ee537 100644 --- a/src/LibraryManager.ts +++ b/src/LibraryManager.ts @@ -78,7 +78,9 @@ export default class LibraryManager { * @returns {Promise} An object which has properties with the existing library machine names. The properties' * values are arrays of Library objects, which represent the different versions installed of this library. */ - public async getInstalled(machineNames?: string[]): Promise { + public async getInstalled( + machineNames?: string[] + ): Promise<{ [key: string]: IInstalledLibrary[] }> { log.verbose(`checking if libraries ${machineNames} are installed`); let libraries = await this.libraryStorage.getInstalled(...machineNames); libraries = ( @@ -201,6 +203,25 @@ export default class LibraryManager { return false; } + /** + * Checks if a library was installed. + * @param library the library to check + * @returns true if the library has been installed + */ + public async libraryExists(library: LibraryName): Promise { + const installed = await this.getInstalled([library.machineName]); + if (!installed || !installed[library.machineName]) { + return false; + } + return ( + installed[library.machineName].find( + l => + l.majorVersion === library.majorVersion && + l.minorVersion === l.minorVersion + ) !== undefined + ); + } + /** * Check if the library contains a file * @param {ILibraryName} library The library to check diff --git a/src/LibraryName.ts b/src/LibraryName.ts index 7493523e3..905498ed1 100644 --- a/src/LibraryName.ts +++ b/src/LibraryName.ts @@ -38,10 +38,21 @@ export default class LibraryName implements ILibraryName { ? /([^\s]+)-(\d+)\.(\d+)/ : /([^\s]+)\s(\d+)\.(\d+)/; - const result: RegExpExecArray = nameRegex.exec(libraryName); + const result = nameRegex.exec(libraryName); if (!result) { - return undefined; + let example = ''; + if (options.useHyphen && options.useWhitespace) { + example = 'H5P.Example-1.0 or H5P.Example 1.0'; + } else if (options.useHyphen && !options.useWhitespace) { + example = 'H5P.Example-1.0'; + } else { + example = 'H5P.Example 1.0'; + } + + throw new Error( + `'${libraryName} is not a valid H5P library name ("ubername"). You must follow this pattern: ${example}'` + ); } return new LibraryName( diff --git a/test/ContentTypeCache.test.ts b/test/ContentTypeCache.test.ts index fb131b2be..3fb7a059a 100644 --- a/test/ContentTypeCache.test.ts +++ b/test/ContentTypeCache.test.ts @@ -51,7 +51,7 @@ describe('getting H5P Hub content types', () => { const cache = new ContentTypeCache(config, storage); const cached = await cache.get(); - expect(cached).toBeUndefined(); + expect(cached).toEqual([]); }); it('loads content type information from the H5P Hub', async () => { const storage = new InMemoryStorage(); diff --git a/test/getLibraryData.test.ts b/test/H5PEditor.getLibraryData.test.ts similarity index 94% rename from test/getLibraryData.test.ts rename to test/H5PEditor.getLibraryData.test.ts index 0510cfa37..1e709cf40 100644 --- a/test/getLibraryData.test.ts +++ b/test/H5PEditor.getLibraryData.test.ts @@ -1,3 +1,5 @@ +import { withDir } from 'tmp-promise'; + import EditorConfig from '../examples/implementation/EditorConfig'; import FileLibraryStorage from '../examples/implementation/FileLibraryStorage'; import H5PEditor from '../src/H5PEditor'; @@ -17,6 +19,7 @@ describe('aggregating data from library folders for the editor', () => { const libraryManager = new LibraryManager(new FileLibraryStorage('')); Object.assign(libraryManager, { + libraryExists: () => Promise.resolve(true), listLanguages: () => Promise.resolve([]), loadLanguage: () => Promise.resolve(null), loadLibrary: () => { @@ -59,6 +62,7 @@ describe('aggregating data from library folders for the editor', () => { const libraryManager = new LibraryManager(new FileLibraryStorage('')); Object.assign(libraryManager, { + libraryExists: () => Promise.resolve(true), listLanguages: () => Promise.resolve([]), loadLanguage: () => Promise.resolve(null), loadLibrary: () => { @@ -102,6 +106,7 @@ describe('aggregating data from library folders for the editor', () => { const libraryManager = new LibraryManager(new FileLibraryStorage('')); Object.assign(libraryManager, { + libraryExists: () => Promise.resolve(true), listLanguages: () => Promise.resolve([]), loadLanguage: () => Promise.resolve(null), loadLibrary: ({ machineName }) => { @@ -112,6 +117,7 @@ describe('aggregating data from library folders for the editor', () => { { machineName: 'EditorDependency', majorVersion: 1, + minorVersion: 0 } ], @@ -193,6 +199,7 @@ describe('aggregating data from library folders for the editor', () => { const libraryManager = new LibraryManager(new FileLibraryStorage('')); Object.assign(libraryManager, { + libraryExists: () => Promise.resolve(true), listLanguages: () => Promise.resolve([]), loadLanguage: () => Promise.resolve(null), loadLibrary: ({ machineName }) => { @@ -292,6 +299,7 @@ describe('aggregating data from library folders for the editor', () => { const libraryManager = new LibraryManager(new FileLibraryStorage('')); Object.assign(libraryManager, { + libraryExists: () => Promise.resolve(true), listLanguages: () => Promise.resolve([]), loadLanguage, loadLibrary: ({ _machineName }) => { @@ -369,6 +377,7 @@ describe('aggregating data from library folders for the editor', () => { const libraryManager = new LibraryManager(new FileLibraryStorage('')); Object.assign(libraryManager, { + libraryExists: () => Promise.resolve(true), listLanguages, loadLibrary: ({ _machineName }) => { switch (_machineName) { @@ -441,6 +450,7 @@ describe('aggregating data from library folders for the editor', () => { const libraryManager = new LibraryManager(new FileLibraryStorage('')); Object.assign(libraryManager, { + libraryExists: () => Promise.resolve(true), listLanguages: () => Promise.resolve([]), loadLanguage: () => Promise.resolve(null), loadLibrary: ({ machineName }) => { @@ -546,4 +556,25 @@ describe('aggregating data from library folders for the editor', () => { ]); }); }); + + it('throws a helpful error message when using an uninstalled library name', async () => { + await withDir( + async ({ path: tempDirPath }) => { + const h5pEditor = new H5PEditor( + null, + new EditorConfig(null), + new FileLibraryStorage(tempDirPath), + null, + null, + (library, name) => '', + null + ); + + expect(h5pEditor.getLibraryData('Foo', 1, 2)).rejects.toThrow( + 'Library Foo-1.2 was not found.' + ); + }, + { keep: false, unsafeCleanup: true } + ); + }); }); diff --git a/test/H5PEditor.saving.test.ts b/test/H5PEditor.saving.test.ts index 4c4931596..48e8f13f1 100644 --- a/test/H5PEditor.saving.test.ts +++ b/test/H5PEditor.saving.test.ts @@ -574,4 +574,27 @@ describe('H5PEditor', () => { { keep: false, unsafeCleanup: true } ); }); + + it('returns a helpful error if libraryName is invalid', async () => { + await withDir( + async ({ path: tempDirPath }) => { + const { h5pEditor } = createH5PEditor(tempDirPath); + const user = new User(); + + // save the content + await expect( + h5pEditor.saveH5P( + undefined, + mockupParametersWithoutImage, + mockupMetadata, + 'abc', + user + ) + ).rejects.toThrowError( + 'mainLibraryName is invalid: \'abc is not a valid H5P library name ("ubername"). You must follow this pattern: H5P.Example 1.0\'' + ); + }, + { keep: false, unsafeCleanup: true } + ); + }); }); diff --git a/test/H5PEditor.test.ts b/test/H5PEditor.test.ts new file mode 100644 index 000000000..de23c2e50 --- /dev/null +++ b/test/H5PEditor.test.ts @@ -0,0 +1,77 @@ +import axios from 'axios'; +import axiosMockAdapter from 'axios-mock-adapter'; + +import EditorConfig from '../examples/implementation/EditorConfig'; +import H5PEditor from '../src/H5PEditor'; +import TranslationService from '../src/TranslationService'; + +import InMemoryStorage from '../examples/implementation/InMemoryStorage'; +import User from '../examples/implementation/User'; + +describe('H5PEditor: general', () => { + const axiosMock = new axiosMockAdapter(axios); + + it("updates ContentTypeCache if it hasn't been downloaded before", async () => { + const config = new EditorConfig(new InMemoryStorage()); + const h5pEditor = new H5PEditor( + new InMemoryStorage(), + config, + null, + null, + new TranslationService({ + 'hub-install-invalid-content-type': + 'hub-install-invalid-content-type' + }), + (library, name) => '', + null + ); + + axiosMock + .onPost(config.hubRegistrationEndpoint) + .reply(200, require('./data/content-type-cache/registration.json')); + axiosMock + .onPost(config.hubContentTypesEndpoint) + .reply( + 200, + require('./data/content-type-cache/1-content-type.json') + ); + + await expect( + h5pEditor.installLibrary('XYZ', new User()) + ).rejects.toThrow('hub-install-invalid-content-type'); + }); + + it("updates ContentTypeCache if it hasn't been downloaded before and tries installing", async () => { + const config = new EditorConfig(new InMemoryStorage()); + const h5pEditor = new H5PEditor( + new InMemoryStorage(), + config, + null, + null, + new TranslationService({ + 'hub-install-invalid-content-type': + 'hub-install-invalid-content-type' + }), + (library, name) => '', + null + ); + + axiosMock + .onPost(config.hubRegistrationEndpoint) + .reply(200, require('./data/content-type-cache/registration.json')); + axiosMock + .onPost(config.hubContentTypesEndpoint) + .reply( + 200, + require('./data/content-type-cache/1-content-type.json') + ); + axiosMock + .onGet(`${config.hubContentTypesEndpoint}H5P.Example1`) + .reply(500); + + // we check against the error message as we we've told axios to reply with 500 to requests to the Hub endpoint. + await expect( + h5pEditor.installLibrary('H5P.Example1', new User()) + ).rejects.toThrow('Request failed with status code 500'); + }); +}); diff --git a/test/LibraryName.test.ts b/test/LibraryName.test.ts index aa892c09e..c47276724 100644 --- a/test/LibraryName.test.ts +++ b/test/LibraryName.test.ts @@ -57,4 +57,29 @@ describe('Library model', () => { }); }).toThrow(); }); + + it("throws an error if an ubername can't be parsed", async () => { + expect(() => { + LibraryName.fromUberName('H5P.Test 0.1'); + }).toThrow( + 'H5P.Test 0.1 is not a valid H5P library name ("ubername"). You must follow this pattern: H5P.Example-1.0' + ); + + expect(() => { + LibraryName.fromUberName('H5P.Test-0.1', { + useWhitespace: true + }); + }).toThrow( + 'H5P.Test-0.1 is not a valid H5P library name ("ubername"). You must follow this pattern: H5P.Example 1.0' + ); + + expect(() => { + LibraryName.fromUberName('XYZ', { + useHyphen: true, + useWhitespace: true + }); + }).toThrow( + 'XYZ is not a valid H5P library name ("ubername"). You must follow this pattern: H5P.Example-1.0 or H5P.Example 1.0' + ); + }); });