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

Refactor SharingService to use CoreDataStack #20265

Merged
merged 12 commits into from
Mar 7, 2023
Merged
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
22.0
-----
* [*] [internal] Refactor managing social connections and social buttons in the "Sharing" screen. [#20265]


21.9
Expand Down
30 changes: 30 additions & 0 deletions WordPress/Classes/Models/PublicizeService+Lookup.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
extension PublicizeService {
/// Finds a cached `PublicizeService` matching the specified service name.
///
/// - Parameter name: The name of the service. This is the `serviceID` attribute for a `PublicizeService` object.
///
/// - Returns: The requested `PublicizeService` or nil.
///
static func lookupPublicizeServiceNamed(_ name: String, in context: NSManagedObjectContext) throws -> PublicizeService? {
let request = NSFetchRequest<PublicizeService>(entityName: PublicizeService.classNameWithoutNamespaces())
request.predicate = NSPredicate(format: "serviceID = %@", name)
return try context.fetch(request).first
Comment on lines +8 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I called this out before, but one great benefit of all this work is how much more Swifty the code is getting by leveraging throws in many more places as opposed to swallowing errors or return .none as a sign of error.

}

@objc(lookupPublicizeServiceNamed:inContext:)
static func objc_lookupPublicizeServiceNamed(_ name: String, in context: NSManagedObjectContext) -> PublicizeService? {
try? lookupPublicizeServiceNamed(name, in: context)
Copy link
Contributor

Choose a reason for hiding this comment

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

LOL. I just commented above about the code being better because it doesn't return .none as a sign of error... 🙃

Of course, this is a special case method, behaving that way to fit with the Objective-C layer 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, a little compromise for Objective-C code.

}

/// Returns an array of all cached `PublicizeService` objects.
///
/// - Returns: An array of `PublicizeService`. The array is empty if no objects are cached.
///
@objc(allPublicizeServicesInContext:error:)
static func allPublicizeServices(in context: NSManagedObjectContext) throws -> [PublicizeService] {
let request = NSFetchRequest<PublicizeService>(entityName: PublicizeService.classNameWithoutNamespaces())
let sortDescriptor = NSSortDescriptor(key: "order", ascending: true)
request.sortDescriptors = [sortDescriptor]
return try context.fetch(request)
}
}
29 changes: 29 additions & 0 deletions WordPress/Classes/Models/SharingButton+Lookup.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
extension SharingButton {

/// Returns an array of all cached `SharingButtons` objects.
///
/// - Returns: An array of `SharingButton`s. The array is empty if no objects are cached.
///
@objc(allSharingButtonsForBlog:inContext:error:)
static func allSharingButtons(for blog: Blog, in context: NSManagedObjectContext) throws -> [SharingButton] {
let request = NSFetchRequest<SharingButton>(entityName: SharingButton.classNameWithoutNamespaces())
request.predicate = NSPredicate(format: "blog = %@", blog)
request.sortDescriptors = [NSSortDescriptor(key: "order", ascending: true)]
return try context.fetch(request)
}

/// Finds a cached `SharingButton` by its `buttonID` for the specified `Blog`
///
/// - Parameters:
/// - buttonID: The button ID of the `SharingButton`.
/// - blog: The blog that owns the sharing button.
///
/// - Returns: The requested `SharingButton` or nil.
///
static func lookupSharingButton(byID buttonID: String, for blog: Blog, in context: NSManagedObjectContext) throws -> SharingButton? {
let request = NSFetchRequest<SharingButton>(entityName: SharingButton.classNameWithoutNamespaces())
request.predicate = NSPredicate(format: "buttonID = %@ AND blog = %@", buttonID, blog)
return try context.fetch(request).first
}

}
414 changes: 182 additions & 232 deletions WordPress/Classes/Services/SharingService.swift

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ - (void)fetchKeyringConnectionsForService:(PublicizeService *)pubServ
[self.delegate sharingAuthorizationHelper:self willFetchKeyringsForService:self.publicizeService];
}

