[v6] (1/2) BACS Direct Debit: Remove confirmation screen#2597
Conversation
…S_remove_review_screen # Conflicts: # AdyenComponents/BACS Direct Debit/BACSDirectDebitComponent.swift
…o chore/BACS_remove_confirmation_screen
|
ℹ️ No baseline data found for 'feature/COSDK-1284_Remove_confirmation_screen_BACS'.
|
There was a problem hiding this comment.
Code Review
This pull request refactors the BACS Direct Debit component to use an MVVM architecture, introducing BACSViewController and BACSViewModel while removing the previous presenter-router pattern and the confirmation screen. It also updates the form UI components to support existential any FormItem types. The review feedback highlights several critical compilation errors in the tests, a missing shopperEmail property and parameter in BACSDirectDebitDetails, a potential silent failure in performSubmit(), unnecessary use of @Published in the view model, and opportunities to reduce code duplication in the form view manager and controller.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the BACS Direct Debit component by removing the confirmation screen and transitioning to a view model-based architecture using BACSViewModel and BACSViewController. However, several critical compilation errors were introduced in the integration tests due to references to undefined protocols, private properties, and removed properties. Additionally, the shopper's email address is collected but not propagated to the backend, and the items array in BACSViewModel should be simplified to a non-optional array using compactMap to avoid unnecessary optional-handling helpers in FormViewController.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
| ❇️ | 1 Addition |
| 🔀 | 1 Modification |
AdyenComponents
BACSDirectDebitDetails
❇️ Added
public let shopperEmail: Swift.String? { get }🔀 Modified
// From
public init(
paymentMethod: Adyen.BACSDirectDebitPaymentMethod,
holderName: Swift.String,
bankAccountNumber: Swift.String,
bankLocationId: Swift.String
)
// To
public init(
paymentMethod: Adyen.BACSDirectDebitPaymentMethod,
holderName: Swift.String,
bankAccountNumber: Swift.String,
bankLocationId: Swift.String,
shopperEmail: Swift.String? = nil
)
/**
Changes:
- Added parameter `shopperEmail: Swift.String? = nil`
*/Analyzed targets: Adyen, AdyenActions, AdyenCard, AdyenCardScanner, AdyenCashAppPay, AdyenCheckout, AdyenComponents, AdyenDelegatedAuthentication, AdyenDropIn, AdyenEncryption, AdyenSession, AdyenSwiftUI, AdyenTwint, AdyenUI, AdyenWeChatPay
| // MARK: - PresentableComponent | ||
|
|
||
| package let viewController: UIViewController | ||
| package lazy var viewController: UIViewController = { |
There was a problem hiding this comment.
Doesn't this approach limit our testing capabilities? Passing a mock view controller could help us cover the lifecycle events if needed.
|
|
||
| // MARK: - Private | ||
|
|
||
| private func createViewController() -> UIViewController { |
There was a problem hiding this comment.
Can we think of extracting assembly code from the payment business logic to another assembly/provider/factory?
| import Combine | ||
| import UIKit | ||
|
|
||
| internal final class BACSViewController: FormViewController { |
There was a problem hiding this comment.
I noticed #if canImport(AdyenUI) bracket in the imports section, but it's missing from here where we actually use the FormViewController itself.
| import Adyen | ||
| #if canImport(AdyenUI) | ||
| import AdyenUI | ||
| @_spi(AdyenInternal) import class AdyenUI.FormTextInputItem |
There was a problem hiding this comment.
This makes view model impossible to reuse when(if) we migrate to SwiftUI without code changes.
There was a problem hiding this comment.
Or maybe there is a way to decouple FormTextInputItem from AdyenUI, as we treat item as a business logic layer model. Just a thought for future decisions.
| return | ||
| } | ||
|
|
||
| guard let holderName = holderNameItem?.value, |
There was a problem hiding this comment.
this code section does exactly what the prior one promised - validation.
If we have to rely of FormViewController validation, then we can extract local validation rules to a separate method that in turn will call validateForm()
| submitButtonItem?.showsActivityIndicator = true | ||
| } | ||
|
|
||
| private func createItems() -> [any FormItem] { |
There was a problem hiding this comment.
Extracting helper methods to a separate extension would make code easier to read (can be skipped honestly)
atmamont
left a comment
There was a problem hiding this comment.
Architecture choice seems to be a bit verbose for the task, with some concerns regarding future adoption of SwiftUI. Happy to discuss it later, these are non-blocking thoughts.
|
|
||
| override internal func viewDidLoad() { | ||
| super.viewDidLoad() | ||
| viewModel.viewDidLoad() |
There was a problem hiding this comment.
thought;
i think if we need to inform the viewModel that controller finished/"did" load, it should be at the end of this method after doing everything? (to avoid any timing issues).
Summary
This PR removes the confirmation screen from the
BACSDirectDebitComponent, turning it into a single-screen component. This aligns BACS with the rest of our payment components (like SEPA and ACH), simplifies the user experience, and reduces checkout friction.Key Changes
submit()Method Support: Form validation and submission can now be triggered programmatically via thesubmit()function.Demo
BACS_v5.mov
BACS_v6.mov
Ticket
COSDK-1284Checklist [Required]