Implemented api-queries and DataView actions #105595
Conversation
matticbot
added
the
[Status] Needs Review
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
| plugins: SitePlugin[]; | ||
| }; | ||
|
|
||
| export type SitePlugin = { |
There was a problem hiding this comment.
Is this a different structure than the existing PluginItem? I'm guessing these types should be abstracted/shared on some level?
export interface PluginItem {
slug: string;
active: boolean;
id: string;
name: string;
plugin_url: string;
version: string;
description: string;
author: string;
author_url: string;
network: boolean;
autoupdate?: boolean;
update: PluginUpdate;
uninstallable?: boolean;
is_managed?: boolean;
action_links?: Record< string, string >;
}
We will need the is_managed?: boolean; property be added for the scheduled updates, but wondering why we have two types that are nearly identical.
| /> | ||
| ), | ||
| isEligible: ( item: PluginListRow ) => { | ||
| return item.isActive === 'none'; |
There was a problem hiding this comment.
I tried deactivating a plugin first to then delete it, but I don't see the option:
There was a problem hiding this comment.
All the plugins need to be deactivated before you can delete the plugin. From your action list you can see that some aren't deactivated since the deactivate action is available.
| import type { PluginListRow } from '../types'; | ||
| import type { PluginItem, PluginsResponse } from '@automattic/api-core'; | ||
|
|
||
| function toTriState( count: number, total: number ): 'all' | 'some' | 'none' { |
There was a problem hiding this comment.
I find the function name a bit confusing, maybe something like countToQuantifier would be more easily understandable. Wdyt?
| return []; | ||
| } | ||
| const sites = response.sites; | ||
| type Aggregated = { |
There was a problem hiding this comment.
Not sure it makes any difference, but I'd move the type outside the function.
There was a problem hiding this comment.
It doesn't make a difference from a compiled point of view. I'll move it outside anyways for clarity.
| errorCount: number; | ||
| }; | ||
|
|
||
| export const runPerSitePlugin = async ( |
There was a problem hiding this comment.
I think this way of manually driving a chain of mutations isn't quite right. Yes we can manually call onSuccess, but what about onSettled, the retry option, etc., etc.
In order to get the magic that TanStack Query provides, the mutation options need to be called along with the useMutation hook. If you just want to treat the actions as regular promises, then I think it makes sense to simply bypass the mutation mechanism all together. Call the api-core functions directly and use Promise.allSettled to wait for them all to finish. You could then wrap it up with a final call to queryClient.invalidateQueries.
If you do want to use the TanStack magic (retries, error handling, etc.) then useMutation will need to be called for each of the different types of mutation. I think it could look something like this:
const { mutateAsync: activatePlugin } = useMutation( sitePluginActivateMutation() );
const { mutateAsync: deactivatePlugin } = useMutation( sitePluginDeactivateMutation() );
// ... there needs to be a `useMutation` for each of these
const tasks = siteIds.map( ( siteId ) => {
switch ( action ) {
case 'activate':
return activatePlugin( siteId );
case 'deactivate':
return deactivatePlugin( siteId );
// ... and so on
}
} );Note that I've used the mutateAsync return value from useMutation, since this is how you get a function that returns a promise. And also I've moved the site ID from being a mutation option parameter, to a mutationFn parameter. Since we can't know when the hook is called what the actual site IDs will be.
If you follow the previous suggestion of merging some of the api-core and api-queries functions together, the number of hooks and switch cases should be smaller too.
There was a problem hiding this comment.
I'll refactor the code to use useMutation.
Part of:
Solves:
Proposed Changes
Known issues:
manage-plugins.mp4
Why are these changes being made?
Testing Instructions
Pre-merge Checklist