Skip to content

Fix crash while changing contact method #19941

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

Merged
NikkiWines merged 2 commits into from
Jun 2, 2023
Merged

Fix crash while changing contact method #19941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d8e214
use current login for the condition
hungvu193 Jun 1, 2023
b8a231e
update shouldShowReportRecipientLocalTime params for other places
hungvu193 Jun 1, 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
5 changes: 3 additions & 2 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,10 +637,11 @@ function hasExpensifyEmails(emails) {
* Whether the time row should be shown for a report.
* @param {Array<Object>} personalDetails
* @param {Object} report
* @param {String} login
* @return {Boolean}
*/
function canShowReportRecipientLocalTime(personalDetails, report) {
const reportParticipants = _.without(lodashGet(report, 'participants', []), sessionEmail);
function canShowReportRecipientLocalTime(personalDetails, report, login) {
const reportParticipants = _.without(lodashGet(report, 'participants', []), login);
const participantsWithoutExpensifyEmails = _.difference(reportParticipants, CONST.EXPENSIFY_EMAILS);
const hasMultipleParticipants = participantsWithoutExpensifyEmails.length > 1;
const reportRecipient = personalDetails[participantsWithoutExpensifyEmails[0]];
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/report/ParticipantLocalTime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class ParticipantLocalTime extends PureComponent {
}

render() {
const reportRecipientDisplayName = this.props.participant.firstName || this.props.participant.displayName;
const reportRecipientDisplayName = lodashGet(this.props, 'participant.firstName') || lodashGet(this.props, 'participant.displayName');

@parasharrajat parasharrajat Jun 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should not be using the lodashGet here. participant prop is required here. So should never be rendering this component if participant is not present. In case, participant is undefined here, it mean the root cause is in upper tree not here.

cc: @NikkiWines

@allroundexperts allroundexperts Jun 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is just a safe guard. We've fixed the root cause as well.

@parasharrajat parasharrajat Jun 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This safe guard promotes more bugs. No one should be able to use this component without passing the participation and error should be thrown to make it clear during development. This should be reverted.


return (
<View style={[styles.chatItemComposeSecondaryRow]}>
Expand Down
3 changes: 2 additions & 1 deletion src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,8 @@ class ReportActionCompose extends React.Component {
const reportParticipants = _.without(lodashGet(this.props.report, 'participants', []), this.props.currentUserPersonalDetails.login);
const participantsWithoutExpensifyEmails = _.difference(reportParticipants, CONST.EXPENSIFY_EMAILS);
const reportRecipient = this.props.personalDetails[participantsWithoutExpensifyEmails[0]];
const shouldShowReportRecipientLocalTime = ReportUtils.canShowReportRecipientLocalTime(this.props.personalDetails, this.props.report) && !this.props.isComposerFullSize;
const shouldShowReportRecipientLocalTime =
ReportUtils.canShowReportRecipientLocalTime(this.props.personalDetails, this.props.report, this.props.currentUserPersonalDetails.login) && !this.props.isComposerFullSize;

@allroundexperts allroundexperts Jun 1, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@hungvu193 Please verify if canShowReportRecipientLocalTime function isn't used elsewhere. If so, then add this third parameter at other places as well!

@hungvu193 hungvu193 Jun 1, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, I should notice this earlier.
Thank you @allroundexperts . I've just updated it again!


// Prevents focusing and showing the keyboard while the drawer is covering the chat.
const isComposeDisabled = this.props.isDrawerOpen && this.props.isSmallScreenWidth;
Expand Down
7 changes: 5 additions & 2 deletions src/pages/home/report/ReportActionsList.js
View file
Open in desktop
Comment thread
hungvu193 marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import * as ReportScrollManager from '../../../libs/ReportScrollManager';
import styles from '../../../styles/styles';
import * as ReportUtils from '../../../libs/ReportUtils';
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions';
import withCurrentUserPersonalDetails, {withCurrentUserPersonalDetailsPropTypes, withCurrentUserPersonalDetailsDefaultProps} from '../../../components/withCurrentUserPersonalDetails';
import {withNetwork, withPersonalDetails} from '../../../components/OnyxProvider';
import ReportActionItem from './ReportActionItem';
import ReportActionItemParentAction from './ReportActionItemParentAction';
Expand Down Expand Up @@ -55,13 +56,15 @@ const propTypes = {

...withDrawerPropTypes,
...windowDimensionsPropTypes,
...withCurrentUserPersonalDetailsPropTypes,
};

const defaultProps = {
newMarkerReportActionID: '',
personalDetails: {},
mostRecentIOUReportActionID: '',
isLoadingMoreReportActions: false,
...withCurrentUserPersonalDetailsDefaultProps,
};

/**
Expand Down Expand Up @@ -146,7 +149,7 @@ const ReportActionsList = (props) => {
// Native mobile does not render updates flatlist the changes even though component did update called.
// To notify there something changes we can use extraData prop to flatlist
const extraData = [!props.isDrawerOpen && props.isSmallScreenWidth ? props.newMarkerReportActionID : undefined, ReportUtils.isArchivedRoom(props.report)];
const shouldShowReportRecipientLocalTime = ReportUtils.canShowReportRecipientLocalTime(props.personalDetails, props.report);
const shouldShowReportRecipientLocalTime = ReportUtils.canShowReportRecipientLocalTime(props.personalDetails, props.report, props.currentUserPersonalDetails.login);
return (
<Animated.View style={[animatedStyles, styles.flex1]}>
<InvertedFlatList
Expand Down Expand Up @@ -196,4 +199,4 @@ ReportActionsList.propTypes = propTypes;
ReportActionsList.defaultProps = defaultProps;
ReportActionsList.displayName = 'ReportActionsList';

export default compose(withDrawerState, withWindowDimensions, withLocalize, withPersonalDetails(), withNetwork())(ReportActionsList);
export default compose(withDrawerState, withWindowDimensions, withLocalize, withPersonalDetails(), withNetwork(), withCurrentUserPersonalDetails)(ReportActionsList);