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

Remove unnecessary calls to OpenPrivatePersonalDetailsPage #37047

Merged
merged 25 commits into from
Apr 9, 2024
Merged

Remove unnecessary calls to OpenPrivatePersonalDetailsPage #37047

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
627bdf2
Remove unnecssary calls to OpenPrivatePersonalDetailsPage
grgia Feb 21, 2024
e5b5536
remaining uses
grgia Feb 21, 2024
ed012d5
prettier
grgia Feb 21, 2024
8410d34
fix LegalNamePage loading
grgia Feb 22, 2024
e33dc85
fix direct link loading for AddressPage
grgia Feb 22, 2024
3d31a5e
Loading pages
grgia Feb 22, 2024
26d1f57
prettier
grgia Feb 22, 2024
5b33277
remove unused function
grgia Feb 22, 2024
d968e0b
add default props to profilepage
grgia Feb 22, 2024
bbf8366
Merge branch 'main' into georgia-private-personal-details
grgia Feb 22, 2024
dd90ad3
use hook
grgia Feb 28, 2024
b42b0b8
Merge branch 'main' into georgia-private-personal-details
grgia Feb 28, 2024
8130664
Merge branch 'main' into georgia-private-personal-details
grgia Mar 4, 2024
96a20fa
remove weird merge file creation
grgia Mar 4, 2024
cc9b06a
Clean up
grgia Mar 4, 2024
c79637c
leave TS type
grgia Mar 4, 2024
67595c7
Merge branch 'main' into georgia-private-personal-details
grgia Mar 5, 2024
1bd4b0a
remove hook and calls to to get private personal details
grgia Mar 5, 2024
d6f6d2f
lint
grgia Mar 7, 2024
970229a
fix lint
grgia Mar 11, 2024
b3bc589
TS error
grgia Mar 11, 2024
b9b2a43
Merge branch 'main' into georgia-private-personal-details
grgia Mar 22, 2024
f8f785e
Merge branch 'main' into georgia-private-personal-details
grgia Mar 28, 2024
f09f94b
Merge branch 'main' into georgia-private-personal-details
grgia Apr 5, 2024
bfd8fb9
remove openPersonalDetails, update test to call openPublicProfilePage…
grgia Apr 5, 2024
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
2 changes: 0 additions & 2 deletions src/components/CardPreview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {View} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
import ExpensifyCardImage from '@assets/images/expensify-card.svg';
import usePrivatePersonalDetails from '@hooks/usePrivatePersonalDetails';
import useThemeStyles from '@hooks/useThemeStyles';
import variables from '@styles/variables';
import ONYXKEYS from '@src/ONYXKEYS';
Expand All @@ -22,7 +21,6 @@ type CardPreviewProps = CardPreviewOnyxProps;

function CardPreview({privatePersonalDetails, session}: CardPreviewProps) {
const styles = useThemeStyles();
usePrivatePersonalDetails();
const {legalFirstName, legalLastName} = privatePersonalDetails ?? {};
const cardHolder = legalFirstName && legalLastName ? `${legalFirstName} ${legalLastName}` : session?.email ?? '';

Expand Down
20 changes: 0 additions & 20 deletions src/hooks/usePrivatePersonalDetails.ts
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s77rt Im wondering about leaving this hook/function instead of deleting it for later if we need to load confidential data for some reason. What do you think? Or should we just add this back when we later need it

Copy link
Contributor

@s77rt s77rt Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We should just add it back when needed. It's always best to avoid unnecessary code hanging in the code base. FWIW I don't think we would ever need this hook again since we will always have the private data on OpenApp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although, I realize that currently logged in users wont have the data unless they log out and back in / reconnect

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right! This is a breaking change for logged users. But it's a temporarily issue. Can we maybe send pusher events to all users to get their private data?

Copy link
Contributor

Choose a reason for hiding this comment

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

@grgia Was there any discussion regarding this issue? or whether it's a blocker?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it makes sense to keep the call for one page only, it may be better to do nothing and close the PR. However I'm inclined to remove the redundancy. I just realized something, how can a user upgrade the app and not call OpenApp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a good point. @marcaaron mind if I ask for your expertise here?

Essentially, we've added the privatePersonalDetails to openApp, so there's no need to send them on these pages. However, we're concerned that this is a breaking change for users until openApp is called when they log in / out. Is this something I need to worry about?

Copy link
Contributor

Choose a reason for hiding this comment

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

@grgia can you help me understand the consequences of showing the stale data to the affected users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcaaron we didn't use to store these private personal details in ONYX, now we do via Open App. So any users who havent logged in yet or called OpenApp in awhile wouldnt have the data in ONYX to access.

So the question is, do we keep a call to the API here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I don't feel too passionate. It feels like we can remove it? Unless something major will break I would treat it as a minor bug affecting older app versions. We are going to force upgrade users to use a new version of the app eventually which should trigger an OpenApp call (at least in the case of native apps).

This file was deleted.

16 changes: 1 addition & 15 deletions src/libs/actions/PersonalDetails.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type {DateOfBirthForm} from '@src/types/form';
import type {PersonalDetails, PersonalDetailsList, PrivatePersonalDetails} from '@src/types/onyx';
import type {PersonalDetails, PersonalDetailsList} from '@src/types/onyx';
import type {SelectedTimezone, Timezone} from '@src/types/onyx/PersonalDetails';
import * as Session from './Session';

Expand All @@ -43,12 +43,6 @@ Onyx.connect({
callback: (val) => (allPersonalDetails = val),
});

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

function updatePronouns(pronouns: string) {
if (currentUserAccountID) {
const parameters: UpdatePronounsParams = {pronouns};
Expand Down Expand Up @@ -446,17 +440,9 @@ function clearAvatarErrors() {
});
}

/**
* Get private personal details value
*/
function getPrivatePersonalDetails(): OnyxEntry<PrivatePersonalDetails> {
return privatePersonalDetails;
}

export {
clearAvatarErrors,
deleteAvatar,
getPrivatePersonalDetails,
openPersonalDetails,
openPublicProfilePage,
updateAddress,
Expand Down
12 changes: 7 additions & 5 deletions src/pages/settings/Profile/PersonalDetails/AddressPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import FullscreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import ScreenWrapper from '@components/ScreenWrapper';
import useLocalize from '@hooks/useLocalize';
import usePrivatePersonalDetails from '@hooks/usePrivatePersonalDetails';
import useThemeStyles from '@hooks/useThemeStyles';
import Navigation from '@libs/Navigation/Navigation';
import type {SettingsNavigatorParamList} from '@libs/Navigation/types';
Expand All @@ -22,6 +21,8 @@ import type {Address} from '@src/types/onyx/PrivatePersonalDetails';
type AddressPageOnyxProps = {
/** User's private personal details */
privatePersonalDetails: OnyxEntry<PrivatePersonalDetails>;
/** Whether app is loading */
isLoadingApp: OnyxEntry<boolean>;
};

type AddressPageProps = StackScreenProps<SettingsNavigatorParamList, typeof SCREENS.SETTINGS.PROFILE.ADDRESS> & AddressPageOnyxProps;
Expand All @@ -41,14 +42,12 @@ function updateAddress(values: FormOnyxValues<typeof ONYXKEYS.FORMS.HOME_ADDRESS
);
}

function AddressPage({privatePersonalDetails, route}: AddressPageProps) {
function AddressPage({privatePersonalDetails, route, isLoadingApp = true}: AddressPageProps) {
const styles = useThemeStyles();
usePrivatePersonalDetails();
const {translate} = useLocalize();
const address = useMemo(() => privatePersonalDetails?.address, [privatePersonalDetails]);
const countryFromUrl = route.params?.country;
const [currentCountry, setCurrentCountry] = useState(address?.country);
const isLoadingPersonalDetails = privatePersonalDetails?.isLoading ?? true;
const [street1, street2] = (address?.street ?? '').split('\n');
const [state, setState] = useState(address?.state);
const [city, setCity] = useState(address?.city);
Expand Down Expand Up @@ -109,7 +108,7 @@ function AddressPage({privatePersonalDetails, route}: AddressPageProps) {
shouldShowBackButton
onBackButtonPress={() => Navigation.goBack()}
/>
{isLoadingPersonalDetails ? (
{isLoadingApp ? (
<FullscreenLoadingIndicator style={[styles.flex1, styles.pRelative]} />
) : (
<AddressForm
Expand All @@ -135,4 +134,7 @@ export default withOnyx<AddressPageProps, AddressPageOnyxProps>({
privatePersonalDetails: {
key: ONYXKEYS.PRIVATE_PERSONAL_DETAILS,
},
isLoadingApp: {
key: ONYXKEYS.IS_LOADING_APP,
},
})(AddressPage);
12 changes: 7 additions & 5 deletions src/pages/settings/Profile/PersonalDetails/DateOfBirthPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import FullscreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import ScreenWrapper from '@components/ScreenWrapper';
import useLocalize from '@hooks/useLocalize';
import usePrivatePersonalDetails from '@hooks/usePrivatePersonalDetails';
import useThemeStyles from '@hooks/useThemeStyles';
import Navigation from '@libs/Navigation/Navigation';
import * as ValidationUtils from '@libs/ValidationUtils';
Expand All @@ -23,14 +22,14 @@ import type {PrivatePersonalDetails} from '@src/types/onyx';
type DateOfBirthPageOnyxProps = {
/** User's private personal details */
privatePersonalDetails: OnyxEntry<PrivatePersonalDetails>;
/** Whether app is loading */
isLoadingApp: OnyxEntry<boolean>;
};
type DateOfBirthPageProps = DateOfBirthPageOnyxProps;

function DateOfBirthPage({privatePersonalDetails}: DateOfBirthPageProps) {
function DateOfBirthPage({privatePersonalDetails, isLoadingApp = true}: DateOfBirthPageProps) {
const {translate} = useLocalize();
const styles = useThemeStyles();
usePrivatePersonalDetails();
const isLoadingPersonalDetails = privatePersonalDetails?.isLoading ?? true;

/**
* @returns An object containing the errors for each inputID
Expand Down Expand Up @@ -59,7 +58,7 @@ function DateOfBirthPage({privatePersonalDetails}: DateOfBirthPageProps) {
title={translate('common.dob')}
onBackButtonPress={() => Navigation.goBack()}
/>
{isLoadingPersonalDetails ? (
{isLoadingApp ? (
<FullscreenLoadingIndicator style={[styles.flex1, styles.pRelative]} />
) : (
<FormProvider
Expand Down Expand Up @@ -90,4 +89,7 @@ export default withOnyx<DateOfBirthPageProps, DateOfBirthPageOnyxProps>({
privatePersonalDetails: {
key: ONYXKEYS.PRIVATE_PERSONAL_DETAILS,
},
isLoadingApp: {
key: ONYXKEYS.IS_LOADING_APP,
},
})(DateOfBirthPage);
12 changes: 7 additions & 5 deletions src/pages/settings/Profile/PersonalDetails/LegalNamePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import HeaderWithBackButton from '@components/HeaderWithBackButton';
import ScreenWrapper from '@components/ScreenWrapper';
import TextInput from '@components/TextInput';
import useLocalize from '@hooks/useLocalize';
import usePrivatePersonalDetails from '@hooks/usePrivatePersonalDetails';
import useThemeStyles from '@hooks/useThemeStyles';
import * as ErrorUtils from '@libs/ErrorUtils';
import Navigation from '@libs/Navigation/Navigation';
Expand All @@ -25,6 +24,8 @@ import type {Errors} from '@src/types/onyx/OnyxCommon';
type LegalNamePageOnyxProps = {
/** User's private personal details */
privatePersonalDetails: OnyxEntry<PrivatePersonalDetails>;
/** Whether app is loading */
isLoadingApp: OnyxEntry<boolean>;
};

type LegalNamePageProps = LegalNamePageOnyxProps;
Expand All @@ -33,13 +34,11 @@ const updateLegalName = (values: PrivatePersonalDetails) => {
PersonalDetails.updateLegalName(values.legalFirstName?.trim() ?? '', values.legalLastName?.trim() ?? '');
};

function LegalNamePage({privatePersonalDetails}: LegalNamePageProps) {
function LegalNamePage({privatePersonalDetails, isLoadingApp = true}: LegalNamePageProps) {
const styles = useThemeStyles();
const {translate} = useLocalize();
usePrivatePersonalDetails();
const legalFirstName = privatePersonalDetails?.legalFirstName ?? '';
const legalLastName = privatePersonalDetails?.legalLastName ?? '';
const isLoadingPersonalDetails = privatePersonalDetails?.isLoading ?? true;

const validate = useCallback((values: FormOnyxValues<typeof ONYXKEYS.FORMS.LEGAL_NAME_FORM>) => {
const errors: Errors = {};
Expand Down Expand Up @@ -86,7 +85,7 @@ function LegalNamePage({privatePersonalDetails}: LegalNamePageProps) {
title={translate('privatePersonalDetails.legalName')}
onBackButtonPress={() => Navigation.goBack()}
/>
{isLoadingPersonalDetails ? (
{isLoadingApp ? (
<FullscreenLoadingIndicator style={[styles.flex1, styles.pRelative]} />
) : (
<FormProvider
Expand Down Expand Up @@ -133,4 +132,7 @@ export default withOnyx<LegalNamePageProps, LegalNamePageOnyxProps>({
privatePersonalDetails: {
key: ONYXKEYS.PRIVATE_PERSONAL_DETAILS,
},
isLoadingApp: {
key: ONYXKEYS.IS_LOADING_APP,
},
})(LegalNamePage);
11 changes: 7 additions & 4 deletions src/pages/settings/Profile/ProfilePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import Section from '@components/Section';
import withCurrentUserPersonalDetails, {withCurrentUserPersonalDetailsDefaultProps, withCurrentUserPersonalDetailsPropTypes} from '@components/withCurrentUserPersonalDetails';
import withLocalize, {withLocalizePropTypes} from '@components/withLocalize';
import withWindowDimensions, {windowDimensionsPropTypes} from '@components/withWindowDimensions';
import usePrivatePersonalDetails from '@hooks/usePrivatePersonalDetails';
import useStyleUtils from '@hooks/useStyleUtils';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
Expand Down Expand Up @@ -59,6 +58,8 @@ const propTypes = {
}),
}),

isLoadingApp: PropTypes.bool,

...withLocalizePropTypes,
...windowDimensionsPropTypes,
...withCurrentUserPersonalDetailsPropTypes,
Expand All @@ -80,6 +81,7 @@ const defaultProps = {
country: '',
},
},
isLoadingApp: true,
};

function ProfilePage(props) {
Expand All @@ -101,10 +103,8 @@ function ProfilePage(props) {
const contactMethodBrickRoadIndicator = UserUtils.getLoginListBrickRoadIndicator(props.loginList);
const emojiCode = lodashGet(props, 'currentUserPersonalDetails.status.emojiCode', '');
const {isSmallScreenWidth} = useWindowDimensions();
usePrivatePersonalDetails();
const privateDetails = props.privatePersonalDetails || {};
const legalName = `${privateDetails.legalFirstName || ''} ${privateDetails.legalLastName || ''}`.trim();
const isLoadingPersonalDetails = lodashGet(props.privatePersonalDetails, 'isLoading', true);

const publicOptions = [
{
Expand Down Expand Up @@ -200,7 +200,7 @@ function ProfilePage(props) {
childrenStyles={styles.pt3}
titleStyles={styles.accountSettingsSectionTitle}
>
{isLoadingPersonalDetails ? (
{props.isLoadingApp ? (
<FullscreenLoadingIndicator style={[styles.flex1, styles.pRelative, StyleUtils.getBackgroundColorStyle(theme.cardBG)]} />
) : (
<>
Expand Down Expand Up @@ -242,5 +242,8 @@ export default compose(
user: {
key: ONYXKEYS.USER,
},
isLoadingApp: {
key: ONYXKEYS.IS_LOADING_APP,
},
}),
)(ProfilePage);
2 changes: 0 additions & 2 deletions src/pages/settings/Wallet/ReportCardLostPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import SingleOptionSelector from '@components/SingleOptionSelector';
import Text from '@components/Text';
import useLocalize from '@hooks/useLocalize';
import usePrevious from '@hooks/usePrevious';
import usePrivatePersonalDetails from '@hooks/usePrivatePersonalDetails';
import useThemeStyles from '@hooks/useThemeStyles';
import * as CardUtils from '@libs/CardUtils';
import Navigation from '@libs/Navigation/Navigation';
Expand Down Expand Up @@ -82,7 +81,6 @@ function ReportCardLostPage({
formData,
}: ReportCardLostPageProps) {
const styles = useThemeStyles();
usePrivatePersonalDetails();

const domainCards = CardUtils.getDomainCards(cardList ?? {})[domain];
const physicalCard = CardUtils.findPhysicalCard(domainCards);
Expand Down
2 changes: 0 additions & 2 deletions src/pages/settings/Wallet/WalletPage/CardDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import MenuItemWithTopDescription from '@components/MenuItemWithTopDescription';
import PressableWithDelayToggle from '@components/Pressable/PressableWithDelayToggle';
import TextLink from '@components/TextLink';
import useLocalize from '@hooks/useLocalize';
import usePrivatePersonalDetails from '@hooks/usePrivatePersonalDetails';
import useThemeStyles from '@hooks/useThemeStyles';
import Clipboard from '@libs/Clipboard';
import Navigation from '@libs/Navigation/Navigation';
Expand Down Expand Up @@ -47,7 +46,6 @@ type CardDetailsProps = CardDetailsOnyxProps & {

function CardDetails({pan = '', expiration = '', cvv = '', privatePersonalDetails, domain}: CardDetailsProps) {
const styles = useThemeStyles();
usePrivatePersonalDetails();
const {translate} = useLocalize();

const handleCopyToClipboard = () => {
Expand Down
Loading