SharingService *sharingService = [[SharingService alloc] initWithManagedObjectContext:[self.blog managedObjectContext]];
SharingService *sharingService = [[SharingService alloc] initWithContextManager:[ContextManager sharedInstance]];
__weak __typeof__(self) weakSelf = self;
[sharingService fetchKeyringConnectionsForBlog:self.blog success:^(NSArray *keyringConnections) {
if ([weakSelf.delegate respondsToSelector:@selector(sharingAuthorizationHelper:didFetchKeyringsForService:)]) {
Expand Down Expand Up @@ -340,7 +340,7 @@ - (void)updateConnection:(PublicizeConnection *)publicizeConnection forKeyringCo

[self dismissNavViewController];

SharingService *sharingService = [[SharingService alloc] initWithManagedObjectContext:[self.blog managedObjectContext]];
SharingService *sharingService = [[SharingService alloc] initWithContextManager:[ContextManager sharedInstance]];

[sharingService updateExternalID:externalID forBlog:self.blog forPublicizeConnection:publicizeConnection success:^{
if ([self.delegate respondsToSelector:@selector(sharingAuthorizationHelper:didConnectToService:withPublicizeConnection:)]) {
Expand All @@ -367,7 +367,7 @@ - (void)connectToServiceWithKeyringConnection:(KeyringConnection *)keyConn andEx

[self dismissNavViewController];

SharingService *sharingService = [[SharingService alloc] initWithManagedObjectContext:[self.blog managedObjectContext]];
SharingService *sharingService = [[SharingService alloc] initWithContextManager:[ContextManager sharedInstance]];
[sharingService createPublicizeConnectionForBlog:self.blog
keyring:keyConn
externalUserID:externalUserID
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,7 @@ import WordPressShared
action: #selector(doneButtonTapped))
}

let service = SharingService(managedObjectContext: viewContext)
buttons = service.allSharingButtonsForBlog(self.blog)
buttons = (try? SharingButton.allSharingButtons(for: blog, in: viewContext)) ?? []
configureTableView()
setupSections()

Expand Down Expand Up @@ -555,7 +554,7 @@ import WordPressShared
/// when finished. Fails silently if there is an error.
///
private func syncSharingButtons() {
let service = SharingService(managedObjectContext: viewContext)
let service = SharingService(coreDataStack: ContextManager.shared)
service.syncSharingButtonsForBlog(self.blog,
success: { [weak self] in
self?.reloadButtons()
Expand Down Expand Up @@ -641,8 +640,7 @@ import WordPressShared
/// `buttons` property and refreshes the button section and the more section.
///
private func reloadButtons() {
let service = SharingService(managedObjectContext: viewContext)
buttons = service.allSharingButtonsForBlog(blog)
buttons = (try? SharingButton.allSharingButtons(for: blog, in: viewContext)) ?? []

refreshButtonsSection()
refreshMoreSection()
Expand All @@ -653,7 +651,7 @@ import WordPressShared
/// - Parameter refresh: True if the tableview sections should be reloaded.
///
private func syncButtonChangesToBlog(_ refresh: Bool) {
let service = SharingService(managedObjectContext: viewContext)
let service = SharingService(coreDataStack: ContextManager.shared)
service.updateSharingButtonsForBlog(blog,
sharingButtons: buttons,
success: {[weak self] in
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ - (instancetype)initWithBlog:(Blog *)blog
if (self) {
_blog = blog;
_publicizeConnection = connection;
SharingService *sharingService = [[SharingService alloc] initWithManagedObjectContext:[self managedObjectContext]];
PublicizeService *publicizeService = [sharingService findPublicizeServiceNamed:connection.service];
PublicizeService *publicizeService = [PublicizeService lookupPublicizeServiceNamed:connection.service inContext:[self managedObjectContext]];
if (publicizeService) {
self.helper = [[SharingAuthorizationHelper alloc] initWithViewController:self
blog:self.blog
Expand Down Expand Up @@ -206,7 +205,7 @@ - (SwitchTableViewCell *)switchTableViewCell
- (void)updateSharedGlobally:(BOOL)shared
{
__weak __typeof(self) weakSelf = self;
SharingService *sharingService = [[SharingService alloc] initWithManagedObjectContext:[self managedObjectContext]];
SharingService *sharingService = [[SharingService alloc] initWithContextManager:[ContextManager sharedInstance]];
[sharingService updateSharedForBlog:self.blog
shared:shared
forPublicizeConnection:self.publicizeConnection
Expand All @@ -225,7 +224,7 @@ - (void)reconnectPublicizeConnection
return;
}

SharingService *sharingService = [[SharingService alloc] initWithManagedObjectContext:[self managedObjectContext]];
SharingService *sharingService = [[SharingService alloc] initWithContextManager:[ContextManager sharedInstance]];

__weak __typeof(self) weakSelf = self;
if (self.helper == nil) {
Expand All @@ -243,7 +242,7 @@ - (void)reconnectPublicizeConnection

- (void)disconnectPublicizeConnection
{
SharingService *sharingService = [[SharingService alloc] initWithManagedObjectContext:[self managedObjectContext]];
SharingService *sharingService = [[SharingService alloc] initWithContextManager:[ContextManager sharedInstance]];
[sharingService deletePublicizeConnectionForBlog:self.blog pubConn:self.publicizeConnection success:nil failure:^(NSError *error) {
DDLogError([error description]);
[SVProgressHUD showDismissibleErrorWithStatus:NSLocalizedString(@"Disconnect failed", @"Message to show when Publicize disconnect failed")];
Expand Down
10 changes: 5 additions & 5 deletions WordPress/Classes/ViewRelated/Blog/SharingViewController.m
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,7 @@ -(void)presentationControllerDidDismiss:(UIPresentationController *)presentation

- (void)refreshPublicizers
{
SharingService *sharingService = [[SharingService alloc] initWithManagedObjectContext:[self managedObjectContext]];
self.publicizeServices = [sharingService allPublicizeServices];
self.publicizeServices = [PublicizeService allPublicizeServicesInContext:[self managedObjectContext] error:nil];
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth to pass an error pointer here, so that we can log and bail if the method throws?

I mean... if the method throws then the data the table should show won't change and this is a no-op, so there's no much waste in not handling the error.

So I guess it all boils down to whether we want to capture and log it or not. 🤔 I don't have a strong opinion about this. I'd be tempted to say "log just in case; more information is always better" but... are those logs ever looked at? I know they are sometimes for support, but not sure how much more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I guess capturing the error or not depending on what we are going to do with it. If it's just logging, I would probably say it's not that valuable. Especially in this case, the error (which is Core Data failing to fetch the model objects) is not likely to happen.


[self.tableView reloadData];
}
Expand Down Expand Up @@ -324,7 +323,7 @@ -(void)showConnectionError

- (void)syncPublicizeServices
{
SharingService *sharingService = [[SharingService alloc] initWithManagedObjectContext:[self managedObjectContext]];
SharingService *sharingService = [[SharingService alloc] initWithContextManager:[ContextManager sharedInstance]];
__weak __typeof__(self) weakSelf = self;
[sharingService syncPublicizeServicesForBlog:self.blog success:^{
[weakSelf syncConnections];
Expand Down Expand Up @@ -358,11 +357,12 @@ - (void)syncSharingButtonsIfNeeded
{
// Sync sharing buttons if they have never been synced. Otherwise, the
// management vc can worry about fetching the latest sharing buttons.
SharingService *sharingService = [[SharingService alloc] initWithManagedObjectContext:[self managedObjectContext]];
NSArray *buttons = [sharingService allSharingButtonsForBlog:self.blog];
NSArray *buttons = [SharingButton allSharingButtonsForBlog:self.blog inContext:[self managedObjectContext] error:nil];
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth to pass an error pointer here, so that we can log and bail if the method throws?

if ([buttons count] > 0) {
return;
}

SharingService *sharingService = [[SharingService alloc] initWithContextManager:[ContextManager sharedInstance]];
[sharingService syncSharingButtonsForBlog:self.blog success:nil failure:^(NSError *error) {
DDLogError([error description]);
}];
Expand Down
12 changes: 12 additions & 0 deletions WordPress/WordPress.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -1131,6 +1131,10 @@
4A2172FF28F688890006F4F1 /* Blog+Media.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A2172FD28F688890006F4F1 /* Blog+Media.swift */; };
4A266B8F282B05210089CF3D /* JSONObjectTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A266B8E282B05210089CF3D /* JSONObjectTests.swift */; };
4A266B91282B13A70089CF3D /* CoreDataTestCase.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A266B90282B13A70089CF3D /* CoreDataTestCase.swift */; };
4A358DE629B5EB8D00BFCEBE /* PublicizeService+Lookup.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A358DE529B5EB8D00BFCEBE /* PublicizeService+Lookup.swift */; };
4A358DE729B5EB8D00BFCEBE /* PublicizeService+Lookup.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A358DE529B5EB8D00BFCEBE /* PublicizeService+Lookup.swift */; };
4A358DE929B5F14C00BFCEBE /* SharingButton+Lookup.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A358DE829B5F14C00BFCEBE /* SharingButton+Lookup.swift */; };
4A358DEA29B5F14C00BFCEBE /* SharingButton+Lookup.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A358DE829B5F14C00BFCEBE /* SharingButton+Lookup.swift */; };
4A526BDF296BE9A50007B5BA /* CoreDataService.m in Sources */ = {isa = PBXBuildFile; fileRef = 4A526BDD296BE9A50007B5BA /* CoreDataService.m */; };
4A526BE0296BE9A50007B5BA /* CoreDataService.m in Sources */ = {isa = PBXBuildFile; fileRef = 4A526BDD296BE9A50007B5BA /* CoreDataService.m */; };
4A82C43128D321A300486CFF /* Blog+Post.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4A82C43028D321A300486CFF /* Blog+Post.swift */; };
Expand Down Expand Up @@ -6619,6 +6623,8 @@
4A2172FD28F688890006F4F1 /* Blog+Media.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Blog+Media.swift"; sourceTree = "<group>"; };
4A266B8E282B05210089CF3D /* JSONObjectTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = JSONObjectTests.swift; sourceTree = "<group>"; };
4A266B90282B13A70089CF3D /* CoreDataTestCase.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CoreDataTestCase.swift; sourceTree = "<group>"; };
4A358DE529B5EB8D00BFCEBE /* PublicizeService+Lookup.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "PublicizeService+Lookup.swift"; sourceTree = "<group>"; };
4A358DE829B5F14C00BFCEBE /* SharingButton+Lookup.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "SharingButton+Lookup.swift"; sourceTree = "<group>"; };
4A526BDD296BE9A50007B5BA /* CoreDataService.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = CoreDataService.m; sourceTree = "<group>"; };
4A526BDE296BE9A50007B5BA /* CoreDataService.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CoreDataService.h; sourceTree = "<group>"; };
4A82C43028D321A300486CFF /* Blog+Post.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "Blog+Post.swift"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -10414,6 +10420,7 @@
E6374DBD1C444D8B00F24720 /* PublicizeConnection.swift */,
4A1E77C82988997C006281CC /* PublicizeConnection+Creation.swift */,
E6374DBE1C444D8B00F24720 /* PublicizeService.swift */,
4A358DE529B5EB8D00BFCEBE /* PublicizeService+Lookup.swift */,
8BB185D424B66FE600A4CCE8 /* ReaderCard+CoreDataClass.swift */,
8BB185D324B66FE500A4CCE8 /* ReaderCard+CoreDataProperties.swift */,
5D42A3DC175E7452005CFF05 /* ReaderPost.h */,
Expand All @@ -10439,6 +10446,7 @@
E1DB326B1D9ACD4A00C8FEBC /* ReaderTeamTopic.swift */,
E10290731F30615A00DAC588 /* Role.swift */,
E6E27D611C6144DB0063F821 /* SharingButton.swift */,
4A358DE829B5F14C00BFCEBE /* SharingButton+Lookup.swift */,
E6D170361EF9D8D10046D433 /* SiteInfo.swift */,
B06378BE253F639D00FD45D2 /* SiteSuggestion+CoreDataClass.swift */,
B06378BF253F639D00FD45D2 /* SiteSuggestion+CoreDataProperties.swift */,
Expand Down Expand Up @@ -20557,6 +20565,7 @@
F12FA5D92428FA8F0054DA21 /* AuthenticationService.swift in Sources */,
E16A76F11FC4758300A661E3 /* JetpackSiteRef.swift in Sources */,
B5B56D3219AFB68800B4E29B /* WPStyleGuide+Reply.swift in Sources */,
4A358DE629B5EB8D00BFCEBE /* PublicizeService+Lookup.swift in Sources */,
C7234A422832C2BA0045C63F /* QRLoginScanningViewController.swift in Sources */,
801D94EF2919E7D70051993E /* JetpackFullscreenOverlayGeneralViewModel.swift in Sources */,
30EABE0918A5903400B73A9C /* WPBlogTableViewCell.m in Sources */,
Expand Down Expand Up @@ -21413,6 +21422,7 @@
E14DFAFB1E07E7C400494688 /* Data.swift in Sources */,
E14B40FD1C58B806005046F6 /* AccountSettingsViewController.swift in Sources */,
98B11B892216535100B7F2D7 /* StatsChildRowsView.swift in Sources */,
4A358DE929B5F14C00BFCEBE /* SharingButton+Lookup.swift in Sources */,
08CC677F1C49B65A00153AD7 /* Menu.m in Sources */,
BE13B3E71B2B58D800A4211D /* BlogListViewController.m in Sources */,
C768B5B425828A5D00556E75 /* JetpackScanStatusCell.swift in Sources */,
Expand Down Expand Up @@ -23550,6 +23560,7 @@
FABB21C22602FC2C00C8785C /* JetpackRestoreFailedViewController.swift in Sources */,
8067340B27E3A50900ABC95E /* UIViewController+RemoveQuickStart.m in Sources */,
FABB21C32602FC2C00C8785C /* ActionSheetViewController.swift in Sources */,
4A358DE729B5EB8D00BFCEBE /* PublicizeService+Lookup.swift in Sources */,
C737553F27C80DD500C6E9A1 /* String+CondenseWhitespace.swift in Sources */,
FABB21C42602FC2C00C8785C /* PostService+Revisions.swift in Sources */,
FABB21C52602FC2C00C8785C /* SourcePostAttribution.m in Sources */,
Expand Down Expand Up @@ -23662,6 +23673,7 @@
FABB22132602FC2C00C8785C /* BlogSiteVisibilityHelper.m in Sources */,
FABB22142602FC2C00C8785C /* TitleBadgeDisclosureCell.swift in Sources */,
FABB22152602FC2C00C8785C /* FormattableContent.swift in Sources */,
4A358DEA29B5F14C00BFCEBE /* SharingButton+Lookup.swift in Sources */,
FABB22162602FC2C00C8785C /* WebAddressWizardContent.swift in Sources */,
C7AFF87D283D5CF4000E01DF /* QRLoginVerifyCoordinator.swift in Sources */,
FABB22172602FC2C00C8785C /* VisitsSummaryStatsRecordValue+CoreDataProperties.swift in Sources */,
Expand Down