Merge pull request #634 from ghiscoding/bugfix/select-editor-multiple…

…-save

fix(editors): select editor should call save only once

examples/webpack-demo-vanilla-bundle/src/examples/example12.ts

@@ -352,7 +352,7 @@ export class Example12 {
       {
         id: 'action', name: 'Action', field: 'action', width: 70, minWidth: 70, maxWidth: 70,
         excludeFromExport: true,
-        formatter: () => `<div class="button-style margin-auto" style="width: 35px; margin-top: -1px;"><span class="mdi mdi-chevron-down mdi-22px color-primary"></span></div>`,
+        formatter: () => `<div class="button-style margin-auto" style="width: 35px; margin-top: -1px;"><span class="mdi mdi-dots-vertical mdi-22px color-primary"></span></div>`,
         cellMenu: {
           hideCloseButton: false,
           commandTitle: 'Commands',
@@ -554,6 +554,7 @@ export class Example12 {
   handleOnCellChange(event) {
     const args = event && event.detail && event.detail.args;
     const dataContext = args && args.item;
+    console.log('cell change', args)
 
     // when the field "completed" changes to false, we also need to blank out the "finish" date
     if (dataContext && !dataContext.completed) {

packages/common/src/editors/__tests__/selectEditor.spec.ts

@@ -539,10 +539,41 @@ describe('SelectEditor', () => {
         editor.loadValue(mockItemData);
         editor.destroy();
 
+        expect(saveSpy).toHaveBeenCalledTimes(1);
         expect(saveSpy).toHaveBeenCalledWith(true);
         expect(lockSpy).toHaveBeenCalled();
       });
 
+      it('should call "save(true)" only once when autoCommitEdit is True and even when both "onClose" and "destroy" are called', () => {
+        mockItemData = { id: 1, gender: 'male', isActive: true };
+        gridOptionMock.autoCommitEdit = true;
+
+        editor = new SelectEditor(editorArguments, true);
+        const saveSpy = jest.spyOn(editor, 'save');
+
+        editor.loadValue(mockItemData);
+        editor.editorDomElement.multipleSelect('close');
+        editor.destroy();
+
+        expect(saveSpy).toHaveBeenCalledTimes(1);
+        expect(saveSpy).toHaveBeenCalledWith(true);
+      });
+
+      it('should call "save(false)" only once when autoCommitEdit is False and even when both "onClose" and "destroy" are called', () => {
+        mockItemData = { id: 1, gender: 'male', isActive: true };
+        gridOptionMock.autoCommitEdit = false;
+
+        editor = new SelectEditor(editorArguments, true);
+        const saveSpy = jest.spyOn(editor, 'save');
+
+        editor.loadValue(mockItemData);
+        editor.editorDomElement.multipleSelect('close');
+        editor.destroy();
+
+        expect(saveSpy).toHaveBeenCalledTimes(1);
+        expect(saveSpy).toHaveBeenCalledWith(false);
+      });
+
       it('should not call "commitCurrentEdit" when "hasAutoCommitEdit" is disabled', () => {
         mockItemData = { id: 1, gender: 'male', isActive: true };
         gridOptionMock.autoCommitEdit = false;

packages/common/src/editors/selectEditor.ts

@@ -37,7 +37,7 @@ export class SelectEditor implements Editor {
 
   // flag to signal that the editor is destroying itself, helps prevent
   // commit changes from being called twice and erroring
-  protected _isDisposing = false;
+  protected _isDisposingOrCallingSave = false;
 
   /** Collection Service */
   protected _collectionService!: CollectionService;
@@ -129,7 +129,8 @@ export class SelectEditor implements Editor {
         if (compositeEditorOptions) {
           this.handleChangeOnCompositeEditor(compositeEditorOptions);
         } else {
-          this.save();
+          this._isDisposingOrCallingSave = true;
+          this.save(this.hasAutoCommitEdit);
         }
       },
     };
@@ -403,12 +404,12 @@ export class SelectEditor implements Editor {
   destroy() {
     // when autoCommitEdit is enabled, we might end up leaving an editor without it being saved, if so do call a save before destroying
     // this mainly happens doing a blur or focusing on another cell in the grid (it won't come here if we click outside the grid, in the body)
-    if (this.$editorElm && this.hasAutoCommitEdit && this.isValueChanged() && !this._isDisposing && !this.isCompositeEditor) {
-      this._isDisposing = true; // change destroying flag to avoid infinite loop
+    if (this.$editorElm && this.hasAutoCommitEdit && this.isValueChanged() && !this._isDisposingOrCallingSave && !this.isCompositeEditor) {
+      this._isDisposingOrCallingSave = true; // change destroying flag to avoid infinite loop
       this.save(true);
     }
 
-    this._isDisposing = true;
+    this._isDisposingOrCallingSave = true;
     if (this.$editorElm && typeof this.$editorElm.multipleSelect === 'function') {
       this.$editorElm.multipleSelect('destroy');
       this.$editorElm.remove();
@@ -548,7 +549,7 @@ export class SelectEditor implements Editor {
     const validation = this.validate();
     const isValid = validation?.valid ?? false;
 
-    if ((!this._isDisposing || forceCommitCurrentEdit) && this.hasAutoCommitEdit && isValid) {
+    if ((!this._isDisposingOrCallingSave || forceCommitCurrentEdit) && this.hasAutoCommitEdit && isValid) {
       // do not use args.commitChanges() as this sets the focus to the next row.
       // also the select list will stay shown when clicking off the grid
       this.grid.getEditorLock().commitCurrentEdit();