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

[$250] Dupe detect - Confirmation page with empty data opens when opening invalid dupe confirm link #45835

Closed
6 tasks done
lanitochka17 opened this issue Jul 20, 2024 · 28 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 20, 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: 9.0.10-2
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

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace chat
  3. Submit two same expenses (same distance)
  4. Go to transactiont thread of any expense
  5. Click Review duplicates
  6. Click Keep this one
  7. Proceed to confirmation page
  8. Copy the link
  9. Modify the report ID in the link and navigate to it
  10. Click Confirm

Expected Result:

In Step 9, app should open not here RHP when navigating to dupe review confirmation page of invalid report

Actual Result:

In Step 9, app opens confirmation page with empty data when navigating to invalid dupe confirmation link
In Step 10, when clicking Confirm, not here page opens

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

Bug6547833_1721456233030.20240720_141358.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01437c46405734d3fc
  • Upwork Job ID: 1815459015971079692
  • Last Price Increase: 2024-07-22
  • Automatic offers:
    • gijoe0295 | Contributor | 103220593
@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Jul 20, 2024
@github-actions github-actions bot added the Hourly KSv2 label Jul 20, 2024
Copy link

melvin-bot bot commented Jul 20, 2024

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

Copy link
Contributor

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

@gijoe0295
Copy link
Contributor

gijoe0295 commented Jul 20, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

App does not show not found page when the report ID is incorrect.

What is the root cause of that problem?

We do not handle the case of incorrect report ID or transaction in Confirmation page.

What changes do you think we should make in order to solve the problem?

Wrap the page with FullPageNotFoundView whose shouldShow condition is the existence of the transaction ID just like we did with Review page:

<FullPageNotFoundView shouldShow={transactionID === '-1'}>

Note that the transaction ID existence only can cover the report existence. That means we could never retrieve a transaction when it is not linked to any report.

@mountiny
Copy link
Contributor

@mountiny mountiny removed the DeployBlocker Indicates it should block deploying the API label Jul 20, 2024
@mountiny
Copy link
Contributor

The proposal seems quite straightforward, @parasharrajat should we go with it?

@mountiny
Copy link
Contributor

Dupe detection is behind beta so we can remove the blocker

@mountiny mountiny added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Jul 21, 2024
@parasharrajat
Copy link
Member

parasharrajat commented Jul 21, 2024

@mountiny FYI, I will get all of these issues by tomorrow.

@mountiny
Copy link
Contributor

@parasharrajat Thanks! I was not aware this is behind beta, since it is, we do not have to treat these as blockers which helps

@parasharrajat
Copy link
Member

@gijoe0295's proposal makes sense to me.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Jul 22, 2024

Current assignee @stitesExpensify is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@gijoe0295
Copy link
Contributor

@stitesExpensify We're missing External label.

@stitesExpensify stitesExpensify added the External Added to denote the issue can be worked on by a contributor label Jul 22, 2024
Copy link

melvin-bot bot commented Jul 22, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01437c46405734d3fc

@melvin-bot melvin-bot bot changed the title Dupe detect - Confirmation page with empty data opens when opening invalid dupe confirm link [$250] Dupe detect - Confirmation page with empty data opens when opening invalid dupe confirm link Jul 22, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 22, 2024
Copy link

melvin-bot bot commented Jul 22, 2024

Current assignee @parasharrajat is eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 22, 2024
Copy link

melvin-bot bot commented Jul 22, 2024

📣 @gijoe0295 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@gijoe0295
Copy link
Contributor

I spotted several issues and working on them. Expect to finish in several hours.

@parasharrajat
Copy link
Member

@gijoe0295 Could you please post summary of issues you faced?

@gijoe0295
Copy link
Contributor

gijoe0295 commented Jul 24, 2024

The REVIEW_DUPLICATES Onyx entry takes a noticeably long time to load. We don't know if it is still loading or actually undefined. We also couldn't count only on either report or transaction data. I think the final condition to show not found page is:

  • REVIEW_DUPLICATES data is ready (maybe use useOnyx's status metadata)
  • REPORT data is not found
  • transactionID is undefined

Let me test this further.

@parasharrajat
Copy link
Member

Yes, you can do something like this

const [policies, fetchStatus] = useOnyx(ONYXKEYS.COLLECTION.POLICY);

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jul 25, 2024
Copy link

melvin-bot bot commented Jul 25, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Aug 19, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

This issue has not been updated in over 15 days. @parasharrajat, @stitesExpensify, @gijoe0295 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!

@pecanoro pecanoro added Daily KSv2 and removed Monthly KSv2 labels Aug 19, 2024
@parasharrajat
Copy link
Member

PR merged. Not deployed to prod yet.

Copy link

melvin-bot bot commented Aug 27, 2024

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

@parasharrajat
Copy link
Member

This should be ready to be paid today. We need a BZ person here @stitesExpensify

@stitesExpensify stitesExpensify added the Bug Something is broken. Auto assigns a BugZero manager. label Aug 27, 2024
Copy link

melvin-bot bot commented Aug 27, 2024

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@parasharrajat
Copy link
Member

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

Regression Test Steps

For Web & mWeb:

  1. Submit 2 duplicated expenses
  2. Press Review duplicates >> Keep this one (any)
  3. Proceed to confirmation page
  4. Modify the report ID in the URL to anything and navigate to it
  5. Verify not found page appears

Do you agree 👍 or 👎 ?

@puneetlath
Copy link
Contributor

Payment summary:

Thanks everyone!

@parasharrajat
Copy link
Member

Payment requested as per #45835 (comment)

@JmillsExpensify
Copy link

$250 approved for @parasharrajat

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. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
Status: Done
Development

No branches or pull requests

8 participants