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 2024-03-20] [$500] iOS - BA - Confirm button on double check pages is closer to the bottom than other pages #37339

Closed
1 of 6 tasks
kbecciv opened this issue Feb 27, 2024 · 31 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Feb 27, 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: v1.4.44-4
Reproducible in staging?: y
Reproducible in production?: y
Issue reported by: Applause - Internal Team
Issue found when executing PR: #37025

Action Performed:

Pre-requisite: the user must be logged in and have created a workspace.

  1. Go to Workspace > Bank account.
  2. Initiate the add bank account flow until you reach a double check page (for account selection, personal information, company information)
  3. Verify the Confirm button is displayed closer to the button than the other pages.

Expected Result:

The Confirm button should be displayed on the same place as the other pages.

Actual Result:

The Confirm button is displayed closer to the button than the other pages.

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

image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a9093a007c21da81
  • Upwork Job ID: 1764463270160367616
  • Last Price Increase: 2024-03-04
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 27, 2024
Copy link

melvin-bot bot commented Feb 27, 2024

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

@kbecciv
Copy link
Author

kbecciv commented Feb 27, 2024

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

@Krishna2323

This comment was marked as off-topic.

@melvin-bot melvin-bot bot added the Overdue label Mar 1, 2024
@bernhardoj
Copy link
Contributor

Proposal

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

The bank info confirmation button is too close at the button compared to other page.

What is the root cause of that problem?

The bank info page is wrapped with a screen wrapper that disables includeSafeAreaPaddingBottom because it contains a form which already has it,

