Skip to content

Commit 731160d

Browse files
brandonpageclaude
andcommitted
Address review: extract isDiscoveryDomain class method; fix test names
- Extract the stateless isDiscoveryDomain check to a class method on SFDomainDiscoveryCoordinator so callers no longer alloc a throwaway instance. The instance method delegates to it; existing callers are unchanged. Converts the LFA menu-visibility check, the LFA phase-1 guard, and the pre-existing setCurrentUser discovery check. - Add class-method + instance/class parity tests. - Capitalize "LoginAsAdmin"/"LoginForAdmin" in test method names (property/variable references stay lowercase). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent cba3d81 commit 731160d

5 files changed

Lines changed: 56 additions & 23 deletions

File tree

libs/SalesforceSDKCore/SalesforceSDKCore/Classes/Login/SFLoginViewController.m

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -376,8 +376,7 @@ + (BOOL)shouldShowLoginForAdminForSession:(nullable SFSDKAuthSession *)session {
376376
}
377377

378378
NSString *loginHost = session.oauthRequest.loginHost;
379-
SFDomainDiscoveryCoordinator *discoveryCoordinator = [[SFDomainDiscoveryCoordinator alloc] init];
380-
BOOL isDiscoveryHost = [discoveryCoordinator isDiscoveryDomain:loginHost];
379+
BOOL isDiscoveryHost = [SFDomainDiscoveryCoordinator isDiscoveryDomain:loginHost];
381380
// Hide only when we are mid-discovery (no My Domain selected yet).
382381
if (isDiscoveryHost && !coordinator.domainUpdated) {
383382
return NO;

libs/SalesforceSDKCore/SalesforceSDKCore/Classes/OAuth/DomainDiscoveryCoordinator.swift

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,11 @@ public class DomainDiscoveryCoordinator: NSObject {
100100

101101
@objc
102102
public func isDiscoveryDomain(_ domain: String?) -> Bool {
103+
return Self.isDiscoveryDomain(domain)
104+
}
105+
106+
/// Whether the given login host is a My Domain discovery host (e.g.`welcome.salesforce.com/discovery`).
107+
public class func isDiscoveryDomain(_ domain: String?) -> Bool {
103108
guard let domain = domain else { return false }
104109
let isDiscovery = domain.lowercased().contains(DomainDiscovery.URLComponent.path.rawValue)
105110
return isDiscovery

libs/SalesforceSDKCore/SalesforceSDKCore/Classes/UserAccount/SFUserAccountManager.m

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1152,8 +1152,7 @@ - (void)loginViewControllerDidSelectLoginForAdmin:(SFLoginViewController *)login
11521152
// and we have no My Domain to advance to. Switching to ASWebAuthenticationSession
11531153
// here would launch the browser against welcome.salesforce.com — wrong UX.
11541154
// No-op until phase 2 lands.
1155-
SFDomainDiscoveryCoordinator *discoveryCoordinator = [[SFDomainDiscoveryCoordinator alloc] init];
1156-
if ([discoveryCoordinator isDiscoveryDomain:session.oauthRequest.loginHost] && !coordinator.domainUpdated) {
1155+
if ([SFDomainDiscoveryCoordinator isDiscoveryDomain:session.oauthRequest.loginHost] && !coordinator.domainUpdated) {
11571156
[SFSDKCoreLogger w:[self class] format:@"%@: Login for Admin is not available before a My Domain has been selected on the Welcome Discovery page; ignoring.", NSStringFromSelector(_cmd)];
11581157
return;
11591158
}
@@ -1750,7 +1749,7 @@ - (void)setCurrentUserInternal:(SFUserAccount*)user {
17501749
// next login is web based it should not try to use that url.
17511750
// Also skip if the app uses a Welcome/Discovery domain — persisting the My Domain
17521751
// would pollute the server picker and prevent returning to the discovery page on logout.
1753-
BOOL isDiscoveryLogin = [[[SFDomainDiscoveryCoordinator alloc] init] isDiscoveryDomain:self.loginHost];
1752+
BOOL isDiscoveryLogin = [SFDomainDiscoveryCoordinator isDiscoveryDomain:self.loginHost];
17541753
if (user.credentials.domain && !isNativeLogin && !isDiscoveryLogin)
17551754
self.loginHost = user.credentials.domain;
17561755
[self didChangeValueForKey:@"currentUser"];

libs/SalesforceSDKCore/SalesforceSDKCoreTests/LoginForAdminTests.swift

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -46,12 +46,12 @@ class LoginForAdminTests: XCTestCase {
4646

4747
// MARK: - SFSDKAuthRequest loginAsAdmin Property
4848

49-
func testGivenNewAuthRequest_whenCreated_thenloginAsAdminIsFalse() {
49+
func testGivenNewAuthRequest_whenCreated_thenLoginAsAdminIsFalse() {
5050
let request = SFSDKAuthRequest()
5151
XCTAssertFalse(request.loginAsAdmin, "loginAsAdmin should default to false")
5252
}
5353

54-
func testGivenAuthRequest_whenloginAsAdminSet_thenUseBrowserAuthUnchanged() {
54+
func testGivenAuthRequest_whenLoginAsAdminSet_thenUseBrowserAuthUnchanged() {
5555
let request = SFSDKAuthRequest()
5656
XCTAssertFalse(request.useBrowserAuth, "useBrowserAuth should default to false")
5757

@@ -63,7 +63,7 @@ class LoginForAdminTests: XCTestCase {
6363

6464
// MARK: - SFSDKAuthSession Coordinator Initialization
6565

66-
func testGivenloginAsAdmin_whenAuthSessionCreated_thenCoordinatorUsesBrowserAuth() {
66+
func testGivenLoginAsAdmin_whenAuthSessionCreated_thenCoordinatorUsesBrowserAuth() {
6767
let request = makeAuthRequest()
6868
request.loginAsAdmin = true
6969
request.useBrowserAuth = false
@@ -95,7 +95,7 @@ class LoginForAdminTests: XCTestCase {
9595

9696
// MARK: - SFOAuthCoordinator Auth Info Type
9797

98-
func testGivenloginAsAdmin_whenAuthenticate_thenAuthInfoIsAdvancedBrowser() {
98+
func testGivenLoginAsAdmin_whenAuthenticate_thenAuthInfoIsAdvancedBrowser() {
9999
createTestAppIdentity()
100100

101101
let request = makeAuthRequest()
@@ -149,7 +149,7 @@ class LoginForAdminTests: XCTestCase {
149149

150150
// MARK: - Approval URL Web Server Flow
151151

152-
func testGivenloginAsAdmin_whenGenerateApprovalUrl_thenUsesWebServerFlow() {
152+
func testGivenLoginAsAdmin_whenGenerateApprovalUrl_thenUsesWebServerFlow() {
153153
createTestAppIdentity()
154154
SalesforceManager.shared.useWebServerAuthentication = false
155155

@@ -165,7 +165,7 @@ class LoginForAdminTests: XCTestCase {
165165
"Approval URL should not use response_type=token when loginAsAdmin is true")
166166
}
167167

168-
func testGivenNologinAsAdmin_whenWebServerAuthDisabled_thenUsesUserAgentFlow() {
168+
func testGivenNoLoginAsAdmin_whenWebServerAuthDisabled_thenUsesUserAgentFlow() {
169169
createTestAppIdentity()
170170
SalesforceManager.shared.useWebServerAuthentication = false
171171
SalesforceManager.shared.useHybridAuthentication = false
@@ -183,7 +183,7 @@ class LoginForAdminTests: XCTestCase {
183183

184184
// MARK: - No Global State Mutation
185185

186-
func testGivenloginAsAdmin_whenSet_thenGlobalWebServerAuthUnchanged() {
186+
func testGivenLoginAsAdmin_whenSet_thenGlobalWebServerAuthUnchanged() {
187187
let originalValue = SalesforceManager.shared.useWebServerAuthentication
188188

189189
let request = SFSDKAuthRequest()
@@ -193,7 +193,7 @@ class LoginForAdminTests: XCTestCase {
193193
"Setting loginAsAdmin should not change the global useWebServerAuthentication")
194194
}
195195

196-
func testGivenloginAsAdmin_whenAuthSessionCreated_thenGlobalStateUnchanged() {
196+
func testGivenLoginAsAdmin_whenAuthSessionCreated_thenGlobalStateUnchanged() {
197197
SalesforceManager.shared.useWebServerAuthentication = false
198198

199199
let request = makeAuthRequest()
@@ -209,7 +209,7 @@ class LoginForAdminTests: XCTestCase {
209209

210210
// MARK: - Cancel Flow: loginAsAdmin Clears on Cancel
211211

212-
func testGivenloginAsAdmin_whenCancelled_thenloginAsAdminCleared() {
212+
func testGivenLoginAsAdmin_whenCancelled_thenLoginAsAdminCleared() {
213213
let request = makeAuthRequest()
214214
request.loginAsAdmin = true
215215

@@ -226,7 +226,7 @@ class LoginForAdminTests: XCTestCase {
226226
"Coordinator should not use browser auth after loginAsAdmin is cleared")
227227
}
228228

229-
func testGivenloginAsAdminCancelled_whenNewSession_thenAuthInfoMatchesGlobalSetting() {
229+
func testGivenLoginAsAdminCancelled_whenNewSession_thenAuthInfoMatchesGlobalSetting() {
230230
createTestAppIdentity()
231231
SalesforceManager.shared.useWebServerAuthentication = true
232232

@@ -270,7 +270,7 @@ class LoginForAdminTests: XCTestCase {
270270

271271
// MARK: - useBrowserAuth Not Modified
272272

273-
func testGivenloginAsAdmin_whenFullLifecycle_thenUseBrowserAuthNeverMutated() {
273+
func testGivenLoginAsAdmin_whenFullLifecycle_thenUseBrowserAuthNeverMutated() {
274274
let request = makeAuthRequest()
275275
request.useBrowserAuth = false
276276

@@ -302,7 +302,7 @@ class LoginForAdminTests: XCTestCase {
302302

303303
// MARK: - SFUserAccountManager Cancel Browser Auth (loginAsAdmin path)
304304

305-
func testGivenloginAsAdmin_whenBrowserAuthCancelled_thenloginAsAdminCleared() {
305+
func testGivenLoginAsAdmin_whenBrowserAuthCancelled_thenLoginAsAdminCleared() {
306306
let request = makeAuthRequest()
307307
request.loginAsAdmin = true
308308

@@ -317,7 +317,7 @@ class LoginForAdminTests: XCTestCase {
317317
XCTAssertFalse(session.oauthRequest.loginAsAdmin, "loginAsAdmin should be cleared after cancel")
318318
}
319319

320-
func testGivenloginAsAdmin_whenBrowserAuthCancelled_thenUserCancelledNotificationNotPosted() {
320+
func testGivenLoginAsAdmin_whenBrowserAuthCancelled_thenUserCancelledNotificationNotPosted() {
321321
let request = makeAuthRequest()
322322
request.loginAsAdmin = true
323323

@@ -340,7 +340,7 @@ class LoginForAdminTests: XCTestCase {
340340
NotificationCenter.default.removeObserver(observer)
341341
}
342342

343-
func testGivenNologinAsAdmin_whenBrowserAuthCancelled_thenUserCancelledNotificationPosted() {
343+
func testGivenNoLoginAsAdmin_whenBrowserAuthCancelled_thenUserCancelledNotificationPosted() {
344344
let request = makeAuthRequest()
345345
request.loginAsAdmin = false
346346
request.useBrowserAuth = false
@@ -373,7 +373,7 @@ class LoginForAdminTests: XCTestCase {
373373
UserAccountManager.shared.authCancelledByUserHandlerBlock = nil
374374
}
375375

376-
func testGivenloginAsAdmin_whenBrowserAuthCancelled_thenHandlerBlockNotCalled() {
376+
func testGivenLoginAsAdmin_whenBrowserAuthCancelled_thenHandlerBlockNotCalled() {
377377
let request = makeAuthRequest()
378378
request.loginAsAdmin = true
379379

@@ -444,7 +444,7 @@ class LoginForAdminTests: XCTestCase {
444444

445445
// MARK: - SFUserAccountManager loginViewControllerDidSelectLoginForAdmin
446446

447-
func testGivenAuthSession_whenLoginForAdminSelected_thenloginAsAdminSetAndAuthRestarted() {
447+
func testGivenAuthSession_whenLoginForAdminSelected_thenLoginAsAdminSetAndAuthRestarted() {
448448
let uam = UserAccountManager.shared
449449

450450
// Get the test app's active window scene to obtain a real sceneId
@@ -611,7 +611,7 @@ class LoginForAdminTests: XCTestCase {
611611
window.rootViewController = nil
612612
}
613613

614-
func test_authRequestRoundTripsloginAsAdminOverrides() {
614+
func test_authRequestRoundTripsLoginAsAdminOverrides() {
615615
// The LFA-scoped override fields on SFSDKAuthRequest must round-trip so that
616616
// restartAuthentication: can forward them through authenticateWithRequest:loginHint:
617617
// without mutating the request's permanent loginHost.
@@ -702,7 +702,7 @@ class LoginForAdminTests: XCTestCase {
702702
}
703703

704704
@available(*, deprecated, message: "Exercises deprecated public API")
705-
func testGivenAuthSession_whenPublicLoginForAdminCalled_thenloginAsAdminSet() {
705+
func testGivenAuthSession_whenPublicLoginForAdminCalled_thenLoginAsAdminSet() {
706706
let uam = UserAccountManager.shared
707707

708708
guard let windowScene = UIApplication.shared.connectedScenes.first as? UIWindowScene else {

libs/SalesforceSDKCore/SalesforceSDKCoreTests/WelcomeDiscoveryLoginHostTests.swift

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,36 @@ final class WelcomeDiscoveryLoginHostTests: XCTestCase {
8585
XCTAssertFalse(coordinator.isDiscoveryDomain(nil))
8686
}
8787

88+
// MARK: - DomainDiscoveryCoordinator.isDiscoveryDomain class method tests
89+
90+
func test_givenDiscoveryDomain_whenCheckingIsDiscoveryDomainClassMethod_thenReturnsTrue() {
91+
XCTAssertTrue(DomainDiscoveryCoordinator.isDiscoveryDomain(discoveryDomain))
92+
XCTAssertTrue(DomainDiscoveryCoordinator.isDiscoveryDomain("welcome.salesforce.com/discovery"))
93+
XCTAssertTrue(DomainDiscoveryCoordinator.isDiscoveryDomain("mycompany.salesforce.com/discovery"))
94+
}
95+
96+
func test_givenNonDiscoveryDomain_whenCheckingIsDiscoveryDomainClassMethod_thenReturnsFalse() {
97+
XCTAssertFalse(DomainDiscoveryCoordinator.isDiscoveryDomain(myDomain))
98+
XCTAssertFalse(DomainDiscoveryCoordinator.isDiscoveryDomain("login.salesforce.com"))
99+
XCTAssertFalse(DomainDiscoveryCoordinator.isDiscoveryDomain("test.salesforce.com"))
100+
}
101+
102+
func test_givenNilDomain_whenCheckingIsDiscoveryDomainClassMethod_thenReturnsFalse() {
103+
XCTAssertFalse(DomainDiscoveryCoordinator.isDiscoveryDomain(nil))
104+
}
105+
106+
func test_givenAnyDomain_whenComparingClassAndInstanceIsDiscoveryDomain_thenResultsMatch() {
107+
// The instance method now delegates to the class method; both must agree so
108+
// existing callers (e.g. SFOAuthCoordinator) and the new class-method callers
109+
// stay in lockstep.
110+
let coordinator = DomainDiscoveryCoordinator()
111+
for domain in [discoveryDomain, myDomain, "login.salesforce.com", "x.salesforce.com/discovery", nil] {
112+
XCTAssertEqual(coordinator.isDiscoveryDomain(domain),
113+
DomainDiscoveryCoordinator.isDiscoveryDomain(domain),
114+
"Instance and class isDiscoveryDomain must agree for \(domain ?? "nil")")
115+
}
116+
}
117+
88118
// MARK: - Login host persistence tests
89119

90120
func test_givenDiscoveryLoginHost_whenSetCurrentUser_thenLoginHostNotOverwritten() {

0 commit comments

Comments
 (0)