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] Keep product image's dateCreated optional. #14317

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

selanthiraiyan
Copy link
Contributor

@selanthiraiyan selanthiraiyan commented Nov 5, 2024

Closes: #14312

Description

In #14013 we made the Product object’s date and dateCreated properties optional, to avoid crashes when transforming deleted product objects in Core Data. It’s possible that (now that we aren’t crashing on the product objects) a similar crash is now happening when transforming deleted product image objects.

This PR follows the same approach from #14013 and marks the dateCreated Swift property of ProductImage as optional.

Steps to reproduce

Read the following excerpt from internal discussion - p91TBi-cd4-p2

When attempting to apply the update to the background context using persistent container’s newBackgroundContext and automaticallyMergesChangesFromParent turned off for the view context, I encountered a crash in Product+ReadOnlyConvertible when converting a product’s date and dateCreated from a storage model to a readonly object.

These attributes were declared as non-optional without any default values. A Core Data object can be deleted and there can still be a reference to it somewhere, therefore things that are marked as non-optional can be gone at runtime since Core Data can never fulfill its dynamic expectation.

The solution is to always be mindful when using non-optionals with Core Data in case we want to manually handle the merging for the view context in the future. We can set default values for the above date attributes or make them optional and set default values to them when finding nil values while converting to readonly objects.

To ensure safety, we should check all the other existing entities for non-optional attributes without default values. We should also do a thorough smoke test to see if there’s any similar crash happening anywhere else in the app.

Based on the above explanation, this crash is related to introducing background context to core data. This cannot be reproduced locally following a fixed set of steps.

  • Install a build from trunk
  • Login into the app
  • Switch to this branch and run the app to replace the build from trunk
  • Create a product with images
  • Smoke test that product image upload, viewing and deletion works as expected

Testing information

  • I tested that a build from trunk can be replaced without any core data related crashes.
  • I tested that product image upload, viewing and deletion works as expected.

Screenshots

Simulator Screen Recording - iPhone 15 Pro Max - 2024-11-05 at 18 11 23


  • 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.

@selanthiraiyan selanthiraiyan added type: crash The worst kind of bug. feature: product details Related to adding or editing products, including Product Settings. labels Nov 5, 2024
@selanthiraiyan selanthiraiyan added this to the 21.0 ❄️ milestone Nov 5, 2024
@dangermattic
Copy link
Collaborator

1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

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 Numberpr14317-84e15e1
Version21.0
Bundle IDcom.automattic.alpha.woocommerce
Commit84e15e1
App Center BuildWooCommerce - Prototype Builds #11452
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@selanthiraiyan selanthiraiyan marked this pull request as ready for review November 5, 2024 12:42
@rachelmcr rachelmcr self-assigned this Nov 5, 2024
Copy link
Contributor

@rachelmcr rachelmcr left a comment

Choose a reason for hiding this comment

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

I can't reproduce the reported crash, either, but the testing steps here worked well for me. I also tested deleting a product with an image and had no issues with that, either.

I'll go ahead and merge this fix so we can release a new beta!

@rachelmcr rachelmcr merged commit f182ccf into release/21.0 Nov 5, 2024
23 checks passed
@rachelmcr rachelmcr deleted the bugfix/14312-product-image branch November 5, 2024 16:58
Copy link

sentry-io bot commented Nov 9, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ EXC_BREAKPOINT: Exception 6, Code 1, Subcode 6571544800 ProductImage.toReadOnly View Issue
  • ‼️ EXC_BREAKPOINT: Exception 6, Code 1, Subcode 6682751816 > ProductImage.toReadOnly View Issue
  • ‼️ EXC_BREAKPOINT: Exception 6, Code 1, Subcode 7000527072 ProductImage.toReadOnly View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: product details Related to adding or editing products, including Product Settings. type: crash The worst kind of bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants