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 Stack updates #14013

Merged
merged 30 commits into from
Oct 15, 2024
Merged

Core Data Stack updates #14013

merged 30 commits into from
Oct 15, 2024

Conversation

itsmeichigo
Copy link
Contributor

@itsmeichigo itsmeichigo commented Sep 23, 2024

Closes: #14054
Also closes: #2370

Description

This PR adds a few improvements to the Core Data stack:

  • Updated the writerDerivedStorage to be independent of the main context. Instead, the background context is now directly associated with the persistent container following Apple's recommendation. The main context now merges changes from the persistent container automatically.
  • A writer operation queue has been introduced to take care of write operations one-by-one following WP/JP's implementation here. I'm following the simple variation for the completion handler without the result parameter as we don't currently have any failure handling for the write operations - I suppose we can add that variation when we need it.
  • Removed the unused method performBackgroundTask - we want to avoid creating too many background context through this method.
  • Updated saveDerivedType to only save the background context to the persistent container.
  • Marked access to writerDerivedStorage and saveDerivedType as deprecated and added a helper method performAndSave for performing write operations similarly to WP/JP's Core Data stack. Updates for the stores will be handled in future PRs.
  • Added check to ensure write operations are handled in a background context. The checks are handled in debug mode only, and a new launch argument -enforce-core-data-write-in-background has been added to enable assert when writing is done on the main context.
  • Initialize the storage manager along with AppDelegate to ensure that the Core Data stack is created before the stores manager. This is an attempt to eliminate confusing stack traces [peaMlT-Rx-p2#comment-2194]. The order of initialization should now be as follows: CrashLogging > CoreDataManager > DefaultStoresManager.
  • Keep product's date and dateCreated Swift properties optional to avoid crashes when transforming deleted product objects in Core Data.

TODO: @itsmeichigo to update the release notes before merging.

Steps to reproduce

Smoke testing the app

  • With -enforce-core-data-write-in-background disabled, build and run the app in debug mode.
  • Log in to a test store with existing products/orders/Blaze campaigns.
  • Smoke test the app following the guide P91TBi-bVe-p2.

Testing enforcing write in background

  • With -enforce-core-data-write-in-background enabled, build and run the app in debug mode.
  • Log in to a test store with existing products/orders/Blaze campaigns.
  • Confirm that the assert causes a fatal error where a write operation is handled in the main context.

Testing information

Tested on device iPad pro 4th gen iOS 17.6:

  • Logged in to an atomic store with wpcom account successfully.
  • Confirmed that dashboard works as before: stats cards with different time ranges, all cards enabled with different filters.
  • Confirmed that orders work normally: pagination, searching, order creation and edit, creating shipping labels, marking orders as complete.
  • Confirmed that products work normally: pagination, searching, bulk update, product creation with AI, creating & editing variable products, updating product details.
  • Confirmed that hub menu works normally: store switching, settings, payments, google ads, blaze, woo admin, view store, coupons, reviews, inbox, customers.

Tested on simulator iPhone 15 Pro iOS 17.4 and -enforce-core-data-write-in-background enabled and confirmed that write operations in the main context causes assertion failure.

Screenshots

N/A


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

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: enhancement A request for an enhancement. feature: product list Related to the product list. labels Sep 23, 2024
@itsmeichigo itsmeichigo added this to the 20.6 milestone Sep 23, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 23, 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 Numberpr14013-64c71ce
Version20.7
Bundle IDcom.automattic.alpha.woocommerce
Commit64c71ce
App Center BuildWooCommerce - Prototype Builds #11135
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot wpmobilebot modified the milestones: 20.6, 20.7 Sep 27, 2024
@wpmobilebot
Copy link
Collaborator

Version 20.6 has now entered code-freeze, so the milestone of this PR has been updated to 20.7.

@itsmeichigo itsmeichigo marked this pull request as ready for review September 30, 2024 05:28
@itsmeichigo itsmeichigo modified the milestones: 20.7, 20.8 Sep 30, 2024
@itsmeichigo
Copy link
Contributor Author

itsmeichigo commented Sep 30, 2024

Moved the milestone to 20.8 to avoid causing any issue during the Woo DM. Other PRs will be based on top of this PR and merged gradually after this PR is merged.

@itsmeichigo itsmeichigo added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Sep 30, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Sep 30, 2024

1 Warning
⚠️ This PR is assigned to the milestone 20.8. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

Copy link
Contributor

@selanthiraiyan selanthiraiyan 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 working on this, Huong! 👏

The changes LGTM, and it works well on an iPhone simulator running iOS 17.4.

I left a few non-blocking suggestions and questions.

WooCommerce/Classes/AppDelegate.swift Outdated Show resolved Hide resolved
@itsmeichigo itsmeichigo removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Oct 14, 2024
@itsmeichigo
Copy link
Contributor Author

Thanks @selanthiraiyan for the reviews! I responded to your comments regarding the unit tests, let me know what you think. I'll merge the PR once I get another green light from you.

Copy link
Contributor

@selanthiraiyan selanthiraiyan 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 updates and comments, Huong! Let us merge this PR. 🚢

@itsmeichigo itsmeichigo merged commit 62f2b7b into trunk Oct 15, 2024
14 checks passed
@itsmeichigo itsmeichigo deleted the task/core-data-update branch October 15, 2024 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: product list Related to the product list. type: enhancement A request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Core Data Stack following WP/JP Untangle Core Data Setup and CrashLogging
6 participants