Skip to content

Fix/24389 - Request a money - Clicking on the avatar open a wrong profile #24624

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
pecanoro merged 10 commits into from
Aug 22, 2023
Merged

Fix/24389 - Request a money - Clicking on the avatar open a wrong profile #24624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d6c7de8
Display participants list when there are multiple avatars for IOU rep…
spcheema Aug 16, 2023
d162c91
Memoized dependency variable
spcheema Aug 17, 2023
b84c560
Buld participants list
spcheema Aug 17, 2023
085f972
Update comment
spcheema Aug 17, 2023
85ad12e
Display participants list on clicking avatars on IOU report header
spcheema Aug 18, 2023
988ae48
Remove report util changes
spcheema Aug 19, 2023
001b04a
Code lint
spcheema Aug 19, 2023
9c476bb
Show details/profile on clicking request money header avatars
spcheema Aug 22, 2023
9256f94
Merge branch 'main' into fix/24389-request-a-money-clicking-on-the-av…
spcheema Aug 22, 2023
5594626
Merge branch 'main' into fix/24389-request-a-money-clicking-on-the-av…
spcheema Aug 22, 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
52 changes: 38 additions & 14 deletions src/components/AvatarWithDisplayName.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ import * as OptionsListUtils from '../libs/OptionsListUtils';
import Text from './Text';
import * as StyleUtils from '../styles/StyleUtils';
import ParentNavigationSubtitle from './ParentNavigationSubtitle';
import PressableWithoutFeedback from './Pressable/PressableWithoutFeedback';
import Navigation from '../libs/Navigation/Navigation';
import ROUTES from '../ROUTES';

