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

Add modal for existing owners error #4157

Merged
merged 7 commits into from
Jul 21, 2021
Merged

Conversation

jasperhuangg
Copy link
Contributor

@jasperhuangg jasperhuangg commented Jul 20, 2021

Details

Previously, setupWithdrawalAccount would silently error when a user tried to add a withdrawal bank account that we already assigned an owner to.

This PR adds some text similar to the text in Web-Secure.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/169896

Tests

  1. Paste this code block here. This'll simulate the server returning with an existingOwners error:
    return new Promise(resolve => resolve({existingOwners: ['patrick@expensify.com', 'spongebob@expensify.com', 'sandy@expensify.com']})).then((response) => {
        const existingOwnersList = response.existingOwners.reduce((ownersStr, owner, i, ownersArr) => {
            let separator = ',\n';
            if (i === 0) {
                separator = '\n';
            } else if (i === ownersArr.length - 1) {
                separator = ' and\n';
            }
            return `${ownersStr}${separator}${owner}`;
        }, '');
        Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false, error: CONST.BANK_ACCOUNT.ERROR.EXISTING_OWNERS, existingOwnersList});
    });
  2. Navigate to the add bank account flow for a workspace.
  3. Verify that you're unable to add the account since the bank account already has an existing owner, and verify that the error step displays (see screenshots).

QA

  1. Create a new workspace and click "Get Started".
  2. Add a bank account manually following the steps here.
  3. In a different account, follow steps 1 and 2.
  4. Verify that you're unable to add the account since the bank account already has an existing owner, and verify that the error step displays (see screenshots).

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-07-20 at 5 40 26 PM

Desktop

Screen Shot 2021-07-20 at 5 40 43 PM

Mobile-Web

Screen Shot 2021-07-20 at 5 41 35 PM

iOS

Screen Shot 2021-07-20 at 5 40 54 PM

@jasperhuangg jasperhuangg self-assigned this Jul 20, 2021
@shawnborton
Copy link
Contributor

Maybe we don't need that extra padding around the inner message?

@jasperhuangg
Copy link
Contributor Author

@shawnborton Removed the padding, let me know if there's anything else that catches your eye!

@jasperhuangg jasperhuangg marked this pull request as ready for review July 21, 2021 01:22
@jasperhuangg jasperhuangg requested a review from a team as a code owner July 21, 2021 01:22
@MelvinBot MelvinBot requested review from luacmartins and removed request for a team July 21, 2021 01:22
shawnborton
shawnborton previously approved these changes Jul 21, 2021
Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@luacmartins luacmartins merged commit fdb15d9 into main Jul 21, 2021
@luacmartins luacmartins deleted the jasper-existingOwnersModal branch July 21, 2021 15:40
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-5🚀

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

@isagoico
Copy link

@jasperhuangg While testing this, I noticed only 2 of the current users of the Bank account are displayed. I wanted to verify if there are any limits to how many users are displayed in the modal. For the account I just tried it has a LOT of active owners, that's why I found it weird that only 2 were displayed.

image

@jasperhuangg
Copy link
Contributor Author

@isagoico I'm honestly not too sure about this. The code in this PR simply handles displaying the modal with information that was already retrieved from the API. I'll reach out to @marcaaron and ask about this.

@isagoico
Copy link

Oh sure, the we could open a separate issue for this and check the PR off since the modal is correctly displayed right?

@roryabraham
Copy link
Contributor

I'm going to check this off on the checklist and run the production deploy. @isagoico Can you create a follow-up issue please? 🙏

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.80-2🚀

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

@marcaaron
Copy link
Contributor

For the account I just tried it has a LOT of active owners, that's why I found it weird that only 2 were displayed.

I honestly also have no idea about this but it sounds like something we should have followed up on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants