Themes: Add Dialog when Switching From Retired Theme

[Aurorum]

Changes proposed in this Pull Request

This converts the dialog that displays when activating a theme that has a Homepage Layout to one that serves that purpose, as well as warning when switching from a retired theme. I don't think that I'm able to change a few other cases of the previous name as it appears they involve conditions outside this repo, but I don't think that's a big deal.

Testing instructions

I took immense care here to ensure that nothing is changed when switching to a Homepage Layout, but feel free to verify this just in case! The same dialog should still appear.

This just adds a message when switching to a retired theme. Select a retired theme through WP-Admin (eg. Harmonic) and attempt to activate another theme. This should appear, and clicking the primary button should activate the theme as usual. Verify that this dialog doesn't appear in other cases.

cc @lancewillett

Fixes #40310

[a8ci18n]

🆗 This change will be queued for retranslation. We'll use the existing translations in the meantime.

[Aurorum]

Bit stick on fixing the remaining three tests; it isn't the only thing dispatched, but I'm not sure how to ensure the tests recognise that.

wp-calypso/client/state/themes/test/actions.js

Line 651 in acdd5ab

test( 'should dispatch (only) installAndActivateTheme() and pass the suffixed themeId', () => {

[obenland]

@Aurorum Would you be up for a refresh on this one?

[Aurorum]

Will try to give it a go! There's a lot of conflicts though, so I think this will take some time (or it might just be worth creating a new branch instead).

[sixhours]

👋 Hey @Aurorum ! Let us know if you want to try rebasing this or just create a new PR. :)

[Aurorum]

Yes, sorry, I'll probably fix this up over the weekend (assuming it's not urgent!) :)

[kwight]

@Aurorum No, certainly not urgent, we're happy for whatever time you have to contribute. ❤️

[Aurorum]

Thanks @sixhours! Uhm, I can't immediately see a solution to that. Is there any way which I'm able to run those tests locally? I've tried updating the branch and rewriting the isUsingRetiredTheme selector slightly, but I don't really understand the problem that the tests are designed to warn against.

[mreishus]

@Aurorum : Try yarn test-client:watch in one terminal, then changing a comment in client/state/themes/test/actions.js to trigger those tests to run. That worked for me. yarn test-client should also work but it's pretty slow to run all of them.

[Aurorum]

Thanks @mreishus!

I've been looking into this a lot over the weekend, and I don't feel like I'm any closer to identifying the issue; even if you remove the getActiveTheme usage in isUsingRetiredTheme, an error still appears. There aren't any explicit references to the auto loading homepage modal or anything being changed here, so I don't understand what exactly is causing the tests to fail here - would anyone have any ideas by any chance? :)

[sixhours]

I've been looking into this, and my best guess is it's some conflict with using getActiveTheme within the theme activation action. Maybe due to the lag between the API and the state documented here:

That said, I haven't been able to figure it out for sure. 🙃

[mreishus]

client/state/themes/actions/activate.js -> Change line 44 to themeHasAutoLoadingHomepage( getState(), themeId ) && -> Test starts passing.

I'm not completely sure what's going on, but it's possibly that calling activate on a theme in a test used to fetch its data, but after the changes made, it does not, it only shows a warning instead.

yarn run jest -c=test/client/jest.config.js client/state/themes/test/actions.js to run the test directly

[sixhours]

I'm not completely sure what's going on, but it's possibly that calling activate on a theme in a test used to fetch its data, but after the changes made, it does not, it only shows a warning instead.

I was wondering about this -- it's funny because the site ID we pass to the test is a very old site using a retired theme. I bet that could product unexpected results when we show a modal but we're expecting to activate a theme instead. I'll dig into this more.

[sixhours]

Nope, changing the site ID to one that uses a theme that is not already retired doesn't seem to make a difference. I'm stumped.

[mreishus]

is-using-retired-theme is crashing when it calls get-active-theme, because activeThemes are not defined and it assumes that activeThemes[siteId] exists

diff --git a/client/state/themes/selectors/get-active-theme.js b/client/state/themes/selectors/get-active-theme.js
index 5a7afdff91..fa05b88cac 100644
--- a/client/state/themes/selectors/get-active-theme.js
+++ b/client/state/themes/selectors/get-active-theme.js
@@ -19,7 +19,10 @@ import 'calypso/state/themes/init';
  * @returns {?string}         Theme ID
  */
 export function getActiveTheme( state, siteId ) {
+       console.log( { activeThemes: state.themes.activeThemes, siteId } );
+       console.log( 'before crash 2' );
        const activeTheme = state.themes.activeThemes[ siteId ] ?? null;
+       console.log( 'after crash 2' );
        // If the theme ID is suffixed with -wpcom, remove that string. This is because
        // we want to treat WP.com themes identically, whether or not they're installed
        // on a given Jetpack site (where the -wpcom suffix would be appended).
diff --git a/client/state/themes/selectors/is-using-retired-theme.js b/client/state/themes/selectors/is-using-retired-theme.js
index ee052390e0..05de6623b7 100644
--- a/client/state/themes/selectors/is-using-retired-theme.js
+++ b/client/state/themes/selectors/is-using-retired-theme.js
@@ -14,5 +14,9 @@ import 'calypso/state/themes/init';
  * @returns {?boolean}          Whether the current theme is retired.
  */
 export function isUsingRetiredTheme( state, siteId ) {
-       return getCanonicalTheme( state, siteId, getActiveTheme( state, siteId ) )?.retired;
+       console.log( 'before crash' );
+       console.log( getActiveTheme( state, siteId ) );
+       console.log( 'after crash' );
+       // console.log( getCanonicalTheme( state, siteId, getActiveTheme( state, siteId ) ) );
+       return false;
 }

[Aurorum]

Thanks for looking into this. I'm still unclear as to how other selectors can rely on getActiveTheme but I can't here though.

wp-calypso/client/state/selectors/should-customize-homepage-with-gutenberg.js

Line 17 in 38c37bb

const theme = getActiveTheme( state, siteId );

I feel like I'm probably missing something quite obvious but I'm not sure what?

[mreishus]

It may have to do with the state set up in the tests. I think adding safe navigation operators may be sufficient:

diff --git a/client/state/themes/selectors/get-active-theme.js b/client/state/themes/selectors/get-active-theme.js
index 5a7afdff91..d23e4d8c7b 100644
--- a/client/state/themes/selectors/get-active-theme.js
+++ b/client/state/themes/selectors/get-active-theme.js
@@ -19,7 +19,7 @@ import 'calypso/state/themes/init';
  * @returns {?string}         Theme ID
  */
 export function getActiveTheme( state, siteId ) {
-       const activeTheme = state.themes.activeThemes[ siteId ] ?? null;
+       const activeTheme = state.themes?.activeThemes?.[ siteId ] ?? null;
        // If the theme ID is suffixed with -wpcom, remove that string. This is because
        // we want to treat WP.com themes identically, whether or not they're installed
        // on a given Jetpack site (where the -wpcom suffix would be appended).

You'd want to test this out when actually using the site as well

[Aurorum]

Thank you! That seems to work while still ensuring everything functions the same.

[sixhours]

Yay, tests are passing! But I found another bug. 🙃 I don't think we can ship this until it's fixed.

When you switch from a retired theme to another non-auto-loading-homepage theme in the Trending tab, the modal doesn't appear and the theme doesn't activate. This behavior is working on production but not with this PR. Going to dig into it and see if I can find the cause.

[sixhours]

Urgh, it's related to the bug I patched temporarily in #54648 -- updating the number of themes to show by default to a ridiculous number like 300 fixes the bug.

Because Twenty Eleven appears in the All Themes view somewhere within the top 300 themes, but not the top 30, so it doesn't "count" when viewing a non-paginated static query like Trending, even if it appears close to the top of that query.

[sixhours]

I spent all afternoon chasing this bug down, and I think I finally found it.

I've been able to trace this back to installingTheme being null when we call it from a theme that isn't yet loaded into state. The modal doesn't get passed an installingTheme object so it never opens:

https://github.com/Automattic/wp-calypso/pull/40793/files#diff-a1a31bc09661de2bd55c1207c918ade388a6c3bcc7932a369893bc9247bc2efaR165

I confirmed the dispatch action is happening as intended and passing the correct installingThemeId. However, getTheme returns null for that ID because the query has only loaded the first 30 results into state (value controlled by this number):

https://github.com/Automattic/wp-calypso/blob/trunk/client/state/themes/selectors/get-theme.js#L23

Any theme outside the first 30 results for that query is null and never gets loaded into state unless you visit All Themes and scroll down to the bottom of the Showcase (which refreshes the query and updates state with the data for every single theme).

This may be partly fixed in this PR by rebasing to include this recent bug fix -- the query might be pulling back the wrong first 30 themes because it's a merged query with the Trending themes and Recommended themes -- but it won't fix the larger issue, which is finding a way to load all the theme results for a particular query into state. That could involve enabling pagination for custom queries, or setting the static integer value high enough to include all the themes we expect to pull back in a particular custom query.

I think in the interim, since we have a relatively small number of custom queries, we can pass a prop to ThemesSelection that specifies the number we expect to pull back. In the case of Trending themes, that would be all of them, for Recommended themes, it's about 30. For FSE themes, it's about 5, etc. It's hacky, but I suspect it will be faster and easier than trying to enable pagination on custom queries.

[sixhours]

This PR also breaks theme activations for FSE themes when you enable the flag ?flags=gutenboarding/site-editor

Again, because those themes aren't pulled into state with the initial Recommended query. 🙃 I think rebasing to include #54770 might fix that, though.

[Aurorum]

Sorry for the additional ping while resolving conflicts - didn't realise that would happen! This doesn't impact that codebase, it was just for updating the branch.

[Aurorum]

Thanks for taking the time to look into this @sixhours! I've included #54770 as suggested, and I think that's fixed the issues you've mentioned, but I must admit that I never noticed them to begin with so I'm not certain.

[sixhours]

Unfortunately it's still breaking activations for some themes when you're switching away from a retired theme using the Trending section. 🙃 I don't know if this is edge-case enough for it to be considered a blocker.

@kwight what do you think? Continue holding on this or ship it and document the bug? I'm hoping @mreishus forays into using React Query will have the happy side effect of fixing this bug.

[kwight]

@sixhours If the broken behaviour isn't in prod, let's not introduce the regression.

If I understand correctly, the issue is that it's possible to have an empty installingTheme object if switching from (or to, in the case of FSE?) a theme that's not in state (whether retired, or not part of the current tab context)? (If that's true, where does the current theme info come from, a different part of the state tree?)

Hm, retired or not, it seems like the current theme needs to be where it's expected in the state tree, and FSE themes would all be in state for the tab to display properly (and a theme to get selected)? @blackjackkent @mreishus is there something we're missing here? 🙃

[kwight]

I'm so sorry @Aurorum ; after some internal discussion about retired themes and tooling, I think we're better off leaving this dialog as-is (so, dropping this PR). Again, so sorry this one didn't work out, we do really appreciate all of your work. ❤️

[Aurorum]

No worries - thanks to everyone who spent time helping to try and figure this out, and sorry it couldn't work in the end. :)