[MBL-866] Remove Add New Card Page #1829
Conversation
Generated by 🚫 Danger |
Codecov Report
@@ Coverage Diff @@
## main #1829 +/- ##
==========================================
+ Coverage 84.33% 84.46% +0.13%
==========================================
Files 1277 1274 -3
Lines 115881 115167 -714
Branches 30810 30666 -144
==========================================
- Hits 97728 97278 -450
+ Misses 17086 16821 -265
- Partials 1067 1068 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@scottkicks So as for the acceptence criteria I went through most of it - the stuff I skipped was anything Apple Pay related, required backend support or didn't surface the Stripe payment sheet. Here's what I tested: Here's what I didn't test: Feel free to do as much of the untested stuff as you want. I'm confident based on the code changes and testing there were no regressions introduced. If you can find any - great! I encourage you to try to break this code - ie. find ways it might not be production ready. |
|
@scottkicks Tests were passing before I assigned, they just got re-run because |
| "revision" : "a5f16ba68913840ee5df91b8dc06f5cc063579de", | ||
| "version" : "1.2021110200.0" |
There was a problem hiding this comment.
Do we need to commit these version updates in this PR?
There was a problem hiding this comment.
Okay good call. I'll remove these updates because they could potentially introduce breaking changes. It seemed okay in testing, but these dependencies were not tested individually in their use cases. It's on my plate to go over the dependencies with Isabel and understand what we need to update each quarter. We'll update this file separately.
There was a problem hiding this comment.
I said they were okay in a previous comment, but re-thinking it here is safer not to commit these changes.
There was a problem hiding this comment.
Ran through the test cases and couldn't get the add new payment feature to break. nice work!
I did notice that in PledgePaymentMethodsViewModelTests we should be able to remove private let goToAddCardIntent on line: 14. It isn’t being mapped to any view model outputs and may just be left over from some of the optimally removal work
One other question I have:
Is the expected UI when adding a card from settings that the loading indicator keeps showing until the new card has been added or cancelled? Or do we want that to stop loading once the add card screen pops up?
Yea totally valid - I missed cleaning up The loading spinner was implemented a long time ago - like last year - so reworking that is a little out of scope on this PR. It was acceptable UI when the Stripe payment sheet went in. Ideally we'd have loading off after the payment sheet is shown and turned on again before the payment cards are updated. Then off again after the update. This is a nice-to-have. It wasn't an introduced regression from this code. Was this way since last year if I remember correctly. |
📲 What
With the removal of Optimizely as our feature flagging tool, we've hardcoded the Stripe payment sheet as permanent feature. With that done, we no longer need our older Add New Card page.
This also simplifies the update to Stripe's framework (less UI components to update).
🤔 Why
It's part of technical roadmap for the rest of 2023, but also a precursor to upgrading Stripe.
🛠 How
AddNewCardViewControllerAddNewCardViewControllerTests-> There were no associated screenshotsAddNewCardViewModelAddNewCardViewModelTestsAssociated views that present the above screen:
PledgePaymentMethodsViewControllerPaymentMethodsViewControllerAdjustments were made to remove input and output signals to functions presenting the view.
👀 See
No difference, Stripe payment sheet is displayed as before.
✅ Acceptance criteria
⏰ TODO