From a66fa19e330187706730762c8af98ab919839ba2 Mon Sep 17 00:00:00 2001 From: Arjunlal B Date: Wed, 20 Sep 2023 14:45:02 -0700 Subject: [PATCH 1/6] feat: add column config local storage support to table --- .../components/src/table/table.component.ts | 95 ++++++++++++++++--- .../table/table-widget-renderer.component.ts | 83 +++------------- 2 files changed, 93 insertions(+), 85 deletions(-) diff --git a/projects/components/src/table/table.component.ts b/projects/components/src/table/table.component.ts index 8c4a6e592..f598e7871 100644 --- a/projects/components/src/table/table.component.ts +++ b/projects/components/src/table/table.component.ts @@ -2,8 +2,8 @@ /* eslint-disable @angular-eslint/component-max-inline-declarations */ import { ModalService } from '../modal/modal.service'; import { - TableEditColumnsModalConfig, - TableEditColumnsModalComponent + TableEditColumnsModalComponent, + TableEditColumnsModalConfig } from './columns/table-edit-columns-modal.component'; import { CdkHeaderRow } from '@angular/cdk/table'; import { @@ -29,13 +29,16 @@ import { Dictionary, DomElementMeasurerService, isEqualIgnoreFunctions, + isNonEmptyString, NavigationService, NumberCoercer, + PreferenceService, + StorageType, TypedSimpleChanges } from '@hypertrace/common'; -import { isNil, without } from 'lodash-es'; -import { BehaviorSubject, combineLatest, merge, Observable, Subject } from 'rxjs'; -import { switchMap, take, filter, map } from 'rxjs/operators'; +import { isNil, pick, without } from 'lodash-es'; +import { BehaviorSubject, combineLatest, merge, Observable, of, Subject } from 'rxjs'; +import { filter, first, map, switchMap, take, tap } from 'rxjs/operators'; import { FilterAttribute } from '../filtering/filter/filter-attribute'; import { LoadAsyncConfig } from '../load-async/load-async.service'; import { PageEvent } from '../paginator/page.event'; @@ -308,6 +311,9 @@ export class TableComponent id: '$$detail' }; + @Input() + public id?: string; + @Input() public columnConfigs?: TableColumnConfig[]; @@ -433,7 +439,9 @@ export class TableComponent private readonly columnConfigsSubject: BehaviorSubject = new BehaviorSubject< TableColumnConfigExtended[] >([]); - public readonly columnConfigs$: Observable = this.columnConfigsSubject.asObservable(); + public readonly columnConfigs$: Observable< + TableColumnConfigExtended[] + > = this.columnConfigsSubject.asObservable().pipe(tap(columns => this.saveTablePreferences(columns))); public columnDefaultConfigs?: TableColumnConfigExtended[]; public visibleColumnConfigs: TableColumnConfigExtended[] = []; @@ -505,7 +513,8 @@ export class TableComponent private readonly activatedRoute: ActivatedRoute, private readonly domElementMeasurerService: DomElementMeasurerService, private readonly tableService: TableService, - private readonly modalService: ModalService + private readonly modalService: ModalService, + private readonly preferenceService: PreferenceService ) { combineLatest([this.activatedRoute.queryParamMap, this.columnConfigs$]) .pipe( @@ -686,6 +695,51 @@ export class TableComponent } } + private dehydratePersistedColumnConfig(column: TableColumnConfig): PersistedTableColumnConfig { + /* + * Note: The table columns have nested methods, so those are lost here when persistService uses JSON.stringify + * to convert and store. We want to just pluck the relevant properties that are required to be saved. + */ + return pick(column, ['id', 'visible']); + } + + private saveTablePreferences(columns: TableColumnConfig[]): void { + if (isNonEmptyString(this.id)) { + this.getLocalPreferences().subscribe(preferences => + this.setLocalPreferences({ + ...preferences, + columns: columns.map(column => this.dehydratePersistedColumnConfig(column)) + }) + ); + } + } + + private getLocalPreferences(): Observable { + return isNonEmptyString(this.id) + ? this.preferenceService.get(this.id, {}, StorageType.Local).pipe(first()) + : of({ columns: [] }); + } + + private setLocalPreferences(preferences: TableLocalPreferences): void { + if (isNonEmptyString(this.id)) { + this.preferenceService.set(this.id, preferences, StorageType.Local); + } + } + + private hydratePersistedColumnConfigs( + columns: TableColumnConfigExtended[], + persistedColumns: PersistedTableColumnConfig[] + ): TableColumnConfigExtended[] { + return columns.map(column => { + const found = persistedColumns.find(persistedColumn => persistedColumn.id === column.id); + + return { + ...column, // Apply default column config + ...(found ? found : {}) // Override with any saved properties + }; + }); + } + private setColumnResizeDefaults(columnResizeHandler: HTMLDivElement): void { columnResizeHandler.style.backgroundColor = Color.Transparent; columnResizeHandler.style.right = '2px'; @@ -714,15 +768,23 @@ export class TableComponent this.checkColumnWidthCompatibilityOrThrow(column.width); this.checkColumnWidthCompatibilityOrThrow(column.minWidth); }); - const columnConfigurations = this.buildColumnConfigExtendeds(columnConfigs ?? this.columnConfigs ?? []); + const columnConfigurations = this.buildColumnConfigExtended(columnConfigs ?? this.columnConfigs ?? []); if (isNil(this.columnDefaultConfigs)) { this.columnDefaultConfigs = columnConfigurations; } - const visibleColumns = columnConfigurations.filter(column => column.visible); - this.initialColumnConfigIdWidthMap = new Map(visibleColumns.map(column => [column.id, column.width ?? -1])); - this.updateVisibleColumns(visibleColumns); - this.columnConfigsSubject.next(columnConfigurations); + this.getLocalPreferences() + .pipe(take(1)) + .subscribe(preferences => { + const restoredColumnConfigs = this.hydratePersistedColumnConfigs( + columnConfigurations, + preferences.columns ?? [] + ); + const visibleColumns = restoredColumnConfigs.filter(column => column.visible); + this.initialColumnConfigIdWidthMap = new Map(visibleColumns.map(column => [column.id, column.width ?? -1])); + this.updateVisibleColumns(visibleColumns); + this.columnConfigsSubject.next(restoredColumnConfigs); + }); } private checkColumnWidthCompatibilityOrThrow(width?: TableColumnWidth): void { @@ -849,6 +911,7 @@ export class TableComponent ) ) .subscribe(editedColumnConfigs => { + this.saveTablePreferences(editedColumnConfigs); this.initializeColumns(editedColumnConfigs); }); } @@ -913,7 +976,7 @@ export class TableComponent return this.hasExpandableRows() ? index - 1 : index; } - private buildColumnConfigExtendeds(columnConfigs: TableColumnConfig[]): TableColumnConfigExtended[] { + private buildColumnConfigExtended(columnConfigs: TableColumnConfig[]): TableColumnConfigExtended[] { const stateColumns = []; if (this.hasMultiSelect()) { @@ -1183,3 +1246,9 @@ interface ColumnInfo { element: HTMLElement; bounds: ColumnBounds; } + +interface TableLocalPreferences { + columns?: PersistedTableColumnConfig[]; +} + +type PersistedTableColumnConfig = Pick; diff --git a/projects/observability/src/shared/dashboard/widgets/table/table-widget-renderer.component.ts b/projects/observability/src/shared/dashboard/widgets/table/table-widget-renderer.component.ts index 6f5a137b6..793dc2c37 100644 --- a/projects/observability/src/shared/dashboard/widgets/table/table-widget-renderer.component.ts +++ b/projects/observability/src/shared/dashboard/widgets/table/table-widget-renderer.component.ts @@ -33,8 +33,8 @@ import { } from '@hypertrace/components'; import { WidgetRenderer } from '@hypertrace/dashboards'; import { Renderer } from '@hypertrace/hyperdash'; -import { RendererApi, RENDERER_API } from '@hypertrace/hyperdash-angular'; -import { capitalize, isEmpty, isEqual, pick } from 'lodash-es'; +import { RENDERER_API, RendererApi } from '@hypertrace/hyperdash-angular'; +import { capitalize, isEmpty, isEqual } from 'lodash-es'; import { BehaviorSubject, combineLatest, Observable, of, Subject } from 'rxjs'; import { filter, @@ -58,7 +58,6 @@ import { MetadataService } from '../../../services/metadata/metadata.service'; import { InteractionHandler } from '../../interaction/interaction-handler'; import { TableWidgetRowInteractionModel } from './selections/table-widget-row-interaction.model'; import { TableWidgetBaseModel } from './table-widget-base.model'; -import { SpecificationBackedTableColumnDef } from './table-widget-column.model'; import { TableWidgetControlSelectOptionModel } from './table-widget-control-select-option.model'; import { TableWidgetViewToggleModel } from './table-widget-view-toggle.model'; import { TableWidgetModel } from './table-widget.model'; @@ -96,6 +95,7 @@ import { TableWidgetModel } from './table-widget.model'; @@ -316,21 +315,14 @@ export class TableWidgetRendererComponent } private getColumnConfigs(): Observable { - return this.getLocalPreferences().pipe( - switchMap(preferences => - combineLatest([this.getScope(), this.api.change$.pipe(mapTo(true), startWith(true))]).pipe( - switchMap(([scope]) => this.model.getColumns(scope)), - startWith([]), - map((columns: SpecificationBackedTableColumnDef[]) => - this.hydratePersistedColumnConfigs(columns, preferences.columns ?? []) - ), - pairwise(), - filter(([previous, current]) => !isEqualIgnoreFunctions(previous, current)), - map(([_, current]) => current), - share(), - tap(() => this.onDashboardRefresh()) - ) - ) + return combineLatest([this.getScope(), this.api.change$.pipe(mapTo(true), startWith(true))]).pipe( + switchMap(([scope]) => this.model.getColumns(scope)), + startWith([]), + pairwise(), + filter(([previous, current]) => !isEqualIgnoreFunctions(previous, current)), + map(([_, current]) => current), + share(), + tap(() => this.onDashboardRefresh()) ); } @@ -512,17 +504,6 @@ export class TableWidgetRendererComponent }); } - public onColumnsChange(columns: TableColumnConfig[]): void { - if (isNonEmptyString(this.model.getId())) { - this.getLocalPreferences().subscribe(preferences => - this.setLocalPreferences({ - ...preferences, - columns: columns.map(column => this.dehydratePersistedColumnConfig(column)) - }) - ); - } - } - public onRowClicked(row: StatefulTableRow): void { this.getRowClickInteractionHandler(row)?.execute(row); } @@ -558,28 +539,6 @@ export class TableWidgetRendererComponent : viewItems[TableWidgetRendererComponent.DEFAULT_TAB_INDEX]; } - private hydratePersistedColumnConfigs( - columns: SpecificationBackedTableColumnDef[], - persistedColumns: TableColumnConfig[] - ): SpecificationBackedTableColumnDef[] { - return columns.map(column => { - const found = persistedColumns.find(persistedColumn => persistedColumn.id === column.id); - - return { - ...column, // Apply default column config - ...(found ? found : {}) // Override with any saved properties - }; - }); - } - - private dehydratePersistedColumnConfig(column: TableColumnConfig): PersistedTableColumnConfig { - /* - * Note: The table columns have nested methods, so those are lost here when persistService uses JSON.stringify - * to convert and store. We want to just pluck the relevant properties that are required to be saved. - */ - return pick(column, ['id', 'visible']); - } - private getViewPreferences(): Observable { return isNonEmptyString(this.model.viewId) ? this.preferenceService.get(this.model.viewId, {}, StorageType.Local).pipe(first()) @@ -592,20 +551,6 @@ export class TableWidgetRendererComponent } } - private getLocalPreferences(): Observable { - return isNonEmptyString(this.model.getId()) - ? this.preferenceService - .get(this.model.getId()!, {}, StorageType.Local) - .pipe(first()) - : of({}); - } - - private setLocalPreferences(preferences: TableWidgetLocalPreferences): void { - if (isNonEmptyString(this.model.getId())) { - this.preferenceService.set(this.model.getId()!, preferences, StorageType.Local); - } - } - private getSessionPreferences(): Observable { return isNonEmptyString(this.model.getId()) ? this.preferenceService @@ -649,13 +594,7 @@ interface TableWidgetViewPreferences { activeView?: string; } -interface TableWidgetLocalPreferences { - columns?: PersistedTableColumnConfig[]; -} - interface TableWidgetSessionPreferences { checkboxes?: TableCheckboxControl[]; selections?: TableSelectControl[]; } - -type PersistedTableColumnConfig = Pick; From 9f7ab2e1547be5b0cd46a35632da0dcb988ff078 Mon Sep 17 00:00:00 2001 From: Arjunlal B Date: Wed, 20 Sep 2023 15:13:18 -0700 Subject: [PATCH 2/6] fix: add `getOnce` method to preference service --- .../src/preference/preference.service.ts | 18 +++++++++ .../src/table/table.component.test.ts | 6 ++- .../components/src/table/table.component.ts | 39 ++++++++----------- 3 files changed, 39 insertions(+), 24 deletions(-) diff --git a/projects/common/src/preference/preference.service.ts b/projects/common/src/preference/preference.service.ts index d498d341d..890feb61c 100644 --- a/projects/common/src/preference/preference.service.ts +++ b/projects/common/src/preference/preference.service.ts @@ -55,6 +55,24 @@ export class PreferenceService { ); } + /** + * Returns the current storage value if defined, else the default value. + */ + public getOnce( + key: PreferenceKey, + defaultValue?: T, + type: StorageType = PreferenceService.DEFAULT_STORAGE_TYPE + ): T { + const storedValue = this.preferenceStorage(type).get(this.asStorageKey(key)); + const value = this.fromStorageValue(storedValue) ?? defaultValue; + + if (value === undefined) { + throw Error(`No value found or default provided for preferenceKey: ${key}`); + } + + return value; + } + public set( key: PreferenceKey, value: PreferenceValue, diff --git a/projects/components/src/table/table.component.test.ts b/projects/components/src/table/table.component.test.ts index a99b7a62c..3a7020b80 100644 --- a/projects/components/src/table/table.component.test.ts +++ b/projects/components/src/table/table.component.test.ts @@ -1,7 +1,7 @@ /* eslint-disable max-lines */ import { fakeAsync, flush } from '@angular/core/testing'; import { ActivatedRoute, convertToParamMap } from '@angular/router'; -import { DomElementMeasurerService, NavigationService } from '@hypertrace/common'; +import { DomElementMeasurerService, NavigationService, PreferenceService } from '@hypertrace/common'; import { runFakeRxjs } from '@hypertrace/test-utils'; import { createHostFactory, mockProvider } from '@ngneat/spectator/jest'; import { MockComponent } from 'ng-mocks'; @@ -75,6 +75,10 @@ describe('Table component', () => { }), mockProvider(ModalService, { createModal: jest.fn().mockReturnValue({ closed$: of([]) }) + }), + mockProvider(PreferenceService, { + getOnce: jest.fn().mockReturnValue({ columns: [] }), + set: jest.fn() }) ], declarations: [MockComponent(PaginatorComponent), MockComponent(SearchBoxComponent)], diff --git a/projects/components/src/table/table.component.ts b/projects/components/src/table/table.component.ts index f598e7871..0fd4ecf87 100644 --- a/projects/components/src/table/table.component.ts +++ b/projects/components/src/table/table.component.ts @@ -37,8 +37,8 @@ import { TypedSimpleChanges } from '@hypertrace/common'; import { isNil, pick, without } from 'lodash-es'; -import { BehaviorSubject, combineLatest, merge, Observable, of, Subject } from 'rxjs'; -import { filter, first, map, switchMap, take, tap } from 'rxjs/operators'; +import { BehaviorSubject, combineLatest, merge, Observable, Subject } from 'rxjs'; +import { filter, map, switchMap, take, tap } from 'rxjs/operators'; import { FilterAttribute } from '../filtering/filter/filter-attribute'; import { LoadAsyncConfig } from '../load-async/load-async.service'; import { PageEvent } from '../paginator/page.event'; @@ -705,19 +705,17 @@ export class TableComponent private saveTablePreferences(columns: TableColumnConfig[]): void { if (isNonEmptyString(this.id)) { - this.getLocalPreferences().subscribe(preferences => - this.setLocalPreferences({ - ...preferences, - columns: columns.map(column => this.dehydratePersistedColumnConfig(column)) - }) - ); + this.setLocalPreferences({ + ...this.getLocalPreferences(), + columns: columns.map(column => this.dehydratePersistedColumnConfig(column)) + }); } } - private getLocalPreferences(): Observable { + private getLocalPreferences(): TableLocalPreferences { return isNonEmptyString(this.id) - ? this.preferenceService.get(this.id, {}, StorageType.Local).pipe(first()) - : of({ columns: [] }); + ? this.preferenceService.getOnce(this.id, {}, StorageType.Local) + : { columns: [] }; } private setLocalPreferences(preferences: TableLocalPreferences): void { @@ -773,18 +771,13 @@ export class TableComponent this.columnDefaultConfigs = columnConfigurations; } - this.getLocalPreferences() - .pipe(take(1)) - .subscribe(preferences => { - const restoredColumnConfigs = this.hydratePersistedColumnConfigs( - columnConfigurations, - preferences.columns ?? [] - ); - const visibleColumns = restoredColumnConfigs.filter(column => column.visible); - this.initialColumnConfigIdWidthMap = new Map(visibleColumns.map(column => [column.id, column.width ?? -1])); - this.updateVisibleColumns(visibleColumns); - this.columnConfigsSubject.next(restoredColumnConfigs); - }); + const preferences = this.getLocalPreferences(); + + const restoredColumnConfigs = this.hydratePersistedColumnConfigs(columnConfigurations, preferences.columns ?? []); + const visibleColumns = restoredColumnConfigs.filter(column => column.visible); + this.initialColumnConfigIdWidthMap = new Map(visibleColumns.map(column => [column.id, column.width ?? -1])); + this.updateVisibleColumns(visibleColumns); + this.columnConfigsSubject.next(restoredColumnConfigs); } private checkColumnWidthCompatibilityOrThrow(width?: TableColumnWidth): void { From 7bcde5a464c8363cf12eaa899370a672dd262ce1 Mon Sep 17 00:00:00 2001 From: Arjunlal B Date: Wed, 20 Sep 2023 18:49:52 -0700 Subject: [PATCH 3/6] fix: update tests and minor refactor --- .../components/src/table/table.component.test.ts | 9 +++++---- projects/components/src/table/table.component.ts | 14 ++++++++------ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/projects/components/src/table/table.component.test.ts b/projects/components/src/table/table.component.test.ts index 3a7020b80..09024a2b8 100644 --- a/projects/components/src/table/table.component.test.ts +++ b/projects/components/src/table/table.component.test.ts @@ -1,7 +1,7 @@ /* eslint-disable max-lines */ import { fakeAsync, flush } from '@angular/core/testing'; import { ActivatedRoute, convertToParamMap } from '@angular/router'; -import { DomElementMeasurerService, NavigationService, PreferenceService } from '@hypertrace/common'; +import { DomElementMeasurerService, NavigationService, PreferenceService, PreferenceValue } from '@hypertrace/common'; import { runFakeRxjs } from '@hypertrace/test-utils'; import { createHostFactory, mockProvider } from '@ngneat/spectator/jest'; import { MockComponent } from 'ng-mocks'; @@ -20,6 +20,7 @@ import { TableColumnConfigExtended, TableService } from './table.service'; import { ModalService } from '../modal/modal.service'; describe('Table component', () => { + let localStorage: PreferenceValue = { columns: [] }; // TODO remove builders once table stops mutating inputs const buildData = () => [ { @@ -56,7 +57,6 @@ describe('Table component', () => { imports: [LetAsyncModule], providers: [ mockProvider(NavigationService), - mockProvider(ActivatedRoute), mockProvider(ActivatedRoute, { queryParamMap: EMPTY }), @@ -77,8 +77,8 @@ describe('Table component', () => { createModal: jest.fn().mockReturnValue({ closed$: of([]) }) }), mockProvider(PreferenceService, { - getOnce: jest.fn().mockReturnValue({ columns: [] }), - set: jest.fn() + getOnce: () => localStorage, + set: (_: unknown, value: PreferenceValue) => (localStorage = value) }) ], declarations: [MockComponent(PaginatorComponent), MockComponent(SearchBoxComponent)], @@ -277,6 +277,7 @@ describe('Table component', () => { syncWithUrl: false } }); + spectator.tick(); spectator.component.onSortChange(TableSortDirection.Ascending, columns[0]); diff --git a/projects/components/src/table/table.component.ts b/projects/components/src/table/table.component.ts index 0fd4ecf87..757290613 100644 --- a/projects/components/src/table/table.component.ts +++ b/projects/components/src/table/table.component.ts @@ -441,7 +441,7 @@ export class TableComponent >([]); public readonly columnConfigs$: Observable< TableColumnConfigExtended[] - > = this.columnConfigsSubject.asObservable().pipe(tap(columns => this.saveTablePreferences(columns))); + > = this.columnConfigsSubject.asObservable().pipe(tap(columnConfigs => this.saveTablePreferences(columnConfigs))); public columnDefaultConfigs?: TableColumnConfigExtended[]; public visibleColumnConfigs: TableColumnConfigExtended[] = []; @@ -714,7 +714,7 @@ export class TableComponent private getLocalPreferences(): TableLocalPreferences { return isNonEmptyString(this.id) - ? this.preferenceService.getOnce(this.id, {}, StorageType.Local) + ? this.preferenceService.getOnce(this.id, { columns: [] }, StorageType.Local) : { columns: [] }; } @@ -772,12 +772,14 @@ export class TableComponent } const preferences = this.getLocalPreferences(); - - const restoredColumnConfigs = this.hydratePersistedColumnConfigs(columnConfigurations, preferences.columns ?? []); - const visibleColumns = restoredColumnConfigs.filter(column => column.visible); + const columnConfigsWithPersistedData = this.hydratePersistedColumnConfigs( + columnConfigurations, + preferences.columns ?? [] + ); + const visibleColumns = columnConfigsWithPersistedData.filter(column => column.visible); this.initialColumnConfigIdWidthMap = new Map(visibleColumns.map(column => [column.id, column.width ?? -1])); this.updateVisibleColumns(visibleColumns); - this.columnConfigsSubject.next(restoredColumnConfigs); + this.columnConfigsSubject.next(columnConfigsWithPersistedData); } private checkColumnWidthCompatibilityOrThrow(width?: TableColumnWidth): void { From 4a0e369784f79663d494f323e8f32cfc3e8e66da Mon Sep 17 00:00:00 2001 From: Arjunlal B Date: Thu, 21 Sep 2023 14:00:27 -0700 Subject: [PATCH 4/6] fix: fix table test --- .../src/table/table.component.test.ts | 16 ++++++++------- .../components/src/table/table.component.ts | 20 +++++++++---------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/projects/components/src/table/table.component.test.ts b/projects/components/src/table/table.component.test.ts index 09024a2b8..9598c4175 100644 --- a/projects/components/src/table/table.component.test.ts +++ b/projects/components/src/table/table.component.test.ts @@ -260,12 +260,15 @@ describe('Table component', () => { }); spectator.tick(); - expect(spectator.component.columnConfigs![0]).toEqual( - expect.objectContaining({ - sort: TableSortDirection.Ascending, - id: 'firstId' - }) - ); + runFakeRxjs(({ expectObservable }) => { + expectObservable(spectator.component.columnConfigs$.pipe(map(columns => columns[0]))).toBe('x', { + x: expect.objectContaining({ + sort: TableSortDirection.Ascending, + id: 'firstId' + }) + }); + }); + flush(); })); test('does not alter the URL on sorting if syncWithUrl false', fakeAsync(() => { @@ -277,7 +280,6 @@ describe('Table component', () => { syncWithUrl: false } }); - spectator.tick(); spectator.component.onSortChange(TableSortDirection.Ascending, columns[0]); diff --git a/projects/components/src/table/table.component.ts b/projects/components/src/table/table.component.ts index 757290613..d7f33bbcb 100644 --- a/projects/components/src/table/table.component.ts +++ b/projects/components/src/table/table.component.ts @@ -36,9 +36,9 @@ import { StorageType, TypedSimpleChanges } from '@hypertrace/common'; -import { isNil, pick, without } from 'lodash-es'; +import { isNil, without } from 'lodash-es'; import { BehaviorSubject, combineLatest, merge, Observable, Subject } from 'rxjs'; -import { filter, map, switchMap, take, tap } from 'rxjs/operators'; +import { filter, map, switchMap, take } from 'rxjs/operators'; import { FilterAttribute } from '../filtering/filter/filter-attribute'; import { LoadAsyncConfig } from '../load-async/load-async.service'; import { PageEvent } from '../paginator/page.event'; @@ -439,9 +439,7 @@ export class TableComponent private readonly columnConfigsSubject: BehaviorSubject = new BehaviorSubject< TableColumnConfigExtended[] >([]); - public readonly columnConfigs$: Observable< - TableColumnConfigExtended[] - > = this.columnConfigsSubject.asObservable().pipe(tap(columnConfigs => this.saveTablePreferences(columnConfigs))); + public readonly columnConfigs$: Observable = this.columnConfigsSubject.asObservable(); public columnDefaultConfigs?: TableColumnConfigExtended[]; public visibleColumnConfigs: TableColumnConfigExtended[] = []; @@ -700,7 +698,7 @@ export class TableComponent * Note: The table columns have nested methods, so those are lost here when persistService uses JSON.stringify * to convert and store. We want to just pluck the relevant properties that are required to be saved. */ - return pick(column, ['id', 'visible']); + return { id: column.id, visible: column.visible }; } private saveTablePreferences(columns: TableColumnConfig[]): void { @@ -733,7 +731,7 @@ export class TableComponent return { ...column, // Apply default column config - ...(found ? found : {}) // Override with any saved properties + ...(found ? { visible: found.visible } : {}) // Override with any saved properties }; }); } @@ -772,14 +770,15 @@ export class TableComponent } const preferences = this.getLocalPreferences(); - const columnConfigsWithPersistedData = this.hydratePersistedColumnConfigs( + const columnConfigsWithUserPref = this.hydratePersistedColumnConfigs( columnConfigurations, preferences.columns ?? [] ); - const visibleColumns = columnConfigsWithPersistedData.filter(column => column.visible); + + const visibleColumns = columnConfigsWithUserPref.filter(column => column.visible); this.initialColumnConfigIdWidthMap = new Map(visibleColumns.map(column => [column.id, column.width ?? -1])); this.updateVisibleColumns(visibleColumns); - this.columnConfigsSubject.next(columnConfigsWithPersistedData); + this.columnConfigsSubject.next(columnConfigsWithUserPref); } private checkColumnWidthCompatibilityOrThrow(width?: TableColumnWidth): void { @@ -884,6 +883,7 @@ export class TableComponent const updatedColumns = this.columnConfigsSubject.value; this.updateVisibleColumns(updatedColumns.filter(c => c.visible)); this.distributeWidthToColumns(TableColumnWidthUtil.getColWidthInPx(column.width)); + this.saveTablePreferences(updatedColumns); this.columnConfigsSubject.next(updatedColumns); } From 5bf3aa40d2dfd2d4ba663a524f30d3773fb495b8 Mon Sep 17 00:00:00 2001 From: Arjunlal B Date: Thu, 21 Sep 2023 14:43:01 -0700 Subject: [PATCH 5/6] fix: add more tests --- .../src/table/table.component.test.ts | 162 +++++++++++++++++- .../components/src/table/table.component.ts | 2 +- 2 files changed, 161 insertions(+), 3 deletions(-) diff --git a/projects/components/src/table/table.component.test.ts b/projects/components/src/table/table.component.test.ts index 9598c4175..bfe6ca13a 100644 --- a/projects/components/src/table/table.component.test.ts +++ b/projects/components/src/table/table.component.test.ts @@ -1,7 +1,13 @@ /* eslint-disable max-lines */ import { fakeAsync, flush } from '@angular/core/testing'; import { ActivatedRoute, convertToParamMap } from '@angular/router'; -import { DomElementMeasurerService, NavigationService, PreferenceService, PreferenceValue } from '@hypertrace/common'; +import { + DomElementMeasurerService, + NavigationService, + PreferenceService, + PreferenceValue, + StorageType +} from '@hypertrace/common'; import { runFakeRxjs } from '@hypertrace/test-utils'; import { createHostFactory, mockProvider } from '@ngneat/spectator/jest'; import { MockComponent } from 'ng-mocks'; @@ -77,7 +83,7 @@ describe('Table component', () => { createModal: jest.fn().mockReturnValue({ closed$: of([]) }) }), mockProvider(PreferenceService, { - getOnce: () => localStorage, + getOnce: jest.fn().mockReturnValue(localStorage), set: (_: unknown, value: PreferenceValue) => (localStorage = value) }) ], @@ -587,5 +593,157 @@ describe('Table component', () => { ] }); }); + flush(); + })); + + test('saved preferences should be accounted for while building column configs', fakeAsync(() => { + const columns = buildColumns(); + let localStorageOverride: PreferenceValue = { + columns: [ + { + id: 'firstId', + visible: false + }, + { id: 'secondId', visible: true } + ] + }; + const spectator = createHost( + '', + { + hostProps: { + columnConfigs: columns, + data: buildData(), + selectionMode: TableSelectionMode.Single, + mode: TableMode.Flat + }, + providers: [ + mockProvider(PreferenceService, { + getOnce: jest.fn().mockReturnValue(localStorageOverride), + set: jest.fn().mockImplementation((_key: string, value: PreferenceValue, _storageType: StorageType) => { + localStorageOverride = value; + }) + }) + ] + } + ); + spectator.tick(); + + runFakeRxjs(({ expectObservable }) => { + expectObservable(spectator.component.columnConfigs$).toBe('x', { + x: [ + expect.objectContaining({ + id: 'firstId', + visible: false + }), + expect.objectContaining({ + id: 'secondId', + visible: true + }) + ] + }); + }); + + expect(spectator.inject(PreferenceService).getOnce).toHaveBeenLastCalledWith( + 'test-table', + { columns: [] }, + StorageType.Local + ); + flush(); + })); + + test('should save user preferences for columns when a column is hidden', fakeAsync(() => { + const columns = buildColumns(); + let localStorageOverride: PreferenceValue = { + columns: [] + }; + const spectator = createHost( + '', + { + hostProps: { + columnConfigs: columns, + data: buildData(), + selectionMode: TableSelectionMode.Single, + mode: TableMode.Flat + }, + providers: [ + mockProvider(PreferenceService, { + getOnce: jest.fn().mockReturnValue(localStorageOverride), + set: jest.fn().mockImplementation((_key: string, value: PreferenceValue, _storageType: StorageType) => { + localStorageOverride = value; + }) + }) + ] + } + ); + spectator.component.onHideColumn({ ...columns[0] }); + spectator.tick(); + expect(spectator.inject(PreferenceService).set).toHaveBeenCalledWith( + 'test-table', + { + columns: expect.arrayContaining([ + { + id: 'firstId', + visible: false + }, + { + id: 'secondId', + visible: true + } + ]) + }, + StorageType.Local + ); + + flush(); + })); + + test('should save user preferences for columns when columns are edited', fakeAsync(() => { + const columns = buildColumns(); + let localStorageOverride: PreferenceValue = { + columns: [] + }; + const spectator = createHost( + '', + { + hostProps: { + columnConfigs: columns, + data: buildData(), + selectionMode: TableSelectionMode.Single, + mode: TableMode.Flat + }, + providers: [ + mockProvider(PreferenceService, { + getOnce: jest.fn().mockReturnValue(localStorageOverride), + set: jest.fn().mockImplementation((_key: string, value: PreferenceValue, _storageType: StorageType) => { + localStorageOverride = value; + }) + }), + mockProvider(ModalService, { + createModal: jest.fn().mockReturnValue({ closed$: of([columns[0], { ...columns[1], visible: false }]) }) + }) + ] + } + ); + spectator.tick(); + spectator.component.showEditColumnsModal(); + spectator.tick(); + expect(spectator.inject(PreferenceService).set).toHaveBeenCalledWith( + 'test-table', + { + columns: expect.arrayContaining([ + { + id: 'firstId', + visible: true + }, + { + id: 'secondId', + visible: false + } + ]) + }, + StorageType.Local + ); + + flush(); })); }); diff --git a/projects/components/src/table/table.component.ts b/projects/components/src/table/table.component.ts index d7f33bbcb..12482360f 100644 --- a/projects/components/src/table/table.component.ts +++ b/projects/components/src/table/table.component.ts @@ -883,7 +883,7 @@ export class TableComponent const updatedColumns = this.columnConfigsSubject.value; this.updateVisibleColumns(updatedColumns.filter(c => c.visible)); this.distributeWidthToColumns(TableColumnWidthUtil.getColWidthInPx(column.width)); - this.saveTablePreferences(updatedColumns); + this.saveTablePreferences(updatedColumns.map(col => (column.id === col.id ? { ...col, visible: false } : col))); this.columnConfigsSubject.next(updatedColumns); } From 4437af94a84dca1c19aa09bb9676af60769a5c47 Mon Sep 17 00:00:00 2001 From: Arjunlal B Date: Thu, 21 Sep 2023 15:00:01 -0700 Subject: [PATCH 6/6] fix: mock preferences in explorer test --- .../src/pages/explorer/explorer.component.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/projects/observability/src/pages/explorer/explorer.component.test.ts b/projects/observability/src/pages/explorer/explorer.component.test.ts index 90c92b25b..9922ca470 100644 --- a/projects/observability/src/pages/explorer/explorer.component.test.ts +++ b/projects/observability/src/pages/explorer/explorer.component.test.ts @@ -11,7 +11,9 @@ import { LayoutChangeService, NavigationService, PreferenceService, + PreferenceValue, RelativeTimeRange, + StorageType, TimeDuration, TimeRangeService, TimeUnit @@ -71,6 +73,7 @@ describe('Explorer component', () => { const testTimeRange = new RelativeTimeRange(new TimeDuration(15, TimeUnit.Minute)); const createComponent = createComponentFactory({ component: ExplorerComponent, + shallow: true, imports: [ ExplorerModule.withDashboardBuilderFactory({ useFactory: (metadataService: MetadataService, filterBuilderLookupService: FilterBuilderLookupService) => @@ -111,7 +114,10 @@ describe('Explorer component', () => { } }, mockProvider(PreferenceService, { - get: jest.fn().mockReturnValue(of(true)) + get: jest.fn().mockReturnValue(of(true)), + getOnce: jest + .fn() + .mockImplementation((_key: string, defaultValue: PreferenceValue, _storageType: StorageType) => defaultValue) }), ...getMockFlexLayoutProviders() ]