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

Core Data: Move write operations in CardPresentPaymentStore to the background #14131

Merged
merged 7 commits into from
Oct 17, 2024

Conversation

itsmeichigo
Copy link
Contributor

@itsmeichigo itsmeichigo commented Oct 16, 2024

Closes: #14130

Description

This PR continues the work to improve our Core Data usage in the app by moving write operations CardPresentPaymentStore to the background.

For more context: we want to handle all write operations in the background context, and save the changes to its parent - the persistent container. The view context would get merges directly from the persistent container, and should be used only for reading data. Writing to the view context causes performances issues, and changes made are not merged back to the background context causing data discrepancy.

The changes in this PR include:

  • Updated loading payment gateway accounts (both Stripe and WCPay):
    • Moved upsertStoredAccountInBackground and deleteStaleAccount implementation to be handled in the background.
    • Updated upsertStoredAccountInBackground to always delete existing accounts and insert new ones. Previously, we attempted to fetch an existing account and update it after deleting existing accounts, which is redundant.
  • Updated upsertCharge and deleteCharge to be handled in the background.

Steps to reproduce

Testing account fetching:

  • Log in to a test store set up for card present payments (with WCPay or Stripe).
  • Navigate to the Settings tab and confirm that there are no longer warning for write operations in the main thread like ⚠️ Write operations for PaymentGatewayAccount should only be done on a background context or ⚠️ Saving data should only be done on a background context.
  • Select Payments and confirm that payment accounts are loaded successfully.

Testing charge fetching:

  • Log in to a test store set up for card present payments (with WCPay or Stripe).
  • Create an order and collect payment using a card reader.
  • Open the order details on the app and select Issue Refund.
  • Confirm that in Xcode consoles there are no warnings about write operations for WCPayCharge and WCPayCardPresentPaymentDetails.
  • Select an order item and update the amount to 1 and submit the refund. Confirm that the refund works correctly.

Testing information

Tested on simulator iPhone 16 Pro iOS 18.1 and confirmed that payment account loading and refund both work as expected.

Screenshots

N/A


  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

TODO: @itsmeichigo to update the release notes before merging

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

@itsmeichigo itsmeichigo added type: task An internally driven task. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. labels Oct 16, 2024
@itsmeichigo itsmeichigo added this to the 20.8 milestone Oct 16, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Oct 16, 2024

1 Warning
⚠️ This PR is assigned to the milestone 20.8. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 16, 2024

WooCommerce iOS📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr14131-ccd27ef
Version20.7
Bundle IDcom.automattic.alpha.woocommerce
Commitccd27ef
App Center BuildWooCommerce - Prototype Builds #11167
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@itsmeichigo itsmeichigo marked this pull request as ready for review October 16, 2024 05:14
Copy link
Collaborator

@staskus staskus left a comment

Choose a reason for hiding this comment

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

Thanks for the work!

✅ Code changes make sense to me.

✅ I tested the described cases and additionally checked how this new background writing code behaves when switching accounts or logging in / logging out.

✅ The CardPresentPaymentStore itself is covered with unit tests and they continue to succeed.

Given the devil's in the details with Core Data and Payments code, we can wait for a second pair of eyes to take a look at these changes as well.

@jaclync
Copy link
Contributor

jaclync commented Oct 17, 2024

Given the devil's in the details with Core Data and Payments code, we can wait for a second pair of eyes to take a look at these changes as well.

Thanks for the initial review @staskus, I'm taking a look now as well.

Copy link
Contributor

@jaclync jaclync left a comment

Choose a reason for hiding this comment

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

Code changes and testing LGTM on an iOS 17.0.1 iPad simulator ✅

Just curious, do we have a way to measure the performance difference like some metrics that we're monitoring?

@itsmeichigo
Copy link
Contributor Author

itsmeichigo commented Oct 17, 2024

Thanks for the reviews @staskus and @jaclync!

Just curious, do we have a way to measure the performance difference like some metrics that we're monitoring?

Good question! The initial motivation of this project was to mitigate the recent rise in crashes coming from Core Data (peaMlT-Rx-p2). But it makes sense to establish some way to measure the performance impact as well, and I'm asking Eagle for advice on this as they have been observing app performance pe5pgL-5Wc-p2#comment-4628. I'll update the project thread with any updates soon.

Please feel free to suggest any other insight regarding payments, that would be extremely helpful!

@itsmeichigo itsmeichigo merged commit 3fc2808 into trunk Oct 17, 2024
14 checks passed
@itsmeichigo itsmeichigo deleted the task/14130-cardpresentpaymentstore-storage-update branch October 17, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: mobile payments Related to mobile payments / card present payments / Woo Payments. type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize storage usage in CardPresentPaymentStore
5 participants