return (
<ScreenWrapper
testID={BankInfo.displayName}
includeSafeAreaPaddingBottom={false}

except for confirmation sub step because it just uses a plain scroll view.

return (
<ScrollView
style={styles.pt0}

This also happens to other bank account confirmation sub step (personal info confirmation, business info confirmation, etc.).

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

We should enable includeSafeAreaPaddingBottom when the page requires it, that is the confirmation page. There are many ways to do it, but some examples are:

  • wrap each page with SafeAreaConsumer to get the safeAreaPaddingBottomStyle and apply it to the root view.
<SafeAreaConsumer>
    {({safeAreaPaddingBottomStyle}) => (
        <ScrollView ... contentContainerStyle={[..., safeAreaPaddingBottomStyle]}>
  • in the parent step component, enable includeSafeAreaPaddingBottom if the sub step is the confirmation step
includeSafeAreaPaddingBottom={SubStep === Confirmation}
  • assuming the confirmation page is always at the last page, we can enable includeSafeAreaPaddingBottom if the sub step index is the last item
includeSafeAreaPaddingBottom={screenIndex === manualSubsteps.length - 1}
  • enrich the sub step component array by adding includeSafeAreaPaddingBottom property to it.
const manualSubsteps = [{component: Manual}, {component: Confirmation, includeSafeAreaPaddingBottom: true}];
...
includeSafeAreaPaddingBottom={!!SubStep.includeSafeAreaPaddingBottom}
...
<SubStep.Component

@bfitzexpensify bfitzexpensify added the External Added to denote the issue can be worked on by a contributor label Mar 4, 2024
Copy link

melvin-bot bot commented Mar 4, 2024

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

@melvin-bot melvin-bot bot changed the title iOS - BA - Confirm button on double check pages is closer to the bottom than other pages [$500] iOS - BA - Confirm button on double check pages is closer to the bottom than other pages Mar 4, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 4, 2024
Copy link

melvin-bot bot commented Mar 4, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Mar 4, 2024
@bfitzexpensify
Copy link
Contributor

Yes, definitely something we want to fix. @Ollyws, looks like we have a couple of proposals already ready for review

@scriptprojectsdev
Copy link

Proposal

Problem:

The "Confirm" button at the bottom of the screen is not at the same height as other buttons for other sub steps

Cause of the problem:

The issue is that this confirmation page is missing the SafeAreaConsumer that provides the proper padding for the bottom. Since the other steps are forms the SafeAreaConsumer is included in that making the other screens behave correctly.

Solution:

Add the SafeAreaConsumer to this screen to make it behave the same as other screens. There may be other screens that also don't have a form and display the bottom button in the wrong place. The solution should be applied to those screens as well.

Copy link

melvin-bot bot commented Mar 4, 2024

📣 @scriptprojectsdev! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@scriptprojectsdev
Copy link

Contributor details
Your Expensify account email: erick@scriptprojects.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01c16575c840ed0ce7

Copy link

melvin-bot bot commented Mar 5, 2024

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@Ollyws
Copy link
Contributor

Ollyws commented Mar 5, 2024

@bernhardoj's proposal LGTM. I think it's best we go with the SafeAreaConsumer solution.
🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Mar 5, 2024

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

@arosiclair
Copy link
Contributor

I think it's best we go with the SafeAreaConsumer solution.

This sounds good to me too

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

melvin-bot bot commented Mar 6, 2024

❌ There was an error making the offer to @Ollyws for the Reviewer role. The BZ member will need to manually hire the contributor.

Copy link

melvin-bot bot commented Mar 6, 2024

❌ There was an error making the offer to @bernhardoj for the Contributor role. The BZ member will need to manually hire the contributor.

@arosiclair
Copy link
Contributor

Weird those failed. You know anything about it @bfitzexpensify? Maybe ask in #bug-zero?

@bfitzexpensify
Copy link
Contributor

Yeah it happened across a few issues, Jack is working with Upwork

@bernhardoj
Copy link
Contributor

PR is ready

cc: @Ollyws

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 13, 2024
@melvin-bot melvin-bot bot changed the title [$500] iOS - BA - Confirm button on double check pages is closer to the bottom than other pages [HOLD for payment 2024-03-20] [$500] iOS - BA - Confirm button on double check pages is closer to the bottom than other pages Mar 13, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 13, 2024
Copy link

melvin-bot bot commented Mar 13, 2024

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

Copy link

melvin-bot bot commented Mar 13, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.51-3 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 2024-03-20. 🎊

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

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

Copy link

melvin-bot bot commented Mar 13, 2024

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:

  • [@Ollyws] The PR that introduced the bug has been identified. Link to the PR:
  • [@Ollyws] 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:
  • [@Ollyws] 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:
  • [@Ollyws] Determine if we should create a regression test for this bug.
  • [@Ollyws] 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.
  • [@bfitzexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 20, 2024
Copy link

melvin-bot bot commented Mar 20, 2024

Payment Summary

Upwork Job

  • ROLE: @Ollyws paid $(AMOUNT) via Upwork (LINK)
  • ROLE: @bernhardoj paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@bfitzexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1764463270160367616/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@Ollyws
Copy link
Contributor

Ollyws commented Mar 21, 2024

BugZero Checklist:

  • The PR that introduced the bug has been identified. Link to the PR:

#36763

  • 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/36763/files#r1533596226

  • 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. Open a workspace bank account page
2. Go to 1st & 2nd confirmation page
3. Verify the confirm button in each page isn't too close at the bottom
4. Go to the 5th step
5. Select yes for both sub-steps
6. Complete the personal information until you arrive at the confirmation page
7. Verify the confirm button in each page isn't too close at the bottom
8. Press confirm
9. Select no for "Are there more individuals who own 25% or more" and go next
10. Verify the confirm button in each page isn't too close at the bottom

Do we agree 👍 or 👎

@bfitzexpensify
Copy link
Contributor

@Ollyws & @bernhardoj - in Upwork, I'm seeing that the offer for this job has been sent to both of you and is awaiting acceptance, but given this comment, I suspect that that might not be correct.

Job name is [$500] iOS - BA - Confirm button on double check pages is closer to the bottom than other pages #37339 - do you both have an Upwork offer tied to that?

@bernhardoj
Copy link
Contributor

We received the offer but can't accept it.
image

@Ollyws
Copy link
Contributor

Ollyws commented Mar 21, 2024

I'm having the same problem as above.

@bfitzexpensify
Copy link
Contributor

Gotcha. I've re-sent the offers, let me know if they're working or not. I might need to recreate the job fully if they aren't.

@bernhardoj
Copy link
Contributor

It works, accepted

@Ollyws
Copy link
Contributor

Ollyws commented Mar 21, 2024

Accepted, thanks.

@bfitzexpensify
Copy link
Contributor

Cool, payments complete, regression steps proposed in https://github.com/Expensify/Expensify/issues/381507, closing this out - 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 Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

7 participants