Expensify
[$500] IOU - New green line appears when delete second IOU
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 https://github.com/Expensify/App/pull/34011
Action Performed:
- Go to FAB> Request money>
- Request money from a user that has no existing chat
- Click on + button> Request Money> Complete the IOU flow
- Delete the last created IOU
- Navigate back to the conversation
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?
- [x] Android: Native
- [x] Android: mWeb Chrome
- [x] iOS: Native
- [ ] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [ ] MacOS: Desktop
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/fd6caafc-e53b-415f-a496-afbcb174122c
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~01aa6428b2af1b32c9
- Upwork Job ID: 1750655802310635520
- Last Price Increase: 2024-01-25
Job added to Upwork: https://www.upwork.com/jobs/~01aa6428b2af1b32c9
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External)
Proposal
Please 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 created field of the REPORTPREVIEW report action is not set correctly.
When we create a money request here, the created of the REPORTPREVIEW will be updated to current timestamp, this is because we want the REPORTPREVIEW to show up as latest message in the main chat report, so the user will notice.
So let's say the main chat has:
1 REPORTPREVIEW: At Jan 24 (initial money request is at Jan 24)
1 comment "What's up": At Jan 25
After the user creates a new money request on Jan 26, the REPORTPREVIEW will jump to Jan 26 and become the latest message. This is correct:
1 comment "What's up": At Jan 25
1 REPORTPREVIEW: At Jan 26
But when deleting a money request that we just created, we don't do anything to the REPORTPREVIEW report action's created time, see here, we revert everything to the previous value before that money request is created, but the created field remains the same.
So after deletion, it will still look like:
1 comment "What's up": At Jan 25
1 REPORTPREVIEW: At Jan 26
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 created field that is of current timestamp, as if the REPORTPREVIEW was recently created. That timestamp is larger than the last read time of the chat report, so the new green line shows.
What changes do you think we should make in order to solve the problem?
When deleting a money request, we need to revert the created of the report preview to the created time of the latest money request (amongst the remaining ones) in the IOU report. This needs to be done in optimistic data and in back-end as well.
There're some fields in the iouReport that might need to be reverted to that timestamp as well, like lastVisibleActionCreated.
What alternative solutions did you explore? (Optional)
NA
Proposal
Please 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 shouldDisplayNewMarker which is a combination of a lot of conditions.
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
_.isEqual(prevProps.report.isDeletedParentAction, nextProps.report.isDeletedParentAction)
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 https://github.com/Expensify/App/issues/34190 as well?
Gonna put this in #vip-vsp then. @jjcoffee, @dukenv0307 and @cjoshi-zeals - are sure this shouldn't be a BE change like https://github.com/Expensify/App/issues/34190 as well?
@twisterdotcom Yes i am pretty sure it wont require any BE changes.
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).
the back-end will send back a created field that is of current timestamp, as if the REPORTPREVIEW was recently created. That timestamp is larger than the last read time of the chat report, so the new green line shows.
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.
:ribbon::eyes::ribbon: 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 created field to reflect the correct timestamp of the last IOU request before deletion.
I'll handle the backend changes and the frontend changes it'll be easier for me to handle both. @dukenv0307 your proposal LGTM to me thanks, you should have 25% of the reward (cc @twisterdotcom).
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.
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...
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. Also the majority of the changes are backend related. They are not just one line changes and they also include tests as well.
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!
ideally the reportPreview should move back up in the chat to its original location
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"?
How did we determine that this is the expected behavior?
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.
So after deletion, it will still look like: 1 comment "What's up": At Jan 25 1 REPORTPREVIEW: At Jan 26
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.
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"?
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)
@twisterdotcom I was only assigned here for payment as I provided a proposal. Ref here: https://github.com/Expensify/App/issues/35211#issuecomment-1918762559. @lakchote raises the PR to fix this.
Of course! I forgot about this issue. Okay, yeah I see we discussed this here: https://github.com/Expensify/App/issues/35211#issuecomment-1916732326. Sorry for the bump. Now... @lakchote! How we doing??
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??
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 created action timestamp (further internal discussion ).
It has been handled internally in wave 5 with a different solution (see here. So for this reason, I don't think you should be eligible for payment on this one @dukenv0307? I let @twisterdotcom handle this.
@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!