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

IOU - Comment on a deleted IOU isn't opening from the combined report view #39512

Closed
1 of 6 tasks
lanitochka17 opened this issue Apr 3, 2024 · 16 comments
Closed
1 of 6 tasks
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Monthly KSv2 Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Apr 3, 2024

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: 1.4.59.3
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Issue found when executing PR #39433

Action Performed:

  1. Create a manual IOU with an account you never had contact before
  2. Create a second manual IOU
  3. Open the first IOU
  4. Add some comments
  5. Delete the first IOU
  6. Navigate to the combined report view
  7. Click on "Replies"

Expected Result:

I should be able to view comments

Actual Result:

Comment on a deleted IOU isn't opening from the combined report view after deleting the first 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

Bug6436784_1712153486741.bandicam_2024-04-03_16-02-31-958.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Apr 3, 2024
Copy link

melvin-bot bot commented Apr 3, 2024

Triggered auto assignment to @tgolen (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link
Contributor

github-actions bot commented Apr 3, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@lanitochka17
Copy link
Author

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

@tgolen
Copy link
Contributor

tgolen commented Apr 3, 2024

Yeah, I do think this is a bug. Before going external with it, there are a couple of suspect PRs on the deploy list which might have caused this.

@NikkiWines @bernhardoj Could this be a regression from #39419?
@srikarparsi @pac-guerreiro Could this be a regression from #35253?

@bernhardoj
Copy link
Contributor

#39419 only touches the receipt attachment modal page, so it shouldn't cause this issue.

@srikarparsi
Copy link
Contributor

srikarparsi commented Apr 3, 2024

Still checking if this has to do something with the TS migration but wanted to say that I don't think the issue is related to commenting. The expense report level page is missing in staging for reports with single expenses (this could be expected behavior but it's not the behavior in production). And that's why in this bug, clicking replies takes you directly to the transaction thread instead of the expense report.

Production (Chat -> Expense Report -> Transaction Thread):
https://github.com/Expensify/App/assets/48188732/8d0c1a07-91e4-4181-acc7-c78b2343f916

Staging (Chat -> Transaction Thread):
https://github.com/Expensify/App/assets/48188732/1b978870-a954-4508-8c53-07fa33099c4d

I'm guessing that we recently made a change to directly go to the transaction thread when clicking on a report if the report only has one expense.

I'll try looking at any other PRs that might've caused this and also try rolling back the TS migration but I don't think that it caused this.

@srikarparsi
Copy link
Contributor

Ah, looks like a PR for single transaction pages does exist. @NikkiWines do you think this could be a regression from that?

@tgolen
Copy link
Contributor

tgolen commented Apr 3, 2024

Thanks @srikarparsi if we can't find this one quickly, I'll make sure I can reproduce it and then use git bisect to find out exactly what broke it. I'll be able to do that in about 2 hours as I'm in a meeting currently.

@NikkiWines
Copy link
Contributor

Sorry for the delay! It does look like it's likely tied to the one-transaction changes - though I'm not sure what the expected behavior is for comments on a request that no longer exists.

I'll double-check how we handle it on prod

@pac-guerreiro
Copy link
Contributor

pac-guerreiro commented Apr 3, 2024

@NikkiWines I reverted your PR locally and the issue got resolved 😅

This is the previous behaviour without your changes:

Screen.Recording.2024-04-03.at.19.06.40.mp4

The deleted report should still show up on the list, with a message saying that it was deleted

@NikkiWines
Copy link
Contributor

NikkiWines commented Apr 3, 2024

Oh interesting, because we keep it on the report 🤔 Thanks for checking that out @pac-guerreiro

I wonder if then we should just not consider it a one-transaction report if there's a previously deleted request... I'm not sure how we'd show the deleted request in a reasonable way otherwise

What do you all think?

@tgolen tgolen assigned NikkiWines and unassigned tgolen Apr 3, 2024
@tgolen
Copy link
Contributor

tgolen commented Apr 3, 2024

I'm going to assign this to you for now @NikkiWines.

As for the behavior, I think the situation where there is a deleted request would indicate it couldn't be a one-transaction report.

@NikkiWines
Copy link
Contributor

Cool, I think that makes sense. I'm makng a follow up PR to handle some of these edge-cases and bugs we've found for this new view, so I'm gonna mark this as a non-blocker and handle it in that follow up PR.

@NikkiWines NikkiWines added Daily KSv2 Improvement Item broken or needs improvement. and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Apr 3, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 5, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review and removed Daily KSv2 labels Apr 6, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Overdue labels Apr 6, 2024
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Apr 29, 2024
Copy link

melvin-bot bot commented Apr 29, 2024

This issue has not been updated in over 15 days. @NikkiWines eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

Copy link

melvin-bot bot commented Jun 26, 2024

@NikkiWines, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

@NikkiWines
Copy link
Contributor

Oops, this was actually handled as part of #39472 but the issue was never closed 😅 regardless, this one has been successfully resolved 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

6 participants