Site-kit-wp: Analytics: Property and View dropdowns show "Setup ..." option multiple times

[felixarntz]

Bug Description

When editing Analytics settings, I see the "Setup a new property" and "Setup a new profile" options multiple times in the respective dropdowns.

I suspect there is some loop issue in the code, because in my case I was seeing both items 4 times in each dropdown instead of 1 time, and I have 4 properties. After creating 2 more, both items should up 6 times, so this seems related.

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• The "Setup a new property" and "Setup a new profile" options in Analytics settings should only display once each.

Implementation Brief

• Refactor getAccounts() in AnalyticsSetup https://github.com/google/site-kit-wp/blob/fef0338719b4eba8159ebe0c50e69b3bbe7f4c41/assets/js/modules/analytics/setup.js#L317 to remove the properties.push calls and instead add a single "Setup a New Profile" item to the end of the array. Right now the responses are being modified (not ideal) and .push() has the possibility of being called several times.
• The processAccountChange() needs similar refactoring as it also pushes onto responseData.properties and responseData.profiles.
• Fix getCache and setCache (https://github.com/google/site-kit-wp/blob/b26b4c435f859a58f50d9ce7704ad24ec2af0b6c/assets/js/components/data.js#L312-L323 & https://github.com/google/site-kit-wp/blob/4bd761ff60559e1a2fa3e65257b6fbceae0a1c7a/assets/js/components/data.js#L277-L302) to store and retrieve _copies_ of compound variables (eg Object and Array—Function types should not be cached...) rather than references to the variables. This will prevent future bugs related to cached variables being mutated, as is likely the case here (see https://github.com/google/site-kit-wp/issues/522#issuecomment-533496798 below)

Changelog entry

• Fix bug where the options to set up a new Analytics property or profile were shown multiple times.

[tofumatt]

Looks like there's both an issue with .push() being called on the arrays used to populate these <select> fields but possibly also an issue with them being called multiple times. It seems like the wrong property might be getting updated as well, but I've noted in the IB that, essentially, the way each array for the <select> fields are being constructed needs refactoring—it's probably best to augment the array with a "Setup new property" item at render time instead of push()ing in multiple places.

[felixarntz]

IB ✅

[aaemnnosttv]

@tofumatt You're right about modifying the responses but I think the more important underlying issue here is that cached values can actually be mutated once returned from the variable cache.

https://github.com/google/site-kit-wp/blob/b26b4c435f859a58f50d9ce7704ad24ec2af0b6c/assets/js/components/data.js#L312-L323

This should return a copy for reference safety if the value is a non-primitive. Also, setCache should also store a copy so that the cached value cannot be mutated by reference after caching.

[tofumatt]

Oh my, I see! I didn't realise that, and I suppose it makes sense that it's contributing to the issue here.

I've updated the IB to include this fix. I wonder if any other bits of the codebase are affected by this bug; could be!

[felixarntz]

Thanks for catching this @aaemnnosttv, that's also gonna be a quite major fix to save us from pain in the future!

IB ✅

[tofumatt]

Marking this as "S" just because of the implications of how getCache works in general and wanting to test this one well. I think it's on the lower side of "S", but giving it headroom just in-case.

[tofumatt]

Actually, looking through the code it seems the cache mutation is the sole cause of problems, so I don't think the .push() calls are the issue. I didn't fully grok that those calls weren't ever run one after the other. It's possible those calls were messing with the referenced data though, and my patch fixes that cache issue.

It's worth noting I'm finding it difficult to replicate this issue now, but my cache fix will prevent future gotchas 😄

[aaemnnosttv]

@felixarntz CR ✅

[adamsilverstein]

QAing on https://github.com/google/site-kit-wp/pull/624 and after those changes I no longer see the repeated "Setup..." menu items.

[marrrmarrr]

Testing this on https://culinarian.life/, with 1.0.0-beta.1.0.8-RC1, I still see the multiple entries both during the Analytics setup flow and in the settings page:

[aaemnnosttv]

@marrrmarrr do you see the same with RC2?

[felixarntz]

In RC2, this works as expected. ✅