Fix - Search - Expense from the list does not disappear after deletion #54407
+23
−12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
melvin-bot
bot
requested review from
rayane-djouah
and removed request for
a team
|
@rayane-djouah 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] |
This reverts commit af28f6e.
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-01-03.at.12.19.47.AM.movAndroid: mWeb ChromeScreen.Recording.2025-01-02.at.11.21.56.PM.moviOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-01-03.at.00.51.26.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-01-02.at.23.19.06.mp4MacOS: Chrome / SafariScreen.Recording.2025-01-02.at.11.16.47.PM.movMacOS: DesktopScreen.Recording.2025-01-02.at.11.16.07.PM.mov |
luacmartins
reviewed
Jan 2, 2025
There was a problem hiding this comment.
@FitseTLT let's add some unit tests as suggested in the proposal
rayane-djouah
suggested changes
Jan 2, 2025
| const hasTransactionChange = !isEqual(transactionIDList, previousTransactionIDList); | ||
| const hasReportActionChange = !isEqual(reportActionIDList, previousReportActionIDList); | ||
|
|
||
| // Check if there is a change in transaction or report action list |
There was a problem hiding this comment.
Suggested change
| // Check if there is a change in transaction or report action list | |
| // Check if there is a change in the transactions list or the report actions list |
Comment on lines
+57
to
+59
| // We only want to highlight new items only if addition of transactions or report actions triggered the search. | ||
| // This is because on deletion of items sometimes the BE returns old items in place of the deleted ones | ||
| // but we don't want to highlight these old items although they are new to the current search result. |
There was a problem hiding this comment.
Suggested change
| // We only want to highlight new items only if addition of transactions or report actions triggered the search. | |
| // This is because on deletion of items sometimes the BE returns old items in place of the deleted ones | |
| // but we don't want to highlight these old items although they are new to the current search result. | |
| // We only want to highlight new items if the addition of transactions or report actions triggered the search. | |
| // This is because, on deletion of items, the backend sometimes returns old items in place of the deleted ones. | |
| // We don't want to highlight these old items, even if they appear new in the current search results. |
| @@ -26,6 +27,7 @@ function useSearchHighlightAndScroll({searchResults, transactions, previousTrans | |||
| // Ref to track if the search was triggered by this hook | |||
| const triggeredByHookRef = useRef(false); | |||
| const searchTriggeredRef = useRef(false); | |||
| const hasItemBeenAddedRef = useRef(false); | |||
There was a problem hiding this comment.
Suggested change
| const hasItemBeenAddedRef = useRef(false); | |
| const hasNewItemsRef = useRef(false); |
| @@ -34,20 +36,29 @@ function useSearchHighlightAndScroll({searchResults, transactions, previousTrans | |||
|
|
|||
| // Trigger search when a new report action is added while on chat or when a new transaction is added for the other search types. | |||
| useEffect(() => { | |||
| const previousTransactionsLength = previousTransactions && Object.keys(previousTransactions).length; | |||
| const transactionsLength = transactions && Object.keys(transactions).length; | |||
| const previousTransactionIDList = Object.keys(previousTransactions ?? {}); | |||
There was a problem hiding this comment.
Suggested change
| const previousTransactionIDList = Object.keys(previousTransactions ?? {}); | |
| const previousTransactionsIDs = Object.keys(previousTransactions ?? {}); |
| const previousTransactionsLength = previousTransactions && Object.keys(previousTransactions).length; | ||
| const transactionsLength = transactions && Object.keys(transactions).length; | ||
| const previousTransactionIDList = Object.keys(previousTransactions ?? {}); | ||
| const transactionIDList = Object.keys(transactions ?? {}); |
There was a problem hiding this comment.
Suggested change
| const transactionIDList = Object.keys(transactions ?? {}); | |
| const transactionsIDs = Object.keys(transactions ?? {}); |
| const prevReportActionsLength = previousReportActions && Object.values(previousReportActions).reduce((sum, curr) => sum + Object.keys(curr ?? {}).length, 0); | ||
| // Return early if search was already triggered or there's no change in current and previous data length | ||
| if (searchTriggeredRef.current || (!isChat && previousTransactionsLength === transactionsLength) || (isChat && reportActionsLength === prevReportActionsLength)) { | ||
| const reportActionIDList = Object.values(reportActions ?? {}) |
There was a problem hiding this comment.
Suggested change
| const reportActionIDList = Object.values(reportActions ?? {}) | |
| const reportActionsIDs = Object.values(reportActions ?? {}) |
| const reportActionIDList = Object.values(reportActions ?? {}) | ||
| .map((actions) => Object.keys(actions ?? {})) | ||
| .flat(); | ||
| const previousReportActionIDList = Object.values(previousReportActions ?? {}) |
There was a problem hiding this comment.
Suggested change
| const previousReportActionIDList = Object.values(previousReportActions ?? {}) | |
| const previousReportActionsIDs = Object.values(previousReportActions ?? {}) |
rayane-djouah
suggested changes
Jan 2, 2025
| return; | ||
| } | ||
| const newTransactionAdded = transactionsLength && typeof previousTransactionsLength === 'number' && transactionsLength > previousTransactionsLength; | ||
| const newReportActionAdded = reportActionsLength && typeof prevReportActionsLength === 'number' && reportActionsLength > prevReportActionsLength; | ||
| const hasTransactionChange = !isEqual(transactionIDList, previousTransactionIDList); |
There was a problem hiding this comment.
Suggested change
| const hasTransactionChange = !isEqual(transactionIDList, previousTransactionIDList); | |
| const hasTransactionsIDsChange = !isEqual(transactionIDList, previousTransactionIDList); |
| const newTransactionAdded = transactionsLength && typeof previousTransactionsLength === 'number' && transactionsLength > previousTransactionsLength; | ||
| const newReportActionAdded = reportActionsLength && typeof prevReportActionsLength === 'number' && reportActionsLength > prevReportActionsLength; | ||
| const hasTransactionChange = !isEqual(transactionIDList, previousTransactionIDList); | ||
| const hasReportActionChange = !isEqual(reportActionIDList, previousReportActionIDList); |
There was a problem hiding this comment.
Suggested change
| const hasReportActionChange = !isEqual(reportActionIDList, previousReportActionIDList); | |
| const hasReportActionsIDsChange = !isEqual(reportActionIDList, previousReportActionIDList); |
| @@ -103,7 +114,7 @@ function useSearchHighlightAndScroll({searchResults, transactions, previousTrans | |||
| // Find new transaction IDs that are not in the previousTransactionIDs and not already highlighted | |||
| const newTransactionIDs = currentTransactionIDs.filter((id) => !previousTransactionIDs.includes(id) && !highlightedIDs.current.has(id)); | |||
|
|
|||
| if (!triggeredByHookRef.current || newTransactionIDs.length === 0) { | |||
| if (!triggeredByHookRef.current || newTransactionIDs.length === 0 || !hasItemBeenAddedRef.current) { | |||
|
@FitseTLT Could you please look into this edge case? When a transaction is deleted from another device, it doesn’t get deleted on the current device: Screen.Recording.2025-01-02.at.11.04.03.PM.1.movAdditionally, in the video, you can see that a scroll was triggered. I’m not entirely sure about the exact steps to reproduce this behavior, but it seems that we need to fix it. |
|
@rayane-djouah I don't know if it aligns with the bug you discovered but on my side when the expense deleted was the only expense on the report the transaction list is not updated so we don't trigger search. But in this case the report actions list is updated so if you agree we can apply a workaround solution by triggering the search for expense case too when the report actions list is updated by changing WDYT |
|
Any updates on this one? @rayane-djouah @FitseTLT |
@FitseTLT I think that this could lead to unnecessary search API calls, which might impact performance |
Waiting on @FitseTLT to address the review comments |
@rayane-djouah I know it would result in unnecessary search API calls that's why I asked to confirm the workaround solution from FE but it appears we are out of options if we want a FE solution because the only info we have got in FE to trigger the search API is change of transactions and report actions and in this case transaction list is not changed when the deleted expense is the only expense of the iou report. |
|
Yea, it seems that Screen.Recording.2025-01-09.at.9.49.33.PM.movScreen.Recording.2025-01-09.at.9.49.33.PM.mov@luacmartins Can you please look for a backend fix for including deleted transactions data in response as well? @FitseTLT Let's address code review comments and add unit tests, I think that the above bug needs to be fixed from backend |
|
Hmm I do see us setting the transaction to null in
|
|
@luacmartins we're setting the transaction to null for |
|
Hmm interesting. I see that the
|
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ #53687
PROPOSAL: #53687 (comment)
Tests
Offline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so 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
a.search.mp4
Android: mWeb Chrome
aw.search.mp4
iOS: Native
i.search.mp4
iOS: mWeb Safari
iw.search.mp4
MacOS: Chrome / Safari
w.search.mp4
MacOS: Desktop
d.sear.mp4