Composite Editor manipulating DataContext via editor save method not persisted

[zewa666]

Describe the bug

When using the CompositeEditor it renders also my custom editor. this Foreign Key lookup editor does modify not only the bound column but also others in the dataContext (label + value) in its save command.

When I do this via cell edits, all is good and leaving the cell will modify both the bound column and my modified additional column.

If I do the same within the onSave of the CompositeEditor (e.g dataContext["foo"] = "bar" it works as well, just not in that editors save command as mentioned.

Reproduction

as described above

Which Framework are you using?

Angular

Environment Info

| Executable          | Version |
| ------------------- | ------- |
| (framework used)    | VERSION |
| Slickgrid-Universal | VERSION |
| TypeScript          | VERSION |
| Browser(s)          | VERSION |
| System OS           | VERSION |

Validations

• Follow our Code of Conduct
• Read the Wikis.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion.
• The provided reproduction is a minimal reproducible example of the bug.

[ghiscoding]

I don't really understand your problem, since with the Composite Editor you're supposed to use the Composite Editor onSave callback and then use either the 1st arg formValues or the last arg dataContextOrUpdatedDatasetPreview. Have you read the docs that I wrote? It's been a while since I've look at the code

this.compositeEditorInstance?.openDetails({
  onSave: (formValues, _selection, dataContextOrUpdatedDatasetPreview) => {
    // save it
  }
}

[zewa666]

I did yeah and as said it works if done in the onSave callback. yet its a super weird unexpected behavior that it does work in cell edit mode from custom editors save method but the same editor and save function running inside composite mode behaves different.

I suspect that the args.items, passed in isnt the same reference as the original dataContext used later on for the actual grids row

[ghiscoding]

I think you'll have to debug this one yourself since I'm not really sure what you mean and without a repro, it's hard to provide any help. Technically speaking there's 2 files to handle composite

• slick-composite-editor.component.ts
• compositeEditor.factory.ts

The data context you're looking for is probably under the slick-composite-editor.component.ts file and there's some areas where it does deep copy, so I'm not sure if that's your cause of concern or not.

There's also some composite code in each editor (dateEditor, selectEditor, ...) but it's code for knowing if the editor is editable and if we should show/disable editor in the modal... so I don't think your issue would be in an editor but rather in the slick-composite-editor.component.ts

I already have plenty of Cypress tests and we've been using this Composite Editor in our Salesforce environment for couple years already, it's working fine for us.

[zewa666]

Yep I've been a bit in hurry yesterday, so I've added the repro as linked PR (nice to be able to use Stackblitz for quick repros).

I'll take a proper look myself as well ofc, just wanted to let you know about this one. Guess the cloning is a good hint of what perhaps went wrong. It most likely has to do with keeping the original one back for changed comparisions (highlighting modified editors)

[zewa666]

oh ok ... I think I got the issue.

When the save method is executed it will eventually call this.grid.getEditController()?.commitCurrentEdit(); which does further down the line this to handle a create:

// editing new item to add to dataset
const newItem = {};
self.currentEditor.applyValue(newItem, self.currentEditor.serializeValue());
self.makeActiveCellNormal(true);
self.triggerEvent(self.onAddNewRow, { item: newItem, column });  // <----------------------------------

so instead of keeping the original item around, it creates a new one and fills it up with the current serializeValue's from all composite editors and sends that to the onAddNewRow event. That ofc means my modification of the dataContext is discarded.

In cell mode though, the commitCurrentEdit gets called from the editors save method, and does not enter that above shown path as its not a new entry but an update of an existing row.

---

The problem I see here is that while this logically sounds like a bug:

• throwing away the original instance
• overwriting every property of the dataContext with the CURRENT status of every editor
• calling the addNew event with the NEW instance

it's most likely done that way due to the situation that it wasn't easily possible to pass in the original instance to the commitCurrentEdit method for a new record, since that function relies on getting the value from the dataset via getDataItem(activeRow)

[zewa666]

One approach to fix that would be the following. Allow the commitCurrentEdit to take an optional newItem to use in the case of a new row and call that in the save handler of the Composite Editor.

diff --git a/packages/common/src/core/slickGrid.ts b/packages/common/src/core/slickGrid.ts
index 7b3d5fab..2571b2dc 100644
--- a/packages/common/src/core/slickGrid.ts
+++ b/packages/common/src/core/slickGrid.ts
@@ -6261,7 +6261,7 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e

   // IEditor implementation for the editor lock

-  protected commitCurrentEdit(): boolean {
+  protected commitCurrentEdit(newItem: TData = {} as TData): boolean {
     const self = this as SlickGrid<TData, C, O>;
     const item = self.getDataItem(self.activeRow);
     const column = self.columns[self.activeCell];
@@ -6306,7 +6306,6 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
             }
           } else {
             // editing new item to add to dataset
-            const newItem = {};
             self.currentEditor.applyValue(newItem, self.currentEditor.serializeValue());
             self.makeActiveCellNormal(true);
             self.triggerEvent(self.onAddNewRow, { item: newItem, column });
diff --git a/packages/common/src/interfaces/editController.interface.ts b/packages/common/src/interfaces/editController.interface.ts
index 58c39b63..06dfc992 100644
--- a/packages/common/src/interfaces/editController.interface.ts
+++ b/packages/common/src/interfaces/editController.interface.ts
@@ -1,6 +1,6 @@
-export interface EditController {
+export interface EditController<TData = any> {
   /** Commit Current Editor command */
-  commitCurrentEdit: () => boolean;
+  commitCurrentEdit: (newItem?: TData) => boolean;

   /** Cancel Current Editor command */
   cancelCurrentEdit: () => boolean;
diff --git a/packages/composite-editor-component/src/slick-composite-editor.component.ts b/packages/composite-editor-component/src/slick-composite-editor.component.ts
index cf8cf251..a340c3fb 100644
--- a/packages/composite-editor-component/src/slick-composite-editor.component.ts
+++ b/packages/composite-editor-component/src/slick-composite-editor.component.ts
@@ -971,7 +971,7 @@ export class SlickCompositeEditorComponent implements ExternalResource {
         // commit the changes into the grid
         // if it's a "create" then it will triggered the "onAddNewRow" event which will in term push it to the grid
         // while an "edit" will simply applies the changes directly on the same row
-        let isFormValid = this.grid.getEditController()?.commitCurrentEdit();
+        let isFormValid = this.grid.getEditController()?.commitCurrentEdit(this._itemDataContext);

         // if the user provided the "onSave" callback, let's execute it with the item data context
         if (isFormValid && typeof this._options?.onSave === 'function') {

This issue here is though that I can't really tell how much of the code or the consumers already rely on this wrong behavior. So fixing it could now introduce potential other sideeffects

[ghiscoding]

hmm it's been quite a while since I've done this but I know 1 thing to really make sure if the Clone behavior, we need to make sure that the original data context (item row) is unaffected by the change. Also another thing to remember is what we didn't have SlickGrid code directly when I create this Composite couple years ago.

This issue here is though that I can't really tell how much of the code or the consumers already rely on this wrong behavior. So fixing it could now introduce potential other sideeffects

~I would go ahead with the change if it fixes your issue because if it's a bug then it's a bug even if the behavior is not the same. ~ ... wait commitCurrentEdit() never had any arguments, changing this to now require an argument is a huge change. Can't you go for an optional argument instead? It looks like you have 1 optional and the other is not!? Being optional, wouldn't introduce a breaking change, also I'm not too crazy about changing the signature of this method from the original, because that has been like this for years and there's a ton of Stack Overflow and other resources that reference this type of code, so I surely wouldn't want to deviate too much from the original code.

But anyway, in our Salesforce project where we use this Composite, we only use it for multiple changes (multiple row selections) and also for Mass Update, in other words we're not using the Create, neither the Clone (I've done the Clone because it wasn't much more difficult to add it when we already have the Create).

What about the Example 12 Cypress tests, do they all pass?

[zewa666]

hmm it's been quite a while since I've done this but I know 1 thing to really make sure if the Clone behavior, we need to make sure that the original data context (item row) is unaffected by the change.

yep makes sense for dirty tracking, but won't affect the create scenario.

Also another thing to remember is what we didn't have SlickGrid code directly when I create this Composite couple years ago.

This issue here is though that I can't really tell how much of the code or the consumers already rely on this wrong behavior. So fixing it could now introduce potential other sideeffects

~I would go ahead with the change if it fixes your issue because if it's a bug then it's a bug even if the behavior is not the same. ~ ... wait commitCurrentEdit() never had any arguments, changing this to now require an argument is a huge change. Can't you go for an optional argument instead? It looks like you have 1 optional and the other is not!? Being optional, wouldn't introduce a breaking change, also I'm not too crazy about changing the signature of this method from the original, because that has been like this for years and there's a ton of Stack Overflow and other resources that reference this type of code, so I surely wouldn't want to deviate too much from the original code.

the parameter is optional in both cases, just uses a default parameter (backwards compat) in the implementation.

but yeah my biggest gripes with this is also changing such an old well known API, hence what I initially said that people are already used to the "bug"

But anyway, in our Salesforce project where we use this Composite, we only use it for multiple changes (multiple row selections) and also for Mass Update, in other words we're not using the Create, neither the Clone (I've done the Clone because it wasn't much more difficult to add it when we already have the Create).

What about the Example 12 Cypress tests, do they all pass?

havent checked but I'm certain they would. I'll check back and let you know

[ghiscoding]

@zewa666 what will be the resolution for this issue? Is it because you've been too busy or does this have to go on next major?

[zewa666]

ups totally forgot about this one due to vacation, sickness and whatsonot 😅

we've postponed that feature for a while but I'll see whats the status next week

also there is no hurry on that one from my side. so if you're fine keeping this open for longer ok otherwise we can close and reopen later once I got a fix

[ghiscoding]

that's fine, we can leave it open until you have time to fix it, there's really no rush and I was just asking in case you forgot, which you did :)