[HOLD for payment 2024-10-24] [$250] Expensify Card - Only the last two digits can be deleted via BNP when input is not focused

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.45-2
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+pso@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

• User has been assigned a physical Expensify card.

1. Launch New Expensify app.
2. Go to Account settings > Wallet.
3. Tap on the Expensify card.
4. Tap Get physical card.
5. Tap Ship card on the comfirmation page.
6. Tap Activate physical card.
7. Enter any 4 digits.
8. After the error shows up, tap < button on the BNP to delete the digits (do not focus on the input, remove the focus on the input if it is focused).

Expected Result:

The digits can be deleted completely via < button on the BNP when the input is not in focus.

Actual Result:

Only the last two digits can be deleted via < button on the BNP when the input is not in focus.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6626370_1728241456205.ScreenRecording_10-07-2024_02-49-47_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021843956765151192500
• Upwork Job ID: 1843956765151192500
• Last Price Increase: 2024-10-09
• Automatic offers:
• hoangzinh | Reviewer | 104376130
• truph01 | Contributor | 104376133

Issue Owner

Current Issue Owner: @hoangzinh

[melvin-bot]

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[IuliiaHerets]

We think that this bug might be related to #wave-collect - Release 2

[IuliiaHerets]

@sonialiap FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[shahinyan11]

Edited by proposal-police: This proposal was edited at 2024-10-07 15:06:01 UTC.

Proposal

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

There are two different errors

1. Only the last two digits can be deleted via BNP when input is not focused
2. The order of deletions will be destroyed when after blurring you press another digit to update the last digit and then try to delete digits.

Untitled.mov

What is the root cause of that problem?

1. First error RCA: Here we do not update editIndex index after deleting last digit
2. Second error RCA: I don't know what onFocus function is used for, as the code works fine without it. but in existence of this function then we do not update lastFocusedIndex appropriately after deleting digits .

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

1. First error solution: Add the following code setEditIndex(indexBeforeLastEditIndex); here

2. Second error solution: Add following code lastFocusedIndex.current = newFocusedIndex in this line;

What alternative solutions did you explore? (Optional)

Alternative for second issue: Remove onFocus function

[melvin-bot]

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

[melvin-bot]

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

[truph01]

Edited by proposal-police: This proposal was edited at 2024-10-10 11:26:26 UTC.

Proposal

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

Only the last two digits can be deleted via < button on the BNP when the input is not in focus.

What is the root cause of that problem?

• This logic:

App/src/components/MagicCodeInput.tsx

Lines 280 to 281 in 69162d2

	const formElement = inputRefs.current as HTMLFormElement | null;
	(formElement?.[indexToFocus] as HTMLInputElement)?.focus();

are outdated, it was add in this PR, at that time, each character we user separate input for it. Eg, in case of this bug, we have 4 inputs component. So the inputRefs stores 4 refs of these text inputs.

• But in this PR, instead of 4 separate text inputs there is only one wrapped inside TapGestureHandler that determine which number should be inputed. So the inputRefs stores only the ref of the text input.

• As a result, call (inputRefs?.[indexToFocus] as HTMLInputElement)?.focus(); does not work anymore, that leads to the bug. But why does it not work?

• Assume we are fill in all 4 digits and we are focusing on the last digit. Now, blur the input. Then, when clicking on "<", logic:
  

  App/src/components/MagicCodeInput.tsx

  Line 276 in 69162d2

  if (isDisableKeyboard && focusedIndex === undefined) {

  
  is called. In there, it set editIndex to 3.

• Then, click "<" again and again. Logic

  App/src/components/MagicCodeInput.tsx

  Line 276 in 69162d2

  if (isDisableKeyboard && focusedIndex === undefined) {

  
  is always called and the indexToFocus is always 2 in that if block. As a result, only the last two digits can be deleted.

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

• This logic:

App/src/components/MagicCodeInput.tsx

Lines 280 to 281 in 69162d2

	const formElement = inputRefs.current as HTMLFormElement | null;
	(formElement?.[indexToFocus] as HTMLInputElement)?.focus();

should be replaced by:

                if (indexToFocus !== undefined) {
                    lastFocusedIndex.current = indexToFocus;
                    inputRefs.current?.focus();
                }

and this:

App/src/components/MagicCodeInput.tsx

Line 317 in 69162d2

inputRefs.current?.focus();

should be replaced by:

                lastFocusedIndex.current = newFocusedIndex;
                inputRefs.current?.focus();

What alternative solutions did you explore? (Optional)

[truph01]

Proposal updated

[hoangzinh]

Thanks for proposals, everyone. @truph01 do you know why we have to set lastFocusedIndex.current before we focus the input? (like you mentioned in your solution)

[truph01]

• When we focus on input, then the onFocus is triggered. Before focusing on input, we need to set lastFocusedIndex.current since we are using lastFocusedIndex.current in this:

App/src/components/MagicCodeInput.tsx

Lines 201 to 207 in 69162d2

	const onFocus = (event: NativeSyntheticEvent<TextInputFocusEventData>) => {
	if (shouldFocusLast.current) {
	lastValue.current = TEXT_INPUT_EMPTY_STATE;
	setInputAndIndex(lastFocusedIndex.current);
	}
	event.preventDefault();
	};

to call setInputAndIndex(lastFocusedIndex.current) with the correct focused index value.

• The similar logic is used in this:
  

  App/src/components/MagicCodeInput.tsx

  Line 224 in 69162d2

  lastFocusedIndex.current = index;

[hoangzinh]

Thanks for the updates. @truph01's proposal looks better to me. He explained the root cause well, and his solution looks good.

Link to proposal #50315 (comment)

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @hoangzinh 🎉 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]

📣 @truph01 🎉 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 📖

[truph01]

PR is ready

[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 9.0.49-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Only the last two digits can be deleted via BNP when input is not focused #50707

If no regressions arise, payment will be issued on 2024-10-24. 🎊

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

• @hoangzinh requires payment automatic offer (Reviewer)
• @truph01 requires payment automatic 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:

• [@hoangzinh] The PR that introduced the bug has been identified. Link to the PR:
• [@hoangzinh] 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:
• [@hoangzinh] 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:
• [@hoangzinh] Determine if we should create a regression test for this bug.
• [@hoangzinh] 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.
• [@sonialiap] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[sonialiap]

Payment summary:

• @hoangzinh $250 - paid in upwork ✔️ - please complete the checklist
• @truph01 $250 - paid in upwork ✔️

[hoangzinh]

BugZero Checklist:

• The PR that introduced the bug has been identified. Link to the PR: Blur Magic Code input when click outside #24012
• 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/24012/files#r1815257149
• 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: 🚫 I lean tower to no, it's just a small UX issue, the user can still use app

[sonialiap]

Thanks!

• Link the GH issue for creating/updating the regression test once above steps have been agreed upon: None