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

Fix flickering in Available Software Updates panel #2789

Merged
merged 2 commits into from
Jul 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,15 @@ const containerClassNames = classNames(
function AvailableSoftwareUpdates({
className,
settingsConfigured = false,
softwareUpdatesSettingsLoading,
softwareUpdatesLoading,
loading,
relevantPatches,
upgradablePackages,
tooltip,
onBackToSettings = noop,
onNavigateToPatches = noop,
onNavigateToPackages = noop,
}) {
if (softwareUpdatesSettingsLoading) {
if (loading) {
return <Loading className={containerClassNames} />;
}

Expand Down Expand Up @@ -60,7 +59,6 @@ function AvailableSoftwareUpdates({
title="Relevant Patches"
critical={gt(relevantPatches, 0)}
tooltip={tooltip}
loading={softwareUpdatesLoading}
icon={<EOS_HEALING size="xl" />}
onNavigate={onNavigateToPatches}
>
Expand All @@ -70,7 +68,6 @@ function AvailableSoftwareUpdates({
<Indicator
title="Upgradable Packages"
tooltip={tooltip}
loading={softwareUpdatesLoading}
icon={<EOS_PACKAGE_UPGRADE_OUTLINED size="xl" />}
onNavigate={onNavigateToPackages}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,24 +54,11 @@ describe('AvailableSoftwareUpdates component', () => {
});

it('renders Software Updates Settings Loading status', () => {
render(
<AvailableSoftwareUpdates
settingsConfigured
softwareUpdatesSettingsLoading
/>
);
render(<AvailableSoftwareUpdates settingsConfigured loading />);

expect(screen.getAllByLabelText('Loading')).toHaveLength(1);
});

it('renders Software Updates Loading status', () => {
render(
<AvailableSoftwareUpdates settingsConfigured softwareUpdatesLoading />
);

expect(screen.getAllByText('Loading...')).toHaveLength(2);
});

it('renders "SUSE Manager is not configured"', () => {
render(<AvailableSoftwareUpdates settingsConfigured={false} />);

Expand Down
4 changes: 1 addition & 3 deletions assets/js/pages/HostDetailsPage/HostDetails.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ function HostDetails({
upgradablePackages,
softwareUpdatesLoading,
softwareUpdatesSettingsSaved,
softwareUpdatesSettingsLoading,
softwareUpdatesTooltip,
userAbilities,
cleanUpHost,
Expand Down Expand Up @@ -245,8 +244,7 @@ function HostDetails({
relevantPatches={relevantPatches}
upgradablePackages={upgradablePackages}
tooltip={softwareUpdatesTooltip}
softwareUpdatesSettingsLoading={softwareUpdatesSettingsLoading}
softwareUpdatesLoading={softwareUpdatesLoading}
loading={softwareUpdatesLoading}
onBackToSettings={() => navigate(`/settings`)}
onNavigateToPatches={() => navigate(`/hosts/${hostID}/patches`)}
onNavigateToPackages={() => navigate(`/hosts/${hostID}/packages`)}
Expand Down
47 changes: 2 additions & 45 deletions assets/js/pages/HostDetailsPage/HostDetails.test.jsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react';
import { screen, within } from '@testing-library/react';
import { screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import 'intersection-observer';
import '@testing-library/jest-dom';
Expand Down Expand Up @@ -310,50 +310,7 @@ describe('HostDetails component', () => {
/>
);

const relevantPatchesElement = screen
.getByText(/Relevant Patches/)
.closest('div');
const upgradablePackagesElement = screen
.getByText(/Upgradable Packages/)
.closest('div');

expect(
within(relevantPatchesElement).getByText('Loading...')
).toBeVisible();
expect(
within(upgradablePackagesElement).getByText('Loading...')
).toBeVisible();
});

it('should a SUMA software updates when a connection error occurred', () => {
const relevantPatches = faker.number.int(100);
const upgradablePackages = faker.number.int(100);

renderWithRouter(
<HostDetails
agentVersion="2.0.0"
suseManagerEnabled
softwareUpdatesSettingsSaved
softwareUpdatesLoading
relevantPatches={relevantPatches}
upgradablePackages={upgradablePackages}
userAbilities={userAbilities}
/>
);

const relevantPatchesElement = screen
.getByText(/Relevant Patches/)
.closest('div');
const upgradablePackagesElement = screen
.getByText(/Upgradable Packages/)
.closest('div');

expect(
within(relevantPatchesElement).getByText('Loading...')
).toBeVisible();
expect(
within(upgradablePackagesElement).getByText('Loading...')
).toBeVisible();
expect(screen.getAllByLabelText('Loading')).toHaveLength(1);
});
});

Expand Down
20 changes: 6 additions & 14 deletions assets/js/pages/HostDetailsPage/HostDetailsPage.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,7 @@ import { getInstancesOnHost } from '@state/selectors/sapSystem';
import { getCatalog } from '@state/selectors/catalog';
import { getLastExecution } from '@state/selectors/lastExecutions';
import {
getSoftwareUpdatesSettingsLoading,
getSoftwareUpdatesSettingsSaved,
} from '@state/selectors/softwareUpdatesSettings';
import {
getSoftwareUpdatesSettingsConfigured,
getSoftwareUpdatesLoading,
getSoftwareUpdatesStats,
} from '@state/selectors/softwareUpdates';
Expand All @@ -31,7 +28,6 @@ import {
} from '@state/lastExecutions';

import { deregisterHost } from '@state/hosts';
import { fetchSoftwareUpdatesSettings } from '@state/softwareUpdatesSettings';
import { fetchSoftwareUpdates } from '@state/softwareUpdates';
import HostDetails from './HostDetails';

Expand Down Expand Up @@ -60,12 +56,10 @@ function HostDetailsPage() {

const [exportersStatus, setExportersStatus] = useState([]);

const softwareUpdatesSettingsLoading = useSelector((state) =>
getSoftwareUpdatesSettingsLoading(state)
);
const softwareUpdatesConnectionSaved = useSelector((state) =>
getSoftwareUpdatesSettingsSaved(state)
const settingsConfigured = useSelector((state) =>
getSoftwareUpdatesSettingsConfigured(state)
);

const { numRelevantPatches, numUpgradablePackages } = useSelector((state) =>
getSoftwareUpdatesStats(state, hostID)
);
Expand All @@ -90,11 +84,10 @@ function HostDetailsPage() {
);

useEffect(() => {
dispatch(fetchSoftwareUpdates(hostID));
getExportersStatus();
refreshCatalog();
dispatch(updateLastExecution(hostID));
dispatch(fetchSoftwareUpdates(hostID));
dispatch(fetchSoftwareUpdatesSettings());
}, []);

if (!host) {
Expand Down Expand Up @@ -125,8 +118,7 @@ function HostDetailsPage() {
suseManagerEnabled={suseManagerEnabled}
relevantPatches={numRelevantPatches}
upgradablePackages={numUpgradablePackages}
softwareUpdatesSettingsSaved={softwareUpdatesConnectionSaved}
softwareUpdatesSettingsLoading={softwareUpdatesSettingsLoading}
softwareUpdatesSettingsSaved={settingsConfigured}
softwareUpdatesLoading={softwareUpdatesLoading}
softwareUpdatesTooltip={
numRelevantPatches === undefined && numUpgradablePackages === undefined
Expand Down
24 changes: 23 additions & 1 deletion assets/js/state/sagas/softwareUpdates.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import {
import {
FETCH_UPGRADABLE_PACKAGES_PATCHES,
FETCH_SOFTWARE_UPDATES,
setSettingsConfigured,
setSettingsNotConfigured,
startLoadingSoftwareUpdates,
setSoftwareUpdates,
setEmptySoftwareUpdates,
Expand All @@ -21,10 +23,20 @@ export function* fetchSoftwareUpdates({ payload: hostID }) {
try {
const response = yield call(getSoftwareUpdates, hostID);
yield put(setSoftwareUpdates({ hostID, ...response.data }));
yield put(setSettingsConfigured());
} catch (error) {
yield put(setEmptySoftwareUpdates({ hostID }));

const errors = get(error, ['response', 'data'], []);
const errorCode = get(error, ['response', 'status']);
const { errors } = get(error, ['response', 'data'], []);
const suma_unauthorized = errors.some(
({ detail }) => detail === 'SUSE Manager authentication error.'
);

if (errorCode === 422 && suma_unauthorized) {
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 check the status code here for completeness, though technically we may not need to.

yield put(setSettingsNotConfigured());
}

yield put(setSoftwareUpdatesErrors({ hostID, errors }));
}
}
Expand All @@ -37,7 +49,17 @@ export function* fetchUpgradablePackagesPatches({
data: { patches },
} = yield call(getPatchesForPackages, packageIDs);
yield put(setPatchesForPackages({ hostID, patches }));
yield put(setSettingsConfigured());
} catch (error) {
const errorCode = get(error, ['response', 'status']);
const { errors } = get(error, ['response', 'data'], []);
const suma_unauthorized = errors.some(
({ detail }) => detail === 'SUSE Manager authentication error.'
);

if (errorCode === 422 && suma_unauthorized) {
yield put(setSettingsNotConfigured());
}
yield put(setPatchesForPackages({ hostID, patches: [] }));
}
}
Expand Down
85 changes: 82 additions & 3 deletions assets/js/state/sagas/softwareUpdates.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import { patchForPackageFactory } from '@lib/test-utils/factories';
import { networkClient } from '@lib/network';

import {
setSettingsConfigured,
setSettingsNotConfigured,
startLoadingSoftwareUpdates,
setSoftwareUpdates,
setSoftwareUpdatesErrors,
Expand Down Expand Up @@ -64,12 +66,30 @@ describe('Software Updates saga', () => {
expect(dispatched).toEqual([
startLoadingSoftwareUpdates({ hostID }),
setSoftwareUpdates({ hostID, ...response }),
setSettingsConfigured(),
]);
});

it.each([
{ status: 404, body: { message: '404 Not found' } },
{ status: 500, body: { message: 'java.lang.NullPointerException' } },
{
status: 404,
body: {
errors: [
{
title: 'Not Found',
detail: 'The requested resource cannot be found.',
},
],
},
},
{
status: 500,
body: {
errors: [
{ title: 'Internal Server Error', detail: 'Something went wrong.' },
],
},
},
])(
'should empty software updates settings on failed fetching',
async ({ status, body }) => {
Expand All @@ -87,10 +107,39 @@ describe('Software Updates saga', () => {
expect(dispatched).toEqual([
startLoadingSoftwareUpdates({ hostID }),
setEmptySoftwareUpdates({ hostID }),
setSoftwareUpdatesErrors({ hostID, errors: body }),
setSoftwareUpdatesErrors({ hostID, errors: body.errors }),
]);
}
);

it('should set settings not configured when 422 with relevant error message', async () => {
const axiosMock = new MockAdapter(networkClient);
const hostID = faker.string.uuid();

const errorBody = {
errors: [
{
title: 'Unprocessable Entity',
detail: 'SUSE Manager authentication error.',
},
],
};

axiosMock
.onGet(`/api/v1/hosts/${hostID}/software_updates`)
.reply(422, errorBody);

const dispatched = await recordSaga(fetchSoftwareUpdates, {
payload: hostID,
});

expect(dispatched).toEqual([
startLoadingSoftwareUpdates({ hostID }),
setEmptySoftwareUpdates({ hostID }),
setSettingsNotConfigured(),
setSoftwareUpdatesErrors({ hostID, errors: errorBody.errors }),
]);
});
});

describe('Fetching patches for packages', () => {
Expand All @@ -114,6 +163,36 @@ describe('Software Updates saga', () => {

expect(dispatched).toEqual([
setPatchesForPackages({ hostID, patches: response.patches }),
setSettingsConfigured(),
]);
});

it('should set settings not configured when 422 with relevant error message', async () => {
const axiosMock = new MockAdapter(networkClient);
const hostID = faker.string.uuid();

const errorBody = {
errors: [
{
title: 'Unprocessable Entity',
detail: 'SUSE Manager authentication error.',
},
],
};

axiosMock
.onGet(`/api/v1/hosts/${hostID}/software_updates`)
.reply(422, errorBody);

const dispatched = await recordSaga(fetchSoftwareUpdates, {
payload: hostID,
});

expect(dispatched).toEqual([
startLoadingSoftwareUpdates({ hostID }),
setEmptySoftwareUpdates({ hostID }),
setSettingsNotConfigured(),
setSoftwareUpdatesErrors({ hostID, errors: errorBody.errors }),
]);
});
});
Expand Down
5 changes: 5 additions & 0 deletions assets/js/state/selectors/softwareUpdates.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ export const getSoftwareUpdatesStats = createSelector(
})
);

export const getSoftwareUpdatesSettingsConfigured = createSelector(
[(state) => getSoftwareUpdates(state)],
(softwareUpdates) => get(softwareUpdates, ['settingsConfigured'], false)
);

export const getSoftwareUpdatesPatches = createSelector(
[(state, id) => getSoftwareUpdatesForHost(id)(state)],
(softwareUpdates) => get(softwareUpdates, ['relevant_patches'], [])
Expand Down
Loading