Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide partner themes from theme browser #22812

Merged
merged 11 commits into from
Mar 13, 2024
Merged

Hide partner themes from theme browser #22812

merged 11 commits into from
Mar 13, 2024

Conversation

guarani
Copy link
Contributor

@guarani guarani commented Mar 12, 2024

The Jetpack app should hide all partner themes because they require a separate purchase. Paid non-partner themes will still be shown because the app shows a helpful message when users attempt to active them: "Please upgrade your plan to access this theme".

Important

AT sites aren't showing paid themes due to a separate issue, see #22812 (comment))

When a user tried to activate a partner theme, the unhelpful message, "The specified theme could was not found" was shown. Instead of updating the message, this PR matches Android's behavior in wordpress-mobile/WordPress-Android#19497 by hiding partner themes.

Related: wordpress-mobile/WordPressKit-iOS#750

Fixes #21932

To test

Partner themes are hidden

  1. On WP.com, locate a Partner theme at https://wordpress.com/themes/partner/{your site}.wordpress.com
  2. In the JP app, open My Site > More > Themes
  3. Verify that the partner theme is not there

Free themes are visible

  1. On WP.com, locate a Free theme at https://wordpress.com/themes/free/{your site}.wordpress.com
  2. In the JP app, open My Site > More > Themes
  3. Verify that the Free theme is there

Purchased partner themes are visible

  1. On WP.com, locate a Partner theme at https://wordpress.com/themes/partner/{your site}.wordpress.com
  2. Install it on your site (likely needs a Creator plan)
  3. In the JP app, locate the site, and open My Site > More > Themes
  4. Verify that the purchased partner theme is shown at the top of the list

Other scenarios

  • A self-hosted site without Jetpack won't show the themes screen at all
  • A Jetpack-connected self-hosted site should show WP.com themes (and possibly .org themes)

Regression Notes

  1. Potential unintended areas of impact: Theme screen
  2. What I did to test those areas of impact (or what existing automated tests I relied on): Manual testing
  3. What automated tests I added (or what prevented me from doing so): I think it might be worth adding tests but didn't investigate further than to look at ThemeServiceTests

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:

  • WordPress.com sites and self-hosted Jetpack sites

The app does not allow users to obtain paid themes (WP.com paid themes, WP.com partner themes, etc).

The app was already hiding most paid themes, but partner themes were still being shown. This change hides them as well.
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 12, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr22812-a732602
Version24.4
Bundle IDorg.wordpress.alpha
Commita732602
App Center BuildWPiOS - One-Offs #9153
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 12, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr22812-a732602
Version24.4
Bundle IDcom.jetpack.alpha
Commita732602
App Center Buildjetpack-installable-builds #8197
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

