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.
|
src/libs/MergeTransactionUtils.ts
Outdated
| updatedValues.iouRequestType = transaction?.iouRequestType; | ||
| if (transaction?.iouRequestType) { | ||
| updatedValues.iouRequestType = transaction?.iouRequestType; | ||
| } |
There was a problem hiding this comment.
Will it cause an issue where we can't clear iouRequestType?
There was a problem hiding this comment.
@youssef-lr can you also take a look at this concern? TY
There was a problem hiding this comment.
Looking into this now
src/libs/TransactionUtils/index.ts
Outdated
| function isManagedCardTransaction(transaction: OnyxEntry<Transaction>): boolean { | ||
| return !!transaction?.managedCard; | ||
| function isManagedCardTransaction(transaction: OnyxEntry<Transaction>, cardList?: CardList): boolean { | ||
| return !!transaction?.managedCard || !!(transaction?.cardID && !!cardList?.[transaction.cardID]); |
There was a problem hiding this comment.
Should we use either getTransactionType util or the transactionType in onyx result?
There was a problem hiding this comment.
Ah I missed that. Will update.
There was a problem hiding this comment.
I need to keep in mind this though: #70487. Or simply keep merging logic for all type of cards including personal ones until that issue is done? I think I'll go with this for simplicity now. So I'll use either transactionType or the utility.
There was a problem hiding this comment.
also just noticing, wow, since when did we start returning transactionThreadReportID in the transactions response? I think this is gonna fix a navigation issue after merging
|
Ready for review @hoangzinh! Please check the QA section for test steps related to all the blockers. |
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.2.88-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.88-7 🚀
|
| reportID: string; | ||
|
|
||
| /** The policyID of the report */ | ||
| policyID?: string; |
There was a problem hiding this comment.
@youssef-lr We shouldn’t add a new field to SearchTransaction since it’s deprecated. We need to use the Transaction type instead, so I think we should make a small revert for this newly added field.
cc @luacmartins
There was a problem hiding this comment.
@youssef-lr For more context: #73974. In this project, we’re removing SearchTransaction and using the Transaction type instead. So we shouldn’t add new fields that don’t exist on Transaction, unless we add policyID to the Transaction type.
There was a problem hiding this comment.
If adding policyID to the Transaction type is OK, I can include it in this PR: #77647
There was a problem hiding this comment.
Agreed. We should revert this and access the data via the transaction's report.
There was a problem hiding this comment.
If adding policyID to the Transaction type is OK, I can include it in this PR: #77647
No, we shouldn't do that. We should access it via the transaction's report
| */ | ||
| function isFromCreditCardImport(transaction: OnyxEntry<Transaction>): boolean { | ||
| // This can be set in transactions found in the search snapshot | ||
| if (transaction?.transactionType === CONST.SEARCH.TRANSACTION_TYPE.CARD) { |
There was a problem hiding this comment.
Checklist from #81167: the condition transaction?.bank === CONST.COMPANY_CARD.FEED_BANK_NAME.UPLOAD must be checked before transactionType; otherwise we cannot merge the CSV-imported card.
Explanation of Change
Take 2 of bringing Merge to the reports page with the previous blockers below fixed. Previous PR.
Fixed Issues
#74110
#78211
#78208
#78206
#78182
#78195
#78189
#78179
#78194
#78225
#78181 (comment)
#78190
#78174
#78209
Tests
See QA section.
Offline tests
Same test steps while offline.
QA Steps
In the tests, editable means that the transaction is unreported, is in a draft report, or is a report that is awaiting the first approval.
For a manager, they can edit transactions where they are currently the manager and the report is in the submitted state.
For admins, they can edit all transactions of the policy where the parent report is either draft, or submitted.
Two editable transactions without receipts
Two editable transactions, one with a receipt
Two editable card transactions
One card transaction and one cash transaction with receipt
Manager with editable expenses
Admin
Admin with approved transactions
Tests from blockers of the previous PR
#78211 – Merchant is shown instead of Distance & Rate field when merging with manual distance expense
#78208 – When merging with per diem expense, both receipts show the selected per diem expense
Precondition:
#78206 – When merging with failed scan expense, receipt is missing / Amount shows “Scanning”
Bug 1
Precondition:
Bug 2
Precondition:
Expected:
#78195 – Amount can be selected when merging with card expense after opening Reports
Precondition:
#78194 – Not here page opens when bulk merging split and non-split expense
#78190 – Category & Reimbursable missing on confirmation page
#78189 – Merging cash with card expense causes blank details page / infinite loading
Precondition:
#78182 – Merge option shown when bulk selecting card expenses
Precondition:
#78181 – Allow merging negative expenses everywhere
#78179 – Merge option shown when bulk selecting admin and member expenses
Precondition:
#78174 – App opens Inbox after merging expenses on Reports
#78209 – "From" field displayed empty after merging track expense with expense in workspace
Prerequisite 1: Account has a Self DM.
Prerequisite 2: Account has at least one workspace.
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
MacOS: Desktop