[HOLD for payment 2022-05-23] [$250] Auto-handle phone number format by stripping any parentheses/dashes on submit - Reported by @thesahindia

[mvtglobally]

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 app
2. Navigate to Settings > Workspace > Connect bank account
3. Proceed with VBA flow to Company information page
4. Enter phone number in different formats (e.g with/without parentheses/dashes)

Expected Result:

User should be able to enter phone number in any format

Actual Result:

The phone number placeholder suggests that the input should be in this format (xxx)xxx-xxxx however, it will throw an error if we do that. So, either the placeholder should be updated or the input should be handled accordingly.

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.1.24-0 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/43995119/148008018-4a6b6ec5-cdbb-4d2a-92d8-0287b82abc99.mov

Expensify/Expensify Issue URL: Issue reported by: @thesahindia Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1640355502103900

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @puneetlath (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[MelvinBot]

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[thesahindia]

Proposal

using the below function, we can remove the special characters from the phone number and avoid the error that we are getting right now

    stripSpecialCharacters(number) {
        return number.replace(CONST.REGEX.NUMBERS_WITHOUT_SPECIAL_CHARS, '');
    }

Inside https://github.com/Expensify/App/blob/main/src/pages/ReimbursementAccount/CompanyStep.js the submit function will look like this -

 submit() {
        if (!this.validate()) {
            BankAccounts.showBankAccountErrorModal();
            return;
        }

        const incorporationDate = moment(this.state.incorporationDate).format(CONST.DATE.MOMENT_FORMAT_STRING);
        const companyPhone = this.stripSpecialCharacters(this.state.companyPhone);
        BankAccounts.setupWithdrawalAccount({...this.state, incorporationDate, companyPhone});
    }

[MelvinBot]

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

[MelvinBot]

Current assignee @puneetlath is eligible for the Exported assigner, not assigning anyone new.

[puneetlath]

Upwork job is here: https://www.upwork.com/jobs/~01e285df2aaa09959a

[parasharrajat]

@thesahindia Where will you do these changes?

[thesahindia]

@thesahindia Where will you do these changes?

Sorry, forgot to add that!

This is the file I'm referring : https://github.com/Expensify/App/blob/main/src/pages/ReimbursementAccount/CompanyStep.js

[parasharrajat]

Ok, Thanks for linking. I think we should update all of our phone input to behave the same. So Could you please list down the places where we have a phone input and create a component for phone input and use it in all places?

let me know if you have any questions.

[rushatgabhane]

@thesahindia I'm interested in this regex NUMBERS_WITHOUT_SPECIAL_CHARS, could you please comment it here?

Asking because you're sending it to the API, and it may not work for numbers with an extension. (Seen phone extensions mentioned in #7067 ) correct me if I'm wrong :) cc: @parasharrajat

[thesahindia]

@rushatgabhane, sure.

NUMBERS_WITHOUT_SPECIAL_CHARS: /[^0-9.]/g

These are the places where we have a phone input -

1. RequestCallPage
2. AdditionalDetailsStep
3. CompanyStep
4. AddSecondaryLoginPage

cc: @parasharrajat

[parasharrajat]

Thanks for sharing @thesahindia. Could you please share a proposal on how are you planning to solve this issue based of the requested changes?

[thesahindia]

@parasharrajat

So Could you please list down the places where we have a phone input and create a component for phone input and use it in all places?

I had one question regarding this request. Since, we are only stripping or ignoring the value from the phone input just before submitting the form. And the phone input will still have the same value as it is [eg : (202)888-1122 ] .. No change is being made. So, how is creating another component for phone Input will help since the functionality remains the same.

I think to have the same behaviour, we would need to update the submit function in other places as well. Also, the placeholder in other places would need to be changed.

submit() {
     if (!this.validate()) {
         BankAccounts.showBankAccountErrorModal();
         return;
     }

     const incorporationDate = moment(this.state.incorporationDate).format(CONST.DATE.MOMENT_FORMAT_STRING);
     const companyPhone = this.stripSpecialCharacters(this.state.companyPhone);
     BankAccounts.setupWithdrawalAccount({...this.state, incorporationDate, companyPhone});
 }

[parasharrajat]

Yeah, the plan was to keep the same phone input format for all the places, same validations, and format, etc.

