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

[PAY-1990] Fix Crash On Apple Pay Pledge Page #1741

Merged
merged 3 commits into from Oct 4, 2022

Conversation

msadoon
Copy link
Contributor

@msadoon msadoon commented Oct 3, 2022

πŸ“² What

A crash was introduced with the new loading states for the payment sheet. An untested flow allowed the pre-mature update of a section before it had any visible data. This causes a crash related to the table view being out-of-sync with its' data source when reloading.

πŸ€” Why

We don't want to break this page in any way, even if we're introducing a feature hidden behind a flag. All pre-existing flows must work as they always did or conversion rates will go down.

πŸ›  How

Before we simply took the last section in the table view as "Add New Card" section. This was incorrect because the last section in the table view is not the "Add New Card" section (even though that is the last one we saw visually). It was the "Loading/Shimmer" state.

The key to understanding this is to know how ValueCellDataSource creates the sections. When it calls its' set method, it also calls its' padValuesForSection method. Which uses the section integer value (an enum raw value in this case PaymentMethodsTableViewSection) to fill out the empty sections between 0 and the section integer value.

That means, the section has been created for Add New Card, but during loading it isn't visible because its datasource values are empty but do exist within the UI for the tableview at the time of reload.

We just need to check that the reload of that section is updating an actual non-empty value. Again not necessary, but why update something that doesn't exist twice?

πŸ‘€ See

Before πŸ›

RPReplay_Final1664482675.MP4

After πŸ¦‹

RPReplay_Final1664837440.MP4

βœ… Acceptance criteria

  • Ensure hitting the Apple Pay button doesn't crash the app while the payment methods are loading.

@msadoon msadoon self-assigned this Oct 3, 2022
@msadoon msadoon added this to the release-5.5.0 milestone Oct 3, 2022
Revert changes to .pbxproj
Missed one reversion.
@codecov
Copy link

codecov bot commented Oct 3, 2022

Codecov Report

Merging #1741 (b9794c5) into main (0141c4a) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1741   +/-   ##
=======================================
  Coverage   85.35%   85.35%           
=======================================
  Files        1275     1275           
  Lines      114983   114983           
  Branches    30388    30389    +1     
=======================================
  Hits        98147    98147           
  Misses      15783    15783           
  Partials     1053     1053           
Impacted Files Coverage Ξ”
...ontroller/PledgePaymentMethodsViewController.swift 46.47% <100.00%> (ΓΈ)

πŸ“£ We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link

@sclampet sclampet left a comment

Choose a reason for hiding this comment

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

Nice catch πŸ‘πŸ»

@msadoon msadoon requested a review from sclampet October 4, 2022 00:21
@msadoon
Copy link
Contributor Author

msadoon commented Oct 4, 2022

I think @sclampet you approved this, but I didn't assign that id to the PR initially and re-assigning lost the approval. Hopefully its' not a huge deal to merge this as admin.

@msadoon msadoon merged commit c7e235e into main Oct 4, 2022
@msadoon msadoon deleted the pay-1990/crash-on-pledge-page branch October 4, 2022 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants