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

[NT-1450] Allow Dissimilar Shipping Cost for Add-Ons #1257

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,8 @@ final class PledgeAmountViewController: UIViewController {

// MARK: - Accessors

func selectedShippingAmountChanged(to amount: Double) {
self.viewModel.inputs.selectedShippingAmountChanged(to: amount)
func unavailableAmountChanged(to amount: Double) {
self.viewModel.inputs.unavailableAmountChanged(to: amount)
}
}

Expand Down
4 changes: 2 additions & 2 deletions Kickstarter-iOS/Views/Controllers/PledgeViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -329,10 +329,10 @@ final class PledgeViewController: UIViewController,
self?.pledgeCTAContainerView.configureWith(value: value)
}

self.viewModel.outputs.notifyPledgeAmountViewControllerShippingAmountChanged
self.viewModel.outputs.notifyPledgeAmountViewControllerUnavailableAmountChanged
.observeForUI()
.observeValues { [weak self] amount in
self?.pledgeAmountViewController.selectedShippingAmountChanged(to: amount)
self?.pledgeAmountViewController.unavailableAmountChanged(to: amount)
}

self.viewModel.outputs.configureSummaryViewControllerWithData
Expand Down
2 changes: 2 additions & 0 deletions KsApi/GraphSchema.swift
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ public enum Query {
}

public enum ShippingRule {
case cost(NonEmptySet<Money>)
case id
case location(NonEmptySet<Location>)
}
Expand Down Expand Up @@ -580,6 +581,7 @@ extension Query.BankAccount: QueryType {
extension Query.ShippingRule: QueryType {
public var description: String {
switch self {
case let .cost(fields): return "cost { \(join(fields)) }"
case .id: return "id"
case let .location(fields): return "location { \(join(fields)) }"
}
Expand Down
1 change: 1 addition & 0 deletions KsApi/models/Reward+ManagePledgeView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public extension Reward {
remaining: backingReward.remainingQuantity,
rewardsItems: rewardItemsData(from: backingReward, with: project),
shipping: shippingData(from: backingReward),
shippingRules: nil,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment we don't need this data in the ManagePledgeViewModel.

startsAt: backingReward.startsAt,
title: backingReward.name
)
Expand Down
21 changes: 21 additions & 0 deletions KsApi/models/Reward+RewardAddOnSelectionView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public extension Reward {
remaining: reward.remainingQuantity,
rewardsItems: rewardItemsData(from: reward, with: project),
shipping: shippingData(from: reward),
shippingRules: shippingRulesData(from: reward),
startsAt: reward.startsAt,
title: reward.name
)
Expand Down Expand Up @@ -85,3 +86,23 @@ private func shippingData(
type: nil
)
}

private func shippingRulesData(
from reward: RewardAddOnSelectionViewEnvelope.Project.Reward
) -> [ShippingRule]? {
guard let shippingRules = reward.shippingRules else { return nil }

return shippingRules.map { shippingRule in
ShippingRule(
cost: shippingRule.cost.amount,
id: decompose(id: shippingRule.id),
location: Location(
country: shippingRule.location.country,
displayableName: shippingRule.location.displayableName,
id: decompose(id: shippingRule.location.id) ?? 0,
localizedName: shippingRule.location.countryName,
name: shippingRule.location.name
)
)
}
}
19 changes: 16 additions & 3 deletions KsApi/models/Reward.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ public struct AddOnData {
}

public struct Reward {
public var addOnData: AddOnData?
public var addOnData: AddOnData? // FIXME: to be removed in future
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 will be removed when we refactor this code.

public let backersCount: Int?
public let convertedMinimum: Double
public let description: String
Expand All @@ -22,7 +22,8 @@ public struct Reward {
public let minimum: Double
public let remaining: Int?
public let rewardsItems: [RewardsItem]
public let shipping: Shipping
public let shipping: Shipping // only v1
public let shippingRules: [ShippingRule]? // only GraphQL
public let startsAt: TimeInterval?
public let title: String?

Expand All @@ -31,9 +32,20 @@ public struct Reward {
return self.id == Reward.noReward.id
}

/**
Returns the closest matching `ShippingRule` for this `Reward` to `otherShippingRule`.
If no match is found `otherShippingRule` is returned, this is to be backward-compatible
with v1 Rewards that do not include the `shippingRules` array.
*/
public func shippingRule(matching otherShippingRule: ShippingRule?) -> ShippingRule? {
return self.shippingRules?
.first { shippingRule in shippingRule.location.id == otherShippingRule?.location.id }
?? otherShippingRule
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make sense to coalesce this to otherShippingRule? I'm just thinking if there's no match for some reason that it should use the base reward's shipping cost, but I'm not sure if that would ever happen 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I would leave it as nil in the edge case that a shipping rule is not provided. I'm also wondering how we handle creators that specify free shipping? Will that still be a shipping rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, let me update to not coalesce this to the matching rule.

Yes, I confirmed it will be returned as a shipping rule with 0 cost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update comment to explain that this is to work with legacy v1 Rewards.

}

public struct Shipping: Swift.Decodable {
public let enabled: Bool
public let location: Location?
public let location: Location? /// via v1 if `ShippingType` is `singleLocation`
public let preference: Preference?
public let summary: String?
public let type: ShippingType?
Expand Down Expand Up @@ -94,6 +106,7 @@ extension Reward: Argo.Decodable {
return tmp2
<*> ((json <|| "rewards_items") <|> .success([]))
<*> tryDecodable(json)
<*> json <||? "shipping_rules"
<*> json <|? "starts_at"
<*> json <|? "title"
}
Expand Down
5 changes: 5 additions & 0 deletions KsApi/models/RewardAddOnSelectionViewEnvelope.swift
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,16 @@ public struct RewardAddOnSelectionViewEnvelope: Swift.Decodable {
}

public struct ShippingRule: Swift.Decodable {
public var cost: Money
public var id: String
public var location: Location

public struct Location: Swift.Decodable {
public var country: String
public var countryName: String
public var displayableName: String
public var id: String
public var name: String
}
}
}
Expand Down
22 changes: 20 additions & 2 deletions KsApi/models/RewardAddOnSelectionViewEnvelopeTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,18 @@ final class RewardAddOnSelectionViewEnvelopeTests: XCTestCase {
"shippingPreference": "restricted",
"shippingRules": [
[
"cost": [
"amount": "15.0",
"currency": "USD",
"symbol": "$"
],
"id": "U2hpcHBpbmdSdWxlLTEwMzc5NTgz",
"location": [
"id": "TG9jYXRpb24tMQ=="
"country": "CA",
"countryName": "Canada",
"displayableName": "Canada",
"id": "TG9jYXRpb24tMjM0MjQ3NzU=",
"name": "Canada"
]
]
]
Expand Down Expand Up @@ -81,8 +90,17 @@ final class RewardAddOnSelectionViewEnvelopeTests: XCTestCase {
XCTAssertEqual(value.project.addOns?.nodes[0].remainingQuantity, nil)
XCTAssertEqual(value.project.addOns?.nodes[0].startsAt, nil)
XCTAssertEqual(value.project.addOns?.nodes[0].shippingPreference, .restricted)
XCTAssertEqual(
value.project.addOns?.nodes[0].shippingRules?[0].cost,
Money(amount: 15.0, currency: .usd, symbol: "$")
)
XCTAssertEqual(value.project.addOns?.nodes[0].shippingRules?[0].id, "U2hpcHBpbmdSdWxlLTEwMzc5NTgz")
XCTAssertEqual(value.project.addOns?.nodes[0].shippingRules?[0].location.id, "TG9jYXRpb24tMQ==")
XCTAssertEqual(value.project.addOns?.nodes[0].shippingRules?[0].location.id, "TG9jYXRpb24tMjM0MjQ3NzU=")
XCTAssertEqual(value.project.addOns?.nodes[0].shippingRules?[0].location.country, "CA")
XCTAssertEqual(value.project.addOns?.nodes[0].shippingRules?[0].location.countryName, "Canada")
XCTAssertEqual(value.project.addOns?.nodes[0].shippingRules?[0].location.displayableName, "Canada")
XCTAssertEqual(value.project.addOns?.nodes[0].shippingRules?[0].location.name, "Canada")

} catch {
XCTFail((error as NSError).description)
}
Expand Down
34 changes: 34 additions & 0 deletions KsApi/models/RewardTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -186,4 +186,38 @@ final class RewardTests: XCTestCase {
XCTAssertEqual(false, reward.value?.shipping.enabled)
XCTAssertEqual(.noShipping, reward.value?.shipping.type)
}

func testRewardShippingRule_Match() {
let shippingRule1 = ShippingRule.template
|> ShippingRule.lens.cost .~ 5.0
|> ShippingRule.lens.location .~ (.template |> Location.lens.id .~ 1)
let shippingRule2 = ShippingRule.template
|> ShippingRule.lens.cost .~ 1.0
|> ShippingRule.lens.location .~ (.template |> Location.lens.id .~ 2)
let reward = Reward.template
|> Reward.lens.shippingRules .~ [shippingRule1, shippingRule2]

let match = ShippingRule.template
|> ShippingRule.lens.cost .~ 500.0
|> ShippingRule.lens.location .~ (.template |> Location.lens.id .~ 1)

XCTAssertEqual(reward.shippingRule(matching: match), shippingRule1)
}

func testRewardShippingRule_NoMatch() {
let shippingRule1 = ShippingRule.template
|> ShippingRule.lens.cost .~ 5.0
|> ShippingRule.lens.location .~ (.template |> Location.lens.id .~ 5)
let shippingRule2 = ShippingRule.template
|> ShippingRule.lens.cost .~ 1.0
|> ShippingRule.lens.location .~ (.template |> Location.lens.id .~ 2)
let reward = Reward.template
|> Reward.lens.shippingRules .~ [shippingRule1, shippingRule2]

let match = ShippingRule.template
|> ShippingRule.lens.cost .~ 500.0
|> ShippingRule.lens.location .~ (.template |> Location.lens.id .~ 1)

XCTAssertEqual(reward.shippingRule(matching: match), match)
}
}
37 changes: 37 additions & 0 deletions KsApi/models/lenses/RewardLenses.swift
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -39,6 +40,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -60,6 +62,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -81,6 +84,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -102,6 +106,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -123,6 +128,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -144,6 +150,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -165,6 +172,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -186,6 +194,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -207,6 +216,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -228,6 +238,7 @@ extension Reward {
remaining: $0,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -249,6 +260,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $0,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -270,6 +282,29 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $0,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $1.title
) }
)

public static let shippingRules = Lens<Reward, [ShippingRule]?>(
view: { $0.shippingRules },
set: { Reward(
addOnData: $1.addOnData,
backersCount: $1.backersCount,
convertedMinimum: $1.convertedMinimum,
description: $1.description,
endsAt: $1.endsAt,
estimatedDeliveryOn: $1.estimatedDeliveryOn,
hasAddOns: $1.hasAddOns,
id: $1.id,
limit: $1.limit,
minimum: $1.minimum,
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $0,
startsAt: $1.startsAt,
title: $1.title
) }
Expand All @@ -291,6 +326,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $0,
title: $1.title
) }
Expand All @@ -312,6 +348,7 @@ extension Reward {
remaining: $1.remaining,
rewardsItems: $1.rewardsItems,
shipping: $1.shipping,
shippingRules: $1.shippingRules,
startsAt: $1.startsAt,
title: $0
) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,20 @@ extension RewardAddOnSelectionViewEnvelope.Project.Reward {
)
}

extension RewardAddOnSelectionViewEnvelope.Project.Reward.ShippingRule.Location {
static let template = RewardAddOnSelectionViewEnvelope.Project.Reward.ShippingRule.Location(
country: "CA",
countryName: "Canada",
displayableName: "Canada",
id: "TG9jYXRpb24tMjM0MjQ3NzU=",
name: "Canada"
)
}

extension RewardAddOnSelectionViewEnvelope.Project.Reward.ShippingRule {
static let template = RewardAddOnSelectionViewEnvelope.Project.Reward.ShippingRule(
cost: Money(amount: 10, currency: .usd, symbol: "$"),
id: "U2hpcHBpbmdSdWxlLTEwMzc5NTgz",
location: Location(id: "TG9jYXRpb24tMjM0MjQ5Nzc=")
location: .template
)
}
Loading