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

Granular template checks for marketplace theme validation #518

Merged
merged 7 commits into from
Jun 2, 2021
Merged
Show file tree
Hide file tree
Changes from 9 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
35 changes: 8 additions & 27 deletions packages/cli-lib/__tests__/templates.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const fs = require('fs');

const { isTemplate } = require('../templates');
const { isCodedFile } = require('../templates');

jest.mock('fs');

Expand All @@ -15,39 +15,20 @@ const makeAnnotation = (options = {}) => {
};

describe('cli-lib/templates', () => {
describe('isTemplate()', () => {
describe('isCodedFile()', () => {
it('should return falseinvalid input', () => {
expect(isTemplate()).toBe(false);
expect(isTemplate(null)).toBe(false);
expect(isTemplate(1)).toBe(false);
expect(isCodedFile()).toBe(false);
expect(isCodedFile(null)).toBe(false);
expect(isCodedFile(1)).toBe(false);
});
it('should return false for modules', () => {
expect(isTemplate('folder.module/module.html')).toBe(false);
});
it('should return false for partials', () => {
// Standard partials
fs.readFileSync.mockReturnValue(
makeAnnotation({ isAvailableForNewContent: 'false' })
);
expect(isTemplate('folder.module/partial.html')).toBe(false);

// Global partials
fs.readFileSync.mockReturnValue(
makeAnnotation({ templateType: 'global_partial' })
);

expect(isTemplate('folder.module/partial.html')).toBe(false);
});
it('should return false for templateType none', () => {
fs.readFileSync.mockReturnValue(makeAnnotation({ templateType: 'none' }));

expect(isTemplate('folder.module/template.html')).toBe(false);
expect(isCodedFile('folder.module/module.html')).toBe(false);
});
it('should return true for templates', () => {
// Without isAvailableForNewContent
fs.readFileSync.mockReturnValue(makeAnnotation({ templateType: 'page' }));

expect(isTemplate('folder.module/template.html')).toBe(true);
expect(isCodedFile('folder.module/template.html')).toBe(true);

// With isAvailableForNewContent
fs.readFileSync.mockReturnValue(
Expand All @@ -57,7 +38,7 @@ describe('cli-lib/templates', () => {
})
);

expect(isTemplate('folder.module/template.html')).toBe(true);
expect(isCodedFile('folder.module/template.html')).toBe(true);
});
});
});
14 changes: 3 additions & 11 deletions packages/cli-lib/templates.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,28 +37,20 @@ const getAnnotationValue = (annotations, key) => {
* Returns true if:
* .html extension (ignoring module.html)
* isAvailableForNewContent is null or true
* templateType is NOT 'global_partial' or 'none'
*/
const isTemplate = filePath => {
const isCodedFile = filePath => {
if (TEMPLATE_EXTENSION_REGEX.test(filePath)) {
const annotations = getFileAnnotations(filePath);
if (!annotations.length) {
return false;
}

const templateType = getAnnotationValue(
annotations,
ANNOTATION_KEYS.templateType
);
const isAvailableForNewContent = getAnnotationValue(
annotations,
ANNOTATION_KEYS.isAvailableForNewContent
);

return (
isAvailableForNewContent !== 'false' &&
!['global_partial', 'none'].includes(templateType)
);
return isAvailableForNewContent !== 'false';
}
return false;
};
Expand All @@ -67,5 +59,5 @@ module.exports = {
ANNOTATION_KEYS,
getAnnotationValue,
getFileAnnotations,
isTemplate,
isCodedFile,
};
3 changes: 3 additions & 0 deletions packages/cli/commands/marketplaceValidate/validateTheme.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const { walk } = require('@hubspot/cli-lib');
const {
addConfigOptions,
addAccountOptions,
addUseEnvironmentOptions,
setLogLevel,
getAccountId,
} = require('../../lib/commonOpts');
Expand Down Expand Up @@ -78,6 +79,8 @@ exports.handler = async options => {
exports.builder = yargs => {
addConfigOptions(yargs, true);
addAccountOptions(yargs, true);
addUseEnvironmentOptions(yargs, true);

yargs.options({
json: {
describe: 'Output raw json data',
Expand Down
9 changes: 6 additions & 3 deletions packages/cli/commands/watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,12 @@ exports.handler = async options => {
logger.info(
`The "watch" command no longer uploads the watched directory when started. The directory "${src}" was not uploaded.`
);
logger.info(
'To upload the directory run "hs upload" beforehand or add the "--initial-upload" option when running "hs watch".'
);

if (!initialUpload) {
logger.info(
'To upload the directory run "hs upload" beforehand or add the "--initial-upload" option when running "hs watch".'
);
}
}

trackCommandUsage('watch', { mode }, accountId);
Expand Down
20 changes: 4 additions & 16 deletions packages/cli/lib/validators/__tests__/ModuleValidator.js
Original file line number Diff line number Diff line change
@@ -1,31 +1,19 @@
const fs = require('fs');
const ModuleValidator = require('../marketplaceValidators/theme/ModuleValidator');
const { VALIDATION_RESULT } = require('../constants');
const { generateModulesList, makeFindError } = require('./validatorTestUtils');

jest.mock('fs');

const MODULE_LIMIT = 50;

const makeFilesList = numFiles => {
const files = [];
for (let i = 0; i < numFiles; i++) {
const base = `module-${i}.module`;
files.push(`${base}/meta.json`);
files.push(`${base}/fields.json`);
files.push(`${base}/module.html`);
files.push(`${base}/module.js`);
}
return files;
};

const findError = (errors, errorKey) =>
errors.find(error => error.key === `module.${errorKey}`);
const findError = makeFindError('module');

describe('validators/marketplaceValidators/theme/ModuleValidator', () => {
it('returns error if module limit is exceeded', async () => {
const validationErrors = ModuleValidator.validate(
'dirName',
makeFilesList(MODULE_LIMIT + 1)
generateModulesList(MODULE_LIMIT + 1)
);
const limitError = findError(validationErrors, 'limitExceeded');
expect(limitError).toBeDefined();
Expand All @@ -35,7 +23,7 @@ describe('validators/marketplaceValidators/theme/ModuleValidator', () => {
it('returns no limit error if module limit is not exceeded', async () => {
const validationErrors = ModuleValidator.validate(
'dirName',
makeFilesList(MODULE_LIMIT)
generateModulesList(MODULE_LIMIT)
);
const limitError = findError(validationErrors, 'limitExceeded');
expect(limitError).not.toBeDefined();
Expand Down
57 changes: 44 additions & 13 deletions packages/cli/lib/validators/__tests__/TemplateValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,50 +3,58 @@ const templates = require('@hubspot/cli-lib/templates');

const TemplateValidator = require('../marketplaceValidators/theme/TemplateValidator');
const { VALIDATION_RESULT } = require('../constants');
const {
generateTemplatesList,
makeFindError,
} = require('./validatorTestUtils');

jest.mock('fs');
jest.mock('@hubspot/cli-lib/templates');

const TEMPLATE_LIMIT = 50;

const makeFilesList = numFiles => {
const files = [];
for (let i = 0; i < numFiles; i++) {
files.push(`file-${i}.html`);
}
return files;
const mockGetAnnotationValue = (templateType, rest) => {
templates.getAnnotationValue.mockImplementation((x, key) => {
if (key === 'templateType') {
return templateType;
}
return rest;
});
};

const findError = (errors, errorKey) =>
errors.find(error => error.key === `template.${errorKey}`);
const findError = makeFindError('template');

describe('validators/marketplaceValidators/theme/TemplateValidator', () => {
beforeEach(() => {
templates.isTemplate.mockReturnValue(true);
templates.isCodedFile.mockReturnValue(true);
});

it('returns error if template limit is exceeded', async () => {
mockGetAnnotationValue('page');

const validationErrors = TemplateValidator.validate(
'dirName',
makeFilesList(TEMPLATE_LIMIT + 1)
generateTemplatesList(TEMPLATE_LIMIT + 1)
);
const limitError = findError(validationErrors, 'limitExceeded');
expect(limitError).toBeDefined();
expect(validationErrors[0].result).toBe(VALIDATION_RESULT.FATAL);
});

it('returns no errors if template limit is not exceeded', async () => {
mockGetAnnotationValue('page');

const validationErrors = TemplateValidator.validate(
'dirName',
makeFilesList(TEMPLATE_LIMIT)
generateTemplatesList(TEMPLATE_LIMIT)
);
const limitError = findError(validationErrors, 'limitExceeded');
expect(limitError).not.toBeDefined();
});

it('returns error if template annotation is missing label and screenshotPath', async () => {
fs.readFileSync.mockReturnValue('mock');
templates.getAnnotationValue.mockReturnValue(null);
mockGetAnnotationValue('page');

const validationErrors = TemplateValidator.validate('dirName', [
'template.html',
Expand All @@ -55,13 +63,36 @@ describe('validators/marketplaceValidators/theme/TemplateValidator', () => {
expect(validationErrors[0].result).toBe(VALIDATION_RESULT.FATAL);
});

it('returns error if template type is not allowed', async () => {
fs.readFileSync.mockReturnValue('mock');
mockGetAnnotationValue('starter_landing_pages', 'value');

const validationErrors = TemplateValidator.validate('dirName', [
'template.html',
]);

expect(validationErrors.length).toBe(1);
});

it('returns no error if templateType is not found', async () => {
fs.readFileSync.mockReturnValue('mock');
mockGetAnnotationValue(null, 'value');

const validationErrors = TemplateValidator.validate('dirName', [
'template.html',
]);

expect(validationErrors.length).toBe(0);
});

it('returns no error if template annotation has label and screenshotPath', async () => {
fs.readFileSync.mockReturnValue('mock');
templates.getAnnotationValue.mockReturnValue('some-value');
mockGetAnnotationValue('page', 'value');

const validationErrors = TemplateValidator.validate('dirName', [
'template.html',
]);

expect(validationErrors.length).toBe(0);
});
});
31 changes: 31 additions & 0 deletions packages/cli/lib/validators/__tests__/validatorTestUtils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//HACK so we can keep this util file next to the tests that use it
test.skip('skip', () => null);

const makeFindError = baseKey => (errors, errorKey) =>
errors.find(error => error.key === `${baseKey}.${errorKey}`);

const generateModulesList = numFiles => {
const files = [];
for (let i = 0; i < numFiles; i++) {
const base = `module-${i}.module`;
files.push(`${base}/meta.json`);
files.push(`${base}/fields.json`);
files.push(`${base}/module.html`);
files.push(`${base}/module.js`);
}
return files;
};

const generateTemplatesList = numFiles => {
const files = [];
for (let i = 0; i < numFiles; i++) {
files.push(`template-${i}.html`);
}
return files;
};

module.exports = {
generateModulesList,
generateTemplatesList,
makeFindError,
};
Loading