-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix: no preview message after switching priorities #59252
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: no preview message after switching priorities #59252
Conversation
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
src/libs/OptionsListUtils.ts
Outdated
|
||
// If lastMessageTextFromReport is empty, try to extract the message from the lastReportAction instead | ||
if (!lastMessageTextFromReport) { | ||
const lastReportActionMessage = getReportActionMessageText(lastReportAction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After taking a look into method getReportPreviewMessage
, I don't think it's a good method because there're quite different. I know the below code will fix the effect but I don't think that's what we prefer.
Is lastReportAction
same as lastIOUMoneyReportAction
? If so, can we pass lastReportAction
to method getReportPreviewMessage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
lastReportAction
same aslastIOUMoneyReportAction
? If so, can we passlastReportAction
to methodgetReportPreviewMessage
Good point, I tried it and it works, but we still need to format the message, because it shows the mail address of the user instead of their full name. The code will look like this. What do you think?

const lastIOUMoneyReportAction = iouReport?.reportID
? allSortedReportActions[iouReport.reportID]?.find(
(reportAction, key): reportAction is ReportAction<typeof CONST.REPORT.ACTIONS.TYPE.IOU> =>
shouldReportActionBeVisible(reportAction, key, canUserPerformWriteAction(report)) &&
reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE &&
isMoneyRequestAction(reportAction),
)
: undefined;
const reportPreviewMessage = getReportPreviewMessage(
!isEmptyObject(iouReport) ? iouReport : null,
lastIOUMoneyReportAction ?? lastReportAction,
true,
reportUtilsIsChatReport(report),
null,
true,
lastReportAction,
);
lastMessageTextFromReport = formatReportLastMessageText(reportPreviewMessage);
// If lastIOUMoneyReportAction is empty, format the message with the actor's display name
if (!lastIOUMoneyReportAction) {
// Get the actor's display name instead of using email
const actorAccountID = lastReportAction?.actorAccountID;
const actorDetails = actorAccountID ? allPersonalDetails?.[actorAccountID] : null;
if (actorDetails) {
const actorDisplayName = getLastActorDisplayName(actorDetails);
const actorLastName = getLastActorLastName(actorDetails);
const actorName = actorLastName ? `${actorDisplayName} ${actorLastName}` : actorDisplayName;
// Format the message with the display name if available
if (actorDisplayName && actorDetails.login) {
lastMessageTextFromReport = lastMessageTextFromReport.replace(actorDetails.login, actorName);
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we still need to format the message, because it shows the mail address of the user instead of their full name.
@mustafapsd Can you look into method getReportPreviewMessage
please? I think we should understand what causes the gap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of this line in the getReportPreviewMessage
method, report
is undefined, so it returns the message directly without formatting.

If the report
is not null, the function finishes with this return:
return translateLocal('iou.payerOwesAmount', {payer: payerName ?? '', amount: formattedAmount, comment});
So maybe we can use the same translation key. What do you think?
Thank you for your detailed review @eh2077
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mustafapsd I don't agree the difference is just the end return. I saw a lot of IOU report use cases are handled differently in the method. So, I have concern about the solution your solution.
I think we dig a bit more to check if IOU report is available by some way.
Or maybe we can just use the msg return method getReportPreviewMessage
according to this comment
Lines 4033 to 4037 in 6bac38c
if (isEmptyObject(report) || !report?.reportID) { | |
// The iouReport is not found locally after SignIn because the OpenApp API won't return iouReports if they're settled | |
// As a temporary solution until we know how to solve this the best, we just use the message that returned from BE | |
return reportActionMessage; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, there is a logical mistake. If the report is an empty object, the case will be line 4019, because if it is undefined
or an empty object, it will not have a reportID
. So those statements are basically the same. Based on the comment, we shouldn't format the message. So, should I remove the formatting lines?
Lines 4025 to 4037 in 6bac38c
if (!report?.reportID) { | |
return reportActionMessage; | |
} | |
const allReportTransactions = getReportTransactions(report.reportID); | |
const transactionsWithReceipts = allReportTransactions.filter(hasReceiptTransactionUtils); | |
const numberOfScanningReceipts = transactionsWithReceipts.filter(isReceiptBeingScanned).length; | |
if (isEmptyObject(report) || !report?.reportID) { | |
// The iouReport is not found locally after SignIn because the OpenApp API won't return iouReports if they're settled | |
// As a temporary solution until we know how to solve this the best, we just use the message that returned from BE | |
return reportActionMessage; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mustafapsd It's not easy to follow your idea when you mentioned about source code. Just a suggestion, can you just use permanent link to a code snippet method to link source code? Like what I did, so, you don't need to copy the code in your comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean combining the two return cases? If yes, then I agree with you.
Lines 4025 to 4038 in 6bac38c
if (!report?.reportID) { | |
return reportActionMessage; | |
} | |
const allReportTransactions = getReportTransactions(report.reportID); | |
const transactionsWithReceipts = allReportTransactions.filter(hasReceiptTransactionUtils); | |
const numberOfScanningReceipts = transactionsWithReceipts.filter(isReceiptBeingScanned).length; | |
if (isEmptyObject(report) || !report?.reportID) { | |
// The iouReport is not found locally after SignIn because the OpenApp API won't return iouReports if they're settled | |
// As a temporary solution until we know how to solve this the best, we just use the message that returned from BE | |
return reportActionMessage; | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used a perma link, but it doesn't show up somehow. I'm sorry. Yes, these lines are what I mean. In line 4025, if the condition fits line 4033, it also fits the 4025. The program never reaches there, but it returns the same result. So, should we follow the comment? Thank you again @eh2077
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used a perma link, but it doesn't show up somehow.
It should work right? My input is like

In line 4025, if the condition fits line 4033, it also fits the 4025. The program never reaches there, but it returns the same result. So, should we follow the comment?
Yes, I think we can follow the comment. Would you mind improving these lines? Like replace
Lines 4025 to 4027 in 6bac38c
if (!report?.reportID) { | |
return reportActionMessage; | |
} |
with
Lines 4033 to 4037 in 6bac38c
if (isEmptyObject(report) || !report?.reportID) { | |
// The iouReport is not found locally after SignIn because the OpenApp API won't return iouReports if they're settled | |
// As a temporary solution until we know how to solve this the best, we just use the message that returned from BE | |
return reportActionMessage; | |
} |
src/libs/ReportUtils.ts
Outdated
// The iouReport is not found locally after SignIn because the OpenApp API won't return iouReports if they're settled | ||
// As a temporary solution until we know how to solve this the best, we just use the message that returned from BE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mustafapsd Can you improve the comments? Since, in this case, I think the iou report is available in local storage but hasn't been loaded into the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The iouReport is not found locally after SignIn because the OpenApp API won't return iouReports if they're settled | |
// As a temporary solution until we know how to solve this the best, we just use the message that returned from BE | |
// The IOU report associated with this action might not be loaded into the 'allReports' | |
// Alternatively, settled IOU reports might not be returned by the backend's OpenApp command. | |
// In either case, we lack the full report details needed for rich formatting, | |
// As a temporary solution until we know how to solve this the best, we just use the message that returned from BE |
Will this work?
@mustafapsd Please use the original PR template to include recordings for all platforms. You can take a look at other PRs. |
Updated! @eh2077 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-04-07.at.9.41.41.PM.movAndroid: mWeb ChromeScreen.Recording.2025-04-07.at.9.38.31.PM.moviOS: NativeiOS: mWeb SafariScreen.Recording.2025-04-07.at.9.37.52.PM.movMacOS: Chrome / SafariScreen.Recording.2025-04-07.at.9.32.12.PM.movMacOS: DesktopScreen.Recording.2025-04-07.at.9.52.33.PM.mov |
@mustafapsd Some of your recordings can't be previewed. Can you please upload them using the default template ### Screenshots/Videos
<details>
<summary>Android: Native</summary>
<!-- add screenshots or videos here -->
</details>
<details>
<summary>Android: mWeb Chrome</summary>
<!-- add screenshots or videos here -->
</details>
<details>
<summary>iOS: Native</summary>
<!-- add screenshots or videos here -->
</details>
<details>
<summary>iOS: mWeb Safari</summary>
<!-- add screenshots or videos here -->
</details>
<details>
<summary>MacOS: Chrome / Safari</summary>
<!-- add screenshots or videos here -->
</details>
<details>
<summary>MacOS: Desktop</summary>
<!-- add screenshots or videos here -->
</details> |
Could you check again? Thanks @eh2077 |
Cool! Thanks for your patience! Btw, my iOS App build is failing but I saw you tested it well, so I believe it works |
@justinpersaud This PR needs your help to manually trigger the workflow because this is the first PR from @mustafapsd , thank you |
approved them |
@mustafapsd none of your commits are signed. Can you please follow this and update your commits? You also have some linting errors. |
Signed-off-by: Mustafa Daglioglu <[email protected]>
…neyReportAction Ensure the report preview message is correctly formatted by using lastReportAction as a fallback when lastIOUMoneyReportAction is null. This prevents empty message texts and improves the display of actor details. Signed-off-by: Mustafa Daglioglu <[email protected]>
Signed-off-by: Mustafa Daglioglu <[email protected]>
Signed-off-by: Mustafa Daglioglu <[email protected]>
Signed-off-by: Mustafa Daglioglu <[email protected]>
Signed-off-by: Mustafa Daglioglu <[email protected]>
f350078
to
10c3dfb
Compare
The linting errors come from the files I didn't work on. I can update them too if you want. I signed the commits now @justinpersaud |
Ah ok thanks! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/justinpersaud in version: 9.1.24-2 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀
|
1 similar comment
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀
|
Explanation of Change
The current solution tries to get the full report, but we can simplify it by just formatting the message from the lastReportAction object directly if there is no report. I need to get
actorDetails
because there is a user email instead of their name in the last message. Here's how we can modify the code:This solution uses two helper functions:
getLastActorDisplayName
- Gets the first name or display name of the actor (Exists)getLastActorLastName
- Gets the last name of the actor (I added):Together, these functions ensure that when there's no formatted report preview message, we still get a properly formatted message with full names instead of email addresses.
Fixed Issues
$ #58879
PROPOSAL: $ #58879 (comment)
Tests
Offline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
2025-04-05_21-23-33.mp4
Android: mWeb Chrome
2025-04-05_21-23-33.mp4
iOS: Native
2025-04-05_18-06-26.mp4
iOS: mWeb Safari
2025-04-05_18-10-36.mp4
MacOS: Chrome / Safari
2025-04-04_13-11-10.mp4
MacOS: Desktop
2025-04-05_18-29-39.mp4