const propTypes = {
/** The report currently being looked at */
Expand Down Expand Up @@ -50,6 +53,20 @@ const defaultProps = {
size: CONST.AVATAR_SIZE.DEFAULT,
};

const showActorDetails = (report) => {
if (ReportUtils.isExpenseReport(report)) {
Navigation.navigate(ROUTES.getProfileRoute(report.ownerAccountID));
return;
}

if (!ReportUtils.isIOUReport(report) && report.participantAccountIDs.length === 1) {
Navigation.navigate(ROUTES.getProfileRoute(report.participantAccountIDs[0]));
return;
}

Navigation.navigate(ROUTES.getReportParticipantsRoute(report.reportID));
};

function AvatarWithDisplayName(props) {
const title = ReportUtils.getReportName(props.report);
const subtitle = ReportUtils.getChatRoomSubtitle(props.report);
Expand All @@ -61,24 +78,31 @@ function AvatarWithDisplayName(props) {
const shouldShowSubscriptAvatar = ReportUtils.shouldReportShowSubscript(props.report);
const isExpenseRequest = ReportUtils.isExpenseRequest(props.report);
const defaultSubscriptSize = isExpenseRequest ? CONST.AVATAR_SIZE.SMALL_NORMAL : props.size;

return (
<View style={[styles.appContentHeaderTitle, styles.flex1]}>
{Boolean(props.report && title) && (
<View style={[styles.flex1, styles.flexRow, styles.alignItemsCenter, styles.justifyContentBetween]}>
{shouldShowSubscriptAvatar ? (
<SubscriptAvatar
backgroundColor={themeColors.highlightBG}
mainAvatar={icons[0]}
secondaryAvatar={icons[1]}
size={defaultSubscriptSize}
/>
) : (
<MultipleAvatars
icons={icons}
size={props.size}
secondAvatarStyle={[StyleUtils.getBackgroundAndBorderStyle(themeColors.highlightBG)]}
/>
)}
<PressableWithoutFeedback
onPress={() => showActorDetails(props.report)}
accessibilityLabel={title}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
>
{shouldShowSubscriptAvatar ? (
<SubscriptAvatar
backgroundColor={themeColors.highlightBG}
mainAvatar={icons[0]}
secondaryAvatar={icons[1]}
size={defaultSubscriptSize}
/>
) : (
<MultipleAvatars
icons={icons}
size={props.size}
secondAvatarStyle={[StyleUtils.getBackgroundAndBorderStyle(themeColors.highlightBG)]}
/>
)}
</PressableWithoutFeedback>
<View style={[styles.flex1, styles.flexColumn, shouldShowSubscriptAvatar && !isExpenseRequest ? styles.ml4 : {}]}>
<DisplayNames
fullTitle={title}
Expand Down
9 changes: 8 additions & 1 deletion src/pages/ReportParticipantsPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,14 @@ const defaultProps = {
* @return {Array}
*/
const getAllParticipants = (report, personalDetails, translate) => {
const {participantAccountIDs} = report;
let participantAccountIDs = report.participantAccountIDs;

// Build participants list for IOU report - there is a possibility that participantAccountIDs may be undefined/empty
if (ReportUtils.isIOUReport(report)) {
const managerID = report.managerID || '';
const ownerAccountID = report.ownerAccountID || '';
participantAccountIDs = [managerID, ownerAccountID];
}

return _.chain(participantAccountIDs)
.map((accountID, index) => {
Expand Down
9 changes: 7 additions & 2 deletions src/pages/home/report/ReportActionItemSingle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ function ReportActionItemSingle(props) {

// If this is a report preview, display names and avatars of both people involved
let secondaryAvatar = {};
const displayAllActors = props.action.actionName === CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW && props.iouReport;
const displayAllActors = useMemo(() => props.action.actionName === CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW && props.iouReport, [props.action.actionName, props.iouReport]);

@dummy-1111 dummy-1111 Aug 23, 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.

There is no need to use useMemo here. I don't understand why it's used
Please tell me if I missed something

@narefyev91 narefyev91 Aug 23, 2023 •
edited
Loading

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.

We should use it because - it's used inside deps for useCallback - const showActorDetails = useCallback(() => {,
and any new render will create displayAllActors (without memo) which will re-creates useCallback - which should not be

@narefyev91 narefyev91 Aug 23, 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.

either we should not use useMemo and useCallback or should use it correctly with memoizing deps

@dummy-1111 dummy-1111 Aug 23, 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.

displayAllActors would change depends on props.action.actionName and props.iouReport regardless the use of useMemo. If those props don't change, displayAllActors won't be changed.

@narefyev91 narefyev91 Aug 23, 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.

yes - and if displayAllActors (if we use useMemo) will not changed - useCallback will also not be changed
Without useMemo - displayAllActors will be as usual variable - which will be re-created every time - and will cause re-creation of useCallback

@bernhardoj bernhardoj Aug 23, 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.

I agree with @s-alves10 (this one). Btw, displayAllActors will return props.iouReport object if it's true instead of a boolean. I think we should wrap it with a Boolean? (Boolean(props.iouReport))

@narefyev91 narefyev91 Aug 23, 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.

After some checks - i agree that simple passed variable will not trigger recreation of useCallback. Here can be 2 options either leave as it's now in case props.iouReport is object and useMemo needed here or refactor as @bernhardoj suggested to Boolean(props.iouReport) and remove useMemo. I think it's not critical

@pecanoro pecanoro Aug 23, 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.

If nothing is going to break, I think we can leave it as is and maybe we can clean up in the future. It's just semantics, I also missed it, I thought it was necessary, but it is also true you guys are probably more knowledgable about React that me 😄

@spcheema spcheema Aug 24, 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.

@pecanoro @narefyev91 Is there any action we need to take now? Or we have decided to refactor later.

@narefyev91 narefyev91 Aug 24, 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.

We good to jump on it later

const primaryDisplayName = displayName;
if (displayAllActors) {
const secondaryUserDetails = props.personalDetailsList[props.iouReport.ownerAccountID] || {};
Expand Down Expand Up @@ -144,9 +144,14 @@ function ReportActionItemSingle(props) {
if (isWorkspaceActor) {
showWorkspaceDetails(props.report.reportID);
} else {
// Show participants page IOU report preview
if (displayAllActors) {
Navigation.navigate(ROUTES.getReportParticipantsRoute(props.iouReport.reportID));
return;
}
showUserDetails(props.action.delegateAccountID ? props.action.delegateAccountID : actorAccountID);
}
}, [isWorkspaceActor, props.report.reportID, actorAccountID, props.action.delegateAccountID]);
}, [isWorkspaceActor, props.report.reportID, actorAccountID, props.action.delegateAccountID, props.iouReport.reportID, displayAllActors]);
Comment thread
spcheema marked this conversation as resolved.
Outdated

const shouldDisableDetailPage = useMemo(
() => !isWorkspaceActor && ReportUtils.isOptimisticPersonalDetail(props.action.delegateAccountID ? props.action.delegateAccountID : actorAccountID),
Expand Down