Reader: Local Default Topics #23788

kean · 2024-11-08T23:25:02Z

Background

When you open the Reader, it fetches /read/menu that contains the following fields:

  "default": {
    "discover": {
      "URL": "https://public-api.wordpress.com/rest/v1.2/read/sites/53424024/posts",
      "title": "Discover"
    },
    "following": {
      "URL": "https://public-api.wordpress.com/rest/v1.2/read/following",
      "title": "Followed Sites"
    },
    "liked": {
      "URL": "https://public-api.wordpress.com/rest/v1.2/read/liked",
      "title": "My Likes"
    },
  },

For each of these menu items, the app creates a ReaderDefaultTopic instance of Core Data. The main role of these topics is to be used in predicates for caching posts: NSPredicate(format: "topic = %@).

There are two main issue with this approach:

The app can't open any of these menus until it successfully fetches /read/menu
The app hardcodes this path (read/sites/53424024/posts) for Discover. If the path ever changes on the backend, Discover will completely stop working in the app.

With Discover, the app uses neither the URL nor the title field from the response, so it's a pretty easy update to no longer rely on this value. This is not the case for "Followed Sites" and "My Likes" where the app actually uses the path in multiple ways, including for fetching the respective feeds.

Changes

Create ReaderDefaultTopic for "Discover" locally
Remove currentTopic (no longer used)
Remove ReaderTopicType (not needed anymore)

To test:

Set a network breakpoint on /read/menu
Open the app for the first time
Open Reader
Tap Discover
Verify that the screen opens and loads the channels ("Recommended", "Latest", etc)

Future Changes

Update "Recent" and "Likes" to also stop relying on the /read/menu. This is a larger change and I decided not to make it in the scope of this PR.

Regression Notes

Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)

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.

Testing checklist:

dangermattic · 2024-11-08T23:25:43Z

	1 Warning
⚠️	This PR is assigned to the milestone 25.5. The due date for this milestone has already passed. Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

wpmobilebot · 2024-11-08T23:35:05Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr23788-6f712d3
	Version	25.4.2
	Bundle ID	org.wordpress.alpha
	Commit	`6f712d3`
	App Center Build	WPiOS - One-Offs #11059

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2024-11-08T23:35:47Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr23788-6f712d3
	Version	25.4.2
	Bundle ID	com.jetpack.alpha
	Commit	`6f712d3`
	App Center Build	jetpack-installable-builds #10099

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

kean · 2024-11-08T23:56:08Z

WordPress/Classes/Models/ReaderDefaultTopic.swift

+        let topic = ReaderDefaultTopic(entity: ReaderDefaultTopic.entity(), insertInto: context)
+        topic.type = TopicType
+        topic.path = path.rawValue
+        topic.title = ""


This is a required field but it's never used only by other topic types.

kean · 2024-11-08T23:56:24Z

WordPress/Classes/Services/ReaderTopicService.m

@@ -10,9 +10,6 @@
 @import WordPressKit;
 @import WordPressShared;

-NSString * const ReaderTopicFreshlyPressedPathCommponent = @"freshly-pressed";


No longer used.

kean · 2024-11-08T23:57:51Z

WordPress/Classes/ViewRelated/Reader/Controllers/ReaderPostActions/ReaderPostMenu.swift

+        if ReaderHelpers.isTopicTag(topic) {
+            return true
+        }
+        return (topic is ReaderDefaultTopic) && !ReaderHelpers.topicIsLiked(topic)


There has to be a better way to do it, but I'm not refactoring it in the scope of this PR.

kean · 2024-11-08T23:58:17Z

WordPress/Classes/ViewRelated/Reader/Controllers/ReaderStreamViewController.swift

@@ -214,11 +214,6 @@ import AutomatticTracks
    /// - Returns: An instance of the controller
    ///
    @objc class func controllerWithTopic(_ topic: ReaderAbstractTopic) -> ReaderStreamViewController {
-        // if a default discover topic is provided, treat it as a site to retrieve the header
-        if ReaderHelpers.topicIsDiscover(topic) {


I don't think we need this anymore. The worst case scenario, it'll just open as a regular site feed.

crazytonyli · 2024-11-13T20:19:39Z

Set a network breakpoint on /read/menu

Is this a breakpoint in Xcode, or do you mean intercepting the HTTP request from HTTP proxy apps?

crazytonyli · 2024-11-13T20:25:57Z

WordPress/Classes/Models/ReaderDefaultTopic.swift

+        case recommended = "/jpios/discover/recommended"
+        case firstPosts = "/jpios/discover/first-posts"
+        case latest = "/jpios/discover/latest"
+    }


Does it mean there should be three unique "default topics" (a.k.a "Discover" topics?) in Core Data? If that's the case, should we delete -[ReaderTopicService defaultTopicForRemoteTopic:inContext:] to avoid arbitrary ReaderDefaultTopic being created?

There are two that are still remaining: one for "Recent" and one for "Likes", so I can't remove them just yet. The "Discover" topic will also be in the database for now, but the app never displays them anywhere, so it's going to be fine. My plan is to ultimately remove ReaderDefaultTopic entirely. There are no good reasons to store them in Core Data.

Btw, this PR also fixes the following issue:

Steps

Open Discover

Select "Latest Posts" and wait until they load

Terminate the app

Reopen Discover (it shows "Recommended" again)

Expected result:

It loads new posts

Actual result:

It shows posts cached for "Latest Posts", then reloads

crazytonyli · 2024-11-15T11:12:46Z

WordPress/Classes/Models/ReaderDefaultTopic.swift

+        topic.type = TopicType
+        topic.path = path.rawValue
+        topic.title = ""
+        try context.save()


We should use performAndSave instead of saving the context directly. Or, at bare minimal, use ContextManager.save.

Also, we don't update and save the main context.

#23788 (comment)

kean · 2024-11-18T16:41:08Z

I don't feel comfortable merging this late in the game, so I'm moving it to draft.

wpmobilebot · 2024-12-05T12:06:46Z

Version 25.5 has now entered code-freeze, so the milestone of this PR has been updated to 25.6.

wpmobilebot · 2024-12-16T15:54:03Z

Version 25.6 has now entered code-freeze, so the milestone of this PR has been updated to 25.7.

kean force-pushed the fix/default-topics branch from 114123d to 2c1d3ac Compare November 8, 2024 23:55

kean commented Nov 8, 2024

View reviewed changes

kean requested a review from crazytonyli November 8, 2024 23:58

kean added the Reader label Nov 8, 2024

kean added this to the 25.5 milestone Nov 8, 2024

kean marked this pull request as ready for review November 9, 2024 00:06

crazytonyli reviewed Nov 13, 2024

View reviewed changes

crazytonyli previously approved these changes Nov 15, 2024

View reviewed changes

crazytonyli reviewed Nov 15, 2024

View reviewed changes

kean force-pushed the fix/default-topics branch from 78e1ff7 to 45c09af Compare November 15, 2024 13:02

kean mentioned this pull request Nov 15, 2024

"Your tags" shows incorrect dates and doesn't refresh #23813

Closed

kean added 8 commits November 15, 2024 08:29

Show Discover using the topic created locally

c7cf4d6

Remove currentTopic (no longer used)

9a940e7

Remove topic isFreshlyPRessed

7db2e4a

Remove remainig currentTopic usage

202004b

Remove topicIsDiscover

534a4e9

Remove remaining unused code

c84148d

Remove ReaderContentType

f4e561f

Remove redundant tests

6f712d3

kean force-pushed the fix/default-topics branch from 45c09af to 6f712d3 Compare November 15, 2024 13:29

kean marked this pull request as draft November 18, 2024 16:41

wpmobilebot modified the milestones: 25.5, 25.6 Dec 5, 2024

kean mentioned this pull request Dec 11, 2024

Update Blog.dotComID value if needed before saving #23884

Merged

14 tasks

wpmobilebot modified the milestones: 25.6, 25.7 Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reader: Local Default Topics #23788

Reader: Local Default Topics #23788

kean commented Nov 8, 2024 •

edited

Loading

dangermattic commented Nov 8, 2024 •

edited

Loading

wpmobilebot commented Nov 8, 2024 •

edited

Loading

wpmobilebot commented Nov 8, 2024 •

edited

Loading

kean Nov 8, 2024

kean Nov 8, 2024

kean Nov 8, 2024

kean Nov 8, 2024

crazytonyli commented Nov 13, 2024

crazytonyli Nov 13, 2024

kean Nov 14, 2024

kean Nov 14, 2024

crazytonyli Nov 15, 2024

kean commented Nov 18, 2024

wpmobilebot commented Dec 5, 2024

wpmobilebot commented Dec 16, 2024

Reader: Local Default Topics #23788

Are you sure you want to change the base?

Reader: Local Default Topics #23788

Conversation

kean commented Nov 8, 2024 • edited Loading

Background

Changes

To test:

Future Changes

Regression Notes

dangermattic commented Nov 8, 2024 • edited Loading

wpmobilebot commented Nov 8, 2024 • edited Loading

wpmobilebot commented Nov 8, 2024 • 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

crazytonyli commented Nov 13, 2024

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

kean commented Nov 18, 2024

wpmobilebot commented Dec 5, 2024

wpmobilebot commented Dec 16, 2024

kean commented Nov 8, 2024 •

edited

Loading

dangermattic commented Nov 8, 2024 •

edited

Loading

wpmobilebot commented Nov 8, 2024 •

edited

Loading

wpmobilebot commented Nov 8, 2024 •

edited

Loading