-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Track expense lhn #41668
Track expense lhn #41668
Conversation
@c3024 We need to address this on the backend too. It works fine in offline mode, but when the open report API is called, the last message text reverts to 'What would you like to do with this expense? We still have one unhandled case regarding expense deletion. Let's first test this solution, and if everything works fine, we'll decide whether we need to fix that here too. |
I see it working fine for all but I found the last message showing as I see we only use the App/src/libs/OptionsListUtils.ts Line 625 in 23297de
and this here App/src/libs/ReportActionsUtils.ts Line 304 in 2d621b7
is filtering track actions as well and removing the track action in the sorted report actions to display here App/src/libs/OptionsListUtils.ts Line 274 in 23297de
and only CREATED action is left in the actions and the fallback is taken as last message. Since the backend still sends the whispers also as report actions I think we need not change the lastMessage sent from backend. We should fix the above to include the track action in the actions to display.
|
Please check this. I don't think it needs a backend fix. |
@c3024 I'm on it. I'll complete it today. |
src/libs/ReportActionsUtils.ts
Outdated
@@ -320,7 +320,7 @@ function getCombinedReportActions(reportActions: ReportAction[], transactionThre | |||
// Filter out request and send money request actions because we don't want to show any preview actions for one transaction reports | |||
const filteredReportActions = [...reportActions, ...filteredTransactionThreadReportActions].filter((action) => { | |||
const actionType = (action as OriginalMessageIOU).originalMessage?.type ?? ''; | |||
return actionType !== CONST.IOU.REPORT_ACTION_TYPE.CREATE && actionType !== CONST.IOU.REPORT_ACTION_TYPE.TRACK && !isSentMoneyReportAction(action); | |||
return actionType !== CONST.IOU.REPORT_ACTION_TYPE.CREATE && !isSentMoneyReportAction(action); |
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.
We should not remove it directly.
Removing it breaks shows the report action in transaction report.
reportActionInWorkspaceChat.mp4
We should not filter track actions only in self DMs as far as I see. The following snippet fixes our bug and appears to cause no regressions.
return actionType !== CONST.IOU.REPORT_ACTION_TYPE.CREATE && !isSentMoneyReportAction(action); | |
const report = allReports?.[action?.reportID ?? '']; | |
const isSelfDM = report?.chatType === CONST.REPORT.CHAT_TYPE.SELF_DM; | |
if (isSelfDM) { | |
return actionType !== CONST.IOU.REPORT_ACTION_TYPE.CREATE && !isSentMoneyReportAction(action); | |
} | |
return actionType !== CONST.IOU.REPORT_ACTION_TYPE.CREATE && actionType !== CONST.IOU.REPORT_ACTION_TYPE.TRACK && !isSentMoneyReportAction(action); |
please fix the lint and ts failures. |
@c3024 Hey, could you help me solve this TypeScript error? I've tried but haven't had any luck. |
src/libs/ReportActionsUtils.ts
Outdated
* Checks whether an action is actionable track expense. | ||
* | ||
*/ | ||
function isActionableTrackExpense(reportAction: OnyxEntry<ReportAction>): boolean { |
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.
function isActionableTrackExpense(reportAction: OnyxEntry<ReportAction>): boolean { | |
function isActionableTrackExpense(reportAction: OnyxEntry<ReportAction>): reportAction is ReportActionBase & OriginalMessageActionableTrackedExpenseWhisper { |
We should retain the type predicate so that TS knows the originalMessage
is of track whisper when this returns true
.
src/libs/ReportActionsUtils.ts
Outdated
const report = allReports?.[action?.reportID ?? '']; | ||
const isSelfDM = report?.chatType === CONST.REPORT.CHAT_TYPE.SELF_DM; |
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.
const report = allReports?.[action?.reportID ?? '']; | |
const isSelfDM = report?.chatType === CONST.REPORT.CHAT_TYPE.SELF_DM; |
My bad! There is no reportID
on action
. Could not catch Copilot's shenanigans. I think we better pass the reportID
to getCombinedReportActions
in OptionsListUtils
and add reportID
as an optional parameter for getCombinedReportActions
.
@@ -320,6 +319,11 @@ function getCombinedReportActions(reportActions: ReportAction[], transactionThre | |||
// Filter out request and send money request actions because we don't want to show any preview actions for one transaction reports | |||
const filteredReportActions = [...reportActions, ...filteredTransactionThreadReportActions].filter((action) => { |
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.
const filteredReportActions = [...reportActions, ...filteredTransactionThreadReportActions].filter((action) => { | |
const report = allReports?.[reportID ?? '']; | |
const isSelfDM = report?.chatType === CONST.REPORT.CHAT_TYPE.SELF_DM; | |
const filteredReportActions = [...reportActions, ...filteredTransactionThreadReportActions].filter((action) => { |
src/libs/ReportActionsUtils.ts
Outdated
@@ -309,17 +309,22 @@ function shouldIgnoreGap(currentReportAction: ReportAction | undefined, nextRepo | |||
* Returns a sorted and filtered list of report actions from a report and it's associated child | |||
* transaction thread report in order to correctly display reportActions from both reports in the one-transaction report view. | |||
*/ | |||
function getCombinedReportActions(reportActions: ReportAction[], transactionThreadReportActions: ReportAction[]): ReportAction[] { | |||
function getCombinedReportActions(reportActions: ReportAction[], transactionThreadReportActions: ReportAction[], reportID: string): ReportAction[] { |
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.
function getCombinedReportActions(reportActions: ReportAction[], transactionThreadReportActions: ReportAction[], reportID: string): ReportAction[] { | |
function getCombinedReportActions(reportActions: ReportAction[], transactionThreadReportActions: ReportAction[], reportID?: string): ReportAction[] { |
@@ -149,7 +149,7 @@ function ReportActionsView({ | |||
// Get a sorted array of reportActions for both the current report and the transaction thread report associated with this report (if there is one) | |||
// so that we display transaction-level and report-level report actions in order in the one-transaction view | |||
const combinedReportActions = useMemo( | |||
() => ReportActionsUtils.getCombinedReportActions(allReportActions, transactionThreadReportActions), | |||
() => ReportActionsUtils.getCombinedReportActions(allReportActions, transactionThreadReportActions,reportID), |
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.
() => ReportActionsUtils.getCombinedReportActions(allReportActions, transactionThreadReportActions,reportID), | |
() => ReportActionsUtils.getCombinedReportActions(allReportActions, transactionThreadReportActions), |
@@ -3041,31 +3057,6 @@ const getConvertTrackedExpenseInformation = ( | |||
}, | |||
}); | |||
|
|||
// Resolve actionable whisper message | |||
optimisticData?.push({ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@Nodebrute there are conflicts as well now. |
@c3024 I'll priorities this and finish it today. |
@c3024 Hey, I have resolved the conflicts in my code. I have tested it and it still shows the message when the openReport API is called. Can you help me understand what the next steps are to finish this up? Let's get this issue closed. |
Reviewer Checklist
Screenshots/VideosAndroid: NativetrackExpenseAndroid.mp4Android: mWeb ChrometrackExpenseAndroidChrome.mp4iOS: NativetrackExpenseiOS.mp4iOS: mWeb SafaritrackExpenseiOSSafari.mp4MacOS: Chrome / SafaritrackExpenseChrome.mp4MacOS: DesktoptrackExpenseDesktop.mp4 |
All works well except with a small issue.
App/src/libs/OptionsListUtils.ts Lines 299 to 302 in 525ad6f
now does not use shouldReportActionBeVisibleAsLastAction where isResolvedActionTrackExpense change is included.
So, we should change this App/src/libs/OptionsListUtils.ts Lines 299 to 302 in 525ad6f
to ReportActionUtils.shouldReportActionBeVisible(reportAction, actionKey) &&
!ReportActionUtils.isWhisperAction(reportAction) &&
reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED &&
reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE &&
!ReportActionUtils.isResolvedActionTrackExpense(reportAction), |
Yes, this needs a backend fix separately. |
Please fix this and it is done. |
@c3024 Hey, I have added videos for all platforms. I have updated the code and it works well. Thank you for your patience. |
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.
LGTM
@thienlnam there is a backend fix required as well because when all track expenses are submitted/categorized (when all track expense whispers are resolved) there are no valid visible report actions and backend sends last message for the report as "What would you like to do with this expense?" even though all the whispers are resolved and expenses are moved to other reports. I think backend should not send this as last message text when all the track expense whispers are resolved. |
Thanks, I'll create an issue to track that internally |
Is this still a problem? https://github.com/Expensify/App/pull/41668/files#r1600275772 |
@thienlnam No. cc @c3024 |
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.
Great, 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/thienlnam in version: 1.4.77-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 🚀
|
Details
Fixed Issues
$ #41183
PROPOSAL: #41183 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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
Screen.Recording.2024-05-28.at.2.08.34.AM.mov
Android: mWeb Chrome
Screen.Recording.2024-05-28.at.2.14.52.AM.mov
iOS: Native
Screen.Recording.2024-05-27.at.10.58.41.PM.mov
iOS: mWeb Safari
Screen.Recording.2024-05-27.at.11.04.28.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2024-05-06.at.4.25.24.PM.mov
MacOS: Desktop
Screen.Recording.2024-05-27.at.10.35.54.PM.mov