UISACQCOMP-218: add "Amount must be a positive number" validation for "Set exchange rate" field. #815
Conversation
… "Set exchange rate" field.
|
@alisher-epam With the change you mentioned, Selection doesn't render the list of options if the dropdown is closed. It may just need to be opened before the assertions/actions are performed. Looking at to click the Selection control and open the dropdown prior to clicking the option. |
| @@ -36,13 +36,14 @@ const renderAffiliationsSelection = (props = {}) => render( | |||
| ); | |||
|
|
|||
| describe('AffiliationsSelection', () => { | |||
| it('should render affiliation selection with provided options', () => { | |||
| it('should render affiliation selection with provided options', async () => { | |||
| @@ -25,7 +24,7 @@ describe('CountryFilter component', () => { | |||
| expect(getByText('ui-organizations.filterConfig.country')).toBeDefined(); | |||
| }); | |||
|
|
|||
| it('should invoke onChange callback when something is selected', async () => { | |||
| it('should invoke onChange callback when something is selected', () => { | |||
| @@ -65,6 +65,14 @@ const CurrencyExchangeRateFields = ({ | |||
| resetExchangeRate(); | |||
| }, [change, resetExchangeRate, currencyFieldName]); | |||
|
|
|||
| const validateExchangeRate = (value) => { | |||
| if (value === undefined || value === null) { | |||
| return validateRequired(value); | |||
There was a problem hiding this comment.
where did you find such requirement? doing this you introduce inconsistency
| @@ -32,6 +32,14 @@ const renderFieldLocationFinalContainer = (props = {}, formProps = {}) => (rende | |||
| )); | |||
|
|
|||
| describe('FieldLocationFinalContainer component', () => { | |||
| beforeEach(() => { | |||
| renderFieldLocationFinalContainer(); | |||
There was a problem hiding this comment.
if you do it in before each, why do you keep render in tests? if you want to avoid such code duplication - update render function, it's not a place for beforeEach
| import React from 'react'; | ||
| import { render, cleanup, fireEvent } from '@testing-library/react'; | ||
| import { | ||
| cleanup, |
There was a problem hiding this comment.
you removed function call, but not import...
| @@ -146,7 +146,7 @@ const CurrencyExchangeRateFields = ({ | |||
| readOnly={!isExchangeRateEnabled} | |||
| tooltipText={tooltipTextExchangeRate} | |||
| required={isExchangeRateRequired} | |||
| validate={isExchangeRateRequired ? validateRequired : undefined} | |||
| validate={isExchangeRateRequired ? validateRequiredPositiveAmount : undefined} | |||
There was a problem hiding this comment.
is it expected to skip "positive amount" validation if the field is not required?
There was a problem hiding this comment.
Correct. If the user wants to set a custom exchange rate then the required checkbox should be checked
There was a problem hiding this comment.
when field is not required you just need to validate when value is populated, introduce another validation method (if doesn't exist) validatePositiveAmount
There was a problem hiding this comment.
Screen.Recording.2024-09-27.at.19.50.42.mp4
|
Purpose
UISACQCOMP-218: add "Amount must be a positive number" validation for "Set exchange rate" field.
Approach
Screen.Recording.2024-09-26.at.13.56.05.mp4
Pre-Merge Checklist
Before merging this PR, please go through the following list and take appropriate actions.
If there are breaking changes, please STOP and consider the following:
Ideally all of the PRs involved in breaking changes would be merged in the same day to avoid breaking the folio-testing environment. Communication is paramount if that is to be achieved, especially as the number of intermodule and inter-team dependencies increase.
While it's helpful for reviewers to help identify potential problems, ensuring that it's safe to merge is ultimately the responsibility of the PR assignee.