-
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
[$500] IOU - New green line appears when delete second IOU #35211
Comments
Job added to Upwork: https://www.upwork.com/jobs/~01aa6428b2af1b32c9 |
Triggered auto assignment to @twisterdotcom ( |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee ( |
We think that this bug might be related to #vip-vsp |
ProposalPlease re-state the problem that we are trying to solve in this issue.New green line appears when delete second IOU What is the root cause of that problem?When we delete the money request, the When we create a money request here, the So let's say the main chat has: After the user creates a new money request on Jan 26, the 1 comment "What's up": At Jan 25 But when deleting a money request that we just created, we don't do anything to the So after deletion, it will still look like: Which is wrong, because now when the user clicks on the report preview, they will only see 1 initial money request from Jan 24, and have no idea why they see a report preview on Jan 26 but in the iou report itself, there's nothing happening on Jan 26. If the user is online, the back-end will send back a What changes do you think we should make in order to solve the problem?When deleting a money request, we need to revert the There're some fields in the What alternative solutions did you explore? (Optional)NA |
ProposalPlease re-state the problem that we are trying to solve in this issue.The new green line appears when deleting second IOU What is the root cause of that problem?For displaying the new line marker we have a prop To not display the green line we need to remove , i have tested and it wont impact any other things in the App and it will fix the issue as well
What changes do you think we should make in order to solve the problem?Need to remove the condition i have specified above to make sure it is fixed What alternative solutions did you explore? (Optional)NA |
Gonna put this in #vip-vsp then. @jjcoffee, @dukenv0307 and @cjoshi-zeals - are sure this shouldn't be a BE change like #34190 as well? |
@twisterdotcom |
This appears to be mostly a BE issue as @dukenv0307 points out in their proposal (the unread indicator also doesn't show if performing the same action offline).
I'd say it's also worth fixing the adjacent issue mentioned in the proposal, where ideally the reportPreview should move back up in the chat to its original location. @cjoshi-zeals Thanks for the proposal, though it could use more detail (see the proposal above for a better example, note that there are links to relevant portions of code that are being referred to). I'm not convinced that removing the condition you mention fixes this issue without causing any regressions. 🎀👀🎀 C+ reviewed |
Triggered auto assignment to @lakchote, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@twisterdotcom, @jjcoffee Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
I do agree with @jjcoffee, it's mostly a BE issue and should not be handled only in frontend. We should definitely update the I'll handle the backend changes and the frontend changes it'll be easier for me to handle both. |
Current assignee @jjcoffee is eligible for the Internal assigner, not assigning anyone new. |
Cool, so @lakchote will push the BE and FE PRs. We'll pay @dukenv0307 $125 for their solution assuming no regression. |
@twisterdotcom Sorry I believe in this case usually the contributor will be raising the front-end PR and gets the full reward 😄 I see @lakchote already raised the PR so maybe we can just stick with that, but I think 50% reward is more fair since both the back-end and front-end use my suggested solutions, just that I was not the one (given the chance to) raising the PR... |
It was easier for me to already make the couple lines frontend changes to confirm the backend fix works as expected. For these reasons and after gathering external feedback on the matter, I still think 25% of the amount is fair. |
Sure, @lakchote would you mind assigning me to this issue, so later payment can be handled. Thank you! |
How did we determine that this is the expected behavior? @JmillsExpensify @mountiny what do you guys think? Should we be moving the report preview component backwards when they are deleted? Or would it move to the bottom because it was "updated"? |
IMO the UX is currently confusing, more explanation below (link to proposal). Also we already do such reverted updated date in other places like LHN when comment is deleted.
|
Nope I dont think we should move it backwards, quite contrary the deletion is "latest" action done to the expense report so the total changes and attention of the user might be desired so moving it to latest makes more sense than backwards. But I think not moving it in this case is also fine (if that is what happens now) |
How is this going @dukenv0307? |
@twisterdotcom I was only assigned here for payment as I provided a proposal. Ref here: #35211 (comment). @lakchote raises the PR to fix this. |
Of course! I forgot about this issue. Okay, yeah I see we discussed this here: #35211 (comment). Sorry for the bump. Now... @lakchote! How we doing?? |
No problem. |
The solution suggested by @dukenv0307 did not fit as I had to revert the PR. See @mountiny comment here, we don't want to revert the It has been handled internally in wave 5 with a different solution (see here. |
@lakchote okay, given the solution we were going to pay for wasn't the solution anyway, I think we should just close. I hope you understand this too @dukenv0307! |
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:**1.4.32-2
**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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Issue found when executing PR #34011
Action Performed:
Expected Result:
Green line should not appears since there is no new message
Actual Result:
New green line appears when delete second IOU
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6355252_1706223349466.Recording__1926.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: