Skip to content

Decouple WPAccount from WordPressAuthenticationManager - #24392

Merged
mokagio merged 6 commits into
trunkfrom
mokagio/wpaccount-notifications
Apr 6, 2025
Merged

Decouple WPAccount from WordPressAuthenticationManager#24392
mokagio merged 6 commits into
trunkfrom
mokagio/wpaccount-notifications

Conversation

@mokagio

@mokagio mokagio commented Apr 2, 2025

Copy link
Copy Markdown
Contributor

Part of #24165. See also p1743540472163989-slack-C08HQR4C2TS.

There's logic in WPAccount that generates an API client and checks the associated state. If the client token is "invalid", it presents a dedicated sign-in screen to re-authenticate.

This is a bit of a blocker for moving WPAccount to WordPressData, because of all the dependencies that come with the authentication process, not to mention the fact that authentication should be unrelated with WordPressData.

To decouple the two, we can make WPAccount post a notification instead of directly triggering the sign-in presentation.

I don't know how to test this in the real world, but I covered the new logic with unit tests where possible.

@mokagio mokagio added this to the 25.9 milestone Apr 2, 2025
@mokagio mokagio added the Core Data Issues related to Core Data label Apr 2, 2025
@mokagio mokagio self-assigned this Apr 2, 2025
@dangermattic

dangermattic commented Apr 2, 2025

Copy link
Copy Markdown
Collaborator
1 Warning
⚠️ This PR is assigned to the milestone 25.9. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

@mokagio
mokagio force-pushed the mokagio/wpaccount-notifications branch from eb98d90 to d5b1292 Compare April 2, 2025 03:40
@mokagio
mokagio force-pushed the mokagio/wpaccount-notifications branch from d5b1292 to 4d2af8c Compare April 2, 2025 05:07
@wpmobilebot

wpmobilebot commented Apr 2, 2025

Copy link
Copy Markdown
Contributor
App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
ConfigurationRelease-Alpha
Build Number27156
VersionPR #24392
Bundle IDorg.wordpress.alpha
Commita6a94f4
Installation URL4hor8llih9n3g
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

wpmobilebot commented Apr 2, 2025

Copy link
Copy Markdown
Contributor
App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
ConfigurationRelease-Alpha
Build Number27156
VersionPR #24392
Bundle IDcom.jetpack.alpha
Commita6a94f4
Installation URL1hsga40m2kj20
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

Comment thread WordPress/Classes/Models/WPAccount+RestApi.swift Outdated
@mokagio
mokagio force-pushed the mokagio/wpaccount-notifications branch from 4d2af8c to 5c5c991 Compare April 2, 2025 09:07
@mokagio

mokagio commented Apr 3, 2025

Copy link
Copy Markdown
Contributor Author

Kept trying to figure out what the issues was with the unit tests getting stuck after my change...

image

I thought it had to do with the keychain warning in the console.

Then I finally thought to look at the Simulator (🤦‍♂️ for not thinking of it straightaway)

image

I'll timebox 30 mins for this. If I don't figure it out, I'll just remove the test in the interest of moving forward with #24165.

@mokagio
mokagio force-pushed the mokagio/wpaccount-notifications branch from 5c5c991 to 76aca24 Compare April 3, 2025 00:48
@mokagio
mokagio marked this pull request as ready for review April 3, 2025 04:24
@mokagio
mokagio force-pushed the mokagio/wpaccount-notifications branch from 4131962 to 9f043d1 Compare April 3, 2025 04:24
}

guard let authToken, !authToken.isEmpty else {
DispatchQueue.main.async {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we should delete this dispatch async call. We need to either post the notification from the main thread or respond to the notification from the main thread.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wordPressComRestApi gets accessed from the background threads.

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.

I went for handling the notification on the main thread asynchronously.

I thought of using a block to leverage the queue: .main argument, 297481e, but it adds the need to store the observer and then remove it. Using the #selector is not as compact, but requires remembering less things, 4023911

@mokagio
mokagio force-pushed the mokagio/wpaccount-notifications branch from 297481e to 4023911 Compare April 6, 2025 21:59
@mokagio
mokagio enabled auto-merge April 6, 2025 22:17
@mokagio
mokagio added this pull request to the merge queue Apr 6, 2025
Merged via the queue into trunk with commit d933e3c Apr 6, 2025
@mokagio
mokagio deleted the mokagio/wpaccount-notifications branch April 6, 2025 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Core Data Issues related to Core Data

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants