From c547f089df7fd0965e844ac2a6c913e4edb58db8 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Mon, 13 Aug 2018 10:03:50 -0700 Subject: [PATCH] Ensure error changes are added to changes block (#303) --- README.md | 64 ++++++++++++++++++++ addon/index.js | 40 +++++++++---- addon/utils/computed/object-to-array.js | 7 ++- tests/unit/changeset-test.js | 80 +++++++++++++++++++++---- 4 files changed, 164 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index 59587cef..114a0bf1 100644 --- a/README.md +++ b/README.md @@ -850,6 +850,70 @@ if (isChangeset(model)) { - [`ember-changeset-hofs`](https://github.com/nucleartide/ember-changeset-hofs) - Higher-order validation functions - [`ember-bootstrap-changeset-validations`](https://github.com/kaliber5/ember-bootstrap-changeset-validations) - Adds support for changeset validations to `ember-bootstrap` +## Tips and Tricks + +- General Input Helper with ember-concurrency + +```js +export default Component.extend({ + classNameBindings: ['hasError:validated-input--error'], + + _checkValidity: task(function* (changeset, valuePath, value) { + yield timeout(150); + + let snapshot = changeset.snapshot(); + + // valuePath is the property on the changeset, e.g. firstName + set(changeset, valuePath, value); + + if (!changeset.get(`error.${valuePath}`)) { + set(this, 'hasError', false); + } else { + // if error, restore changeset so don't show error in template immediately' + // i.e. wait for onblur action to validate and show error in template + changeset.restore(snapshot); + } + }).restartable(), + + actions: { + /** + * @method validateProperty + * @param {Object} changeset + * @param {String} valuePath + * @param {Object} e + */ + validateProperty(changeset, valuePath, e) { + set(changeset, valuePath, e.target.value); + + if (changeset.get(`error.${valuePath}`)) { + set(this, 'hasError', true); + } else { + set(this, 'hasError', false); + } + }, + + /** + * @method checkValidity + * @param {Object} changeset + * @param {String|Integer} value + */ + checkValidity(changeset, value) { + get(this, '_checkValidity').perform(changeset, this.valuePath, value); + } + } +}); +``` + +```hbs + +``` + ## Installation * `git clone` this repository diff --git a/addon/index.js b/addon/index.js index 678fda3c..f7921bb1 100644 --- a/addon/index.js +++ b/addon/index.js @@ -411,16 +411,26 @@ export function changeset( * * @public * @chainable - * @param {String} key optional key to rollback invalid + * @param {String} key optional key to rollback invalid values * @return {Changeset} */ rollbackInvalid(key /*: string | void */) /*: ChangesetDef */ { + let errorKeys = keys(get(this, ERRORS)); + if (key) { this._notifyVirtualProperties([key]); this._deleteKey(ERRORS, key); + if (errorKeys.indexOf(key) > -1) { + this._deleteKey(CHANGES, key); + } } else { this._notifyVirtualProperties(); set(this, ERRORS, {}); + + // if on CHANGES hash, rollback those as well + errorKeys.forEach((errKey) => { + this._deleteKey(CHANGES, errKey); + }) } return this; @@ -492,7 +502,6 @@ export function changeset( // Remove `key` from changes map. let c = (this /*: ChangesetDef */); - c._deleteKey(CHANGES, key); // Add `key` to errors map. let errors /*: Errors */ = get(this, ERRORS); @@ -626,10 +635,14 @@ export function changeset( let validation /*: ValidationResult | Promise */ = c._validate(key, value, oldValue); + let v /*: ValidationResult */ = (validation /*: any */); + + c.trigger(BEFORE_VALIDATION_EVENT, key); + let result = c._setProperty(v, { key, value, oldValue }); + // TODO: Address case when Promise is rejected. if (isPromise(validation)) { c._setIsValidating(key, true); - c.trigger(BEFORE_VALIDATION_EVENT, key); let v /*: Promise */ = (validation /*: any */); return v.then(resolvedValidation => { @@ -639,10 +652,9 @@ export function changeset( }); } - c.trigger(BEFORE_VALIDATION_EVENT, key); c.trigger(AFTER_VALIDATION_EVENT, key); - let v /*: ValidationResult */ = (validation /*: any */); - return c._setProperty(v, { key, value, oldValue }); + + return result; }, /** @@ -689,12 +701,6 @@ export function changeset( // Shorthand for `this`. let c /*: ChangesetDef */ = this; - // Error case. - if (!isValid) { - let v /*: ValidationErr */ = (validation /*: any */); - return c.addError(key, { value, validation: v }); - } - // Happy path: remove `key` from error map. c._deleteKey(ERRORS, key); @@ -715,6 +721,12 @@ export function changeset( c.notifyPropertyChange(CHANGES); c.notifyPropertyChange(key); + // Error case. + if (!isValid) { + let v /*: ValidationErr */ = (validation /*: any */); + return c.addError(key, { value, validation: v }); + } + // Return new value. return value; }, @@ -828,7 +840,9 @@ export function changeset( key /*: string */ = '' ) /*: void */ { let obj /*: InternalMap */ = get(this, objName); - if (obj.hasOwnProperty(key)) delete obj[key]; + if (obj.hasOwnProperty(key)) { + delete obj[key]; + } let c /*: ChangesetDef */ = this; c.notifyPropertyChange(`${objName}.${key}`); c.notifyPropertyChange(objName); diff --git a/addon/utils/computed/object-to-array.js b/addon/utils/computed/object-to-array.js index ba24574f..d4020719 100644 --- a/addon/utils/computed/object-to-array.js +++ b/addon/utils/computed/object-to-array.js @@ -1,8 +1,11 @@ // @flow -import Ember from 'ember'; import { computed, get } from '@ember/object'; import { typeOf } from '@ember/utils'; +import { assign as EmberAssign } from '@ember/polyfills'; +import { merge } from '@ember/polyfills' + +const assign = EmberAssign || merge; /*:: import type Change from 'ember-changeset/-private/change'; @@ -10,8 +13,6 @@ import type Err from 'ember-changeset/-private/err'; */ const { keys } = Object; -// eslint-disable-next-line ember/new-module-imports -const assign = Ember.assign || Ember.merge; /** * Compute the array form of an object. diff --git a/tests/unit/changeset-test.js b/tests/unit/changeset-test.js index 83ebda86..b2b50b3e 100644 --- a/tests/unit/changeset-test.js +++ b/tests/unit/changeset-test.js @@ -74,12 +74,13 @@ test('content can be an empty hash', function(assert) { * #error */ -test('#error returns the error object', function(assert) { +test('#error returns the error object and keeps changes', function(assert) { let dummyChangeset = new Changeset(dummyModel, dummyValidator); let expectedResult = { name: { validation: 'too short', value: 'a' } }; dummyChangeset.set('name', 'a'); assert.deepEqual(get(dummyChangeset, 'error'), expectedResult, 'should return error object'); + assert.deepEqual(get(dummyChangeset, 'change'), { name: 'a' }, 'should return error object'); }); /** @@ -400,7 +401,7 @@ test('#set removes a change if set back to original value when obj is ProxyObjec ); }); -test('#set does not add a change if invalid', function(assert) { +test('#set does add a change if invalid', function(assert) { let expectedErrors = [ { key: 'name', validation: 'too short', value: 'a' }, { key: 'password', validation: ['foo', 'bar'], value: false } @@ -413,7 +414,8 @@ test('#set does not add a change if invalid', function(assert) { let isValid = get(dummyChangeset, 'isValid'); let isInvalid = get(dummyChangeset, 'isInvalid'); - assert.deepEqual(changes, [], 'should not add change'); + let expectedChanges = [{ "key": "name", "value": "a" }, { "key": "password", "value": false }]; + assert.deepEqual(changes, expectedChanges, 'should add change'); assert.deepEqual(errors, expectedErrors, 'should have errors'); assert.notOk(isValid, 'should not be valid'); assert.ok(isInvalid, 'should be invalid'); @@ -679,6 +681,7 @@ test('#save proxies to content', function(assert) { assert.equal(result, undefined, 'precondition'); let promise = dummyChangeset.save('test options'); assert.equal(result, 'ok', 'should save'); + assert.deepEqual(get(dummyChangeset, 'change'), { name: 'foo' }, 'should save'); assert.equal(options, 'test options', 'should proxy options when saving'); assert.ok(!!promise && typeof promise.then === 'function', 'save returns a promise'); promise.then((saveResult) => { @@ -769,7 +772,7 @@ test('#merge merges invalid changesets', function(assert) { let dummyChangesetD = dummyChangesetA.merge(dummyChangesetB); dummyChangesetD = dummyChangesetD.merge(dummyChangesetC); - let expectedChanges = [{ key: 'age', value: 21 }]; + let expectedChanges = [{ key: 'age', value: 21 }, { key: 'name', value: 'b'}]; let expectedErrors = [{ key: 'name', 'validation': 'too short', value: 'b' }]; assert.deepEqual(get(dummyChangesetA, 'isInvalid'), true, 'changesetA is not valid becuase of name'); @@ -834,7 +837,8 @@ test('#rollback restores old values', function(assert) { let dummyChangeset = new Changeset(dummyModel, dummyValidator); let expectedChanges = [ { key: 'firstName', value: 'foo' }, - { key: 'lastName', value: 'bar' } + { key: 'lastName', value: 'bar' }, + { key: 'name', value: '' } ]; let expectedErrors = [{ key: 'name', validation: 'too short', value: '' }]; dummyChangeset.set('firstName', 'foo'); @@ -861,7 +865,8 @@ test('#rollbackInvalid clears errors and keeps valid values', function(assert) { let dummyChangeset = new Changeset(dummyModel, dummyValidator); let expectedChanges = [ { key: 'firstName', value: 'foo' }, - { key: 'lastName', value: 'bar' } + { key: 'lastName', value: 'bar' }, + { key: 'name', value: '' } ]; let expectedErrors = [{ key: 'name', validation: 'too short', value: '' }]; dummyChangeset.set('firstName', 'foo'); @@ -871,6 +876,10 @@ test('#rollbackInvalid clears errors and keeps valid values', function(assert) { assert.deepEqual(get(dummyChangeset, 'changes'), expectedChanges, 'precondition'); assert.deepEqual(get(dummyChangeset, 'errors'), expectedErrors, 'precondition'); dummyChangeset.rollbackInvalid(); + expectedChanges = [ + { key: 'firstName', value: 'foo' }, + { key: 'lastName', value: 'bar' } + ]; assert.deepEqual(get(dummyChangeset, 'changes'), expectedChanges, 'should not rollback'); assert.deepEqual(get(dummyChangeset, 'errors'), [], 'should rollback'); }); @@ -879,7 +888,9 @@ test('#rollbackInvalid a specific key clears key error and keeps valid values', let dummyChangeset = new Changeset(dummyModel, dummyValidator); let expectedChanges = [ { key: 'firstName', value: 'foo' }, - { key: 'lastName', value: 'bar' } + { key: 'lastName', value: 'bar' }, + { key: 'password', value: false }, + { key: 'name', value: '' } ]; let expectedErrors = [ { key: 'password', validation: ['foo', 'bar'], value: false }, @@ -893,6 +904,11 @@ test('#rollbackInvalid a specific key clears key error and keeps valid values', assert.deepEqual(get(dummyChangeset, 'changes'), expectedChanges, 'precondition'); assert.deepEqual(get(dummyChangeset, 'errors'), expectedErrors, 'precondition'); dummyChangeset.rollbackInvalid('name'); + expectedChanges = [ + { key: 'firstName', value: 'foo' }, + { key: 'lastName', value: 'bar' }, + { key: 'password', value: false } + ]; assert.deepEqual(get(dummyChangeset, 'changes'), expectedChanges, 'should not rollback'); expectedErrors = [ { key: 'password', validation: ['foo', 'bar'], value: false } @@ -909,6 +925,40 @@ test('#rollbackInvalid resets valid state', function(assert) { assert.ok(get(dummyChangeset, 'isValid'), 'should be valid'); }); +test('#rollbackInvalid will not remove changes that are valid', function(assert) { + let dummyChangeset = new Changeset(dummyModel, dummyValidator); + dummyChangeset.set('name', 'abcd'); + + let expectedChanges = [ + { key: 'name', value: 'abcd' }, + ]; + assert.deepEqual(get(dummyChangeset, 'changes'), expectedChanges, 'has correct changes'); + assert.ok(get(dummyChangeset, 'isValid'), 'should be valid'); + dummyChangeset.rollbackInvalid('name'); + assert.deepEqual(get(dummyChangeset, 'changes'), expectedChanges, 'should not remove valid changes'); + assert.ok(get(dummyChangeset, 'isValid'), 'should still be valid'); +}); + + +test('#rollbackInvalid works for keys not on changeset', function(assert) { + let dummyChangeset = new Changeset(dummyModel, dummyValidator); + let expectedChanges = [ + { key: 'firstName', value: 'foo' }, + { key: 'lastName', value: 'bar' }, + { key: 'name', value: '' } + ]; + let expectedErrors = [{ key: 'name', validation: 'too short', value: '' }]; + dummyChangeset.set('firstName', 'foo'); + dummyChangeset.set('lastName', 'bar'); + dummyChangeset.set('name', ''); + + assert.deepEqual(get(dummyChangeset, 'changes'), expectedChanges, 'precondition'); + assert.deepEqual(get(dummyChangeset, 'errors'), expectedErrors, 'precondition'); + dummyChangeset.rollbackInvalid('dowat?'); + assert.deepEqual(get(dummyChangeset, 'changes'), expectedChanges, 'should not rollback'); + assert.deepEqual(get(dummyChangeset, 'errors'), expectedErrors, 'precondition'); +}); + test('#rollbackProperty restores old value for specified property only', function(assert) { set(dummyModel, 'firstName', 'Jim'); set(dummyModel, 'lastName', 'Bob'); @@ -927,7 +977,8 @@ test('#rollbackProperty clears errors for specified property', function(assert) let dummyChangeset = new Changeset(dummyModel, dummyValidator); let expectedChanges = [ { key: 'firstName', value: 'foo' }, - { key: 'lastName', value: 'bar' } + { key: 'lastName', value: 'bar' }, + { key: 'name', value: '' } ]; let expectedErrors = [{ key: 'name', validation: 'too short', value: '' }]; dummyChangeset.set('firstName', 'foo'); @@ -937,6 +988,10 @@ test('#rollbackProperty clears errors for specified property', function(assert) assert.deepEqual(get(dummyChangeset, 'changes'), expectedChanges, 'precondition'); assert.deepEqual(get(dummyChangeset, 'errors'), expectedErrors, 'precondition'); dummyChangeset.rollbackProperty('name'); + expectedChanges = [ + { key: 'firstName', value: 'foo' }, + { key: 'lastName', value: 'bar' } + ]; assert.deepEqual(get(dummyChangeset, 'changes'), expectedChanges, 'should not rollback'); assert.deepEqual(get(dummyChangeset, 'errors'), [], 'should rollback'); }); @@ -1086,7 +1141,7 @@ test('#validate marks actual valid changes', function(assert) { run(() => { dummyChangeset.validate().then(() => { - assert.deepEqual(get(dummyChangeset, 'changes'), [{ key: 'name', value: 'foo bar' }]); + assert.deepEqual(get(dummyChangeset, 'changes'), [{ key: 'name', value: 'foo bar' }, { key: 'password', value: false }]); done(); }); }); @@ -1157,7 +1212,7 @@ test('#addError adds an error to the changeset using the shortcut', function (as assert.ok(get(dummyChangeset, 'isInvalid'), 'should be invalid'); assert.equal(get(dummyChangeset, 'error.email.validation'), 'Email already taken', 'should add the error'); assert.equal(get(dummyChangeset, 'error.email.value'), 'jim@bob.com', 'addError uses already present value'); - assert.deepEqual(get(dummyChangeset, 'changes'), [], 'pushErrors clears the changes on the changeset'); + assert.deepEqual(get(dummyChangeset, 'changes'), [{ key: 'email', value: 'jim@bob.com'}], 'errors set as changes on changeset'); dummyChangeset.set('email', 'unique@email.com'); assert.ok(get(dummyChangeset, 'isValid'), 'should be valid'); assert.deepEqual(get(dummyChangeset, 'changes')[0], { key: 'email', value: 'unique@email.com' }, 'has correct changes'); @@ -1204,7 +1259,7 @@ test('#snapshot creates a snapshot of the changeset', function(assert) { dummyChangeset.set('password', false); let snapshot = dummyChangeset.snapshot(); let expectedResult = { - changes: { name: 'Pokemon Go' }, + changes: { name: 'Pokemon Go', password: false }, errors: { password: { validation: ['foo', 'bar'], value: false } } }; @@ -1281,8 +1336,11 @@ test('isValidating returns true when validations have not resolved', function(as set(dummyModel, 'reservations', 'ABC12345'); dummyChangeset = new Changeset(dummyModel, _validator, _validations); + set(dummyChangeset, 'reservations', 'DCE12345'); dummyChangeset.validate(); + assert.deepEqual(get(dummyChangeset, 'change'), { reservations: 'DCE12345' }); + assert.ok(dummyChangeset.isValidating(), 'isValidating should be true when no key is passed in and something is validating'); assert.ok(dummyChangeset.isValidating('reservations'),