[HOLD for payment 2022-12-20] [HOLD] [$1000] Mobile keyboard doesn't include `/` char when expiration field is in focus #13133

Julesssss · 2022-11-29T15:39:08Z

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

CC @Gonals

Action Performed:

Navigate to Profile > Payments > Add A debit Card
Fill in the data until you get to the expiration date field
Select the expiration date input
Enter 2 characters

Expected Result:

Either the / char should appear in the soft keyboard, or we should automatically add it for users

Actual Result:

Neither the / char appears in the soft keyboard, or do we automatically add it for users

Note: Users still seem to pass validation without it, but this is still confusing

Platform:

Where is this issue occurring?

iOS
Android
Mobile Web

Version Number: all
Reproducible in staging?: yes
Reproducible in production?: yes
Email or phone of affected tester (no customers): all

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01f986d0cdc2cf028e
Upwork Job ID: 1597827787499143168

The text was updated successfully, but these errors were encountered:

melvin-bot · 2022-11-29T15:39:14Z

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

Puneet-here · 2022-11-29T15:49:04Z

Proposal

I suggest that we change the placeholder to MMYY so that the user will know that they don't have to add / Many sites do the same.

App/src/languages/en.js

Line 415 in 3e47bce

expirationDate: 'MM/YY',

Another solution is to remove the keyboard this way the user can switch the keyboard to add numbers and /
We have keyboard type numbers-and-punctuation but it is only for ios. If we change this to numbers-and-punctuation this won't be a problem in ios and in android user will see default keyboard. We can also use platform specific file and use phone-pad for android.

Here's a vid of phone-pad -

Screen.Recording.2022-11-29.at.9.24.07.PM.mov

Here's numbers-and-punctuation keyboard

App/src/pages/settings/Payments/AddDebitCardPage.js

Line 141 in 3e47bce

keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD}

mananjadhav · 2022-11-29T17:34:18Z

@Julesssss is this about not able to add from keyboard? or not able to add slash at all? I remember working on this PR here to add auto-slash

Julesssss · 2022-11-29T17:42:59Z

@Julesssss is this about not able to add from keyboard? or not able to add slash at all? I remember working on this PR here to add auto-slash

Not during my testing today. We use the numerical keyboard type, and it wasn't auto-added. I do also recall a conversation about this though 😕

Puneet-here · 2022-11-29T17:48:02Z

It was removed here. ~~We can add it back.~~
Looks like we can't add it back because of Form component

kadiealexander · 2022-11-30T05:39:39Z

From this SO: https://stackoverflow.com/c/expensify/questions/14418

The "bug" is actually a bug: Yes!
The bug is not a duplicate report of an existing GH (close the GH and add any novel details to the original GH instead): No
The bug is reproducible, following the reproduction steps: Yes
If you’re unable to reproduce the bug, add the Needs reproduction label. Comment on the issue outlining the steps you took to try to reproduce the bug, your results and tag the issue reporter and the Applause QA member who created the issue. Ask them to clarify reproduction steps and/or to check the reproduction steps again. Close issue.: n/a
The GH template is filled out as fully as possible -- this means the GH body and title are clear (ie. could an external contributor understand it and work on it?): Yes
The GH was created by an Expensify employee or a QA tester: Yes
If the bug is an OldDot issue, you should decide whether it is widespread enough to justify fixing or it is better to wait for reunification. If it’s better to wait, close the GH & provide this justification: n/a
If there's a link to Slack, check the discussion to see if we decided not to fix it: n/a
Decide if the GH should be resolved by an External contributor or Internal engineer, add the appropriate label: External.

melvin-bot · 2022-11-30T05:40:02Z

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

melvin-bot · 2022-11-30T05:40:09Z

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

melvin-bot · 2022-11-30T05:40:14Z

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

melvin-bot · 2022-11-30T05:40:17Z

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

thesahindia · 2022-11-30T06:17:56Z

It was removed here. ~~We can add it back.~~
Looks like we can't add it back because of Form component

@Julesssss, any thoughts on this? We can't interfere with the value right now. We need to wait and see if someone can refactor the Form component to do so.

railway17 · 2022-11-30T08:29:54Z

Proposal

Reproducible
This issue is easy to reproduce and I don't think I have to attach video or screenshot again. 😄
Why reproduced?
As @Puneet-here mentioned, appending slash module was removed while implementing with new Form logic.
And need to update the our Form as @thesahindia mentioned.
How to resolve?
We can use the changes from here if it was working correctly.

Add new props shouldAppendSlash props to the TextInput in AddDebitCartPage.js

        <TextInput
                                inputID="expirationDate"
                                label={this.props.translate('addDebitCardPage.expiration')}
                                placeholder={this.props.translate('addDebitCardPage.expirationDate')}
                                keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD}
+                                shouldAppendSlash
                            />

Update the Form component like below:

        this.inputRefs = {};
        this.touchedInputs = {};

        this.setTouchedInput = this.setTouchedInput.bind(this);
....
+  /**
+     * Append slash if needed e.g. ExpirationDate input
+     * @param {string} inputValue - input value on text change event
+     * @param {string} inputKey - key of state for specific input field
+     * @returns {string}
+     */
+    appendSlashIfNeeded(inputValue, inputKey) {
+        let slashAppended = inputValue;
+        const inputStateValue = this.state.inputValues[inputKey]
+        if (inputStateValue && inputValue.length < inputStateValue.length
+            && (((inputValue.length === 3 && inputValue.charAt(2) === '/')
+              || (inputValue.length === 2 && inputStateValue.charAt(1) === '/')))) {
+                slashAppended = inputValue.substring(0, inputValue.length - 1);
+        } else if (inputValue.length === 2 && _.indexOf(inputValue, '/') === -1) {
+            // An Expiry Date was added, so we should append a slash '/'
+            slashAppended = `${inputValue}/`;
+        } else if (inputValue.length > 2 && _.indexOf(inputValue, '/') === -1) {
+            // Expiry Date with MM and YY without slash, hence adding slash(/)
+            slashAppended = `${inputValue.slice(0, 2)}/${inputValue.slice(2)}`;
+        }
+        return slashAppended;
+    }
...
                 onInputChange: (value, key) => {
                     const inputKey = key || inputID;
-                    this.setState(prevState => ({
-                        inputValues: {
-                            ...prevState.inputValues,
-                            [inputKey]: value,
-                        },
-                    }), () => this.validate(this.state.inputValues));
+                    if (child.props.shouldAppendSlash) {
+                        const slashAppended = this.appendSlashIfNeeded(value, inputKey);
+                        this.setState(prevState => ({
+                            inputValues: {
+                                ...prevState.inputValues,
+                                [inputKey]: slashAppended,
+                            },
+                        }), () => this.validate(this.state.inputValues));                        
+                    } else {
+                        this.setState(prevState => ({
+                            inputValues: {
+                                ...prevState.inputValues,
+                                [inputKey]: value,
+                            },
+                        }), () => this.validate(this.state.inputValues));
+                    }
...

Screen.Recording.2022-11-30.at.4.28.21.PM.mov

varshamb · 2022-11-30T08:55:57Z

Proposal

We can use keyboardType='numeric' specific for Android.

App/src/pages/settings/Payments/AddDebitCardPage.js

Line 141 in 3e47bce

keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD}

thesahindia · 2022-11-30T09:54:59Z

@railway17, I don't think that's a good solution. We want to keep the logic at AddDebitCardPage.js.

thesahindia · 2022-11-30T09:56:03Z

@varshamb, how is that going to solve this issue?

bernhardoj · 2022-11-30T09:58:02Z

Proposal

We can receive a new prop for the Form children, let say it is called transformer, which will receive a function with 2 parameter that will transform the input every input change, in our case appending a /.

<TextInput
    inputID="expirationDate"
    label={this.props.translate('addDebitCardPage.expiration')}
    placeholder={this.props.translate('addDebitCardPage.expirationDate')}
    keyboardType={CONST.KEYBOARD_TYPE.NUMBER_PAD}
+   transformer={this.addOrRemoveSlashToExpiryDate}
/>

The addOrRemoveSlashToExpiryDate taken from old code with few changes.

    addOrRemoveSlashToExpiryDate(prevExpiryDate = '', inputExpiryDate) {
        let expiryDate = inputExpiryDate;

        // Remove the digit and '/' when backspace is pressed with expiry date ending with '/'
        if (inputExpiryDate.length < prevExpiryDate.length
            && (((inputExpiryDate.length === 3 && lodashEndsWith(inputExpiryDate, '/'))
                || (inputExpiryDate.length === 2 && lodashEndsWith(prevExpiryDate, '/'))))) {
            expiryDate = inputExpiryDate.substring(0, inputExpiryDate.length - 1);
        } else if (inputExpiryDate.length === 2 && _.indexOf(inputExpiryDate, '/') === -1) {
            // An Expiry Date was added, so we should append a slash '/'
            expiryDate = `${inputExpiryDate}/`;
        } else if (inputExpiryDate.length > 2 && _.indexOf(inputExpiryDate, '/') === -1) {
            // Expiry Date with MM and YY without slash, hence adding slash(/)
            expiryDate = `${inputExpiryDate.slice(0, 2)}/${inputExpiryDate.slice(2)}`;
        }
        return expiryDate
    }

