Skip to content

Conversation

@joshheald
Copy link
Contributor

Description

This PR adds a reset function which is called on logout, to delete data from all tables when someone logs out of the app.

Steps to reproduce

Check unit tests pass.

I tested by adding the following to the top of POSCatalogSyncCoordinator.shouldPerformFullSync(for:maxAge:):

try? await grdbManager.databaseConnection.read { db in
            let productCount = try PersistedProduct.fetchCount(db)
            let productImageCount = try PersistedProductImage.fetchCount(db)
            let productAttributeCount = try PersistedProductAttribute.fetchCount(db)
            let variationCount = try PersistedProductVariation.fetchCount(db)
            let variationImageCount = try PersistedProductVariationImage.fetchCount(db)
            let variationAttributeCount = try PersistedProductVariationAttribute.fetchCount(db)

            DDLogInfo("Initially found \(productCount) products, \(productImageCount) product images, " +
                      "\(productAttributeCount) product attributes, \(variationCount) variations, " +
                      "\(variationImageCount) variation images, \(variationAttributeCount) variation attributes")
        }
  1. Launch the app and select a POS eligible site
  2. Wait until the sync completes and is persisted
  3. Log out, and then log back in to the same site
  4. When you log in, observe that all the counts logged are 0

Screenshots


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

@joshheald joshheald added this to the 23.3 milestone Sep 10, 2025
@joshheald joshheald requested a review from jaclync September 10, 2025 13:56
@joshheald joshheald added type: task An internally driven task. status: feature-flagged Behind a feature flag. Milestone is not strongly held. feature: POS labels Sep 10, 2025
@joshheald joshheald force-pushed the woomob-1322-woo-poslocal-catalog-remove-stored-data-on-logout branch from 9a3eaac to c8889d7 Compare September 10, 2025 13:59
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 10, 2025

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

App NameWooCommerce iOS Prototype
Build Numberpr16114-e931cbe
Version23.2
Bundle IDcom.automattic.alpha.woocommerce
Commite931cbe
Installation URL34iq10amlp588
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

…that it does not call `DefaultStoresManager.deauthenticate` which accesses ServiceLocator dependencies.
…bled by only resetting `ServiceLocator.grdbManager` when feature flag is enabled. `MockStoresManager.deauthenticate` depends on `DefaultStoresManager` implementation for some auth related test cases.
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.

I tested by printing the database path and checking all the tables were non-empty before logout, then became empty after logout. :shipit:

Hope you don't mind my attempts to fix the unit test failure on test_user_is_logged_out_when_tapping_secondary_button, feel free to change the fix. I was able to reproduce the crash by running the WooCommerceTests suite as a whole (running individual test class passed). The crash was from the fatalError on catalog i1 feature flag disabled while accessing ServiceLocator.grdbManager.

WooCommerce/ServiceLocator.swift:203: Fatal error: GRDBManager accessed when pointOfSaleLocalCatalogi1 feature flag is disabled

because MockStoresManager in WooCommerceTests does not override deauthenticate, it calls DefaultStoresManager.deauthenticate which calls ServiceLocator.grdbManager.

Overriding MockStoresManager.deauthenticate as my first attempt didn't work, because some test cases in WooCommerceTests depend on the DefaultStoresManager.deauthenticate implementation to update the authentication state. As we don't have a good instance to return when the catalog i1 feature is disabled in ServiceLocator.grdbManager, I updated the GRDB reset to only be executed when the same feature flag is enabled.

Comment on lines +587 to +591
#expect(countsBefore.sites == 2) // Original site + new site
#expect(countsBefore.products == 4)
#expect(countsBefore.variations == 4)
#expect(countsBefore.productAttributes == 4)
#expect(countsBefore.variationAttributes == 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: how about checking the product/variation images as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we'll start adding every entity to this function in future, so it seems over the top to add all of them now...

@joshheald
Copy link
Contributor Author

Hope you don't mind my attempts to fix the unit test failure on test_user_is_logged_out_when_tapping_secondary_button, feel free to change the fix. I was able to reproduce the crash by running the WooCommerceTests suite as a whole (running individual test class passed). The crash was from the fatalError on catalog i1 feature flag disabled while accessing ServiceLocator.grdbManager.

Not at all, makes total sense.

It didn't fail for me locally at all, glad you could reproduce it. I wonder whether my simulator had some leftover feature flag state or something...

@joshheald joshheald merged commit d17e8a4 into trunk Sep 11, 2025
14 checks passed
@joshheald joshheald deleted the woomob-1322-woo-poslocal-catalog-remove-stored-data-on-logout branch September 11, 2025 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: POS status: feature-flagged Behind a feature flag. Milestone is not strongly held. type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants