Refactor `SharingService` to use `CoreDataStack` #20265

crazytonyli · 2023-03-07T01:16:58Z

Use CoreDataStack instead of NSManagedObjectContext in SharingService. See the "Issue" and "Proposal" sections of #19893 for rationale behind this change.

Like other similar PRs, I recommend reviewing this one commit by commit too.

Test Instructions

Only the "Sharing" screen (under the My Site tab) is affected by this change. Verifying the features in it still works should be enough, i.e. connect/disconnect to a SNS, show/hide sharing buttons.

Regression Notes

Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
What are describe in the "Test Instructions" section above.
What automated tests I added (or what prevented me from doing so)
None.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

wpmobilebot · 2023-03-07T01:41:50Z

You can test the changes in WordPress from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr20265-27c44c7 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

wpmobilebot · 2023-03-07T01:43:19Z

You can test the changes in Jetpack from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr20265-27c44c7 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

mokagio

Left a few nitpicks. That's all.

mokagio · 2023-03-07T01:28:17Z

WordPress/Classes/Models/PublicizeService+Lookup.swift

+    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


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.

mokagio · 2023-03-07T01:29:35Z

WordPress/Classes/Models/PublicizeService+Lookup.swift

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


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 😄

Yeah, a little compromise for Objective-C code.

mokagio · 2023-03-07T01:34:11Z

WordPress/Classes/Models/SharingButton+Lookup.swift

+    ///
+    /// - Parameters
+    ///     - blog: A `Blog` object


Nitpick: This documentation uses a different format that the one in the file before. Which one should we use?

Regardless, how would you feel about removing it? Unlike the one above, that explains the relationship between name and serviceID, this one doesn't explain much.

Suggested change

///

/// - Parameters

/// - blog: A `Blog` object

Updated in e95fd66

mokagio · 2023-03-07T01:34:23Z

WordPress/Classes/Models/PublicizeService+Lookup.swift

+
+

Suggested change

mokagio · 2023-03-07T01:36:46Z

WordPress/Classes/Models/SharingButton+Lookup.swift

+    /// 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
+    }


Wow... I was not expecting a button to be stored in the database 🤔

I guess there are a few properties worth storing, but still...

WordPress-iOS/WordPress/Classes/Models/SharingButton.swift

Lines 12 to 18 in f5c8424

@NSManaged open var buttonID: String

@NSManaged open var name: String

@NSManaged open var shortname: String

@NSManaged open var custom: Bool

@NSManaged open var enabled: Bool

@NSManaged open var visibility: String?

@NSManaged open var order: NSNumber

Yeah, it's pretty common in the app actually.

There are roughly three kinds of database records (a.k.a the managed objects) in the app.

The database records are actually useful and must not be discarded. Like Post for local drafts.

The database records are just a cache, which is used to display UI initially but refreshed later. Like, media objects in the WordPress media library.

The database records are saved for the convenience of displaying them using a NSFetchedResultController. These records can be deleted immediately after the user exists the corresponding screen, without affecting the app. Like, sharing buttons in this PR, and "managed people" in "People" screens, some posts in the Reader tab, etc.

My feeling is, the third kind is the majority.

The database records are saved for the convenience of displaying them using a NSFetchedResultController. These records can be deleted immediately after the user exists the corresponding screen, without affecting the app.

Is the convenience of displaying them in this way worth the overhead?

That's not an easy question to answer, I think 🥲 .

Here is another way to look at it. It's a super common pattern in the app to use a NSFetchedResultController to display a list of things. It's so common that we have a WPTableViewHandler for this exact purpose.

Now, here comes my assumption. When we start building a new feature that displays a list of stuff, I think the following is probably a part of the mindset. Naturally we would go with the existing patterns, the most common one, which is using a NSFetchedResultController. Then we'll see what it requires. It requires a managed object? Okay, I'll add a new model to the Core Data model file then. That's probably how we end up with so many the third kind of model types I mentioned above.

And a question in my head is, when it comes to implementing screens like the "People" or the "Menus" screen (which both uses the third kind of model types I mentioned above—ManagedPerson and Menu are not used anywhere else in the app and the records stored in the database aren't used), do we need to create a Core Data model for it? Obviously, we surely can build them without using Core Data. We just don't have alternative to consider (maybe there is that I am not aware of).

Thank you, this is super insightful. Sounds like a matter of path dependance and inertia. @alpavanoglu and I talked briefly privately about something similar in regards to adopting SwiftUI. There are already well established patterns that serve us well enough, sticking with them is often the path of least resistance.

mokagio · 2023-03-07T01:44:02Z

WordPress/Classes/ViewRelated/Blog/SharingViewController.m

-    SharingService *sharingService = [[SharingService alloc] initWithManagedObjectContext:[self managedObjectContext]];
-    self.publicizeServices = [sharingService allPublicizeServices];
+    self.publicizeServices = [PublicizeService allPublicizeServicesInContext:[self managedObjectContext] error:nil];


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.

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.

mokagio · 2023-03-07T01:47:10Z

WordPress/Classes/Services/SharingService.swift

+                if value.oldValue == shared {
+                    success?()
+                    return
+                }


Have you considered using guard to ensure the compiler reminds us to early return?

Suggested change

if value.oldValue == shared {

success?()

return

}

guard value.oldValue != shared else {

success?()

return

}

I find the if statement reads more naturally: "if the old value is the same as the updated value, then we should call the success block and exist early". Whereas the double negative in the guard statement feels less nature: "make sure the old value is not the same as the updated value, otherwise we should call the success block and exist early".

mokagio · 2023-03-07T01:48:50Z

WordPress/Classes/Services/SharingService.swift

+                        WPAppAnalytics.track(.sharingPublicizeConnectionAvailableToAllChanged, withProperties: properties, withBlogID: value.siteID)
+
+                        self.coreDataStack.performAndSave({ context in
+                            _ = try self.createOrReplacePublicizeConnectionForBlogWithObjectID(blogObjectID, remoteConnection: remoteConnection, in: context)


Would it be useful to make the method @discaradableResult? Or is the fact that we are calling it and ignoring it's result a warning bell?

Update: There is code below that calls the method without an assignment, so I guess this was just something lost in translation here.

Suggested change

_ = try self.createOrReplacePublicizeConnectionForBlogWithObjectID(blogObjectID, remoteConnection: remoteConnection, in: context)

try self.createOrReplacePublicizeConnectionForBlogWithObjectID(blogObjectID, remoteConnection: remoteConnection, in: context)

Updated in eaea3de

mokagio · 2023-03-07T01:52:20Z

WordPress/Classes/Services/SharingService.swift

+                            if errorCode == self.SharingAPIErrorNotFound {
+                                // This is a special situation. If the call to disconnect the service returns not_found then the service
+                                // has probably already been disconnected and the call was made with stale data.
+                                // Assume this is the case and treat this error as a successful disconnect.


Wonder how often this happens 🤔

mokagio · 2023-03-07T01:52:49Z

WordPress/Classes/Services/SharingService.swift

-            let blog = try managedObjectContext.existingObject(with: blogObjectID) as! Blog
-            let pubConn = PublicizeConnection.createOrReplace(from: remoteConnection, in: self.managedObjectContext)
-            pubConn.blog = blog
+    fileprivate func createOrReplacePublicizeConnectionForBlogWithObjectID(


Could this be private?

Suggested change

fileprivate func createOrReplacePublicizeConnectionForBlogWithObjectID(

private func createOrReplacePublicizeConnectionForBlogWithObjectID(

Updated in 27c44c7

peril-wordpress-mobile · 2023-03-07T09:04:57Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

crazytonyli added 8 commits March 7, 2023 13:54

Refactor merging publicize services in SharingService

7afedb0

Move lookup functions out of SharingService

bf99efc

Refactor create and update publicize connection methods

27db21a

Move SharingButton lookup functions out of SharingService

89c98fe

Refactor the deleting publicize connection function

102e386

Re-indent SharingService.updateSharedForBlog

df02800

Refactor SharingService.updateSharedForblog

651530c

Update SharingService initialiser

2de4175

crazytonyli added the Core Data Issues related to Core Data label Mar 7, 2023

crazytonyli added this to the 22.0 milestone Mar 7, 2023

crazytonyli requested a review from mokagio March 7, 2023 01:16

crazytonyli self-assigned this Mar 7, 2023

Update release notes

460d0cb

mokagio approved these changes Mar 7, 2023

View reviewed changes

crazytonyli added 3 commits March 7, 2023 22:02

Update API docs

e95fd66

Un-discard the return value

eaea3de

Change some functions' scope from fileprivate to private

27c44c7

crazytonyli enabled auto-merge March 7, 2023 09:05

crazytonyli merged commit 6b26b0b into trunk Mar 7, 2023

crazytonyli deleted the remove-context-from-sharing-service branch March 7, 2023 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `SharingService` to use `CoreDataStack` #20265

Refactor `SharingService` to use `CoreDataStack` #20265

crazytonyli commented Mar 7, 2023 •

edited by mokagio

Loading

wpmobilebot commented Mar 7, 2023 •

edited

Loading

wpmobilebot commented Mar 7, 2023 •

edited

Loading

mokagio left a comment

mokagio Mar 7, 2023

mokagio Mar 7, 2023

crazytonyli Mar 7, 2023

mokagio Mar 7, 2023

crazytonyli Mar 7, 2023

mokagio Mar 7, 2023

mokagio Mar 7, 2023

crazytonyli Mar 7, 2023

mokagio Mar 8, 2023

crazytonyli Mar 8, 2023

mokagio Mar 9, 2023

mokagio Mar 7, 2023

crazytonyli Mar 7, 2023

mokagio Mar 7, 2023

crazytonyli Mar 7, 2023

mokagio Mar 7, 2023

mokagio Mar 7, 2023

crazytonyli Mar 7, 2023

mokagio Mar 7, 2023

mokagio Mar 7, 2023

crazytonyli Mar 7, 2023

peril-wordpress-mobile bot commented Mar 7, 2023

	@NSManaged open var buttonID: String
	@NSManaged open var name: String
	@NSManaged open var shortname: String
	@NSManaged open var custom: Bool
	@NSManaged open var enabled: Bool
	@NSManaged open var visibility: String?
	@NSManaged open var order: NSNumber

	_ = try self.createOrReplacePublicizeConnectionForBlogWithObjectID(blogObjectID, remoteConnection: remoteConnection, in: context)
	try self.createOrReplacePublicizeConnectionForBlogWithObjectID(blogObjectID, remoteConnection: remoteConnection, in: context)

	fileprivate func createOrReplacePublicizeConnectionForBlogWithObjectID(
	private func createOrReplacePublicizeConnectionForBlogWithObjectID(

Refactor SharingService to use CoreDataStack #20265

Refactor SharingService to use CoreDataStack #20265

Conversation

crazytonyli commented Mar 7, 2023 • edited by mokagio Loading

Test Instructions

Regression Notes

wpmobilebot commented Mar 7, 2023 • edited Loading

wpmobilebot commented Mar 7, 2023 • edited Loading

mokagio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peril-wordpress-mobile bot commented Mar 7, 2023

Refactor `SharingService` to use `CoreDataStack` #20265

Refactor `SharingService` to use `CoreDataStack` #20265

crazytonyli commented Mar 7, 2023 •

edited by mokagio

Loading

wpmobilebot commented Mar 7, 2023 •

edited

Loading

wpmobilebot commented Mar 7, 2023 •

edited

Loading