Blog service lookup blog #19366

crazytonyli · 2022-09-28T07:57:03Z

This PR moves some Blog history query functions from BlogService to a Blog extension.

Again, the new code (the extension) in this PR is translated from Objective-C code. I don't see big risk in it. Most changes in this PR are switching to call the new API. An outlier is e0ebe8f, I noticed the blog argument is not used anywhere, so I removed it, along with a Core Data query.

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)
None.
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.

crazytonyli · 2022-09-28T07:59:26Z

WordPress/Classes/Models/Blog+History.swift

+        blog(with: NSPredicate(format: "visible = YES"), in: context)
+    }
+
+    private static func blog(with predicate: NSPredicate, in context: NSManagedObjectContext) -> Blog? {


This function is translated from BlogService.blogsWithPredicate

wpmobilebot · 2022-09-28T08:22:35Z

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 pr19366-274e85d on your iPhone

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

wpmobilebot · 2022-09-28T08:25:18Z

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 pr19366-274e85d on your iPhone

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

mokagio · 2022-09-28T10:07:05Z

WordPress/Classes/Models/Blog+History.swift

+    private static func blog(with predicate: NSPredicate, in context: NSManagedObjectContext) -> Blog? {
+        let request = NSFetchRequest<Blog>(entityName: NSStringFromClass(Blog.self))
+        request.includesSubentities = false
+        request.predicate = predicate
+        request.fetchLimit = 1
+        request.sortDescriptors = [NSSortDescriptor(key: "settings.name", ascending: true)]
+
+        do {
+            return try context.fetch(request).first
+        } catch {
+            DDLogError("Couldn't fetch blogs with predicate \(predicate): \(error)")
+            return nil
+        }
+    }


Maybe in a future iteration it would be cool to throw or return Result<Blog, Error>.

Of course, that's out of scope now and also would require making a choice over which approach to use. Just thinking out loud.

mokagio · 2022-09-28T10:08:23Z

WordPress/Classes/Services/BlogService.h

-/**
- Returns the blog currently flagged as the one last used, or the primary blog,
- or the first blog in an alphanumerically sorted list that supports the given
- feature, whichever is found first.
- */
- (nullable Blog *)lastUsedOrFirstBlogThatSupports:(BlogFeature)feature;


This method hasn't been converted. But the code builds without it, so it was clearly unused 😄 👍

mokagio · 2022-09-28T10:23:33Z

WordPress/WordPressTest/ReaderReblogActionTests.swift

+private class MockViewController: UIViewController {
+
+    var presentExpectation: XCTestExpectation?
+
+    override func present(_ viewControllerToPresent: UIViewController, animated flag: Bool, completion: (() -> Void)? = nil) {
+        presentExpectation?.fulfill()
+        super.present(viewControllerToPresent, animated: flag, completion: completion)
+    }
+
+}


I can see this being used in other tests, too. (I'm not actually sure there are other use cases, but I assume this mustn't be the only test calling present....

How would you feel about making it "stricter"? I.e. require the expectation to be set, as that's the whole point why one would use this UIViewController subclass.

It could be done by throwing if presentExpectation is nil. Or, we could provide a new, dedicated initializer, although, I always forget whether you can make the other ones private when working with UIViewController subclasses.

Another idea could be to move the whole expectation logic within the object, and make it expose a method to verify the presentation occurred. That would also be truer to the "mock" name, defined in the xunit patterns as a test double that verifies "indirect outputs of SUT" by verifying "correctness against expectations". (See my thoughts on test double naming here).

let presenterMock = PresenterMockViewController() ... presenter.presentReblog(blogService: blogService!, readerPost: readerPost!, origin: presenterMock) ... presenterMock.expectPresentationToOccurr() // In PresenterMockViewController func expectPresentationToOccurr(file: ... = #file, line: ... = #line) { guard let presentExpectation = self.presentExpectation else { /* fail */ } waitForExpectations([presentExpectation], timeout: ...) { error in guard let error = error else { return } XCTFail("present was not called", file: file, line: line) } }

I agree with your thinking here—it should do what a "mock" type does if it's called a "mock". I tried to come up with some other ways to do this, like using OCMock(I couldn't get it to work in a Swift code), or capture the presented view controller and make assertions on it in the test function. But eventually I decided to go with what I had here, simply because that's the pattern used in the test class here (also a few other test classes that I ran into).

I do worry about having mixed patterns, especially if they are in the same file. Others(or myself in 6 months) may not know which pattern to follow when they make changes to this file. I'd prefer that we spend dedicate time to investigate alternative options than the "custom mock type with XCTExpectation" pattern used here, and update the whole test with the new pattern. What do you think?

I'd prefer that we spend dedicate time to investigate alternative options than the "custom mock type with XCTExpectation" pattern used here, and update the whole test [unit tests suite?] with the new pattern.

That would be very appropriate. It would also gives us a chance to see how the verify the different use cases.

Others(or myself in 6 months)

So true! I don't trust my future self's memory either 😄

Tracked in #19373

mokagio · 2022-09-28T10:26:36Z

WordPress/Classes/Models/Blog+History.swift

+    /// Returns the blog currently flagged as the one last used, or the primary blog,
+    /// or the first blog in an alphanumerically sorted list, whichever is found first.
+    @objc(lastUsedOrFirstBlogInContext:)
+    static func lastUsedOrFirstBlog(in context: NSManagedObjectContext) -> Blog? {


I didn't think this while reading the method name, but, after seeing this in action, I wonder if we can do without the "Blog".

Without it, the usage would be:

Blog.lastUsedOrFirst(in: context)

That reads fine to me, and it seems pretty clear that the return value would be a Blog.

What do you think? I'm not too bothered by it, just an idea. But, if you like the suggestion, I think we should document it somewhere as the desired style for these ActiveRecord-like methods.

Good point. The "Blog" does seem redundant. 👍

mokagio

Re-iterating my approval.

There were a couple of build steps that failed but I don't think they were related to these changes. I restarted them.

crazytonyli added 7 commits September 28, 2022 18:08

Remove an unused function BlogService.lastUsedOrFirstBlogThatSupports

25d8a4a

Make BlogService.firstBlog private API

d7c273f

Move history blog query functions out of BlogService

4a4d114

Replace BlogService.lastUsedOrFirstBlog with the new function

ba698a8

Replace BlogService.lastUsedBlog with the new API

dd51e2a

Replace BlogService.primaryBlog with calling WPAccount API directly

d715dc1

Remove an unused blog argument from ReaderReblogPresenter

e0ebe8f

crazytonyli added the Core Data Issues related to Core Data label Sep 28, 2022

crazytonyli added this to the 20.9 milestone Sep 28, 2022

crazytonyli requested a review from mokagio September 28, 2022 07:57

crazytonyli self-assigned this Sep 28, 2022

Remove an unnecessary semicolon

be70b56

crazytonyli commented Sep 28, 2022

View reviewed changes

mokagio approved these changes Sep 28, 2022

View reviewed changes

Remove 'Blog' from function name

274e85d

mokagio mentioned this pull request Sep 29, 2022

Establish a unique pattern for testing UIViewController presentation #19373

Closed

2 tasks

mokagio approved these changes Sep 29, 2022

View reviewed changes

crazytonyli merged commit 7624caf into trunk Sep 29, 2022

crazytonyli deleted the blog-service-lookup-blog branch September 29, 2022 04:48

staskus mentioned this pull request Oct 7, 2022

Fix crash on tapping Account Settings menu for accounts with no sites #19418

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blog service lookup blog #19366

Blog service lookup blog #19366

crazytonyli commented Sep 28, 2022

crazytonyli Sep 28, 2022

wpmobilebot commented Sep 28, 2022 •

edited

Loading

wpmobilebot commented Sep 28, 2022 •

edited

Loading

mokagio Sep 28, 2022

mokagio Sep 28, 2022

mokagio Sep 28, 2022

crazytonyli Sep 28, 2022

mokagio Sep 29, 2022

mokagio Sep 29, 2022

mokagio Sep 28, 2022

crazytonyli Sep 28, 2022

mokagio left a comment

Blog service lookup blog #19366

Blog service lookup blog #19366

Conversation

crazytonyli commented Sep 28, 2022

Regression Notes

Choose a reason for hiding this comment

wpmobilebot commented Sep 28, 2022 • edited Loading

wpmobilebot commented Sep 28, 2022 • edited Loading

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

mokagio left a comment

Choose a reason for hiding this comment

wpmobilebot commented Sep 28, 2022 •

edited

Loading

wpmobilebot commented Sep 28, 2022 •

edited

Loading