Skip to content

EditableDetailPanel: compare form values against model with update columns#1959

Open
labkey-nicka wants to merge 7 commits intodevelopfrom
fb_mv_edit_960
Open

EditableDetailPanel: compare form values against model with update columns#1959
labkey-nicka wants to merge 7 commits intodevelopfrom
fb_mv_edit_960

Conversation

@labkey-nicka
Copy link
Contributor

@labkey-nicka labkey-nicka commented Mar 20, 2026

Rationale

This addresses #960 by refactoring EditableDetailPanel to compare form values against a query model row that contains all update columns.

Related Pull Requests

Changes

  • Factor EditingForm out of EditableDetailPanel and load model with update columns. This model is subsequently used for value comparison.
  • Update extractChanges() to account for column.jsonType === 'array'
  • Refactor arrayEquals to fix edge cases of array mutation and delimiter collisions

@labkey-nicka labkey-nicka requested a review from cnathe March 20, 2026 21:38
@labkey-nicka labkey-nicka self-assigned this Mar 20, 2026
Copy link
Contributor

@cnathe cnathe left a comment

Choose a reason for hiding this comment

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

Code changes and refactor look good. Minor feedback.

test('does not mutate original arrays', () => {
const arrA = ['b', 'a'];
const arrB = ['a', 'b'];
arrayEquals(arrA, arrB, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to check the other iterations / combinations of the 3rd and 4th params here? I assume not, just checking.

});

test('handles duplicate elements correctly with ignoreOrder', () => {
expect(arrayEquals(['a', 'a', 'b'], ['a', 'b', 'b'], true)).toBeFalsy();
Copy link
Contributor

Choose a reason for hiding this comment

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

is it worth a quick check as well that flipping the ignoreOrder boolean gives the expected behavior as well?

const queryConfigs = useMemo(() => ({ model: queryConfig }), [queryConfig]);
const { keyValue, schemaQuery } = queryConfig;
const { schemaName, queryName } = schemaQuery;
const key = `${schemaName}.${queryName}.${keyValue}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we had some helper on QueryConfig or QueryModel for getting this type of key value, but I'm not seeing it.

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

Successfully merging this pull request may close these issues.

2 participants