Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.53k stars 2.88k forks source link

[$1000] Log in - User unable to proceed after re-enter last digit of magic code number #21958

Closed kbecciv closed 10 months ago

kbecciv commented 1 year ago

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


Action Performed:

  1. Open Login Page
  2. Input email => Continue
  3. input wrong magic: example: 111111
  4. Click last input
  5. Input again 1
  6. input any number

Expected Result:

User able to proceed after re-enter last digit of magic code number

Actual Result:

User unable to proceed after re-enter last digit of magic code number

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.34-1 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 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/56b17ede-ed41-4739-aa3d-0017108ef367

https://github.com/Expensify/App/assets/93399543/96ffccb2-c5a6-41e4-96c5-304d47d85a8e

Expensify/Expensify Issue URL: Issue reported by: @namhihi237 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688039417283349

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01306ade9d70001f15
  • Upwork Job ID: 1674835066467475456
  • 2023-07-28
  • Automatic offers:
    • samh-nl | Contributor | 25862341
    • namhihi237 | Reporter | 25862342
Issue OwnerCurrent Issue Owner: @mallenexpensify
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

samh-nl commented 1 year ago

Proposal

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

The user is unable to change the magic code if an error has occurred without refocusing manually.

What is the root cause of that problem?

Upon submitting, the magic code input is blurred:

https://github.com/Expensify/App/blob/0bbf3fccfce0994ddf4fd9e95fb33dd9edbd1868/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js#L200-L202

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

Remove the automatic blurring of the input.

What alternative solutions did you explore? (Optional)

An alternative solution is to keep the auto blurring of the input, but refocus only if an error subsequently occurs.

EDIT: Added code snippet.

namhihi237 commented 1 year ago

Proposal

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

User able to proceed after re-enter last digit of magic code number

What is the root cause of that problem?

The cause of this problem lies here: This issue occurs when after entering last input same as current number in that cell, it won't call API so value value of input will not be reset resulting numbersArr will always be equal to last value before when the 6th cell is entered, and does not change no matter what numbers are entered after that. For example, when the last number is 1 => we focus on the last cell => re-enter the number 1 => value = "1", then the next entered random numbers value = "1xxxx". this code will always return [1]. https://github.com/Expensify/App/blob/0bbf3fccfce0994ddf4fd9e95fb33dd9edbd1868/src/components/MagicCodeInput.js#L214-L218

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

We will update: when in the last input box, we will always take the last value of the user entered `value

let numbersArr = []
numbersArr = value
    .trim()
    .split('')
    .slice(0, props.maxLength - editIndex);

// handle the last number if after entering the last number but still enter:
let newValue = '';
if (value.length > props.maxLength - editIndex) {
    const lastInput = value[value.length - 1];
    numbersArr[numbersArr.length - 1] = lastInput;
    newValue = value.slice(0, props.maxLength - editIndex) + lastInput;
}

setInput(newValue || value);

Also we need to remove the limit maxLength={props.maxLength} because in case when user input wrong magic code then user click first input and re-enter same wrong magic code above then the user numbers input will not work because the length is already equal to maxLength even though inpiut still has focus and does not give an error. We can omit it because above we have covered the magic code where the set will not be larger than prop.maxLength. Result:

https://github.com/Expensify/App/assets/39086067/d895695d-0f27-43fb-86e9-65cb72aea587

 ### What alternative solutions did you explore? (Optional)  We can unfocus when inputting the last number or after fulfilling whether it calls API or not

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @CortneyOfstad is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

CortneyOfstad commented 1 year ago

I am heading OoO for the week (back July 10) so re-assigning this in the meantime 👍

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

ugur-akin commented 1 year ago

Proposal

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

When clicked/touched on the last digit of a magic code input that is previously non-empty, upon re-inputting the same digit as previously entered (i.e. the last digit was previously 6, user presses 6 again after clicking the last digit), the code will not auto submit and nor will any further digit inputs will update the last digit.

What is the root cause of that problem?

The handling of the underlying text input in magic code input component is implemented incorrectly to handle the case when the user re-types the same last digit as the previously entered one.

https://github.com/Expensify/App/blob/0bbf3fccfce0994ddf4fd9e95fb33dd9edbd1868/src/components/MagicCodeInput.js#L214-L217

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

Recommended Approach:

I think there's a good opportunity to improve MagicCodeInput component and the magic code UX for Expensify (see alterative solutions explored section).

Fixing the existing implementation:

Split up the underlying text input to: [...digitsUpToLastDigit, lastDigit] .

const  valueAsNumbers  =  value.trim().split('');

const  numDigitsToTakeIfValueIsPastMaxLength  =  props.maxLength  -  editIndex  -  1;
const  numDigitsToTakeIfValueIsShorterThanMaxLength  =  value.length -  1;
const  numDigitsToTake  =  Math.min(numDigitsToTakeIfValueIsPastMaxLength, numDigitsToTakeIfValueIsShorterThanMaxLength);
const  valueUpToLastDigit  =  value.slice(0, numDigitsToTake);  

const  lastDigitInValue  =  valueAsNumbers[valueAsNumbers.length -  1];

const  effectiveValueAsNumbers  = [...valueUpToLastDigit, lastDigitInValue];

const  updatedFocusedIndex  =  Math.min(editIndex  + (effectiveValueAsNumbers.length -  1) +  1, props.maxLength  -  1);

let  numbers  =  decomposeString(props.value, props.maxLength);
numbers  = [...numbers.slice(0, editIndex), ...effectiveValueAsNumbers, ...numbers.slice(effectiveValueAsNumbers.length +  editIndex, props.maxLength)];

What alternative solutions did you explore?

Opinionated Statement: The complexity of the magic code input can be reduced drastically if we opt for better UX choices. Claim: Often re-typing the entire code is easier for the user than correcting digits. Fixing a digit requires identifying the incorrect digit, focusing it (either by clicking/touching the right digit or through arrow keys navigation) and entering the right digit.

Clear on incorrect submission

Maintain focus on the magic input and clear it upon an unsuccessful code entry. Claim: Users are pretty effective with the keyboard (especially on mobile). Don't make them re-focus the input, let them re-type without hassle. A bonus of this is a second opportunity to leverage pasting a value or automatic text fills such as iOS' SMS Passcode feature. This method is also used widely in the industry and is Tinder's choice of UX for one-time password inputs.

Don't let users enter past the first empty index

Claim: Allowing users to enter digits past the first empty digit is practically useless - arguably nobody fills starting from mid-way through and if it happens, it's probably a mistake. If the user loses focus on the text input, let them click anywhere on the magic code and automatically register the input for the first empty digit. Along with clear on incorrect submission, this will entirely remove the need for complicated input-index/focus-index state management. If we don't like to clear input, this would still be a worth improvement on its own.

Edit: Removed anchor links to comment sections - they weren't working. Clarifications and typo/styling fixes.

melvin-bot[bot] commented 1 year ago

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

melvin-bot[bot] commented 1 year ago

📣 @ugur-akin! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
ugur-akin commented 1 year ago

Contributor details Your Expensify account email: ugurakinsoftware@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0161bd395d1f6814a7

melvin-bot[bot] commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

lazar-stamenkovic commented 1 year ago

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

User unable to proceed after re-enter last digit of magic code

What is the root cause of that problem?

This issue is caused by following reason

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

Screenshot 2023-07-02 at 10 53 09

melvin-bot[bot] commented 1 year ago

@eVoloshchak, @mallenexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

lazar-stamenkovic commented 1 year ago

@eVoloshchak, @mallenexpensify please take a look my proposal. https://github.com/Expensify/App/issues/21958#issuecomment-1616711090

eVoloshchak commented 1 year ago

Reviewing today

eVoloshchak commented 1 year ago

@namhihi237's proposal was the first to correctly identify the root cause. This is indeed the cause, the issue is happening only when you enter the same number

This issue occurs when after entering last input same as current number in that cell,

I agree with @ugur-akin, UX seems to be a bit off.. Currently, if you've entered an incorrect code, you can edit only a single digit, after which the API call is made and input is blurred.

I think there's a good opportunity to improve MagicCodeInput component and the magic code UX for Expensify (see alterative solutions explored section). Maintain focus on the magic input and clear it upon an unsuccessful code entry.

@mallenexpensify, what are your thoughts on @ugur-akin's alternative solution? I could also create a bigger discussion on Slack

lazar-stamenkovic commented 1 year ago

@eVoloshchak would you please take a look my proposal? https://github.com/Expensify/App/issues/21958#issuecomment-1616711090 believe I have resolved the issue by updating the code with minimal changes. I also agree with @ugur-akin. think it's a new feature that will require minor updates on the MagicCodeInput component. Specifically, it needs to include an action, such as a button click, to trigger the code submission within the component using MagicCodeInput

eVoloshchak commented 1 year ago

@lazar-stamenkovic, I did review your proposal, but would like to clarify the expected behavior first. I think simplifying the logic is the way to go, but would like to hear what others think

lazar-stamenkovic commented 1 year ago

@lazar-stamenkovic, I did review your proposal, but would like to clarify the expected behavior first. I think simplifying the logic is the way to go, but would like to hear what others think

Got it. Please let me know if you decide considering my solution again. Thanks for reviewing my proposal.

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

mallenexpensify commented 1 year ago

Sorry for the delay, I was out for a couple days last week.

what are your thoughts on @ugur-akin's https://github.com/Expensify/App/issues/21958#issuecomment-1615866011? I could also create a bigger discussion on Slack

Can you please raise a discussion in Slack please @eVoloshchak? I feel like we could maybe improve aspects of how we're managing magic codes (not an engineer!). If we decide to make larger changes, we should discuss potential compensation for @namhihi237 for being the first to identify the root cause here, since that'd fix the specific bug here

eVoloshchak commented 1 year ago

Will raise a discussion shortly

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 year ago

@eVoloshchak @mallenexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

eVoloshchak commented 1 year ago

Not overdue, started a discussion in https://expensify.slack.com/archives/C01GTK53T8Q/p1689617742443109

melvin-bot[bot] commented 1 year ago

@eVoloshchak, @mallenexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 year ago

@eVoloshchak @mallenexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 1 year ago

@eVoloshchak, @mallenexpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

eVoloshchak commented 1 year ago

Original discussion didn't get any traction, bumping on Slack

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 year ago

@eVoloshchak @mallenexpensify this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @eVoloshchak is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

@eVoloshchak, @mallenexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

eVoloshchak commented 1 year ago

As per the discussion

Also I believe we've had discussion in the past about clearing the input on failure, and we came to the conclusion that correcting 6 digits isn't really much of a barrier to retyping so it wasn't seen as necessary to have We should keep focused input on the field if incorrect

Which is exactly what @samh-nl's has proposed here. I think we can proceed with the proposal

🎀👀🎀 C+ reviewed!

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @mallenexpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Current assignee @eVoloshchak is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

📣 @eVoloshchak Please request via NewDot manual requests for the Reviewer role ($1000)

melvin-bot[bot] commented 1 year ago

📣 @samh-nl 🎉 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[bot] commented 1 year ago

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

Offer link Upwork job

Pujan92 commented 1 year ago

@eVoloshchak I think the selected proposal won't work as we blur the MagicCodeInput once it is fulfilled. https://github.com/Expensify/App/blob/cb8aac115eb2c0130dcab208cccb60ba6f21fa77/src/components/MagicCodeInput.js#L122-L129

Pujan92 commented 1 year ago

Proposal

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

Last input text is not focused when we get an error from the server

What is the root cause of that problem?

Currently, we are blurring the focusedIndex once the user fulfills the magic code and we are not focusing back on the last input once we receive an error from the server. https://github.com/Expensify/App/blob/cb8aac115eb2c0130dcab208cccb60ba6f21fa77/src/components/MagicCodeInput.js#L129

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

As per the requirement, we do need to focus on the last input box once we get the incorrect magic code error from the server for user convenience.

  1. In the MagicCodeInput component we need to check for the previous hasError value. const prevHasError = usePrevious(props.hasError);

  2. We can create useEffect block where we consider prevHasError and hasError to decide whether to focus or not on the last index input.

    useEffect(() => {
        if(prevHasError || !props.hasError) {
            return;
        }
        inputRefs.current[props.maxLength - 1].focus();
    }, [prevHasError, props.hasError, props.maxLength])
Result https://github.com/Expensify/App/assets/14358475/760ff53a-b1fd-4cef-a491-5314bc4deaf0
samh-nl commented 1 year ago

PR is ready for review: https://github.com/Expensify/App/pull/23995

samh-nl commented 1 year ago

@Pujan92 I think the focus can remain instead of blurring and refocusing again?

Pujan92 commented 1 year ago

@samh-nl I don't think so otherwise keeping focus will make the api calls on each input change even though the previous call is in process, as per the slack discussion what I understood is that only for the failure/error case we need to make the focus.