Skip to content
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

Configure the AuthenticationService later now that we have 2 flows on the start screen. #3316

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

pixlwave
Copy link
Member

@pixlwave pixlwave commented Sep 23, 2024

This PR is the first of a series to re-work the authentication flow:

  • We no longer configure the authentication service before proceeding from the AuthenticationStartScreen. Instead we do it either when selecting a server, or if this hasn't happened, when continuing to use the default homeserver. (This lines up with how the flow works on Android).
  • We can now show an alert when attempting to register on a server that isn't open for registration instead of setting the subtitle on the screen which wasn't particularly obvious.
  • Allow the creation of the AuthenticationService with a client builder factory so we can test it with a ClientSDKMock.
  • Make the factory preconfigured enough to replace the hand built MockAuthenticationService with the real one in all of our tests.
  • Add a bunch of tests on the AuthenticationService and ServerConfirmationScreenViewModel.

Fixes #3028 from the perspective of if matrix.org is blocked (although that long timeout still needs addressing if you were to tap continue to try matrix.org in this instance.

More tests to follow, but this PR has already grown larger than I would have liked. Definitely one to review commit-by-commit.

@pixlwave pixlwave requested a review from a team as a code owner September 23, 2024 08:51
@pixlwave pixlwave requested review from stefanceriu and removed request for a team September 23, 2024 08:51
Copy link

github-actions bot commented Sep 23, 2024

Warnings
⚠️ This pull request seems relatively large. Please consider splitting it into multiple smaller ones.
⚠️ Some of the commits are missing ticket numbers. Please consider squashing all commits that don't have a tracking number.
⚠️ You seem to have made changes to views. Please consider adding screenshots.

Generated by 🚫 Danger Swift against 5af32d9

@pixlwave pixlwave added the pr-change for updates to an existing feature label Sep 23, 2024
Copy link

codecov bot commented Sep 23, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
832 1 831 0
View the top 1 failed tests by shortest run time
UserSessionFlowCoordinatorTests testRoomClearsStack()
Stack Traces | 1.83s run time
XCTAssertTrue failed (.../UnitTests/Sources/UserSessionFlowCoordinatorTests.swift:190)

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@pixlwave pixlwave force-pushed the doug/delay-configure-homeserver branch from b6569a9 to abe1ecb Compare September 23, 2024 15:30
Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

Very very nice cleanup and testing! 👏
Talked about general api design concerns offline, feel free to tackle them in a separate PR if necessary.


@testable import ElementX

class AuthenticationServiceTests: XCTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

👏

@@ -107,6 +106,8 @@ final class ServerSelectionScreenCoordinator: CoordinatorProtocol {
viewModel.displayError(.invalidWellKnownAlert(error))
case .slidingSyncNotAvailable:
viewModel.displayError(.slidingSyncAlert)
case .registrationNotSupported:
viewModel.displayError(.registrationAlert) // TODO: [DOUG] Test me!
Copy link
Member

Choose a reason for hiding this comment

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

😅

Copy link
Member Author

@pixlwave pixlwave Sep 25, 2024

Choose a reason for hiding this comment

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

(I have this already in a follow-up branch).


// MARK: - Helpers

private func setupMocks(clientConfiguration: ClientSDKMock.Configuration = .init()) {
Copy link
Member

Choose a reason for hiding this comment

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

Really nice!

// MARK: - Mocks

extension AuthenticationService {
static var mock = AuthenticationService(userSessionStore: UserSessionStoreMock(configuration: .init()),
Copy link
Member

Choose a reason for hiding this comment

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

🙌

@@ -11,5 +11,117 @@ import XCTest

@MainActor
class ServerConfirmationScreenViewModelTests: XCTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

image

@pixlwave pixlwave force-pushed the doug/delay-configure-homeserver branch from abe1ecb to 5af32d9 Compare September 25, 2024 09:12
Copy link

sonarcloud bot commented Sep 25, 2024

@pixlwave
Copy link
Member Author

Merging this as the snapshot failures are emoji provider flakiness. Going to spend some time on these failures now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-change for updates to an existing feature
Projects
None yet
2 participants