And we will use that function on Form children onInputChange props

+const transformer = lodashGet(child.props, 'transformer', (v1, v2) => v2);

.....
onInputChange: (value, key) => {
    const inputKey = key || inputID;
    this.setState(prevState => ({
        inputValues: {
            ...prevState.inputValues,
-           [inputKey]: value,
+           [inputKey]: transformer(prevState.inputValues[inputKey], value),
        },
     }), () => this.validate(this.state.inputValues));
     ....
}

Result

327907.t.mp4

thesahindia · 2022-11-30T21:32:10Z

That looks great @bernhardoj! I need to test all the cases first since it can cause regression. It's EOD for me, I will test this in the morning. Meanwhile can you update the proposal to resolve eslint errors?

thesahindia · 2022-11-30T21:37:05Z

Also cc: @Julesssss. Should we get some opinions on slack ? Concerned because of this part.

bernhardoj · 2022-12-01T10:12:54Z

Sorry, I am not aware there is some eslint errors. I have updated the code. @thesahindia

thesahindia · 2022-12-01T22:47:55Z

Also cc: @Julesssss. Should we get some opinions on slack ? Concerned because of this part.

Gentle bump @Julesssss

Julesssss · 2022-12-02T15:49:08Z

Yeah, given that this has been discussed before we should probably discuss further. I'm going to hold this in the meantime.

melvin-bot · 2022-12-13T22:11:06Z

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

Remove / from placeholder #13441

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

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

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

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2022-12-13T22:11:09Z

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:

[@thesahindia / @Julesssss] The PR that introduced the bug has been identified. Link to the PR:
[@thesahindia / @Julesssss] 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:
[@thesahindia / @Julesssss] 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:
[@kadiealexander] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

kadiealexander · 2022-12-20T04:25:19Z

Paid @thesahindia and @Puneet-here the price + bonus for this issue.

Julesssss · 2022-12-20T11:04:18Z

^ This was not a regression

mallenexpensify · 2022-12-24T01:22:42Z

^ This was not a regression

@Julesssss , we still want BZ to update the regression steps in TestRail so QA can check, right?

melvin-bot · 2022-12-28T18:28:41Z

@Julesssss, @kadiealexander, @thesahindia, @Puneet-here Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot · 2022-12-30T18:28:37Z

@Julesssss, @kadiealexander, @thesahindia, @Puneet-here Eep! 4 days overdue now. Issues have feelings too...

thesahindia · 2023-01-18T13:25:00Z

No actions are needed from me so unassigning.

kadiealexander · 2023-01-19T03:06:32Z

@Julesssss, bumping to get your thoughts on #13133 (comment)

kadiealexander · 2023-01-24T00:41:05Z

Jules is on leave until next week.

kadiealexander · 2023-01-30T17:23:36Z

Not overdue.

Julesssss · 2023-01-30T18:11:54Z

Hey, sorry but I ran out of time for this today. Will return to this tomorrow.

Julesssss · 2023-01-31T12:34:04Z

@Julesssss , we still want BZ to update the regression steps in TestRail so QA can check, right?

I don't think so in this case. This was specifically designed this way originally, and we simpy improved it.

Julesssss · 2023-01-31T12:34:38Z

I'm going to close this due to the above reasoning, but let me know if you disagree @mallenexpensify and we can discuss further

Julesssss added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 29, 2022

Julesssss self-assigned this Nov 29, 2022

melvin-bot bot assigned kadiealexander Nov 29, 2022

kadiealexander added the External Added to denote the issue can be worked on by a contributor label Nov 30, 2022

melvin-bot bot changed the title ~~Mobile keyboard doesn't include / char when expiration field is in focus~~ [$1000] Mobile keyboard doesn't include / char when expiration field is in focus Nov 30, 2022

melvin-bot bot assigned thesahindia Nov 30, 2022

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 30, 2022

Julesssss changed the title ~~[$1000] Mobile keyboard doesn't include / char when expiration field is in focus~~ [HOLD] [$1000] Mobile keyboard doesn't include / char when expiration field is in focus Dec 2, 2022

