Skip to content
This repository was archived by the owner on Sep 15, 2025. It is now read-only.

Do not show disconnected Jetpack sites #835

Merged
merged 4 commits into from
Apr 9, 2025
+17 −4
Merged

Do not show disconnected Jetpack sites #835

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3258ec3
Exclude unactive Jetpack sites
crazytonyli Mar 31, 2025
aa4ea90
Update xcframework url
crazytonyli Mar 31, 2025
777c651
Update docs to be consistent with the HTTP API doc
crazytonyli Apr 1, 2025
b6ada40
Update xcframework url
crazytonyli Apr 9, 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
4 changes: 2 additions & 2 deletions Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ let package = Package(
targets: [
.binaryTarget(
name: "WordPressKit",
url: "https://github.com/user-attachments/files/19339848/WordPressKit.zip",
checksum: "5bf1ff361992dccf44dfe41b0a442ee4c3c13dc0a1ce9b647a8ed1b976b5c3fc"
url: "https://github.com/user-attachments/files/19533607/WordPressKit.zip",
checksum: "d0c8d8994a64ff7ac0cf0fb1cc23c4fa419df011406c327f716b40af7dd8faa0"
),
]
)
6 changes: 5 additions & 1 deletion Sources/WordPressKit/Models/RemoteBlog.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,12 @@ import Foundation
/// Features available for the current blog's plan.
public var planActiveFeatures = [String]()

/// Indicates whether it's a jetpack site, or not.
/// Indicates whether the jetpack connection is active.
Copy link
Contributor

@kean kean Mar 31, 2025

Choose a reason for hiding this comment

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

Shouldn't this comment apply to the jetpackConnection property?

/// Indicates whether the jetpack connection is active.
public var jetpack: Bool = false

Let's follow the Swift naming conversion and name these something like:

var isJetpackConnectionActive: Bool { get }
var isJetpackConnected: Bool { get } 
Indicates whether it's a Jetpack connected site.

Does this mean that the site is a non-WP.com site?

Copy link
Contributor Author

@crazytonyli crazytonyli Apr 1, 2025

Choose a reason for hiding this comment

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

I have updated the API doc to match the properties on the HTTP API 777c651.

Does this mean that the site is a non-WP.com site?

I believe it's possible that atomic sites have jetpack_connection == true & jetpack == false. Just by jetpack_connection property alone, we can't tell if a site is a WP.com site.

public var jetpack: Bool = false

/// Indicates whether it's a Jetpack connected site.
public var jetpackConnection: Bool = false

/// Boolean indicating whether the current user has Admin privileges, or not.
public var isAdmin: Bool = false

Expand Down Expand Up @@ -70,6 +73,7 @@ import Foundation
self.url = json.string(forKey: "URL") ?? ""
self.xmlrpc = json.string(forKeyPath: "meta.links.xmlrpc")
self.jetpack = json.number(forKey: "jetpack")?.boolValue ?? false
self.jetpackConnection = json.number(forKey: "jetpack_connection")?.boolValue ?? false
self.icon = json.string(forKeyPath: "icon.img")
self.capabilities = json.object(forKey: "capabilities") as? [String: Bool] ?? [:]
self.isAdmin = json.number(forKeyPath: "capabilities.manage_options")?.boolValue ?? false
Expand Down
11 changes: 10 additions & 1 deletion Sources/WordPressKit/Services/AccountServiceRemoteREST.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,16 @@ - (NSArray *)remoteBlogsFromJSONArray:(NSArray *)jsonBlogs
// Exclude deleted sites from query result, since the app does not handle deleted sites properly.
// I tried to use query arguments `site_visibility=visible` and `site_activity=active`, but neither excludes
// deleted sites.
return !blog.isDeleted;
if (blog.isDeleted) {
return false;
}

// Exclude sites that are connected via Jetpack, but without an active Jetpack connection.
if (blog.jetpackConnection && !blog.jetpack) {
Copy link
Contributor

@kean kean Mar 31, 2025

Choose a reason for hiding this comment

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

What does WP.com do in the same scenario? Does it also remove the site from your account? What does "active Jetpack" mean? Can a site temporarily become inactive?

Copy link
Contributor Author

@crazytonyli crazytonyli Apr 1, 2025

Choose a reason for hiding this comment

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

jetpack_connection = true means the site was connected to WP.com via jetpack.

jetpack = true means WP.com can still communicate with the site via Jetpack (a.k.a, jetpack connection is active).

You typically get jetpack_connection = true && jetpack = false when the site was connected to WP.com a while ago, but its domain expired.

Copy link
Contributor Author

@crazytonyli crazytonyli Apr 1, 2025

Choose a reason for hiding this comment

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

WP.com does not show sites with jetpack_connection = true && jetpack = false, hence the fix.

There is more complicated logic on WP.com (see here) to decide whether to show a site or not. I didn't try to understand all of them. But this PR fixes the particular case where the site domain has expired.

Copy link
Contributor Author

@crazytonyli crazytonyli Apr 1, 2025

Choose a reason for hiding this comment

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

Can a site temporarily become inactive?

I think that's entirely possible, like when the site is down, or for whatever reason that WP.com decides that it can't communicate with the site.

Copy link
Contributor

@kean kean Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not entirely sure how the logic on WP.com translate to this:

if ( hasJetpackActivePlugins( site ) && ! isJetpackSiteOrJetpackCloud( site ) ) { return false; }

Is there a good way to test it?

jetpack = true means WP.com can still communicate with the site via Jetpack (a.k.a, jetpack connection is active).

There is also a chance the UX on WP.com isn't optimal. WP.com can't not if the site was actually deleted or if it's a temporarily issue, can it? As a user, I'd probably expect it to show some kind of a message like "site is not responding" with a way to remove it from my WP.com account.

I'd also suggest moving filtering (the entire logic inside wpkit_filter) to the app. It's not WordPressKit job to decide what app displays.

Copy link
Contributor Author

@crazytonyli crazytonyli Apr 7, 2025

Choose a reason for hiding this comment

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

I didn't follow the exact filtering logic on calypso. This is the code I referenced when looking into the API responses.

// Sites connected through A4A plugin are listed on wordpress.com/sites even when Jetpack is deactivated.
export const isDisconnectedJetpackAndNotAtomic = ( site: SiteExcerptNetworkData ) => {
	return ! site?.is_wpcom_atomic && site?.jetpack_connection && ! site?.jetpack;
};

I'd also suggest moving filtering (the entire logic inside wpkit_filter) to the app. It's not WordPressKit job to decide what app displays.

Agreed. I'll do that.

return false;
}

return true;
}];
}

Expand Down