Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Jambo build validation hook #229

Merged
merged 8 commits into from
Jun 8, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/commands/build/pagewriter.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const UserError = require('../../errors/usererror');
const { NO_LOCALE } = require('../../constants');
const LocalizationConfig = require('../../models/localizationconfig');
const TemplateArgsBuilder = require('./templateargsbuilder');
const TemplateDataValidator = require('./templatedatavalidator');
const { info } = require('../../utils/logger');

/**
Expand All @@ -30,6 +31,12 @@ module.exports = class PageWriter {
*/
this._templateArgsBuilder =
new TemplateArgsBuilder(config.templateDataFormatterHook);

/**
* @type {TemplateDataValidator}
*/
this._templateDataValidator =
new TemplateDataValidator(config.templateDataValidationHook);
}

/**
Expand All @@ -51,7 +58,6 @@ module.exports = class PageWriter {
throw new UserError(`Error: No config found for page: ${page.getName()}`);
}

info(`Writing output file for the '${page.getName()}' page`);
const templateArguments = this._templateArgsBuilder.buildArgs({
relativePath: this._calculateRelativePath(page.getOutputPath()),
pageName: page.getName(),
Expand All @@ -61,7 +67,13 @@ module.exports = class PageWriter {
locale: pageSet.getLocale(),
env: this._env
});

this._templateDataValidator.validate({
pageName: page.getName(),
pageData: templateArguments
});

info(`Writing output file for the '${page.getName()}' page`);
const template = hbs.compile(page.getTemplateContents());
const outputHTML = template(templateArguments);

Expand Down
3 changes: 3 additions & 0 deletions src/commands/build/sitesgenerator.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,10 +178,13 @@ class SitesGenerator {

const templateDataFormatterHook = path.resolve(
config.dirs.themes, config.defaultTheme, 'hooks', 'templatedataformatter.js');
const templateDataValidationHook = path.resolve(
config.dirs.themes, config.defaultTheme, 'hooks', 'templatedatavalidator.js');
// Write pages
new PageWriter({
outputDirectory: config.dirs.output,
templateDataFormatterHook: templateDataFormatterHook,
templateDataValidationHook: templateDataValidationHook,
env: env,
}).writePages(pageSet);
}
Expand Down
41 changes: 41 additions & 0 deletions src/commands/build/templatedatavalidator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
const fs = require('file-system');
const UserError = require('../../errors/usererror');
const { isCustomError } = require('../../utils/errorutils');
const { info } = require('../../utils/logger');

/**
* TemplateDataValidator is reponsible for checking data supplied to a page
* using Theme's custom validation steps (if any).
*/
module.exports = class TemplateDataValidator {
constructor(templateDataValidationHook) {
/**
* The path to template data validation hook.
* @type {string}
*/
this._templateDataValidationHook = templateDataValidationHook;
}

/**
* Execute validation hook's function if file exists
* @param {string} pageName name of the current page
* @param {Object} pageData template arguments for the current page
*/
validate({ pageName, pageData }) {
if (!fs.existsSync(this._templateDataValidationHook)) {
return;
}
try {
info(`Validating configuration for page "${pageName}".`);
const validatorFunction = require(this._templateDataValidationHook);
validatorFunction(pageData);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we want this class and the hook to throw an exception when the data is invalid. The alternative is that both return a boolean. I think the boolean has its advantages:

  • Consumers of the validators can more easily decide how they want to handle bad templates. It may be that they thrown an exception if the validation result is false. Or, they try and handle more gracefully. As an example, in the future, maybe users can configure Jambo to gracefully ignore bad pages and continue generation (although I think the more catastrophic failure is the way to go for now).
  • We can differentiate between an actual issue running the Theme supplied validator and an invalid template. Right now, if there was an exception thrown because the validator couldn't run properly, we would treat that the same as the validator finding the template to be no good. We should probably treat the two situations differently and give different errors.

Copy link
Contributor Author

@yen-tt yen-tt Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, it would give user more flexibility on exception handling. This would fall on the Theme’s hook to log in detail what is invalid, and the pageWriter will throw a new error of general bad template if hook return false. Updated in new commit

} catch (err) {
if(isCustomError(err)) {
throw err;
}
const msg =
`Error executing validation hook from ${this._templateDataValidationHook}: `;
throw new UserError(msg, err.stack);
}
}
}
83 changes: 83 additions & 0 deletions tests/commands/build/templatedatavalidator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
const TemplateDataValidator = require('../../../src/commands/build/templatedatavalidator');
const path = require('path');
const UserError = require('../../../src/errors/usererror');

describe('TemplateDataValidator validates config data using hook properly', () => {
const currentPageConfig = {
url: 'examplePage.html',
verticalKey: 'examplePage',
pageTitle: 'Example Page',
pageSettings: { search: { verticalKey: 'examplePage', defaultInitialSearch: '' } },
componentSettings: {
prop: 'example1',
},
verticalsToConfig: {
examplePage: {
prop: 'example2'
}
}
};
const verticalConfigs = {
page1: {
config: {
prop: 'example'
}
},
page2: {
config: {
prop: 'example2'
}
}
};
const global_config = {
sdkVersion: 'X.X',
apiKey: 'exampleKey',
experienceKey: 'slanswers',
locale: 'en'
};
const params = {
example: 'param'
};
const relativePath = '..';
const env = {
envVar: 'envVar',
};

it('does not throw an error with a correct config', () => {
const templateDataValidationHook = path.resolve(
__dirname, '../../fixtures/hooks/templatedatavalidator.js');
const templateData = {
currentPageConfig,
verticalConfigs,
global_config,
params,
relativePath,
env
};

expect(() => new TemplateDataValidator(templateDataValidationHook).validate({
pageName: 'examplePage',
pageData: templateData
})).not.toThrow(UserError);
});

it('throws an error when a field in config is missing', () => {
const templateDataValidationHook = path.resolve(
__dirname, '../../fixtures/hooks/templatedatavalidator.js');
const global_config_missing_key = {};
const templateData = {
currentPageConfig,
verticalConfigs,
global_config_missing_key,
params,
relativePath,
env
};

expect(() => new TemplateDataValidator(templateDataValidationHook).validate({
pageName: 'examplePage',
pageData: templateData
})).toThrow(UserError);

});
});
12 changes: 12 additions & 0 deletions tests/fixtures/hooks/templatedatavalidator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
const UserError = require('../../../src/errors/usererror');

/**
* A test data validator hook.
*
* @param {Object} pageData configuration(s) of a page template
*/
module.exports = function (pageData) {
if(!pageData["global_config"]["experienceKey"]) {
throw new UserError('Missing Info: experienceKey in config file(s)');
}
}