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

feat: notify onValidationError on paste if validation failed #1462

Merged
merged 9 commits into from
Apr 15, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const mockGetSelectionModel = {
const returnValueStub = jest.fn();
const gridStub = {
getActiveCell: jest.fn(),
getActiveCellNode: jest.fn(),
getColumns: jest.fn().mockReturnValue([
{ id: 'firstName', field: 'firstName', name: 'First Name', },
{ id: 'lastName', field: 'lastName', name: 'Last Name' },
Expand All @@ -47,6 +48,7 @@ const gridStub = {
triggerEvent: jest.fn().mockReturnValue({ getReturnValue: returnValueStub }),
onCellChange: new SlickEvent(),
onKeyDown: new SlickEvent(),
onValidationError: new SlickEvent(),
} as unknown as SlickGrid;

const mockCellSelectionModel = {
Expand All @@ -67,6 +69,7 @@ const mockTextEditor = {
applyValue: jest.fn(),
loadValue: jest.fn(),
serializeValue: jest.fn(),
validate: jest.fn().mockReturnValue({ valid: true, msg: null }),
} as unknown as InputEditor;

const mockTextEditorImplementation = jest.fn().mockImplementation(() => mockTextEditor);
Expand All @@ -80,9 +83,9 @@ describe('CellExternalCopyManager', () => {
lastNameElm.textContent = 'Last Name';
const mockEventCallback = () => { };
const mockColumns = [
{ id: 'firstName', field: 'firstName', name: 'First Name', editor: Editors.text, editorClass: Editors.text },
{ id: 'firstName', field: 'firstName', name: 'First Name', editor: { model: Editors.text }, editorClass: Editors.text },
{ id: 'lastName', field: 'lastName', name: lastNameElm, },
{ id: 'age', field: 'age', name: 'Age', editor: Editors.text, editorClass: Editors.text },
{ id: 'age', field: 'age', name: 'Age', editor: { model: Editors.text }, editorClass: Editors.text },
] as Column[];
let plugin: SlickCellExternalCopyManager;
const gridOptionsMock = {
Expand Down Expand Up @@ -196,6 +199,22 @@ describe('CellExternalCopyManager', () => {
expect(applyValSpy).toHaveBeenCalledWith(mockItem, 'some value');
});

it('should call "setDataItemValueForColumn" and expect an onValidationError triggered if validation failed', () => {
const validationResults = { valid: false, msg: 'foobar' };
const applyValSpy = jest.spyOn(mockTextEditor, 'applyValue');
const loadValSpy = jest.spyOn(mockTextEditor, 'loadValue');
const validationSpy = jest.spyOn(mockTextEditor, 'validate').mockReturnValue(validationResults);
const notifySpy = jest.spyOn(gridStub.onValidationError, 'notify');
const mockItem = { firstName: 'John', lastName: 'Doe' };
plugin.init(gridStub);
plugin.setDataItemValueForColumn(mockItem, mockColumns[0], 'some value');

expect(loadValSpy).toHaveBeenCalledWith(mockItem);
expect(applyValSpy).toHaveBeenCalledWith(mockItem, 'some value');
expect(validationSpy).toHaveBeenCalled();
expect(notifySpy).toHaveBeenCalledWith(expect.objectContaining({ validationResults }));
});

it('should call "setDataItemValueForColumn" and expect item last name to change with new value when no Editor is provided', () => {
const mockItem = { firstName: 'John', lastName: 'Doe' };
plugin.init(gridStub);
Expand Down
28 changes: 21 additions & 7 deletions packages/common/src/extensions/slickCellExternalCopyManager.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { createDomElement, getHtmlStringOutput, stripTags } from '@slickgrid-universal/utils';

import type { Column, ExcelCopyBufferOption, ExternalCopyClipCommand, OnEventArgs } from '../interfaces/index';
import type { Column, Editor, ExcelCopyBufferOption, ExternalCopyClipCommand, OnEventArgs } from '../interfaces/index';
import { SlickEvent, SlickEventData, SlickEventHandler, type SlickGrid, SlickRange, type SlickDataView, Utils as SlickUtils } from '../core/index';

// using external SlickGrid JS libraries
Expand Down Expand Up @@ -127,15 +127,15 @@ export class SlickCellExternalCopyManager {

// if a custom getter is not defined, we call serializeValue of the editor to serialize
if (columnDef) {
if (columnDef.editor) {
if (columnDef.editor?.model) {
Copy link
Owner

@ghiscoding ghiscoding Apr 9, 2024

Choose a reason for hiding this comment

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

actually I think it would be best to call the one that the SlickGrid is aware of which is editorClass since SlickGrid doesn't have any knowledge whatsoever of editor.model (you won't find editor.model anywhere in SlickGrid core file, it is assigned later by the wrapper which can be VanillaGridBundle, AngularSlickgrid, AureliaSlickgrid, ...). Note that there a few of them to replace

Copy link
Contributor Author

@zewa666 zewa666 Apr 9, 2024

Choose a reason for hiding this comment

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

thats interesting bc the examples all assign the model directly in the column definitions.

EDIT: ah ok misread, you're not using it in the core itself

hmm that also means though, while internalEditor is only deprecated but not gone we should take that first before editorClass?

Copy link
Owner

@ghiscoding ghiscoding Apr 9, 2024

Choose a reason for hiding this comment

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

hmm that also means though, while internalEditor is only deprecated but not gone we should take that first before editorClass?

hmm I don't so, I would think that using editorClass should work, internalEditor is only used by the wrappers. I understand that it might be confusing, it might be just in my head but because I want to keep my slickgrid.core.ts file as close as possible to the original SlickGrid project. Some properties are not meant to be used within SlickGrid core file, for example any props related to export services and any plugin props and so on.. when I wasn't sure if I was going to still use 6pac/slickgrid in here, I added Generics on SlickGrid that could be extended and so when I was using it in the wrapper, I could simply provide my custom GridOption and Column interfaces from the wrapper and SlickGrid was happy with it but it's just an extra layer that I would rather not really use too much (in fact I still have a lot of places where I have grid: SlickGrid instead of the full typed ref of grid: SlickGrid<GridOption, Column> because I find it too long and I don't have to do that anymore since I merged SlickGrid in here and typically the props I want to use are already in the GridOption/Column base interfaces)... so long story short, it's kind of "only me" knowing this little thing but I'm not sure I really want to go with the full Generics+extend interface way either because I think it would overcomplicate things. I don't want to extend GridOption 3 times (1x for SlickGrid, 1x for Slickgrid-Universal, 1x. for wrapper Angular-Slickgrid... I'm already doing an extend on the wrapper and because of that I always need to release Slickgrid-Universal + all wrappers on the same day because if I don't, TypeScript complains for some reasons that I've never figured out.

export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O extends BaseGridOption<C> = BaseGridOption<C>> {
// Public API

Copy link
Owner

@ghiscoding ghiscoding Apr 9, 2024

Choose a reason for hiding this comment

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

hmm that also means though, while internalEditor is only deprecated but not gone we should take that first before editorClass?

the internalEditor will be removed completely in the next major and its goal was simply to hide the complexity of Editor creation to the user. I didn't want to introduce a breaking change so it's still in place and still work but will be removed completely in next major. SlickGrid always had a editor grid option (it never had the filter option though) and it was used like this editor: Slick.longText (which is simple uninstantiated Class), but since I use a few external libraries (ms-select, autocomplete, flatpickr, ...), I needed a way to potentially provide extra options and I strongly felt that the developer would still prefer to use editor without necessarily caring about how it was used internally which is why at the end I came up with that internalEditor thing, it's something that was meant to be used only within the wrapper and hidden to the user...but in some cases the user want to have access to the editor instance and modify some props (e.g. add some items to the ms-select collection) and in that case the user needed to reference the instance (not the class itself that SlickGrid editor had) and that was confusing that the developer was creating it with editor but later had to reference it with internalEditor... so this is the long story of how it all started, hope it helps understanding why I'm trying to make it simpler ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok got it. yeah I make use of the internalEditor a couple of times in my ForeignKey Filter/Editor so I understand what you're trying to simplify here.

const tmpP = document.createElement('p');
const editor = new (columnDef as any).editor({
const editor = new (columnDef as any).editor.model({
container: tmpP, // a dummy container
column: columnDef,
event,
position: { top: 0, left: 0 }, // a dummy position required by some editors
grid: this._grid,
});
}) as Editor;
editor.loadValue(item);
retVal = editor.serializeValue();
editor.destroy();
Expand All @@ -155,15 +155,29 @@ export class SlickCellExternalCopyManager {
}

// if a custom setter is not defined, we call applyValue of the editor to unserialize
if (columnDef.editor) {
if (columnDef.editor?.model) {
const tmpDiv = document.createElement('div');
const editor = new (columnDef as any).editor({
const editor = new (columnDef as any).editor.model({
container: tmpDiv, // a dummy container
column: columnDef,
position: { top: 0, left: 0 }, // a dummy position required by some editors
grid: this._grid
});
}) as Editor;
Copy link
Owner

@ghiscoding ghiscoding Apr 15, 2024

Choose a reason for hiding this comment

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

@zewa666 ahh sorry, I missed this earlier, I think we can remove the any on the column and make the editorClass parsed as EditorConstructor that was added not long ago. I'm guessing we might have to explicitly parse it though

const editor = new (columnDef.editorClass as EditorConstructor)({ })`

unless this other suggestion works, which would be better

const editor = new columnDef.editorClass({ }) as EditorConstructor`

Note that there's 2 of these to change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if thats gonna work as editorClass can be both the ctor and an editor according to the type

Copy link
Owner

Choose a reason for hiding this comment

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

yeah ok I'll take a look at that later, I wanted to check on Stackblitz but it looks like it's blocked by the VPN on work laptop, so anyway I can review that on a separate PR if it could be improved (or not)

editor.loadValue(item);
const validationResults = editor.validate(undefined, value);
if (!validationResults.valid) {
const activeCell = this._grid.getActiveCell()!;
this._grid.onValidationError.notify({
editor,
cellNode: this._grid.getActiveCellNode()!,
validationResults,
row: activeCell?.row,
cell: activeCell?.cell,
column: columnDef,
grid: this._grid,
});
}

editor.applyValue(item, value);
editor.destroy();
tmpDiv.remove();
Expand Down
Loading