Skip to content
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

Closed
4 of 6 tasks
lanitochka17 opened this issue Jan 25, 2024 · 26 comments
Closed
4 of 6 tasks

[$500] IOU - New green line appears when delete second IOU #35211

lanitochka17 opened this issue Jan 25, 2024 · 26 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 25, 2024

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:

  1. Go to FAB> Request money>
  2. Request money from a user that has no existing chat
  3. Click on + button> Request Money> Complete the IOU flow
  4. Delete the last created IOU
  5. 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?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6355252_1706223349466.Recording__1926.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01aa6428b2af1b32c9
  • Upwork Job ID: 1750655802310635520
  • Last Price Increase: 2024-01-25
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 25, 2024
@melvin-bot melvin-bot bot changed the title IOU - New green line appears when delete second IOU [$500] IOU - New green line appears when delete second IOU Jan 25, 2024
Copy link

melvin-bot bot commented Jan 25, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01aa6428b2af1b32c9

Copy link

melvin-bot bot commented Jan 25, 2024

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 25, 2024
Copy link

melvin-bot bot commented Jan 25, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External)

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-vsp
CC @quinthar

@dukenv0307
Copy link
Contributor

dukenv0307 commented Jan 26, 2024

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

@cjoshidev
Copy link

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

@twisterdotcom
Copy link
Contributor

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?

@cjoshidev
Copy link

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
Yes i am pretty sure it wont require any BE changes.

@melvin-bot melvin-bot bot added the Overdue label Jan 29, 2024
@jjcoffee
Copy link
Contributor

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.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Jan 29, 2024

Triggered auto assignment to @lakchote, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Copy link

melvin-bot bot commented Jan 29, 2024

@twisterdotcom, @jjcoffee Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@lakchote
Copy link
Contributor

lakchote commented Jan 30, 2024

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).

@lakchote lakchote added the Internal Requires API changes or must be handled by Expensify staff label Jan 30, 2024
Copy link

melvin-bot bot commented Jan 30, 2024

Current assignee @jjcoffee is eligible for the Internal assigner, not assigning anyone new.

@lakchote lakchote removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Jan 30, 2024
@twisterdotcom
Copy link
Contributor

Cool, so @lakchote will push the BE and FE PRs. We'll pay @dukenv0307 $125 for their solution assuming no regression.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 30, 2024
@dukenv0307
Copy link
Contributor

dukenv0307 commented Jan 30, 2024

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...

@lakchote
Copy link
Contributor

lakchote commented Jan 31, 2024

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.

@dukenv0307
Copy link
Contributor

Sure, @lakchote would you mind assigning me to this issue, so later payment can be handled.

Thank you!

@marcaaron
Copy link
Contributor

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"?

@dukenv0307
Copy link
Contributor

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.

@mountiny
Copy link
Contributor

mountiny commented Feb 6, 2024

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
Copy link
Contributor

How is this going @dukenv0307?

@dukenv0307
Copy link
Contributor

dukenv0307 commented Feb 29, 2024

@twisterdotcom I was only assigned here for payment as I provided a proposal. Ref here: #35211 (comment). @lakchote raises the PR to fix this.

@twisterdotcom
Copy link
Contributor

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??

@dukenv0307
Copy link
Contributor

No problem.

@lakchote
Copy link
Contributor

lakchote commented Mar 4, 2024

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.

@twisterdotcom
Copy link
Contributor

@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!

@github-project-automation github-project-automation bot moved this from LOW to CRITICAL in [#whatsnext] #vip-vsb Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Status: CRITICAL
Development

No branches or pull requests

8 participants