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.

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: remove getDisplayNameForTypingIndicator #31495

Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
a57d229
fix: remove getDisplayNameForTypingIndicator
koko57 Nov 17, 2023
9093ff3
Merge branch 'main' into refactor/31312-consolidate-getdisplayname-me…
koko57 Nov 20, 2023
6358b1e
Merge branch 'main' into refactor/31312-consolidate-getdisplayname-me…
koko57 Nov 21, 2023
1ff3017
fix: apply requested changes
koko57 Nov 21, 2023
b30cb29
fix: run prettier
koko57 Nov 21, 2023
921a070
Merge branch 'main' into refactor/31312-consolidate-getdisplayname-me…
koko57 Nov 30, 2023
2ffcec2
fix: destructure props
koko57 Nov 30, 2023
5fec558
Merge branch 'main' into refactor/31312-consolidate-getdisplayname-me…
koko57 Dec 1, 2023
fe1e320
fix: apply requested changes
koko57 Dec 1, 2023
e7d667c
fix: run prettier
koko57 Dec 1, 2023
8af4811
fix: minor fix
koko57 Dec 1, 2023
9d26c00
Merge branch 'main' into refactor/31312-consolidate-getdisplayname-me…
koko57 Dec 6, 2023
c0a9c6a
fix: minor fix
koko57 Dec 6, 2023
e6ed623
fix: display name that should be hidden as someone
koko57 Dec 6, 2023
e186fd3
fix: run prettier
koko57 Dec 6, 2023
3ec5e40
Merge branch 'main' into refactor/31312-consolidate-getdisplayname-me…
koko57 Dec 8, 2023
88cf035
Merge branch 'main' into refactor/31312-consolidate-getdisplayname-me…
koko57 Dec 11, 2023
31df21e
Merge branch 'main' into refactor/31312-consolidate-getdisplayname-me…
koko57 Dec 12, 2023
8fb1f2f
fix: resolve conflicts
koko57 Dec 18, 2023
69ba3a0
fix: prettier
koko57 Dec 18, 2023
c1435f4
Merge branch 'main' into refactor/31312-consolidate-getdisplayname-me…
koko57 Dec 28, 2023
a4e422f
Merge branch 'main' into refactor/31312-consolidate-getdisplayname-me…
koko57 Dec 29, 2023
10f49fa
fix: do not check for login if is present in personalDetails
koko57 Dec 29, 2023
1bab5a9
fix: lint
koko57 Dec 29, 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
26 changes: 0 additions & 26 deletions src/libs/actions/PersonalDetails.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,31 +60,6 @@ function getDisplayName(login: string, personalDetail: Pick<PersonalDetails, 'fi
return fullName || userLogin;
}

/**
* @param [defaultDisplayName] display name to use if user details don't exist in Onyx or if
* found details don't include the user's displayName or login
*/
function getDisplayNameForTypingIndicator(userAccountIDOrLogin: string, defaultDisplayName = ''): string {
// Try to convert to a number, which means we have an accountID
const accountID = Number(userAccountIDOrLogin);

// If the user is typing on OldDot, userAccountIDOrLogin will be a string (the user's login),
// so Number(string) is NaN. Search for personalDetails by login to get the display name.
if (Number.isNaN(accountID)) {
const detailsByLogin = Object.entries(allPersonalDetails ?? {}).find(([, value]) => value?.login === userAccountIDOrLogin)?.[1];

// It's possible for displayName to be empty string, so we must use "||" to fallback to userAccountIDOrLogin.
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
return detailsByLogin?.displayName || userAccountIDOrLogin;
}

const detailsByAccountID = allPersonalDetails?.[accountID];

// It's possible for displayName to be empty string, so we must use "||" to fallback to login or defaultDisplayName.
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
return detailsByAccountID?.displayName || detailsByAccountID?.login || defaultDisplayName;
}

/**
* Gets the first and last name from the user's personal details.
* If the login is the same as the displayName, then they don't exist,
Expand Down Expand Up @@ -585,7 +560,6 @@ export {
extractFirstAndLastNameFromAvailableDetails,
getCountryISO,
getDisplayName,
getDisplayNameForTypingIndicator,
getPrivatePersonalDetails,
openPersonalDetailsPage,
openPublicProfilePage,
Expand Down
94 changes: 44 additions & 50 deletions src/pages/home/report/ReportTypingIndicator.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,80 +2,74 @@ import PropTypes from 'prop-types';
import React, {useMemo} from 'react';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import networkPropTypes from '@components/networkPropTypes';
import {withNetwork} from '@components/OnyxProvider';
import Text from '@components/Text';
import TextWithEllipsis from '@components/TextWithEllipsis';
import withLocalize, {withLocalizePropTypes} from '@components/withLocalize';
import compose from '@libs/compose';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import useThemeStyles from '@styles/useThemeStyles';
import * as PersonalDetails from '@userActions/PersonalDetails';
import ONYXKEYS from '@src/ONYXKEYS';

const propTypes = {
/** Key-value pairs of user accountIDs/logins and whether or not they are typing. Keys are accountIDs or logins. */
userTypingStatuses: PropTypes.objectOf(PropTypes.bool),

/** Information about the network */
network: networkPropTypes.isRequired,

...withLocalizePropTypes,
};

const defaultProps = {
userTypingStatuses: {},
};

function ReportTypingIndicator(props) {
function ReportTypingIndicator({userTypingStatuses}) {
const {translate} = useLocalize();
const {isOffline} = useNetwork();

const styles = useThemeStyles();
const usersTyping = useMemo(() => _.filter(_.keys(props.userTypingStatuses), (loginOrAccountID) => props.userTypingStatuses[loginOrAccountID]), [props.userTypingStatuses]);
const usersTyping = useMemo(() => _.filter(_.keys(userTypingStatuses), (loginOrAccountID) => userTypingStatuses[loginOrAccountID]), [userTypingStatuses]);
const firstUserTyping = usersTyping[0];

const firstUserTypingID = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move old comment to here

    // If the user is typing on OldDot, firstUserTyping will be a string (the user's login)

() => (firstUserTyping && Number.isNaN(Number(firstUserTyping)) ? PersonalDetailsUtils.getAccountIDsByLogins([firstUserTyping])[0] : firstUserTyping),
[firstUserTyping],
);

// If we are offline, the user typing statuses are not up-to-date so do not show them
if (props.network.isOffline) {
if (isOffline || !firstUserTyping) {
return null;
}

const numUsersTyping = _.size(usersTyping);

// Decide on the Text element that will hold the display based on the number of users that are typing.
switch (numUsersTyping) {
case 0:
return null;

case 1:
return (
<TextWithEllipsis
leadingText={PersonalDetails.getDisplayNameForTypingIndicator(usersTyping[0], props.translate('common.someone'))}
trailingText={` ${props.translate('reportTypingIndicator.isTyping')}`}
textStyle={[styles.chatItemComposeSecondaryRowSubText]}
wrapperStyle={[styles.chatItemComposeSecondaryRow, styles.flex1]}
leadingTextParentStyle={styles.chatItemComposeSecondaryRowOffset}
/>
);
// If the user is typing on OldDot, firstUserTyping will be a string (the user's login)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// If the user is typing on OldDot, firstUserTyping will be a string (the user's login)
// If the user is typing on OldDot, firstUserTyping will be a string (the user's displayName)

const firstUserTypingDisplayName = ReportUtils.getDisplayNameForParticipant(firstUserTypingID, false, false) || firstUserTyping;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this will work when users don't know each other.
There's case when Somone is typing... but with this change, it will never happen

Copy link
Contributor

@situchan situchan Dec 1, 2023

Choose a reason for hiding this comment

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

I have 2 solutions in mind:

  • getAccountIDsByLogins accepts 2nd parameter - shouldGenerateAccountID. If it's obvious that 1st param is not login but displayName, no need to generate random accountID but just return 0 or null. And then update this line like this:
    const firstUserTypingDisplayName = firstUserTypingID ? ReportUtils.getDisplayNameForParticipant(firstUserTypingID, false, false) : firstUserTyping;
  • check displayName if it comes from oldDot by doing email/phone validation

I prefer 1st one since user can still set any random email as display name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@situchan can even such a scenario occur? How could I reproduce it?

Copy link
Contributor

Choose a reason for hiding this comment

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

which one are you not able to reproduce?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's repro step:

  1. on A, create new chat with B
  2. A sends any message to B
  3. on B, type something
  4. Observe chat on A

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't set display name on both accounts and try again

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 the purpose of hidden logic is to secure sensitive info (login in our case)
So if display name not set, show "Hidden"
if display name set, show that display name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but for one of them it was set automatically

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you logged with google?
Btw you can unset display name in setting

Copy link
Contributor Author

@koko57 koko57 Dec 1, 2023

Choose a reason for hiding this comment

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

I unset it, still the same. I'll get back to it on Monday

Screen.Recording.2023-12-01.at.18.40.35.mp4


default:
return (
<Text
style={[styles.chatItemComposeSecondaryRowSubText, styles.chatItemComposeSecondaryRowOffset]}
numberOfLines={1}
>
{props.translate('reportTypingIndicator.multipleUsers')}
{` ${props.translate('reportTypingIndicator.areTyping')}`}
</Text>
);
if (usersTyping.length === 1) {
return (
<TextWithEllipsis
leadingText={firstUserTypingDisplayName || translate('common.someone')}
trailingText={` ${translate('reportTypingIndicator.isTyping')}`}
textStyle={[styles.chatItemComposeSecondaryRowSubText]}
wrapperStyle={[styles.chatItemComposeSecondaryRow, styles.flex1]}
leadingTextParentStyle={styles.chatItemComposeSecondaryRowOffset}
/>
);
}
return (
<Text
style={[styles.chatItemComposeSecondaryRowSubText, styles.chatItemComposeSecondaryRowOffset]}
numberOfLines={1}
>
{translate('reportTypingIndicator.multipleUsers')}
{` ${translate('reportTypingIndicator.areTyping')}`}
</Text>
);
}

ReportTypingIndicator.propTypes = propTypes;
ReportTypingIndicator.defaultProps = defaultProps;
ReportTypingIndicator.displayName = 'ReportTypingIndicator';

export default compose(
withLocalize,
withNetwork(),
withOnyx({
userTypingStatuses: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_USER_IS_TYPING}${reportID}`,
initialValue: {},
},
}),
)(ReportTypingIndicator);
export default withOnyx({
userTypingStatuses: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT_USER_IS_TYPING}${reportID}`,
initialValue: {},
},
})(ReportTypingIndicator);
Loading