Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.42k stars 1.99k forks source link

"My Themes" tab not working/loading #61213

Closed jp-imagines closed 2 years ago

jp-imagines commented 2 years ago

Quick summary

On atomic sites, the "My Themes" tab (in Calypso at Appearance > Themes) should display a list of installed themes. Currently, it seems to always display the full list of themes from "All Themes" instead.

Steps to reproduce

  1. On an atomic site, navigate to Appearance > Themes.
  2. Click the "All Themes" tab.
  3. Switch to the "My Themes" tab.

What you expected to happen

"My Themes" should show only installed themes.

What actually happened

"My Themes" shows the same list as "All Themes".

Context

4789426-zd-woothemes

Simple, Atomic or both?

Atomic

Theme-specific issue?

No response

Browser, operating system and other notes

No response

Reproducibility

Consistent

Severity

Most (> 50%)

Available workarounds?

Yes, easy to implement

Workaround details

Installed themes are still accessible from WP Admin (via the Screen Options > Classic view toggle).

Greatdane commented 2 years ago

Another report: 4800974-zd-woothemes

Robertght commented 2 years ago

Asking here for a second look: p1645512559450039-slack-C0Q664T29

sixhours commented 2 years ago

I traced this back to #60980; going to investigate that diff to see what might have changed/broken this.

sixhours commented 2 years ago

This line is the issue:

https://github.com/Automattic/wp-calypso/pull/60980/files#diff-3e59937e475ddbe025a5e9ad02ba04982a7872f910c99164212098f138ac899cR218

Adding ! isAtomic to that logic causes the ThemeSelection for "My Themes" to be passed list of all wpcom themes rather than themes specific to the site's ID for Atomic sites.

/cc @DustyReagan @mmtr could you take a look? I'm not sure why the logic for the themes query needs to change to show/hide the price and upsell as suggested by the original issue:

On the themes page, Premium themes should include the price and upsell nudge/star on Atomic sites with sub Premium plans. Atomic sites with Premium and higher plans should not show the price or upsell nudge/star.

mmtr commented 2 years ago

I'm not sure why the logic for the themes query needs to change to show/hide the price and upsell

I don't know either (@DustyReagan might know more since he worked on the fix) but I just made a quick test removing the isAtomic check from that line and I've confirmed that it replaces the star icon + price (which is the intended behavior) with a PREMIUM label for free Atomic sites, so apparently it's needed 🤔.

With isAtomic check Without isAtomic check
Screen Shot 2022-02-23 at 14 12 59 Screen Shot 2022-02-23 at 14 08 02
sixhours commented 2 years ago

The logic we need to work with to change what's shown there is in the individual theme card:

https://github.com/Automattic/wp-calypso/blob/trunk/client/components/theme/index.jsx

And I think this is related to a similar issue I ran into when adding the "Premium" flag in #60207

We're checking the price prop on the theme object to determine whether to show the price, which is set depending on the user's access to the themes, not their actual price -- a theme that's $50 will have a null/undefined value for price to a user on the Premium plan, for example, and a value of $50 to a user on a Free plan.

My hunch is that somewhere up the chain where the theme data is set, we're assuming Atomic sites (regardless of plan) have access to all premium themes, so the price prop comes back undefined/null. Going to work my way backwards from there and see if we can adjust the logic to account for the plan type, not just the site type.

sixhours commented 2 years ago

My hunch is not right; this seems to have something to do with state management. I've found the price prop does get set and shown correctly if you visit the individual theme page, then go back to the themes list. Video demo:

https://user-images.githubusercontent.com/2124984/155351566-68ee6e14-3b30-4751-ae1d-12c456f39a98.mov

The price data isn't in state at first. Loading the individual theme page populates all the data for one theme correctly so it's there when you go back to the larger view.

I'll keep digging!

sixhours commented 2 years ago

I traced it back through multiple selectors to this isPremium check, where we do a get request for the theme's stylesheet that initially returns false for my free Atomic site:

https://github.com/Automattic/wp-calypso/blob/trunk/client/state/themes/utils.js#L23-L26

Since there's no stylesheet found, the theme is considered "not premium" and does not show the price correctly.

For free Atomic sites, I don't think the themes are loaded into state correctly at first; I noticed a permissions error with the themes API request for my site (/rest/v1.1/sites/{free-atomic-site-id}/themes):

Screen Shot 2022-02-23 at 11 43 47 AM

But the themes query and the single theme query are successful because they're public endpoints: /rest/v1.2/themes/ and /rest/v1.2/themes/{theme-id}

The permissions error comes from the Jetpack JSON API. So my free Atomic site is treated as a Jetpack site on which I don't have permissions (though I do!) and theme data is not available via the site's Jetpack API, which probably explains why a get request does not find the theme data initially.

I don't know if the API permissions issue is unique to my free Atomic site, or if it's an issue with all free Atomic sites, though.

sixhours commented 2 years ago

@DustyReagan this looks more complex than I thought (see above). :D I'm thinking we should revert #60980 since the broken "My Themes" tab feels more urgent than the upsell issue with the individual theme cards; what do you think?

DustyReagan commented 2 years ago

Thanks for looking at this @sixhours! And sorry for the delay! I had my head in another issue. This seems about as complicated as I remember! 😅 I think you're right about priorities. I can take a closer look tomorrow and setup a revert if necessary.

DustyReagan commented 2 years ago

Can I get your thoughts on this PR @sixhours?

DustyReagan commented 2 years ago

Fixed in https://github.com/Automattic/wp-calypso/pull/61468