-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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-331] Perform Update Backing Mutation Request #874
Conversation
# Conflicts: # KsApi/models/Backing.swift # KsApi/models/lenses/BackingLenses.swift # KsApi/models/templates/BackingTemplates.swift
# Conflicts: # KsApi/models/UpdateBackingEnvelopeTests.swift # KsApi/mutations/inputs/UpdateBackingInput.swift
# Conflicts: # Library/ViewModels/ManagePledgeViewModel.swift
|
||
extension Backing: GraphIDBridging { | ||
public static var modelName: String { | ||
return "Backing" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to confirm this name.
KsApi/models/Location.swift
Outdated
|
||
extension Location: GraphIDBridging { | ||
public static var modelName: String { | ||
return "Location" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to confirm this name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the locationId
mutation input is actually not a graphID
. In other words you should just be able to use location.id
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should remove this conformance since it's no longer being used?
} | ||
} | ||
|
||
func pledgeAmountString(withAmount amount: Double, shippingRule: ShippingRule?) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will move this to a shared place once we implement the new CreateBackingInput
. Also not sure if we're going the route of splitting out amount and shipping rule for that.
Generated by 🚫 Danger |
# Conflicts: # Kickstarter-iOS/Views/Controllers/PledgeViewController.swift # Library/ViewModels/PledgeViewModel.swift # Library/ViewModels/PledgeViewModelTests.swift
pledgeAmountString(withAmount: pledgeAmount, shippingRule: updateBackingData.shippingRule) | ||
} | ||
let backingId: String = updateBackingData.backing.graphID | ||
let locationId: String? = updateBackingData.shippingRule.flatMap { $0.location.graphID } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I mentioned I think this is actually just the location.id
@@ -99,6 +101,10 @@ public final class ManagePledgeViewModel: | |||
|
|||
self.rewardReceivedViewControllerViewIsHidden = projectAndReward | |||
.map { project, reward in reward.isNoReward || project.personalization.backing?.status != .collected } | |||
|
|||
self.showSuccessBannerWithMessage = self.pledgeViewControllerDidUpdatePledgeSignal.mapConst( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea behind putting this string in ManagePledgeViewModel
instead of PledgeViewModel
(where the update pledge logic happens) that we're anticipating reusing this string for other "update" types? (ie. update payment method, update reward).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, just that this is a side-effect from the delegate being notified about the success of the request so that the banner is visible when the VC is popped off the stack.
project.map { $0.personalization.backing?.pledgeAmount }, | ||
self.pledgeAmountDataSignal.map { $0.0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.pledgeAmountDataSignal.map(first)
? Might also be valuable to unpack the backing seperately, for clarity:
let backing = project.map { $0.personalization.backing }.skipNil()
let pledgeAmount = Signal.combineLatest(
backing.map { $0.pledgeAmount },
self.pledgeAmountDataSignal.map(first))
Looks like backing
would also come in useful below in a couple places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't do self.pledgeAmountDataSignal.map(first)
because the tuple is 4 values and our versions of first
, second
, third
only support up to 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alas
// MARK: Update Backing | ||
|
||
let updateBackingData = Signal.combineLatest( | ||
project.map { $0.personalization.backing }.skipNil(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use backing
here too if you go that route.
let valuesChanged = Signal.combineLatest( | ||
amountChanged, | ||
shippingRuleChanged, | ||
self.viewDidLoadProperty.signal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't need to combine with viewDidLoad
because amountChanged
and shippingRuleChanged
both already depend on viewDidLoad
🤔. You could easily test this by removing viewDidLoad
from here - your tests should still pass.
|> Project.lens.personalization.backing .~ ( | ||
.template | ||
|> Backing.lens.paymentSource .~ GraphUserCreditCard.amex | ||
|> Backing.lens.status .~ .errored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this status is .errored
? Maybe .pledged
would be more consistent with what we'd be likely to see during this flow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably copy-pasted this from somewhere 😄
self.confirmButtonEnabled.assertDidNotEmitValue() | ||
|
||
self.vm.inputs.pledgeAmountViewControllerDidUpdate( | ||
with: (amount: 10.0, min: 25.0, max: 10_000.0, isValid: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little confusing because the min is 25
, the amount is 10
but it says isValid: true
🤔
) | ||
|
||
self.confirmButtonHidden.assertValues([false]) | ||
self.confirmButtonEnabled.assertValues([false]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add an assertion message here? Took me a while to figure out why the confirm button isn't enabled in this case 😅 (it's because the amount
is actually just the same as was already pledged, right?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure will do :)
shippingRuleChanged | ||
) | ||
.map { amountChanged, shippingRuleChanged -> Bool in | ||
[amountChanged, shippingRuleChanged].contains(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good old contains
😄 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol might I suggest amountChanged || shippingRuleChanged
😅nothing against contains
I just think an OR
seems to illustrate the logic more clearly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments besides that looks good and I don't see anything blocking this PR.
@@ -156,6 +160,14 @@ final class ManagePledgeViewController: UIViewController { | |||
.observeValues { [weak self] project, backing in | |||
self?.goToCancelPledge(project: project, backing: backing) | |||
} | |||
|
|||
self.viewModel.outputs.showSuccessBannerWithMessage | |||
.observeForUI() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes we use .observeForUI
other times we use .observeForControllerAction()
is there any difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding on the difference between these two is that there was some bug in the ReactiveSwift scheduler before that required this distinction. I should actually did in to see whether that was since fixed.
For consistency though I'll update this.
guard let self = self else { return } | ||
|
||
_ = self | ||
|> \.title %~ { _ in title } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wish we had a RAC extension on UIViewController
for dis ¯_(ツ)_/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be nice.
} | ||
} | ||
|
||
func pledgeAmountString(withAmount amount: Double, shippingRule: ShippingRule?) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we either make this private
and/or test
it? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to make it private for now and consider it tested by way of this constructor being tested.
This will become a shared function when we add the newer CreateBacking
mutation and then I'll break it out and write tests for it specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of non-blocking comments to address at your discretion.
Works great! 👏
Tested on:
- iPhone 7 Plus (11.4)
- iPhone 8 (11.4)
KsApi/models/Location.swift
Outdated
|
||
extension Location: GraphIDBridging { | ||
public static var modelName: String { | ||
return "Location" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should remove this conformance since it's no longer being used?
var paymentMethodsViewHidden: Signal<Bool, Never> { get } | ||
var popViewController: Signal<(), Never> { get } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So something I noticed while testing is it seems like the popViewController
and notifyDelegateUpdatePledgeDidSucceedWithMessage
outputs are firing at the same time, which means that the pop animation and the message banner animation overlap. I think it would be nice to only show the message banner once the pop animation has completed. 🤔 Maybe setting a delay on the notifyDelegateUpdatePledgeDidSucceedWithMessage
signal could help? I'm not sure if it's worth the trouble but just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this to the JIRA ticket in which we'll address the refreshing of that view.
shippingRuleChanged | ||
) | ||
.map { amountChanged, shippingRuleChanged -> Bool in | ||
[amountChanged, shippingRuleChanged].contains(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol might I suggest amountChanged || shippingRuleChanged
😅nothing against contains
I just think an OR
seems to illustrate the logic more clearly.
|
||
let valuesChangedAndValid = Signal.combineLatest( | ||
valuesChanged, | ||
self.pledgeAmountDataSignal.map { $0.3 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the parameter name here instead? I think it's $0.isValid
?
📲 What
UpdateBacking
request when the pledge amount or shipping location has changed.✋ What Not
🤔 Why
Brings the functionality to allow the user to update their pledge amount or shipping location.
🛠 How
UpdateBackingData
typealias to gather the information that will be needed to update the backing.👀 See
✅ Acceptance criteria
Navigate Update Pledge on a project that you have backed that is still live.
⏰ TODO