One way of doing this would be:

1. Create a PhoneInput component that does the following: a) Allow the user to type the phone with braces and other special chars. b) sets the parent component's state with only the phone number and no special chars. c) sets the parent component's state with an error if any.

Let's ask @puneetlath if we want to do that. Currently, I see we have validations and Striping logic in each of these components for phone input. I see a possible opportunity to dry this up and avoid errors like this issue in the future.

@puneetlath What do you say?

[puneetlath]

I think it's a good idea. I like it.

[parasharrajat]

@thesahindia Waiting for your updated proposal...

[thesahindia]

@parasharrajat, I will create a PhoneTextInput component and will have another function similar to onChangeText which will provide the formatted value ..since we need the regular onChangeText to let the user write special characters as well. And all errors and other functionality will just be the same as regular TextInput comp.

onChangeFormattedText={formattedPhoneNumber => this.setState({formattedPhoneNumber})}

[parasharrajat]

Could you please lay down the outline of logic that would be contained in the component and the clean up you would do from the places, it will be used?

[thesahindia]

Sure, here's what I'm thinking

Will create these three files for PhoneTextInput :

src/components/PhoneTextInput/index.js

import React, {forwardRef} from 'react';
import styles from '../../styles/styles';
import * as baseTextPhoneInputPropTypes from './baseTextPhoneInputPropTypes';
import BaseTextPhoneInput from './BaseTextPhoneInput';

const PhoneTextInput = forwardRef((props, ref) => (
    <BaseTextPhoneInput
        // eslint-disable-next-line react/jsx-props-no-spreading
        {...props}
        innerRef={ref}
        inputStyle={[styles.baseTextInput, styles.textInputDesktop]}
    />
));

PhoneTextInput.propTypes = baseTextPhoneInputPropTypes.propTypes;
PhoneTextInput.defaultProps = baseTextPhoneInputPropTypes.defaultProps;
PhoneTextInput.displayName = 'PhoneTextInput';

export default PhoneTextInput;

src/components/PhoneTextInput/BasePhoneTextInput.js

import _ from 'underscore';
import React, {Component} from 'react';
import {
    Animated, View, TouchableWithoutFeedback,
} from 'react-native';
import Str from 'expensify-common/lib/str';
import TextInputLabel from '../TextInput/TextInputLabel';
import themeColors from '../../styles/themes/default';
import styles from '../../styles/styles';
import InlineErrorText from '../InlineErrorText';
import * as styleConst from '../TextInput/styleConst';
import TextInputWithName from '../TextInputWithName';
import CONST from '../../CONST';
import * as basePhoneTextnputPropTypes from './basePhoneTextInputPropTypes';

class BasePhoneTextInput extends Component {
    constructor(props) {
        super(props);

        this.value = props.value || props.defaultValue || '';
        this.formattedValue = this.stripSpecialCharacters(props.value || props.defaultValue || '');
        const activeLabel = props.forceActiveLabel || this.value.length > 0;

        this.state = {
            isFocused: false,
            labelTranslateY: new Animated.Value(activeLabel ? styleConst.ACTIVE_LABEL_TRANSLATE_Y : styleConst.INACTIVE_LABEL_TRANSLATE_Y),
            labelScale: new Animated.Value(activeLabel ? styleConst.ACTIVE_LABEL_SCALE : styleConst.INACTIVE_LABEL_SCALE),
        };

        this.input = null;
        this.isLabelActive = activeLabel;
        this.onPress = this.onPress.bind(this);
        this.onFocus = this.onFocus.bind(this);
        this.onBlur = this.onBlur.bind(this);
        this.setValue = this.setValue.bind(this);
    }

    componentDidMount() {
        // We are manually managing focus to prevent this issue: https://github.com/Expensify/App/issues/4514
        if (!this.props.autoFocus || !this.input) {
            return;
        }

        this.input.focus();
    }

    componentDidUpdate(prevProps) {
        // activate or deactivate the label when value is changed programmatically from outside
        if (prevProps.value === this.props.value) {
            return;
        }

        this.value = this.props.value;
        this.formattedValue = this.props.value;
        if (this.props.value) {
            this.activateLabel();
        } else if (!this.state.isFocused) {
            this.deactivateLabel();
        }
    }

