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

Don't set default badge when only one defaultable payment method is available #7767

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

marcaaron
Copy link
Contributor

Details

Fixed Issues

$ #7613

Tests

  1. Add a payment method and verify there is no "Default" badge
    2022-02-15_15-02-36
  2. Add paypal.me and verify there is no "Default" badge
    2022-02-15_15-02-18
  3. Add another payment method and verify there is now a "Default" badge
    2022-02-15_15-01-58
  • Verify that no errors appear in the JS console

QA Steps (Internal)

  1. Run above test steps
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron added the InternalQA This pull request required internal QA label Feb 16, 2022
@marcaaron marcaaron self-assigned this Feb 16, 2022
@marcaaron marcaaron requested a review from a team as a code owner February 16, 2022 01:05
@MelvinBot MelvinBot requested review from timszot and removed request for a team February 16, 2022 01:05
@marcaaron
Copy link
Contributor Author

Note: This might be difficult to test until the deploy blocker mentioned here is resolved. I did the test steps on production and then checked the local PR against the production data to verify things work.

Copy link
Contributor

@timszot timszot left a comment

Choose a reason for hiding this comment

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

LGTM

@timszot
Copy link
Contributor

timszot commented Feb 16, 2022

Should we HOLD this PR until that blocker merged?

@marcaaron
Copy link
Contributor Author

Should we HOLD this PR until that blocker merged?

Yeah, let's do that. Thanks! This will fail QA if we try to test it now.

@marcaaron marcaaron changed the title Don't set default badge when only one defaultable payment method is available [HOLD issue 7769] Don't set default badge when only one defaultable payment method is available Feb 16, 2022
@marcaaron marcaaron changed the title [HOLD issue 7769] Don't set default badge when only one defaultable payment method is available Don't set default badge when only one defaultable payment method is available Feb 24, 2022
@marcaaron
Copy link
Contributor Author

Took this one off hold and merging

@marcaaron marcaaron merged commit 0368dad into main Feb 24, 2022
@marcaaron marcaaron deleted the marcaaron-noDefaultBadgeWhenOnlyOneMethod branch February 24, 2022 01:49
@MelvinBot
Copy link

Triggered auto assignment to @marcochavezf (InternalQA), see https://stackoverflow.com/c/expensify/questions/5042 for more details.

@botify
Copy link

botify commented Feb 24, 2022

@marcaaron looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@marcaaron
Copy link
Contributor Author

add a note explaining why this was done and remove the Emergency label if this is not an emergency.

Not an emergency. The label seems to have been applied in error as all tests were passing.

@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

OSBotify commented Mar 1, 2022

🚀 Deployed to staging by @marcaaron in version: 1.1.41-0 🚀

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

@marcaaron
Copy link
Contributor Author

Tested this on staging and it works as intended.

2022-03-01_14-12-14
2022-03-01_14-08-24

However, when we add a debit card we get a weird error:

[alrt] [Onyx] An error occurred while applying merge for key: cardList, Error: TypeError: Invalid attempt to spread non-iterable instance.
In order to be iterable, non-array objects must have a [Symbol.iterator]() method. - {"stack":"\"Error\\n    at Dc.serverLoggingCallback.alert (https://staging.new.expensify.com/app-d13d9cba0ab7ef47113e.bundle.js:213:194703)\\n    at https://staging.new.expensify.com/app-d13d9cba0ab7ef47113e.bundle.js:213:1479194\\n    at a (https://staging.new.expensify.com/app-d13d9cba0ab7ef47113e.bundle.js:25:76847)\\n    at https://staging.new.expensify.com/app-d13d9cba0ab7ef47113e.bundle.js:25:98763\""}

Not a blocker, but I'll create an issue.

@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2022

🚀 Deployed to staging by @marcaaron in version: 1.1.41-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.1.41-6 🚀

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
InternalQA This pull request required internal QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants