Skip to content

Conversation

@joshheald
Copy link
Contributor

@joshheald joshheald commented Nov 3, 2025

Description

This PR shows a notice banner at the top of the item list when the last full sync was 7 days or longer ago.

The banner can be dismissed, and will be shown again when you next launch POS.

Refreshing the catalog in settings will result in the banner being dismissed.

Test Steps

To make it easier to test, update the POSCatalogSyncCoordinator.isSyncStale(for:maxDays:) function to use the .minute component.

  1. Launch the app and open POS
  2. If it's been more than 7 minutes since your last full sync, the banner will be shown
  3. Dismiss the banner
  4. Make an order and take payment – observe that when you go back to the item list, the banner's still hidden.
  5. Leave POS and go back in – observe that the banner is shown again.
  6. Go to POS Settings and refresh the catalog
  7. Observe that the banner is hidden when you leave settings.
  8. Leave POS and go back in, observe that the banner is still hidden.

Screenshots

catalog.sync.warning.mp4

  • 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.7 milestone Nov 3, 2025
@joshheald joshheald requested a review from jaclync November 3, 2025 13:55
@joshheald joshheald added type: task An internally driven task. feature: POS labels Nov 3, 2025
@joshheald joshheald changed the base branch from trunk to woomob-1565-woo-poslocal-catalog-loading-screen-for-missing-catalog-when November 3, 2025 13:55
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 3, 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 Numberpr16309-8b959bf
Version23.6
Bundle IDcom.automattic.alpha.woocommerce
Commit8b959bf
Installation URL7m9ksa5hkil8o
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

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.

LGTM, an issue during testing but feel free to merge after addressing it 🚢

While testing in iPad A16 iOS 18.5 simulator with Xcode 26, with the threshold changed to 1 minute:

  1. Go to POS Settings and refresh the catalog
  2. Observe that the banner is hidden when you leave settings.

In step 7, the banner was still shown after refreshing the catalog successfully and leaving the POS settings. From setting a breakpoint on await posModel.checkStaleSyncStatus() in ItemListView, the item list view's async closure was not invoked after leaving the settings screen. The banner was hidden after exiting POS and re-entering POS.


Other observations that can be addressed separately:

  1. (Accessibility) With the largest font size, the banner could take up the whole height in certain devices and languages (English and iPad A16 iOS 18.5 for example). The list wasn't visible in landscape orientation.
Image
  1. (Minor, not reproducible) One time, the bottom of the item list overlaps with the floating control a bit. I couldn't reproduce it though, and it could be pre-existing.
Image

#expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 1)
}

// MARK: - isSyncStale Tests
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: test_ prefix isn't necessary with Swift Testing in this file, it's confusing when writing with both XCTest and Swift Testing 😅


@Test func test_isSyncStale_returns_false_when_full_sync_is_recent() async throws {
// Given - last full sync was 3 days ago
let threeDaysAgo = Calendar.current.date(byAdding: .day, value: -3, to: Date())!
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can use try #require instead of explicitly unwrapping (similarly for other ! usage in this file)

Suggested change
let threeDaysAgo = Calendar.current.date(byAdding: .day, value: -3, to: Date())!
let threeDaysAgo = try #require(Calendar.current.date(byAdding: .day, value: -3, to: Date()))

#expect(isStale == false)
}

@Test func test_isSyncStale_boundary_past_threshold() async throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this test case feels similar to test_isSyncStale_returns_true_when_full_sync_is_old, maybe the last full sync could be like 1 second past the threshold or exactly at the threshold?

@joshheald joshheald marked this pull request as ready for review November 4, 2025 10:46
@joshheald
Copy link
Contributor Author

Thanks for the review!

In step 7, the banner was still shown after refreshing the catalog successfully and leaving the POS settings. From setting a breakpoint on await posModel.checkStaleSyncStatus() in ItemListView, the item list view's async closure was not invoked after leaving the settings screen. The banner was hidden after exiting POS and re-entering POS.

So sorry for this... it's because I forgot to push 0c53ee1.

I'll look at the other points now.

@joshheald
Copy link
Contributor Author

2. (Minor, not reproducible) One time, the bottom of the item list overlaps with the floating control a bit. I couldn't reproduce it though, and it could be pre-existing.

I couldn't repro this either...

…sing-catalog-when' into woomob-1111-woo-poslocal-catalog-add-catalog-needs-refreshing-warning
Base automatically changed from woomob-1565-woo-poslocal-catalog-loading-screen-for-missing-catalog-when to trunk November 5, 2025 08:16
@joshheald joshheald enabled auto-merge November 5, 2025 08:16
@joshheald joshheald merged commit ea5ebd9 into trunk Nov 5, 2025
15 checks passed
@joshheald joshheald deleted the woomob-1111-woo-poslocal-catalog-add-catalog-needs-refreshing-warning branch November 5, 2025 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: POS type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants