Skip to content
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

Widgets: Share store TZ settings #7707

Merged
merged 7 commits into from
Sep 13, 2022
Merged

Conversation

ealeksandrov
Copy link
Contributor

Closes: #7564

Description

To make sure we're using store TZ, share it as a parameter to widget extension and use in stats requests.

How

  • Adds defaultStoreTimeZoneGMTOffset userdefault property
  • Moves Date+StartAndEnd extension to shared WooFoundation module
  • Uses shared extension with store timezone in API requests

Note: Testing this is complicated and sometimes I got different stats result in app dashboard and widget.
Stats dashboard uses store TZ for UI, but device TZ for requests:

// Since the stats charts are based on site time zone, we set the time zone for the stats UI to be the site time zone.
// On the other hand, when syncing the stats data with the API, we want to use the device time zone to find the time range since the API date parameters
// have no time zone information and are relative to the site time zone (e.g. 12:00am-11:59pm for "Today" tab).
let timezoneForStatsDates = TimeZone.siteTimezone
let timezoneForSync = TimeZone.current

Should we do the same?

Testing Steps

  • Set timezone on device and store to be very different. In wp-admin it's in Settings -> General -> Timezone.
  • Add orders close to days transitions in both TZs.
  • Launch the app.
  • Compare stats data on app dashboard and widget.

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

@ealeksandrov ealeksandrov added the feature: widgets iOS Homescreen widgets label Sep 12, 2022
@ealeksandrov ealeksandrov added this to the 10.4 milestone Sep 12, 2022
@@ -510,6 +510,7 @@ private extension DefaultStoresManager {
// Non-critical store info
UserDefaults.group?[.defaultStoreID] = siteID
UserDefaults.group?[.defaultStoreName] = sessionManager.defaultSite?.name
UserDefaults.group?[.defaultStoreTimeZoneGMTOffset] = sessionManager.defaultSite?.siteTimezone.secondsFromGMT() ?? TimeZone.current.secondsFromGMT()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This presentation is picked because we init it same way from API response:

/// Returns the TimeZone using the gmtOffset
///
var siteTimezone: TimeZone {
let secondsFromGMT = Int(gmtOffset * 3600)
return TimeZone(secondsFromGMT: secondsFromGMT) ?? .current
}

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 12, 2022

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr7707-eedc08e on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@ealeksandrov ealeksandrov mentioned this pull request Sep 12, 2022
1 task
@Ecarrion Ecarrion self-assigned this Sep 12, 2022
Copy link
Contributor

@Ecarrion Ecarrion left a comment

Choose a reason for hiding this comment

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

Code looks good!

Should we do the same?

I think we should match the way the app fetches & displays information. Because if not the merchant won't know what source of information to trust.

App Widget
IMG_4700 IMG_4701

Since the stats charts are based on site time zone, we set the time zone for the stats UI to be the site time zone.

Wouldn't that means that we don't need the site timezone? As we don't show charts?

@@ -25,10 +25,15 @@ final class StoreInfoDataService {
///
private var network: AlamofireNetwork

init(authToken: String) {
/// Timezone of the website
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Timezone of the website
/// Timezone of the store

@ealeksandrov
Copy link
Contributor Author

ealeksandrov commented Sep 13, 2022

Wouldn't that means that we don't need the site timezone? As we don't show charts?

That's what I think now. Should we close it and maybe get back if we found inconsistencies in testing?

Update: Actually, I think Date+StartAndEnd reuse worth merging. So I reverted TZ sharing (49687de) and updated code to use device TZ (eedc08e).

Copy link
Contributor

@Ecarrion Ecarrion left a comment

Choose a reason for hiding this comment

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

Update: Actually, I think Date+StartAndEnd reuse worth merging. So I reverted TZ sharing (49687de) and updated code to use device TZ (eedc08e).

Agree specially since we recently added some logs due to a new crash around date parsing.

///
func fetchTodaysVisitors(for storeID: Int64) async throws -> SiteVisitStats {
try await withCheckedThrowingContinuation { continuation in
// `WKWebView` is accessed internally, we are foreced to dispatch the call in the main thread.
Task { @MainActor in
siteVisitStatsRemote.loadSiteVisitorStats(for: storeID,
unit: .day,
latestDateToInclude: Date.startOfToday,
latestDateToInclude: Date().endOfDay(timezone: .current),
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow I don't know why I used startOfToday 🤦

@ealeksandrov ealeksandrov merged commit 67b8432 into trunk Sep 13, 2022
@ealeksandrov ealeksandrov deleted the issue/7564-store-tz-settings branch September 13, 2022 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: widgets iOS Homescreen widgets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Widgets] Fetch and Share data store info
3 participants