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

[HOLD for payment 2023-12-20] IOU - Add receipt header is missing in Add receipt page #32831

Closed
2 of 6 tasks
kbecciv opened this issue Dec 11, 2023 · 23 comments
Closed
2 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering

Comments

@kbecciv
Copy link

kbecciv commented Dec 11, 2023

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: v1.4.11-1
Reproducible in staging?: y
Reproducible in production?: n
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:

Action Performed:

  1. Launch New Expensify app.
  2. Tap FAB > Request money.
  3. Enter amount > Next.
  4. Select a member > Next.
  5. Tap 3-dot menu > Add receipt.

Expected Result:

"Add receipt" header will not disappear.

Actual Result:

"Add receipt" header is missing.

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

Bug6309284_1702312889338.Screen_Recording_20231211_232642_New_Expensify.mp4

View all open jobs on GitHub

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Dec 11, 2023
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.

Copy link

melvin-bot bot commented Dec 11, 2023

Triggered auto assignment to @srikarparsi (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@kbecciv kbecciv changed the title OU - Add receipt header is missing in Add receipt page IOU - Add receipt header is missing in Add receipt page Dec 11, 2023
@bernhardoj
Copy link
Contributor

Proposal

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

There is no Receipt header when we press Add receipt menu item from the confirmation page, only on native.

What is the root cause of that problem?

We have a native platform file for the add receipt page where we only use a View to wrap the page and we don't have a header component to show the title.

return (
<View style={styles.flex1}>

In index.js file, we wrap it with StepScreenDragAndDropWrapper and show the header if backTo param is exist, which is true for add receipt case.

return (
<StepScreenDragAndDropWrapper
headerTitle={translate('common.receipt')}
onBackButtonPress={navigateBack}
shouldShowWrapper={Boolean(backTo)}
testID={IOURequestStepScan.displayName}
>

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

Native doesn't support drag and drop, so we can just wrap it with StepScreenWrapper.

const navigateBack = () => {
    Navigation.goBack(backTo || ROUTES.HOME);
};

<StepScreenWrapper 
    headerTitle={translate('common.receipt')}
    onBackButtonPress={navigateBack}
    shouldShowWrapper={Boolean(backTo)}
    testID={IOURequestStepScan.displayName}
>

@ishpaul777
Copy link
Contributor

ishpaul777 commented Dec 11, 2023

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

There is no Receipt header when we press Add receipt menu item from the confirmation page, only on native.

What is the root cause of that problem?

we don't have a header component to show the title in src/pages/iou/request/step/IOURequestStepScan/index.native.js because the IOURequestStepScan is also used as embeded component within other page.

Changes Suggested

while we can wrap it with StepScreenWrapper and set shouldShowWrapper false when back to is not passed in url, or if we use the same component is used in EditRequest page as long term plan would be depriciate ReceiptSelector and use IOURequestStepScan as most structure of the component is same

Screenshot 2023-12-12 at 12 49 33 AM

i think the better solution is to introduce a shouldShowWrapper and pass it false from IOURequestStartPage so when the backTo is removed from the url by user it still shows the header other wise there will be no Header.

@luacmartins
Copy link
Contributor

@bernhardoj are you available to work on a PR?

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Hourly KSv2 labels Dec 11, 2023
@luacmartins
Copy link
Contributor

Put up a PR with the fix.

@bernhardoj
Copy link
Contributor

Sorry, just available now.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Dec 12, 2023
@jjcoffee
Copy link
Contributor

@luacmartins Would you mind assigning me here as I reviewed the PR?

@ishpaul777
Copy link
Contributor

ishpaul777 commented Dec 12, 2023

@luacmartins Just want clarify if it is expected that if user deeplink to add receipt page and dont have backto param in url there is no header, this feels broken IMO

PHOTO-2023-12-12-20-53-27

@luacmartins luacmartins added the Bug Something is broken. Auto assigns a BugZero manager. label Dec 12, 2023
Copy link

melvin-bot bot commented Dec 12, 2023

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 12, 2023

This comment was marked as off-topic.

@luacmartins luacmartins removed the Bug Something is broken. Auto assigns a BugZero manager. label Dec 12, 2023
@luacmartins
Copy link
Contributor

@stephanieelliott we just need to pay @jjcoffee for the review and @bernhardoj for the proposal (I wrote the PR since they were unavailable at the time)

@luacmartins
Copy link
Contributor

This issue is still reproducible in staging. Gonna take a look

@luacmartins
Copy link
Contributor

I'm not sure why this is happening. It's fixed on the simulator, but not on a physical device

@shubham1206agra
Copy link
Contributor

@luacmartins Can you create an adhoc build to test this?

@luacmartins
Copy link
Contributor

Nvm this is fixed in the latest 1.4.11-20 build. Maybe the CP hadn't made it to -16.

@luacmartins luacmartins removed the DeployBlockerCash This issue or pull request should block deployment label Dec 12, 2023
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Dec 13, 2023
@melvin-bot melvin-bot bot changed the title IOU - Add receipt header is missing in Add receipt page [HOLD for payment 2023-12-20] IOU - Add receipt header is missing in Add receipt page Dec 13, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Dec 13, 2023
Copy link

melvin-bot bot commented Dec 13, 2023

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Dec 13, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.11-25 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-12-20. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

  • @jjcoffee requires payment (Needs manual offer from BZ)
  • @bernhardoj requires payment (Needs manual offer from BZ)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 20, 2023
@jjcoffee
Copy link
Contributor

  • The PR that introduced the bug has been identified. Link to the PR: Refactor the money request creation flow #28618
  • The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/28618/files#r1433852688
  • A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
  • Determine if we should create a regression test for this bug. Yes
  • If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

  1. Tap FAB -> Request money.
  2. Enter any amount and proceed.
  3. Select any member and proceed.
  4. Tap 3-dot menu > Add receipt.
  5. Verify that the "Add receipt" header shows.

Do we agree 👍 or 👎

@luacmartins
Copy link
Contributor

We already have steps covering this flow, so I think we're good!

@stephanieelliott
Copy link
Contributor

Summarizing payment on this issue:

  • Contributor: $500, @bernhardoj I extended the offer on Upwork, can you please accept?
  • Contributor+: $500, @jjcoffee I extended the offer on Upwork, can you please accept?

Upwork job is here: https://www.upwork.com/jobs/~017a077bb53d8ea02c

@jjcoffee
Copy link
Contributor

@stephanieelliott Accepted, thanks!

@stephanieelliott
Copy link
Contributor

All paid, thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering
Projects
None yet
Development

No branches or pull requests

8 participants