From 8f5461fc9ee080ebc1f7befe713cc86ccd96fd39 Mon Sep 17 00:00:00 2001 From: Christian Crowhurst Date: Wed, 21 Dec 2016 20:51:25 +0000 Subject: [PATCH] feat(NgTableParams): nested paramater values never undefined This is part of supporting typescript strictNullChecks compiler option. BREAKING CHANGES `NgTableParams.parameters`: any param value supplied as undefined will now be ignored Instead of `undefined` you will need to supply a suitable value as described below: * `count`, `page` - `0` * `filter`, `group`, `sorting` - an empty object to remove existing values --- src/core/ngTableDefaults.ts | 4 +- src/core/ngTableParams.ts | 90 ++++++------ test/specs/settings.spec.ts | 4 +- test/specs/table.spec.ts | 6 +- test/specs/tableParams.spec.ts | 246 +++++++++++++++++++++++++++------ 5 files changed, 259 insertions(+), 91 deletions(-) diff --git a/src/core/ngTableDefaults.ts b/src/core/ngTableDefaults.ts index 74817d02..34dd9cec 100644 --- a/src/core/ngTableDefaults.ts +++ b/src/core/ngTableDefaults.ts @@ -6,7 +6,7 @@ * @license New BSD License */ -import { ParamValues } from './ngTableParams'; +import { ParamValuesPartial } from './ngTableParams'; import { SettingsPartial } from './ngTableSettings'; @@ -15,7 +15,7 @@ import { SettingsPartial } from './ngTableSettings'; * an instance of `NgTableParams` */ export interface Defaults { - params?: ParamValues; + params?: ParamValuesPartial; settings?: SettingsPartial } diff --git a/src/core/ngTableParams.ts b/src/core/ngTableParams.ts index 224d01e1..a7721f0c 100644 --- a/src/core/ngTableParams.ts +++ b/src/core/ngTableParams.ts @@ -9,6 +9,7 @@ import * as ng1 from 'angular'; import { ILogService, IPromise, IQService } from 'angular'; import { convertSortToOrderBy, isGroupingFun } from './util'; +import { assignPartialDeep } from '../shared'; import { Defaults } from './ngTableDefaults' import { NgTableEventsChannel } from './ngTableEventsChannel' import { SettingsPartial, Settings } from './ngTableSettings' @@ -25,31 +26,34 @@ export interface InternalTableParams extends NgTableParams { isNullInstance: boolean } + +export type ParamValuesPartial = Partial>; + /** * The runtime values for {@link NgTableParams} that determine the set of data rows and * how they are to be displayed in a table */ -export interface ParamValues { +export class ParamValues { /** * The index of the "slice" of data rows, starting at 1, to be displayed by the table. */ - page?: number; + page = 1; /** * The number of data rows per page */ - count?: number; + count = 10; /** * The filter that should be applied to restrict the set of data rows */ - filter?: FilterValues; + filter: FilterValues = {}; /** * The sort order that should be applied to the data rows. */ - sorting?: SortingValues; + sorting: SortingValues = {}; /** * The grouping that should be applied to the data rows */ - group?: string | Grouping; + group: string | Grouping = {}; } @@ -85,17 +89,11 @@ export class NgTableParams { private ngTableDefaults: Defaults private ngTableEventsChannel: NgTableEventsChannel; private prevParamsMemento: Memento; - private _params: ParamValues = { - page: 1, - count: 10, - filter: {}, - sorting: {}, - group: {} - }; + private _params = new ParamValues(); private _settings = this.defaultSettings; private $q: IQService; private $log: ILogService - constructor(baseParameters?: ParamValues | boolean, baseSettings?: SettingsPartial) { + constructor(baseParameters?: ParamValuesPartial | boolean, baseSettings?: SettingsPartial) { // the ngTableController "needs" to create a dummy/null instance and it's important to know whether an instance // is one of these @@ -115,7 +113,7 @@ export class NgTableParams { } })(); - ng1.extend(this._params, this.ngTableDefaults.params); + assignPartialDeep(this._params, this.ngTableDefaults.params); this.settings(baseSettings); this.parameters(baseParameters, true); @@ -268,7 +266,7 @@ export class NgTableParams { return this._params.group; } - const newParameters: ParamValues = { + const newParameters: ParamValuesPartial = { page: 1 }; if (isGroupingFun(group) && sortDirection !== undefined) { @@ -382,39 +380,45 @@ export class NgTableParams { /** * Set new parameters */ - parameters(newParameters?: ParamValues | { [name: string]: string }, parseParamsFromUrl?: boolean): this - parameters(newParameters?: ParamValues | { [name: string]: string }, parseParamsFromUrl?: boolean): ParamValues | this { + parameters(newParameters?: ParamValuesPartial | { [name: string]: string }, parseParamsFromUrl?: boolean): this + parameters(newParameters?: ParamValuesPartial | { [name: string]: string }, parseParamsFromUrl?: boolean): ParamValues | this { + if (newParameters === undefined) { + return this._params; + } + + // todo: move parsing of url like parameters into a seperate method + parseParamsFromUrl = parseParamsFromUrl || false; - if (newParameters !== undefined) { - for (const key in newParameters) { - let value = newParameters[key]; - if (parseParamsFromUrl && key.indexOf('[') >= 0) { - const keys = key.split(/\[(.*)\]/).reverse() - let lastKey = ''; - for (let i = 0, len = keys.length; i < len; i++) { - const name = keys[i]; - if (name !== '') { - const v = value; - value = {}; - value[lastKey = name] = (isNumber(v) ? parseFloat(v) : v); - } - } - if (lastKey === 'sorting') { - this._params[lastKey] = {}; + for (const key in newParameters) { + let value = newParameters[key]; + if (parseParamsFromUrl && key.indexOf('[') >= 0) { + const keys = key.split(/\[(.*)\]/).reverse() + let lastKey = ''; + for (let i = 0, len = keys.length; i < len; i++) { + const name = keys[i]; + if (name !== '') { + const v = value; + value = {}; + value[lastKey = name] = (isNumber(v) ? parseFloat(v) : v); } - this._params[lastKey] = ng1.extend(this._params[lastKey] || {}, value[lastKey]); + } + if (lastKey === 'sorting') { + this._params[lastKey] = {}; + } + this._params[lastKey] = ng1.extend(this._params[lastKey] || {}, value[lastKey]); + } else { + if (newParameters[key] === undefined) { + // skip + } + else if (key === 'group') { + this._params[key] = this.parseGroup(newParameters[key]); } else { - if (key === 'group') { - this._params[key] = this.parseGroup(newParameters[key]); - } else { - this._params[key] = (isNumber(newParameters[key]) ? parseFloat(newParameters[key]) : newParameters[key]); - } + this._params[key] = (isNumber(newParameters[key]) ? parseFloat(newParameters[key]) : newParameters[key]); } } - this.log('ngTable: set parameters', this._params); - return this; } - return this._params; + this.log('ngTable: set parameters', this._params); + return this; } /** * Trigger a reload of the data rows diff --git a/test/specs/settings.spec.ts b/test/specs/settings.spec.ts index 00f39e74..f22171ac 100644 --- a/test/specs/settings.spec.ts +++ b/test/specs/settings.spec.ts @@ -12,7 +12,7 @@ describe('Settings', () => { beforeAll(() => expect(ngTableCoreModule).toBeDefined()); let defaultOverrides: SettingsPartial; - let standardSettings: SettingsPartial; + let standardSettings: Settings; beforeEach(ng1.mock.module('ngTable-core')); @@ -103,7 +103,7 @@ describe('Settings', () => { filterDelay: 20 } }; - const originalNewSettings = { ...newSettings, filterOptions: { ... newSettings.filterOptions } }; + const originalNewSettings = { ...newSettings, filterOptions: { ...newSettings.filterOptions } }; const actual = Settings.merge(allSettings, newSettings); expect(actual).not.toBe(allSettings); diff --git a/test/specs/table.spec.ts b/test/specs/table.spec.ts index aeb68413..5b9169a3 100644 --- a/test/specs/table.spec.ts +++ b/test/specs/table.spec.ts @@ -1,7 +1,7 @@ import { IAugmentedJQuery, ICompileService, IQService, IPromise, IScope, ITimeoutService } from 'angular'; import * as ng1 from 'angular'; import { ngTableModule } from '../../index'; -import { NgTableParams, ParamValues, SettingsPartial, SortingValues } from '../../src/core'; +import { NgTableParams, ParamValuesPartial, SettingsPartial, SortingValues } from '../../src/core'; import { ColumnDef, FilterTemplateDef, FilterTemplateDefMap, SelectOption } from '../../src/browser' describe('ng-table', () => { @@ -72,10 +72,10 @@ describe('ng-table', () => { scope.model = {}; })); - function createNgTableParams(initialParams?: ParamValues, settings?: SettingsPartial): NgTableParams; + function createNgTableParams(initialParams?: ParamValuesPartial, settings?: SettingsPartial): NgTableParams; function createNgTableParams(settings?: SettingsPartial): NgTableParams; function createNgTableParams(settings?: any): NgTableParams { - let initialParams: ParamValues; + let initialParams: ParamValuesPartial; if (arguments.length === 2) { initialParams = arguments[0]; settings = arguments[1]; diff --git a/test/specs/tableParams.spec.ts b/test/specs/tableParams.spec.ts index eddec5cc..ff1bd5d2 100644 --- a/test/specs/tableParams.spec.ts +++ b/test/specs/tableParams.spec.ts @@ -2,8 +2,8 @@ import { IControllerService, IQService, IScope } from 'angular'; import * as ng1 from 'angular'; import * as _ from 'lodash'; import { - DataRowGroup, DataSettings, DefaultGetData, Defaults, NgTableEventsChannel, FilterSettings, - GroupingFunc, GroupSettings, GroupValues, InternalTableParams, NgTableParams, PageButton, ParamValues, SettingsPartial, + DataRowGroup, DataSettings, DefaultGetData, Defaults, NgTableEventsChannel, FilterSettings, + GroupingFunc, GroupSettings, GroupValues, InternalTableParams, NgTableParams, PageButton, ParamValues, ParamValuesPartial, SettingsPartial, ngTableCoreModule } from '../../src/core'; @@ -14,7 +14,7 @@ describe('NgTableParams', () => { let scope: IScope, $rootScope: ScopeWithPrivates; - + beforeAll(() => expect(ngTableCoreModule).toBeDefined()); beforeEach(ng1.mock.module("ngTable-core")); @@ -35,10 +35,10 @@ describe('NgTableParams', () => { scope = $rootScope.$new(); })); - function createNgTableParams(initialParams?: ParamValues, settings?: SettingsPartial): NgTableParams; + function createNgTableParams(initialParams?: ParamValuesPartial, settings?: SettingsPartial): NgTableParams; function createNgTableParams(settings?: SettingsPartial): NgTableParams; function createNgTableParams(settings?: any): NgTableParams { - let initialParams: ParamValues; + let initialParams: ParamValuesPartial; if (arguments.length === 2) { initialParams = arguments[0]; settings = arguments[1]; @@ -117,22 +117,6 @@ describe('NgTableParams', () => { expect(params.page()).toBe(3); }); - it('NgTableParams parse url parameters', () => { - const params = new NgTableParams({ - 'sorting[name]': 'asc', - 'sorting[age]': 'desc', - 'filter[name]': 'test', - 'filter[age]': '20', - 'group[name]': 'asc', - 'group[age]': 'desc', - 'group[surname]': undefined - } as any); - - expect(params.filter()).toEqual({ 'name': 'test', 'age': 20 }); - expect(params.sorting()).toEqual({ 'age': 'desc' }); // sorting only by one column - todo: remove restriction - expect(params.group()).toEqual({ 'name': 'asc', 'age': 'desc', 'surname': undefined }); - }); - it('NgTableParams return url parameters', () => { const params = new NgTableParams({ 'sorting[name]': 'asc', @@ -884,23 +868,203 @@ describe('NgTableParams', () => { }); }); - it('ngTableParams test defaults', inject(($q: IQService, ngTableDefaults: Defaults) => { - ngTableDefaults.params.count = 2; - ngTableDefaults.settings.counts = []; - let tp = new NgTableParams({}); + describe('parameters', () => { + let tp: NgTableParams; + let defaultParamValues: ParamValues; + + beforeEach(() => { + defaultParamValues = { + count: 10, + filter: {}, + page: 1, + sorting: {}, + group: {} + }; + }); + + it('should be assigned default values when not supplied', () => { + tp = new NgTableParams(); + expect(tp.parameters()).toEqualPlainObject(defaultParamValues); + }); - expect(tp.count()).toEqual(2); - expect(tp.page()).toEqual(1); + it('missing values should be assigned default values', () => { + tp = new NgTableParams(); + expect(tp.parameters()).toEqualPlainObject(defaultParamValues); + }); - const settings = tp.settings(); - expect(settings.counts.length).toEqual(0); - expect(settings.interceptors.length).toEqual(0); - expect(settings.filterOptions.filterDelay).toEqual(500); + it('ngTableDefaults should override default values', inject((ngTableDefaults: Defaults) => { + try { + ngTableDefaults.params.filter = { age: 10 }; + tp = new NgTableParams(); - ngTableDefaults.settings.interceptors = [{ response: ng1.identity }]; - tp = new NgTableParams({}); - expect(tp.settings().interceptors.length).toEqual(1); - })); + const expected = defaultParamValues; + expected.filter = ngTableDefaults.params.filter; + + expect(tp.parameters()).toEqualPlainObject(expected); + } finally { + // reset + ngTableDefaults.params = {}; + } + })); + + it('should replace existing filter with new filter supplied', () => { + // given + tp = new NgTableParams({ + filter: { + name: 'stuff' + } + }); + const newParams = { + filter: { + age: 20 + } + }; + + // when + tp.parameters(newParams); + + // then + const expected = new ParamValues(); + expected.filter = newParams.filter; + const actual = tp.parameters(); + expect(actual).toEqualPlainObject(expected); + + // note: below we're testing the existing behaviour which is actually not a good one + // instead of assigning a reference params.filter we should be taking a copy + expect(actual.filter).toBe(newParams.filter); + }); + + it('should replace existing group with new group supplied', () => { + // given + tp = new NgTableParams({ + group: { + name: 'asc' + } + }); + const newParams: ParamValuesPartial = { + group: { + age: 'asc' + } + }; + + // when + tp.parameters(newParams); + + // then + const expected = new ParamValues(); + expected.group = newParams.group; + const actual = tp.parameters(); + expect(actual).toEqualPlainObject(expected); + + // note: below we're testing the existing behaviour which is actually not a good one + // instead of assigning a reference params.group we should be taking a copy + expect(actual.group).toBe(newParams.group); + }); + + it('should replace existing sorting with new sorting supplied', () => { + // given + tp = new NgTableParams({ + sorting: { + name: 'asc' + } + }); + const newParams: ParamValuesPartial = { + sorting: { + age: 'asc' + } + }; + + // when + tp.parameters(newParams); + + // then + const expected = new ParamValues(); + expected.sorting = newParams.sorting; + const actual = tp.parameters(); + expect(actual).toEqualPlainObject(expected); + + // note: below we're testing the existing behaviour which is actually not a good one + // instead of assigning a reference params.sorting we should be taking a copy + expect(actual.sorting).toBe(newParams.sorting); + }); + + it('undefined values in new params should be ignored', () => { + const newParams = _.mapValues(defaultParamValues, _.constant(undefined)); + tp = new NgTableParams(); + + tp.parameters(newParams); + + const actual = tp.parameters(); + expect(actual).toEqualPlainObject(defaultParamValues); + }); + + it('undefined values in ngTableDefaults should by ignored', inject((ngTableDefaults: Defaults) => { + try { + ngTableDefaults.params = _.mapValues(defaultParamValues, _.constant(undefined)); + tp = new NgTableParams(); + + expect(tp.parameters()).toEqualPlainObject(defaultParamValues); + } finally { + // reset + ngTableDefaults.params = {}; + } + })); + + it('should parse from url like parameters', () => { + const tp = new NgTableParams({ + 'sorting[name]': 'asc', + 'sorting[age]': 'desc', + 'filter[name]': 'test', + 'filter[age]': '20', + 'group[name]': 'asc', + 'group[age]': 'desc', + 'group[surname]': undefined + } as any); + + const expected = defaultParamValues; + expected.filter = { 'name': 'test', 'age': 20 }; + // sorting only by one column - todo: remove restriction + expected.sorting = { 'age': 'desc' }; + expected.group = { 'name': 'asc', 'age': 'desc', 'surname': undefined }; + + expect(tp.parameters()).toEqualPlainObject(defaultParamValues); + }); + + it('url like parameters are merged with existing values (badly!)', () => { + + // note: this test is here to show what the current behaviour IS + // and NOT what is necessarily correct + // There are at least 2 problems: + // 1. for consistency existing filtering, sorting, grouping should be replaced, not merged + // 2. there are bugs (see expected result below) + + // given + const tp = new NgTableParams({ + filter: { + name: 'test' + }, + sorting: { + name: 'asc' + } + }); + const newParams = { + 'filter[age]': '20', + 'sorting[age]': 'desc' + }; + + // when + tp.parameters(newParams); + + // then + const expected = defaultParamValues; + // this is buggy - age is not being included in the filter, sorting object + expected.filter = { name: 'test' }; + expected.sorting = { name: 'asc' }; + expected['filter[age]'] = 20; + expected['sorting[age]'] = 'desc'; + expect(tp.parameters()).toEqualPlainObject(defaultParamValues); + }); + }); describe('hasFilter', () => { let tp: NgTableParams; @@ -2092,7 +2256,7 @@ describe('NgTableParams', () => { actualPublisher = params; actualEventArgs = [newVal]; }); - var data = [1,2,3]; + var data = [1, 2, 3]; var params = createNgTableParams({ count: 5 }, { counts: [5, 10], dataset: data }); // when @@ -2111,11 +2275,11 @@ describe('NgTableParams', () => { actualEventArgs = [newVal]; }); - var initialDs = [10, 10, 101, 5]; + var initialDs = [10, 10, 101, 5]; var expectedDs = [10, 10, 101]; // when filtered with "10" var params = createNgTableParams({ dataset: initialDs }); params.filter({ $: "10" }); - + // when params.reload(); scope.$digest(); @@ -2133,7 +2297,7 @@ describe('NgTableParams', () => { actualPublisher = params; actualEventArgs = [newVal]; }); - var data = [1,2,3]; + var data = [1, 2, 3]; var params = createNgTableParams({ count: 5 }, { counts: [5, 10], dataset: data }); // when @@ -2152,11 +2316,11 @@ describe('NgTableParams', () => { actualEventArgs = [newVal]; }); - var initialDs = [10, 10, 101, 5]; + var initialDs = [10, 10, 101, 5]; var expectedDs = [10, 10, 101]; // when filtered with "10" var params = createNgTableParams({ dataset: initialDs }); params.filter({ $: "10" }); - + // when params.reload(); scope.$digest();