[Woo POS][Local Catalog] Skip manual one-time sync on site switch if it's not needed #14864

samiuelson · 2025-10-30T12:38:33Z

Description

This PR refactors the triggerManualFullCatalogSync() method to check sync requirements before triggering a full catalog sync, preventing unnecessary sync operations when the catalog is already up to date or when the local catalog is disabled.

Test Steps

The feature can be tested by switching back and forth between sites and observing if the full sync is performed.

Images/gif

N/A

I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

wpmobilebot · 2025-10-30T12:43:31Z

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

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`d63f43d`
	Direct Download	`woocommerce-wear-prototype-build-pr14864-d63f43d.apk`

wpmobilebot · 2025-10-30T12:55:51Z

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

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`d63f43d`
	Direct Download	`woocommerce-prototype-build-pr14864-d63f43d.apk`

codecov-commenter · 2025-10-30T13:06:29Z

Codecov Report

❌ Patch coverage is 85.71429% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.27%. Comparing base (00a1930) to head (d63f43d).
⚠️ Report is 62 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...oopos/localcatalog/WooPosLocalCatalogSyncWorker.kt	86.36%	1 Missing and 2 partials ⚠️
...os/localcatalog/WooPosLocalCatalogSyncScheduler.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##              trunk   #14864   +/-   ##
=========================================
  Coverage     38.27%   38.27%           
- Complexity    10117    10119    +2     
=========================================
  Files          2140     2140           
  Lines        121101   121115   +14     
  Branches      16599    16602    +3     
=========================================
+ Hits          46346    46358   +12     
- Misses        70046    70047    +1     
- Partials       4709     4710    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull Request Overview

This PR refactors the triggerManualFullCatalogSync() method to check sync requirements before triggering a full catalog sync, preventing unnecessary sync operations when the catalog is already up to date or when local catalog is disabled.

Added WooPosFullSyncStatusChecker dependency to determine if sync is actually needed
Modified sync trigger logic to conditionally execute based on sync requirement status
Enhanced logging with context-specific messages for different sync requirement scenarios

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…-full-sync-on-site-switch

malinajirka

Thanks for working on this @samiuelson!

I'd like to propose an alternative approach and hear your thoughts about it. I'm thinking it might be easier to follow the code, if we had these checks within the worker instead of having them in the scheduler for two reasons:

Single source of truth - skipping sync because the data are up to date is similar to skipping it because POS is inactive, and this logic currently lives in the worker itself.
Skipping sync when the check sync requirements fails feels off to me - I believe we should retry it in such case. The worker handles this scenario already, since we run retry() on an unexpected error. However, this PR doesn't handle this scenario I believe.

No matter which approach we decide to go with, I have a couple more thoughts:

We skip when sync is NotRequired => however, NotRequired is returned for any site that hasn't been synced in 0-6.999 days. Is this intentional or should we run it even if it has been run more than 24 hours ago?
logger.d("Manual full sync skipped: Catalog is up to date") feels slightly misleading, I'd suggesting updating the log to Catalog was updated on xyz.. Wdyt?

…-full-sync-on-site-switch

Moves the logic for checking if a full catalog sync is required from the `WooPosLocalCatalogSyncScheduler` to the `WooPosLocalCatalogSyncWorker`. This change simplifies the scheduler by making it responsible only for enqueuing the sync work, while the worker itself now determines if the sync should proceed or be skipped based on the current status.

…-do-not-run-full-sync-on-site-switch' into woomob-1327-woo-poslocal-catalog-do-not-run-full-sync-on-site-switch

…-full-sync-on-site-switch

Copilot

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…-full-sync-on-site-switch

…-do-not-run-full-sync-on-site-switch' into woomob-1327-woo-poslocal-catalog-do-not-run-full-sync-on-site-switch

samiuelson · 2025-11-04T12:42:02Z

Thank you for thorough review, @malinajirka!

I'd like to propose an alternative approach and hear your thoughts about it. I'm thinking it might be easier to follow the code, if we had these checks within the worker instead of having them in the scheduler for two reasons:

Single source of truth - skipping sync because the data are up to date is similar to skipping it because POS is inactive, and this logic currently lives in the worker itself.

Skipping sync when the check sync requirements fails feels off to me - I believe we should retry it in such case. The worker handles this scenario already, since we run retry() on an unexpected error. However, this PR doesn't handle this scenario I believe.

I agree. I addressed this in: 9f2a174

No matter which approach we decide to go with, I have a couple more thoughts:

We skip when sync is NotRequired => however, NotRequired is returned for any site that hasn't been synced in 0-6.999 days. Is this intentional or should we run it even if it has been run more than 24 hours ago?

IMO this is a bug. I addressed it in a separate PR on top of this PR's feature branch: #14894. Please let me know if you agree.

logger.d("Manual full sync skipped: Catalog is up to date") feels slightly misleading, I'd suggesting updating the log to Catalog was updated on xyz.. Wdyt?

I improved this within 9f2a174 as well.

malinajirka

Thanks @samiuelson! It works as expected, I've left a couple non-blocking comments.

malinajirka · 2025-11-05T05:51:43Z

...c/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncWorker.kt


+        checkSyncRequirement()?.let { return it }
+
        val site = isCatalogSyncSupported() ?: return Result.failure()


Is this check still needed when we are using the checker? Seems like there is some duplication in the status checks, wdyt?

Refactored in f0726ab and f95953e

malinajirka · 2025-11-05T05:52:00Z

...c/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncWorker.kt

    }

+    @Suppress("ReturnCount")
+    private suspend fun checkSyncRequirement(): Result? {


It'd be nice to add unit tests for this logic, wdyt?

Added: 12d1651

…-full-sync-on-site-switch

Skip manual one-time sync if not needed

7d27182

samiuelson added type: task An internally driven task. feature: point of sale POS project labels Oct 30, 2025

samiuelson added this to the 23.6 milestone Oct 30, 2025

samiuelson requested a review from Copilot October 30, 2025 13:14

Copilot AI reviewed Oct 30, 2025

View reviewed changes

samiuelson modified the milestones: 23.6, 23.7 Oct 30, 2025

Merge branch 'trunk' into woomob-1327-woo-poslocal-catalog-do-not-run…

e1c3dd6

…-full-sync-on-site-switch

samiuelson marked this pull request as ready for review October 31, 2025 15:58

samiuelson requested a review from malinajirka October 31, 2025 16:13

malinajirka self-assigned this Nov 3, 2025

malinajirka reviewed Nov 3, 2025

View reviewed changes

samiuelson added 4 commits November 3, 2025 17:59

Merge branch 'trunk' into woomob-1327-woo-poslocal-catalog-do-not-run…

1a4dd34

…-full-sync-on-site-switch

Merge remote-tracking branch 'origin/woomob-1327-woo-poslocal-catalog…

2bc0265

…-do-not-run-full-sync-on-site-switch' into woomob-1327-woo-poslocal-catalog-do-not-run-full-sync-on-site-switch

Merge branch 'trunk' into woomob-1327-woo-poslocal-catalog-do-not-run…

79f4e74

…-full-sync-on-site-switch

samiuelson requested a review from Copilot November 4, 2025 10:28

Copilot AI reviewed Nov 4, 2025

View reviewed changes

samiuelson added 4 commits November 4, 2025 12:09

Merge branch 'trunk' into woomob-1327-woo-poslocal-catalog-do-not-run…

d92a51f

…-full-sync-on-site-switch

Pass correct site id

0163d51

Update tests

2412222

Merge remote-tracking branch 'origin/woomob-1327-woo-poslocal-catalog…

2568ebc

…-do-not-run-full-sync-on-site-switch' into woomob-1327-woo-poslocal-catalog-do-not-run-full-sync-on-site-switch

samiuelson requested a review from malinajirka November 4, 2025 12:42

malinajirka approved these changes Nov 5, 2025

View reviewed changes

Return error instead of throwing in case site is null

f0726ab

samiuelson added 3 commits November 5, 2025 12:24

Remove the redundant isLocalCatalogSupported dependency

f95953e

Add tests

12d1651

Merge branch 'trunk' into woomob-1327-woo-poslocal-catalog-do-not-run…

d63f43d

…-full-sync-on-site-switch

samiuelson enabled auto-merge November 5, 2025 11:43

samiuelson merged commit d41963c into trunk Nov 5, 2025
18 checks passed

samiuelson deleted the woomob-1327-woo-poslocal-catalog-do-not-run-full-sync-on-site-switch branch November 5, 2025 11:58


		checkSyncRequirement()?.let { return it }

		val site = isCatalogSyncSupported() ?: return Result.failure()

[Woo POS][Local Catalog] Skip manual one-time sync on site switch if it's not needed #14864

[Woo POS][Local Catalog] Skip manual one-time sync on site switch if it's not needed #14864

Uh oh!

Conversation

samiuelson commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test Steps

Images/gif

Uh oh!

wpmobilebot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

samiuelson commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

malinajirka Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

samiuelson Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

malinajirka Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

samiuelson Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

samiuelson commented Oct 30, 2025 •

edited

Loading

wpmobilebot commented Oct 30, 2025 •

edited

Loading

wpmobilebot commented Oct 30, 2025 •

edited

Loading

codecov-commenter commented Oct 30, 2025 •

edited

Loading

samiuelson commented Nov 4, 2025 •

edited

Loading