fix(h5p-editor): improve error messages (#265)

* fix(h5p-editor): Improved error message when requesting uninstalled library data (#233)

* fix(Content Type Cache): The cache is now automatically downloaded if it hasn't been called before

* fix(h5p-editor): Improved error message when saving content with invalid library name

* fix(h5p-editor): Improved error messages

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<ContentId> {
         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,

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<boolean>} 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<boolean> {
+    public async forceUpdate(): Promise<any> {
         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<HubContentType[]>} Cached hub data in a format in which the version objects are flattened into the main object,
      */
     public async get(...machineNames: string[]): Promise<HubContentType[]> {
         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;

src/ContentTypeInformationRepository.ts

@@ -53,15 +53,6 @@ export default class ContentTypeInformationRepository {
     public async get(user: IUser): Promise<any> {
         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

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,17 +384,27 @@ 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)
         );
 
         const newContentId = await this.contentStorer.saveOrUpdateContent(
             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<IContentMetadata> {
         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,

src/LibraryManager.ts

@@ -78,7 +78,9 @@ export default class LibraryManager {
      * @returns {Promise<any>} 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<any> {
+    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<boolean> {
+        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

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(

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();