-
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
[$1000] Web - Delete Button Fails to Delete Changed Description in IOU #25997
Comments
Triggered auto assignment to @CortneyOfstad ( |
Bug0 Triage Checklist (Main S/O)
|
Job added to Upwork: https://www.upwork.com/jobs/~01462271c6de352d0a |
Current assignee @CortneyOfstad is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.The Delete button fails to delete the changed description inside the IOU. What is the root cause of that problem?In these lines, we have the logic to check whether we should delete the transaction threads, we'll delete it if there's no visible message. However, we're not excluding the It doesn't make sense to keep all the What changes do you think we should make in order to solve the problem?We need to exclude the Line 1072 in 64ff471
This can be done by, for example, adding a And then we can update this line to
We'll use it to filter out the
Similarly, we need to update the usage of If it's desirable to delete all The back-end will need to be fixed similarly What alternative solutions did you explore? (Optional)There're other ways to do the above besides Details on the
so it won't include the modified expense action if the
The back-end will also need to be fixed to delete the transaction thread in that case. |
ProposalPlease re-state the problem that we are trying to solve in this issue.The Delete request button fails to delete the changed description inside the IOU. What is the root cause of that problem?The transactionThread is deleted if there are not visible comments in thread . incase of ModifiedExpense Lines 1075 to 1078 in cf1d50a
What changes do you think we should make in order to solve the problem?we have to exclude the ModifiedExpense from What alternative solutions did you explore? (Optional)N/A |
I think we should remove the reportActions of I also think deleting the change description actions alone will result in the FE adjusting the report to the desired experience. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Delete Button Fails to Delete Changed Description in IOU What is the root cause of that problem?When request is deleted, the function that filters action with the name "MODIFIEDEXPENSE" is missing.
What changes do you think we should make in order to solve the problem?So, I updated the that code line as shown below and solved this issue simply.
What alternative solutions did you explore? (Optional)N/A |
I'll review the proposals in the morning, thanks! |
@mollfpr Just checking to see if you had time to check the proposals? TIA! |
Hi @CortneyOfstad, I'm in the middle of testing the proposals now. For an update now, @davewish proposal is similar to what @dukenv0307 proposed. , and for @binary3oul
I don't understand what this step has to do with the solution. |
We can ignore that step :) |
@binary3oul I tried your solution, but it deletes the thread regardless of whether it only has a comment. Line 1075 in 5db2445
The thread shouldn't be deleted. |
@dukenv0307 Could you elaborate on the solution for adding |
Sharing my thoughts on this issue. I think we should get a confirmation from the team on whether we should delete the transaction thread or not if the thread only contains MODIFIEDEXPENSE actions because from my testing, looks like BE keep the thread. It would be weird if we optimistically delete the thread on the FE but we actually still have the thread on the BE. How do we know if BE doesn't delete the thread? Assuming we delete the thread, here are the steps I use:
expected: if BE also deletes the thread, we will get not found page Compare the behavior when we delete the transaction thread with empty actions. |
ProposalPlease re-state the problem that we are trying to solve in this issue.The Delete button fails to delete the changed description inside the IOU. What is the root cause of that problem?Because IOU's thread delete check is based on last message of the thread and without checking the type of the message. In this line: Line 1072 in 64ff471
What changes do you think we should make in order to solve the problem?My solution is to use already available function library :
or
What alternative solutions did you explore? (Optional)
|
📣 @tsa321! 📣
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
@CortneyOfstad, @mollfpr, @jasperhuangg, @dukenv0307 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
The PR is still in review. |
@CortneyOfstad, @mollfpr, @jasperhuangg, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@CortneyOfstad, @mollfpr, @jasperhuangg, @dukenv0307 Huh... This is 4 days overdue. Who can take care of this? |
@mollfpr update on the PR? Thank you! |
@CortneyOfstad, @mollfpr, @jasperhuangg, @dukenv0307 Eep! 4 days overdue now. Issues have feelings too... |
The linked PR is still under review. #28995 Some changes have occurred to the associated back-end behavior, so the PR has had to be updated accordingly. |
Thanks @jasperhuangg! |
@CortneyOfstad This specific issue is not reproducible anymore because the thread report is now deleted when it has no chat message. We still show the edit expense message after the request is deleted. I don't know if this was intended, but I remember not showing the message before. |
I asked for clarification on how to proceed, will let you know what the decision is. |
@CortneyOfstad, @mollfpr, @jasperhuangg, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
a PR isn't necessary here anymore, but @dukenv0307 and @mollfpr have spent a considerable amount of time with investigating this issue. Even though we didn't end up merging any code, I think we should move ahead with paying them out for the time they've spent on this. @CortneyOfstad can we be sure that this happens? |
@jasperhuangg I think that would be fair. Do you have a price in mind? Or would you be more comfortable opening it up to the BZ for a broader discussion? |
I already posted here in BZ without any response yet. Definitely think we should see if we can get some more eyes on that thread. |
@jasperhuangg @CortneyOfstad For this case I think the payment should be the full price since we spent a considerable amount of time with investigating this issue. We have some same cases in the part here #27570 (comment) and #30692 (comment) |
For #27570 (comment)
and our internal doc states
This issue is different because we didn't make a decision to fix it on the backend. For #30692 (comment) Part of what leads me to believe 50% is fair is because the hire date was Oct 5 and the PR was still being worked on as of Dec 28th and there were multiple bumps to move the PR along. If this PR would have been merged before the one that fixed it, would it have made a difference? (I don't know). If so, then we could have saved time both working on this one AND the other by closing them both sooner. We appreciate and value the time and investment, our main goal is to get bugs fixed and to do it with urgency, which is what I consider the a benchmark for similar issues like there where another PR (appears to) magically fix the bug (from what I know, I'm not the most well versed on this specific issue or PR) |
@mallenexpensify I believe part of the reason for the delay is this is a fairly complex issue, we faced many technical challenges during the implementation and tried to make sure to resolve all of them (~50 commits) before merging to not cause regressions/affect the app performance. So I'd appreciate if we can keep the full payment here since the PR is mostly ready for merging before it was made outdated due to another change. Also the reason why the other PR was able to resolve the bugs is not because that PR resolves the technical issues we faced, just that they achieved a different expectation that conflicts with the expectations in this issue so this issue is no longer reproducible.
@mallenexpensify regarding this, it will not make a difference since the 2 PRs have 2 conflicting expectations, if this PR is merged first, the other PR would still override the expectation in this PR. |
Thanks @dukenv0307 , we're discussing internally. |
@dukenv0307 @mollfpr Thank you for your patience with this matter. We've discussed this internally and have decided to keep full payment on your work considering the sheer amount of time and effort you've put into implementation and review. Thank you for sticking with it, we really appreciate your hard work! |
@CortneyOfstad can you help with issuing payment here? |
Created a new Job post here — https://www.upwork.com/jobs/~016e3532aa6b8d64fd @dukenv0307 sent you a proposal in Upwork @mollfpr you'll be paid via NewDot — I'll create the proposal once Duc accepts in Upwork 👍 |
@CortneyOfstad I've accepted, thank you! |
@CortneyOfstad @jasperhuangg May I inquire about my eligibility for compensation regarding the bug report? Thank you. |
ah.. yes, @tewodrosGirmaA you are eligible since this was reported forever ago, ha. Amount is $250 because it was before the decrease to $50. . |
Sorry about that @tewodrosGirmaA! I sent you a proposal in Upwork — let me know once you accept and I'll get that paid ASAP. Thanks! |
Hello @CortneyOfstad i accept it , Thank you |
Payment Summary@tewodrosGirmaA (reporter) — paid $250 via Upwork |
Thank you @CortneyOfstad |
$1,000 approved for @mollfpr for based on summary above. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
The Delete button should delete the changed description from the IOU as well.
Actual Result:
The Delete button fails to delete the changed description inside the IOU.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.57-5
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
screen-capture.-.2023-08-17T112708.989.webm
Recording.4040.mp4
Expensify/Expensify Issue URL:
Issue reported by: @tewodrosGirmaA
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692252491955339
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: