A4A > Sites Dashboard: Fix the refresh issue with the favorite and monitor toggle functionality

[cleacos]

Related to #

Proposed Changes

We had an issue when we clicked on the Favorite star and the Monitor to toggle the value. The Monitor toggle issue is in production, but the Favorite star fails with the PR that updates the DataViews package for the same issue.

The issue is with the queryKey, both actions (Favorite and Monitor switchers) works in an optimistic approach, that means that they update the React query cache without trying to refetch the sites. The problem is the way that the queryKey is created. For example, if the parameter is undefined and when the data was fetched the parameter is empty, the queryKey is different, so that is the reason that optimistic implementation fails because it doesn't find the site in the cache returned. The same happen if the queryKey creating doesn't follow the same parameter orders, if you introduce a parameter in a different order, the queryKey is different, and again, the optimistic implementation fails.

A part of this queryKey issue, the Monitor toggle had another issue, it had this missing parameter: showOnlyDevelopmentSites.

This PR fixes the queryKey issue, adding a simple helper function to create a hash of the parameter, regardless of the parameter order or if one of them is undefined or null. The hash function is simple; it sums the character codes of each parameter.

Why are these changes being made?

Testing Instructions

• Check the code
• Go to the /sites section
• Try to toggle the Monitor of one of your sites.
  • In trunk, it won't the value, but the query is successful. If you try to toggle again, an error will be displayed
  • This branch will refresh the value and work normally.
• Check that the Favorite starts to continue working. It will be tested in the DataViews updated branch.

Pre-merge Checklist

• Has the general commit checklist been followed? (PCYsg-hS-p2)
• Have you written new tests for your changes?
• Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
• Have you checked for TypeScript, React or other console errors?
• Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
• Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
• For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

[github-actions]

Calypso Live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-118984

Jetpack Cloud live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-118984&env=jetpack

Automattic for Agencies live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-118984&env=a8c-for-agencies

[matticbot]

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

• notifications
• wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug fix/a4a/favorite-and-monitor-toggle-refresh-isssue on your sandbox.

[matticbot]

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

Sections (~194 bytes added 📈 [gzipped])

name                           parsed_size           gzip_size
a8c-for-agencies-sites              +438 B  (+0.0%)     +135 B  (+0.0%)
jetpack-cloud-agency-sites-v2       +268 B  (+0.0%)      +85 B  (+0.0%)
a8c-for-agencies-overview           +108 B  (+0.0%)      +30 B  (+0.0%)
a8c-for-agencies-marketplace        +108 B  (+0.0%)      +25 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.

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.

[yashwin]

Great work on fixing this, @cleacos!

• Code changes: Mostly looks great. I have left a few comments mostly on the missing dependencies.
• Testing: The Monitor functionality works great. Favorites are working as expected, and I don't see any difference from the production. Is there anything specific I need to be testing here?

Overall, it looks good. I think it's better to update the dependencies. Let me know your thoughts!

[oandregal]

So, essentially, the fix is to provide an empty string if a parameter is undefined? This was the issue we ran to in this PR #93503 and it would have fixed, indeed.

However, in my view, the core issue is different and I can provide a test case to demonstrate how this PR doesn't fix it. Apply the following patch to your environment and you'll see how the toggle monitor doesn't work even with all this hashing:

diff --git a/client/jetpack-cloud/sections/agency-dashboard/hooks/use-toggle-activate-monitor.tsx b/client/jetpack-cloud/sections/agency-dashboard/hooks/use-toggle-activate-monitor.tsx
index a16c3dbaf8..0f65b914a6 100644
--- a/client/jetpack-cloud/sections/agency-dashboard/hooks/use-toggle-activate-monitor.tsx
+++ b/client/jetpack-cloud/sections/agency-dashboard/hooks/use-toggle-activate-monitor.tsx
@@ -25,7 +25,7 @@ export default function useToggleActivateMonitor(
 
        const queryClient = useQueryClient();
        const { filter, search, currentPage, sort } = useContext( SitesOverviewContext );
-       const { dataViewsState, showOnlyFavorites, showOnlyDevelopmentSites } =
+       const { dataViewsState, showOnlyFavorites } =
                useContext( SitesDashboardContext );
 
        const agencyId = useSelector( getActiveAgencyId );
@@ -41,7 +41,6 @@ export default function useToggleActivateMonitor(
                                                        {
                                                                issueTypes: getSelectedFilters( dataViewsState.filters ),
                                                                showOnlyFavorites: showOnlyFavorites || false,
-                                                               showOnlyDevelopmentSites: showOnlyDevelopmentSites || false,
                                                        },
                                                        dataViewsState.sort,
                                                        dataViewsState.perPage,
@@ -58,7 +57,7 @@ export default function useToggleActivateMonitor(
                                                        ...( agencyId ? [ agencyId ] : [] ),
                                                ] ),
                                  ],
-               [ agencyId, currentPage, dataViewsState, showOnlyFavorites, showOnlyDevelopmentSites ]
+               [ agencyId, currentPage, dataViewsState, showOnlyFavorites ]
        );
 
        const toggleActivateMonitoring = useToggleActivateMonitorMutation( {

[oandregal]

In my view, the core issue is that query keys are generated in different places and each place may mis-introduce data (empty string vs undefined, missing a key, stale data in one place but not the other, etc.). The proper fix would be having an unique place to generate the keys.

I see a few approaches:

• Distribute the key. We could have a central place to generate the key and distribute it via props, context, etc. This can result in unnecessary re-renders and we already re-render more than we need. Not ideal, better to avoid.
• Distribute a key generator. Given the queryKey is a derived value, the most promising approach would be to distribute a "key generator" that takes the variable state and returns the key. This key generator would be used in all places that need retrieving the queryKey.

function getQueryKey( dataViewsState, context ) {
  const { search, currentPage, perPage, filters, sort } = dataViewsState;
  const { currentPage } = context;

  // Get all other keys: filter, agencyId.

  return [
			'jetpack-agency-dashboard-sites',
				search,
				currentPage,
				filter,
				sort,
				perPage,
				...( agencyId ? [ agencyId ] : [] ),
			] ),
		]
}

With something along these lines, consumers wouldn't run in situations like I shared above: they either pass the proper parameters to the function or they don't. Whether we fallback to empty strings when the value is undefined doesn't really matter: all the places that use the key would use the same.

Looking up best practices in maintaining query keys, the approach I'm suggesting is in line with what the library authors promote as well. Though, they use a special package called "queryKey factory" which is too much for our use case.

[yashwin]

I was able to reproduce this issue by applying the diff.

While this will fix the issue with the monitor. I agree there could be scenarios that might not work well.

In my view, the core issue is that query keys are generated in different places and each place may mis-introduce data (empty string vs undefined, missing a key, stale data in one place but not the other, etc.). The proper fix would be having an unique place to generate the keys.

Distribute a key generator

I think this approach will work well, as you suggested. We are following a similar approach at a couple of places already, but this would be slight different considering the params.

Thanks!

[cleacos]

Hey André, thank you for jumping into this PR.

So, essentially, the fix is to provide an empty string if a parameter is undefined?

Yes, that code line fixes the Favorite-star issue with the search query parameter with null or undefined values. But also for other potential parameters, present and future ones.

This PR mainly fixes two issues (not directly related to DataViews at all):

• Parameters with undefined/null values: After setting the parameter values for the query cache, if any part of the code we want to access to a specific query cache, and one of those parameters is set to undefined or null, it won't get the correct cache data. That's the reason it was failing with the search parameter. (finding cache issues is not as easy task). This fixes this potential issue for any parameter and for all React Query use cases inside of A4A that use the query cache, not only for sites.
• There are two Monitor toggle issues, and this PR fixes both. However, applying only one of them will not fix the Monitor toggle issue.
  • An 'undefined' parameter. The hasher helper function fixes these cases for any parameter.
  • The parameter showOnlyDevelopmentSites is missing. If this parameter is not added as part of the query key, the code won't get the correct cache data, so it won't be able to find the site object and update the monitor status value.
  • Note that this issue is an old one and it's currently in production. (unrelated to the DataViews update project).
• Bonus improvement: The hash helper function prevents potential issues related to adding parameters in any order. So, to get the correct cache data for specific parameters, the queryKey doesn't need to follow a particular order, it will hash the query parameters in any order resulting the same hash value.

Additional test to be 100%

I rebased the branch update/dataviews-package-version onto this branch fix/a4a/favorite-and-monitor-toggle-refresh-isssue, and it works well, Monitor toggle works and the Favorite star works too. So also it fixes the Monitor issue.

[cleacos]

I didn't refresh the GH page, and I didn't see this comment.

The proper fix would be having an unique place to generate the keys.

Yep, a central key generator is ideal for the current live query, which mostly matches our use case, and also it's compatible with hashing the parameters, which is suitable for those uncovered rare cases if we want to retrieve cache data for a specific query, not the current one. Anyway, the "optimistic" solution requires invalidating the cache and retrieving the sites again; the reason is that other cache keys could have the same site that was updated in the other cache key.

[cleacos]

I'll go ahead and implement the central key generator, making it more generic.

[cleacos]

.

[cleacos]

I reverted some of the changes because they were in Jetpack Cloud. We don't have to change anything on that side.

[cleacos]

Also, I implemented the getQueryKey function with strict type, that way, we ensure that we have all the required parameters. If optional parameters are set in the useQuery cache and missed on other part, we will have the same issue. But that is how it works.

This fixes the Monitor toggle issue.

[oandregal]

Also, I implemented the getQueryKey function with strict type, that way, we ensure that we have all the required parameters. If optional parameters are set in the useQuery cache and missed on other part, we will have the same issue. But that is how it works.

Instead of doing:

getQueryKey( {
    search,
    currentPage,
    filter,
    sort,
    perPage,
    agencyId,
} ),

what if we do:

getQueryKey( dataViewsState, context, etc. );

I thought passing the necessary pieces state as a whole to the query key generator (as opposed to passing individual properties) would feel less fragile and prone to human errors.

[cleacos]

The problem is, for example, this code from the useToggleActivateMonitor modifies one of the parameters for some reason—I'm not sure why that specific get, it's a component from Jetpack Cloud.

issueTypes: getSelectedFilters( dataViewsState.filters ),

{
		searchQuery: dataViewsState.search,
		currentPage: dataViewsState.page,
		filter: {
			...filter,
			issueTypes: getSelectedFilters( dataViewsState.filters ),
		},
		sort: dataViewsState.sort,
		perPage: dataViewsState.perPage,
		agencyId,
}

The same happens in BoostLicenseInfoModal and useUpdateMonitorSetting (by mistake, I reverted some changes thinking that it was Jetpack Manage code that we were not using, but it's used).

In other case, we could use just this way:

getQueryKey( { ...dataViewsState, agencyId, } );

But we have the field name "issue". Yes, we could map it in the getQueryKey as a temporary solution.

Side note: The primary root issue concerns a mix of data from the context and state and how the data flows through them. For instance, DataViewsState has the page field, but the context also has it in currentPage. Both values should be the same, so we don't have to pass the context by parameter. Also, some field names don't match, like the currentPage vs page and the searchQuery vs search. For the right solution, we should fix it and, as always, ensure that it's working well for any part of the code that uses useFetchDashboardSites, but these minor issues are not a priority now. (cc: @vitozev )

At this point, we cannot control all the cases perfectly. But using types is a good improvement to ensure there are no missing fields in advance, which is a potential issue (that has already happened in the past, with the Favorite star stopping working). So, if we add a new filter in the future, the linter should complain about those required fields.

[cleacos]

I've talked with Kristian, and after finishing other priority tasks, we will address that possible update of the field renaming to be able to use the dataViewsState directly and other improvements in another PR. It will require exploration to check to do it well (without mapping and checking if getSelectedFilters is necessary) and to ensure we won't break anything.

I'll finish this PR, fixing the failing test and restoring the commits I reverted by mistake.