Repository: jsperf/jsperf.com (js)

Issue:
Allow a code snippet to be removed
Currently, there is no UI element to remove a code snippet when creating or editing a test. A button that removes the DOM elements should do the trick.

Pull request:
allow removing of code snippet
closes #309 

This is the _easier_ approach to allowing a code snippet to be removed although it would be nice to have a button or something. All of the server logic is already in place to deal with a deletion this way.

Patch:
diff --git a/server/lib/defaults.js b/server/lib/defaults.js
index fe67905..e6db37d 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 224defb..da1fb91 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/web/edit/index.hbs b/server/web/edit/index.hbs
index 3fddd0b..a67d912 100644
--- a/server/web/edit/index.hbs
+++ b/server/web/edit/index.hbs
@@ -149,6 +149,11 @@ This edit will create a new revision.
       </fieldset>
       {{/each}}
     </fieldset>
+
+    <p>
+      <em>NB: leave <code>Title</code> and <code>Code</code> blank to remove code snippet.</em>
+    </p>
+
     <div class="buttons">
       <input type="submit" class="submit" value="Save test case" title="Save and view test case">
     </div>
diff --git a/server/web/edit/index.js b/server/web/edit/index.js
index b8a5ccf..19320d2 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 3a6dd63..9977350 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);
+          }
         }
       });
     }


Test patch:
diff --git a/server/repositories/tests.js b/server/repositories/tests.js
index 856dd85..c3dd1b6 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/test/unit/server/lib/schema.js b/test/unit/server/lib/schema.js
index 3a4ad13..695fdf7 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 f603772..01f5246 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 7f77564..065847f 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 2388150..8df3bda 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();
         });