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-169] Add shipping type to Reward model #837

Merged
merged 4 commits into from
Sep 17, 2019

Conversation

justinswart
Copy link
Contributor

@justinswart justinswart commented Sep 11, 2019

📲 What

Adds ShippingType to Reward to allow us to be more descriptive about the type of shipping options a particular reward has.

Related back-end PR: https://github.com/kickstarter/kickstarter/pull/16934

🤔 Why

Currently we only use the non-localized summary field that is returned with each reward's shipping info. By using an enumerable type we can maintain more meaningful localized strings at the client.

🛠 How

  • Added ShippingType and Location to Reward.Shipping.
  • Updated tests.

👀 See

Limited shipping Ships worldwide US only (faked for snapshot 😅)
image image image

✅ Acceptance criteria

  • Rewards that ship to anywhere display "Ships worldwide".
  • Rewards that ship to multiple_locations display "Limited shipping".
  • Rewards that ship to single_location where that location is United States display "United States only".
  • Rewards that have no_shipping do not display a pill for shipping.

⏰ TODO

  • Add strings for translations.

@@ -25,14 +25,28 @@ public struct Reward {

public struct Shipping {
public let enabled: Bool
public let location: Location?
public let preference: Preference?
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 is deprecated but I think we should only remove it when we remove DeprecatedRewardPledgeViewController.

if project.state == .live, reward.shipping.enabled, let type = reward.shipping.type {
switch type {
case .anywhere:
return localizedString(key: "Ships_worldwide", defaultValue: "Ships worldwide")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add these strings for translations.

Copy link
Contributor

@dusi dusi left a comment

Choose a reason for hiding this comment

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

Looks good..had couple questions

@@ -319,8 +319,7 @@ let allRewards: [(String, Reward)] = {
|> Reward.lens.shipping .~ (
.template
|> Reward.Shipping.lens.enabled .~ true
|> Reward.Shipping.lens.preference .~ .restricted
|> Reward.Shipping.lens.summary .~ "Anywhere in the world"
|> Reward.Shipping.lens.type .~ .anywhere
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we also wanted to add tests for other ShippingType cases or do you think this is sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are tested in the VM tests so I think it's ok to just have the anywhere case for snapshots.

XCTAssertEqual(reward.value?.backersCount, 10)
XCTAssertEqual(true, reward.value?.shipping.enabled)
XCTAssertEqual(.singleLocation, reward.value?.shipping.type)
XCTAssertEqual(.init(id: 123, localizedName: "United States"), reward.value?.shipping.location)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess same question here about testing all the remaining cases (noShipping & multipleLocations)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, let me add those.

if let name = reward.shipping.location?.localizedName {
return localizedString(
key: "location_name_only",
defaultValue: "%{location_name} only",
Copy link
Contributor

Choose a reason for hiding this comment

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

When these strings are translated does this mean that in German it would result to United States nur? I believe (and my German is very rusty at this point) that the right order should be Nur United States. 🤔

Do we have a mechanism to switch the order?

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 the translator can move these around, I'm adding those strings for translation shortly 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants