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

[Product] Update product tag store to migrate Core Data usage and fix a few issues #14511

Merged
merged 24 commits into from
Nov 29, 2024

Conversation

itsmeichigo
Copy link
Contributor

@itsmeichigo itsmeichigo commented Nov 26, 2024

Part of #14091
Closes #14513

Description

This PR updates the usage of storage in ProductTagStore:

  • Replace the deprecated usage of writerDerivedStorage with the new method performAndSave for upserting product tags.
  • Handle removing unused tags as part of the upsertion.
  • Move deleteStoredProductTags to be executed in the background.
  • Replace multiple fetch requests with a single one to optimize performance.

While testing the above changes, I found that even though we remove unused tags upon syncing, those tags are still displayed when selecting tags for products. The reason is that we are adding all the tags fetched from the remote to storage again eventually.

I fixed this by parsing the count property for product tags and use it to filter only used tags when syncing. This ensures that only used tags are recommended, similar to how the feature works in WooCommerce Core.

Also: addressed #14513 by not triggering the tag creation endpoint when the tag list is empty.

Steps to reproduce

  • Log in to a store with existing products.
  • Create a new product or open an existing one.
  • Add a new tag that is not used for any other products and save the changes.
  • Open another product and select the tag list. Confirm that the new tag is suggested for the product.
  • Open the initial product and select the tag list. Clear all the added tags and save the changes.
  • Open another product and select the tag list. Confirm that the new tag is no longer suggested.
  • Also: confirm that the tag list is displayed with cached items upon loading.

Testing information

Tested and confirmed with simulator iPhone 16 Pro iOS 18.1:

  • Product list is loaded correctly.
  • Tags are listed correctly for existing products.
  • Add/edit products: only used tags are suggested.
  • Removing all references to a tag hides it from the suggested list for products.

Screenshots

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-11-26.at.15.27.13.mp4

  • 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: task An internally driven task. feature: product list Related to the product list. feature: add/edit products Related to adding or editing products. labels Nov 26, 2024
@itsmeichigo itsmeichigo added this to the 21.3 milestone Nov 26, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 26, 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 Numberpr14511-8cd948a
Version21.2
Bundle IDcom.automattic.alpha.woocommerce
Commit8cd948a
App Center BuildWooCommerce - Prototype Builds #11849
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@itsmeichigo itsmeichigo changed the title Core Data: Update storage usage for product tag and ensure to display only used tags [Product] Update product tag store to migrate Core Data usage and fix a few issues Nov 26, 2024
@itsmeichigo itsmeichigo marked this pull request as ready for review November 26, 2024 10:21
Copy link
Member

@hichamboushaba hichamboushaba left a comment

Choose a reason for hiding this comment

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

Nice work @itsmeichigo, all works as expected (tested using iPhone 16 simulator, iOS 18.1)

One point though, it seems the cached data is not used anywhere, we always fetch tags from remote before showing them, I think a good improvement to do to this screen is to update it to show the cached data first, WDYT? I can create a ticket for this improvement if you agree.

Yosemite/Yosemite/Stores/ProductTagStore.swift Outdated Show resolved Hide resolved
Comment on lines +91 to +93
guard tags.isEmpty == false else {
return onCompletion(.success([]))
}
Copy link
Member

Choose a reason for hiding this comment

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

I found it interesting that we need this check here, after checking the code, the issue is in the handling of tags in iOS, the logic in ProductTagsViewController could use some optimization, as currently it always tries to create tags even the already added ones, and by extension causes the issue that you noticed.
Normally, the logic should be create just the new tags, and would speed up the process also (when the user picks just existing ones).

I'll create an issue to keep track of this improvement; for now, this fix here should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is a simple fix, I sneaked it in c77d166.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @itsmeichigo for the additional improvement, this looks good, but my remark touches another point, currently the logic of iOS always triggers an API request to create the tags, but this should be needed only when the user adds a new one that doesn't already exist, when the user picks tags from the suggestions, we don't need to re-create them. The following screen recording shows what I mean (I'm selecting an existing tag, and yet we show a loading indicator and we send an API request):

Simulator.Screen.Recording.-.iPhone.16.-.2024-11-29.at.18.32.01.mp4

I created this ticket #14555 to keep track of this.

@itsmeichigo
Copy link
Contributor Author

Thanks @hichamboushaba for the helpful feedback!

One point though, it seems the cached data is not used anywhere, we always fetch tags from remote before showing them, I think a good improvement to do to this screen is to update it to show the cached data first, WDYT?

Nice observation! This is a simple fix also so I added an update in cd4946a.

I also updated the release notes and PR description to take into account the new update. Please let me know if there is anything else needs updating.

@hichamboushaba hichamboushaba merged commit 84339c5 into trunk Nov 29, 2024
15 checks passed
@hichamboushaba hichamboushaba deleted the task/14091-product-tag-store-update branch November 29, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: add/edit products Related to adding or editing products. feature: product list Related to the product list. type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Product: Error message is displayed when clearing all tags even though saving succeeds
3 participants