Issue 86223 update cta and workspace chat copy for track intent users#92254
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
…223-update-CTA-and-Workspace-Chat-Copy-For-Track-intent-users
…py-For-Track-intent-users
…py-For-Track-intent-users
…e' when replying in a thread
… are eligible; hide action button for mixed selections
… the 'Mark as done' text where applicable
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 958b6c0981
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| options.push({ | ||
| icon: expensifyIcons.Send, | ||
| text: translate('common.submit'), | ||
| text: selectedReports.length > 0 && allReportsShouldMarkAsDone ? translate('common.markAsDone') : translate('common.submit'), |
There was a problem hiding this comment.
Use Mark as done for transaction bulk submits
When a track-intent user selects submit-able transaction rows rather than report rows, selectedReports.length is 0, so this always labels the bulk action as Submit even when every selected transaction belongs to a report where the per-row CTA is now Done/Mark as done. This leaves the search bulk CTA out of the new copy for transaction-search selections; derive the mark-as-done state from the selected transactions' report IDs as well, and apply the same mixed-selection guard there.
Useful? React with 👍 / 👎.
|
@MelvinBot, why is the |
|
Root cause: The change to the "Self review" branch in text: hasTransactions ? 'submit' : 'add',and this PR replaced it with: text: shouldShowMarkAsDoneCopy ? 'mark this as done' : 'submit',This dropped the The failing test The test file itself wasn't touched in this PR — only Recommended fix: restore the text: shouldShowMarkAsDoneCopy ? 'mark this as done' : hasTransactions ? 'submit' : 'add',Why only this block (and not the isReopen / Manual submission blocks)The other two blocks you changed in the same file only ever emitted Next Steps: Reply with |
…py-For-Track-intent-users
…py-For-Track-intent-users
|
@ZhenjaHorbach Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7db15ea83
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| options.push({ | ||
| icon: expensifyIcons.Send, | ||
| text: translate('common.submit'), | ||
| text: allReportsShouldMarkAsDone ? translate('common.markAsDone') : translate('common.submit'), |
There was a problem hiding this comment.
Avoid marking transaction-only submits as done
For transaction-only selections selectedReports is empty, so allReportsShouldMarkAsDone comes from selectedReports.every(...) and is true by vacuous truth. That makes every transaction search bulk-submit CTA render as “Mark as done”, even for non-track users or reports that should still say “Submit”; derive this label from the selected transactions' reports or guard the report-only boolean with selectedReports.length > 0.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot any suggestion for this comment, If we add the selectedReports.length > 0 length check here, we got this comment #92254 (comment), and do you think this change is needed here ?
There was a problem hiding this comment.
The two bot comments aren't actually contradictory — they're two symptoms of the same root cause, and the selectedReports.length > 0 guard does fix a real bug.
Root cause: allReportsShouldMarkAsDone is selectedReports.every(...), which is true by vacuous truth when selectedReports is empty (i.e. transaction-row selections). So at line 1473 a transaction-only selection always renders "Mark as done" — even for non-track users. That's the regression r3348950734 is flagging, and it's worth fixing.
So yes, add the guard:
text: selectedReports.length > 0 && allReportsShouldMarkAsDone ? translate('common.markAsDone') : translate('common.submit'),This restores "Submit" for every transaction-only selection. Report-row selections are unaffected (length is truthy, and that branch is already guarded inside shouldShowSubmitOption).
About the opposite comment (r3346664959): it only matters if you intend track-intent transaction-row selections to also show "Mark as done". That's beyond this PR's scope — the feature is built around report-row selection (see Test 7), so treating transaction-row selections as plain "Submit" is a reasonable, intentional limitation, and that bot nit can be dismissed. This is the same scoping call you already made for the approve case in useSelectionModeReportActions.ts.
If you do want to support it later, the clean way is to derive the state from selectedTransactionReportIDs (already computed) when selectedReports is empty, rather than hardcoding "Submit" — but that's optional and not needed to ship this PR.
| if (hasSubmitAction && !shouldBlockSubmit) { | ||
| actions[idx++] = { | ||
| text: translate('common.submit'), | ||
| text: shouldShowMarkAsDone({policy, report, isTrackIntentUser}) ? translate('common.markAsDone') : translate('common.submit'), |
There was a problem hiding this comment.
Apply Mark as done copy to approve selection actions
This only updates the submit entry in the report selection toolbar, but the approve entry in the same selectionModeReportLevelActions still uses translate('iou.approve'). For a track-intent submit-and-close report where the user is also the next approver, selecting all expenses exposes the APPROVE action via MoneyRequestReportView/SelectionToolbar, so this path still shows “Approve” while the header/dropdown primary actions now show “Mark as done”.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It's fine not to use it for the approve case, since we won't have a scenario where the policy is set to "submit and close" and the "Approve" button is displayed at the same time.
|
Sorry for the ping, @ZhenjaHorbach . @mananjadhav will review this PR, as previous PR was reverted and had already been reviewed by Manan. |
|
@mananjadhav, I was thinking that we should temporarily add a feature flag to control the "Mark as done" functionality. Since it's currently enabled, having a flag would allow us to quickly disable it if this PR ends up causing multiple regressions. That way, we wouldn't need to revert the PR or raise another large PR with extensive changes. Instead, we could simply disable the feature and focus on fixing the regressions. WDYT ? |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ceaf2366ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (selectedReports.length | ||
| ? selectedReports.every((report) => report.canSubmit) | ||
| ? selectedReports.every((report) => report.canSubmit) && | ||
| // Disable for mixed selections: all must be the same submit type | ||
| (isTrackIntentUser ? allReportsShouldMarkAsDone || noReportsShouldMarkAsDone : true) | ||
| : selectedTransactionsKeys.every((id) => selectedTransactions[id].action === CONST.SEARCH.ACTION_TYPES.SUBMIT)); |
There was a problem hiding this comment.
Disable mixed submit/done transaction selections
When the selection comes from transaction rows, selectedReports.length is zero, so this branch only checks that every row has the SUBMIT action and ignores the new allReportsShouldMarkAsDone || noReportsShouldMarkAsDone guard. A track-intent user can therefore select transactions from both a submit-and-close/self report and a regular submit report and still get one bulk Submit option, even though the report-selection path explicitly disables mixed submit types; apply the same mark-as-done consistency check to the transaction branch.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35eb6ffcf3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if ( | ||
| shouldShowMarkAsDone({ | ||
| isTrackIntentUser, | ||
| policy, | ||
| report: originalReport, | ||
| }) | ||
| ) { | ||
| return <ReportActionItemBasicMessage message={translate('iou.markedAsDone', getOriginalMessage(action)?.message)} />; |
There was a problem hiding this comment.
Preserve automatic-submit/DEW messages before mark-done copy
For track-intent submit-and-close reports, this new early return runs before the existing wasSubmittedViaHarvesting and pending DEW checks, so an auto-submitted or queued-DEW submit action is rendered as “marked as done” instead of “automatically submitted” / “queued to submit via DEW”. The LHN/copy-message paths still check harvesting/DEW first, so the main report action content becomes inconsistent and hides the explanation for those environments.
Useful? React with 👍 / 👎.
| text: actionText, | ||
| }, | ||
| { | ||
| text: shouldShowMarkAsDoneCopy ? '.' : ' %expenses.', |
There was a problem hiding this comment.
Keep the expense noun when the report is empty
When a track-intent submit-and-close report has no transactions, shouldShowMarkAsDoneCopy can still be true because shouldShowMarkAsDone does not check hasTransactions, while actionText remains add. This line then drops the %expenses. suffix and renders the deprecated next step as “Waiting for … to add.” instead of “Waiting for … to add expenses.”
Useful? React with 👍 / 👎.
Explanation of Change
For users who onboarded with the
Organize my personal spending/Track expenses for my businessintent (PERSONAL_SPEND/TRACK_WORKSPACE), the product copy still uses generic expense management language. This PR updates three areas to reflect the track-intent workflow:These changes only apply when both conditions are met:
TRACK_WORKSPACEorPERSONAL_SPEND)Fixed Issues
$ #86223
PROPOSAL: #86223 (comment)
Tests
Test 1:
Test 2:
Test 3:
Test 4:
Prerequisites:
Steps:
Test 5:
Test 6:
Prerequisite:
Steps:
Test 7:
Test 8:
Prerequisite:
Steps:
Offline tests
Same as Tests
QA Steps
Same as Tests
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))npm run compress-svg)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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari