-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD for payment 2025-02-04] [$250] Reports page - Draft report not showing in the draft status filter #55235
Comments
Job added to Upwork: https://www.upwork.com/jobs/~021879246404932856846 |
Current assignee @trjExpensify is eligible for the Bug assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @hungvu193 ( |
From the logs, we can see that the Search API request was made with offset of 100 which is incorrect:
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Reports page - Draft report not showing in the draft status filter What is the root cause of that problem?When we use pagination for any of the chat tabs, we set offset of last loaded page. But when we change the tab we are not resetting offset. And we load the data by using previous tab offset. For ex, previously What changes do you think we should make in order to solve the problem?To fix issue issue we need to reset the offset on tab change. Solution 1 We can pass uniq key here. By passing uniq key we are make sure that whenever we change tab, search component will re-render.
Solution 2 we can use useEffect here to reset the offset when status change.
Here I added condition for CHAT type only, but we can applied for all kind reports. What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?UI bug so not sure if we can test it or not. If possible we can test if offset is reset to 0 when chat report tab is changed. What alternative solutions did you explore? (Optional)Na |
🚨 Edited by proposal-police: This proposal was edited at 2025-01-16 08:30:32 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.The empty state page appears. What is the root cause of that problem?The App/src/components/Search/index.tsx Line 428 in 7b9e55e
When we change from App/src/components/Search/SearchStatusBar.tsx Lines 180 to 182 in 7b9e55e
What changes do you think we should make in order to solve the problem?If we reset the offset whenever the status is changed, we still have a problem the For example: We're in the Screen.Recording.2025-01-16.at.15.28.33.movTo resolve it, we can store the offset as an object that will store the
Then when we fetch more results, update the offset of this tab
App/src/components/Search/index.tsx Line 428 in 5133090
Here is the result with this solution, the API only called one time Screen.Recording.2025-01-16.at.15.29.54.movWhat specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?What alternative solutions did you explore? (Optional)Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
I created 2 open report on OD and there's no expense on draft of ND even when the offset of the params is 0. Screen.Recording.2025-01-15.at.15.33.49.mov |
@hungvu193 Can you see the response in the API? Maybe it's a case of a backend bug. |
@hungvu193 You can try to reproduce by logging into an account that has many expenses (> 50) then scrolling until we fetch more result and then open the Screen.Recording.2025-01-15.at.15.49.24.mov |
@hungvu193 This is UI issue, we are passing wrong offset parameter as mention by #55235 (comment), due to this response is empty. |
There are 2 issues here:
I believe the first one should be fixed from BE. I'll review the second one, which both proposals mentioned about. |
I agree about the @nkdengineer What do you mean by this one? Doesn't that Search page will always rerender when switching from LHN?
|
@hungvu193 When we click on LHN, we will navigate to a new page instead of changing the route params in the existing search page. App/src/pages/Search/SearchTypeMenu.tsx Lines 263 to 265 in 16ad2bc
|
Ah ok, I got it. However, I think that's small detail that we can address during PR process. |
@hungvu193 I already added @nkdengineer point in my proposal. |
Let's go with @ishakakkad 's proposal. They were the first to propose the correct RCA and solution. 🎀 👀 🎀 C+ reviewed |
Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new. |
@hungvu193 Just found a problem with the current solution. Please check my updated proposal again. |
@nkdengineer @hungvu193 My first solution already fixed that issue and it is more robust. |
I agree that @ishakakkad's proposal is the simplest. I think instead of just using status, we should compare hashes though since that means a new search was made and we need to reset the offset then. |
@luacmartins I have made changes but I am getting issue in Android configuration. Currently, I am fixing this, as soon as I fixed it I will create the PR. |
Thanks! Feel free to create the PR as draft and link to this issue. It helps for others to see the progress on it. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.89-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2025-02-04. 🎊 For reference, here are some details about the assignees on this issue:
|
@hungvu193 @trjExpensify @hungvu193 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button] |
BugZero Checklist:
Bug classificationSource of bug:
Where bug was reported:
Who reported the bug:
Regression Test ProposalPrecondition:User should have at least 50 reports and at least 1 draft report Test:
Do we agree 👍 or 👎 |
For the test steps, we need to make sure that there are draft reports, otherwise no draft reports will show up. |
Ah cool. I updated the steps |
Payment Summary
BugZero Checklist (@trjExpensify)
|
Regression test request created. Payment summary as follows:
|
Paid @ishakakkad, closing! |
$250 approved for @hungvu193 |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number:
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: No
Email or phone of affected tester (no customers): customer
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @trjExpensify
Slack conversation (hyperlinked to channel name): #migrate
Action Performed:
This was a migrated user, so the
open
draft reports weren't created in NewDotopen
report in OldDotDrafts
status filterExpected Result:
The draft report should have appeared.
Actual Result:
The empty state page appears.
From @luacmartins: "Something weird happened, as I can see we sent OFFSET 100 so it seems like App thought the user was on the 3rd page of results for drafts, which is why it didn't return the single report that matched it"
Workaround:
Use OldDot
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @trjExpensifyThe text was updated successfully, but these errors were encountered: