[Performance Dashboard Card]: Update cached data display logic

hafizrahman commented 5 days ago

Closes: #14436

Description

Please have a look at the issue https://github.com/woocommerce/woocommerce-ios/issues/14436 for more details. But, essentially, this PR allows for cached data on Performance card to be displayed correctly if it is decided that it's too early to remotely fetch data again.

Steps to reproduce

Have a demo site with a newly completed Order within this month, so stats can appear
Start app,
Enable Performance card, and set range to This Month.
Wait until data appears properly (Orders, Visitors, and Conversion), then quickly build the app again in Xcode.
This will start the app again. Ensure Visitors and Conversions values are shown.

You can also contrast this by testing the same flow in trunk to check the issue. In step 5, the values will be hidden.

Testing information

I tested this in Simulator iPhone 16 with iOS 18.

[x] 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:

[x] 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.
[x] Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
[x] 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.

wpmobilebot commented 5 days ago

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

	App Name	WooCommerce iOS
	Build Number	pr14438-3a02b98
	Version	21.1
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	3a02b98c10ad668ea5c7c17e836a6d3cbf6905be
	App Center Build	WooCommerce - Prototype Builds #11663

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

hafizrahman commented 5 days ago

@selanthiraiyan on 96620e2f8c3b456481c5d75c6959063cffbb4f0e I attempted to create a unit test for this, but it's not working yet.

This part especially

https://github.com/woocommerce/woocommerce-ios/pull/14438/files#diff-fd13b018fba799bd2a5dbc78450971395882bf27f2aa290f21aa54d8e66a9d74R408-R409

        mockStorageManager.insertSampleSiteVisitStats(siteVisitStats)
        mockStorageManager.insertSampleOrderStats(orderStats)

Does not seem to insert the mock data correctly, while mockStorageManager.insertSampleSiteSummaryStats(summaryStats) is working fine.

I can see the insert is not working because when StoreStatsPeriodViewModel.noDataFound is being called, siteStatsResultsController.isEmpty || orderStatsResultsController.isEmpty are both true. Meanwhile summaryStatsResultsController.isEmpty returns false.

I can't figure out how to confirm whether the mock data is saved correctly or not. Because this is a unit test, I can't use an app to check the CoreData sqlite value. Any idea how to check that?

selanthiraiyan commented 4 days ago

👋 @hafizrahman,

I can't figure out how to confirm whether the mock data is saved correctly or not. Because this is a unit test, I can't use an app to check the CoreData sqlite value. Any idea how to check that?

You could use this method to load all stored objects of a particular kind and check the count. Example

You can wrap the above check inside a waitUntil block to proceed after the storage is ready with saved items. What do you think? Does this help?

hafizrahman commented 3 days ago

@selanthiraiyan Thanks for the suggestion. While working on your suggestion, I discovered that StoreStatsPeriodViewModelTests has helper methods for saving, similar to what's needed in the unit test here, so I end up reusing that. https://github.com/woocommerce/woocommerce-ios/pull/14438/commits/b39cee05167c412f439734ad9d6b977d5c03b0bb

This is good for reviewing now.

hafizrahman commented 3 days ago

@selanthiraiyan

I tested by using "This Year" as the time range filter value.

This seems to be a new bug that is uncovered after the duplicate storage was fixed on 29a8deb2565f23a4edd2f98b6b69778bd74152d4

I can replicate it like so:

Start app, login to site freshly (can switch from another site to it),
Switch to This Year in Performance card,
Confirm that the data is shown correctly,
Rebuild app,
Check Performance card again, confirm that the visitors and conversion data are now redacted.

From debugging it seems to have to do with endOfYear() logic here:

https://github.com/woocommerce/woocommerce-ios/blob/09e8f0e1f2acd92d5dca596935ba821185e74904/WooFoundation/WooFoundation/Extensions/Date%2BStartAndEnd.swift#L146-L156

So when rebuilding app and using the "This Year" time range, when this is called:

timeRange.latestDate returns 2025-01-01, while the existing cached data is set correctly at 2024-31-12. This then resulted in nothing being found in database, so nothing is shown. I'm still investigating further.

hafizrahman commented 2 days ago

@selanthiraiyan I investigated the issue further and can confirm that:

I can replicate on your test site, but not on my test site.
the issue seems to be triggered when the timezone discrepancy is different between device timezone and site timezone (e.g: my device timezone is UTC+7 and the site timezone is UTC-6).
On my test site when both site and device timezone is UTC+7, there is no issue.
The problem seems to be that on initial load, the site fetches using device timezone because ServiceLocator.stores.sessionManager.defaultSite?.siteTimezone doesn't exist yet. Then on rebuild, the site timezone now exists and is used to get cached data, but the calculation becomes off by 1. As the timezone is used for the query to fetch site summary data, it then fails to fetch correctly.
The issue also happens in trunk, meaning it is not introduced by this PR.

Given that, what do you think if with this PR you re-review with a site that has a similar timezone as device timezone? Because the screenshot PR depends on this PR, I think it would be better if we merge this PR first. It already contains a partial fix for sites where device and site timezone is the same, which is enough for the screenshot fix.

I will open a new issue for the timezone discrepancy problem, and it can be fixed separately. What do you think?

woocommerce / woocommerce-ios