From 4cc20172c531a511d7274c3f30de1ec30b010573 Mon Sep 17 00:00:00 2001 From: Oliver Eyton-Williams Date: Fri, 15 Apr 2022 16:17:49 +0200 Subject: [PATCH] fix: fallback to english challenges (#45635) * fix: fallback to english challenges All challenges will use the english version if a translated file is not available. SHOW_NEW_CURRICULUM still gates what's shown in the client. * refactor: use closures to simplify createChallenge * refactor: remove messy destructure * refactor: add meta via helper * fix: fallback to [] for meta.required * fix: repair challenge.block * refactor: use CONST_CASE for meta + challenge dirs * fix: catch empty superblocks immediately * fix: clean up path.resolves * fix: invalid syntax in JS project steps * fix: default to english comments and relax tests Instead of always throwing errors when a comment is not translated, the tests now warn while SHOW_UPCOMING_CHANGES is true, so that tests will pass while we're developing and allow translators time to work. They still throw when SHOW_UPCOMING_CHANGES is false to catch issues in production * test: update createCommentMap test * refactor: delete stale comment * refactor: clarify validate with explanatory consts * feat: throw if audited cert falls back to english * fix: stop testing upcoming localized curriculum --- .github/workflows/node.js-tests.yml | 1 - client/utils/build-challenges.js | 22 +- .../basic-javascript-rpg-game/step-129.md | 2 +- .../step-063.md | 2 +- .../step-122.md | 2 +- curriculum/dictionaries/english/comments.json | 3 +- curriculum/getChallenges.js | 211 ++++++++++-------- curriculum/getChallenges.test.js | 42 ++-- curriculum/schema/challengeSchema.js | 2 +- curriculum/test/test-challenges.js | 18 +- 10 files changed, 171 insertions(+), 134 deletions(-) diff --git a/.github/workflows/node.js-tests.yml b/.github/workflows/node.js-tests.yml index e86b059238..1fb3ea950b 100644 --- a/.github/workflows/node.js-tests.yml +++ b/.github/workflows/node.js-tests.yml @@ -141,7 +141,6 @@ jobs: - name: Set Environment variables run: | cp sample.env .env - echo 'SHOW_UPCOMING_CHANGES=true' >> .env cat .env - name: Install Dependencies diff --git a/client/utils/build-challenges.js b/client/utils/build-challenges.js index 06d8bb882d..73a03d575a 100644 --- a/client/utils/build-challenges.js +++ b/client/utils/build-challenges.js @@ -5,8 +5,9 @@ const _ = require('lodash'); const envData = require('../../config/env.json'); const { getChallengesForLang, - createChallenge, - challengesDir, + generateChallengeCreator, + CHALLENGES_DIR, + META_DIR, getChallengesDirForLang } = require('../../curriculum/getChallenges'); @@ -20,22 +21,19 @@ exports.replaceChallengeNode = () => { const blockNameRe = /\d\d-[-\w]+\/([^/]+)\//; const posix = path.normalize(filePath).split(path.sep).join(path.posix.sep); const blockName = posix.match(blockNameRe)[1]; - const metaPath = path.resolve( - __dirname, - `../../curriculum/challenges/_meta/${blockName}/meta.json` - ); + const metaPath = path.resolve(META_DIR, `/${blockName}/meta.json`); delete require.cache[require.resolve(metaPath)]; const meta = require(metaPath); // TODO: reimplement hot-reloading of certifications - return await createChallenge( - challengesDir, - filePath, - curriculumLocale, - meta - ); + return await createChallenge(filePath, meta); }; }; +const createChallenge = generateChallengeCreator( + CHALLENGES_DIR, + curriculumLocale +); + exports.buildChallenges = async function buildChallenges() { const curriculum = await getChallengesForLang(curriculumLocale); const superBlocks = Object.keys(curriculum); diff --git a/curriculum/challenges/english/02-javascript-algorithms-and-data-structures/basic-javascript-rpg-game/step-129.md b/curriculum/challenges/english/02-javascript-algorithms-and-data-structures/basic-javascript-rpg-game/step-129.md index ec2e5921e6..19699584e6 100644 --- a/curriculum/challenges/english/02-javascript-algorithms-and-data-structures/basic-javascript-rpg-game/step-129.md +++ b/curriculum/challenges/english/02-javascript-algorithms-and-data-structures/basic-javascript-rpg-game/step-129.md @@ -199,7 +199,7 @@ const locations = [ "button text": ["REPLAY?", "REPLAY?", "REPLAY?"], "button functions": [restart, restart, restart], text: "You die. ☠️" - } + }, { name: "win", "button text": ["Fight slime", "Fight fanged beast", "Go to town square"], diff --git a/curriculum/challenges/english/02-javascript-algorithms-and-data-structures/functional-programming-spreadsheet/step-063.md b/curriculum/challenges/english/02-javascript-algorithms-and-data-structures/functional-programming-spreadsheet/step-063.md index 47b7eddbc2..fbaf60e87a 100644 --- a/curriculum/challenges/english/02-javascript-algorithms-and-data-structures/functional-programming-spreadsheet/step-063.md +++ b/curriculum/challenges/english/02-javascript-algorithms-and-data-structures/functional-programming-spreadsheet/step-063.md @@ -111,7 +111,7 @@ const charRange = (start, end) => const evalFormula = x => { const rangeRegex = /([A-J])([1-9][0-9]?):([A-J])([1-9][0-9]?)/gi; const rangeFromString = (n1, n2) => range(parseInt(n1), parseInt(n2)); - const elemValue = n => (c => document.getElementById(c + n).value)); + const elemValue = n => c => document.getElementById(c + n).value; const fn = elemValue("1"); return fn("A"); }; diff --git a/curriculum/challenges/english/02-javascript-algorithms-and-data-structures/functional-programming-spreadsheet/step-122.md b/curriculum/challenges/english/02-javascript-algorithms-and-data-structures/functional-programming-spreadsheet/step-122.md index abb7002b4d..dc00ccf4e4 100644 --- a/curriculum/challenges/english/02-javascript-algorithms-and-data-structures/functional-programming-spreadsheet/step-122.md +++ b/curriculum/challenges/english/02-javascript-algorithms-and-data-structures/functional-programming-spreadsheet/step-122.md @@ -90,7 +90,7 @@ const spreadsheetFunctions = { lasttwo: arr => arr.slice(-2), even: nums => nums.filter(isEven), sum: nums => nums.reduce((a, x) => a + x), - has2: arr => arr.includes(2) + has2: arr => arr.includes(2), nodups: arr => arr.reduce((a, x) => a.includes(x), []) }; diff --git a/curriculum/dictionaries/english/comments.json b/curriculum/dictionaries/english/comments.json index 4330bb636c..42b1753551 100644 --- a/curriculum/dictionaries/english/comments.json +++ b/curriculum/dictionaries/english/comments.json @@ -106,5 +106,6 @@ "es69h6": "When you join two windows into one window", "fho5t5": "When you open a new tab at the end", "00kcrm": "yields true", - "sxpg2a": "Your mailbox, drive, and other work sites" + "sxpg2a": "Your mailbox, drive, and other work sites", + "4143lf": "initialize buttons" } diff --git a/curriculum/getChallenges.js b/curriculum/getChallenges.js index 97d418a1a9..bc77bc1c18 100644 --- a/curriculum/getChallenges.js +++ b/curriculum/getChallenges.js @@ -1,8 +1,9 @@ const fs = require('fs'); const path = require('path'); const util = require('util'); +const assert = require('assert'); const yaml = require('js-yaml'); -const { findIndex } = require('lodash'); +const { findIndex, isEmpty } = require('lodash'); const readDirP = require('readdirp'); const { helpCategoryMap } = require('../client/utils/challenge-types'); const { showUpcomingChanges } = require('../config/env.json'); @@ -22,13 +23,13 @@ const { getSuperOrder, getSuperBlockFromDir } = require('./utils'); const access = util.promisify(fs.access); -const challengesDir = path.resolve(__dirname, './challenges'); -const metaDir = path.resolve(challengesDir, '_meta'); -exports.challengesDir = challengesDir; -exports.metaDir = metaDir; +const CHALLENGES_DIR = path.resolve(__dirname, 'challenges'); +const META_DIR = path.resolve(CHALLENGES_DIR, '_meta'); +exports.CHALLENGES_DIR = CHALLENGES_DIR; +exports.META_DIR = META_DIR; const COMMENT_TRANSLATIONS = createCommentMap( - path.resolve(__dirname, './dictionaries') + path.resolve(__dirname, 'dictionaries') ); function getTranslatableComments(dictionariesDir) { @@ -109,20 +110,19 @@ function getTranslationEntry(dicts, { engId, text }) { if (entry) { return { ...acc, [lang]: entry }; } else { - throw Error(`Missing translation for comment -'${text}' - with id of ${engId}`); + // default to english + return { ...acc, [lang]: text }; } }, {}); } function getChallengesDirForLang(lang) { - return path.resolve(challengesDir, `./${lang}`); + return path.resolve(CHALLENGES_DIR, `${lang}`); } function getMetaForBlock(block) { return JSON.parse( - fs.readFileSync(path.resolve(metaDir, `./${block}/meta.json`), 'utf8') + fs.readFileSync(path.resolve(META_DIR, `${block}/meta.json`), 'utf8') ); } @@ -153,7 +153,9 @@ const walk = (root, target, options, cb) => { }; exports.getChallengesForLang = async function getChallengesForLang(lang) { - const root = getChallengesDirForLang(lang); + // english determines the shape of the curriculum, all other languages mirror + // it. + const root = getChallengesDirForLang('english'); // scaffold the curriculum, first set up the superblocks, then recurse into // the blocks const curriculum = await walk( @@ -162,6 +164,9 @@ exports.getChallengesForLang = async function getChallengesForLang(lang) { { type: 'directories', depth: 0 }, buildSuperBlocks ); + Object.entries(curriculum).forEach(([name, superBlock]) => { + assert(!isEmpty(superBlock.blocks), `superblock ${name} has no blocks`); + }); const cb = (file, curriculum) => buildChallenges(file, curriculum, lang); // fill the scaffold with the challenges return walk( @@ -173,10 +178,7 @@ exports.getChallengesForLang = async function getChallengesForLang(lang) { }; async function buildBlocks({ basename: blockName }, curriculum, superBlock) { - const metaPath = path.resolve( - __dirname, - `./challenges/_meta/${blockName}/meta.json` - ); + const metaPath = path.resolve(META_DIR, `${blockName}/meta.json`); if (fs.existsSync(metaPath)) { // try to read the file, if the meta path does not exist it should be a certification. @@ -240,9 +242,10 @@ async function buildChallenges({ path: filePath }, curriculum, lang) { ) { return; } + const createChallenge = generateChallengeCreator(CHALLENGES_DIR, lang); const challenge = isCert - ? await createCertification(challengesDir, filePath, lang) - : await createChallenge(challengesDir, filePath, lang, meta); + ? await createCertification(CHALLENGES_DIR, filePath, lang) + : await createChallenge(filePath, meta); challengeBlock.challenges = [...challengeBlock.challenges, challenge]; } @@ -258,8 +261,7 @@ async function parseTranslation(transPath, dict, lang, parse = parseMD) { : translatedChal; } -// eslint-disable-next-line no-unused-vars -async function createCertification(basePath, filePath, lang) { +async function createCertification(basePath, filePath) { function getFullPath(pathLang) { return path.resolve(__dirname, basePath, pathLang, filePath); } @@ -270,90 +272,111 @@ async function createCertification(basePath, filePath, lang) { return parseCert(getFullPath('english')); } -async function createChallenge(basePath, filePath, lang, maybeMeta) { - function getFullPath(pathLang) { +// This is a slightly weird abstraction, but it lets us define helper functions +// without passing around a ton of arguments. +function generateChallengeCreator(basePath, lang) { + function getFullPath(pathLang, filePath) { return path.resolve(__dirname, basePath, pathLang, filePath); } - let meta; - if (maybeMeta) { - meta = maybeMeta; - } else { - const metaPath = path.resolve( - metaDir, - `./${getBlockNameFromPath(filePath)}/meta.json` - ); - meta = require(metaPath); - } - const { superBlock } = meta; - if (!curriculumLangs.includes(lang)) - throw Error(`${lang} is not a accepted language. - Trying to parse ${filePath}`); - if (lang !== 'english' && !(await hasEnglishSource(basePath, filePath))) - throw Error(`Missing English challenge for + + async function validate(filePath, superBlock) { + const invalidLang = !curriculumLangs.includes(lang); + if (invalidLang) + throw Error(`${lang} is not a accepted language. +Trying to parse ${filePath}`); + + const missingEnglish = + lang !== 'english' && !(await hasEnglishSource(basePath, filePath)); + if (missingEnglish) + throw Error(`Missing English challenge for ${filePath} It should be in -${getFullPath('english')} +${getFullPath('english', filePath)} `); - // assumes superblock names are unique - // while the auditing is ongoing, we default to English for un-audited certs - // once that's complete, we can revert to using isEnglishChallenge(fullPath) - const useEnglish = lang === 'english' || !isAuditedCert(lang, superBlock); - const challenge = await (useEnglish - ? parseMD(getFullPath('english')) - : parseTranslation(getFullPath(lang), COMMENT_TRANSLATIONS, lang)); + const missingAuditedChallenge = + isAuditedCert(lang, superBlock) && + !fs.existsSync(getFullPath(lang, filePath)); + if (missingAuditedChallenge) + throw Error(`Missing ${lang} audited challenge for +${filePath} +No audited challenges should fallback to English. + `); + } - const challengeOrder = findIndex( - meta.challengeOrder, - ([id]) => id === challenge.id - ); - const { - name: blockName, - hasEditableBoundaries, - order, - isPrivate, - required = [], - template, - time, - usesMultifileEditor - } = meta; - challenge.block = dasherize(blockName); - challenge.hasEditableBoundaries = !!hasEditableBoundaries; - challenge.order = order; - const superOrder = getSuperOrder(superBlock, { - showNewCurriculum: process.env.SHOW_NEW_CURRICULUM === 'true' - }); - if (superOrder !== null) challenge.superOrder = superOrder; - /* Since there can be more than one way to complete a certification (using the + function addMetaToChallenge(challenge, meta) { + const challengeOrder = findIndex( + meta.challengeOrder, + ([id]) => id === challenge.id + ); + + challenge.block = meta.name ? dasherize(meta.name) : null; + challenge.hasEditableBoundaries = !!meta.hasEditableBoundaries; + challenge.order = meta.order; + const superOrder = getSuperOrder(meta.superBlock, { + showNewCurriculum: process.env.SHOW_NEW_CURRICULUM === 'true' + }); + if (superOrder !== null) challenge.superOrder = superOrder; + /* Since there can be more than one way to complete a certification (using the legacy curriculum or the new one, for instance), we need a certification field to track which certification this belongs to. */ - // TODO: generalize this to all superBlocks - challenge.certification = - superBlock === '2022/responsive-web-design' - ? 'responsive-web-design' - : superBlock; - challenge.superBlock = superBlock; - challenge.challengeOrder = challengeOrder; - challenge.isPrivate = challenge.isPrivate || isPrivate; - challenge.required = required.concat(challenge.required || []); - challenge.template = template; - challenge.time = time; - challenge.helpCategory = - challenge.helpCategory || helpCategoryMap[challenge.block]; - challenge.translationPending = - lang !== 'english' && !isAuditedCert(lang, superBlock); - challenge.usesMultifileEditor = !!usesMultifileEditor; - if (challenge.challengeFiles) { - // The client expects the challengeFiles to be an array of polyvinyls - challenge.challengeFiles = challengeFilesToPolys(challenge.challengeFiles); - } - if (challenge.solutions?.length) { - // The test runner needs the solutions to be arrays of polyvinyls so it - // can sort them correctly. - challenge.solutions = challenge.solutions.map(challengeFilesToPolys); + // TODO: generalize this to all superBlocks + challenge.certification = + meta.superBlock === '2022/responsive-web-design' + ? 'responsive-web-design' + : meta.superBlock; + challenge.superBlock = meta.superBlock; + challenge.challengeOrder = challengeOrder; + challenge.isPrivate = challenge.isPrivate || meta.isPrivate; + challenge.required = (meta.required || []).concat(challenge.required || []); + challenge.template = meta.template; + challenge.time = meta.time; + challenge.helpCategory = + challenge.helpCategory || helpCategoryMap[challenge.block]; + challenge.translationPending = + lang !== 'english' && !isAuditedCert(lang, meta.superBlock); + challenge.usesMultifileEditor = !!meta.usesMultifileEditor; + if (challenge.challengeFiles) { + // The client expects the challengeFiles to be an array of polyvinyls + challenge.challengeFiles = challengeFilesToPolys( + challenge.challengeFiles + ); + } + if (challenge.solutions?.length) { + // The test runner needs the solutions to be arrays of polyvinyls so it + // can sort them correctly. + challenge.solutions = challenge.solutions.map(challengeFilesToPolys); + } } - return challenge; + async function createChallenge(filePath, maybeMeta) { + const meta = maybeMeta + ? maybeMeta + : require(path.resolve( + META_DIR, + `${getBlockNameFromPath(filePath)}/meta.json` + )); + + await validate(filePath, meta.superBlock); + + const useEnglish = + lang === 'english' || + !isAuditedCert(lang, meta.superBlock) || + !fs.existsSync(getFullPath(lang, filePath)); + + const challenge = await (useEnglish + ? parseMD(getFullPath('english', filePath)) + : parseTranslation( + getFullPath(lang, filePath), + COMMENT_TRANSLATIONS, + lang + )); + + addMetaToChallenge(challenge, meta); + + return challenge; + } + return createChallenge; } function challengeFilesToPolys(files) { @@ -390,4 +413,4 @@ function getBlockNameFromPath(filePath) { exports.hasEnglishSource = hasEnglishSource; exports.parseTranslation = parseTranslation; -exports.createChallenge = createChallenge; +exports.generateChallengeCreator = generateChallengeCreator; diff --git a/curriculum/getChallenges.test.js b/curriculum/getChallenges.test.js index b982d1aeb3..d40ad6b317 100644 --- a/curriculum/getChallenges.test.js +++ b/curriculum/getChallenges.test.js @@ -1,7 +1,7 @@ const path = require('path'); const { - createChallenge, + generateChallengeCreator, hasEnglishSource, createCommentMap } = require('./getChallenges'); @@ -12,21 +12,25 @@ const MISSING_CHALLENGE_PATH = 'no/challenge.md'; const basePath = '__fixtures__'; describe('create non-English challenge', () => { - describe('createChallenge', () => { - it('throws if lang is an invalid language', async () => { - await expect( - createChallenge(basePath, EXISTING_CHALLENGE_PATH, 'notlang', {}) - ).rejects.toThrow('notlang is not a accepted language'); - }); - it('throws an error if the source challenge is missing', async () => { - await expect( - createChallenge(basePath, MISSING_CHALLENGE_PATH, 'chinese', {}) - ).rejects.toThrow( - `Missing English challenge for + describe('generateChallengeCreator', () => { + describe('createChallenge', () => { + it('throws if lang is an invalid language', async () => { + const createChallenge = generateChallengeCreator(basePath, 'notlang'); + await expect( + createChallenge(EXISTING_CHALLENGE_PATH, {}) + ).rejects.toThrow('notlang is not a accepted language'); + }); + it('throws an error if the source challenge is missing', async () => { + const createChallenge = generateChallengeCreator(basePath, 'chinese'); + await expect( + createChallenge(MISSING_CHALLENGE_PATH, {}) + ).rejects.toThrow( + `Missing English challenge for ${MISSING_CHALLENGE_PATH} It should be in ` - ); + ); + }); }); }); describe('hasEnglishSource', () => { @@ -69,9 +73,15 @@ It should be in expect(typeof createCommentMap(dictionaryDir)).toBe('object'); }); - it('throws if an entry is missing', () => { - expect.assertions(1); - expect(() => createCommentMap(incompleteDictDir)).toThrow(); + it('fallback to the untranslated string', () => { + expect.assertions(2); + const commentMap = createCommentMap(incompleteDictDir); + expect(commentMap['To be translated one'].spanish).toEqual( + 'Spanish translation one' + ); + expect(commentMap['To be translated two'].spanish).toEqual( + 'To be translated two' + ); }); it('returns an object with an expected form', () => { diff --git a/curriculum/schema/challengeSchema.js b/curriculum/schema/challengeSchema.js index 3f0fa59801..d837e813e2 100644 --- a/curriculum/schema/challengeSchema.js +++ b/curriculum/schema/challengeSchema.js @@ -23,7 +23,7 @@ const fileJoi = Joi.object().keys({ const schema = Joi.object() .keys({ - block: Joi.string().regex(slugRE), + block: Joi.string().regex(slugRE).required(), blockId: Joi.objectId(), challengeOrder: Joi.number(), removeComments: Joi.bool(), diff --git a/curriculum/test/test-challenges.js b/curriculum/test/test-challenges.js index bbb379e90a..bb5bdb0f91 100644 --- a/curriculum/test/test-challenges.js +++ b/curriculum/test/test-challenges.js @@ -317,13 +317,13 @@ function populateTestsForLang({ lang, challenges, meta }) { // Note: the title in meta.json are purely for human readability and // do not include translations, so we do not validate against them. it('Matches an ID in meta.json', function () { - const index = meta[dashedBlockName].challengeOrder.findIndex( + const index = meta[dashedBlockName]?.challengeOrder?.findIndex( arr => arr[0] === challenge.id ); if (index < 0) { throw new AssertionError( - `Cannot find ID "${challenge.id}" in meta.json file` + `Cannot find ID "${challenge.id}" in meta.json file for block "${dashedBlockName}"` ); } }); @@ -370,11 +370,14 @@ function populateTestsForLang({ lang, challenges, meta }) { // currently have the text of a comment elsewhere. If that happens // we can handle that challenge separately. TRANSLATABLE_COMMENTS.forEach(comment => { + const errorText = `English comment '${comment}' should be replaced with its translation`; challenge.challengeFiles.forEach(challengeFile => { if (challengeFile.contents.includes(comment)) - throw Error( - `English comment '${comment}' should be replaced with its translation` - ); + if (process.env.SHOW_UPCOMING_CHANGES == 'true') { + console.warn(errorText); + } else { + throw Error(errorText); + } }); }); @@ -414,7 +417,10 @@ function populateTestsForLang({ lang, challenges, meta }) { if (isEmpty(challenge.__commentCounts) && isEmpty(commentMap)) return; - if (!isEqual(commentMap, challenge.__commentCounts)) + if ( + process.env.SHOW_NEW_CURRICULUM !== 'true' && + !isEqual(commentMap, challenge.__commentCounts) + ) throw Error(`Mismatch in ${challenge.title}. Replaced comments: ${inspect(challenge.__commentCounts)} Comments in translated text: