Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ZoneGridModel more forgiving (handle possibly out of sync state) #3530

Merged
merged 8 commits into from
Nov 21, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## 59.3.2 - 2023-11-21

### 🐞 Bug Fixes

* `ZoneGrid` will more gracefully handle state that has become out of sync with its mapper requirements.

## 59.3.1 - 2023-11-10

### 🐞 Bug Fixes
Expand Down
84 changes: 65 additions & 19 deletions cmp/zoneGrid/ZoneGridModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,21 @@ import {
} from '@ag-grid-community/core';
import {Icon} from '@xh/hoist/icon';
import {throwIf, withDefault} from '@xh/hoist/utils/js';
import {castArray, forOwn, isEmpty, isFinite, isPlainObject, isString} from 'lodash';
import {
castArray,
forOwn,
isEmpty,
isFinite,
isPlainObject,
isString,
dropRight,
remove
} from 'lodash';
import {ReactNode} from 'react';
import {ZoneMapperConfig, ZoneMapperModel} from './impl/ZoneMapperModel';
import {ZoneGridPersistenceModel} from './impl/ZoneGridPersistenceModel';
import {ZoneGridModelPersistOptions, Zone, ZoneLimit, ZoneMapping} from './Types';
import {Exception} from '../../core';

export interface ZoneGridConfig {
/**
Expand Down Expand Up @@ -326,7 +336,7 @@ export class ZoneGridModel extends HoistModel {

this.availableColumns = columns.map(it => ({...it, hidden: true}));
this.limits = limits;
this.mappings = this.parseMappings(mappings);
this.mappings = this.parseMappings(mappings, true);

this.leftColumnSpec = leftColumnSpec;
this.rightColumnSpec = rightColumnSpec;
Expand Down Expand Up @@ -397,7 +407,7 @@ export class ZoneGridModel extends HoistModel {

@action
setMappings(mappings: Record<Zone, Some<string | ZoneMapping>>) {
this.mappings = this.parseMappings(mappings);
this.mappings = this.parseMappings(mappings, false);
this.gridModel.setColumns(this.getColumns());
}

Expand Down Expand Up @@ -611,46 +621,77 @@ export class ZoneGridModel extends HoistModel {
}

private parseMappings(
mappings: Record<Zone, Some<string | ZoneMapping>>
mappings: Record<Zone, Some<string | ZoneMapping>>,
strict: boolean
): Record<Zone, ZoneMapping[]> {
const ret = {} as Record<Zone, ZoneMapping[]>;
forOwn(mappings, (rawMapping, zone) => {
// 1) Standardize mapping into an array of ZoneMappings
const mapping = [];
let mapping = [];
castArray(rawMapping).forEach(it => {
if (!it) return;

const ret = isString(it) ? {field: it} : it,
col = this.findColumnSpec(ret);

throwIf(!col, `Column not found for field ${ret.field}`);
return mapping.push(ret);
if (col) {
mapping.push(ret);
} else {
const message = `Column not found for field '${ret.field}'`;
this.throwOrWarn(message, strict);
febbraiod marked this conversation as resolved.
Show resolved Hide resolved
}
});

// 2) Ensure mapping respects configured limits
const limit = this.limits?.[zone];
if (limit) {
throwIf(
isFinite(limit.min) && mapping.length < limit.min,
`Requires minimum ${limit.min} mappings in zone "${zone}"`
);
throwIf(
isFinite(limit.max) && mapping.length > limit.max,
`Exceeds maximum ${limit.max} mappings in zone "${zone}"`
);
if (isFinite(limit.min) && mapping.length < limit.min) {
const message = `Requires minimum ${limit.min} mappings in zone "${zone}. Setting to default."`;
febbraiod marked this conversation as resolved.
Show resolved Hide resolved

this.throwOrWarn(message, strict);

mapping = this._defaultState.mappings[zone];
}

if (isFinite(limit.max) && mapping.length > limit.max) {
const diff = mapping.length - limit.max,
message = `Exceeds maximum ${limit.max} mappings in zone "${zone}". Dropping last ${diff} fields`;

this.throwOrWarn(message, strict);

mapping = dropRight(mapping, mapping.length - limit.max);
}

if (!isEmpty(limit.only)) {
const offenders = [];
mapping.forEach(it => {
throwIf(
!limit.only.includes(it.field),
`Field "${it.field}" not allowed in zone "${zone}"`
);
if (!limit.only.includes(it.field)) {
const message = `Field "${it.field}" not allowed in zone "${zone}. Removing"`;

this.throwOrWarn(message, strict);

offenders.push(it.field);
}
});

if (!isEmpty(offenders)) {
remove(mapping, it => offenders.includes(it.field));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set to default again here. Only need to warn for one field and say we're going to the default immediately at that point.

}
}
}

// 3) Ensure top zones have at least the minimum required single field
if ((zone == 'tl' || zone == 'tr') && isEmpty(mapping)) {
const message = `Top mapping '${zone}' requires at least one field. Setting to default.`;

this.throwOrWarn(message, strict);

mapping = this._defaultState.mappings[zone];
}

ret[zone] = mapping;
});

return ret;
}

Expand All @@ -663,4 +704,9 @@ export class ZoneGridModel extends HoistModel {
}
return mapperModel ? new ZoneMapperModel({zoneGridModel: this}) : null;
}

private throwOrWarn(message, strict) {
if (strict) throw Exception.create(message);
console.warn(message);
}
}
Loading