    onPress(event) {
        if (this.props.disabled) {
            return;
        }

        if (this.props.onPress) {
            this.props.onPress(event);
        }

        if (!event.isDefaultPrevented()) {
            this.input.focus();
        }
    }

    onFocus(event) {
        if (this.props.onFocus) { this.props.onFocus(event); }
        this.setState({isFocused: true});
        this.activateLabel();
    }

    onBlur(event) {
        if (this.props.onBlur) { this.props.onBlur(event); }
        this.setState({isFocused: false});
        this.deactivateLabel();
    }

    /**
     * Set Value & activateLabel
     *
     * @param {String} value
     * @memberof BasePhoneTextInput
     */
    setValue(value) {
        this.value = value;
        this.formattedValue = LoginUtil.getPhoneNumberWithoutSpecialChars(value);
        Str.result(this.props.onChangeFormattedText, this.formattedValue);
        Str.result(this.props.onChangeText, value);
        this.activateLabel();
    }

    activateLabel() {
        if (this.value.length < 0 || this.isLabelActive) {
            return;
        }

        this.animateLabel(
            styleConst.ACTIVE_LABEL_TRANSLATE_Y,
            styleConst.ACTIVE_LABEL_SCALE,
        );
        this.isLabelActive = true;
    }

    deactivateLabel() {
        if (this.props.forceActiveLabel || this.value.length !== 0) {
            return;
        }

        this.animateLabel(styleConst.INACTIVE_LABEL_TRANSLATE_Y, styleConst.INACTIVE_LABEL_SCALE);
        this.isLabelActive = false;
    }

    animateLabel(translateY, scale) {
        Animated.parallel([
            Animated.spring(this.state.labelTranslateY, {
                toValue: translateY,
                duration: 80,
                useNativeDriver: true,
            }),
            Animated.spring(this.state.labelScale, {
                toValue: scale,
                duration: 80,
                useNativeDriver: true,
            }),
        ]).start();
    }

    render() {
        // eslint-disable-next-line react/forbid-foreign-prop-types
        const inputProps = _.omit(this.props, _.keys(basePhoneTextnputPropTypes.propTypes));
        const hasLabel = Boolean(this.props.label.length);
        return (
            <View>
                <View
                    style={[
                        !this.props.multiline && styles.componentHeightLarge,
                        ...this.props.containerStyles,
                    ]}
                >
                    <TouchableWithoutFeedback onPress={this.onPress} focusable={false}>
                        <View
                            style={[
                                styles.textInputContainer,
                                this.state.isFocused && styles.borderColorFocus,
                                (this.props.hasError || this.props.errorText) && styles.borderColorDanger,
                            ]}
                        >
                            {hasLabel ? (
                                <>
                                    {/* Adding this background to the label only for multiline text input,
                                    to prevent text overlapping with label when scrolling */}
                                    {this.props.multiline && <View style={styles.textInputLabelBackground} pointerEvents="none" />}
                                    <TextInputLabel
                                        label={this.props.label}
                                        labelTranslateY={this.state.labelTranslateY}
                                        labelScale={this.state.labelScale}
                                        for={this.props.nativeID}
                                    />
                                </>
                            ) : null}
                            <View style={[styles.textInputAndIconContainer]}>
                                <TextInputWithName
                                    ref={(ref) => {
                                        if (typeof this.props.innerRef === 'function') { this.props.innerRef(ref); }
                                        this.input = ref;
                                    }}
                                    // eslint-disable-next-line
                                    {...inputProps}
                                    value={this.value}
                                    placeholder={(this.state.isFocused || !this.props.label) ? this.props.placeholder : null}
                                    placeholderTextColor={themeColors.placeholderText}
                                    underlineColorAndroid="transparent"
                                    style={[this.props.inputStyle, styles.flex1, styles.w100, !hasLabel && styles.pv0]}
                                    multiline={this.props.multiline}
                                    onFocus={this.onFocus}
                                    onBlur={this.onBlur}
                                    onChangeText={this.setValue}
                                    onPressOut={this.props.onPress}
                                    name={this.props.name}
                                />
                            </View>
                        </View>
                    </TouchableWithoutFeedback>
                </View>
                {!_.isEmpty(this.props.errorText) && (
                    <InlineErrorText>
                        {this.props.errorText}
                    </InlineErrorText>
                )}
            </View>
        );
    }
}

BasePhoneTextInput.propTypes = basePhoneTextnputPropTypes.propTypes;
BasePhoneTextInput.defaultProps = basePhoneTextnputPropTypes.defaultProps;

export default BasePhoneTextInput;

src/components/PhoneTextInput/basePhoneTextInputPropTypes.js

import PropTypes from 'prop-types';

const propTypes = {
    /** Input label */
    label: PropTypes.string,

    /** Name attribute for the input */
    name: PropTypes.string,

    /** Input value */
    value: PropTypes.string,

    /** Default value - used for non controlled inputs */
    defaultValue: PropTypes.string,

    /** Input value placeholder */
    placeholder: PropTypes.string,

    /** Error text to display */
    errorText: PropTypes.string,

    /** Should the input be styled for errors  */
    hasError: PropTypes.bool,

    /** Customize the TextInput container */
    containerStyles: PropTypes.arrayOf(PropTypes.object),

    /** input style */
    inputStyle: PropTypes.arrayOf(PropTypes.object),

    /** If present, this prop forces the label to remain in a position where it will not collide with input text */
    forceActiveLabel: PropTypes.bool,

    /** Should the input auto focus? */
    autoFocus: PropTypes.bool,
};

const defaultProps = {
    label: '',
    name: '',
    errorText: '',
    placeholder: '',
    hasError: false,
    containerStyles: [],
    inputStyle: [],
    autoFocus: false,

    /**
     * To be able to function as either controlled or uncontrolled component we should not
     * assign a default prop value for `value` or `defaultValue` props
     */
    value: undefined,
    defaultValue: undefined,
    forceActiveLabel: false,
};

export {propTypes, defaultProps};

Use

for RequestCallPage.js

1. add formattedPhoneNumber: null after line 92 https://github.com/Expensify/App/blob/ca085ae41de731893bf6c99cfd3401533ef1196e/src/pages/RequestCallPage.js#L90-L94

2. On line 143 I will remove LoginUtil.getPhoneNumberWithoutSpecialChars and will pass formattedPhoneNumber https://github.com/Expensify/App/blob/ca085ae41de731893bf6c99cfd3401533ef1196e/src/pages/RequestCallPage.js#L138-L144

3. Same as 2 for line 165 https://github.com/Expensify/App/blob/ca085ae41de731893bf6c99cfd3401533ef1196e/src/pages/RequestCallPage.js#L164-L169

4. Will replace TextInput with PhoneTextInput and I will add onChangeFormattedText={formattedPhoneNumber => this.setState({formattedPhoneNumber})} https://github.com/Expensify/App/blob/ca085ae41de731893bf6c99cfd3401533ef1196e/src/pages/RequestCallPage.js#L305-L314

will do the similar changes for other pages as well

[parasharrajat]

Hmm, I see. But you are not utilizing composition instead duplicating a lot of code. Here is the outline.

1. we should create a PhoneTextInput component.
2. That wraps TextInput component.
3. This component's only duty would be to maintain state value and call onChangeText from props.
4. OnChangeText, it should remove the special chars and return the value.

let me know if you have any questions. Otherwise, if you are to follow the above steps what is going to be your proposal now.

[thesahindia]

Have one question regarding the 4th point, if we return the formatted value then the user will not be able to write the number in (xxx)xxx-xxxx form since it will always go back to numbers only format on each change. I thought we wanted 2 values, one is for user to allow the use of special chars and other one to use it when we submit the form

[parasharrajat]

Yeah, you are correct. That's why we will create a local state in the component that will preserve user input.

[thesahindia]

@parasharrajat,

updated proposal

import React from 'react';
import Str from 'expensify-common/lib/str';
import * as phoneTextnputPropTypes from './phoneTextnputPropTypes';
import TextInput from '../TextInput';
import LoginUtil from '../../libs/LoginUtil';

class PhoneTextInput extends React.Component {
    constructor(props) {
        super(props);

        this.value = props.value || props.defaultValue || '';
        this.setValue = this.setValue.bind(this);
    }

    setValue(val) {
        this.value = val;
        const formattedValue = LoginUtil.getPhoneNumberWithoutSpecialChars(val);
        Str.result(this.props.onChangeText, formattedValue);
    }

    render() {
        return (
            <TextInput
                ref={el => this.textInput = el}
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...this.props}
                value={this.value}
                onChangeText={this.setValue}

            />
        );
    }
}

PhoneTextInput.propTypes = phoneTextnputPropTypes.propTypes;
PhoneTextInput.defaultProps = phoneTextnputPropTypes.defaultProps;

export default React.forwardRef((props, ref) => (
    /* eslint-disable-next-line react/jsx-props-no-spreading */
    <PhoneTextInput {...props} forwardedRef={ref} />
));

[parasharrajat]

Ok, sounds good. That will work. @thesahindia 's proposal looks good.

cc: @puneetlath :ribbon: :eyes: :ribbon: C+ reviewed

[MelvinBot]

📣 @thesahindia You have been assigned to this job by @puneetlath! Please apply to this job in Upwork 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 📖

[puneetlath]

Sounds good to me too. @thesahindia I sent you a hiring offer in Upwork.

[puneetlath]

How is this issue going?

[thesahindia]

@puneetlath, made all the required changes just need a review now.

[puneetlath]

Great! Just comment on the PR asking for a re-review.

[puneetlath]

Review is ongoing.

[puneetlath]

Not overdue. PR ongoing.

[puneetlath]

PR ongoing. Not overdue. I think we're almost there.

[MelvinBot]

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

[MelvinBot]

Current assignee @parasharrajat is eligible for the Exported assigner, not assigning anyone new.

[MelvinBot]

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

[puneetlath]

@kadiealexander @marcaaron I'm handing this over to y'all since I'm going on my two month sabbatical now.

@marcaaron I've assigned you to the PR as well.

@kadiealexander Contract for the fix: https://www.upwork.com/ab/c/8577561/contracts/29255931#milestones%2F20220203%2F20220304 Looks like there isn't a C+ contract so we'll need to create that as well.

[kadiealexander]

@parasharrajat could you please apply to this job so I can ensure you're paid as C+ for this issue?

https://www.upwork.com/jobs/~014e81c4ea89fec588

[kelvinkaicheung]

There is a function I would use to remove the ( ) and - accept my proposal on upwork to continue

[kadiealexander]

@kelvinkaicheung before we accept on Upwork we will need to review a detailed proposal from you for how this would be fixed, including the functions used, and we would then need to decide that your proposal is best suited. Please update your proposal to include this detail and the team will review this for you and determine whether this is a suitable fix.

[parasharrajat]

@kadiealexander Help Wanted label is accidental here. It got added on reapplying Exported label. This issue is no more open for proposals. Thanks.

[kelvinkaicheung]

There is not much further I can say. Accept my help on upwork and I will start fixing this small bug.

[kadiealexander]

@kelvinkaicheung, as @parasharrajat mentioned above, this issue has already been assigned to another contributor for fixing (sorry for missing this).

[parasharrajat]

Any update @thesahindia on the PR?

[thesahindia]

Yeah I am on it, will push the changes in few hours.

[thesahindia]

I have asked some questions related to the PR, can you share your views on it. Here's the slack thread cc: @marcaaron

[marcaaron]

Hmm sorry, what's your question exactly @thesahindia ?

[thesahindia]

1. We have changed the error message for invalid phone number at some places and AddSecondaryLoginPage.js is left. It shows the error from the backend and I think we don't really need to change the error message here as the error message is fine on this one. It says: I couldn't validate the phone number, please try again with the country code (e.g. +15005550006) so should I leave this one?

2. We have changed the placeholder for all the phone number inputs but on Add Phone number page we don't have one, so should I add one there? Edit: We can leave it as it is. (slack comment)

[kadiealexander]

Please ignore the above, I updated the wrong issue.

[parasharrajat]

@thesahindia Bump..

[thesahindia]

@parasharrajat, PR is ready just need to resolve some conflicts.

1. We have changed the error message for invalid phone number at some places and AddSecondaryLoginPage.js is left. It shows the error from the backend and I think we don't really need to change the error message here as the error message is fine on this one. It says: I couldn't validate the phone number, please try again with the country code (e.g. +15005550006) so should I leave this one?

I was just waiting for the decision on this.

@marcaaron, do we wanna change the error message here?