Remove knowledge of AppConfiguration from WPAccount Objective-C
#24231
Conversation
| [sharedDataIssueSolver migrateAuthKeyFor:username]; | ||
| } | ||
| SharedDataIssueSolver *sharedDataIssueSolver = [SharedDataIssueSolver instance]; | ||
| [sharedDataIssueSolver migrateAuthKeyFor:username]; |
There was a problem hiding this comment.
Notice that moving isJetpack into SharedDataIssueSolver would not have severed the thread because, as you can see, SharedDataIssueSolver is used in WPAccount and therefore needs to be moved to WordPressData as well.
There was a problem hiding this comment.
It is possible to move the entire + (NSString *)tokenForUsername:(NSString *)username isJetpack:(BOOL)isJetpack implementation to the app target?
There was a problem hiding this comment.
Not easily, because unfortunately it is used to get the authToken property in WPAccount (also used by Blog, which reads it from its .account).
There are ~30 usages in the production codebase between blog and account (based on a quick Search, maybe there's more), which made me hopeful in extracting it. But there's also code like this that sets the expectation for having a token stored in the model object.
[self.coreDataStack performAndSaveUsingBlock:^(NSManagedObjectContext *context) {
WPAccount *account = [context existingObjectWithID:accountObjectID error:nil];
// Even if we find an account via its userID we should still update
// its authtoken, otherwise the Authenticator's authtoken fixer won't
// work.
account.authToken = authToken;
}];So, I'm not super confident in taking off to remove it just yet.
| static func tokenForUsername(_ username: String) -> String { | ||
| token(forUsername: username, isJetpack: AppConfiguration.isJetpack) | ||
| } | ||
|
|
There was a problem hiding this comment.
Not sure whether the +Lookup extension is the best where to host this implementation, but it will do for now. "token for username" kind feels like looking up information, no?
| static func tokenForUsername(_ username: String) -> String { | ||
| token(forUsername: username, isJetpack: AppConfiguration.isJetpack) | ||
| } | ||
|
|
There was a problem hiding this comment.
Also, @kean I am aware that AppConfiguration is due for removal and that isJetpack should be removed in favor of an enum. I don't know when that's going to happen, though, so I just kicked the can down the road.
After all, this is a net zero change. I doesn't remove an AppConfiguration usage but it doesn't add one either. It simply moves it from one place to another.
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr24231-7bfce84 | |
| Version | 25.8 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 7bfce84 | |
| App Center Build | WPiOS - One-Offs #11752 |
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr24231-7bfce84 | |
| Version | 25.8 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 7bfce84 | |
| App Center Build | jetpack-installable-builds #10778 |
|
Reverting to draft. Tomorrow I'll investigate the crashes in the tests and UI tests in CI. |
|
Okay, the issue is that token(forUsername: username, isJetpack: AppConfiguration.isJetpack)returns Why does it return nil? Because NSString *authToken = [SFHFKeychainUtils getPasswordForUsername:username
andServiceName:[WPAccount authKeychainServiceName]
accessGroup:nil
error:&error];returns How was it working before, I wonder... How has the change here surfaced this issue? |
mokagio
force-pushed
the
mokagio/decouple-wpaccount-appconfiguration
branch
from
31b232f to
aabe8cf
Compare
mokagio
force-pushed
the
mokagio/decouple-wpaccount-appconfiguration
branch
from
aabe8cf to
7bfce84
Compare
Simple: The code was allowed to return Once we got to Swift, I assumed the value would have been non-nil and typed |
Part of #24165.
Similarly to #24230 , pushing knowledge of app-target-related types out of
WPAccounthelps us cutting all the threads that prevent the type from being moved out of the app target and into WordPressData.Tested by running on WordPress on device and smoke testing.