[HOLD for payment 2023-12-12] [$500] Personal details - No green check mark and the year is not selected using Enter key

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v1.4.2-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation: @

Action Performed:

1. Go to URL https://staging.new.expensify.com/
2. Login with any account
3. Go to Settings -> Profile -> Personal details -> Date of birth -> Date -> Year
4. Enter the year manually in the input field -> Tap Enter

Expected Result:

The green mark near the year is present.
The page closes with the data saved.

Actual Result:

There is no green check mark near the year.
The page closes, the new selected year is not saved.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6286963_1700650222106.Recording__6846.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014b429ddfb40b7555
• Upwork Job ID: 1727297908221693952
• Last Price Increase: 2023-11-22
• Automatic offers:
• aimane-chnaif | Reviewer | 27849808
• mkhutornyi | Contributor | 27849810

Issue Owner

Current Issue Owner: @adelekennedy

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014b429ddfb40b7555

[melvin-bot]

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)

[mkhutornyi]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Pressing Enter key on year picker modal closes Date of birth page as well

What is the root cause of that problem?

This was fixed in #30300 but the fix was reverted during TS migration refactor of useKeyboardShortcut component in #29226

What changes do you think we should make in order to solve the problem?

Bring back that change
https://github.com/Expensify/App/pull/30300/files#diff-c8122e6723c6a4dde72bcbf6da39cfdc7d8d35540105e70c6a305d00e63c7943

[aimane-chnaif]

@mkhutornyi's proposal looks good to me.
As the regression period of #29226 is over, we can treat this as new issue.
🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @marcochavezf, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[DylanDylann]

Proposal

Please re-state the problem that we are trying to solve in this issue.

• There are two bugs encounter that related to this issue:

1. After selecting the year, the input filed at the top does not update.
2. The page closes, the new selected year is not saved.ENTER key
   Note: There is no green check mark near the year. (It is not a bug)

What is the root cause of that problem?

1. In here:
   

   App/src/components/NewDatePicker/CalendarPicker/index.js

   Line 82 in b30b555

   onYearSelected(year) {

   
   we do not call this.props.onSelected to update to datetime like we did with onDayPressed, that lead to the bug "After selecting the year, the input filed at the top does not update."

2. In YeahPickerModal, pressing on ENTER when user is focusing on the input component will auto submit the form because of the below logic:
   

   App/src/components/FormSubmit/index.js

   Lines 22 to 26 in 3583f39

   	// ENTER is pressed on INPUT or SELECT element, call the submit callback.
   	if (tagName === 'INPUT' || tagName === 'SELECT') {
   	onSubmit();
   	return;
   	}

• The form is submitted but because the datetime is not updated (bug 1) so leads to the bug 2 "The page closes, the new selected year is not saved.ENTER key"

What changes do you think we should make in order to solve the problem?

1. We can just update

   App/src/components/NewDatePicker/CalendarPicker/index.js

   Lines 82 to 95 in 3583f39

   	onYearSelected(year) {
   	this.setState((prev) => {
   	const newCurrentDateView = setYear(new Date(prev.currentDateView), year);
   	return {
   	currentDateView: newCurrentDateView,
   	isYearPickerVisible: false,
   	years: _.map(prev.years, (item) => ({
   	...item,
   	isSelected: item.value === newCurrentDateView.getFullYear(),
   	})),
   	};
   	});
   	}

   
   to:

onYearSelected(year) {
        this.setState(
            (prev) => {
                const newCurrentDateView = setYear(new Date(prev.currentDateView), year);
                return {
                    currentDateView: newCurrentDateView,
                    isYearPickerVisible: false,
                    years: _.map(prev.years, (item) => ({
                        ...item,
                        isSelected: item.value === newCurrentDateView.getFullYear(),
                    })),
                };
            },
+          () => this.props.onSelected(format(new Date(this.state.currentDateView), CONST.DATE.FNS_FORMAT_STRING)),
        );
    }

2. We just call the onSubmit if we are focusing on the DateOfBirthPage. Do it by updating:
   

   App/src/pages/settings/Profile/PersonalDetails/DateOfBirthPage.js

   Line 80 in 3583f39

   onSubmit={PersonalDetails.updateDateOfBirth}

   
   to:

onSubmit={isFocusOnScreen ? PersonalDetails.updateDateOfBirth : () => {}}

What alternative solutions did you explore? (Optional)

• The RCA of this bug is that, currently, in the FormSubmit component, we did not use onKeyDownCapture to listen the keydown event rather than using useKeyboardShortcut as we did entire the app, that leads to the inconsistency, and we cannot manage the keydown event listener in FormSubmit by using bindHandlerToKeydownEvent.
• Solution:

1. First, refactor the FormSubmit to use the useKeyboardShortcut for consistency.

2. Then, just need to set shouldBubble: false in YearPickerModal `s keydown event listener.

[DylanDylann]

@aimane-chnaif Please help check my proposal, that indicates all encounted bugs and the RCAs

[aimane-chnaif]

After selecting the year, the input filed at the top does not update.

@DylanDylann this is not bug but intentional. The input field is updated only when select date

[DylanDylann]

@aimane-chnaif but, if I change the date, it will update. So, there is a inconsistency here

[aimane-chnaif]

The UX is: when select year, month don't update input. when select date, sync input with selected year, month, date

[aimane-chnaif]

@DylanDylann what you're suggesting is new feature, not relevant to this issue. If you persist, please raise discussion on slack with Problem / Solution.

[DylanDylann]

Yeah. Thanks for your information. What about the 2nd bug that is mentioned in my proposal?

[aimane-chnaif]

Yeah. Thanks for your information. What about the 2nd bug that is mentioned in my proposal?

@DylanDylann I don't like your solution. please check #24198 for more context. The accepted one is already pre-approved solution.

[DylanDylann]

@aimane-chnaif Many thanks

[DylanDylann]

@aimane-chnaif @marcochavezf Just my suggestion related to issue Web – DOB – Error appears when year is selected using keyboard but doesn’t trough mouse.:

• The RCA of this bug is that, currently, in the FormSubmit component, we did not use onKeyDownCapture to listen the keydown event rather than using useKeyboardShortcut as we did entire the app, that leads to the inconsistency, and we cannot manage the keydown event listener in FormSubmit by using bindHandlerToKeydownEvent.
• Solution:

1. First, refactor the FormSubmit to use the useKeyboardShortcut for consistency.

2. Then, just need to set shouldBubble: false in YearPickerModal `s keydown event listener.

[shubham1206agra]

@aimane-chnaif We may remove onKeyDownCapture entirely in #31433.
cc @cubuspl42 as he is the C+ in this

[DylanDylann]

@shubham1206agra So we will use useKeyboardShortcut, right?

[shubham1206agra]

No
We may be going towards onSubmitEditing.

[shubham1206agra]

But if you have a compelling reason for useKeyboardShortcut, please tell

[DylanDylann]

@shubham1206agra

No
We may be going towards onSubmitEditing.

Does onSubmitEditing trigger in desktop web?

[aimane-chnaif]

@DylanDylann I see you raised discussion on slack. So if your approach is approved, maybe it can be created as new issue and refactor would go there. But out of scope for this issue.
This used to work fine. It's just regression from another PR so we should fix it.

[DylanDylann]

@aimane-chnaif Sure, thanks

[marcochavezf]

Thanks @aimane-chnaif for the review, I agree with @mkhutornyi's proposal and also agree we can consider it as a new issue given the regression period has expired in the other issue. Just we'd need to add some comments in the code changes about why this change is needed to avoid being removed for any circumstance in the future.

[melvin-bot]

📣 @aimane-chnaif 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @mkhutornyi 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.7-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• prevent auto submit form on enter key press in selection modal #32176

If no regressions arise, payment will be issued on 2023-12-12. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @aimane-chnaif requires payment offer (Reviewer)
• @mkhutornyi requires payment offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@aimane-chnaif] The PR that introduced the bug has been identified. Link to the PR:
• [@aimane-chnaif] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@aimane-chnaif] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@aimane-chnaif] Determine if we should create a regression test for this bug.
• [@aimane-chnaif] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@adelekennedy] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[marcochavezf]

Hi @aimane-chnaif, I think we're waiting here for the checklist to complete the payment, correct?

[aimane-chnaif]

• The PR that introduced the bug has been identified. Link to the PR: [No QA] [TS migration] Migrate 'useKeyboardShortcut.js' hook to TypeScript #29226
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/29226/files#r1425603574
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• Determine if we should create a regression test for this bug. N/A Caught by Applause
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

[adelekennedy]

Payouts due:

Contributor: $500 @mkhutornyi (Upwork)
Contributor+: $500 @aimane-chnaif (Upwork)