Generalize currency conversion #231
Conversation
|
(Test fixes comin.) |
| pledged: Int($0 * Float($1.goal)), staticUsdRate: $1.staticUsdRate, updatesCount: $1.updatesCount) } | ||
| set: { .init(backersCount: $1.backersCount, commentsCount: $1.commentsCount, | ||
| currentCurrency: $1.currentCurrency, currentCurrencyRate: $1.currentCurrencyRate, | ||
| goal: $1.goal, pledged: Int($0 * Float($1.goal)), staticUsdRate: $1.staticUsdRate, |
There was a problem hiding this comment.
Feels a little strange that this bit Int($0 * Float($1.goal)) is in the lens, should we move it to the model? 🤔
There was a problem hiding this comment.
This is a lens we already had on fundingProgress, which is a computed property on our model. Gotta have the inverse logic here to update the type accordingly.
| } | ||
| .map { project, rewardOrBacking in | ||
| let (country, rate) = zip( | ||
| project.stats.currentCurrency.flatMap(Project.Country.init(currencyCode:)), |
There was a problem hiding this comment.
Compiler clever enough here for .flatMap(.init(currencyCode:))?
There was a problem hiding this comment.
I don't think so. Since function isn't its own type and this is point-free.
| @@ -211,7 +211,6 @@ internal final class AppDelegate: UIResponder, UIApplicationDelegate { | |||
|
|
|||
| return self.viewModel.outputs.applicationDidFinishLaunchingReturnValue | |||
| } | |||
| // swiftlint:enable function_body_length | |||
There was a problem hiding this comment.
While this PR was goin on swiftlint 0.22.0 dropped, which now checks for stale linter rules. It doesn't find these trailing ones, so I did a quick manual check based on our disabled rules.
| lhs.isLoggedIn == rhs.isLoggedIn && | ||
| lhs.isMember == rhs.isMember | ||
| extension TabBarItemsData: Equatable { | ||
| static func == (lhs: TabBarItemsData, rhs: TabBarItemsData) -> Bool { |
There was a problem hiding this comment.
Quick lil static promotion. Bunch more of these we could update in the future. Cleans things mostly up when generic types are involved (since you can omit the generic params).
| @@ -38,7 +37,7 @@ final class UpdatePreviewViewModelTests: TestCase { | |||
| self.vm.inputs.viewDidLoad() | |||
|
|
|||
| let previewUrl = "https://\(Secrets.Api.Endpoint.production)/projects/2/updates/1/preview" | |||
| let query = "client_id=\(self.apiService.serverConfig.apiClientAuth.clientId)" | |||
| let query = "client_id=\(self.apiService.serverConfig.apiClientAuth.clientId)¤cy=USD" | |||
There was a problem hiding this comment.
Right now our way of sending device currency (from the region) is to include it as a param. This means web views should get currency for free. Alternatively we could have synced currency via config, but this could have slowed things during load.
| @@ -54,7 +54,7 @@ internal final class ProjectActivityViewControllerTests: TestCase { | |||
| } | |||
|
|
|||
| func testVoiceOverRunning() { | |||
| withEnvironment(isVoiceOverRunning: { true }) { | |||
| withEnvironment(isVoiceOverRunning: const(true)) { | |||
There was a problem hiding this comment.
New swiftlint rule requires that functions with multiple function parameters avoids trailing closure syntax. For things like either(ifLeft: { … }, ifRight: { … }) I like this, so keeping it enabled and switching these to use const.
| @@ -213,8 +213,6 @@ public final class ProjectPamphletContentViewController: UITableViewController { | |||
| fileprivate func scrollingIsAllowed(_ scrollView: UIScrollView) -> Bool { | |||
| return self.presentingViewController?.presentedViewController?.isBeingDismissed != .some(true) | |||
| && (!scrollView.isTracking || scrollView.contentOffset.y >= 0) | |||
| // swiftlint:disable:previous force_unwrapping | |||
| // NB: this ^ shouldn't be necessary, looks like a bug in swiftlint. | |||
| @@ -50,6 +50,8 @@ public struct Project { | |||
| public struct Stats { | |||
| public let backersCount: Int | |||
| public let commentsCount: Int? | |||
| public let currentCurrency: String? | |||
| public let currentCurrencyRate: Float? | |||
There was a problem hiding this comment.
Our API now returns the "current" currency of the response (which should be pass-through from the request), and the exchange rate against the project currency.
| @@ -240,7 +240,8 @@ public struct AppEnvironment { | |||
| basicHTTPAuth: service.serverConfig.basicHTTPAuth | |||
| ), | |||
| oauthToken: service.oauthToken, | |||
| language: current.language.rawValue | |||
| language: current.language.rawValue, | |||
| currency: current.locale.currencyCode ?? "USD" | |||
There was a problem hiding this comment.
We could do a more graceful regional fallback here...but not sure how to manage that.
| !needsConversion(projectCountry: $0.country, userCountry: AppEnvironment.current.config?.countryCode) | ||
| } | ||
|
|
||
| self.conversionLabelHidden = project.map(needsConversion(project:) >>> negate) |
There was a problem hiding this comment.
This is the meat of the new logic! We generalize the old and new paths (with or without the API returning current_currency to a needsConversion(project:).
Cleaned things up by composing this against negate, and composing it in the filter below against first.
| .map { project, rewardOrBacking in | ||
| let (country, rate) = zip( |
There was a problem hiding this comment.
Fun bit of FP: Optional zip coalescing to a default pair.
| return needsConversion(projectCountry: project.country, | ||
| userCountry: AppEnvironment.current.config?.countryCode) | ||
| } | ||
| return currentCurrency == AppEnvironment.current.apiService.currency |
There was a problem hiding this comment.
If our API ever returns a different currency than the one we send along, we fall back to USD here. We may wanna reconsider that logic, though?
There was a problem hiding this comment.
Could we not keep these defaults on the Config and lens them in the first time?
There was a problem hiding this comment.
Ah, I mistyped. I mean we don't show any conversion info.
Adds support for currency rates from our API.