diff --git a/server/lib/defaults.js b/server/lib/defaults.js index fe679055..e6db37d5 100644 --- a/server/lib/defaults.js +++ b/server/lib/defaults.js @@ -5,6 +5,8 @@ const regex = require('./regex'); const MEDIUM_TEXT_LENGTH = 16777215; exports.mediumTextLength = MEDIUM_TEXT_LENGTH; +exports.deleteMe = 'GENERATED_DEFAULT_DELETE_ME'; + exports.errors = { title: 'You must enter a title for this test case.', slug: 'The slug can only contain alphanumeric characters and hyphens.', diff --git a/server/lib/schema.js b/server/lib/schema.js index 224defb3..da1fb918 100644 --- a/server/lib/schema.js +++ b/server/lib/schema.js @@ -21,12 +21,8 @@ exports.testPage = Joi.object().keys({ setup: mediumText, teardown: mediumText, test: Joi.array().required().min(2).items(Joi.object().required().keys({ - title: Joi.string().required().trim().allow('').empty('').max(255).when('code', { - is: '', - then: Joi.string().length(0), // must be blank too so can delete - otherwise: Joi.string().min(1) - }), - code: Joi.string().required().trim().allow('').max(defaults.mediumTextLength), + title: Joi.string().trim().allow('').empty('').default(defaults.deleteMe).max(255), + code: Joi.string().trim().allow('').empty('').default(defaults.deleteMe).max(defaults.mediumTextLength), defer: Joi.string().default('n').valid('y', 'n'), testID: Joi.number().integer().optional() // only present when editing })) diff --git a/server/repositories/tests.js b/server/repositories/tests.js index 856dd85e..c3dd1b66 100644 --- a/server/repositories/tests.js +++ b/server/repositories/tests.js @@ -1,3 +1,5 @@ +const defaults = require('../lib/defaults'); + const name = 'repositories/tests'; const table = 'tests'; @@ -26,10 +28,12 @@ exports.register = function (server, options, next) { let queries = []; tests.forEach(test => { - // setting existing test title and code to blank indicates it should be deleted ... - if (!test.title && !test.code) { + // setting existing test title and code to blank indicates it should be deleted + // Joi converts this to the default ... + if (test.title === defaults.deleteMe && test.code === defaults.deleteMe) { // ... if it's theirs to delete ... if (isOwn && test.testID) { + server.log('info', `deleting test ${test.testID} from page ${pageID}`); queries.push(db.genericQuery(`DELETE FROM ?? WHERE pageID = ${pageID} AND testID = ${test.testID}`, [table])); } // ... otherwise skip over the test diff --git a/server/web/edit/index.hbs b/server/web/edit/index.hbs index 3fddd0bc..a67d912f 100644 --- a/server/web/edit/index.hbs +++ b/server/web/edit/index.hbs @@ -149,6 +149,11 @@ This edit will create a new revision. {{/each}} + +

+ NB: leave Title and Code blank to remove code snippet. +

+
diff --git a/server/web/edit/index.js b/server/web/edit/index.js index b8a5ccf5..19320d21 100644 --- a/server/web/edit/index.js +++ b/server/web/edit/index.js @@ -79,26 +79,14 @@ exports.register = function (server, options, next) { }; Joi.validate(request.payload, schema.testPage, function (err, pageWithTests) { + let errObj = {}; if (err) { - let errObj = {}; + server.log(['error'], err); try { - const valErr = err.details[0]; - switch (valErr.path) { - case 'title': - errObj.titleError = defaults.errors.title; - break; - default: - const idx = valErr.path.split('.')[1]; - switch (valErr.context.key) { - case 'title': - request.payload.test[idx].codeTitleError = defaults.errors.codeTitle; - break; - case 'code': - request.payload.test[idx].codeError = defaults.errors.code; - break; - default: - throw err; - } + if (err.details[0].path === 'title') { + errObj.titleError = defaults.errors.title; + } else { + throw err; } } catch (ex) { server.log(['error'], ex); @@ -106,24 +94,46 @@ exports.register = function (server, options, next) { } errResp(errObj); } else { - let isOwn = false; + // additional validation that's not possible or insanely complex w/ Joi + let wasAdditionalValidationError = false; + pageWithTests.test.forEach((t, idx) => { + const missingTitle = t.title === defaults.deleteMe; + const missingCode = t.code === defaults.deleteMe; - pagesService.getBySlug(request.params.testSlug, request.params.rev) - .then(values => { - const prevPage = values[0]; - const own = request.yar.get('own') || {}; - isOwn = own[prevPage.id]; - const isAdmin = request.yar.get('admin'); - let update = !!(isAdmin || isOwn); - return pagesService.edit(pageWithTests, update, prevPage.maxRev, prevPage.id); - }) - .then(resultingRevision => { - request.yar.set('authorSlug', pageWithTests.author.replace(' ', '-').replace(/[^a-zA-Z0-9 -]/, '')); + if (missingTitle && !missingCode) { + wasAdditionalValidationError = true; + request.payload.test[idx].codeTitleError = defaults.errors.codeTitle; + } + + if (missingCode && !missingTitle) { + wasAdditionalValidationError = true; + request.payload.test[idx].codeError = defaults.errors.code; + } + }); - const r = resultingRevision > 1 ? `/${resultingRevision}` : ''; + if (wasAdditionalValidationError) { + errResp(errObj); + } else { + let isOwn = false; + + pagesService.getBySlug(request.params.testSlug, request.params.rev) + .then(values => { + const prevPage = values[0]; + const own = request.yar.get('own') || {}; + isOwn = own[prevPage.id]; + const isAdmin = request.yar.get('admin'); + let update = !!(isAdmin || isOwn); + server.log('debug', `isAdmin: ${isAdmin} isOwn: ${isOwn} update: ${update}`); + return pagesService.edit(pageWithTests, update, prevPage.maxRev, prevPage.id); + }) + .then(resultingRevision => { + request.yar.set('authorSlug', pageWithTests.author.replace(' ', '-').replace(/[^a-zA-Z0-9 -]/, '')); - reply.redirect(`/${request.params.testSlug}${r}`); - }).catch(errResp); + const r = resultingRevision > 1 ? `/${resultingRevision}` : ''; + + reply.redirect(`/${request.params.testSlug}${r}`); + }).catch(errResp); + } } }); } diff --git a/server/web/home/index.js b/server/web/home/index.js index 3a6dd637..9977350e 100644 --- a/server/web/home/index.js +++ b/server/web/home/index.js @@ -50,8 +50,8 @@ exports.register = function (server, options, next) { }; Joi.validate(request.payload, schema.testPage, function (er, pageWithTests) { + let errObj = {}; if (er) { - var errObj = {}; // `abortEarly` option defaults to `true` so can rely on 0 index // but just in case... try { @@ -65,44 +65,55 @@ exports.register = function (server, options, next) { errObj.slugError = defaults.errors.slug; break; default: - const idx = valErr.path.split('.')[1]; - switch (valErr.context.key) { - case 'title': - request.payload.test[idx].codeTitleError = defaults.errors.codeTitle; - break; - case 'code': - request.payload.test[idx].codeError = defaults.errors.code; - break; - default: - throw new Error('unknown validation error'); - } + throw new Error('unknown validation error'); } } catch (ex) { errObj.genError = defaults.errors.general; } errResp(errObj); } else { - // Joi defaults any properties not present in `request.payload` so use `payload` from here on out - var payload = pageWithTests; + // additional validation that's not possible or insanely complex w/ Joi + let wasAdditionalValidationError = false; + pageWithTests.test.forEach((t, idx) => { + const missingTitle = t.title === defaults.deleteMe; + const missingCode = t.code === defaults.deleteMe; + + if (missingTitle && !missingCode) { + wasAdditionalValidationError = true; + request.payload.test[idx].codeTitleError = defaults.errors.codeTitle; + } + + if (missingCode && !missingTitle) { + wasAdditionalValidationError = true; + request.payload.test[idx].codeError = defaults.errors.code; + } + }); + + if (wasAdditionalValidationError) { + errResp(errObj); + } else { + // Joi defaults any properties not present in `request.payload` so use `payload` from here on out + var payload = pageWithTests; - pagesService.checkIfSlugAvailable(server, payload.slug) - .then(isAvail => { - if (!isAvail) { - errResp({ - slugError: defaults.errors.slugDupe - }); - } else { - return pagesService.create(payload) - .then(resultPageId => { - const own = request.yar.get('own') || {}; - own[resultPageId] = true; - request.yar.set('own', own); - request.yar.set('authorSlug', payload.author.replace(' ', '-').replace(/[^a-zA-Z0-9 -]/, '')); - reply.redirect('/' + payload.slug); + pagesService.checkIfSlugAvailable(server, payload.slug) + .then(isAvail => { + if (!isAvail) { + errResp({ + slugError: defaults.errors.slugDupe }); - } - }) - .catch(errResp); + } else { + return pagesService.create(payload) + .then(resultPageId => { + const own = request.yar.get('own') || {}; + own[resultPageId] = true; + request.yar.set('own', own); + request.yar.set('authorSlug', payload.author.replace(' ', '-').replace(/[^a-zA-Z0-9 -]/, '')); + reply.redirect('/' + payload.slug); + }); + } + }) + .catch(errResp); + } } }); } diff --git a/test/unit/server/lib/schema.js b/test/unit/server/lib/schema.js index 3a4ad13c..695fdf76 100644 --- a/test/unit/server/lib/schema.js +++ b/test/unit/server/lib/schema.js @@ -1,5 +1,6 @@ var Lab = require('lab'); var Code = require('code'); +const Joi = require('joi'); const schema = require('../../../../server/lib/schema'); var lab = exports.lab = Lab.script(); @@ -21,6 +22,119 @@ lab.experiment('Schema Library', function () { done(); }); + + lab.experiment('test array', () => { + let value; + + lab.beforeEach((done) => { + value = { + author: 'Pitcher Man', + authorEmail: 'kool-aid@kraft.com', + authorURL: 'http://kool-aid.com', + title: 'oh', + slug: 'wee', + info: '', + initHTML: '', + setup: '', + teardown: '' + // `test` intentionally missing. will be set in each test + }; + + done(); + }); + + lab.test('title with code is valid', (done) => { + value.test = [ + { + title: 't1', + code: 't=1' + }, + { + title: 't2', + code: 't=2' + } + ]; + + Joi.validate(value, schema.testPage, (err) => { + Code.expect(err).to.not.exist(); + + done(); + }); + }); + + lab.test('blank title with blank code is valid', (done) => { + value.test = [ + { + title: 't1', + code: 't=1' + }, + { + title: '', // intentionally blank + code: '' // intentionally blank + }, + { + title: 't3', + code: 't=3' + } + ]; + + Joi.validate(value, schema.testPage, (err, res) => { + Code.expect(err).to.not.exist(); + Code.expect(res.test[1].title).to.equal('GENERATED_DEFAULT_DELETE_ME'); + Code.expect(res.test[1].code).to.equal('GENERATED_DEFAULT_DELETE_ME'); + + done(); + }); + }); + + lab.test('blank title with code is invalid', (done) => { + value.test = [ + { + title: 't1', + code: 't=1' + }, + { + title: 't2', + code: 't=2' + }, + { + title: '', // intentionally blank + code: 't=3' + } + ]; + + Joi.validate(value, schema.testPage, (err, res) => { + Code.expect(err).to.not.exist(); + Code.expect(res.test[2].title).to.equal('GENERATED_DEFAULT_DELETE_ME'); + + done(); + }); + }); + + lab.test('title with blank code is invalid', (done) => { + value.test = [ + { + title: 't1', + code: 't=1' + }, + { + title: 't2', + code: 't=2' + }, + { + title: 't3', + code: '' // intentionally blank + } + ]; + + Joi.validate(value, schema.testPage, (err, res) => { + Code.expect(err).to.not.exist(); + Code.expect(res.test[2].code).to.equal('GENERATED_DEFAULT_DELETE_ME'); + + done(); + }); + }); + }); }); lab.experiment('comment', function () { diff --git a/test/unit/server/repositories/tests.js b/test/unit/server/repositories/tests.js index f603772d..01f5246d 100644 --- a/test/unit/server/repositories/tests.js +++ b/test/unit/server/repositories/tests.js @@ -3,6 +3,7 @@ const Code = require('code'); const Hapi = require('hapi'); const sinon = require('sinon'); const Hoek = require('hoek'); +const defaults = require('../../../../server/lib/defaults'); const MockDb = { register: (server, options, next) => { @@ -190,10 +191,10 @@ lab.experiment('Tests Repository', function () { let tClone = Hoek.clone(t); tClone[0].testID = 123; tClone[1].testID = 321; - delete tClone[0].code; - delete tClone[0].title; - delete tClone[1].code; - delete tClone[1].title; + tClone[0].code = defaults.deleteMe; + tClone[0].title = defaults.deleteMe; + tClone[1].code = defaults.deleteMe; + tClone[1].title = defaults.deleteMe; tests.bulkUpdate(pageID, tClone, true) .then(results => { @@ -212,10 +213,10 @@ lab.experiment('Tests Repository', function () { lab.test('does nothing if no title and no code with no test id', function (done) { genericQueryStub.returns(Promise.resolve({ affectedRows: 1 })); let tClone = Hoek.clone(t); - delete tClone[0].code; - delete tClone[0].title; - delete tClone[1].code; - delete tClone[1].title; + tClone[0].code = defaults.deleteMe; + tClone[0].title = defaults.deleteMe; + tClone[1].code = defaults.deleteMe; + tClone[1].title = defaults.deleteMe; tests.bulkUpdate(pageID, tClone, true) .then(results => { @@ -232,10 +233,10 @@ lab.experiment('Tests Repository', function () { let tClone = Hoek.clone(t); tClone[0].testID = 123; tClone[1].testID = 321; - delete tClone[0].code; - delete tClone[0].title; - delete tClone[1].code; - delete tClone[1].title; + tClone[0].code = defaults.deleteMe; + tClone[0].title = defaults.deleteMe; + tClone[1].code = defaults.deleteMe; + tClone[1].title = defaults.deleteMe; tests.bulkUpdate(pageID, tClone, false) .then(results => { diff --git a/test/unit/server/web/edit/index.js b/test/unit/server/web/edit/index.js index 7f77564e..065847f4 100644 --- a/test/unit/server/web/edit/index.js +++ b/test/unit/server/web/edit/index.js @@ -418,8 +418,10 @@ lab.experiment('POST', function () { }); }); - lab.test('test title required', function (done) { - delete request.payload.test[0].title; + lab.test('test title required if code present', function (done) { + // code present in defaults above + Code.expect(request.payload.test[0].code).to.not.be.empty(); + request.payload.test[0].title = ''; server.inject(request, function (response) { Code.expect(response.statusCode).to.equal(400); @@ -430,8 +432,10 @@ lab.experiment('POST', function () { }); }); - lab.test('test code required', function (done) { - delete request.payload.test[0].code; + lab.test('test code required if title present', function (done) { + // title present in defaults above + Code.expect(request.payload.test[0].title).to.not.be.empty(); + request.payload.test[0].code = ''; server.inject(request, function (response) { Code.expect(response.statusCode).to.equal(400); @@ -559,6 +563,34 @@ lab.experiment('POST', function () { }); }); + lab.test('calls edit page service with update params to delete test', function (done) { + pagesServiceStub.edit.onCall(0).returns(Promise.resolve(request.payload)); + + server.inject('/setsession', function (res) { + var header = res.headers['set-cookie']; + var cookie = header[0].match(/(?:[^\x00-\x20\(\)<>@\,;\:\\'\/\[\]\?\=\{\}\x7F]+)\s*=\s*(?:([^\x00-\x20\'\,\;\\\x7F]*))/); // eslint-disable-line no-control-regex, no-useless-escape + + request.headers = {}; + request.headers.cookie = 'session=' + cookie[1]; + + // add extra blank test to simulate deleting a code snippet + request.payload.test.push({ + title: '', + code: '' + }); + + server.inject(request, response => { + let firstCallArgs = pagesServiceStub.edit.args[0]; + + Code.expect(firstCallArgs[0].slug).to.equal(request.payload.slug); + Code.expect(firstCallArgs[1]).to.equal(true); + Code.expect(firstCallArgs[2]).to.equal(22); + + done(); + }); + }); + }); + lab.test('redirects to original url after update', function (done) { const rev = 1; pagesServiceStub.edit.onCall(0).returns(Promise.resolve(rev)); diff --git a/test/unit/server/web/home/index.js b/test/unit/server/web/home/index.js index 23881504..8df3bda3 100644 --- a/test/unit/server/web/home/index.js +++ b/test/unit/server/web/home/index.js @@ -3,7 +3,7 @@ const Lab = require('lab'); const Code = require('code'); const Hapi = require('hapi'); const sinon = require('sinon'); - +const defaults = require('../../../../../server/lib/defaults'); const HomePlugin = require('../../../../../server/web/home/index'); const YarPlugin = { @@ -169,25 +169,29 @@ lab.experiment('home', function () { }); }); - lab.test('test title required', function (done) { - delete request.payload.test[0].title; + lab.test('test title required if code present', function (done) { + // code present in defaults above + Code.expect(request.payload.test[0].code).to.not.be.empty(); + request.payload.test[0].title = ''; server.inject(request, function (response) { Code.expect(response.statusCode).to.equal(400); - Code.expect(response.result).to.include('Please enter a title for this code snippet.'); + Code.expect(response.result).to.include(defaults.errors.codeTitle); done(); }); }); - lab.test('test code required', function (done) { - delete request.payload.test[0].code; + lab.test('test code required if title present', function (done) { + // title present in defaults above + Code.expect(request.payload.test[0].title).to.not.be.empty(); + request.payload.test[0].code = ''; server.inject(request, function (response) { Code.expect(response.statusCode).to.equal(400); - Code.expect(response.result).to.include('Please enter a code snippet.'); + Code.expect(response.result).to.include(defaults.errors.code); done(); });