melvin-bot bot changed the title ~~[HOLD] [$1000] Mobile keyboard doesn't include / char when expiration field is in focus~~ [HOLD for payment 2022-12-20] [HOLD] [$1000] Mobile keyboard doesn't include / char when expiration field is in focus Dec 13, 2022

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 20, 2022

fedirjh mentioned this issue Dec 20, 2022

[Refactor] WorkspaceNewRoomPage Form + Fix From nested children from class component #13713

Merged

60 tasks

mallenexpensify added Weekly KSv2 and removed Daily KSv2 labels Dec 30, 2022

thesahindia removed their assignment Jan 18, 2023

mallenexpensify added Daily KSv2 and removed Reviewing Has a PR in review Weekly KSv2 labels Jan 20, 2023

melvin-bot bot added the Overdue label Jan 23, 2023

melvin-bot bot added Overdue and removed Overdue labels Jan 24, 2023

melvin-bot bot removed the Overdue label Jan 30, 2023

Julesssss closed this as completed Jan 31, 2023

mananjadhav mentioned this issue Jun 21, 2024

[$250] [Payment card / Subscription] No forward slash in the add-payment-card form field for expiration date #44051

Closed

[HOLD for payment 2022-12-20] [HOLD] [$1000] Mobile keyboard doesn't include / char when expiration field is in focus #13133

[HOLD for payment 2022-12-20] [HOLD] [$1000] Mobile keyboard doesn't include / char when expiration field is in focus #13133

Comments

Julesssss commented Nov 29, 2022 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Platform:

Upwork Automation - Do Not Edit

melvin-bot bot commented Nov 29, 2022

Puneet-here commented Nov 29, 2022 • edited Loading

Proposal

mananjadhav commented Nov 29, 2022

Julesssss commented Nov 29, 2022

Puneet-here commented Nov 29, 2022 • edited Loading

kadiealexander commented Nov 30, 2022 • edited Loading

melvin-bot bot commented Nov 30, 2022

melvin-bot bot commented Nov 30, 2022

melvin-bot bot commented Nov 30, 2022

melvin-bot bot commented Nov 30, 2022

thesahindia commented Nov 30, 2022

railway17 commented Nov 30, 2022 • edited Loading

Proposal

varshamb commented Nov 30, 2022

Proposal

thesahindia commented Nov 30, 2022

thesahindia commented Nov 30, 2022

bernhardoj commented Nov 30, 2022 • edited Loading

Proposal

thesahindia commented Nov 30, 2022 • edited Loading

thesahindia commented Nov 30, 2022 • edited Loading

bernhardoj commented Dec 1, 2022

thesahindia commented Dec 1, 2022

Julesssss commented Dec 2, 2022

melvin-bot bot commented Dec 13, 2022 • edited by kadiealexander Loading

melvin-bot bot commented Dec 13, 2022 • edited by Julesssss Loading

kadiealexander commented Dec 20, 2022

Julesssss commented Dec 20, 2022

mallenexpensify commented Dec 24, 2022

melvin-bot bot commented Dec 28, 2022

melvin-bot bot commented Dec 30, 2022

thesahindia commented Jan 18, 2023

kadiealexander commented Jan 19, 2023

kadiealexander commented Jan 24, 2023

kadiealexander commented Jan 30, 2023

Julesssss commented Jan 30, 2023

Julesssss commented Jan 31, 2023

Julesssss commented Jan 31, 2023

[HOLD for payment 2022-12-20] [HOLD] [$1000] Mobile keyboard doesn't include `/` char when expiration field is in focus #13133

[HOLD for payment 2022-12-20] [HOLD] [$1000] Mobile keyboard doesn't include `/` char when expiration field is in focus #13133

Julesssss commented Nov 29, 2022 •

edited by melvin-bot bot

Loading

Puneet-here commented Nov 29, 2022 •

edited

Loading

Puneet-here commented Nov 29, 2022 •

edited

Loading

kadiealexander commented Nov 30, 2022 •

edited

Loading

railway17 commented Nov 30, 2022 •

edited

Loading

bernhardoj commented Nov 30, 2022 •

edited

Loading

thesahindia commented Nov 30, 2022 •

edited

Loading

thesahindia commented Nov 30, 2022 •

edited

Loading

melvin-bot bot commented Dec 13, 2022 •

edited by kadiealexander

Loading

melvin-bot bot commented Dec 13, 2022 •

edited by Julesssss

Loading