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-2040] Payment Sheet Displayed On Unrelated Context #1752

Merged

Conversation

msadoon
Copy link
Contributor

@msadoon msadoon commented Oct 26, 2022

📲 What and 🤔 Why

We want to prevent any janky behaviour with the payment sheet because it does take a moment to show and the user has the chance to back to the previous page in that time.

🛠 How

Check the view type before presenting but after creating the PaymentSheet.FlowController.

👀 See

Before 🐛

RPReplay_Final1666821573.MP4

After 🦋

RPReplay_Final1666822441.MP4

✅ Acceptance criteria

  • Neither PledgeViewController or PaymentMethodsViewController show the payment sheet.

⏰ TODO

  • Test both contexts > 10 times (tricky due to timing)

create function calls for calling vm from vc, ensure nil value still sends the property name. No need to send user information.
@msadoon msadoon added this to the release-5.6.0 milestone Oct 26, 2022
@msadoon msadoon self-assigned this Oct 26, 2022
@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Merging #1752 (4e664e2) into main (d954d24) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1752      +/-   ##
==========================================
- Coverage   85.38%   85.38%   -0.01%     
==========================================
  Files        1275     1275              
  Lines      115183   115189       +6     
  Branches    30455    30457       +2     
==========================================
  Hits        98350    98350              
- Misses      15779    15785       +6     
  Partials     1054     1054              
Impacted Files Coverage Δ
...hods/Controller/PaymentMethodsViewController.swift 60.00% <0.00%> (-1.02%) ⬇️
...ontroller/PledgePaymentMethodsViewController.swift 45.51% <0.00%> (-0.97%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

Tested and this is working as expected. Nice catch!

The instances of these ViewControllers are actually deallocated when going back to the previous screens most of the time. It's only in those very rare cases where the sheet's create completion makes it past those guard statements.

Took me a minute to actually time that right 😆 to see the bug! Just need to update this branch with main

@msadoon
Copy link
Contributor Author

msadoon commented Oct 27, 2022

Tested and this is working as expected. Nice catch!

The instances of these ViewControllers are actually deallocated when going back to the previous screens most of the time. It's only in those very rare cases where the sheet's create completion makes it past those guard statements.

Took me a minute to actually time that right 😆 to see the bug! Just need to update this branch with main

Cool, thanks for the review, it is a timing issue, I didn't think it would matter given how rarely it happens but probably best to tighten up the logic a little. I updated the branch, there's an update branch with main button on the bottom right.

@msadoon msadoon merged commit e61111e into main Oct 27, 2022
@msadoon msadoon deleted the experiment/track-payment-sheet-view-appearance-navigation-stack branch October 27, 2022 15:15
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