-
Notifications
You must be signed in to change notification settings - Fork 3.3k
feat: update Search/Reports page immediately following report actions #56772
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
Conversation
@sobitneupane 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkzie2 we have conflicts
@luacmartins I updated. |
@sobitneupane let's review this one once you're online! |
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.
When I pay using the Pay button in the row, the row gets removed and then reappears due to a payment error when reopening the Approved filtered reports. However, when I open the report in RHP and pay there, the row remains visible, and the error is displayed. Can we replicate this behavior when paying directly through the row?
Screen.Recording.2025-02-17.at.12.53.51.mov
Screen.Recording.2025-02-17.at.12.54.08.mov
@luacmartins While updating the status through RHP, should we close the report in RHP? For example, when I submit a report in Draft, it gets removed from the Draft. Should we close the RHP in this case? IMO, we should keep it open so the user can take multiple steps at once. Screen.Recording.2025-02-17.at.13.05.48.mov |
@shawnborton Could you please check the animation? animation.movScreen.Recording.2025-02-17.at.13.12.00.mov |
@mkzie2 It seems that the required changes for |
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.
Screenshots/Videos
MacOS: Chrome / Safari
Screen.Recording.2025-02-17.at.12.29.01.mov
Screen.Recording.2025-02-17.at.12.32.38.mov
Screen.Recording.2025-02-17.at.12.35.18.mov
Screen.Recording.2025-02-17.at.12.37.25.mov
Screen.Recording.2025-02-17.at.12.39.34.mov
Screen.Recording.2025-02-17.at.12.42.37.mov
Screen.Recording.2025-02-17.at.12.53.51.mov
Screen.Recording.2025-02-17.at.12.54.08.mov
Screen.Recording.2025-02-17.at.13.05.48.mov
@sobitneupane How can we get an error while paying? |
I've asked @Kicu to take a look at this PR since he has a lot of context on Search and can help with the animations too. |
I will look at this today before eod Europe timezone :) |
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.
hey I spend some time trying to understand what is happening here and left some general comments.
There are a few surprising patterns here that I think need further discussions.
My main questions to @mkzie2 is:
- can we not spread
isAllStatus
andhash
all over the app? these are search-specific things (Reports in bottom menu) and should stay separate - can you take another look at how to use exiting animation without changing Opacity View
to @luacmartins and @sobitneupane
- can we somehow simplify the hash usage? I remember Backend needs hash when running actions from search?
- do we have any other patterns where we don't simply accept Onyx data returned from backend but instead build our own data? if no - then this PR should not be the place to introduce this I think (specifically refer to
actions/Search
lines 299-301)
@@ -163,6 +164,8 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea | |||
const isPayAtEndExpense = isPayAtEndExpenseTransactionUtils(transaction); | |||
const isArchivedReport = isArchivedReportWithID(moneyRequestReport?.reportID); | |||
const [archiveReason] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${moneyRequestReport?.reportID}`, {selector: getArchiveReason}); | |||
const {currentSearchHash, isAllStatus} = useSearchContext(); | |||
const hash = isAllStatus ? undefined : currentSearchHash; |
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.
question to @luacmartins In general why do we have to push search hash to these functions which didn't use them before?
In what contexts does the backend need to use hash?
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.
If I'm correct, the backend is not using the hash, this is purely used by the frontend to build optimistic data
needsOffscreenAlphaCompositing={shouldRenderOffscreen ? needsOffscreenAlphaCompositing : undefined} | ||
exiting={shouldAnimateOnRemove ? Exiting : undefined} | ||
onLayout={(e) => { |
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.
This component (OpacityView
) is being used by PressableWithFeedback
which is used all over the app in crucial places.
We cannot add an onLayout
on this animated component, since it might substantially slow down the app. This will result in multiple setState
being run for every pressable component, multiple times.
Is there any other way to add this animation, without affecting OpacityView?
unapproveExpenseReport(moneyRequestReport); | ||
}, [isMoneyRequestExported, moneyRequestReport, isDelegateAccessRestricted]); | ||
unapproveExpenseReport(moneyRequestReport, isAllStatus ? undefined : currentSearchHash); | ||
}, [isDelegateAccessRestricted, isMoneyRequestExported, moneyRequestReport, isAllStatus, currentSearchHash]); |
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 really try to not connect Reports that deeply to Search - these features are separate.
It is unfortunate that we need to pass down isAllStatus
and currentSearchHash
to be able to run animations.
We should try to come up with another way of updating.
@@ -343,14 +374,29 @@ function payMoneyRequestOnSearch(hash: number, paymentData: PaymentData[], trans | |||
]; | |||
|
|||
const optimisticData: OnyxUpdate[] = createOnyxData({isActionLoading: true}); | |||
const failureData: OnyxUpdate[] = createOnyxData({errors: getMicroSecondOnyxErrorWithTranslationKey('common.genericErrorMessage')}); | |||
const finallyData: OnyxUpdate[] = createOnyxData({isActionLoading: false}); | |||
const successData: OnyxUpdate[] = isAllStatus |
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.
Search context is a bit crowded already, and I wonder if we actually need isAllStatus
used in so many places in the app.
Can you write why we need isAllStatus
? How does the search status
change affect handling onyx data?
@@ -8250,7 +8319,7 @@ function getNextApproverAccountID(report: OnyxEntry<OnyxTypes.Report>, isUnappro | |||
return getAccountIDsByLogins([nextApproverEmail]).at(0); | |||
} | |||
|
|||
function approveMoneyRequest(expenseReport: OnyxEntry<OnyxTypes.Report>, full?: boolean) { | |||
function approveMoneyRequest(expenseReport: OnyxEntry<OnyxTypes.Report>, full?: boolean, hash?: number) { |
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.
I think it's a mistake to add optional hash
to all these actions that are report centric.
These functions should not know about the existence of Search. Up until now everything worked nicely with these functions being separate, and now because of adding an animation when something is removed we are combining them together.
I would consider a different approach.
As far as API requests go, the hash usage should be restricted to the
I think for Search, this is the first place where we build data that is NOT |
@@ -80,6 +80,7 @@ import MoneyRequestHeaderStatusBar from './MoneyRequestHeaderStatusBar'; | |||
import type {ActionHandledType} from './ProcessMoneyReportHoldMenu'; | |||
import ProcessMoneyReportHoldMenu from './ProcessMoneyReportHoldMenu'; | |||
import ExportWithDropdownMenu from './ReportActionItem/ExportWithDropdownMenu'; | |||
import {useSearchContext} from './Search/SearchContext'; |
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.
BTW I don't think we should connect this component to Search context since this component is used in the Inbox flow, not Search. I think we need to keep changes contained to the Search components since this is a Search feature
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.
@luacmartins When the user opens the RHP from Search and performs actions like approving or paying a money request, we will need to use the hash to update the search results.
@@ -163,6 +164,8 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea | |||
const isPayAtEndExpense = isPayAtEndExpenseTransactionUtils(transaction); | |||
const isArchivedReport = isArchivedReportWithID(moneyRequestReport?.reportID); | |||
const [archiveReason] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${moneyRequestReport?.reportID}`, {selector: getArchiveReason}); | |||
const {currentSearchHash, isAllStatus} = useSearchContext(); | |||
const hash = isAllStatus ? undefined : currentSearchHash; |
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.
If I'm correct, the backend is not using the hash, this is purely used by the frontend to build optimistic data
Please let me know if you have a better suggestion. Thanks. |
@Kicu Any thought in my comment above. |
@Kicu is focused on a different project and unfortunately has no availability to help on this one.
I'm not sure that I follow the strategy here. Why are we removing data from snapshot when we just update the state/status of the report? @sobitneupane would you be able to help @mkzie2? |
@luacmartins For example, if we're in the draft and submit a report in search or RHP, this report should be removed from the draft if we submit it successfully. So I removed it in the success data to remove this immediately. If not, it's only removed if we refresh the page. |
I'll review this PR and explore possible solutions. I'll provide an update as soon as possible. |
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.
From my understanding so far:
- Different search results are returned from the backend based on the status.
- We need to use
searchHash
to update the search result stored in local storage. hash
is required on components likeMoneyReportHeader
andReportDetailsPage
because users can open expense on RHP and perform different actions. To reflect those changes on search results, search results stored in onyx should be updated.- I believe we can avoid the change made in
actions/IOU
if we use search-specific functions as suggested below.
@luacmartins With the approach suggested by @mkzie2, most of the changes made in the PR seem necessary.
@@ -80,6 +80,7 @@ import MoneyRequestHeaderStatusBar from './MoneyRequestHeaderStatusBar'; | |||
import type {ActionHandledType} from './ProcessMoneyReportHoldMenu'; | |||
import ProcessMoneyReportHoldMenu from './ProcessMoneyReportHoldMenu'; | |||
import ExportWithDropdownMenu from './ReportActionItem/ExportWithDropdownMenu'; | |||
import {useSearchContext} from './Search/SearchContext'; |
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.
@luacmartins When the user opens the RHP from Search and performs actions like approving or paying a money request, we will need to use the hash to update the search results.
@@ -236,10 +239,10 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea | |||
} else if (isInvoiceReport(moneyRequestReport)) { | |||
payInvoice(type, chatReport, moneyRequestReport, payAsBusiness); | |||
} else { | |||
payMoneyRequest(type, chatReport, moneyRequestReport, true); | |||
payMoneyRequest(type, chatReport, moneyRequestReport, true, hash); |
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.
@mkzie2 an we determine if the Expense Report is being opened from Search? If so, would it be possible to use payMoneyRequestOnSearch
instead?
I'm not sure this is needed. We have live Onyx updates, so any changes made to Onyx data should be reflected in any equivalent snapshot_ data, e.g. if |
@luacmartins, that's correct, changes are reflected in the snapshots. However, in our case, we need to remove the report from the snapshot. We maintain different snapshots based on status. Changes made to the report are reflected in the snapshot, but when we approve an expense report, we need to remove it from the snapshot of outstanding expenses. These snapshots are returned from the backend, and I don’t think any filtering is done on the frontend to determine whether the report belongs to a particular status or not. @mkzie2, could you please verify this? |
Yea, that's correct. Maybe in that case we should just re-trigger the search so update the view then. WDYT? |
@luacmartins Yup, I believe we can do that. Should we re-trigger the search after the user closes the RHP? One drawback is that the search will be re-triggered even if the user doesn't perform any action. @mkzie2 Can we re-trigger the search after the user performs certain actions (like paying, approving, etc.), without requiring the search hash? |
Let me investigate if we can do that. |
Now we have two problems that need to be resolved:
|
@sobitneupane I tested this but it has a problem: If the user select a transaction at index 1, another one at index 51, another one at index 101. Re-trigger the cc @luacmartins |
@sobitneupane Any thought on my comment above. |
Oops, Sorry. I missed it earlier.
I’m not sure I fully understand the case you mentioned. Could you please clarify? We want to re-trigger the search when the user performs an action by opening the report in the RHP. |
@sobitneupane I think it's the same; for example if we select an item with index 2 and the current offset is |
Gonna make this a draft while we work on the updated solution |
Re-triggering the search would update the entire view. However, a downside is that the user would be sent back to the top of the list, causing the whole list to re-render, which doesn’t seem ideal. |
@luacmartins This issue is becoming increasingly complicated. The PR and idea proposed by @mkzie2 are mainly based on spreading search-specific elements like |
I agree. Let's do that. Sorry @mkzie2, but let's start fresh with a new proposal for this since the current one is not acceptable. |
Explanation of Change
feat: update Search/Reports page immediately following report actions
Fixed Issues
$ #55211
PROPOSAL: #55211 (comment)
Tests
Precondition: Create a workspace and enable workflow, approval, and delay submission (manually)
Verify that no errors appear in the JS console
Unapprove Report
Offline tests
None
QA Steps
Precondition: Create a workspace and enable workflow, approval, and delay submission (manually)
Verify that no errors appear in the JS console
Unapprove Report
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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.2025-02-12.at.23.48.09.mov
Android: mWeb Chrome
Screen.Recording.2025-02-12.at.23.43.53.mov
iOS: Native
Screen.Recording.2025-02-12.at.23.49.54.mov
iOS: mWeb Safari
Screen.Recording.2025-02-12.at.23.46.38.mov
MacOS: Chrome / Safari
Screen.Recording.2025-02-12.at.23.38.15.mov
Screen.Recording.2025-02-12.at.23.38.50.mov
Screen.Recording.2025-02-12.at.23.38.59.mov
Screen.Recording.2025-02-12.at.23.39.39.mov
Screen.Recording.2025-02-13.at.00.04.19.mov
Screen.Recording.2025-02-17.at.15.15.39.mov
Screen.Recording.2025-02-17.at.15.15.52.mov
MacOS: Desktop
Screen.Recording.2025-02-12.at.23.52.32.mov