Skip to content

Remove knowledge of AppConfiguration from WPAccount Objective-C #24231

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 20, 2025
Merged

Remove knowledge of AppConfiguration from WPAccount Objective-C #24231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa31858
Remove knowledge of `AppConfiguration` from WPAccount Objective-C
mokagio Mar 17, 2025
7bfce84
Account for token coming from `WPAccount` possibly being `nil`
mokagio Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions WordPress/Classes/Models/WPAccount+Lookup.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@ public extension WPAccount {
return self.uuid == uuid
}

// This is here in an extension that belongs to the apps target so we can decouple WPAccount from AppConfiguration.
// Decoupling allows moving the type to WordPressData, see https://github.com/wordpress-mobile/WordPress-iOS/issues/24165.
@objc
static func tokenForUsername(_ username: String) -> String? {
token(forUsername: username, isJetpack: AppConfiguration.isJetpack)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

/// Does this `WPAccount` object have any associated blogs?
///
@objc
Expand Down
2 changes: 1 addition & 1 deletion WordPress/Classes/Models/WPAccount.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
- (void)removeBlogsObject:(Blog *)value;
- (void)addBlogs:(NSSet *)values;
- (void)removeBlogs:(NSSet *)values;
+ (NSString *)tokenForUsername:(NSString *)username;
+ (NSString *)tokenForUsername:(NSString *)username isJetpack:(BOOL)isJetpack;
- (BOOL)hasAtomicSite;

@end
13 changes: 7 additions & 6 deletions WordPress/Classes/Models/WPAccount.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,13 @@ - (BOOL)hasAtomicSite {

#pragma mark - Static methods

+ (NSString *)tokenForUsername:(NSString *)username
+ (NSString *)tokenForUsername:(NSString *)username isJetpack:(BOOL)isJetpack
{
if (isJetpack) {
[WPAccount migrateAuthKeyForUsername:username];
}

NSError *error = nil;
[WPAccount migrateAuthKeyForUsername:username];
NSString *authToken = [SFHFKeychainUtils getPasswordForUsername:username
andServiceName:[WPAccount authKeychainServiceName]
accessGroup:nil
Expand All @@ -147,10 +150,8 @@ + (void)migrateAuthKeyForUsername:(NSString *)username
{
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
if ([AppConfiguration isJetpack]) {
SharedDataIssueSolver *sharedDataIssueSolver = [SharedDataIssueSolver instance];
[sharedDataIssueSolver migrateAuthKeyFor:username];
}
SharedDataIssueSolver *sharedDataIssueSolver = [SharedDataIssueSolver instance];
[sharedDataIssueSolver migrateAuthKeyFor:username];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible to move the entire + (NSString *)tokenForUsername:(NSString *)username isJetpack:(BOOL)isJetpack implementation to the app target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

});
}

Expand Down