-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Update hold flow on the search page #49003
Conversation
src/libs/actions/Search.ts
Outdated
@@ -94,12 +94,34 @@ function createTransactionThread(hash: number, transactionID: string, reportID: | |||
function holdMoneyRequestOnSearch(hash: number, transactionIDList: string[], comment: string) { | |||
const {optimisticData, finallyData} = getOnyxLoadingData(hash); | |||
|
|||
optimisticData.push({ |
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 need failureData
too in case the API request doesn't succeed.
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 we should probably also:
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.
@jjcoffee If we add a comment to the report optimistically, the backend returns the same comment, resulting in a duplicate. To do that, we need to update the backend to return the same optimistic reportActionID so that we no longer get a duplicate comment.
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.
@jjcoffee I prefer not to add comments optimistically. Do you agree to remove optimistic report action?
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.
Yeah it doesn't seem useful if it gets duplicated. Let's revert for now whilst we decide on the approach.
desktop-chrome-2024-09-18_12.15.55.mp4 |
@jjcoffee This is only the draft change to test. If we agree with this approach totally. I will complete the PR and tag you for reviewing |
@jjcoffee For this bug, we're not resetting the selectedTransaction data in the search context when the new value is updated, resulting in outdated information. My solution is to reset the selected transaction whenever the user clicks the hold/unhold option. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@cretadn22 By reset, do you mean any selected transaction(s) would be deselected? |
@jjcoffee Yess |
@cretadn22 This is maybe a bit of a derail but since this approach is getting quite convoluted, I'm wondering if a simpler approach is to update the |
I'm not sure if that would be expected, but I think we can argue that it's out of scope of this issue since it already happens on main even when just bulk selecting and holding an expense (the dropdown doesn't reflect the updated state). So it's a bit of a separate issue, I think. |
This comment was marked as spam.
This comment was marked as spam.
@jjcoffee Could you please provide a more detailed explanation of this comment? |
@cretadn22 The RCA is that the search page header uses a different key ( |
@jjcoffee From what I observed, SearchPageHeader only use data from useSearchContext hook. And the data in useSearchContext hook is outdated and caused this problem. Is this what you mean? |
This data originally comes from the
|
I still see that we’re updating the correct Onyx key with the proper hash, but I’m not entirely sure I fully understand your point. To clarify further, could you please share a video that shows this bug? |
@cretadn22 I'm proposing an alternative approach, not reporting a bug. So taking a look at these steps:
In step 5 I'm proposing we switch back to using |
@jjcoffee Got it, I misunderstood your point earlier. I'll give your new approach a try and give a feedback soon |
I just created a new pull request to implement @jjcoffee's approach. I’ll mark it as ready once I’ve thoroughly tested it. If this approach is fine, we can close this one |
Details
Fixed Issues
$ #46524
PROPOSAL: #46524 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen210.mov
MacOS: Desktop