-
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
[$250] LHN -When deleting expense on search, information doesn´t disappear from chat preview on LHN #53993
Comments
Triggered auto assignment to @johncschuster ( |
I haven't gotten to dig into this one |
@johncschuster Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Job added to Upwork: https://www.upwork.com/jobs/~021870237400147450769 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ZhenjaHorbach ( |
I am here from Upwork, I can see the email contains +91 before @. Usually these kind of emails are being used to create multiple accounts with same email address. For example, if my email is [email protected], then I can create multiple accounts with emails like [email protected], [email protected], and this is because I will receive any OTP or confirmation email at my original email, despite having these extra characters in given email. What I can feel that your logic is dealing with original email, instead of this one (+91@gmail), can you please try this with a normal gmail address? Without any “+” thing? |
📣 @shamzahasan88! 📣
|
Hey, I have worked on this issue and identified root cause of the issue and fixed it. Please add me as a contributor so that I can post my Proposal: Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
ProposalPlease re-state the problem that we are trying to solve in this issue.LHN -When deleting expense on search, information doesn´t disappear from chat preview on LHN What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?On clicking 'Search' icon on top bar, it does not make call to search api because we have empty search query text. function searchInServer(searchInput: string, policyID?: string) {
if (isNetworkOffline || !searchInput.trim().length) {
Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, false);
return;
}
// Why not set this in optimistic data? It won't run until the API request happens and while the API request is debounced
// we want to show the loading state right away. Otherwise, we will see a flashing UI where the client options are sorted and
// tell the user there are no options, then we start searching, and tell them there are no options again.
Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, true);
searchForReports(searchInput, policyID);
} So, final solution will work seamless and always bring updated data which will not show expense amount in this case. if (isNetworkOffline) {
Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, false);
return;
} What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?Nothing What alternative solutions did you explore? (Optional)Another solution might be to manually call an action to update onyx 'personalDetailsList' by removing expense from the selected user. But it could be less efficient. |
@shamzahasan88 @email4shahbaz |
@ZhenjaHorbach Issue.53993.mp4Here are the steps to follow to reproduce issue and see it's solution: 1-I create a new expesce by clicking + button and entere '999' as amount. Then I selected '[email protected]' and completed expense creation. 2-I clicked 'Search' icon on top right to see list of all chats, my newly created chat shows '[email protected] owes PKR 999.' 3-I selected that new created expense and removed it. 4-I again went to search, it still show that old values with PKR 999 (just because this search list is not being updated by any action) 5-To prove that we need to update search results in order to see those expense values updated, I entered one character in the search textfield, it immediately updated search results and removed 'RS.999' from that record. So, that proves that in order to see updated records in search, we must call search api immediately without entering any search query text. For this to work, we already have a useEffect in SearchRouterList.tsx component as follows:
Above code already make api call upon we click 'Search' icon on top right. So if we make my proposed change in Report.ts to ignoring 'searchInput.trim().length', it will always show an updated search result and will not show expense values which is already removed. Thanks and let me know your thoughts. |
@email4shahbaz |
sure, give me few minutes. |
Hi @ZhenjaHorbach, Please re-state the problem that we are trying to solve in this issue.I’ve revisited my earlier proposal and realized that while it is valid, it addresses a different issue related to the functionality triggered by clicking the Search button on the top-right corner of the screen. Let’s now focus on the actual issue, which can be reproduced by following the QA steps or watching the video attached to the issue. What is the root cause of that problem?Inbox items are loaded using the Onyx key personalDetailsList. The expense value text is dynamically appended based on the Onyx-loaded collection of reports. Each report contains lastMessageHtml and lastMessageText fields, which remain unchanged even when an expense is removed. ExampleFor instance, a chat might display the following text: This text is calculated and appended from the API response when a new expense is added. However, if an expense is removed and the total value of a report becomes 0, this text is not updated accordingly.
What changes do you think we should make in order to solve the problem?I propose modifying the handleDeleteExpenses function in src/components/Search/searchPageHeader.tsx to address this issue. Specifically, the changes will include: Checking for Zero Total:Add logic to detect when all expenses are removed, and the total value of the report is 0. Updating Report Fields:Dynamically update the lastMessageHtml and lastMessageText fields in the relevant report within the Onyx reports collection. Syncing with Onyx:Ensure these updates are synchronized with Onyx so the changes are reflected in the UI. What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?This requires a UI test where we need to assert that the lastMessageHtml and lastMessageText of chat item is updated when all the expenses are deleted for a specific chat item. What alternative solutions did you explore? (Optional)Alternatively, I suggest enhancing the getLastMessageTextForReport function in src/libs/OptionsListUtils.ts, which is triggered during re-renders. This function fetches the last message text from the report or special cases based on specific conditions. Since it has access to the variable lastVisibleReportActions, I propose adding logic to check: If the last report action contains a message with the deleted key, and
Note:Of course, we will replace that plain text message with a localized constant for better consistency and adaptability. OutcomeI tested my alternative solution locally, and it worked as expected. The chat list screen dynamically updated the expense values, effectively resolving the inconsistency when expenses were removed. Let me know if you have any suggestions or concerns. Thanks! |
@johncschuster @ZhenjaHorbach this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
@johncschuster, @ZhenjaHorbach Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@email4shahbaz |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@ZhenjaHorbach I recommend reviewing my alternative solution first, as it is more elegant and practical. |
Issue not reproducible during KI retests. (First week) |
@johncschuster, @ZhenjaHorbach Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Still reproducible @email4shahbaz The only thing |
Yes, we can do it from BE too but there are many other conditions are added in getLastMessageTextForReport function in src/libs/OptionsListUtils.ts Also delete report api call from backend is not returning any data in the case of all expenses removed. |
It's true 😅 But let's find out what an internal engineer thinks ! |
Triggered auto assignment to @tylerkaraszewski, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@tylerkaraszewski, @johncschuster, @ZhenjaHorbach Whoops! This issue is 2 days overdue. Let's get this updated quick! |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Are we suggesting that the backend should return empty strings for |
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: 9.0.75-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5335362&group_by=cases:section_id&group_order=asc&group_id=296775
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: Motorola MotoG60 - Android 12 - Chrome / Windows 10 - Chrome
App Component: Left Hand Navigation (LHN)
Action Performed:
Expected Result:
When an expense is deleted on search, the preview of the chat where this expense was submitted, should not display the expense information anymore.
Actual Result:
Expense information is still displayed on chat preview on LHN, after the expense was deleted on search section.
Issue reproduced only with Gmail account
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6691822_1733984604638.LHN_Preview.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @ZhenjaHorbachThe text was updated successfully, but these errors were encountered: