-
Notifications
You must be signed in to change notification settings - Fork 16
New Stats #843
New Stats #843
Conversation
Generated by 🚫 Danger |
| uniqueOpens = try container.decodeIfPresent(Int.self, forKey: .uniqueOpens) | ||
| totalOpens = try container.decodeIfPresent(Int.self, forKey: .totalOpens) | ||
| opensRate = try container.decodeIfPresent(Double.self, forKey: .opensRate) | ||
| } |
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.
Do we need this function? It's the same as the default derived implementation, if I'm not mistaken?
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.
No really - removing.
| } | ||
|
|
||
| extension StatsEmailOpensData { | ||
| public init?(jsonDictionary: [String: AnyObject]) { |
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.
throw instead of silencing the error?
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.
I auto-generated this code with the rest of the file as a reference. It is consistent with the rest of StatsServiceRemoteV2 and how it's used. I'd suggest keeping it as it's throw-away code anyway.
| public let totalSends: Int? | ||
| public let uniqueOpens: Int? | ||
| public let totalOpens: Int? | ||
| public let opensRate: Double? |
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.
Should all these properties be null? Will the response ever be null, like no total_sends or total_send: null in the response?
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.
I'd change it, but it's a throw-away code, so I'm not sure it's worth reworking it and the usage. I'm not sure I have any energy left at this point 😪
|
|
||
| static var hourlyDateFormatter: DateFormatter { | ||
| let df = DateFormatter() | ||
| df.locale = Locale(identifier: "en_US_POS") |
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.
Is the id supposed to be en_US_POSIX?
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.
Updated. This code is unused – the new stats don't request hourly data for subscribers and there is no hourly data for subscribers. The existing formatters in this screen use en_US_POS. I didn't want to make changes to the existing code in the scope of this change.
| let dateFormatter = makeDateFormatter(for: period) | ||
|
|
||
| self.data = data.compactMap { data in | ||
| guard let periodDate = dateFormatter.date(from: data[periodIndex] as? String ?? "") else { |
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.
Nitpick:
| guard let periodDate = dateFormatter.date(from: data[periodIndex] as? String ?? "") else { | |
| guard let date = data[periodIndex] as? String, let periodDate = dateFormatter.date(from: date) else { |
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.
Applied.
|
|
||
| let dateFormatter = DateFormatter() | ||
| dateFormatter.locale = Locale(identifier: "en_US_POSIX") | ||
| dateFormatter.dateFormat = "yyyy-MM-dd HH:mm:ss" |
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.
You'd need to set the timeZone property to parse GMT date strings that do not have a time zone in them, right?
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.
This is an existing issue across all existing decodes in this service. The date is actually returned in the site time zone. The decoders don't know what is the time zone of the site. The response also doesn't contain it. So it end's up creating a wrong timestamp in your local time zone.
I added code to map it in StatsService https://github.com/wordpress-mobile/WordPress-iOS/blob/b82374372485a73d0a3f49bc5cbc3886a3179767/Modules/Sources/JetpackStats/Services/StatsService.swift#L355, but I think I made a mistake at one of the layers. I opened a ticket and will debug it later on Monday: https://linear.app/a8c/issue/CMM-642/dates-are-in-the-wrong-timezone. The fix needs to be on the app level. I want to keep the WPKit behavior as is so it doesn't break the legacy screens.
| var staticProperties = ["period": period.stringValue, | ||
| "unit": unit?.stringValue ?? period.stringValue, | ||
| "date": periodDataQueryDateFormatter.string(from: endingOn)] as [String: AnyObject] | ||
| "date": dateFormatter.string(from: endingOn)] as [String: AnyObject] |
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.
Nitpick: I think you can get rid of all the as statements below if you declare the staticProperties as [String: Any].
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.
Yeah, it support Any, but I just didn't want to change it since it's the existing code. There was already too many changes, and I wanted to keep the changes to the existing code minimum.
| // MARK: - Email Opens | ||
|
|
||
| public extension StatsServiceRemoteV2 { | ||
| func getEmailOpens(for postID: Int, completion: @escaping ((StatsEmailOpensData?, Error?) -> Void)) { |
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.
What do you think about using async functions instead of calls for the new API calls? I presume the callers are written in Swift too, which may prefer async functions?
| posts: fields.firstIndex(of: Metric.posts.rawValue) | ||
| ) | ||
|
|
||
| let dateFormatter = makeDateFormatter(for: period) |
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.
There is this comment above: "/// Periods date in the site timezone."
It's impossible to parse a date string if we don't know what the site timezone is, right? So, it's incorrect to use any DateFormatter instance. I think the correct solution is either passing the site timezone from the app1 to the parsing function here, or returning a plain string and letting the app parse it.
Footnotes
-
Or, making another HTTP request to get site timezone, which does feel like wasteful. ↩
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.
I'll revisit this in the scope of #843 (comment). The app knows the time zone, so it will need to perform the conversion.
In reality, StatsServiceRemoteV2 simply uses incorrect types to represent dates. If It doesn't know what time zone the dates component belong to, it should never returned Date (timestamp). It ends up always creating wrong time stamps (in your app current time zone instead of the site time zone). Unfortunately, it is what it is.
|
There are merge conflicts and failing unit tests. |
|
I merged |
New Features
StatsEmailOpensDatamodelStatsArchiveTimeIntervalDatafor generalized archive endpoint parsingStatsPeriodUnit.hourto enable hourly statistics dataStatsSiteMetricsResponsefor improved site metrics handlingAPI Enhancements
StatsServiceRemoteV2.getDatawithstartDate,timeZone, andfieldsparameterssummarizeparameter support for stats requestsStatsPostDetailsto include all available data fieldsWordPressKit.zip