Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Restricting fetchModuleList calls to Atomic sites only #91170

Merged
merged 5 commits into from
May 29, 2024
Merged

Restricting fetchModuleList calls to Atomic sites only #91170

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
10eefe7
Making fetchModuleList calls only for Atomic sites
paulopmt1 May 27, 2024
57a7ce9
Test added to ensure we're calling fetchModuleList only for Atomic sites
paulopmt1 May 27, 2024
69c9c80
Improved test readability
paulopmt1 May 27, 2024
2e34bb3
Supporting Jetpack non-atomic sites as well
paulopmt1 May 28, 2024
ef93cca
Updated test scenario
paulopmt1 May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion client/components/data/query-jetpack-modules/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ import { useDispatch } from 'react-redux';
import { fetchModuleList } from 'calypso/state/jetpack/modules/actions';
import isFetchingJetpackModules from 'calypso/state/selectors/is-fetching-jetpack-modules';
import isSiteWpcomAtomic from 'calypso/state/selectors/is-site-wpcom-atomic';
import { isJetpackSite } from 'calypso/state/sites/selectors';

const request = ( siteId ) => ( dispatch, getState ) => {
const isJetpack = isJetpackSite( getState(), siteId );
const isAtomic = isSiteWpcomAtomic( getState(), siteId );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about non-atomic Jetpack sites? From what I recall, Jetpack sites (which aren't necessarily Atomic sites) also used the module data - extensively in site settings (but not only there).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @tyxla! Adding support to Jetpack non-Atomic sites here: 2e34bb3


if ( siteId && ! isFetchingJetpackModules( getState(), siteId ) && isAtomic ) {
if ( siteId && ! isFetchingJetpackModules( getState(), siteId ) && ( isAtomic || isJetpack ) ) {
dispatch( fetchModuleList( siteId ) );
}
};
Expand Down
21 changes: 20 additions & 1 deletion client/components/data/query-jetpack-modules/test/query-jetpack-modules.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ jest.mock( 'calypso/state/jetpack/modules/actions', () => ( {
} ) );

let isWpcomAtomic = false;
let isJetpack = false;
const siteId = 1;
const mockStore = configureStore( middlewares );

Expand All @@ -36,6 +37,7 @@ function getStore() {
options: {
is_wpcom_atomic: isWpcomAtomic,
},
jetpack: isJetpack,
},
},
},
Expand All @@ -49,6 +51,9 @@ function getStore() {

describe( 'fetchModuleList', () => {
test( "Ensure we're NOT calling fetchModuleList for simple sites", async () => {
isWpcomAtomic = false;
isJetpack = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we always declare these in the beginning of the test, we might as well remove the let call and move them here as consts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I understood your suggestion since we're using these variables inside mockStore, and if we declare them constants, we can't change them on other tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that makes sense. Let's leave them as they are then 👍


render(
<Provider store={ getStore() }>
<QueryJetpackModules siteId={ siteId } />
Expand All @@ -58,8 +63,9 @@ describe( 'fetchModuleList', () => {
expect( mockFetchModuleList ).not.toHaveBeenCalled();
} );

test( "Ensure we're calling fetchModuleList only for atomic sites", async () => {
test( "Ensure we're calling fetchModuleList for Atomic sites", async () => {
isWpcomAtomic = true;
isJetpack = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't seem like a valid scenario. AFAIK there can't be a working Atomic site that isn't a Jetpack site under the hood.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, updated it here @tyxla.


render(
<Provider store={ getStore() }>
Expand All @@ -69,4 +75,17 @@ describe( 'fetchModuleList', () => {

expect( mockFetchModuleList ).toHaveBeenCalled();
} );

test( "Ensure we're calling fetchModuleList for Jetpack Non Atomic sites", async () => {
isWpcomAtomic = false;
isJetpack = true;

render(
<Provider store={ getStore() }>
<QueryJetpackModules siteId={ siteId } />
</Provider>
);

expect( mockFetchModuleList ).toHaveBeenCalledTimes( 2 );
} );
} );
Loading