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-502] Fix pledge total & update pledge amount for No Reward backings #921

Merged
merged 6 commits into from
Nov 4, 2019

Conversation

ifbarrera
Copy link
Contributor

@ifbarrera ifbarrera commented Oct 31, 2019

πŸ“² What

Fixes a bug where the "Update pledge amount" and "Change payment method" screens were displaying the incorrect pledge amount and totals on No Reward backings that have pledge amounts that are different from the reward minimum ($1).

πŸ€” Why

Need to fix the πŸ›s

πŸ›  How

There was a bug in the function in PledgeAmountViewModel that determines whether a user is backing a given reward, because of the strange structure of No Reward objects.

Updating this logic to use our shared function userIsBacking(reward:inProject) fixed this issue.

I also added more robust tests.

πŸ‘€ See

Change Payment Method screen

After πŸ¦‹ Before πŸ›
Simulator Screen Shot - iPhone 8 - 2019-10-31 at 11 26 49 Simulator Screen Shot - iPhone 8 - 2019-10-31 at 13 20 13

Update Pledge Amount screen

After πŸ¦‹ Before πŸ›
Simulator Screen Shot - iPhone 8 - 2019-10-31 at 13 26 02 Simulator Screen Shot - iPhone 8 - 2019-10-31 at 13 20 21

♿️ Accessibility

N/A

🏎 Performance

N/A

βœ… Acceptance criteria

  • Backing with no shipping
    Change payment method screen:
    - Pledge amount should reflect the backing amount
    - total should reflect the backing amount
    Update pledge amount
    - pledge amount should start at the backing amount

  • Backing with shipping
    Change payment method screen:
    - Pledge amount should reflect the backing amount, not including shipping
    - shipping amount should reflect the selected shipping method amount
    - total should reflect the total backing amount including shipping
    Update pledge amount
    - pledge amount should start at the backing amount
    - shipping amount should start at the original shipping amount

  • No reward backing
    Change payment method screen:
    - Pledge amount should reflect the backing amount
    - total should reflect the backing amount
    Update pledge amount
    - pledge amount should start at the backing amount

  • No reward backing where pledged amount is higher than min
    Change payment method screen:
    - Pledge amount should reflect the backing amount
    - total should reflect the backing amount
    Update pledge amount
    - pledge amount should start at the backing amount

@@ -19,7 +21,6 @@ public protocol PledgeAmountViewModelInputs {
}

public protocol PledgeAmountViewModelOutputs {
var amount: Signal<PledgeAmountData, Never> { get }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed this output to something more descriptive.

@@ -4,6 +4,8 @@ import Prelude
import ReactiveExtensions
import ReactiveSwift

public typealias PledgeAmountData = (amount: Double, min: Double, max: Double, isValid: Bool)
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 typealias was previously in PledgeViewModel for some reason. I thought it made more sense for it to live here.

self.amount.map { $0.0 }
)
.skipRepeats()
self.stepperValue = self.notifyDelegateAmountUpdated.map { $0.0 }.skipRepeats()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no real need to merge with minValue - this is already taken into account in notifyDelegateAmountUpdated.

@@ -261,8 +259,9 @@ private func rounded(_ value: Double) -> Double {
return round(value * 100) / 100
}

private func initialPledgeAmount(from backing: Backing?, reward: Reward, minValue: Double) -> Double {
guard let backing = backing, reward.id == backing.rewardId else { return minValue }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, the bug was essentially that No Reward's rewardId in backing.rewardId comes back as nil, even though the "No reward" Reward object has an id of 0 Β―_(ツ)_/Β―

Our shared function userIsBacking(reward) accounts for this, so using that fixed the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah very silly, thanks!

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Great fix! Thanks!

@ifbarrera ifbarrera merged commit 0693947 into master Nov 4, 2019
@ifbarrera ifbarrera deleted the NT-502 branch November 4, 2019 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants