76855/added compute function#77827
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
e2e3588 to
a41683a
Compare
|
@eVoloshchak 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] |
JmillsExpensify
left a comment
There was a problem hiding this comment.
Part of an on-going product initiative.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-12-28.at.19.40.37.movAndroid: mWeb ChromeScreen.Recording.2025-12-28.at.19.41.29.moviOS: HybridAppScreen.Recording.2025-12-28.at.19.18.11.moviOS: mWeb SafariScreen.Recording.2025-12-28.at.19.20.44.movMacOS: Chrome / SafariScreen.Recording.2026-01-08.at.11.18.28.mov |
neil-marcellini
left a comment
There was a problem hiding this comment.
Looks pretty good, thanks. I have a few comments.
I notice that we're not passing in any data for the new reportTransactions parameters. As a result if we're building an optimistic expense report as a result of creating a new expense, such as with requestMoney and getMoneyRequestInformation, then the computed formula would be inaccurate if it references data on the transaction, right? For example {report:startdate} and the transaction has it's date set to one in the past. Can you please manually test that case?
neil-marcellini
left a comment
There was a problem hiding this comment.
Thanks for the updates, but you still need to pass the reportTransactions to buildOptimisticExpenseReport for this to work optimistically.
You didn't test offline, so that's not properly testing the optimistic handling. Here's proof it doesn't work.
2025-12-31_13-45-29.mp4
Also, given that we're adding this new optimistic behavior with this PR, and it's not fully implemented, can we please only compute the name optimistically if the user is on the beta CUSTOM_REPORT_NAMES?
@neil-marcellini Thanks for the feedback! I've made the following changes:
Regarding passing reportTransactions to buildOptimisticExpenseReport - should I update the call sites in IOU.ts to pass the optimistic transaction data in this PR, or would you prefer that to be done in a follow-up PR? There are several call sites that would need to be updated. |
neil-marcellini
left a comment
There was a problem hiding this comment.
Your last batch of updates looks good.
Regarding passing reportTransactions to buildOptimisticExpenseReport - should I update the call sites in IOU.ts to pass the optimistic transaction data in this PR, or would you prefer that to be done in a follow-up PR? There are several call sites that would need to be updated.
Yes, please do that here. It feels required for the PR to be a meaningful change. I also don't like adding a new parameter that is unused.
…asifaizan70/Expensify into 76855/added-compute-function
|
@neil-marcellini I updated. Please have a look. |
neil-marcellini
left a comment
There was a problem hiding this comment.
Thanks for this. I didn't realize that the optimistic transaction was generated after the report, so that does make it harder. I like what you did with building a minimal transaction, but it looks like some fields are missing. Please be sure to test thoroughly.
Also, pls write more descriptive commit messages. Lmk when it's ready for the next review.
…tation - Add MinimalTransaction type in Formula.ts with Pick<Transaction, ...> including: transactionID, reportID, created, amount, currency, merchant, pendingAction - Update buildMinimalTransactionForFormula to accept reportID and merchant params - Generate expense report ID upfront at all call sites for proper transaction matching - Pass merchant field to enable isPartialTransaction() checks in formula computation
|
@neil-marcellini Done! Added reportID and merchant to the minimal transaction, and created a MinimalTransaction type in Formula.ts. Also generating the report ID upfront now so the transaction's reportID matches the new expense report. Let me know if anything else needs adjusting! |
neil-marcellini
left a comment
There was a problem hiding this comment.
Just a few more small changes, then we need the C+ @eVoloshchak to test again.
|
@neil-marcellini @eVoloshchak resolved feedbacks. |
eVoloshchak
left a comment
There was a problem hiding this comment.
Re-tested offline and online cases, works well
|
✋ 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/neil-marcellini in version: 9.3.0-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.3.0-8 🚀
|
| const optimisticReportName = populateOptimisticReportFormula(titleReportField?.defaultValue ?? CONST.POLICY.DEFAULT_REPORT_NAME_PATTERN, optimisticEmptyReport, policy); | ||
| optimisticEmptyReport.reportName = optimisticReportName; | ||
| // Only compute optimistic report name if the user is on the CUSTOM_REPORT_NAMES beta | ||
| if (Permissions.isBetaEnabled(CONST.BETAS.CUSTOM_REPORT_NAMES, allBetas)) { |
There was a problem hiding this comment.
Hi @abbasifaizan70, could you explain why we don't maintain the old logic to support accounts that haven't been granted CONST.BETAS.CUSTOM_REPORT_NAMES?
I mean:
if (Permissions.isBetaEnabled(CONST.BETAS.CUSTOM_REPORT_NAMES, allBetas)) {
// Your new function
} else {
// populateOptimisticReportFormula function
}There was a problem hiding this comment.
Hi @hoangzinh,
You're absolutely right! We should maintain backward compatibility for users who aren't on the CUSTOM_REPORT_NAMES beta. Currently, those users won't get any optimistic report name computation, which is a regression from the previous behavior.
I'll update the code to add a fallback to populateOptimisticReportFormula for users not on the beta, so they continue to get optimistic report names using the existing logic. This will ensure:
- Users on the beta get the improved
computefunction with better formula handling - Users not on the beta continue to get optimistic report names via
populateOptimisticReportFormula(maintaining existing behavior)
I'll apply this change to all three locations:
buildOptimisticExpenseReportinReportUtils.tsbuildOptimisticEmptyReportinReportUtils.tsconvertIOUReportToExpenseReportinReport.ts
Thanks for catching this!
I created a PR to handle this issue. Let me know if it works.
There was a problem hiding this comment.
@nkdengineer also raised a PR but it does not completely adding check for old users.
There was a problem hiding this comment.
@abbasifaizan70 just wanna double check, is @nkdengineer's approach correct? I'm unsure about this one
Since it's applied for all users now, we can remove this permission check.
There was a problem hiding this comment.
@neil-marcellini let me know about your thinking.



Explanation of Change
This PR will make sure to use function (compute) for computing report names from formula templates.
Fixed Issues
$ #76855
PROPOSAL: #76855 (comment)
Tests
Prerequisites
{report:type} - {report:total}Report {report:id} - {report:policyname}{report:status} Report for {report:workspacename}{report:type}{report:id}{report:total}{report:policyname}{report:workspacename}{report:status}{report:currency}Test Case 1: Create New Expense in Workspace
Test Case 2: Offline Mode
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Test 01
Test 02
Screen.Recording.2025-12-17.at.7.58.13.PM.mov