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.

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

Display error message if update_merchant_product_statuses job throws an error #2324

Merged
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
73a242d
Allow to refresh Stats
jorgemd24 Mar 16, 2024
0530ff8
Add SyncMCStatus
jorgemd24 Mar 16, 2024
87ae0f2
Merge branch 'tweak/frontend-loading-product-statuses' into add/displ…
jorgemd24 Mar 17, 2024
3aa83ae
Merge branch 'tweak/frontend-loading-product-statuses' into add/displ…
jorgemd24 Mar 17, 2024
e23d7b6
Merge branch 'update/ui-loading-product-statuses' into add/display-er…
jorgemd24 Mar 18, 2024
989d225
Rename component to SyncProductStatistics
jorgemd24 Mar 19, 2024
f37e94e
Handle case where failure rate message is too high
jorgemd24 Mar 19, 2024
7b0f59e
Merge branch 'update/ui-loading-product-statuses' into add/display-er…
jorgemd24 Mar 19, 2024
dbf5507
Merge branch 'update/ui-loading-product-statuses' into add/display-er…
jorgemd24 Mar 19, 2024
3079d4a
Clear cache before job runs so we can delete any stale error
jorgemd24 Mar 19, 2024
762937b
Add tests for clear cache before schedule a job
jorgemd24 Mar 19, 2024
7f3eba4
Add the description for SyncProductStatistics
jorgemd24 Mar 19, 2024
1146e15
Pass the error to SyncProductStatistics
jorgemd24 Mar 19, 2024
24af8c2
Add tests When there is an error
jorgemd24 Mar 19, 2024
c213955
Js linting
jorgemd24 Mar 19, 2024
065ff6d
Add tests for refreshStats
jorgemd24 Mar 19, 2024
6b0b4f9
Delete MC statuses
jorgemd24 Mar 20, 2024
6a33e75
Add tests for get_failure_rate_message
jorgemd24 Mar 20, 2024
ef985e8
Tweak response of failure rate and docs
jorgemd24 Mar 20, 2024
0d0b1c1
Add tests for get statistics with failure rate
jorgemd24 Mar 20, 2024
15df732
Add delete_stale_mc_statuses method
jorgemd24 Mar 21, 2024
7a055ce
Add tests for delete_stale_mc_statuses
jorgemd24 Mar 21, 2024
68e6df3
Update transient time
jorgemd24 Mar 21, 2024
e032768
Adjust tests for clear_product_statuses_cache_and_issues
jorgemd24 Mar 21, 2024
8ec00b8
Add missing since
jorgemd24 Mar 22, 2024
345d019
Add useCallback to refresh product stats
jorgemd24 Mar 22, 2024
4aa6426
Use AppButton instead of Button
jorgemd24 Mar 22, 2024
986c6d2
Change error message for the failure rate msg
jorgemd24 Mar 22, 2024
7adeb24
Update tests
jorgemd24 Mar 22, 2024
cbdcba1
Merge pull request #2329 from woocommerce/add/delete-stale-mc-statuses
jorgemd24 Mar 25, 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
12 changes: 12 additions & 0 deletions js/src/hooks/useMCProductStatistics.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { useEffect } from '@wordpress/element';
*/
import useAppSelectDispatch from './useAppSelectDispatch';
import useCountdown from './useCountdown';
import useApiFetchCallback from '.~/hooks/useApiFetchCallback';
import { useAppDispatch } from '.~/data';

/**
Expand All @@ -27,6 +28,16 @@ const useMCProductStatistics = () => {
hasFinishedResolution && data?.loading ? true : false;
const hasStats = hasFinishedResolution && data?.statistics ? true : false;

const [ refreshProductStats ] = useApiFetchCallback( {
path: `/wc/gla/mc/product-statistics/refresh`,
method: 'GET',
} );

const refreshStats = async () => {
await refreshProductStats();
invalidateResolution();
};
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could wrap this function by useCallback, as there are dependencies used in the function. But I'm not sure if it is a best practice tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ianlin for the suggestion, I believe it won't make much difference in terms of performance, but it won't hurt either, so I've added it here: 345d019


useEffect( () => {
// If the job is still processing the data, start the countdown.
if ( isCalculatingStats && second === 0 ) {
Expand Down Expand Up @@ -57,6 +68,7 @@ const useMCProductStatistics = () => {
data,
invalidateResolution,
hasFinishedResolution,
refreshStats,
...rest,
};
};
Expand Down
41 changes: 41 additions & 0 deletions js/src/hooks/useMCProductStatistics.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,17 @@ import useMCProductStatistics from '.~/hooks/useMCProductStatistics';
import useCountdown from '.~/hooks/useCountdown';
import useAppSelectDispatch from '.~/hooks/useAppSelectDispatch';
import { useAppDispatch } from '.~/data';
import useApiFetchCallback from '.~/hooks/useApiFetchCallback';

jest.mock( '.~/hooks/useAppSelectDispatch' );
jest.mock( '.~/hooks/useCountdown' );
jest.mock( '.~/data' );
jest.mock( '.~/hooks/useApiFetchCallback', () => ( {
__esModule: true,
default: jest.fn().mockImplementation( () => {
return [ jest.fn(), null ];
} ),
} ) );

describe( 'useMCProductStatistics', () => {
const startCountdown = jest.fn();
Expand Down Expand Up @@ -139,4 +146,38 @@ describe( 'useMCProductStatistics', () => {
);
} );
} );
describe( 'Refresh the product statistics', () => {
beforeAll( () => {
jest.clearAllMocks();
} );
it( 'The user clicks the refresh button', async () => {
const refreshProductStats = jest.fn();
useCountdown.mockImplementation( () => {
return {
second: 0,
callCount: 0,
startCountdown,
};
} );

useAppSelectDispatch.mockImplementation( () => {
return {
hasFinishedResolution: true,
invalidateResolution,
data: statsData,
};
} );

useApiFetchCallback.mockImplementation( () => {
return [ refreshProductStats, null ];
} );

const { result } = renderHook( () => useMCProductStatistics() );

await result.current.refreshStats();

expect( refreshProductStats ).toHaveBeenCalledTimes( 1 );
expect( invalidateResolution ).toHaveBeenCalledTimes( 1 );
} );
} );
} );
10 changes: 9 additions & 1 deletion js/src/product-feed/product-statistics/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,16 @@ import {
import useMCProductStatistics from '.~/hooks/useMCProductStatistics';
import ProductStatusHelpPopover from './product-status-help-popover';
import SyncStatus from '.~/product-feed/product-statistics/status-box/sync-status';
import SyncProductStatistics from '.~/product-feed/product-statistics/status-box/sync-product-statistics';
import FeedStatus from '.~/product-feed/product-statistics/status-box/feed-status';
import AccountStatus from '.~/product-feed/product-statistics/status-box/account-status';
import Text from '.~/components/app-text';
import AppSpinner from '.~/components/app-spinner';
import './index.scss';

const ProductStatistics = () => {
const { hasFinishedResolution, data } = useMCProductStatistics();
const { hasFinishedResolution, data, refreshStats } =
useMCProductStatistics();

if ( hasFinishedResolution && ! data ) {
return __(
Expand Down Expand Up @@ -135,6 +137,12 @@ const ProductStatistics = () => {
<FeedStatus />
<SyncStatus />
<AccountStatus />
{ hasFinishedResolution && data?.error && (
<SyncProductStatistics
refreshStats={ refreshStats }
error={ data.error }
/>
) }
</CardFooter>
</Card>
);
Expand Down
6 changes: 6 additions & 0 deletions js/src/product-feed/product-statistics/index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -93,5 +93,11 @@
.gridicons-sync {
transform: rotateZ(90deg);
}

.overview-stats-error-button {
height: auto;
padding-top: 0;
padding-left: 0;
}
}
}
29 changes: 29 additions & 0 deletions js/src/product-feed/product-statistics/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,33 @@ describe( 'Product Statistics', () => {
} );
} );
} );
describe( `When there is an error`, () => {
it( 'Should render the error', () => {
useMCProductStatistics.mockImplementation( () => {
return {
hasFinishedResolution: true,
data: {
loading: false,
statistics: null,
error: 'Error loading stats!',
},
};
} );

const { getByText, container } = render( <ProductStatistics /> );

const notAvailableStats = container.querySelectorAll(
'.woocommerce-summary__item-value'
);

expect( notAvailableStats.length ).toBe( 5 );

for ( let i = 0; i < notAvailableStats.length; i++ ) {
expect( notAvailableStats[ i ].textContent ).toBe( 'N/A' );
}

expect( getByText( 'Overview Stats:' ) ).toBeInTheDocument();
expect( getByText( 'Error loading stats!' ) ).toBeInTheDocument();
} );
} );
} );
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/**
* External dependencies
*/
import { __ } from '@wordpress/i18n';
import { Button } from '@wordpress/components';

/**
* Internal dependencies
*/
import Status from '.~/product-feed/product-statistics/status-box/status';
import ErrorIcon from '.~/components/error-icon';

/**
* @typedef {import('.~/data/actions').ProductStatistics } ProductStatistics
*/

/**
* Renders status information for the Product Sync
*
* @param {Object} props The component props
* @param {Function} props.refreshStats
* @param {string} props.error
* @return {JSX.Element} The status for the Product Sync
*/
function SyncProductStatistics( { refreshStats, error } ) {
return (
<Status
title={ __( 'Overview Stats:', 'google-listings-and-ads' ) }
icon={ <ErrorIcon size={ 24 } /> }
label={
<Button
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 using AppButton so we can also add the eventName to it for tracking.

aria-label={ error }
onClick={ refreshStats }
className="overview-stats-error-button"
>
{ __(
'There was an error loading the Overview Stats. Click to retry.',
'google-listings-and-ads'
) }
</Button>
}
description={ error }
/>
);
}

export default SyncProductStatistics;
13 changes: 13 additions & 0 deletions src/Jobs/UpdateMerchantProductStatuses.php
Original file line number Diff line number Diff line change
Expand Up @@ -128,4 +128,17 @@ public function is_scheduled(): bool {
// We set 'args' to null so it matches any arguments. This is because it's possible to have multiple instances of the job running with different page tokens
return $this->is_running( null );
}

/**
* Validate the failure rate of the job.
*
* @return string|void Returns an error message if the failure rate is too high, otherwise null.
*/
public function get_failure_rate_message() {
try {
$this->monitor->validate_failure_rate( $this, $this->get_process_item_hook() );
} catch ( JobException $e ) {
return $e->getMessage();
}
}
}
15 changes: 14 additions & 1 deletion src/MerchantCenter/MerchantStatuses.php
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,19 @@ public function __construct() {
*/
public function get_product_statistics( bool $force_refresh = false ): array {
$job = $this->maybe_refresh_status_data( $force_refresh );
$failure_rate_msg = $job->get_failure_rate_message();
$this->mc_statuses = $this->container->get( TransientsInterface::class )->get( Transients::MC_STATUSES );

// If the failure rate is too high, return an error message so the UI can stop polling.
if ( $failure_rate_msg && null === $this->mc_statuses ) {
return [
'timestamp' => $this->cache_created_time->getTimestamp(),
'statistics' => null,
'loading' => false,
'error' => $failure_rate_msg,
];
}

if ( $job->is_scheduled() || null === $this->mc_statuses ) {
return [
'timestamp' => $this->cache_created_time->getTimestamp(),
Expand Down Expand Up @@ -278,7 +289,9 @@ public function maybe_refresh_status_data( bool $force_refresh = false ): Update

// If force_refresh is true or if not transient, return empty array and scheduled the job to update the statuses.
if ( ! $job->is_scheduled() && ( $force_refresh || ( ! $force_refresh && null === $this->mc_statuses ) ) ) {
// Schedule job to update the statuses.
// Delete the transient before scheduling the job because some errors, like the failure rate message, can occur before the job is executed.
$this->clear_cache();
// Schedule job to update the statuses. If the failure rate is too high, the job will not be scheduled.
$job->schedule();
}

Expand Down
19 changes: 19 additions & 0 deletions tests/Unit/Jobs/UpdateMerchantProductStatusesTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -248,4 +248,23 @@ public function test_update_merchant_product_statuses_when_view_report_throws_ex

do_action( self::PROCESS_ITEM_HOOK, [] );
}

public function test_get_failure_rate_message() {
$this->merchant_center_service->method( 'is_connected' )
->willReturn( true );

$this->monitor->expects( $this->exactly( 1 ) )->method( 'validate_failure_rate' )
->willThrowException( new JobException( 'The "update_merchant_product_statuses" job was stopped because its failure rate is above the allowed threshold.' ) );

$this->assertEquals( 'The "update_merchant_product_statuses" job was stopped because its failure rate is above the allowed threshold.', $this->job->get_failure_rate_message() );
}

public function test_get_with_no_failure_rate_message() {
$this->merchant_center_service->method( 'is_connected' )
->willReturn( true );

$this->monitor->expects( $this->exactly( 1 ) )->method( 'validate_failure_rate' );

$this->assertNull( $this->job->get_failure_rate_message() );
}
}
50 changes: 50 additions & 0 deletions tests/Unit/MerchantCenter/MerchantStatusesTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,18 @@ public function test_get_product_statistics_without_transient() {
$this->update_merchant_product_statuses_job->expects( $this->exactly( 1 ) )
->method( 'schedule' );

$this->update_all_product_job->expects( $this->exactly( 1 ) )
->method( 'can_schedule' )
->with( null )
->willReturn( true );

$this->delete_all_product_job->expects( $this->exactly( 1 ) )
->method( 'can_schedule' )
->with( null )
->willReturn( true );

$this->transients->expects( $this->exactly( 1 ) )->method( 'delete' )->with( Transients::MC_STATUSES );

$this->update_merchant_product_statuses_job->expects( $this->exactly( 2 ) )
->method( 'is_scheduled' );

Expand Down Expand Up @@ -432,6 +444,44 @@ public function test_get_product_statistics_with_force_refresh() {
);
}

public function test_get_product_statistics_with_failure_rate() {
$this->merchant_center_service->expects( $this->once() )
->method( 'is_connected' )
->willReturn( true );

$this->transients->expects( $this->exactly( 2 ) )
->method( 'get' )
->willReturn(
null
);

$this->update_merchant_product_statuses_job->expects( $this->exactly( 1 ) )
->method( 'get_failure_rate_message' )->willReturn( 'Failure rate!' );

$this->update_merchant_product_statuses_job->expects( $this->exactly( 1 ) )
->method( 'schedule' );

$this->update_merchant_product_statuses_job->expects( $this->exactly( 1 ) )
->method( 'is_scheduled' )->willReturn( false );

$product_statistics = $this->merchant_statuses->get_product_statistics();

$this->assertEquals(
null,
$product_statistics['statistics']
);

$this->assertEquals(
false,
$product_statistics['loading']
);

$this->assertEquals(
'Failure rate!',
$product_statistics['error']
);
}

public function test_process_product_statuses() {
$product_1 = WC_Helper_Product::create_simple_product();
$product_2 = WC_Helper_Product::create_simple_product();
Expand Down
Loading