Open lanitochka17 opened 1 month ago
Triggered auto assignment to @sakluger (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
@sakluger FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
We think that this bug might be related to #wave-collect - Release 1
Job added to Upwork: https://www.upwork.com/jobs/~0126f16401c1dcd30e
Triggered auto assignment to Contributor-plus team member for initial proposal review - @hoangzinh (External
)
Issue Report: Expense on Hold is Displayed as a Single Expense when Employee Holds it a Second Time on Expensify
Summary:
When an employee puts an expense on hold for the second time, it is displayed as a single expense on Expensify, rather than showing the hold history. This can lead to confusion and make it difficult to track the expense's status.
Steps to Reproduce:
An employee submits an expense report with an expense. The expense is put on hold by the manager or administrator. The employee makes changes to the expense and resubmits it. The manager or administrator puts the expense on hold again. Expected Result:
The expense should display a hold history, showing that it was previously held and the reasons for the hold.
Actual Result:
The expense is displayed as a single expense, with no indication that it was previously held.
Impact:
This issue can cause confusion and make it difficult to track the status of expenses. It may also lead to delays in expense approval and reimbursement.
Proposed Solution:
To resolve this issue, we recommend updating the Expensify platform to display the hold history for expenses that have been put on hold multiple times. This could be achieved by:
Adding a "Hold History" section to the expense details page, which displays a list of all holds, including the date, reason, and user who initiated the hold. Updating the expense list view to display a "Hold" indicator, which shows the number of times the expense has been held. Estimated Development Time:
We estimate X hours to implement this solution and update the Expensify platform to display hold history for expenses.
Priority:
We consider this issue a medium priority, as it affects the usability and functionality of the Expensify platform.
Please let us know if you would like to discuss this issue further or proceed with implementation.
Hi @kabeer95, can you follow the PROPOSAL_TEMPLATE and also provide a root cause of this issue? Thank you.
Problem Statement: When an employee puts an expense on hold for the second time, it is displayed as a single expense instead of showing the correct hold status.
Root Cause: The current implementation of the hold feature does not correctly update the expense status when an employee puts an expense on hold multiple times.
Changes to Solve the Problem:
updateExpenses
MethodUpdate the updateExpenses
method to handle the case where an expense is already on hold. Specifically, add a check to ensure that the expense is not already on hold before updating it.
const updateExpenses = (expenses, updatedExpense) => {
setExpenses(expenses.map((expense) => {
if (expense.id === updatedExpense.id && !expense.isHold) {
return updatedExpense;
}
return expense;
}));
};
Update the expense status to reflect the correct hold status when an employee puts an expense on hold multiple times.
if (expense.isOnHold) {
expense.holdCount = (expense.holdCount || 0) + 1;
} else {
expense.holdCount = 0;
}
return expense;
};
Display accurate hold information, including the number of times an expense has been put on hold, in the expense list.
if (expense.holdCount > 1) {
return On hold (${expense.holdCount} times)`;
} else if (expense.holdCount === 1) {
return 'On hold';
} else {`
return 'Not on hold';
}
};
Use a Separate Hold Table: Instead of updating the expense status, we could use a separate hold table to store hold information, including the number of times an expense has been put on hold. This approach may be more scalable and flexible. Implement a Hold History: We could implement a hold history feature that displays a list of all hold actions taken on an expense, including the date and time of each hold action. This approach may provide more transparency and accountability.
@hoangzinh it's time first time trying to contribute to the Expensify issues , kindly avoid the basic mistakes
Sure @kabeer95. Can you follow the proposal format here again, please https://github.com/Expensify/App/blob/main/contributingGuides/PROPOSAL_TEMPLATE.md? This is an example of a proposal that follows the format look like https://github.com/Expensify/App/issues/44153#issuecomment-2183075314.
The current implementation of the hold feature does not correctly update the expense status when an employee puts an expense on hold multiple times.
Moreover, can you elaborate on what is not correct about current implementation of hold features? (link to existing LOC/method and explain which lines or points are not correct).
Update the updateExpenses method
I couldn't find out the existing method name updateExpenses
in codebase, can you link me to that method? Thank you.
Unable to reproduce
@sakluger, @hoangzinh Whoops! This issue is 2 days overdue. Let's get this updated quick!
awaiting for proposals
๐ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐ธ
@sakluger, @hoangzinh Eep! 4 days overdue now. Issues have feelings too...
We're still waiting for more proposals.
Still waiting for proposals.
๐ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐ธ
@sakluger Huh... This is 4 days overdue. Who can take care of this?
I changed the title in case it was making people think that the GH issue was on hold ๐ . Maybe we'll get a few proposals now?
Edited by proposal-police: This proposal was edited at 2024-08-21 11:14:45 UTC.
We want the full report to be displayed when the user puts an expense on hold. Currently when two expenses are shown in the report view and the older one is put on hold, the latest one gets truncated.
When navigating back after completing the hold form, the user is navigated to a route where the aforementioned older expense is set to be the linkedAction. The reportActionID of that expense will be stored in the reportActionID variable within the component. The navigation back triggers any react effects using the route variable as a dependency
useLayoutEffect(() => {
setCurrentReportActionID('');
}, [route]);
// Change the list ID only for comment linking to get the positioning right
const listID = useMemo(() => {
if (!reportActionID && !prevReportActionID) {
// Keep the old list ID since we're not in the Comment Linking flow
return listOldID;
}
isFirstLinkedActionRender.current = true;
const newID = generateNewRandomInt(listOldID, 1, Number.MAX_SAFE_INTEGER);
// eslint-disable-next-line react-compiler/react-compiler
listOldID = newID;
return newID;
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [route, reportActionID]);
Calling setCurrentReportActionID in our useLayoutEffect schedules a re-render
That combined with the value of isFirstedLinkedActionRender.current being set to true means the reportActions list gets sliced starting at the index of this expense. src/pages/home/report/ReportActionsView.tsx
const indexOfLinkedAction = useMemo(() => {
if (!reportActionID) {
return -1;
}
return combinedReportActions.findIndex((obj) => String(obj.reportActionID) ===
String(isFirstLinkedActionRender.current ? reportActionID : currentReportActionID));
}, [combinedReportActions, currentReportActionID, reportActionID]);
The problem lies in the fact that when we have 2 report actions, the onStartReached function of our ReportActionView's Flatlist isn't triggered as it would be in other cases.
The trigger of this function is necessary for loading the newer messages after the linkedAction has been positioned at the bottom of the scroll.
We can replace these lines of code src/pages/home/report/ReportActionsView.tsx
useLayoutEffect(() => {
setCurrentReportActionID('');
}, [route]);
// Change the list ID only for comment linking to get the positioning right
const listID = useMemo(() => {
if (!reportActionID && !prevReportActionID) {
// Keep the old list ID since we're not in the Comment Linking flow
return listOldID;
}
isFirstLinkedActionRender.current = true;
const newID = generateNewRandomInt(listOldID, 1, Number.MAX_SAFE_INTEGER);
// eslint-disable-next-line react-compiler/react-compiler
listOldID = newID;
return newID;
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [route, reportActionID]);
Reading the logs, the order of operations appears to be out of sync with our expected end result. In the case of 3 or greater linkedActions we re-render an additional time and cannot see the effect of this.
https://github.com/user-attachments/assets/78774086-7ece-44cf-816a-cc28af7f5dd5
As show above But when we have just 2 linkedActions we do not trigger the re-render that would cause our onStartReached function to trigger. Denoted by the Handling Pagination log.
https://github.com/user-attachments/assets/6f4de829-e538-412d-8bb1-f44835246c50
We can instead modify the code to the below statement, where we can explicitly define the order of operations that will position our content and then load newer content below it.
const [listID, setListID] = useState(listOldID)
// Change the list ID only for comment linking to get the positioning right
useLayoutEffect(() => {
console.log("useLog", "Setting currentReportActionID to empty string")
setCurrentReportActionID('');
if (!reportActionID && !prevReportActionID) {
// Keep the old list ID since we're not in the Comment Linking flow
setListID(listOldID);
}
console.log("useLog", "Changing isFirstLinkedActionRender to true")
isFirstLinkedActionRender.current = true;
const newID = generateNewRandomInt(listOldID, 1, Number.MAX_SAFE_INTEGER);
// eslint-disable-next-line react-compiler/react-compiler
listOldID = newID;
setListID(newID)
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [route]);
Now when reproducing the problem This is the result with 3 or more linked actions
https://github.com/user-attachments/assets/a4f6ba7b-5f28-4406-baee-19360947dd72
And this is the result with two linked actions
https://github.com/user-attachments/assets/63018c51-3fda-44b0-a706-fcdd85bcc9d4
I would also consider renaming indexOfLinkedAction to something more along the lines of indexOfCurrentAction for better clarity. Because it does not always point to the linkedAction but does always point to the "current action"
Here is the test branch with those changes
If we wanted the list to be truncated before we hold the expense, we could prevent loadNewChats, which is essentially *onStartReached** from being triggered on our first render. src/pages/home/report/ReportActionsView.tsx
const loadNewerChats = useCallback(
(force = false) => {
if (!force && isFirstLinkedActionRender.current) {
isFirstLinkedActionRender.current = false
return
}
Here is a test branch with those changes
@sakluger somehow I have been unassigned. Can you help add me back? Thank you.
@Zakpak0 Thanks for your proposal. Reg. the RCA, your direction is correct, but I don't think it's the root cause. Also, your solution might cause a regression when we open expense in the search page
https://github.com/user-attachments/assets/3cef7ec7-e021-4418-9e1e-65d7a1271c3f
@Zakpak0 Thanks for your proposal. Reg. the RCA, your direction is correct, but I don't think it's the root cause. Also, your solution might cause a regression when we open expense in the search page
Hi @hoangzinh I'm getting down to the real root of the issue now. In the reproduction, there is a step where the tester clicks on the link to the report via the header of the opened expense. Clicking that hyperlink navigations us back to the report, yes, but with the reportActionID of this expense appended to the url. I demonstrate that in my below video. So this leaves me with two possible alternative solutions.
- The reportActionID should not be appended to the url because we wanted to be able to see the full list of reportActions when clicking that link.
- The reportActionID should be appended and the list should start at the index of that reportActionID. Meaning the second expense should never be shown to begin with.
Please let me know what direction to go and I can update my proposal based on the correct expected behavior.
https://github.com/user-attachments/assets/7ce7a291-f5d4-4f10-b178-c1529bb92c3f
๐ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐ธ
The reportActionID should be appended and the list should start at the index of that reportActionID. Meaning the second expense should never be shown to begin with.
Hi @Zakpak0, can you elaborate on this one? Basically, I think we should keep reportActionID in the URL, it appears that when user clicks on the link in the header to go back, we want to highlight the linked report action and scroll to it.
๐ฃ @hoangzinh ๐ An offer has been automatically sent to your Upwork account for the Contributor role ๐ Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review ๐งโ๐ป Keep in mind: Code of Conduct | Contributing ๐
Hey @hoangzinh - I'm not sure why I unassigned you, I didn't mean to do that. Sorry about that! I've added you back now ๐
Updated @hoangzinh Here's the breakdown, I also updated my proposal and gave new test branches
The reportActionID should be appended and the list should start at the index of that reportActionID. Meaning the second expense should never be shown to begin with.
Hi @Zakpak0, can you elaborate on this one? Basically, I think we should keep reportActionID in the URL, it appears that when user clicks on the link in the header to go back, we want to highlight the linked report action and scroll to it. Given a reportActionID is at the end of the path
- The reportActionID should be appended and the list should start at the index of that reportActionID. Meaning the second expense should never be shown to begin with. I assume that when an action is linked, the reportAction that came after it should not be shown. But what happens after this is
note: To me this doesn't seem intentional, because why would we have a clause to truncate to list to being with?
Now we can talk about what happens when this is our current state and we hold the expense.
What happens is, our route changes:
isFirstLinkedActionRender is now set to true
const listID = useMemo(() => {
if (!reportActionID) {
// Keep the old list ID since we're not in the Comment Linking flow
return listOldID;
}
isFirstLinkedActionRender.current = true;
const newID = generateNewRandomInt(listOldID, 1, Number.MAX_SAFE_INTEGER);
// eslint-disable-next-line react-compiler/react-compiler
listOldID = newID;
return newID;
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [route, isLoadingInitialReportActions, reportActionID]);
2. **currentReportActionID** is set to be an empty string because of this:
```typescript
useLayoutEffect(() => {
setCurrentReportActionID('');
}, [route]);
wWhile your comment mentions ## Proposal, it does not follow the mandatory template structure, as it lacks several mandatory lines from the proposal template.
Therefore, I need to respond accordingly:
@user Your proposal will be dismissed because you did not follow the proposal template.
No worries @sakluger Thanks for updates @Zakpak0. I will review it today.
Hi @Zakpak0, I tested your solution and it works. But after reviewing your explanation here and also your proposal here, I tried to link between RCA and solution but it appears that your RCA didn't explain well why useLayoutEffect
above cause issue (I'm trying to understand why removing useLayoutEffect
above can fix the issue)
๐ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐ธ
@Zakpak0 let me know if you have any updates on this feedback https://github.com/Expensify/App/issues/45276#issuecomment-2271006611. Thank you
CC @robertjchen @JmillsExpensify for vis
Updated @hoangzinh updated the proposal with a more detailed RCA and a small addition. Thanks for pointing that out!
@hoangzinh can you take a look?
@Zakpak0 can you check if your new updates are still in draft? From my end, it shows that the last edit is August 3rd.
@hoangzinh bump! sorry about that.
Are we good to proceed here? Let's have @robertjchen do the secondary proposal review as he has context.
@trjExpensify Nope, I'm still reviewing proposals. @robertjchen can you self-assign to this issue? So Melvin won't assign to another internal engineer
Thanks for the head's up, I'll review this
Hi @Zakpak0 Thanks for the updates. I understanded your solution, but I'm still not convinced it's a root cause. Moreover, I tested your solution again, but it caused another regression bug.
Steps to reproduce:
https://github.com/user-attachments/assets/bb4a309b-4aeb-4ff5-8696-ea590053cfe5
@robertjchen do you agree with @hoangzinh's review here?
Yes, I think we need some more discussion/exploration here
@Zakpak0 it sounds like your latest proposal still causes regression. Have you had a chance to take another look?
@robertjchen, @sakluger, @hoangzinh Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Commenting again for Melvin.
@sakluger had a bit going on for me this week. I'll prioritize first thing tomorrow
Thanks @Zakpak0 ! Let us know if there's anything we can help with
If you havenโt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 9.0.6-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4706804&group_by=cases:section_id&group_order=asc&group_id=309128 Email or phone of affected tester (no customers): gocemate+workspaceemployee135@gmail.com Issue reported by: Applause - Internal Team
Action Performed:
Preconditions:
Expected Result:
User should remain on expenses page and both expenses (expense on hold and other expense) should be visible
Actual Result:
Expense on hold is displayed as single expense when employee hold it second time
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/7ef247bd-c23b-4f50-823d-4efbe96acd38
https://github.com/Expensify/App/assets/78819774/18dfee41-38f6-41e9-95f5-8e6c04b92a9b
View all open jobs on GitHub
Upwork Automation - Do Not Edit