From 3fd0501b7a97cbdc4ce4880a2953676c5c6efcfd Mon Sep 17 00:00:00 2001 From: soleksy-splunk <143183665+soleksy-splunk@users.noreply.github.com> Date: Thu, 5 Dec 2024 18:43:55 +0100 Subject: [PATCH] feat(modifyFieldsOnValue): enable field to modify itself (#1494) **Issue number:** https://splunk.atlassian.net/browse/ADDON-76687 ### PR Type **What kind of change does this PR introduce?** * [x] Feature * [ ] Bug Fix * [ ] Refactoring (no functional or API changes) * [ ] Documentation Update * [ ] Maintenance (dependency updates, CI, etc.) ## Summary ### Changes Remove verification to block field from applying modification to itself after value change. It is not a risk to allow users to modify field itself as we should be concerned only for value. (it is still not a problem but seems like a bad approach to solving problems). It also enables circular dependencies in case where modifications are not about value. (modifying values still should work good but seems like a bad practise) ### User experience User can right now modify state of field by applying changes to currently edited field, it allows to create ie. much more useful labels and help messages referencing value change. ## Checklist If an item doesn't apply to your changes, leave it unchecked. * [ ] I have performed a self-review of this change according to the [development guidelines](https://splunk.github.io/addonfactory-ucc-generator/contributing/#development-guidelines) * [x] Tests have been added/modified to cover the changes [(testing doc)](https://splunk.github.io/addonfactory-ucc-generator/contributing/#build-and-test) * [ ] Changes are documented * [x] PR title and description follows the [contributing principles](https://splunk.github.io/addonfactory-ucc-generator/contributing/#pull-requests) --------- Co-authored-by: srv-rr-github-token <94607705+srv-rr-github-token@users.noreply.github.com> --- docs/entity/modifyFieldsOnValue.md | 10 +- .../global_config_validator.py | 74 ++++---- tests/unit/test_global_config_validator.py | 2 +- ...h_modification_circular_modifications.json | 22 +-- ...ig_with_modification_for_field_itself.json | 2 +- ...fig_with_modification_on_value_change.json | 4 + .../stories/BaseFormView.stories.tsx | 33 +++- ...ield-modify-itself-after-mods-chromium.png | 3 + ...eFormView-field-modify-itself-chromium.png | 3 + .../{ => tests}/BaseFormView.test.tsx | 12 +- .../tests/BaseFormViewModifications.test.tsx | 62 +++++++ .../BaseFormView/tests/configMocks.ts | 169 ++++++++++++++++++ 12 files changed, 341 insertions(+), 55 deletions(-) create mode 100644 ui/src/components/BaseFormView/stories/__images__/BaseFormView-field-modify-itself-after-mods-chromium.png create mode 100644 ui/src/components/BaseFormView/stories/__images__/BaseFormView-field-modify-itself-chromium.png rename ui/src/components/BaseFormView/{ => tests}/BaseFormView.test.tsx (92%) create mode 100644 ui/src/components/BaseFormView/tests/BaseFormViewModifications.test.tsx create mode 100644 ui/src/components/BaseFormView/tests/configMocks.ts diff --git a/docs/entity/modifyFieldsOnValue.md b/docs/entity/modifyFieldsOnValue.md index f379f1125..90c8abdb4 100644 --- a/docs/entity/modifyFieldsOnValue.md +++ b/docs/entity/modifyFieldsOnValue.md @@ -4,11 +4,11 @@ This feature allows to specify conditions to modify other fields based on curren ### Modification Object Properties -| Property | Type | Description | -| --------------------------------------------------- | ------ | ------------------------------------------------------------------------------------------------------------------- | -| fieldValue\* | string | Value that will trigger the update, put `[[any_other_value]]` to trigger update for any other values than specified | -| mode | string | Mode that adds possibility to use modification only on certain mode | -| fieldsToModify | array | List of fields modifications that will be applied after com ponent value will match | +| Property | Type | Description | +| --------------------------------------------------- | ------ | --------------------------------------------------------------------------------------------------------------------------------- | +| fieldValue\* | string | Value of current field that will trigger the update. Put `[[any_other_value]]` to make update for any other value than specified. | +| mode | string | Mode that adds possibility to use modification only on certain mode. One of ( `create` / `edit` / `clone` / `config` ) | +| fieldsToModify | array | List of fields modifications that will be applied after com ponent value will match. | ### fieldsToModify Properties diff --git a/splunk_add_on_ucc_framework/global_config_validator.py b/splunk_add_on_ucc_framework/global_config_validator.py index e50dc987a..b6ca1d87a 100644 --- a/splunk_add_on_ucc_framework/global_config_validator.py +++ b/splunk_add_on_ucc_framework/global_config_validator.py @@ -530,7 +530,7 @@ def _validate_groups(self) -> None: f"Service {service['name']} uses group field {group_field} which is not defined in entity" ) - def _is_circular( + def _is_circular_modification( self, mods: List[Any], visited: Dict[str, str], @@ -551,61 +551,73 @@ def _is_circular( # no more dependent modification fields visited[current_field] = DEAD_END return visited - else: - for influenced_field in current_field_mods["influenced_fields"]: - if influenced_field not in all_entity_fields: - raise GlobalConfigValidatorException( - f"""Modification in field '{current_field}' for not existing field '{influenced_field}'""" - ) - if influenced_field == current_field: - raise GlobalConfigValidatorException( - f"""Field '{current_field}' tries to modify itself""" - ) + + if current_field in current_field_mods["influenced_fields_value_change"]: + # field can modify itself except "value" property + raise GlobalConfigValidatorException( + f"""Field '{current_field}' tries to modify itself value""" + ) + + for influenced_field in current_field_mods["influenced_fields"]: + if influenced_field not in all_entity_fields: + raise GlobalConfigValidatorException( + f"""Modification in field '{current_field}' for not existing field '{influenced_field}'""" + ) + + if influenced_field in current_field_mods["influenced_fields_value_change"]: if visited[influenced_field] == VISITING: raise GlobalConfigValidatorException( f"""Circular modifications for field '{influenced_field}' in field '{current_field}'""" ) - else: - visited = self._is_circular( - mods, visited, all_entity_fields, influenced_field - ) + # check next influenced by value change field + visited = self._is_circular_modification( + mods, visited, all_entity_fields, influenced_field + ) + # All dependent modifications fields are dead_end visited[current_field] = DEAD_END return visited - def _check_if_circular( + def _check_if_circular_modification( self, all_entity_fields: List[Any], fields_with_mods: List[Any], modifications: List[Any], ) -> None: visited = {field: "not_visited" for field in all_entity_fields} - for start_field in fields_with_mods: # DFS algorithm for all fields with modifications - visited = self._is_circular( + visited = self._is_circular_modification( modifications, visited, all_entity_fields, start_field ) @staticmethod def _get_mods_data_for_single_entity( - fields_with_mods: List[Any], - all_modifications: List[Any], entity: Dict[str, Any], ) -> List[Any]: """ - Add modification entity data to lists and returns them + Get modification entity data as lists """ + entity_modifications = [] if "modifyFieldsOnValue" in entity: + influenced_fields_value_change = set() influenced_fields = set() - fields_with_mods.append(entity["field"]) for mods in entity["modifyFieldsOnValue"]: for mod in mods["fieldsToModify"]: influenced_fields.add(mod["fieldId"]) - all_modifications.append( - {"fieldId": entity["field"], "influenced_fields": influenced_fields} + + if ( + mod.get("value") is not None + ): # circular deps are not a problem if not about value + influenced_fields_value_change.add(mod["fieldId"]) + entity_modifications.append( + { + "fieldId": entity["field"], + "influenced_fields": influenced_fields, + "influenced_fields_value_change": influenced_fields_value_change, + } ) - return [fields_with_mods, all_modifications] + return entity_modifications @staticmethod def _get_all_entities( @@ -638,11 +650,13 @@ def _get_all_modification_data( entities = self._get_all_entities(collections) for entity in entities: - self._get_mods_data_for_single_entity( - fields_with_mods, all_modifications, entity - ) all_fields.append(entity["field"]) + if "modifyFieldsOnValue" in entity: + fields_with_mods.append(entity["field"]) + entity_mods = self._get_mods_data_for_single_entity(entity) + all_modifications.extend(entity_mods) + return [fields_with_mods, all_modifications, all_fields] def _validate_field_modifications(self) -> None: @@ -664,7 +678,7 @@ def _validate_field_modifications(self) -> None: all_fields_config, ) = self._get_all_modification_data(tabs) - self._check_if_circular( + self._check_if_circular_modification( all_fields_config, fields_with_mods_config, all_modifications_config ) @@ -678,7 +692,7 @@ def _validate_field_modifications(self) -> None: all_fields_inputs, ) = self._get_all_modification_data(services) - self._check_if_circular( + self._check_if_circular_modification( all_fields_inputs, fields_with_mods_inputs, all_modifications_inputs ) diff --git a/tests/unit/test_global_config_validator.py b/tests/unit/test_global_config_validator.py index 43dc3b9f2..185dc069e 100644 --- a/tests/unit/test_global_config_validator.py +++ b/tests/unit/test_global_config_validator.py @@ -337,7 +337,7 @@ def test_config_validation_modifications_on_change(): [ ( "invalid_config_with_modification_for_field_itself.json", - "Field 'text1' tries to modify itself", + "Field 'text1' tries to modify itself value", ), ( "invalid_config_with_modification_for_unexisiting_fields.json", diff --git a/tests/unit/testdata/invalid_config_with_modification_circular_modifications.json b/tests/unit/testdata/invalid_config_with_modification_circular_modifications.json index 3c8e0406f..e662dea54 100644 --- a/tests/unit/testdata/invalid_config_with_modification_circular_modifications.json +++ b/tests/unit/testdata/invalid_config_with_modification_circular_modifications.json @@ -53,7 +53,7 @@ "fieldsToModify": [ { "fieldId": "text2", - "disabled": false + "value": "modification" } ] }, @@ -62,7 +62,7 @@ "fieldsToModify": [ { "fieldId": "text3", - "disabled": true + "value": "modification" } ] } @@ -79,7 +79,7 @@ "fieldsToModify": [ { "fieldId": "text3", - "disabled": false + "value": "modification" } ] }, @@ -88,7 +88,7 @@ "fieldsToModify": [ { "fieldId": "text4", - "disabled": true + "value": "modification" } ] } @@ -111,7 +111,7 @@ "fieldsToModify": [ { "fieldId": "text5", - "disabled": false + "value": "modification" } ] }, @@ -120,7 +120,7 @@ "fieldsToModify": [ { "fieldId": "text5", - "disabled": true + "value": "modification" } ] } @@ -137,7 +137,7 @@ "fieldsToModify": [ { "fieldId": "text6", - "disabled": false + "value": "modification" } ] }, @@ -146,7 +146,7 @@ "fieldsToModify": [ { "fieldId": "text6", - "disabled": true + "value": "modification" } ] } @@ -163,7 +163,7 @@ "fieldsToModify": [ { "fieldId": "text7", - "disabled": false + "value": "modification" } ] }, @@ -172,7 +172,7 @@ "fieldsToModify": [ { "fieldId": "text7", - "disabled": true + "value": "modification" } ] } @@ -189,7 +189,7 @@ "fieldsToModify": [ { "fieldId": "text1", - "disabled": false + "value": "modification" } ] } diff --git a/tests/unit/testdata/invalid_config_with_modification_for_field_itself.json b/tests/unit/testdata/invalid_config_with_modification_for_field_itself.json index eb7145f94..a87aabccf 100644 --- a/tests/unit/testdata/invalid_config_with_modification_for_field_itself.json +++ b/tests/unit/testdata/invalid_config_with_modification_for_field_itself.json @@ -53,7 +53,7 @@ "fieldsToModify": [ { "fieldId": "text1", - "disabled": false + "value": "value modification" } ] } diff --git a/tests/unit/testdata/valid_config_with_modification_on_value_change.json b/tests/unit/testdata/valid_config_with_modification_on_value_change.json index f39158c62..13b803d2c 100644 --- a/tests/unit/testdata/valid_config_with_modification_on_value_change.json +++ b/tests/unit/testdata/valid_config_with_modification_on_value_change.json @@ -54,6 +54,10 @@ { "fieldId": "text2", "disabled": false + }, + { + "fieldId": "text1", + "label": "change label for itself" } ] }, diff --git a/ui/src/components/BaseFormView/stories/BaseFormView.stories.tsx b/ui/src/components/BaseFormView/stories/BaseFormView.stories.tsx index 43b7d1c5e..b49967885 100644 --- a/ui/src/components/BaseFormView/stories/BaseFormView.stories.tsx +++ b/ui/src/components/BaseFormView/stories/BaseFormView.stories.tsx @@ -1,6 +1,7 @@ import React from 'react'; import type { Meta, StoryObj } from '@storybook/react'; -import { fn } from '@storybook/test'; +import { fn, userEvent, within } from '@storybook/test'; + import BaseFormView from '../BaseFormView'; import { PAGE_CONFIG_BOTH_OAUTH, @@ -17,6 +18,8 @@ import { getGlobalConfigMockGroupsForConfigPage, getGlobalConfigMockModificationToGroupsConfig, } from '../BaseFormConfigMock'; +import { getGlobalConfigMockModificationToFieldItself } from '../tests/configMocks'; +import { invariant } from '../../../util/invariant'; interface BaseFormStoriesProps extends BaseFormProps { config: GlobalConfig; @@ -143,3 +146,31 @@ export const GroupModificationsConfig: Story = { platform: 'cloud', }, }; + +export const FieldModifyItself: Story = { + args: { + currentServiceState: {}, + serviceName: 'account', + mode: 'create' as Mode, + page: 'configuration', + stanzaName: 'unknownStanza', + handleFormSubmit: fn(), + config: getGlobalConfigMockModificationToFieldItself(), + }, +}; + +export const FieldModifyItselfAfterMods: Story = { + args: FieldModifyItself.args, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + + const modifyInputText = canvas + .getAllByRole('textbox') + .find((el) => el.getAttribute('value') === 'default value'); + + invariant(modifyInputText, 'modification input field should be defined'); + + await userEvent.clear(modifyInputText); + await userEvent.type(modifyInputText, 'modify itself'); + }, +}; diff --git a/ui/src/components/BaseFormView/stories/__images__/BaseFormView-field-modify-itself-after-mods-chromium.png b/ui/src/components/BaseFormView/stories/__images__/BaseFormView-field-modify-itself-after-mods-chromium.png new file mode 100644 index 000000000..3b7febda1 --- /dev/null +++ b/ui/src/components/BaseFormView/stories/__images__/BaseFormView-field-modify-itself-after-mods-chromium.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:7bb703699c8c301f552a38459fb9b237d1f7a4281ce011d3e3fa1ce4592917c0 +size 22951 diff --git a/ui/src/components/BaseFormView/stories/__images__/BaseFormView-field-modify-itself-chromium.png b/ui/src/components/BaseFormView/stories/__images__/BaseFormView-field-modify-itself-chromium.png new file mode 100644 index 000000000..1a20c14c2 --- /dev/null +++ b/ui/src/components/BaseFormView/stories/__images__/BaseFormView-field-modify-itself-chromium.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:d8f806fad0fff2f0417063c3d1638107f87f3edbcc373218c37ee413df522109 +size 19293 diff --git a/ui/src/components/BaseFormView/BaseFormView.test.tsx b/ui/src/components/BaseFormView/tests/BaseFormView.test.tsx similarity index 92% rename from ui/src/components/BaseFormView/BaseFormView.test.tsx rename to ui/src/components/BaseFormView/tests/BaseFormView.test.tsx index 533c388aa..63b31af09 100644 --- a/ui/src/components/BaseFormView/BaseFormView.test.tsx +++ b/ui/src/components/BaseFormView/tests/BaseFormView.test.tsx @@ -2,16 +2,16 @@ import { render, screen } from '@testing-library/react'; import React from 'react'; import userEvent from '@testing-library/user-event'; -import { getGlobalConfigMock } from '../../mocks/globalConfigMock'; -import { setUnifiedConfig } from '../../util/util'; -import BaseFormView from './BaseFormView'; -import { getBuildDirPath } from '../../util/script'; -import mockCustomControlMockForTest from '../CustomControl/CustomControlMockForTest'; +import { getGlobalConfigMock } from '../../../mocks/globalConfigMock'; +import { getBuildDirPath } from '../../../util/script'; +import { setUnifiedConfig } from '../../../util/util'; import { getGlobalConfigMockCustomControl, getGlobalConfigMockGroupsForInputPage, getGlobalConfigMockGroupsForConfigPage, -} from './BaseFormConfigMock'; +} from '../BaseFormConfigMock'; +import mockCustomControlMockForTest from '../../CustomControl/CustomControlMockForTest'; +import BaseFormView from '../BaseFormView'; const handleFormSubmit = jest.fn(); diff --git a/ui/src/components/BaseFormView/tests/BaseFormViewModifications.test.tsx b/ui/src/components/BaseFormView/tests/BaseFormViewModifications.test.tsx new file mode 100644 index 000000000..bdbcb4800 --- /dev/null +++ b/ui/src/components/BaseFormView/tests/BaseFormViewModifications.test.tsx @@ -0,0 +1,62 @@ +import { render, screen, within } from '@testing-library/react'; +import React from 'react'; +import userEvent from '@testing-library/user-event'; + +import { setUnifiedConfig } from '../../../util/util'; +import BaseFormView from '../BaseFormView'; +import { getGlobalConfigMockModificationToFieldItself } from './configMocks'; +import { invariant } from '../../../util/invariant'; + +const handleFormSubmit = jest.fn(); + +const PAGE_CONF = 'configuration'; +const SERVICE_NAME = 'account'; +const STANZA_NAME = 'stanzaName'; + +it('should modify correctly all properties of field, self modification', async () => { + const mockConfig = getGlobalConfigMockModificationToFieldItself(); + setUnifiedConfig(mockConfig); + + render( + + ); + + await screen.findByText('default label'); + + const modifyTextField = document.querySelector( + '[data-name="text_field_with_modifications"]' + ) as HTMLElement; + + expect(modifyTextField).toBeInTheDocument(); + + invariant(modifyTextField, 'modification field should be defined'); + + expect(within(modifyTextField).getByTestId('help')).toHaveTextContent('default help'); + expect(within(modifyTextField).getByTestId('label')).toHaveTextContent('default label'); + expect(within(modifyTextField).getByTestId('msg-markdown')).toHaveTextContent( + 'default markdown message' + ); + expect(within(modifyTextField).queryByText('*')).not.toBeInTheDocument(); + + const inputComponent = within(modifyTextField).getByRole('textbox'); + await userEvent.clear(inputComponent); + await userEvent.type(inputComponent, 'modify itself'); + + expect(within(modifyTextField).getByTestId('help')).toHaveTextContent( + 'help after modification' + ); + expect(within(modifyTextField).getByTestId('label')).toHaveTextContent( + 'label after modification' + ); + expect(within(modifyTextField).getByTestId('msg-markdown')).toHaveTextContent( + 'markdown message after modification' + ); + expect(within(modifyTextField).queryByText('*')).toBeInTheDocument(); +}); diff --git a/ui/src/components/BaseFormView/tests/configMocks.ts b/ui/src/components/BaseFormView/tests/configMocks.ts new file mode 100644 index 000000000..c6c338e99 --- /dev/null +++ b/ui/src/components/BaseFormView/tests/configMocks.ts @@ -0,0 +1,169 @@ +import { z } from 'zod'; +import { GlobalConfigSchema } from '../../../types/globalConfig/globalConfig'; + +const CONFIG_MOCK_MODIFICATION_ON_VALUE_CHANGE_CONFIG = { + pages: { + configuration: { + tabs: [ + { + name: 'account', + table: { + actions: ['edit', 'delete', 'clone'], + header: [ + { + label: 'Name', + field: 'name', + }, + ], + }, + entity: [ + { + type: 'text', + label: 'Name', + validators: [ + { + type: 'regex', + errorMsg: + 'Account Name must begin with a letter and consist exclusively of alphanumeric characters and underscores.', + pattern: '^[a-zA-Z]\\w*$', + }, + { + type: 'string', + errorMsg: 'Length of input name should be between 1 and 100', + minLength: 1, + maxLength: 100, + }, + ], + field: 'name', + help: 'A unique name for the account.', + required: true, + }, + { + type: 'text', + label: 'Example text field', + field: 'text_field_with_modifications', + help: 'Example text field with modification', + required: false, + defaultValue: 'default value', + modifyFieldsOnValue: [ + { + fieldValue: 'default value', + fieldsToModify: [ + { + fieldId: 'text_field_with_modifications', + disabled: false, + required: false, + help: 'default help', + label: 'default label', + markdownMessage: { + text: 'default markdown message', + }, + }, + ], + }, + { + fieldValue: 'modify itself', + fieldsToModify: [ + { + fieldId: 'text_field_with_modifications', + disabled: false, + required: true, + help: 'help after modification', + label: 'label after modification', + markdownMessage: { + text: 'markdown message after modification', + }, + }, + ], + }, + ], + }, + { + type: 'text', + label: 'Example text field to be modified', + field: 'text_field_to_be_modified', + help: 'Example text field to be modified', + required: false, + modifyFieldsOnValue: [ + { + fieldValue: '[[any_other_value]]', + fieldsToModify: [ + { + fieldId: 'text_field_to_be_modified', + required: true, + }, + ], + }, + ], + }, + ], + title: 'Accounts', + }, + ], + title: 'Configuration', + description: 'Set up your add-on', + }, + inputs: { + services: [ + { + name: 'demo_input', + entity: [ + { + type: 'text', + label: 'Name', + validators: [ + { + type: 'regex', + errorMsg: + 'Input Name must begin with a letter and consist exclusively of alphanumeric characters and underscores.', + pattern: '^[a-zA-Z]\\w*$', + }, + { + type: 'string', + errorMsg: 'Length of input name should be between 1 and 100', + minLength: 1, + maxLength: 100, + }, + ], + field: 'name', + help: 'A unique name for the data input.', + required: true, + encrypted: false, + }, + ], + title: 'demo_input', + }, + ], + title: 'Inputs', + description: 'Manage your data inputs', + table: { + actions: ['edit', 'delete', 'clone'], + header: [ + { + label: 'Name', + field: 'name', + }, + ], + moreInfo: [ + { + label: 'Name', + field: 'name', + }, + ], + }, + }, + }, + meta: { + name: 'demo_addon_for_splunk', + restRoot: 'demo_addon_for_splunk', + version: '5.31.1R85f0e18e', + displayName: 'Demo Add-on for Splunk', + schemaVersion: '0.0.3', + checkForUpdates: false, + searchViewDefault: false, + }, +} satisfies z.input; + +export function getGlobalConfigMockModificationToFieldItself() { + return GlobalConfigSchema.parse(CONFIG_MOCK_MODIFICATION_ON_VALUE_CHANGE_CONFIG); +}