-
Notifications
You must be signed in to change notification settings - Fork 14
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
refactor: add proxy support - WPB-16256 #2740
refactor: add proxy support - WPB-16256 #2740
Conversation
…twork-stack-wpb-16256 # Conflicts: # WireAuthentication/Sources/WireAuthentication/Components/DetermineAuthMethodComponent.swift # WireAuthentication/Sources/WireAuthentication/Components/DetermineAuthMethodOnPremComponent.swift # WireAuthentication/Sources/WireAuthentication/Components/LoginViaEmailComponent.swift # WireAuthentication/Sources/WireAuthentication/Needle.generated.swift # WireAuthentication/Sources/WireAuthenticationUI/Mocks/MockDependencies.swift # WireAuthentication/Sources/WireAuthenticationUI/Views/DetermineAuthMethod/DetermineAuthMethodView.swift # WireAuthentication/Sources/WireAuthenticationUI/Views/Login/LoginViaEmail/LoginViaEmailView.swift # WireAuthentication/Sources/WireAuthenticationUI/Views/Login/LoginViaEmail/LoginViaEmailViewModel.swift # WireAuthentication/Sources/WireAuthenticationUI/Views/Root/RootView.swift # WireAuthentication/Tests/WireAuthenticationUITests/LoginViaEmail/LoginViaEmailViewModelTests.swift
Datadog ReportBranch report: ❌ 2 Failed (0 Known Flaky), 1697 Passed, 27 Skipped, 1m 48.17s Total Time ❌ Failed Tests (2)
|
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.
Nice work! I love that there is more code deleted than added despite adding a bunch of files. I added comments for a couple of issues that should be fixed but are simple.
WireAuthentication/Sources/WireAuthenticationLogic/NetworkStack.swift
Outdated
Show resolved
Hide resolved
WireAuthentication/Sources/WireAuthenticationAPI/Models/ProxyCredentials.swift
Show resolved
Hide resolved
...uthentication/Sources/WireAuthenticationUI/Views/Login/LoginViaEmail/LoginViaEmailView.swift
Outdated
Show resolved
Hide resolved
...uthentication/Sources/WireAuthenticationUI/Views/Login/LoginViaEmail/LoginViaEmailView.swift
Outdated
Show resolved
Hide resolved
WireAuthentication/Sources/WireAuthenticationLogic/SubmitProxyCredentialsUseCase.swift
Show resolved
Hide resolved
Test Results 2 files 3 suites 2m 7s ⏱️ For more details on these failures, see this check. Results for commit 78bc22f. ♻️ This comment has been updated with latest results. |
…twork-stack-wpb-16256
import Foundation | ||
import WireAuthenticationAPI | ||
|
||
package struct ValidateEmailUseCase: ValidateEmailUseCaseProtocol { |
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.
thought (non-blocking): I think this is worth discussing in some kind of retrospective of the architecture. I wonder if sometimes we are being forced to add too much boilerplate.
- The implementation here
- Tests for the implementation
- The new type
EmailValidationResult
- Some kind of mock. This mocking I particularly don't like as I think it adds brittleness but I'm a classist not a mockist kind of tester
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.
Certainly! I think also some boilerplate could be avoided using macros.
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.
Nice work! I think there may be a couple of issues around password validation. Ping me when done and I will approve
...tication/Sources/WireAuthenticationUI/Views/Login/LoginViaEmail/LoginViaEmailViewModel.swift
Show resolved
Hide resolved
...uthentication/Sources/WireAuthenticationUI/Views/Login/LoginViaEmail/LoginViaEmailView.swift
Outdated
Show resolved
Hide resolved
...tication/Sources/WireAuthenticationUI/Views/Login/LoginViaEmail/LoginViaEmailViewModel.swift
Show resolved
Hide resolved
.../ReferenceImages/LoginViaEmailViewTests/testColorSchemeVariantsWithoutCreateAccount.dark.png
Outdated
Show resolved
Hide resolved
343d00c
into
refactor/support-proxy-mode-wpb-16256
Issue
This PR adds proxy support in
WireAuthentication
. A number of refactoring were necessary:NetworkStack
to encapsulate network related objects to simplify the component graph, it ensures there is a valid network service, performs api resolving, handles proxy credentials etc.Factory
objectsMockDependencies
into separate filesRootView
Testing
Note
Several tests are missing, but we will add these in another PR.
Checklist
[WPB-XXX]
.UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: