Personal details - DOB field updating once year or month has been changed #18577

burczu · 2023-05-08T13:10:14Z

Details

This PR changes the behaviour of the Calendar Picker component in the way where not only selecting a day means selecting the whole date but also changing a year or month selects the date. In case of selecting an invalid date, the error message is displayed.

Fixed Issues

$ #16514
PROPOSAL: #16514 (comment)

Tests

Happy path

Login to the app
Select: Settings -> Profile -> Personal details -> Date of birth
Select some date
Verify if date selected in the calendar is the same as the date in the input above
Click Save button
Check if date is saved correctly

Changing a month

Login to the app
Select: Settings -> Profile -> Personal details -> Date of birth
Select 1984-03-31
Change the month to February
Check the input field - it should contain incorrect value of 1984-02-31
Check the calendar - there should be no date selected
Click Save button
Verify if an error message appeared below input field and the save was discarded
Select some correct date
Click Save
Check if date is saved correctly

Changing a year

Login to the app
Select: Settings -> Profile -> Personal details -> Date of birth
Select 1984-02-29
Change the year to 1983
Check the input field - it should contain an incorrect value of 1983-02-29
Check the calendar - there should be no date selected
Click Save button
Verify if an error message appeared below input field and the save was discarded
Select some correct date
Click Save
Check if date is saved correctly

Verify that no errors appear in the JS console

Offline tests

The same as above.

QA Steps

The same as above.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

screen-capture-16514.mp4

Mobile Web - Chrome

screen-capture-16514-android-web.mp4

Mobile Web - Safari

screen-capture-16514-ios-web.mp4

Desktop

screen-capture-16514-desktop.mp4

iOS

screen-capture-16514-ios.mp4

Android

screen-capture-16514-android.mp4

src/components/CalendarPicker/index.js

melvin-bot · 2023-05-09T11:00:36Z

@roryabraham @Santhosh-Sellavel One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

Santhosh-Sellavel · 2023-05-11T17:59:51Z

src/components/CalendarPicker/index.js

@@ -19,7 +19,9 @@ class CalendarPicker extends React.PureComponent {
    constructor(props) {
        super(props);

-        let currentDateView = props.value;
+        const currentSelection = moment(props.value);


Don't we need to pass a format for date to parse the value here?

No need to format it cause it is not used to display date itself - the date that is displayed in the input is build from state string values selectedYear, selectedMonth and selectedDay (we need to store them this way cause we need to allow incorrect dates in the input field).

The currentSelection moment object is used only temporarily to make translate props.value date into these three state values.

props.value is the date string. If then don't we need to pass a format for parsing value from the string correctly

https://momentjs.com/guides/#/parsing/known-formats/

Ahhh, right - it's not necessary but it is recommended to pass it as a constructor parameter... Probably to make it easier for parser to parse the date (it's pitty they didn't explain the reason in the docs). I'll add it, then.

Santhosh-Sellavel · 2023-05-11T18:19:55Z

@burczu

If the steps are the same across Test, offline, and QA Steps sections. we can just leave a comment the same as the other section instead of repeating it.

Santhosh-Sellavel · 2023-05-11T18:36:48Z

@burczu

Refer to the below-linked video

Issues_DatePICKET.mov

Initially Date picked was 1998-03-31
Pressing Prev should set the date as 1998-02-31 in the field, no date selection in the calendar and show an error
But it is set as 1998-02-02 ❌
Pressed Next and set the date as 1998-03-31
Now pressing Prev set the date as 1998-02-31 ✅ , no date selection in the calendar ✅. But didn't show an error ❌
Pressing Save shows the error ✅
Now select a valid date i.e 1998-03-31, now error gone ✅
Now pressing Prev set the date as 1998-02-31 ✅ , no date selection in the calendar ✅ and shown the error ✅

From Step 1 to Step 5, made the date valid to invalid but no error was shown, but after Step 6 we show an error. But we made the date valid in step 7 and made it invalid in step 8 and this time we show an error.

Whenever there is invalid date we should show error straightaway as discussed here

Also there is a weird mix-up on step 3 date was set wrongly.

cc: @roryabraham

burczu · 2023-05-12T08:55:09Z

Hey @Santhosh-Sellavel! Thanks for your feedback. All the issues shown in your video are addressed now (date changes correctly, validation is now performed on every date change). Please re-check it, thanks.

Santhosh-Sellavel · 2023-05-12T19:35:38Z

src/components/NewDatePicker/index.js

     */
    setDate(selectedDate) {
        this.setState({selectedDate}, () => {
            this.props.onInputChange(selectedDate);
+            this.props.onBlur(); // force validation on every date change


This is a weird or sort of hacky thing to do. This will make validation occur twice.

The problem is in the Form Component. Specifically here

App/src/components/Form.js

Line 196 in 756e9d8

const errors = _.pick(validationErrors, (inputValue, inputID) => Boolean(this.touchedInputs[inputID]));

The touchedInputs is not set.

We set touchedInputs here on blur. This makes sense as we don't want to show an error on every change in the input, instead only when focusing on a different input in the form.

App/src/components/Form.js

Lines 296 to 303 in 756e9d8

onBlur: (event) => {

// We delay the validation in order to prevent Checkbox loss of focus when

// the user are focusing a TextInput and proceeds to toggle a CheckBox in

// web and mobile web platforms.

setTimeout(() => {

this.setTouchedInput(inputID);

this.validate(this.state.inputValues);

}, 200);

But in our case validation needs in every change maybe we should look for a way to set the touchedInputs once or prop to just to show validation errors without considering the touchedInputs

your thoughts? @burczu @roryabraham

I wouldn't say it's hack-ish solution here - custom form fields like this date picker should decide on their own when they are considered "blurred", and call suitable event handler then.

Of course, it is also connected to the touched state, but I think this state is more internal thing from the form perspective. What I think as an alternative solution is to provide the onTouched event handler, that could be called by the form field instead of calling onBlur but effectively it would be the same thing done in slightly different way.

What I think as an alternative solution is to provide the onTouched event handler, that could be called by the form field instead of calling onBlur but effectively it would be the same thing done in slightly different way.

@burczu Can you elaborate more with implementation details?

@roryabraham Your thoughts so far?

@Santhosh-Sellavel Here, in the Form.js:

App/src/components/Form.js

Lines 285 to 329 in dd5fbb6

return React.cloneElement(child, {

ref: (node) => {

this.inputRefs[inputID] = node;

const {ref} = child;

if (_.isFunction(ref)) {

ref(node);

}

},

value: this.state.inputValues[inputID],

errorText: this.state.errors[inputID] || fieldErrorMessage,

onBlur: (event) => {

// We delay the validation in order to prevent Checkbox loss of focus when

// the user are focusing a TextInput and proceeds to toggle a CheckBox in

// web and mobile web platforms.

setTimeout(() => {

this.setTouchedInput(inputID);

this.validate(this.state.inputValues);

}, 200);

if (_.isFunction(child.props.onBlur)) {

child.props.onBlur(event);

}

},

onInputChange: (value, key) => {

const inputKey = key || inputID;

this.setState(

(prevState) => ({

inputValues: {

...prevState.inputValues,

[inputKey]: value,

},

}),

() => this.validate(this.state.inputValues),

);

if (child.props.shouldSaveDraft) {

FormActions.setDraftValues(this.props.formID, {[inputKey]: value});

}

if (child.props.onValueChange) {

child.props.onValueChange(value);

}

},

});

where we are injecting props to the children, and we can inject one another - onTouched:

onBlur: () => { ... }, onTouched: () => { this.setTouchedInput(inputID); }, onInputChange: () => { ... }

and then, in new the NewDatePicker component, instead of calling this.props.onBlur() ever time we change the date, we could call this.props.onTouched().

Effect is the same, the validation message appears right after we select incorrect date, but I don't think it is the best solution - the touchedInput state is an internal value of the Form component and I don't believe we should allow form fields to amend it directly.

Also, when you check to docs about Creating and using Forms you can find here that the onBlur callback is designed to call validation directly (at least I understand it this way):

"- onBlur: An onBlur handler that calls validate."

As I wrote in my previous comment - I believe the custom field should decide when it is considered as blurred, so calling this.props.onBlur on every date change might be correct.

Looks good to me your thoughts @roryabraham

yes, @roryabraham - your opinion would be appreciated here

@roryabraham bump!

I believe the custom field should decide when it is considered as blurred, so calling this.props.onBlur on every date change might be correct

The only thing I can imagine might be problematic with this is that it could cause the input to lose accessibility focus with every change, which would be annoying for screen readers. However, I'm not confident in this because I'm not experienced with using screen readers

the touchedInput state is an internal value of the Form component and I don't believe we should allow form fields to amend it directly

I don't think I agree with this. If a child input should be able to control when it is blurred, shouldn't it also be able to control when it's touched?

So I'd suggest the alternate solution where we give the child an onTouched prop.

@Santhosh-Sellavel @roryabraham the alternate solution is now applied

roryabraham · 2023-05-23T22:47:32Z

Heads up @burczu, looks like tests are failing

burczu · 2023-05-24T05:22:50Z

Hey @roryabraham, I've noticed. I need to finish another urgent task first, and then I'll get back to this one, make a change discussed with @Santhosh-Sellavel and take a look at tests too.

burczu · 2023-05-24T07:29:27Z

Heads up @burczu, looks like tests are failing

All the tests should be fixed now

Santhosh-Sellavel · 2023-05-25T20:42:26Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web & Desktop

Screen.Recording.2023-05-26.at.1.27.56.AM.mov

Mobile Web - Chrome & Mobile Web - Safari

Screen.Recording.2023-05-26.at.2.03.58.AM.mov

iOS & Android

Screen.Recording.2023-05-26.at.2.06.58.AM.mov

Santhosh-Sellavel

LGTM, tests well!

mountiny

Thanks @burczu and @Santhosh-Sellavel

OSBotify · 2023-05-26T16:30:03Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-05-30T00:28:26Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.20-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Julesssss · 2023-05-31T10:38:03Z

I think this PR introduced this regression.

Julesssss · 2023-05-31T10:41:26Z

It's not critical, but we should figure out what ''Fix the errors' should be doing, now that the input is not selectable.

Santhosh-Sellavel · 2023-05-31T16:31:41Z

@Julesssss Left a comment here

OSBotify · 2023-05-31T16:35:39Z

🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.20-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

burczu force-pushed the 16514_date-picker-year-month-updates branch 2 times, most recently from d3dd9c2 to 76a353f Compare May 9, 2023 06:22

burczu changed the title ~~[WIP] Personal details - DOB field updating once year or month has been changed~~ Personal details - DOB field updating once year or month has been changed May 9, 2023

ArekChr reviewed May 9, 2023

View reviewed changes

burczu marked this pull request as ready for review May 9, 2023 11:00

burczu requested a review from a team as a code owner May 9, 2023 11:00

melvin-bot bot requested review from roryabraham and Santhosh-Sellavel and removed request for a team May 9, 2023 11:00

burczu force-pushed the 16514_date-picker-year-month-updates branch from 86744b1 to e8e0e6f Compare May 10, 2023 05:27

Santhosh-Sellavel reviewed May 11, 2023

View reviewed changes

burczu force-pushed the 16514_date-picker-year-month-updates branch from e8e0e6f to bf1d616 Compare May 12, 2023 07:36

Santhosh-Sellavel reviewed May 12, 2023

View reviewed changes

burczu force-pushed the 16514_date-picker-year-month-updates branch from 2b3b3f2 to e09e680 Compare May 15, 2023 07:46

lschurr mentioned this pull request May 16, 2023

[HOLD for payment 2023-06-07] [$1000] Personal details - DOB field does not get updated if year only changed. #16514

Closed

6 tasks

burczu added 9 commits May 24, 2023 09:10

initial implementation

2613b1b

unnecessary variables removed

1060b15

calendar picker component refactored

69ec101

few comments fixed

e94af5f

typo fixed

6a032f4

using moment.get to split date into parts in the constructor

211f0ff

comment typing fixed

e9eed69

code shortened

76dc417

unnecessary empty line removed

8c305a5

burczu added 4 commits May 24, 2023 09:10

prettier changes applied

2e6e0cb

getting day from the given date fixed

480bc0b

forcing validation on every date change added

127a68a

date format added to the moment constructor

22c9eca

burczu force-pushed the 16514_date-picker-year-month-updates branch from 386a3cc to 22c9eca Compare May 24, 2023 07:11

burczu added 3 commits May 24, 2023 09:14

onTouched handler added to the Form component

9d01fbe

the FORMS contributing guide updated with the onTouched handler

c9c31a3

calendar picker tests fixed

b8b9ef5

Santhosh-Sellavel approved these changes May 25, 2023

View reviewed changes

mountiny approved these changes May 26, 2023

View reviewed changes

mountiny merged commit c7390cd into Expensify:main May 26, 2023

burczu deleted the 16514_date-picker-year-month-updates branch May 29, 2023 06:42

OSBotify mentioned this pull request May 29, 2023

Deploy Checklist: New Expensify 2023-05-29 #19785

Closed

44 tasks

kbecciv mentioned this pull request May 30, 2023

Date of birth - Fix the errors functional is not functional #19852

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Personal details - DOB field updating once year or month has been changed #18577

Personal details - DOB field updating once year or month has been changed #18577

burczu commented May 8, 2023 •

edited

Loading

melvin-bot bot commented May 9, 2023

Santhosh-Sellavel May 11, 2023

burczu May 12, 2023

Santhosh-Sellavel May 12, 2023

burczu May 15, 2023 •

edited

Loading

burczu May 15, 2023

Santhosh-Sellavel commented May 11, 2023

Santhosh-Sellavel commented May 11, 2023 •

edited

Loading

burczu commented May 12, 2023

Santhosh-Sellavel May 12, 2023

burczu May 15, 2023

Santhosh-Sellavel May 15, 2023

burczu May 16, 2023 •

edited

Loading

Santhosh-Sellavel May 17, 2023

burczu May 18, 2023

Santhosh-Sellavel May 19, 2023

roryabraham May 22, 2023

burczu May 24, 2023

roryabraham commented May 23, 2023

burczu commented May 24, 2023 •

edited

Loading

burczu commented May 24, 2023

Santhosh-Sellavel commented May 25, 2023

Santhosh-Sellavel left a comment

mountiny left a comment

OSBotify commented May 26, 2023

OSBotify commented May 30, 2023

Julesssss commented May 31, 2023

Julesssss commented May 31, 2023

Santhosh-Sellavel commented May 31, 2023

OSBotify commented May 31, 2023

	onBlur: (event) => {
	// We delay the validation in order to prevent Checkbox loss of focus when
	// the user are focusing a TextInput and proceeds to toggle a CheckBox in
	// web and mobile web platforms.
	setTimeout(() => {
	this.setTouchedInput(inputID);
	this.validate(this.state.inputValues);
	}, 200);

	return React.cloneElement(child, {
	ref: (node) => {
	this.inputRefs[inputID] = node;

	const {ref} = child;
	if (_.isFunction(ref)) {
	ref(node);
	}
	},
	value: this.state.inputValues[inputID],
	errorText: this.state.errors[inputID] \|\| fieldErrorMessage,
	onBlur: (event) => {
	// We delay the validation in order to prevent Checkbox loss of focus when
	// the user are focusing a TextInput and proceeds to toggle a CheckBox in
	// web and mobile web platforms.
	setTimeout(() => {
	this.setTouchedInput(inputID);
	this.validate(this.state.inputValues);
	}, 200);

	if (_.isFunction(child.props.onBlur)) {
	child.props.onBlur(event);
	}
	},
	onInputChange: (value, key) => {
	const inputKey = key \|\| inputID;
	this.setState(
	(prevState) => ({
	inputValues: {
	...prevState.inputValues,
	[inputKey]: value,
	},
	}),
	() => this.validate(this.state.inputValues),
	);

	if (child.props.shouldSaveDraft) {
	FormActions.setDraftValues(this.props.formID, {[inputKey]: value});
	}

	if (child.props.onValueChange) {
	child.props.onValueChange(value);
	}
	},
	});

Personal details - DOB field updating once year or month has been changed #18577

Personal details - DOB field updating once year or month has been changed #18577

Conversation

burczu commented May 8, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented May 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burczu May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Santhosh-Sellavel commented May 11, 2023

Santhosh-Sellavel commented May 11, 2023 • edited Loading

burczu commented May 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burczu May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roryabraham commented May 23, 2023

burczu commented May 24, 2023 • edited Loading

burczu commented May 24, 2023

Santhosh-Sellavel commented May 25, 2023

Reviewer Checklist

Screenshots/Videos

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

OSBotify commented May 26, 2023

OSBotify commented May 30, 2023

Julesssss commented May 31, 2023

Julesssss commented May 31, 2023

Santhosh-Sellavel commented May 31, 2023

OSBotify commented May 31, 2023

burczu commented May 8, 2023 •

edited

Loading

burczu May 15, 2023 •

edited

Loading

Santhosh-Sellavel commented May 11, 2023 •

edited

Loading

burczu May 16, 2023 •

edited

Loading

burczu commented May 24, 2023 •

edited

Loading