[No QA] Add BeneficialOwnersStep (UI Only) #3585

marcaaron · 2021-06-14T22:24:21Z

cc @NikkiWines since this might affect validation stuff for the RequestorStep and builds on your work there

Details

Hooking up the UI for the ACHContractStep. We will test the API flow in another PR.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/166920

Tests

Force the ACHContractStep to show by modifying the logic here

diff --git a/src/pages/ReimbursementAccount/ReimbursementAccountPage.js b/src/pages/ReimbursementAccount/ReimbursementAccountPage.js
index 101079b76..248fe10c8 100644
--- a/src/pages/ReimbursementAccount/ReimbursementAccountPage.js
+++ b/src/pages/ReimbursementAccount/ReimbursementAccountPage.js
@@ -139,7 +139,7 @@ class ReimbursementAccountPage extends React.Component {
         // allows us to continue the flow from various points depending on where the user left off. We can also
         // specify a specific step to navigate to by using route params.
         const achData = this.props.reimbursementAccount.achData;
-        const currentStep = this.getStepToOpenFromRouteParams() || achData.currentStep;
+        const currentStep = CONST.BANK_ACCOUNT.STEP.ACH_CONTRACT;
         return (
             <ScreenWrapper>
                 <KeyboardAvoidingView>

Try to add a beneficial owner
Remove a beneficial owner
Verify you cannot add more than three beneficial owners if you have also checked that you own 25%
Verify you cannot remove the last beneficial owner unless you check the box that there are no more owners

QA Steps

No QA

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

NikkiWines

Looks good, a little weirdness with the styling of the links on the requestor page:

Also I appear to be able to add 4 beneficial owners, not 3.

NikkiWines · 2021-06-18T00:21:03Z

src/languages/es.js

@@ -304,6 +304,19 @@ export default {
        noPhoneNumber: 'Por favor escribe un número de teléfono que incluya el código de país e.g +447814266907',
        maxParticipantsReached: 'Has llegado al número máximo de participantes para un grupo.',
    },
+    beneficialOwnersStep: {


It's occurred to me that I forgot to add the requestorStep values to this file 🙈

Why was this left in english if this is the spanish file?

I think we are fixing that here https://expensify.slack.com/archives/C21FRDWCV/p1629825626117900

When we started this there was pressure to get it finished within 2 weeks so we moved very quickly and Spanish translations were not prioritized.

marcaaron · 2021-06-18T01:17:37Z

Ah sorry you can actually add 4 owners, but only if you do not check that you own more than 25%

It does look like there is a way to add 5 if you get creative and first add 4 then check the box 😅 - but we are not doing anything about this in Web-Secure so I think maybe it's an edge case.

marcaaron · 2021-06-18T01:22:54Z

Not really sure what is up with that text hmm... was it working before? First guess is we should not be nesting a Pressable instead a Text

marcaaron · 2021-06-18T01:37:02Z

Ah no it's actually a change in TextLink here

marcaaron · 2021-06-18T02:01:34Z

Updated + fixed the text issue

marcaaron · 2021-06-18T17:41:23Z

Alright, I think after this is merged we will be able to unblock hooking the RequestorStep up to the API. Lemme know if there are any additional changes to make here so we can get started on that.

NikkiWines

Looking good, minor question about size="micro" for TextLink

NikkiWines · 2021-06-18T18:05:57Z

src/pages/ReimbursementAccount/RequestorStep.js

+                        <Text style={[styles.mt3, styles.textMicroSupporting]}>
+                            {this.props.translate('requestorStep.onFidoConditions')}
+                            <TextLink
+                                size="micro"


Why not just pass styles.textMicroSupporting in the style prop instead of introducing a new propType?

If you move styles.link to the end of the array of styles in TextLink.js then you can just pass style={styles.textMicroSupporting} here

Couple reasons, but I did have a similar idea initially...

the additionalStyles are added after everything else so the color in textMicroSupporting would overwrite the link styles (which is undesirable since they lose the blue color and appearance as links)

https://github.com/Expensify/Expensify.cash/blob/be71feb42287a5753b2755a7975d4679baefdbc8/src/components/TextLink.js#L34

since this needed to be added a handful of things it seemed like a good idea to just bake it into the component

That said, we could undo this and add it with a style like textMicroSupportingLink or remove the "color" from textMicroSupporting and add it in with another style. But not really sure what is best here.

If you move styles.link to the end of the array of styles in TextLink.js then you can just pass style={styles.textMicroSupporting} here

Right that just undoes what we did here, but I think the change @roryabraham made makes sense. Styles passed in as props should overwrite some base style and not come before the base styles.

one sec, I think I've got a solution

NikkiWines

Looks good!

OSBotify · 2021-06-18T20:05:56Z

🚀 Deployed to staging in version: 1.0.71-2🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-06-25T18:29:39Z

🚀 Deployed to production in version: 1.0.73-3🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

add beneficial owners step

e984e08

marcaaron self-assigned this Jun 14, 2021

marcaaron added 7 commits June 14, 2021 13:16

fix beneficial owners list

73ea817

fix conflicts

e9474fc

add the IdentityForm

a705ca1

add identity form to BeneficialOwners

b310661

use state fields

94ab19e

hook up the beneficial owners step

5567976

add default style + translation

03c0c4e

marcaaron changed the title ~~[WIP] Add BeneficialOwnersStep~~ [No QA] Add BeneficialOwnersStep (UI Only) Jun 17, 2021

hook up nav buttons

4b278b7

marcaaron marked this pull request as ready for review June 17, 2021 21:00

marcaaron requested a review from a team as a code owner June 17, 2021 21:00

MelvinBot requested review from AndrewGable and removed request for a team June 17, 2021 21:01

marcaaron requested a review from NikkiWines June 17, 2021 21:02

NikkiWines reviewed Jun 18, 2021

View reviewed changes

start with empty array of owners

0f14350

give TextLink a size micro option

603671d

fix conflicts

1cdd7cf

NikkiWines reviewed Jun 18, 2021

View reviewed changes

marcaaron added 2 commits June 18, 2021 08:33

pass textMicro instead

aaa1682

useless change

45a77a3

NikkiWines approved these changes Jun 18, 2021

View reviewed changes

NikkiWines merged commit 16e9ef0 into main Jun 18, 2021

NikkiWines deleted the marcaaron-beneficialOwners branch June 18, 2021 19:14

OSBotify mentioned this pull request Jun 18, 2021

Deploy Checklist: Expensify.cash 2021-06-15 #3593

Closed

aldo-expensify mentioned this pull request Nov 16, 2022

[$250] VBA -URL is shown on bottom left corner when Fix the error is hovered reported by @huzaifa-99 #12692

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[No QA] Add BeneficialOwnersStep (UI Only) #3585

[No QA] Add BeneficialOwnersStep (UI Only) #3585

marcaaron commented Jun 14, 2021 •

edited

Loading

NikkiWines left a comment •

edited

Loading

NikkiWines Jun 18, 2021

iwiznia Aug 26, 2021

iwiznia Aug 26, 2021

marcaaron Aug 26, 2021

marcaaron commented Jun 18, 2021

marcaaron commented Jun 18, 2021

marcaaron commented Jun 18, 2021

marcaaron commented Jun 18, 2021

marcaaron commented Jun 18, 2021

NikkiWines left a comment

NikkiWines Jun 18, 2021

NikkiWines Jun 18, 2021

marcaaron Jun 18, 2021

marcaaron Jun 18, 2021

marcaaron Jun 18, 2021

marcaaron Jun 18, 2021

NikkiWines left a comment

OSBotify commented Jun 18, 2021

OSBotify commented Jun 25, 2021

[No QA] Add BeneficialOwnersStep (UI Only) #3585

[No QA] Add BeneficialOwnersStep (UI Only) #3585

Conversation

marcaaron commented Jun 14, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

NikkiWines left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Jun 18, 2021

marcaaron commented Jun 18, 2021

marcaaron commented Jun 18, 2021

marcaaron commented Jun 18, 2021

marcaaron commented Jun 18, 2021

NikkiWines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikkiWines left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 18, 2021

OSBotify commented Jun 25, 2021

marcaaron commented Jun 14, 2021 •

edited

Loading

NikkiWines left a comment •

edited

Loading