Shuffle the list of available site designs during gutenboarding to prevent bias in user design selection

[autumnfjeld]

Changes proposed in this Pull Request

• Randomize the list of available designs during gutenboarding /new/design step
• Persist the randomizedDesigns array in localStorage to maintain the same order of designs during a user's gutenboarding session
• on boot, verify that the cached list of designs is up to date with designs in available-designs-config.json

Testing instructions

• Go to /new , proceed through flow
• When you arrive at http://calypso.localhost:3000/new/design
  • note the oder of the designs
  • check the WP_ONBOARD store
• Continue on through flow for a page or two
• Click Go back link to go back to /new/design
  • ensure that the design order is the same
• Refresh the page at /new/design
  • ensure that the design order is the same
• Clear the WP_OBOARD store
• ensure that the design order is NOT the same, should be a new random order

Fixes

Issue #44874

Follow up

Follow up work noted in comment below #44979 (comment)

[matticbot]

Test live: https://calypso.live/?branch=update/randomize-gutenboarding-design-selection-list

[autumnfjeld]

Just FYI, this is only first draft of reaching the final goal of 1) randomizing the design list 2) persisting the list order during a user session.

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~160 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding      +1088 B  (+0.1%)     +160 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~126 bytes added 📈 [gzipped])

name              parsed_size           gzip_size
gutenberg-editor       +358 B  (+0.0%)      +63 B  (+0.0%)
checkout               +358 B  (+0.0%)      +63 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~63 bytes added 📈 [gzipped])

name                                  parsed_size           gzip_size
async-load-layout-masterbar-checkout       +358 B  (+0.2%)      +63 B  (+0.1%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[autumnfjeld]

@andrewserong

Thanks again for the great tips in this issue!!

Item 1 🍔

I think I'm grasping the functionality of the Gutenboarding Redux store, and that we'll need to add an item in the persist array and create an action and a reducer. (That is the general idea at least.) Then I can just follow the existing dispatch and select patterns in the Gutenboarding code. For example following selectedFonts usage in
https://github.com/Automattic/wp-calypso/blob/8ade0c19c506e83df0947f423b8737ff1c0febc4/client/landing/gutenboarding/onboarding-block/style-preview/index.tsx
and
https://github.com/Automattic/wp-calypso/blob/8ade0c19c506e83df0947f423b8737ff1c0febc4/client/landing/gutenboarding/onboarding-block/style-preview/font-select.tsx.

If all of the above makes sense, then I'm definitely making progress on code comprehension! 🤓

Item 2 🍜

In the process of deciphering all of the above I came across a few things that didn't make sense:

• I found a variable in the persist array called pageLayouts in the Gutenboarding Redux store.
  and thought pageLayouts might be a synonym for site design, so I followed the breadcrumbs on this….
• In client/landing/gutenboarding/stores/onboard/index.ts pageLayouts is in the persist array and it has a reducer in

  wp-calypso/client/landing/gutenboarding/stores/onboard/reducer.ts

  Line 99 in 8ade0c1

  const pageLayouts: Reducer< string[], OnboardAction > = ( state = [], action ) => {

• I see the correlation between the persist array, actions and reducers
  • client/landing/gutenboarding/stores/onboard/index.ts
  • client/landing/gutenboarding/stores/onboard/actions.ts
  • client/landing/gutenboarding/stores/onboard/reducer.ts
• For example I can see the relationship for domainSearch or siteTitle across those three files
• BUT pageLayouts isn’t in actions.ts INSTEAD the singular pageLayout is found
• Now…following breadcrumbs for the pageLayout (no s). I found togglePageLayout, but it doesn’t seem to be used anywhere.
• Am I missing something? Did I find dead code? Or maybe there is intention to use it somewhere?

Item 3 🍰

And a terminology question:
Does pageLayout mean the same thing as design ? Are these terms interchangeable?
Any suggestions on the variable name I should create for persisting the shuffled design list? Ideas: siteDesigns randomizedDesigns

[andrewserong]

Great questions @autumnfjeld, thanks for diving in!

Item 1

Yes, I think actions and reducers are an appropriate way to handle this. It could also make it a good starting point for future follow-ups if we ever wish to replace the hard-coded available-designs-config.json with a call to an endpoint. A good place to call the selector during boot, and then dispatch an action might be here in window.AppBoot — https://github.com/automattic/wp-calypso/blob/HEAD/client/landing/gutenboarding/index.tsx#L77.

Another thing to consider is that if we wish to update any of these designs, will their metadata potentially be cached in the store? So, would it be better to persist the sort order of the designs rather than the designs themselves? Or, instead, can we persist the full list of designs, and then at boot check to make sure that none of them have changed (validate that the cache is safe before proceeding).

For a concrete example, let's say we decide to remove one of the designs from the backend and from the available-design-config.json. What happens for the user with the persisted state? Or, if we change the theme, template or any other key on one of the designs, how do we ensure that they reliably flow through to the user?

Just some thoughts! 🙂

Item 2

I'm so glad you asked this question because I'd completely forgotten about it! Early on there was the idea that there would be a separate step in the flow where you could select which pages you'd like to add to your site. So, you've already selected your design (how the homepage will look), and then you can select which pages you'd like to add, so you might add an About page and a Contact page. Clicking each of these page layouts would add the string for that layout to the state. So if you added both of these you'd get [ 'about', 'contact' ] in state. You'd select these individually, which I think is why we have the toggle action for a single page layout, but not an action for managing all page layouts in state at once.

I think the idea of this feature might still be on the cards at some point, so we might wind up using it eventually. That said, it'd probably be safe for us to take it out in a separate PR at some point if we feel like it'd clean things up by removing it.

Item 3

Does pageLayout mean the same thing as design ? Are these terms interchangeable?

Different things, covered in item 2 ☝️

Any suggestions on the variable name I should create for persisting the shuffled design list? Ideas: siteDesigns randomizedDesigns

I think either works, but I quite like randomizedDesigns for this one. It makes it a bit clearer the intention behind why we decided to keep this in persisted state.

Hope that helps, happy to talk through any of the points in more detail!

[autumnfjeld]

Any idea why line 21 uses a different pattern, with the .getState() call at the end?

I noticed inconsistency with the store name, here STORE_KEY is used but in client/landing/gutenboarding/onboarding-block/design-selector/index.tsx ONBOARD_STORE is used.

And then another difference, the persistent store is called WP_ONBOARD, though I could understand a motivation for calling the persistent store a different name.

[andrewserong]

I think it might be that historically in Gutenboarding we only had a single store, so referred to it as STORE_KEY, then when additional stores were added (e.g. PLANS_STORE) we needed to distinguish between them. In this file it would make sense to use the same import { STORE_KEY as ONBOARD_STORE }; line as we're using elsewhere in Gutenboarding. You're welcome to change it in this PR if you'd like (or leave it as is, whichever you prefer).

I think the WP_ prefix for the persistence is probably going with the pattern borrowed from Gutenberg of using that prefix: https://github.com/wordpress/gutenberg/blob/HEAD/packages/data/src/plugins/persistence/index.js#L38 but I could be wrong!

[autumnfjeld]

Coolio, I'll make the change to use import { STORE_KEY as ONBOARD_STORE }; for consistency.

[autumnfjeld]

@andrewserong This change has a bit of a ripple effect with respect to typings.
In this font-select.tsx file, I would be replacing

	const { selectedDesign, selectedFonts } = useSelect( ( select ) =>
		select( STORE_KEY ).getState()
	);

with

	const { getRandomizedDesigns, getSelectedDesign, getSelectedFonts } = useSelect( ( select ) =>
		select( ONBOARD_STORE )
	); 

and thus replacing all instances of selectedDesign with getSelectedDesign() and replacing selectedFonts with getSelectedFonts().

If we look at selectedFonts in this line

wp-calypso/client/landing/gutenboarding/onboarding-block/style-preview/font-select.tsx

Line 50 in f3a765b

{ getFontTitle( selectedFonts.headings ) }

when I update that with the selector function getSelectedFonts() , typescript complains (see screenshots below) about the potential undefined values.
What I don't understand is that the selector is just returning the reducer value! The selectedFonts reducer is typed to return FontPair | undefined (assuming I'm understanding all this correctly). Do you have an idea about why Typescript was not complaining about a possible undefined state before I made the change to use getSelectedFonts?

[andrewserong]

Good question! For the TypeScript problem, it looks like that line is currently covered by a check that selectedFonts is not undefined (e.g. https://github.com/automattic/wp-calypso/blob/83dbb0fa46689e25608974d5f0768b47a25d8fd3/client/landing/gutenboarding/onboarding-block/style-preview/font-select.tsx#L46), so when the headings key is accessed, that code path can only run if it's really available. However if you replace that check with getSelectedFonts(), TypeScript can't know that the next time you call it, the return of that function is definitely going to be the same. From TypeScript's perspective, that function could still potentially return undefined. In principle this is a good thing, because if the behaviour of the getSelectedFonts function/selector were to change in the future, we'd need to know this part of the code is still safe.

To keep your changeset small, you could update the name of the constant for the store, but otherwise leave the useSelect call as is, and add your line directly after it. E.g.

const { selectedDesign, selectedFonts } = useSelect( ( select ) =>
		select( ONBOARD_STORE ).getState()
	);
const { getRandomizedDesigns } = useSelect( ( select ) => select( ONBOARD_STORE ) );

However it also might make sense to group these together. If you do, on the line you're currently looking at, you wouldn't want to call getFontTitle with an undefined value, so I think this part of the component might still need to be guarded by the selectedFonts check. I guess you could add a line before selectedFontOption — const selectedFonts = getSelectedFonts(); — but either way there's a bit of duplication!

These sorts of little optimisations can sometimes be tricky! My preference is usually to try to change the smallest amount possible that's needed for the change I'm proposing in a PR, and then refactor as necessary in a follow-up janitorial PR. However, sometimes it feels messier to do it that way, so if it makes more sense to change these calls, then by all means, cleaning as you go is good, too!

[autumnfjeld]

Thanks for that clarification with regard to const selectedFontOption = selectedFonts ? ( ....:

it looks like that line is currently covered by a check that selectedFonts is not undefined

Now the typescript issue makes sense. 💡

Now or next :), with a little more awareness under my belt, I did a search for select( ONBOARD_STORE ) and .getState() and I can see there is a mix of usage of calling .getState() and using reducers vs not calling .getState() and using selectors, so I will update the ONBOARD_STORE name but I will not' make the other changes that replace the reducer with the selectors since both patterns are used throughout gutenboarding.

That was all great exploration to understand reducers and selectors!!! 💡 💡

[autumnfjeld]

@andrewserong

I'm ready for another WIP review. 🤓

I left comments on specific lines in https://github.com/Automattic/wp-calypso/pull/44979/files. And of course welcome any additional tips, suggestions, ideas for improvements! 👍

I decided to got with persisting the actual randomized array, rather than the sort order. Turned out to be relatively straightforward to compare arrays with a bit of lodash help. I just need to create a test for the isUpToDate utility function.

I haven't give any thought to tests or checking existing tests for gutenboarding code, so that is what I am going to dive into next.

[andrewserong]

Great work so far @autumnfjeld! This approach is looking very neat to me, particularly the clean, well-named functions in index.tsx, that make it easy to read what's going on. We usually try to limit our dependency on lodash where we can in Gutenboarding and Calypso, because it can sometimes either a) obscure what's going on, or b) unintentionally add a lot of additional code to our bundle size that we might not need.

I think this is a great case for leaning on lodash where it excels, though, which in this case is for deep object comparison and it wouldn't really make sense for us to roll our own here. You've also abstracted away the lodash calls behind isUpToDate, so it'd be straightforward to replace the innards of this function further down the track if we ever decide we want to get rid of lodash altogether. Also, when importing functions that mightn't have been used elsewhere in Gutenboarding I always like to check the matticbot comment on the PR to just make sure that our bundle size isn't increasing unexpectedly, and here we can see that the bundle size difference in this PR is negligible: #44979 (comment), so I think you're good to go!

Excellent idea now looking into adding test coverage. I don't think we've got much in the way of test coverage for Gutenboarding yet, which has been brought up a number of times as something we want to address now that the flow is fairly complex. Not sure if it helps, but we've got some minimal tests in the data-stores package to refer to, and if you search for testing-library across the rest of Calypso you'll find lots of tests. Here's one at random: https://github.com/automattic/wp-calypso/blob/HEAD/client/my-sites/media-library/test/list-item-video.jsx#L8

I haven't tested this PR manually yet, but let us know when it's ready, and I can give it a closer look 😀

[autumnfjeld]

@andrewserong I'm ready for a final review & manual test of my work.

There are three things, as discussed in our PR convos, that I didn't do in the this PR that I will/could do in other PRs:

1. The janitorial alphabetizing
2. Updating the useSelect usage per this convo because my attempts at changes got "complicated" and I felt these should be addressed in a subsequent PR after chatting with you in more detail
3. Tests!!!! I see now there aren't any tests with gutenboarding/ folder (ya? as you mentioned) So....I'd be happy to set up a first test file, but figured this might be an effort that is also more appropriate for a subsequent PR. I feel dirty not having a test for the biz logic of my isUpToDate lodash magic, but am ok with merging PR without a test 😲 if there is consensus that it is ok.

[andrewserong]

Great work here @autumnfjeld! If I run out of time to give this a detailed test today, I'll take a look first thing on Monday. Good idea re: moving those other tasks to follow up PRs!

I feel dirty not having a test for the biz logic of my isUpToDate lodash magic, but am ok with merging PR without a test 😲 if there is consensus that its ok.

If we're feeling confident through manual testing, I think it'll be fine to merge in without a test (the code's looking good to me). However, if you do want to get a test in, we could always move the isUpToDate function up to a utils directory and import it in a test. Adding a test should be fairly straightforward (we can add a test directory anywhere in Gutenboarding and yarn run test-client client/landing/gutenboarding will pick it up), but we will run into issues if we try to import index.tsx directly in a test because of how much it's doing when that module is loaded. So, moving utility functions possibly makes it easier to get them under test. Again, I think that's perfectly fine to do it in a follow up, instead 👍

[alshakero]

Brilliant work, thank you! The xor idea is great, wouldn't occur to me at all.

All the comments I gave are 100% optional and more on the nit side. LGTM!

[alshakero]

Slight nit: It seems this function doesn't test for up-to-date-ness, rather equality disregarding order. Maybe call it something less generic like isNominallyEqual.

[autumnfjeld]

Yes, it is checking equality disregarding order, so I see your point. I renamed the function to isDeepEqual. I didn't really understand the usage of 'nominally' here (after I looked it up in the dictionary and I am a native English speaker lol), so I choose deep equal instead. :)

[alshakero]

Since we know design slugs are unique, maybe we can map these arrays to slugs to make the comparison faster, i.e compare strings vs traverse and compare objects.

Also, an early length check might save a couple cycles.

Suggested change

	return isEmpty( xorWith( stored, available, isEqual ) );
	if( stored.length !== available.length ) return false;
	const storedSlugs = stored.map( ( { slug } ) => slug );
	const availableSlugs = available.map( ( { slug } ) => slug );
	return isEmpty( xorWith( storedSlugs, availableSlugs ) );

If you agree with this, we can get rid of the isEqual comparator, I'm not 100% sure but I think if you omit it, lodash would use ===, which is OK for strings.

[andrewserong]

Thanks for reviewing @alshakero! For this function, I think we'll still need to do a deep comparison to protect the data integrity of any of the design objects. My thinking was that if we change anything in available-designs-config.json we'll want the cache to be invalidated so that someone with persisted state doesn't have incorrect values. E.g. if we swap template, theme, or categories, or accidentally ship a design with a typo in one of these fields, we'll want to make sure that a follow-up deploy will reach users with persisted state.

[alshakero]

oooh totally missed that! Was just thinking about adding or deleting whole designs.

[alshakero]

Type is implied by TS since design comes from getRandomizedDesigns().

Suggested change

	( design: Design ) => design.slug === selectedDesign?.slug
	( design ) => design.slug === selectedDesign?.slug