-
Notifications
You must be signed in to change notification settings - Fork 120
[Local catalog] Catalog Needs Refreshing Warning #16309
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
[Local catalog] Catalog Needs Refreshing Warning #16309
Conversation
The user may have synced their catalog in settings.
|
|
There was a problem hiding this 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:
- Go to POS Settings and refresh the catalog
- 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:
- (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.
- (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.
| #expect(mockIncrementalSyncService.startIncrementalSyncCallCount == 1) | ||
| } | ||
|
|
||
| // MARK: - isSyncStale Tests |
There was a problem hiding this comment.
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())! |
There was a problem hiding this comment.
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)
| 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 { |
There was a problem hiding this comment.
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?
|
Thanks for the review!
So sorry for this... it's because I forgot to push 0c53ee1. I'll look at the other points now. |
I couldn't repro this either... |
…sing-catalog-when' into woomob-1111-woo-poslocal-catalog-add-catalog-needs-refreshing-warning

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.minutecomponent.Screenshots
catalog.sync.warning.mp4
RELEASE-NOTES.txtif necessary.