Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

MBL-1969: Remove unused amount selector from NoShippingPledge flow #2247

Merged
merged 7 commits into from
Jan 27, 2025
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 @@ -41,11 +41,6 @@ final class NoShippingPledgeViewController: UIViewController,

private var paymentSectionLabel: UILabel = { UILabel(frame: .zero) }()

private lazy var pledgeAmountViewController = {
PledgeAmountViewController.instantiate()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pledgeAmountViewController was no longer added to the view hierarchy in NoShippingPledgeViewController - it was here, wired up, but invisible and unused.

|> \.delegate .~ self
}()

internal var processingView: ProcessingView? = ProcessingView(frame: .zero)
private lazy var pledgeDisclaimerView: PledgeDisclaimerView = {
PledgeDisclaimerView(frame: .zero)
Expand Down Expand Up @@ -305,12 +300,6 @@ final class NoShippingPledgeViewController: UIViewController,
self?.pledgeRewardsSummaryViewController.configureWith(pledgeOverTimeData: data)
}

self.viewModel.outputs.configurePledgeAmountViewWithData
.observeForUI()
.observeValues { [weak self] data in
self?.pledgeAmountViewController.configureWith(value: data)
}

self.viewModel.outputs.configurePledgeAmountSummaryViewControllerWithData
.observeForUI()
.observeValues { [weak self] data in
Expand All @@ -337,12 +326,6 @@ final class NoShippingPledgeViewController: UIViewController,
self?.pledgeCTAContainerView.configureWith(value: value)
}

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

self.viewModel.outputs.configurePaymentMethodsViewControllerWithValue
.observeForUI()
.observeValues { [weak self] value in
Expand Down Expand Up @@ -394,7 +377,6 @@ final class NoShippingPledgeViewController: UIViewController,

self.localPickupLocationView.rac.hidden = self.viewModel.outputs.localPickupViewHidden
self.paymentMethodsViewController.view.rac.hidden = self.viewModel.outputs.paymentMethodsViewHidden
self.pledgeAmountViewController.view.rac.hidden = self.viewModel.outputs.pledgeAmountViewHidden

self.viewModel.outputs.title
.observeForUI()
Expand Down Expand Up @@ -429,12 +411,6 @@ final class NoShippingPledgeViewController: UIViewController,
.observeValues { [weak self] errorMessage in
self?.messageBannerViewController?.showBanner(with: .error, message: errorMessage)
}

self.viewModel.outputs.showApplePayAlert
.observeForControllerAction()
.observeValues { [weak self] title, message in
self?.presentApplePayInvalidAmountAlert(title: title, message: message)
}
}

private func goToPaymentAuthorization(_ paymentAuthorizationData: PaymentAuthorizationData) {
Expand All @@ -461,10 +437,6 @@ final class NoShippingPledgeViewController: UIViewController,
self.navigationController?.pushViewController(thanksVC, animated: true)
}

private func presentApplePayInvalidAmountAlert(title: String, message: String) {
self.present(UIAlertController.alert(title, message: message), animated: true)
}

// MARK: - Actions

@objc private func dismissKeyboard() {
Expand Down Expand Up @@ -560,17 +532,6 @@ final class NoShippingPledgeViewController: UIViewController,
}
}

// MARK: - PledgeAmountViewControllerDelegate

extension NoShippingPledgeViewController: PledgeAmountViewControllerDelegate {
func pledgeAmountViewController(
_: PledgeAmountViewController,
didUpdateWith data: PledgeAmountData
) {
self.viewModel.inputs.pledgeAmountViewControllerDidUpdate(with: data)
}
}

// MARK: - STPAuthenticationContext

extension NoShippingPledgeViewController: STPAuthenticationContext {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ final class NoShippingPledgeViewControllerTests: TestCase {
let data = PledgeViewData(
project: project,
rewards: [nonReward],
bonusSupport: 1.0,
selectedShippingRule: shippingRule,
selectedQuantities: [nonReward.id: 1],
selectedLocationId: ShippingRule.template.id,
Expand Down
126 changes: 8 additions & 118 deletions Library/ViewModels/NoShippingPledgeViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ public protocol NoShippingPledgeViewModelInputs {
)
func paymentAuthorizationViewControllerDidFinish()
func paymentPlanSelected(_ paymentPlan: PledgePaymentPlansType)
func pledgeAmountViewControllerDidUpdate(with data: PledgeAmountData)
func pledgeDisclaimerViewDidTapLearnMore()
func scaFlowCompleted(with result: StripePaymentHandlerActionStatusType, error: Error?)
func stripeTokenCreated(token: String?, error: Error?) -> PKPaymentAuthorizationStatus
Expand All @@ -38,7 +37,6 @@ public protocol NoShippingPledgeViewModelOutputs {
var configureEstimatedShippingView: Signal<(String?, String?), Never> { get }
var configureLocalPickupViewWithData: Signal<PledgeLocalPickupViewData, Never> { get }
var configurePaymentMethodsViewControllerWithValue: Signal<PledgePaymentMethodsValue, Never> { get }
var configurePledgeAmountViewWithData: Signal<PledgeAmountViewConfigData, Never> { get }
var configurePledgeAmountSummaryViewControllerWithData: Signal<PledgeAmountSummaryViewData, Never> { get }
var configurePledgeRewardsSummaryViewWithData: Signal<
(PostCampaignRewardsSummaryViewData, Double?, PledgeSummaryViewData),
Expand All @@ -53,13 +51,10 @@ public protocol NoShippingPledgeViewModelOutputs {
var goToLogin#: Signal<LoginIntent, Never> { get }
var localPickupViewHidden: Signal<Bool, Never> { get }
var notifyDelegateUpdatePledgeDidSucceedWithMessage: Signal<String, Never> { get }
var notifyPledgeAmountViewControllerUnavailableAmountChanged: Signal<Double, Never> { get }
var paymentMethodsViewHidden: Signal<Bool, Never> { get }
var pledgeAmountViewHidden: Signal<Bool, Never> { get }
var pledgeAmountSummaryViewHidden: Signal<Bool, Never> { get }
var popToRootViewController: Signal<(), Never> { get }
var processingViewIsHidden: Signal<Bool, Never> { get }
var showApplePayAlert: Signal<(String, String), 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.

This alert was only used to show information about pledge min/max issues. With the amount selector gone, the pledge can never be over/under on this page.

var showErrorBannerWithMessage: Signal<String, Never> { get }
var showWebHelp: Signal<HelpType, Never> { get }
var title: Signal<String, Never> { get }
Expand Down Expand Up @@ -95,7 +90,6 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin

let backing = project.map { $0.personalization.backing }.skipNil()

self.pledgeAmountViewHidden = context.map { $0.pledgeAmountViewHidden }
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 did sometimes return false but as mentioned, the pledgeAmountView wasn't in the view hierarchy any more.

self.pledgeAmountSummaryViewHidden = context.map { $0.pledgeAmountSummaryViewHidden }

self.descriptionSectionSeparatorHidden = Signal.combineLatest(context, baseReward)
Expand Down Expand Up @@ -142,7 +136,7 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin
)

// Initial pledge amount is zero if not backed and not set previously in the flow.
let initialAdditionalPledgeAmount = initialData.map {
let additionalPledgeAmount = initialData.map {
if let bonusSupport = $0.bonusSupport {
return bonusSupport
} else if let backing = $0.project.personalization.backing {
Expand All @@ -152,14 +146,6 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin
}
}

// TODO(MBL-1670): Delete the additional pledge amount, any validation, and the stepper class.
let additionalPledgeAmount = Signal.merge(
self.pledgeAmountDataSignal.map { $0.amount },
initialAdditionalPledgeAmount
)

self.notifyPledgeAmountViewControllerUnavailableAmountChanged = allRewardsTotal

let projectAndReward = Signal.zip(project, baseReward)

/**
Expand Down Expand Up @@ -199,19 +185,6 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin

self.localPickupViewHidden = baseReward.map(isRewardLocalPickup).negate()

self.configurePledgeAmountViewWithData = Signal.combineLatest(
projectAndReward,
initialAdditionalPledgeAmount
)
.map(unpack)
.map { project, reward, additionalPledgeAmount in
(
project,
reward,
additionalPledgeAmount
)
}

self.configureLocalPickupViewWithData = projectAndReward
.switchMap { projectAndReward -> SignalProducer<PledgeLocalPickupViewData?, Never> in
guard let locationName = projectAndReward.1.localPickup?.displayableName else {
Expand Down Expand Up @@ -350,9 +323,6 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin
self.paymentMethodsViewHidden = Signal.combineLatest(isLoggedIn, context)
.map { !$0 || $1.paymentMethodsViewHidden }

let pledgeAmountIsValid: Signal<Bool, Never> = self.pledgeAmountDataSignal
.map { $0.isValid }

self.configureStripeIntegration = Signal.combineLatest(
initialData,
context
Expand Down Expand Up @@ -416,14 +386,6 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin
.filter { $0 == .changePaymentMethod }
.ignoreValues()

let goToApplePayPaymentAuthorization = pledgeAmountIsValid
.takeWhen(self.applePayButtonTappedSignal)
.filter(isTrue)

let showApplePayAlert = pledgeAmountIsValid
.takeWhen(self.applePayButtonTappedSignal)
.filter(isFalse)

let paymentAuthorizationData: Signal<PaymentAuthorizationData, Never> = Signal.combineLatest(
project,
baseReward,
Expand All @@ -445,7 +407,7 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin
}

self.goToApplePayPaymentAuthorization = paymentAuthorizationData
.takeWhen(goToApplePayPaymentAuthorization)
.takeWhen(self.applePayButtonTappedSignal)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pledgeAmount is now always valid, so an extra validation signal isn't needed here.


let pkPaymentData = self.pkPaymentSignal
.map { pkPayment -> PKPaymentData? in
Expand Down Expand Up @@ -671,41 +633,6 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin

// MARK: - Form Validation

let amountChangedAndValid = Signal.combineLatest(
project,
baseReward,
self.pledgeAmountDataSignal,
initialAdditionalPledgeAmount,
context
)
.map(amountValid)

self.showApplePayAlert = Signal.combineLatest(
project,
self.pledgeAmountDataSignal
)
.takeWhen(showApplePayAlert)
.map { project, pledgeAmountData in (project, pledgeAmountData.min, pledgeAmountData.max) }
.map { project, min, max in
(
Strings.Almost_there(),
Strings.Please_enter_a_pledge_amount_between_min_and_max(
min: Format
.currency(
min,
country: projectCountry(forCurrency: project.stats.currency) ?? project.country,
omitCurrencyCode: false
),
max: Format
.currency(
max,
country: projectCountry(forCurrency: project.stats.currency) ?? project.country,
omitCurrencyCode: false
)
)
)
}

let notChangingPaymentMethod = context.map { context in
context.isUpdating && context != .changePaymentMethod
}
Expand All @@ -724,19 +651,20 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin
)

let valuesChangedAndValid = Signal.combineLatest(
amountChangedAndValid,
paymentMethodChangedAndValid,
self.pledgeOverTimeUseCase.pledgeOverTimeIsLoading,
context
)
.map(allValuesChangedAndValid)

let didCreateOrUpdateBacking = createOrUpdateEvent.filter { $0.isTerminating }

let isEnabled = Signal.merge(
self.viewDidLoadProperty.signal.mapConst(false)
.take(until: valuesChangedAndValid.ignoreValues()),
valuesChangedAndValid,
self.submitButtonTappedSignal.mapConst(false),
createOrUpdateEvent.filter { $0.isTerminating }.mapConst(true)
valuesChangedAndValid.takeWhen(didCreateOrUpdateBacking)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found this small bug while running through the tests. The bug looked like this:

  • Go to checkout
  • Don't select a payment method. The CTA is disabled.
  • Hit ApplePay
  • ApplePay fails for some reason
  • The CTA becomes enabled, even though there is no payment method selected still. That's because createOrUpdateEvent was always mapped to true.

This fixes that edge case.

)
.skipRepeats()

Expand Down Expand Up @@ -926,7 +854,7 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin
rewards,
selectedQuantities,
refTag,
initialAdditionalPledgeAmount,
additionalPledgeAmount,
pledgeTotal,
context
)
Expand Down Expand Up @@ -1065,11 +993,6 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin
self.goToLogin#Observer.send(value: ())
}

private let (pledgeAmountDataSignal, pledgeAmountObserver) = Signal<PledgeAmountData, Never>.pipe()
public func pledgeAmountViewControllerDidUpdate(with data: PledgeAmountData) {
self.pledgeAmountObserver.send(value: data)
}

private let (pledgeDisclaimerViewDidTapLearnMoreSignal, pledgeDisclaimerViewDidTapLearnMoreObserver)
= Signal<Void, Never>.pipe()
public func pledgeDisclaimerViewDidTapLearnMore() {
Expand Down Expand Up @@ -1119,7 +1042,6 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin
public let configureEstimatedShippingView: Signal<(String?, String?), Never>
public let configureLocalPickupViewWithData: Signal<PledgeLocalPickupViewData, Never>
public let configurePaymentMethodsViewControllerWithValue: Signal<PledgePaymentMethodsValue, Never>
public let configurePledgeAmountViewWithData: Signal<PledgeAmountViewConfigData, Never>
public let configurePledgeAmountSummaryViewControllerWithData: Signal<PledgeAmountSummaryViewData, Never>
public let configurePledgeRewardsSummaryViewWithData: Signal<
(PostCampaignRewardsSummaryViewData, Double?, PledgeSummaryViewData),
Expand All @@ -1134,14 +1056,11 @@ public class NoShippingPledgeViewModel: NoShippingPledgeViewModelType, NoShippin
public let goToLogin#: Signal<LoginIntent, Never>
public let localPickupViewHidden: Signal<Bool, Never>
public let notifyDelegateUpdatePledgeDidSucceedWithMessage: Signal<String, Never>
public let notifyPledgeAmountViewControllerUnavailableAmountChanged: Signal<Double, Never>
public let paymentMethodsViewHidden: Signal<Bool, Never>
public let pledgeAmountViewHidden: Signal<Bool, Never>
public let pledgeAmountSummaryViewHidden: Signal<Bool, Never>
public let popToRootViewController: Signal<(), Never>
public let processingViewIsHidden: Signal<Bool, Never>
public let showErrorBannerWithMessage: Signal<String, Never>
public let showApplePayAlert: Signal<(String, String), Never>
public let showWebHelp: Signal<HelpType, Never>
public let title: Signal<String, Never>

Expand Down Expand Up @@ -1169,34 +1088,6 @@ private func requiresSCA(_ envelope: StripeSCARequiring) -> Bool {

// MARK: - Validation Functions

private func amountValid(
project: Project,
reward: Reward,
pledgeAmountData: PledgeAmountData,
initialAdditionalPledgeAmount: Double,
context: PledgeViewContext
) -> Bool {
guard
project.personalization.backing != nil,
context.isUpdating,
userIsBacking(reward: reward, inProject: project)
else {
return pledgeAmountData.isValid
}

/**
The amount is valid if it's changed or if the reward has add-ons.
This works because of the validation that would have occurred during add-ons selection,
that is, in `RewardAddOnSelectionViewController` we don't navigate further unless the selection changes.
*/
return [
pledgeAmountData
.amount != initialAdditionalPledgeAmount || (reward.hasAddOns || featureNoShippingAtCheckout()),
pledgeAmountData.isValid
]
.allSatisfy(isTrue)
}

private func shippingRuleValid(
project: Project,
reward: Reward,
Expand Down Expand Up @@ -1242,16 +1133,15 @@ private func paymentMethodValid(
}

private func allValuesChangedAndValid(
amountValid: Bool,
paymentSourceValid: Bool,
pledgeOverTimeIsLoading: Bool,
context: PledgeViewContext
) -> Bool {
if context.isUpdating, context != .updateReward {
return amountValid || paymentSourceValid
return paymentSourceValid
}

return amountValid && !pledgeOverTimeIsLoading
return !pledgeOverTimeIsLoading
}

// MARK: - Helper Functions
Expand Down
Loading