The original issue was about Partner themes should not be listed on mobile (#21932) which is now working well, therefore I am approving the PR.

However, I noticed that for other cases WP.com behaves differently from the app. I suppose not as much attention was paid for Themes purchasing on the app so these cases are not fully handled:

✅ Starter themes are hidden:
Both Starter and Premium themes are hidden. However, they are also hidden for sites with a Creator plan that have these themes included. I think we should include them if possible for sites that can install these themes.
✅ Partner themes are hidden
✅ Free themes are visible
❌ Purchased paid themes are visible:
I chose Cakely theme which requires to Upgrade the plan to activate. After I purchased the plan and activated the theme, it still wasn’t visible on Themes page on the app. Is there another way to purchase a theme?
✅ A self-hosted site without Jetpack won't show the themes screen at all
✅ A Jetpack-connected self-hosted site should show WP.com themes
❌ (and possibly .org themes):
Only WP.com themes are shown. For example, “Hello Elementor” theme is visible on wp-admin but cannot be found on the app. However, it’s a bit confusing. If I visit the “Appearance - Themes” through WP.com for Jetpack-connected site, then I need to upgrade to install “Hello Elementor”. If I visit “Appearance - Themes” through self-hosted WP-admin, then I can install “Hello Elementor” theme. Also, after I install “Hello Elementor”, it is shown as “Current Theme” in themes but I cannot find it when searching.

@dangermattic
Copy link
Collaborator

dangermattic commented Mar 12, 2024

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@guarani
Copy link
Contributor Author

guarani commented Mar 12, 2024

✅ Starter themes are hidden:
Both Starter and Premium themes are hidden. However, they are also hidden for sites with a Creator plan that have these themes included. I think we should include them if possible for sites that can install these themes.

Good catch! This was possible before this PR, so I'll restore this. It used to show a helpful message, "Please upgrade your plan to access this theme," (see https://a8c.slack.com/archives/C0180B5PRJ4/p1709752911304329?thread_ts=1709748614.878359&cid=C0180B5PRJ4) when the user tries to activate a non-partner theme they don't have a plan for. For partner themes, the error message is not helpful, "The specified theme could was not found", so this is something I want to address.

I couldn't find a good way to show only paid themes the user has access to

I looked at two options for showing only the themes the user can access on their plan:

  • Get the user's plan from https://developer.wordpress.com/docs/api/1.1/get/sites/%24site/ and fetch only themes compatible with that plan e.g. /themes?tier=personal. The challenge here is that the site endpoint returns personal-bundle-monthly while the theme endpoint expects personal, and I don't think mapping them in the app would be robust.
  • Get the user's current theme, find its plan (which is of the correct format, e.g. personal), and request themes for that plan. This should work, but might slow down the screen's loading time, because the app would need first to fetch the current theme and then fetch the available themes that are compatible with the current theme's plan (right now the app makes these requests independently of one another). It'll also require updating the Theme Core Data entity (a migration) to include the theme tier.

Neither seemed great, so I stopped here.

So I'm going to hide partner themes using theme_tier == managed-external, like Android did. It's the simplest solution to address the issue this PR is trying to fix.

❌ Purchased paid themes are visible:
I chose Cakely theme which requires to Upgrade the plan to activate. After I purchased the plan and activated the theme, it still wasn’t visible on Themes page on the app. Is there another way to purchase a theme?

I'll just reply here even though it's no longer relevant: Yesterday, I purchased a Creator plan on WP.com and set my theme to Feeling Good, which showed up at the top of the list of themes under Current Theme.

❌ (and possibly .org themes):

That's odd, I'll test this with the new changes and see if I can understand how it works.


I'll let you know when this is ready for a re-review since I've made significant changes since you reviewed.

@guarani
Copy link
Contributor Author

guarani commented Mar 12, 2024

Partner themes are hidden

✅ I checked that the Kereta partner theme is not visible in the app

Free themes are visible

✅ I checked that the Alter free theme is visible in the app

Purchased partner themes are visible

✅ I purchased a partner theme (see instructions here p1695056335721129/1695055562.563419-slack-C029JEQRVRT) and checked it's shown first in the list of themes.

A Jetpack-connected self-hosted site should show WP.com themes (and possibly .org themes)

On WP.com's Appearance > Themes, I saw the Upgrade pill on the Hello Elementor theme, but could still click through and activate the theme. I could also activate it on the site's wp-admin.

On the JP app, I don't see Hello Elementor, so that does seem like an inconsistency that could be fixed separately.


This is ready for another review 🙇

@staskus staskus self-requested a review March 13, 2024 07:00
Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

✅ Partner themes are hidden
✅ Free themes are visible
✅ Purchased partner themes are visible
It worked well now I purchased SolarOne theme with a Creator plan using instructions and it showed up on the Jetpack app Theme page.
❌ Premium themes are visible
They are still not visible to me but they are also not visible on the trunk. I have a Creator plan and Explorer/Starter themes are available on the web but not the app.

I also tested with an Explorer plan but Explorer themes do not show up for me on the app. For example, the Cakely theme you shared here p1709752911304329/1709748614.878359-slack-C0180B5PRJ4 doesn't show up for me both on trunk and on your branch. Any ideas what could be the case?

@guarani
Copy link
Contributor Author

guarani commented Mar 13, 2024

I reproduced this by installing a plugin on my site after buying the plugin plan (to make the site AT). Now, I can't see premium themes either. Without the plugin, despite having the Creator plan, the app does not support custom themes:

if ([blog supports:BlogFeatureCustomThemes]) {

and calls the v1.2/sites/171152168/themes endpoint.

With the plugin, the app supports custom themes and calls a different endpoint: /v1.2/themes and requests only free themes:

freeOnly:![blog supports:BlogFeaturePremiumThemes]

To include paid themes, the app has to support BlogFeaturePremiumThemes, but that requires a Jetpack Professional plan:

case BlogFeaturePremiumThemes:
return [self supports:BlogFeatureCustomThemes] && (self.planID.integerValue == JetpackProfessionalYearlyPlanId
|| self.planID.integerValue == JetpackProfessionalMonthlyPlanId);

This logic is outdated (see p1710190776075689/1710183825.709389-slack-C0CMN0V97), so this should be fixed in another PR.

I'm reasonably confident this is the issue you caught, so I'll go ahead and start merging these PRs. Thanks @staskus!

@guarani guarani changed the title Hide paid themes from theme browser Hide partner themes from theme browser Mar 13, 2024
@staskus
Copy link
Contributor

staskus commented Mar 13, 2024

@guarani, thanks for explaining!

This logic is outdated (see p1710190776075689/1710183825.709389-slack-C0CMN0V97), so this should be fixed in another PR.

Yes, it all sounds super outdated. I feel Themes is one of the features we could consider removing altogether if we're not supporting it properly.

@guarani guarani merged commit 935b373 into trunk Mar 13, 2024
24 checks passed
@guarani guarani deleted the fix/hide-paid-themes branch March 13, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Partner themes should not be listed on mobile
4 participants