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 |
There was a problem hiding this comment.
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.
Is
lastReportActionsame aslastIOUMoneyReportAction? If so, can we passlastReportActionto 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.
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.
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.
@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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
Do you mean combining the two return cases? If yes, then I agree with you.
Lines 4025 to 4038 in 6bac38c
There was a problem hiding this comment.
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.
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
with
Lines 4033 to 4037 in 6bac38c
There was a problem hiding this comment.
@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.
| // 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 <mustafa.psd@yandex.com>
…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 <mustafa.psd@yandex.com>
Signed-off-by: Mustafa Daglioglu <mustafa.psd@yandex.com>
Signed-off-by: Mustafa Daglioglu <mustafa.psd@yandex.com>
Signed-off-by: Mustafa Daglioglu <mustafa.psd@yandex.com>
Signed-off-by: Mustafa Daglioglu <mustafa.psd@yandex.com>
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
actorDetailsbecause 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 Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.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