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

Composite Editor manipulating DataContext via editor save method not persisted #1618

Open
5 tasks done
zewa666 opened this issue Jul 23, 2024 · 11 comments
Open
5 tasks done

Comments

@zewa666
Copy link
Contributor

zewa666 commented Jul 23, 2024

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

@ghiscoding
Copy link
Owner

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
Copy link
Contributor Author

zewa666 commented Jul 23, 2024

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
Copy link
Owner

ghiscoding commented Jul 23, 2024

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

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
Copy link
Contributor Author

zewa666 commented Jul 24, 2024

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
Copy link
Contributor Author

zewa666 commented Jul 25, 2024

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
Copy link
Contributor Author

zewa666 commented Jul 25, 2024

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
Copy link
Owner

ghiscoding commented Jul 26, 2024

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
Copy link
Contributor Author

zewa666 commented Jul 26, 2024

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
Copy link
Owner

@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
Copy link
Contributor Author

zewa666 commented Aug 29, 2024

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
Copy link
Owner

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants