Stats Refresh: Insights fetching

[jklausa]

Description

This is a first step in rewrite of WPStatsServiceRemote, as a part of "Full Stats Refresh".

The old code was very complex, hard to read, obj-c, and did things that arguably don't belong in networking layer (localizing strings! date formatting! keeping track of which requests are currently in flight!).

This is an attempt at simplifying it, specifically addressing fetching "Insights" for now, and with time-series data to follow shortly.

The core idea of this design is a simple protocol and generic function combo in StatsServiceRemoteV2:

public func getInsight<InsightType: InsightProtocol>(completion: @escaping ((InsightType?, Error?) -> Void))

public protocol InsightProtocol {
    static var queryProperties: [String: AnyObject] { get }
    static var pathComponent: String { get }

    init?(jsonDictionary: [String: AnyObject])
}

The protocol provides everything required to fetch a specific kind of insight — the path to where should the reqeust be sent to, optional query properties — and perhaps most importantly, a way to parse the response. Because of this, the code of the StatsServiceRemoteV2 is itself fairly short and simple — most of the gnarly data parsing stuff is delegated to the specific classes conforming to InsightProtocol.

This design also allows for a cleaner call-side implementation — all insights are fetched with a simple call to getInsight(:_), differing only in the generic type being specialized:

api.getInsight { (lastPost: StatsLastPostInsight?, error) in
    dump("hello insight! \(lastPost)")
}

api.getInsight { (allTimesStats: StatsAllTimesInsight?, error) in
     dump("hello all time stats! \(allTimesStats)")       
}

Misc notes:

Some of the actual response parsing in the subclasses is not pretty, due to the weird shape of responses in some of the API endpoints. Not much I could do there :(

I also think I found a weird edge-case in swiftc with the generic design — one specific insight (last post) requires two (mostly unrelated) network calls to compile, and therefore is implemented as a specialization of the generic getInsight(:_).

However, if I then move the StatsLastPostInsight struct definition outside of the same file where StatsServiceRemoteV2 is defined, the project no longer builds, claiming that there's no such type as StatsLastPostInsight in the project! I asked fine folks from platform9 to investigate further if they have some spare cycles this week — if any of y'all have some ideas what might be going on here, I'm all ears!

Testing Details

1. Make sure this PR builds
2. Take a corresponding WPiOS branch for a spin — Switch over most of the insights for new architecture. WordPress-iOS#10994

• Please check here if your pull request includes additional test coverage.

[ghost]

How would you feel about having your implementors conform to Decodable? It might obviate the need for this initializer.

[jklausa]

I desperately wish I could just use Decodable for this, but the underlying layer of WordPressComRestApi already parses the response from Data into a JSON dictionary, and Codable doesn't have a Dictionary Decoder :(

[ghost]

The content of this comment is valid.

From what I can tell, this is the only type you refer to the static members of within this file. That is to say, none of the other eight InsightProtocol-conformers are causing problems because we don't attempt to access them. I tested one in a local branch to confirm.

As for the tone of this comment, I would respectfully request that we moderate it slightly. Instead of:

For some god-forsaken reason 

please consider:

At the moment, I am unable to explain why the

[ghost]

As for the essence of the problem you observed here & rightly noted in the PR description, I tinkered with this a bit, and wanted to report my findings.

I noted that the problematic type (i.e., StatsLastPostInsight) is the one you explicitly reference the static members of. I encountered some difficulty, but ultimately, I wanted to share one approach for your consideration:

1. Create new file StatsLastPostInsight.swift and extract the struct & protocol conformance to it.
2. I created an extension of StatsServiceRemoteV2 in StatsLastPostInsight.swift, and moved the "problem" methods there (i.e., getInsight(completion:), .getLastPostInsight(completion:), and getPostViews(for:completion:).

Bad idea? Worst idea? 😂

[jklausa]

I actually meant to redact that comment before posting this PR, I'm embarassed that I forgot to do that. Thanks for pointing this out!

[ScoutHarris]

I think there's enough from @stevebaranski and I to mark this as Request changes.

[ghost]

@jklausa thanks for taking this on! The PR description looks great! I can confirm that this branch builds.

Absent tests, I checked out the related WPiOS PR and can confirm that values are coming back in StatsInsightStore for each of the calls.

I left a few comments for your consideration. Depending on if/when you'd like to tackle them (e.g., not at all, in a subsequent PR), we can discuss approval & merging.

Please let me know if you have any questions, or need additional information.

[jklausa]

@ScoutHarris @stevebaranski updated and ready for another look!

@stevebaranski re/ StatsLastPostInsight — I'd like to leave it as it is for now — I'll add an issue to the GH project in the WPiOS repo to come back to it and the end of the project.

My hope is that Platform folks will be able to come up with something clever here — if not, your workaround suggestion sounds good to me.

[ScoutHarris]

Thanks @jklausa ! You get a 👍 from me!

[jklausa]

heyo let's get this party started then :P