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

Prevent accounts without account numbers from breaking the payments page #4262

Merged
merged 3 commits into from
Jul 28, 2021

Conversation

stitesExpensify
Copy link
Contributor

@stitesExpensify stitesExpensify commented Jul 27, 2021

Details

  1. Do not return un-issued or deleted cards
  2. If we get an account without an account number just don't display the description.

Fixed Issues

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

Tests

  1. Log into e.cash with an account that has both a bank account and credit card
  2. Make sure there are descriptions under each with the last 4 of the account numbers

QA Steps

  1. Ping @joekaufmanexpensify and ask him to go to the payments page
  2. His page should not crash

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android
    2021-07-28_11-22-20

@stitesExpensify stitesExpensify requested a review from a team as a code owner July 27, 2021 22:45
@stitesExpensify stitesExpensify self-assigned this Jul 27, 2021
@MelvinBot MelvinBot requested review from Dal-Papa and removed request for a team July 27, 2021 22:45
Copy link

@Dal-Papa Dal-Papa left a comment

Choose a reason for hiding this comment

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

Any chance you (or @joekaufmanexpensify) can provide a screenshot ?

@@ -83,12 +83,17 @@ class PaymentMethodList extends Component {
_.each(this.props.bankAccountList, (bankAccount) => {
// Add all bank accounts besides the wallet
if (bankAccount.type !== CONST.BANK_ACCOUNT_TYPES.WALLET) {
const formattedBankAccountNumber = bankAccount.accountNumber

Choose a reason for hiding this comment

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

The famous line broken down 😂

@joekaufmanexpensify
Copy link
Contributor

@Dal-Papa Are you looking for a screenshot of how my screen looks when I try to access the payments page from my test account?

@Dal-Papa
Copy link

Ideally a screenshot of your screen with the fix that @stitesExpensify provided, but I think we can simply QA when that's out.

@joekaufmanexpensify
Copy link
Contributor

Cool sounds good, feel free to ping me when it's time to QA!

@stitesExpensify
Copy link
Contributor Author

2021-07-28_11-22-20
Account without account number and no crash

@Dal-Papa Dal-Papa merged commit aab1fbc into main Jul 28, 2021
@Dal-Papa Dal-Papa deleted the stites-fixBrokenSlice branch July 28, 2021 17:37
@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.80-3🚀

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

@isagoico
Copy link

@joekaufmanexpensify Can you go to your payments page and check if the app crashes? 😬 (following PR orders)

@joekaufmanexpensify
Copy link
Contributor

I was able to access the Payments page from my expensifail account on staging!

2021-07-29_08-11-24

@stitesExpensify
Copy link
Contributor Author

🎉 Thanks @joekaufmanexpensify

@joekaufmanexpensify
Copy link
Contributor

you're welcome!

@stitesExpensify
Copy link
Contributor Author

This issue can be checked off, but it looks like something weird is still going on though, we shouldn't be putting non-activated cards in the wallet since they can't be used. Would you mind making an issue for that @joekaufmanexpensify ?

@joekaufmanexpensify
Copy link
Contributor

Wouldn't we want that listed there so the user can click the validate that card in the future? Even though I'm not using this card because it was sent by mistake, it's still a card that exists. Or is it just not expected behavior that the card is listed there right now because validating expensify cards in newdot doesn't exist yet?

@stitesExpensify
Copy link
Contributor Author

Yeah my thinking was that we would not show it for now since it is not usable, and then we'll start showing those again once we decide we want the ability to activate in newDot

@joekaufmanexpensify
Copy link
Contributor

We are supposed to show the card in NewDot once it is activated though right? If that is what we do, then it could make sense to just add Not Activated or something similar now rather remove it. Because then we'd just need to add it back later.

@stitesExpensify
Copy link
Contributor Author

Well the cards are in different states. So basically the front end would only get activated cards from the server which means that when it is activated, it will automatically show up

@joekaufmanexpensify
Copy link
Contributor

Ah got it, so we haven't really considered how the activation flow will work for Expensify Cards in NewDot at all? And we haven't even mapped out how we want un-activated cards to appear in NewDot yet for N6, so we should remove now and then add something like this when we've actively designed how this will work?

@stitesExpensify
Copy link
Contributor Author

Yep! That's what I'm thinking :)

@joekaufmanexpensify
Copy link
Contributor

Sounds good. I'll make a new issue today!

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.81-4🚀

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

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.

5 participants