feat: hold expense in one transaction view#42374
Conversation
melvin-bot
bot
requested review from
dukenv0307 and
shawnborton
and removed request for
a team
|
@shawnborton @dukenv0307 One of you needs to 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] |
|
@tienifr The logic you are trying to add is quite similar to |
|
Looks pretty good to me from a design standpoint. cc @trjExpensify @JmillsExpensify to confirm accuracy. |
|
Nice same, looks good! |
|
@tienifr any updates? |
|
@dukenv0307 This is a component-level logic, there are different logics and is used for just |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-05-24.at.11.28.53.movAndroid: mWeb ChromeScreen.Recording.2024-05-24.at.11.26.31.moviOS: NativeScreen.Recording.2024-05-24.at.11.28.05.moviOS: mWeb SafariScreen.Recording.2024-05-24.at.11.24.49.movMacOS: Chrome / Safariweb-resize.mp4MacOS: Desktopweb-resize.mp4 |
|
looks good to me |
…ld-one-transaction-view
NikkiWines
left a comment
NikkiWines
left a comment
There was a problem hiding this comment.
Looks good, though the overall height of the report header changes when going between an open request with Next Steps and a held request.
Screen.Recording.2024-05-25.at.22.04.52.mov
|
Good catch on that Nikki! I think I like a combination of both of those... so maybe something like this where we have 16px gap between the rows found in the header (top row is avatar + report name, middle row is button, bottom row is status): cc @Expensify/design for thoughts |
|
Looks good to me, but I think the button is too big, no? Looking at this issue I think we're using |
|
I think you are right - I updated the mock above to reflect the medium/default size. |
…ld-one-transaction-view
Co-authored-by: Nikki Wines <nikkiwines@expensify.com>
…m/tienifr/App into feature/hold-one-transaction-view
…ld-one-transaction-view
|
Seems like @tienifr is able to reproduce the issue. I'll wait for them to push changes before reviewing again |
|
@dukenv0307 I found the solution. Root cause here is that This is the same approach used in |
| } | ||
| return reportActions.find((action) => action.reportActionID === transactionThreadReport.parentReportActionID) as OnyxTypes.ReportAction & OnyxTypes.OriginalMessageIOU; | ||
| }, [reportActions, transactionThreadReport?.parentReportActionID]); | ||
| const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION); |
There was a problem hiding this comment.
Connecting to all transactions seems overkill. Can we use a selector to get only the specific report transactions? I believe that we'd also be able to remove the call to TransactionUtils.getAllReportTransactions in hasOnlyHeldExpenses if we only pass report transactions to it
There was a problem hiding this comment.
I think we can use onyxSubscribe and just save the filtered transactions
There was a problem hiding this comment.
My experiment shows that onyxSubscribe leads to more re-render than subscribing to all transaction collection. There's more, please take a look at the code changes:
const transactionIDs = useMemo(() => TransactionUtils.getAllReportTransactions(moneyRequestReport?.reportID).map((t) => t.transactionID), [moneyRequestReport?.reportID]);
const [reportTransactions, setReportTransactions] = useState<OnyxCollection<OnyxTypes.Transaction>>(null);
useEffect(() => {
const unsubscribeTransactions: Array<() => void> = [];
transactionIDs.forEach((transactionID) => {
unsubscribeTransactions.push(
onyxSubscribe({
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
callback: (val) => {
setReportTransactions((prevState) => ({
...prevState,
[`${transactionID}`]: val,
}));
}
}),
);
});
return () => {
unsubscribeTransactions.forEach((unsubscribeTransaction) => unsubscribeTransaction());
};
}, [transactionIDs]);
const hasOnlyHeldExpenses = !Object.values(reportTransactions ?? {}).some((transaction) => !TransactionUtils.isOnHold(transaction));reportTransactions depends on getAllReportTransactions which itself already subscribes to the whole collection. The problem is we couldn't know when new transactions are added to the expense report to re-evaluate reportTransactions. So if all transactions are currently on hold >> show Hold banner >> submit new expense >> the Hold banner is still there because reportTransactions has not included the new expense yet.
There was a problem hiding this comment.
I think this is a systematic problem and should be put to the table for broader discussion in another issue to clean up allTransactions usages in TransactionUtils because it's obviously not up to date. Any component who needs to subscribe to allTransactions need to do that itself using useOnyx. For now, just let's subscribe to transactions collection for simplification because I does not notice any significant performance degrade here in terms of re-render count.
There was a problem hiding this comment.
Thanks for the clarification. I agree that we should address the issue with allTransactions in TransactionUtils since that won't be up to date for components using it. Given that subscribing to all transactions doesn't increase the number of re-renders, I'm fine with the current solution.
|
@tienifr any updates? |
luacmartins
left a comment
luacmartins
left a comment
There was a problem hiding this comment.
Still waiting on @tienifr to address the comments above
|
@tienifr there's conflicts to resolve now, and a comments above to address. Can you give us an ETA for completing this, please? |
…ld-one-transaction-view
|
@tienifr there are linter errors btw |
|
@tienifr did you address the comments? |
|
It seems like this comment is still pending @tienifr #42374 (comment) |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
We decided to revert this PR, see here. |
|
@tienifr please work on a fix for the issues encountered and submit a new PR. Thank you! |
10 participants
Details
This PR adds the ability to hold expense in one transaction view.
Fixed Issues
$ #39989
PROPOSAL: #39989 (comment)
Tests
Holdbanner appears in report headerHoldbanner disappears in report headerOffline tests
NA
QA Steps
Holdbanner appears in report headerHoldbanner disappears in report headerPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2024-05-21.at.14.02.08-compressed.mov
MacOS: Desktop
Screen.Recording.2024-05-21.at.14.07.39-compressed.mov