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

fix(editor): Fix Remove all fields not removing values in resource mapper #6940

Merged
merged 7 commits into from
Aug 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
:key="action.value"
:command="action.value"
:disabled="action.disabled"
:data-test-id="`action-${action.value}`"
>
{{ action.label }}
<div :class="$style.iconContainer">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
<div
v-if="$slots.options"
:class="{ [$style.options]: true, [$style.visible]: showOptions }"
data-test-id="parameter-input-options-container"
:data-test-id="`${inputName}-parameter-input-options-container`"
>
<slot name="options" />
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<template>
<div>
<div data-test-id="parameter-input">
<parameter-input
ref="param"
:inputSize="inputSize"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import type {
ResourceMapperValue,
} from 'n8n-workflow';
import ParameterInputFull from '@/components/ParameterInputFull.vue';
import ParameterIssues from '../ParameterIssues.vue';
import ParameterOptions from '../ParameterOptions.vue';
import ParameterIssues from '@/components//ParameterIssues.vue';
import ParameterOptions from '@/components//ParameterOptions.vue';
import { computed } from 'vue';
import { i18n as locale } from '@/plugins/i18n';
import { useNDVStore } from '@/stores';
Expand Down Expand Up @@ -276,6 +276,7 @@ defineExpose({
:size="labelSize"
:showOptions="true"
:showExpressionSelector="false"
inputName="columns"
color="text-dark"
>
<template #options>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
<script setup lang="ts">
import type {
INodeProperties,
INodePropertyTypeOptions,
ResourceMapperField,
ResourceMapperFields,
} from 'n8n-workflow';
import { computed, reactive, watch } from 'vue';
import { i18n as locale } from '@/plugins/i18n';
import { useNodeSpecificationValues } from '@/composables';
import ParameterOptions from '@/components/ParameterOptions.vue';

interface Props {
parameter: INodeProperties;
initialValue: string[];
fieldsToMap: ResourceMapperFields['fields'];
typeOptions: INodePropertyTypeOptions | undefined;
Expand All @@ -17,6 +20,7 @@ interface Props {
loading: boolean;
serviceName: string;
teleported?: boolean;
refreshInProgress: boolean;
}

const props = withDefaults(defineProps<Props>(), {
Expand Down Expand Up @@ -47,6 +51,7 @@ watch(

const emit = defineEmits<{
(event: 'matchingColumnsChanged', value: string[]): void;
(event: 'refreshFieldList'): void;
}>();

const availableMatchingFields = computed<ResourceMapperField[]>(() => {
Expand Down Expand Up @@ -103,6 +108,27 @@ const fieldTooltip = computed<string>(() => {
});
});

const parameterActions = computed<Array<{ label: string; value: string; disabled?: boolean }>>(
() => {
return [
{
label: locale.baseText('resourceMapper.refreshFieldList', {
interpolate: { fieldWord: singularFieldWordCapitalized.value },
}),
value: 'refreshFieldList',
},
];
},
);

const fetchingFieldsLabel = computed<string>(() => {
return locale.baseText('resourceMapper.fetchingFields.message', {
interpolate: {
fieldWord: pluralFieldWord.value,
},
});
});

function onSelectionChange(value: string | string[]) {
if (resourceMapperTypeOptions.value?.multiKeyMatch === true) {
state.selected = value as string[];
Expand All @@ -121,6 +147,16 @@ function emitValueChanged() {
}
}

function onParameterActionSelected(action: string): void {
switch (action) {
case 'refreshFieldList':
emit('refreshFieldList');
break;
default:
break;
}
}

defineExpose({
state,
});
Expand All @@ -137,6 +173,15 @@ defineExpose({
:size="labelSize"
color="text-dark"
>
<template #options>
<parameter-options
:parameter="parameter"
:customActions="parameterActions"
:loading="props.refreshInProgress"
:loadingMessage="fetchingFieldsLabel"
@update:modelValue="onParameterActionSelected"
/>
</template>
<n8n-select
:multiple="resourceMapperTypeOptions?.multiKeyMatch === true"
:modelValue="state.selected"
Expand Down
56 changes: 32 additions & 24 deletions packages/editor-ui/src/components/ResourceMapper/ResourceMapper.vue
Original file line number Diff line number Diff line change
Expand Up @@ -348,18 +348,40 @@ function removeField(name: string): void {
}
const fieldName = parseResourceMapperFieldName(name);
if (fieldName) {
if (state.paramValue.value) {
delete state.paramValue.value[fieldName];
const field = state.paramValue.schema.find((f) => f.id === fieldName);
if (field) {
field.removed = true;
state.paramValue.schema.splice(state.paramValue.schema.indexOf(field), 1, field);
}
const field = state.paramValue.schema.find((f) => f.id === fieldName);
if (field) {
deleteField(field);
emitValueChanged();
}
}
}

function removeAllFields(): void {
state.paramValue.schema.forEach((field) => {
if (
!fieldCannotBeDeleted(
field,
showMatchingColumnsSelector.value,
resourceMapperMode.value,
matchingColumns.value,
)
) {
deleteField(field);
}
});
emitValueChanged();
}

// Delete a single field from the mapping (set removed flag to true and delete from value)
// Used when removing one or all fields
function deleteField(field: ResourceMapperField): void {
if (state.paramValue.value) {
delete state.paramValue.value[field.id];
field.removed = true;
state.paramValue.schema.splice(state.paramValue.schema.indexOf(field), 1, field);
}
}

function addField(name: string): void {
if (name === 'addAllFields') {
return addAllFields();
Expand Down Expand Up @@ -395,23 +417,6 @@ function addAllFields(): void {
emitValueChanged();
}

function removeAllFields(): void {
state.paramValue.schema.forEach((field) => {
if (
!fieldCannotBeDeleted(
field,
showMatchingColumnsSelector.value,
resourceMapperMode.value,
matchingColumns.value,
)
) {
field.removed = true;
state.paramValue.schema.splice(state.paramValue.schema.indexOf(field), 1, field);
}
});
emitValueChanged();
}

function emitValueChanged(): void {
pruneParamValues();
emit('valueChanged', {
Expand Down Expand Up @@ -457,6 +462,7 @@ defineExpose({
/>
<matching-columns-select
v-if="showMatchingColumnsSelector"
:parameter="props.parameter"
:label-size="labelSize"
:fieldsToMap="state.paramValue.schema"
:typeOptions="props.parameter.typeOptions"
Expand All @@ -465,7 +471,9 @@ defineExpose({
:initialValue="matchingColumns"
:serviceName="nodeType?.displayName || locale.baseText('generic.service')"
:teleported="teleported"
:refreshInProgress="state.refreshInProgress"
@matchingColumnsChanged="onMatchingColumnsChanged"
@refreshFieldList="initFetching(true)"
/>
<n8n-text v-if="!showMappingModeSelect && state.loading" size="small">
<n8n-icon icon="sync-alt" size="xsmall" :spin="true" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
MAPPING_COLUMNS_RESPONSE,
NODE_PARAMETER_VALUES,
UPDATED_SCHEMA,
getLatestValueChangeEvent,
} from './utils/ResourceMapper.utils';
import { useNodeTypesStore } from '@/stores/nodeTypes.store';
import { waitAllPromises } from '@/__tests__/utils';
Expand All @@ -13,7 +14,8 @@ import { createComponentRenderer } from '@/__tests__/render';
import type { SpyInstance } from 'vitest';

let nodeTypeStore: ReturnType<typeof useNodeTypesStore>;
let fetchFieldsSpy: SpyInstance, resolveParameterSpy: SpyInstance;
let fetchFieldsSpy: SpyInstance;
let resolveParameterSpy: SpyInstance;

const renderComponent = createComponentRenderer(ResourceMapper, DEFAULT_SETUP);

Expand Down Expand Up @@ -289,4 +291,55 @@ describe('ResourceMapper.vue', () => {
await waitAllPromises();
expect(fetchFieldsSpy).not.toHaveBeenCalled();
});

it('should delete fields from UI and parameter value when they are deleted', async () => {
const { getByTestId, emitted } = renderComponent({
props: {
node: {
parameters: {
columns: {
schema: null,
},
},
},
},
});
await waitAllPromises();
// Add some values so we can test if they are gone after deletion
const idInput = getByTestId('parameter-input-value["id"]').querySelector('input');
const firstNameInput = getByTestId('parameter-input-value["First name"]').querySelector(
'input',
);
const lastNameInput = getByTestId('parameter-input-value["Last name"]').querySelector('input');
const usernameInput = getByTestId('parameter-input-value["Username"]').querySelector('input');
const addressInput = getByTestId('parameter-input-value["Address"]').querySelector('input');
if (idInput && firstNameInput && lastNameInput && usernameInput && addressInput) {
await userEvent.type(idInput, '123');
await userEvent.type(firstNameInput, 'John');
await userEvent.type(lastNameInput, 'Doe');
await userEvent.type(usernameInput, 'johndoe');
await userEvent.type(addressInput, '123 Main St');
// All field values should be in parameter value
const valueBeforeRemove = getLatestValueChangeEvent(emitted());
expect(valueBeforeRemove[0].value.value).toHaveProperty('id');
expect(valueBeforeRemove[0].value.value).toHaveProperty('First name');
expect(valueBeforeRemove[0].value.value).toHaveProperty('Last name');
expect(valueBeforeRemove[0].value.value).toHaveProperty('Username');
expect(valueBeforeRemove[0].value.value).toHaveProperty('Address');
// Click on 'Remove all fields' option
await userEvent.click(getByTestId('columns-parameter-input-options-container'));
await userEvent.click(getByTestId('action-removeAllFields'));
// Should delete all non-mandatory fields:
// 1. From UI
expect(
getByTestId('resource-mapper-container').querySelectorAll('.parameter-item').length,
).toBe(3);
// 2. And their values from parameter value
const valueAfterRemove = getLatestValueChangeEvent(emitted());
expect(valueAfterRemove[0].value.value).not.toHaveProperty('Username');
expect(valueAfterRemove[0].value.value).not.toHaveProperty('Address');
} else {
throw new Error('Could not find input fields');
}
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { SETTINGS_STORE_DEFAULT_STATE } from '@/__tests__/utils';
import { STORES } from '@/constants';
import { createTestingPinia } from '@pinia/testing';
import { merge } from 'lodash-es';
import type { ResourceMapperFields } from 'n8n-workflow';
import type { ResourceMapperFields, ResourceMapperValue } from 'n8n-workflow';

export const NODE_PARAMETER_VALUES = {
authentication: 'oAuth2',
Expand Down Expand Up @@ -201,15 +201,16 @@ export const MAPPING_COLUMNS_RESPONSE: ResourceMapperFields = {
type: 'string',
canBeUsedToMatch: true,
},
{
id: 'Last Name',
displayName: 'Last Name',
match: false,
required: false,
defaultMatch: false,
display: true,
type: 'string',
canBeUsedToMatch: true,
},
],
};

// Gets the latest `valueChanged` event emitted by the component
// This will be used to inspect current resource mapper value set in node parameters
export function getLatestValueChangeEvent(emitted: Record<string, unknown[]>) {
const valueChangeEmits = emitted.valueChanged as [];
return valueChangeEmits[valueChangeEmits.length - 1] as Array<{
name: string;
value: ResourceMapperValue;
node: string;
}>;
}