Cleanup Risk Messaging Feature #2089
Conversation
Generated by 🚫 Danger |
scottkicks
requested review from
a team and
ifosli
and removed request for
a team
There was a problem hiding this comment.
Mostly looks good but I'm concerned that the behavior has changed a little (see comments for details). Will you make sure you also test this in the change pledge context if you haven't already? (Ie pledge with a fake card and go try to change your payment method to apple pay? Requires finding a project in staging that has real time left though or testing in prod)
| self.goToApplePayPaymentAuthorization = paymentAuthorizationData | ||
| .takeWhen(goToApplePayPaymentAuthorization) | ||
| .takeWhen(self.applePayButtonTappedSignal) |
There was a problem hiding this comment.
Explain this change? The goToApplePayPaymentAuthorization only emitted when the pledge amount was valid, and now it looks like it'll emit regardless.
| self.applePayButtonTappedSignal.filter(isNativeRiskMessagingControlEnabled), | ||
| applePayButtonTappedAndIsChangingPaymentMethod, | ||
| self.riskMessagingViewControllerDismissedProperty.signal.skipNil().filter(isTrue).ignoreValues() | ||
| let applePayButtonTappedAndOrIsChangingPaymentMethod = Signal.merge( |
There was a problem hiding this comment.
I think this entire signal should just be self.applePayButtonTappedSignal instead (since we're always in the control of the experiment now). The way it's currently written, if the apple pay button is tapped in the change pledge context, the signal looks like it'll emit twice.
There was a problem hiding this comment.
I hear you. There are XCTAssertEquals assertions in the tests for this that compare arrays, so if it was emitting twice, I would think those would catch it.
I think you're suggestion reads more clearly anyway tho. I'll add this to the next commit.
| ) | ||
|
|
||
| let goToApplePayPaymentAuthorization = pledgeAmountIsValid | ||
| .takeWhen(applePayButtonTappedAndOrIsChangingPaymentMethod) |
There was a problem hiding this comment.
Specifically, I think we should just use .takeWhen(self.applePayButtonTappedSignal) here (and for the showApplePayAlert below
|
Thanks @ifosli! I've thoroughly tested changing payment and the rest of the manage pledge flow on top of pledging. I like your suggestions and will get those in today and re-test 👍 |
📲 What
Removes the unused Optimizely Risk Messaging Experiment Feature.
🤔 Why
This extra, unused screen during checkout reminded people that backings weren't guaranteed and should have been removed when Optimizely was deprecated.
🛠 How
RiskMessagingViewController/ViewModelPledgeViewModelcheckout code that referencesisNativeRiskMessagingControlEnabledisNativeRiskMessagingControlEnabledfrom SharedFunctionsfunc trackPledgeConfirmButtonClicked()from our analytics events because it was only used in the Risk Messaging flow.👀 See
There should be no visible changes to the app.
✅ Acceptance criteria