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-363] Select a new reward #894

Merged
merged 38 commits into from
Oct 21, 2019
Merged

[NT-363] Select a new reward #894

merged 38 commits into from
Oct 21, 2019

Conversation

justinswart
Copy link
Contributor

@justinswart justinswart commented Oct 12, 2019

πŸ“² What

Continues work in #887, #893, #890. This is based off the branch of #893.

Adds yet another context to PledgeViewModel, updateReward. Attempts to treat updating a reward the same as creating a new pledge, save for the fact that we make use of the UpdateBacking mutation.

πŸ€” Why

This allows backers to update the reward that they've selected for a project.

πŸ›  How

  • Added updateReward to PledgeViewContext.
  • Changed the behaviour of PledgeShippingLocationViewModel to only return the currently selected ShippingRule if the new reward also shared that location.
  • Changed the behaviour of PledgeAmountViewModel to return the initial value as being the minimum for the newly selected reward.
  • Updated the validation rules in PledgeViewModel to ignore values being the same when a new reward is being selected.

βœ… Acceptance criteria

Back a project using the correct environment and with the stars aligned, navigate to manage pledge, select Choose another reward from the context menu.

  • The RewardsCollectionViewController is presented and allows you to select a different reward taking you to the PledgeViewController.
  • The amount should be the minimum amount of the new reward.
  • The shipping location should be the one that was previously selected if the new reward also provides that location as an option.

* As mentioned in #893 the payment method does not have the correct default selection behaviour, although this may be the way that we'd prefer it for updating a reward.

# Conflicts:
#	Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_de_device_pad@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_de_device_phone4_7inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_es_device_pad@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_es_device_phone4_7inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_fr_device_pad@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_fr_device_phone4_7inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_ja_device_pad@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_UpdateContext_lang_ja_device_phone4_7inch@2x.png
@ksr-ci-bot
Copy link

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@justinswart
Copy link
Contributor Author

@ifbarrera I'll update this to hide the payment methods. I think we'll still need the additional context for that because it looks like we do show the summary view with the thumbnail.

@justinswart justinswart changed the base branch from change-payment-method-mutation to master October 16, 2019 18:57
# Conflicts:
#	Kickstarter-iOS/Views/Controllers/PledgeAmountSummaryViewController.swift
#	Library/PledgeViewContext.swift
#	Library/PledgeViewContextTests.swift
#	Library/ViewModels/PledgeViewModel.swift
#	Library/ViewModels/PledgeViewModelTests.swift
#	Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_ChangePaymentMethodContext_lang_de_device_phone4_7inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_ChangePaymentMethodContext_lang_en_device_phone4_7inch@2x.png
#	Screenshots/_64/Kickstarter_Framework_iOSTests.PledgeViewControllerTests/testView_ChangePaymentMethodContext_lang_fr_device_phone4_7inch@2x.png
# Conflicts:
#	Library/ViewModels/PledgeViewModelTests.swift
@@ -163,8 +163,13 @@ private func shippingValue(of project: Project, with shippingRuleCost: Double) -
private func determineShippingRule(with project: Project, shippingRules: [ShippingRule]) -> ShippingRule? {
guard
let backing = project.personalization.backing,
let selectedRule = shippingRules.filter({ $0.location.id == backing.locationId }).first
let selectedRule = shippingRules.filter({ $0.location.id == backing.locationId }).first,
locationId(selectedRule.location.id, isValidIn: shippingRules)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused by this line that was added. Doesn't it do the same thing as line 166? πŸ€”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's say you picked Reward A and selected Canada as your shipping location. Now if you're changing to Reward B it will pre-select Canada, but only if Reward B also ships to Canada. So the locationId(_:isValidIn:) check is this second bit of logic. If it fails it returns the default shipping rule.


self.vm.inputs.shippingRuleSelected(.init(cost: 1, id: 1, location: .brooklyn))

self.submitButtonEnabled.assertValues(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also test for the behavior when the submit button is tapped? πŸ€”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, thanks.

with: (amount: 690, min: 25.0, max: 10_000.0, isValid: true)
)

self.submitButtonEnabled.assertValues([false, false], "Amount unchanged")
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something with this test, but why is the submit button disabled here? I would expect that because the VM is being configured with reward (which is different from project.backing.reward) that the submit button would be enabled because the pledge amount is between the min and max of the new reward. What am I missing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reward has shipping enabled so the way the validation rules are working is that a shipping rule needs to be selected in order to change the reward.

self.vm.inputs.shippingRuleSelected(.init(cost: 1, id: 1, location: .brooklyn))

self.submitButtonEnabled.assertValues([false, false], "Shipping rule and amount unchanged")
self.submitButtonEnabled.assertValues([false, false, false], "Shipping rule and amount unchanged")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider throwing a skipRepeats() on this output? πŸ˜…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but not right now πŸ˜…

@justinswart justinswart merged commit 8c1d144 into master Oct 21, 2019
@justinswart justinswart deleted the select-this-reward-instead branch July 14, 2021 21:20
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.

None yet

3 participants