Do not show disconnected Jetpack sites

[crazytonyli]

Description

The app displays sites inaccessible Jetpack self-hosted sites(for example, deleted Jurassic Ninja sites). Similar to deleted WP.com sites, when switching to those sites, the app displays almost empty content. This PR removes those sites from API responses.

WordPressKit.zip

ℹ Please replace the above with a link to the issue this pull request addresses, as well as a summary of the implementation details.

Testing Details

ℹ Please replace this with a clear and concise description of the steps required to validate this pull request.

---

• Please check here if your pull request includes additional test coverage.
• I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

[dangermattic]

	1 Warning
⚠️	Package.swift was changed without updating its corresponding Package.resolved. Please resolve the Swift packages in Xcode.

Generated by 🚫 Danger

[pmusolino]

LGTM, but I'm waiting for the final PR in the WordPress-iOS repo before approving it!

[kean]

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?

[crazytonyli]

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.

[kean]

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?

[crazytonyli]

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.

[crazytonyli]

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.

[crazytonyli]

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.

[kean]

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.

[crazytonyli]

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.

[kean]

Please make sure to rebase and re-create the binary if needed.