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

Fix: Private personal details are not fetched when navigating by direct link #22906

Merged
merged 18 commits into from
Jul 25, 2023
Merged

Fix: Private personal details are not fetched when navigating by direct link #22906

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
b6f562c
refactor: private personal details prop types
tienifr Jul 14, 2023
17ee08a
usePrivatePersonalDetails hook
tienifr Jul 14, 2023
eed1ecc
isLoading private personal details
tienifr Jul 14, 2023
0078199
pass context value
tienifr Jul 14, 2023
f418f98
modify onyx key
tienifr Jul 14, 2023
d786e80
apply HOC in affected pages
tienifr Jul 14, 2023
6dc0046
Merge branch 'main' into fix/22606
tienifr Jul 14, 2023
edc1cfe
fix lint
tienifr Jul 14, 2023
755701d
added prop types for HOC
tienifr Jul 14, 2023
1df7bff
add session prop
tienifr Jul 15, 2023
4e0bc85
add shouldComponentUpdate in
tienifr Jul 17, 2023
b3a8c30
refactor
tienifr Jul 17, 2023
f17ca65
fix lint
tienifr Jul 17, 2023
5bd9c3a
fix: 22606 use hook to fetch private personal detail data
tienifr Jul 24, 2023
d452118
fix: add wrapper component
tienifr Jul 24, 2023
ff8ff30
merge main
tienifr Jul 24, 2023
7df5cf8
Revert "add wrapper component"
tienifr Jul 25, 2023
60ed833
fix comment and add shouldEnableMaxHeight
tienifr Jul 25, 2023
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
7 changes: 7 additions & 0 deletions src/components/NewDatePicker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,13 @@ class NewDatePicker extends React.Component {
this.defaultValue = props.defaultValue ? moment(props.defaultValue).format(CONST.DATE.MOMENT_FORMAT_STRING) : '';
}

componentDidUpdate(prevProps) {
if (prevProps.value === this.props.value) {
return;
}
this.setDate(this.props.value);
}
tienifr marked this conversation as resolved.
Show resolved Hide resolved

/**
* Trigger the `onInputChange` handler when the user input has a complete date or is cleared
* @param {string} selectedDate
Expand Down
17 changes: 17 additions & 0 deletions src/hooks/usePrivatePersonalDetails.js
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we using similar pattern elsewhere? I think it would be better to add PersonalDetails.openPersonalDetailsPage call along with the OpenApp call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm not yet, but I don't think fetching personalDetails along with OpenApp is a good idea. Firstly, we may not access personalDetail page, so fetching personalDetails at first is unnecessary. Secondly, we have many places that use openPage like PersonalDetails.openPublicProfilePage(accountID) and it's not executed along with OpenApp

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... You do have a valid point. Can you think of a way so that we just have to rely on onyx for getting data inside the component instead of using this hook?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're still using data from onyx inside the component. we just use the hook to fetch data if needed and don't worry about the performance because we have the logic to prevent fetching data again if it's already in onyx

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about this again, I think this approach is fine actually. We won't have to repeat the usage of useEffect on every page where we need this data.

Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import {useEffect, useContext} from 'react';
import * as PersonalDetails from '../libs/actions/PersonalDetails';
import {NetworkContext} from '../components/OnyxProvider';

/**
* Hook for fetching private personal details
*/
export default function usePrivatePersonalDetails() {
const {isOffline} = useContext(NetworkContext);

useEffect(() => {
if (isOffline || Boolean(PersonalDetails.getPrivatePersonalDetails())) {
return;
}
PersonalDetails.openPersonalDetailsPage();
}, [isOffline]);
}
47 changes: 46 additions & 1 deletion src/libs/actions/PersonalDetails.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ Onyx.connect({
callback: (val) => (allPersonalDetails = val),
});

let privatePersonalDetails;
Onyx.connect({
key: ONYXKEYS.PRIVATE_PERSONAL_DETAILS,
callback: (val) => (privatePersonalDetails = val),
});

tienifr marked this conversation as resolved.
Show resolved Hide resolved
/**
* Returns the displayName for a user
*
Expand Down Expand Up @@ -325,7 +331,37 @@ function updateSelectedTimezone(selectedTimezone) {
* Fetches additional personal data like legal name, date of birth, address
*/
function openPersonalDetailsPage() {
API.read('OpenPersonalDetailsPage');
const optimisticData = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PRIVATE_PERSONAL_DETAILS,
value: {
isLoading: true,
},
},
];

const successData = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PRIVATE_PERSONAL_DETAILS,
value: {
isLoading: false,
},
},
];

const failureData = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.PRIVATE_PERSONAL_DETAILS,
value: {
isLoading: false,
},
},
];

API.read('OpenPersonalDetailsPage', {}, {optimisticData, successData, failureData});
tienifr marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -478,6 +514,14 @@ function clearAvatarErrors() {
});
}

/**
* Get private personal details value
* @returns {Boolean}
tienifr marked this conversation as resolved.
Show resolved Hide resolved
*/
function getPrivatePersonalDetails() {
return privatePersonalDetails;
}

export {
getDisplayName,
getDisplayNameForTypingIndicator,
Expand All @@ -495,4 +539,5 @@ export {
updateAutomaticTimezone,
updateSelectedTimezone,
getCountryISO,
getPrivatePersonalDetails,
};
7 changes: 7 additions & 0 deletions src/pages/settings/Profile/PersonalDetails/AddressPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import CountryPicker from '../../../../components/CountryPicker';
import StatePicker from '../../../../components/StatePicker';
import Navigation from '../../../../libs/Navigation/Navigation';
import ROUTES from '../../../../ROUTES';
import usePrivatePersonalDetails from '../../../../hooks/usePrivatePersonalDetails';
import FullscreenLoadingIndicator from '../../../../components/FullscreenLoadingIndicator';

const propTypes = {
/* Onyx Props */
Expand Down Expand Up @@ -61,6 +63,7 @@ function updateAddress(values) {
}

function AddressPage(props) {
usePrivatePersonalDetails();
const {translate} = props;
const [countryISO, setCountryISO] = useState(PersonalDetails.getCountryISO(lodashGet(props.privatePersonalDetails, 'address.country')) || CONST.COUNTRY.US);
const isUSAForm = countryISO === CONST.COUNTRY.US;
Expand Down Expand Up @@ -117,6 +120,10 @@ function AddressPage(props) {
return errors;
}, []);

if (lodashGet(props.privatePersonalDetails, 'isLoading', true)) {
return <FullscreenLoadingIndicator />;
}

return (
<ScreenWrapper includeSafeAreaPaddingBottom={false}>
<HeaderWithBackButton
Expand Down
9 changes: 9 additions & 0 deletions src/pages/settings/Profile/PersonalDetails/DateOfBirthPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import moment from 'moment';
import PropTypes from 'prop-types';
import React, {useCallback} from 'react';
import {withOnyx} from 'react-native-onyx';
import lodashGet from 'lodash/get';
import CONST from '../../../../CONST';
import ONYXKEYS from '../../../../ONYXKEYS';
import ROUTES from '../../../../ROUTES';
Expand All @@ -15,6 +16,8 @@ import * as ValidationUtils from '../../../../libs/ValidationUtils';
import * as PersonalDetails from '../../../../libs/actions/PersonalDetails';
import compose from '../../../../libs/compose';
import styles from '../../../../styles/styles';
import usePrivatePersonalDetails from '../../../../hooks/usePrivatePersonalDetails';
import FullscreenLoadingIndicator from '../../../../components/FullscreenLoadingIndicator';

const propTypes = {
/* Onyx Props */
Expand All @@ -34,6 +37,8 @@ const defaultProps = {
};

function DateOfBirthPage({translate, privatePersonalDetails}) {
usePrivatePersonalDetails();

/**
* @param {Object} values
* @param {String} values.dob - date of birth
Expand All @@ -55,6 +60,10 @@ function DateOfBirthPage({translate, privatePersonalDetails}) {
return errors;
}, []);

if (lodashGet(privatePersonalDetails, 'isLoading', true)) {
return <FullscreenLoadingIndicator />;
}

return (
<ScreenWrapper includeSafeAreaPaddingBottom={false}>
<HeaderWithBackButton
Expand Down
12 changes: 8 additions & 4 deletions src/pages/settings/Profile/PersonalDetails/LegalNamePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import * as PersonalDetails from '../../../../libs/actions/PersonalDetails';
import compose from '../../../../libs/compose';
import Navigation from '../../../../libs/Navigation/Navigation';
import ROUTES from '../../../../ROUTES';
import usePrivatePersonalDetails from '../../../../hooks/usePrivatePersonalDetails';
import FullscreenLoadingIndicator from '../../../../components/FullscreenLoadingIndicator';

const propTypes = {
/* Onyx Props */
Expand All @@ -42,6 +44,7 @@ const updateLegalName = (values) => {
};

function LegalNamePage(props) {
usePrivatePersonalDetails();
const legalFirstName = lodashGet(props.privatePersonalDetails, 'legalFirstName', '');
const legalLastName = lodashGet(props.privatePersonalDetails, 'legalLastName', '');

Expand All @@ -63,11 +66,12 @@ function LegalNamePage(props) {
return errors;
}, []);

if (lodashGet(props.privatePersonalDetails, 'isLoading', true)) {
return <FullscreenLoadingIndicator />;
}

return (
<ScreenWrapper
includeSafeAreaPaddingBottom={false}
shouldEnableMaxHeight
>
<ScreenWrapper includeSafeAreaPaddingBottom={false}>
tienifr marked this conversation as resolved.
Show resolved Hide resolved
<HeaderWithBackButton
title={props.translate('privatePersonalDetails.legalName')}
onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_PERSONAL_DETAILS)}
Expand Down
18 changes: 9 additions & 9 deletions src/pages/settings/Profile/PersonalDetails/PersonalDetailsInitialPage.js
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

@tienifr Can we somehow wrap the following repeating logic into a component of its own?

usePrivatePersonalDetails();
    if (lodashGet(props.privatePersonalDetails, 'isLoading', true)) {
        return <FullscreenLoadingIndicator />;
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

A component named PersonalDetailsScreenWrapper can replace ScreenWrapper that we're using here.

Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import React, {useEffect} from 'react';
import React from 'react';
import PropTypes from 'prop-types';
import {ScrollView, View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import lodashGet from 'lodash/get';
import ScreenWrapper from '../../../../components/ScreenWrapper';
import HeaderWithBackButton from '../../../../components/HeaderWithBackButton';
import withLocalize, {withLocalizePropTypes} from '../../../../components/withLocalize';
Expand All @@ -11,9 +12,10 @@ import styles from '../../../../styles/styles';
import Navigation from '../../../../libs/Navigation/Navigation';
import compose from '../../../../libs/compose';
import MenuItemWithTopDescription from '../../../../components/MenuItemWithTopDescription';
import * as PersonalDetails from '../../../../libs/actions/PersonalDetails';
import ONYXKEYS from '../../../../ONYXKEYS';
import {withNetwork} from '../../../../components/OnyxProvider';
import usePrivatePersonalDetails from '../../../../hooks/usePrivatePersonalDetails';
import FullscreenLoadingIndicator from '../../../../components/FullscreenLoadingIndicator';

const propTypes = {
/* Onyx Props */
Expand Down Expand Up @@ -54,13 +56,7 @@ const defaultProps = {
};

function PersonalDetailsInitialPage(props) {
useEffect(() => {
if (props.network.isOffline) {
return;
}
PersonalDetails.openPersonalDetailsPage();
}, [props.network.isOffline]);

usePrivatePersonalDetails();
const privateDetails = props.privatePersonalDetails || {};
const address = privateDetails.address || {};
const legalName = `${privateDetails.legalFirstName || ''} ${privateDetails.legalLastName || ''}`.trim();
Expand All @@ -87,6 +83,10 @@ function PersonalDetailsInitialPage(props) {
return formattedAddress.trim().replace(/,$/, '');
};

if (lodashGet(props.privatePersonalDetails, 'isLoading', true)) {
return <FullscreenLoadingIndicator />;
}

return (
<ScreenWrapper>
<HeaderWithBackButton
Expand Down