Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Replace AppleUser with SocialService.User#763

Merged
mokagio merged 8 commits into
trunkfrom
mokagio/refactor-social-service
Mar 29, 2023
Merged

Replace AppleUser with SocialService.User#763
mokagio merged 8 commits into
trunkfrom
mokagio/refactor-social-service

Conversation

@mokagio

@mokagio mokagio commented Mar 28, 2023

Copy link
Copy Markdown
Contributor

Instead of using a type with "Apple" in its name as the associated type for SocialService case .apple, we can use a general purpose type, SocialService.User. After all, there is nothing Apple-specific in the email, full name pair and the information that the service used was Google or Apple is already encoded in the case name.

The next step for this will be to make case .google use User, too, as described in #759. But I need #761 first.

The bulk of this PR is actually code to enable testing the change.

@ScoutHarris @jaclync, I asked for your review as suggested by GitHub. I assume that's because you worked on Sing In with Apple? If so, your input would be much appreciated 🙇‍♂️


  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

@mokagio mokagio requested review from ScoutHarris, crazytonyli and jaclync and removed request for crazytonyli and jaclync March 28, 2023 05:03
Comment on lines 166 to +169
socialService: SocialServiceName? = nil,
socialServiceIDToken: String? = nil,
googleUser: GIDGoogleUser? = nil,
appleUser: AppleUser? = nil) {
appleUser: SocialService.User? = nil) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Eventually, I think all these social related fields could be collapsed into one custom type to rule them all 💍

Comment on lines +1 to +2
/// A type that can create WordPress.com users given a social users, either coming from Google or Apple.
protocol SocialUserCreating: AnyObject {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is for the purpose of controlling behavior in the tests.

Comment on lines +228 to +230
let service = loginFields.meta.appleUser.map {
SocialService.apple(user: $0)
}

@mokagio mokagio Mar 28, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

1/2 changes from the code deleted above: use map instead of flatMap. There's no need to flatMap because the transformation doesn't return Optional.


updateLoginFields(email: email, fullName: name, token: token)

signupService.createWPComUserWithApple(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

2/2 changes from the code deleted above: use the signupService property instead of instantiating ad hoc.

@mokagio mokagio force-pushed the mokagio/refactor-social-service branch from 4f53f04 to 22a7b69 Compare March 28, 2023 05:18
@ScoutHarris

Copy link
Copy Markdown
Contributor

Hey @Gio2018 . My contribution was years ago, and I'm on Day One now. You might want to replace me with someone on the WP team. Sorry!

@Gio2018

Gio2018 commented Mar 28, 2023

Copy link
Copy Markdown
Contributor

Hey @Gio2018 . My contribution was years ago, and I'm on Day One now. You might want to replace me with someone on the WP team. Sorry!

Good to see you all!! I take you might have the wrong gio here? 😄 😄

@ScoutHarris

ScoutHarris commented Mar 28, 2023

Copy link
Copy Markdown
Contributor

Hahaha! Sorry @Gio2018 ! Clearly I subconsciously missed you! 😄 Hope you're doing well!

(yes, I meant @mokagio .)

@mokagio mokagio removed the request for review from ScoutHarris March 28, 2023 23:54
Comment thread WordPressAuthenticatorTests/SingIn/AppleAuthenticatorTests.swift Outdated
Comment thread WordPressAuthenticatorTests/SingIn/AppleAuthenticatorTests.swift Outdated
Co-authored-by: Tony Li <tony.li@automattic.com>
@mokagio mokagio enabled auto-merge March 29, 2023 11:10
@mokagio mokagio merged commit 47e1aef into trunk Mar 29, 2023
@mokagio mokagio deleted the mokagio/refactor-social-service branch March 29, 2023 11:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants