Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(search): Debounce and phone number validation #3124

Closed
wants to merge 11 commits into from
Closed

feat(search): Debounce and phone number validation #3124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
74f0c59
feat: Debouncing of search with phone number validation
pranshuchittora May 25, 2021
f1ee5ff
feat: Loader on search page
pranshuchittora May 18, 2021
f9f000a
feat: Design changes
pranshuchittora May 20, 2021
3effa64
feat: No email message
pranshuchittora May 25, 2021
cf25b91
feat: Err msg or results not both
pranshuchittora Jun 6, 2021
2000719
Merge remote-tracking branch 'origin/main' into pranshuchittora/fix/2936
pranshuchittora Jun 7, 2021
8a8b1cf
feat: Liniting changes
pranshuchittora Jun 7, 2021
4c41ecc
fix: Minor linitng changes
pranshuchittora Jun 7, 2021
39581a4
fix: Minor styling fixes
pranshuchittora Jun 9, 2021
429d9aa
Merge remote-tracking branch 'origin/main' into pranshuchittora/fix/2936
pranshuchittora Jun 9, 2021
cfb09ce
feat: PR review changes
pranshuchittora Jun 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/components/OptionsList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,13 +206,13 @@ class OptionsList extends Component {
render() {
return (
<View style={this.props.listContainerStyles}>
{this.props.headerMessage ? (
<View style={[styles.ph5, styles.pb5]}>
<View>
<View style={[styles.ph5]}>
<Text style={[styles.textLabel, styles.colorMuted]}>
{this.props.headerMessage}
</Text>
</View>
) : null}
</View>
<SectionList
ref={this.props.innerRef}
bounces={false}
Expand Down
4 changes: 3 additions & 1 deletion src/components/OptionsSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ class OptionsSelector extends Component {
<TextInputWithFocusStyles
styleFocusIn={[styles.textInputReversedFocus]}
ref={el => this.textInput = el}
style={[styles.textInput]}
style={[styles.textInput, styles.flex1]}
value={this.props.value}
onChangeText={this.props.onChangeText}
onKeyPress={this.handleKeyPress}
Expand All @@ -203,6 +203,7 @@ class OptionsSelector extends Component {
placeholderTextColor={themeColors.placeholderText}
/>
</View>

<OptionsList
ref={el => this.list = el}
optionHoveredStyle={styles.hoveredComponentBG}
Expand All @@ -218,6 +219,7 @@ class OptionsSelector extends Component {
forceTextUnreadStyle={this.props.forceTextUnreadStyle}
showTitleTooltip={this.props.showTitleTooltip}
/>

</View>
);
}
Expand Down
1 change: 1 addition & 0 deletions src/languages/en.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ export default {
cameraPermissionsNotGranted: 'Camera permissions not granted',
messages: {
noPhoneNumber: 'Please enter a phone number including the country code e.g +447814266907',
noEmailOrPhone: 'Please enter a valid email address or phone number.',
maxParticipantsReached: 'You\'ve reached the maximum number of participants for a group chat.',
},
onfidoStep: {
Expand Down
14 changes: 14 additions & 0 deletions src/libs/API.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,19 @@ function GetCurrencyList() {
return Mobile_GetConstants({data: ['currencyList']});
}

/**
* Validates PhoneNumber
*
* @param {Object} parameters
* @param {String} parameters.phoneNumber
* @returns {Promise}
*/
function IsValidPhoneNumber(parameters) {
const commandName = 'IsValidPhoneNumber';
requireParameters(['phoneNumber'], parameters, commandName);
return Network.post(commandName, parameters);
}

export {
Authenticate,
BankAccount_Create,
Expand All @@ -860,6 +873,7 @@ export {
GetPolicySummaryList,
GetRequestCountryCode,
Graphite_Timer,
IsValidPhoneNumber,
Log,
PayIOU,
PayWithWallet,
Expand Down
154 changes: 114 additions & 40 deletions src/pages/SearchPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, {Component} from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'lodash';
import {withOnyx} from 'react-native-onyx';
import OptionsSelector from '../components/OptionsSelector';
import {getSearchOptions, getHeaderMessage} from '../libs/OptionsListUtils';
Expand All @@ -18,6 +19,7 @@ import CONST from '../CONST';
import FullScreenLoadingIndicator from '../components/FullscreenLoadingIndicator';
import withLocalize, {withLocalizePropTypes} from '../components/withLocalize';
import compose from '../libs/compose';
import * as API from '../libs/API';

const personalDetailsPropTypes = PropTypes.shape({
/** The login of the person (either email or phone number) */
Expand Down Expand Up @@ -51,6 +53,9 @@ const propTypes = {
email: PropTypes.string.isRequired,
}).isRequired,

/** */
countryCode: PropTypes.string.isRequired,

/** Window Dimensions Props */
...windowDimensionsPropTypes,

Expand All @@ -74,9 +79,14 @@ class SearchPage extends Component {
'',
props.betas,
);
this.validateInput = _.debounce(this.validateInput.bind(this), 300);


this.preserveRecentReports = recentReports;

this.state = {
searchValue: '',
headerMessage: '',
recentReports,
personalDetails,
userToInvite,
Expand All @@ -88,10 +98,10 @@ class SearchPage extends Component {
}

/**
* Returns the sections needed for the OptionsSelector
*
* @returns {Array}
*/
* Returns the sections needed for the OptionsSelector
*
* @returns {Array}
*/
getSections() {
const sections = [{
title: this.props.translate('common.recents'),
Expand All @@ -101,48 +111,112 @@ class SearchPage extends Component {
}];

if (this.state.userToInvite) {
sections.push(({
sections.push({
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
undefined,
data: [this.state.userToInvite],
shouldShow: true,
indexOffset: 0,
}));
});
}

return sections;
}

/**
* Reset the search value and redirect to the selected report
*
* @param {Object} option
*/
* Reset the search value and redirect to the selected report
* @param {Object} option
*/
selectReport(option) {
if (!option) {
return;
}

if (option.reportID) {
this.setState(
{
searchValue: '',
},
() => {
Navigation.navigate(ROUTES.getReportRoute(option.reportID));
},
);
} else {
fetchOrCreateChatReport([this.props.session.email, option.login]);
}
}


/**
* Validates search input via regexes and validation API (phone numbers only)
* @param {String} searchValue
* @returns {void}
*/
validateInput(searchValue) {
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we're going to have to move this into OptionsListUtils since the validation you're adding is going to apply to all pages that have account-search functionality. Can you figure out how we can reuse getSearchOptions here (by editing getOptions)? I'm thinking we can have getOptions also return headerMessage

Copy link
Contributor Author

@pranshuchittora pranshuchittora Jun 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discussed this with you earlier. That my initial approach was to add the changes in getOptions only but it's not flexible enough and might require good amount of refactoring.

Because there are a lot of things happening internally and the code is a bit messy and adding more code will increase the complexity and maintainability efforts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah you did? I'm sorry I don't remember you bringing up that approach, maybe I misunderstood :O

Hmm I don't see why we can't refactor getHeaderMessage just a bit, and make getOptions also return something like headerMessage: getMeaderMessage(...). Here's why I'm thinking this will work (please let me know if you disagree on these points):

  1. getHeaderMessage takes only a few parameters, and all of the params should be able to be calculated from other variables inside getOptions
  2. getOptions is only used in OptionsListUtils, which has get...Options functions for specific pages, all of those pages will need this type of functionality (the same type of logic for headerMessage)

Copy link
Contributor Author

@pranshuchittora pranshuchittora Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Beamanator
I tried merging these changes into utils. So getSearchOptions & getHeaderMessage are independent of each other. They both take the searchValue as one of the input. With this, we might end up hitting the phone number validation API twice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm once you push your latest changes I can check and see if I can help come up with a way to only hit that validation API once 👍

if (!searchValue) {
return;
}

let modifiedSearchValue = searchValue;
const headerMessage = getHeaderMessage(
this.state.personalDetails.length !== 0,
false,
searchValue,
);
Comment on lines +160 to +164
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this function, it looks like the only time we're using this headerMessage is if the the phone number is invalid, so I think it would be better if we just do headerMessage: translate(preferredLocale, 'messages.noPhoneNumber') directly in the else statement below - since this will only be displayed if the modifiedSearchValue is an invalid phone number - we shouldn't need to call getHeaderMessage right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am keeping the call to getHeaderMessage because if we want to make any change in the underlying messaging part then we have only one place to change. Or a single source of truth.


if (/^[0-9]+$/.test(searchValue) || /^[0-9]*$/.test(searchValue)) {
// Appends country code
if (!searchValue.includes('+')) {
modifiedSearchValue = `+${this.props.countryCode}${searchValue}`;
}
API.IsValidPhoneNumber({phoneNumber: modifiedSearchValue}).then(
(resp) => {
// Early return if the user had cleared the input before the API responsed.
if (!this.state.searchValue) { return; }
Beamanator marked this conversation as resolved.
Show resolved Hide resolved

if (resp.isValid) {
const {
recentReports,
personalDetails,
userToInvite,
} = getSearchOptions(
this.props.reports,
this.props.personalDetails,
searchValue,
);
this.setState({
userToInvite,
recentReports,
personalDetails,
headerMessage: '',
});
} else {
this.setState({
recentReports: [],
userToInvite: null,
headerMessage,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, let's do something like headerMessage: translate(preferredLocale, 'messages.noPhoneNumber') here

});
}
},
);
} else {
const {recentReports, personalDetails, userToInvite} = getSearchOptions(
this.props.reports,
this.props.personalDetails,
searchValue,
);
this.setState({
searchValue: '',
}, () => {
Navigation.navigate(ROUTES.getReportRoute(option.reportID));
userToInvite,
recentReports,
personalDetails,
headerMessage: recentReports.length + personalDetails.length
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could be cleaner to just make this else an else if (Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue)), where headerMessage should always be '', then add an else which displays the this.props.translate('messages.noEmailOrPhone') error message, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validation is not required as it is done internally by getSearchOptions

=== 0 && !userToInvite ? this.props.translate('messages.noEmailOrPhone') : '',
});
} else {
fetchOrCreateChatReport([
this.props.session.email,
option.login,
]);
}
}

render() {
const sections = this.getSections();
const headerMessage = getHeaderMessage(
(this.state.recentReports.length + this.state.personalDetails.length) !== 0,
Boolean(this.state.userToInvite),
this.state.searchValue,
);

return (
<ScreenWrapper>
{({didScreenTransitionEnd}) => (
Expand All @@ -159,24 +233,21 @@ class SearchPage extends Component {
value={this.state.searchValue}
onSelectRow={this.selectReport}
onChangeText={(searchValue = '') => {
const {
recentReports,
personalDetails,
userToInvite,
} = getSearchOptions(
this.props.reports,
this.props.personalDetails,
searchValue,
this.props.betas,
);
this.setState({
searchValue,
userToInvite,
recentReports,
personalDetails,
});
this.setState({searchValue});

// Clears the header message on clearing the input
if (!searchValue) {
this.validateInput.cancel();
this.setState({
headerMessage: '',
userToInvite: null,
recentReports: this.preserveRecentReports,
});
} else {
this.validateInput(searchValue);
}
}}
headerMessage={headerMessage}
headerMessage={this.state.headerMessage}
hideSectionHeaders
hideAdditionalOptionStates
showTitleTooltip
Expand Down Expand Up @@ -207,6 +278,9 @@ export default compose(
session: {
key: ONYXKEYS.SESSION,
},
countryCode: {
key: ONYXKEYS.COUNTRY_CODE,
},
betas: {
key: ONYXKEYS.BETAS,
},
Expand Down