diff --git a/assets/js/common/ActivityLogsSettingsModal/ActivityLogsSettingsModal.jsx b/assets/js/common/ActivityLogsSettingsModal/ActivityLogsSettingsModal.jsx index 2c646d372a..1784683e77 100644 --- a/assets/js/common/ActivityLogsSettingsModal/ActivityLogsSettingsModal.jsx +++ b/assets/js/common/ActivityLogsSettingsModal/ActivityLogsSettingsModal.jsx @@ -7,7 +7,7 @@ import { InputNumber } from '@common/Input'; import Select from '@common/Select'; import Label from '@common/Label'; -import { getError } from '@lib/api/validationErrors'; +import { getError, getGlobalError } from '@lib/api/validationErrors'; const defaultErrors = []; @@ -22,9 +22,7 @@ const toRetentionTimeErrorMessage = (errors) => .filter(Boolean) .join('; '); -const toGenericErrorMessage = (errors) => - // the first error of type string is considered the generic error - errors.find((error) => typeof error === 'string'); +const toGlobalErrorMessage = (errors) => capitalize(getGlobalError(errors)); function TimeSpan({ time: initialTime, error = false, onChange = noop }) { const [time, setTime] = useState(initialTime); @@ -90,7 +88,7 @@ function ActivityLogsSettingsModal({ const [retentionTime, setRetentionTime] = useState(initialRetentionTime); const retentionTimeError = toRetentionTimeErrorMessage(errors); - const genericError = toGenericErrorMessage(errors); + const genericError = toGlobalErrorMessage(errors); return ( faker.number.int({ min: 1 }); @@ -125,8 +126,8 @@ describe('ActivityLogsSettingsModal component', () => { ${'unit error'} | ${[unitError]} | ${unitError.detail} ${'value and unit errors (1)'} | ${[valueError, unitError]} | ${valueError.detail} ${'value and unit errors (2)'} | ${[valueError, unitError]} | ${unitError.detail} - ${'generic error'} | ${['a generic error']} | ${'a generic error'} - ${'generic error and value error (1)'} | ${['a generic error', valueError]} | ${'a generic error'} + ${'generic error'} | ${['a generic error']} | ${defaultGlobalError.detail} + ${'generic error and value error (1)'} | ${['a generic error', valueError]} | ${defaultGlobalError.detail} ${'generic error and value error (2)'} | ${['a generic error', valueError]} | ${valueError.detail} `( 'should display errors on $scenario', diff --git a/assets/js/lib/api/validationErrors.js b/assets/js/lib/api/validationErrors.js index de19dcebcf..c0eaefdf64 100644 --- a/assets/js/lib/api/validationErrors.js +++ b/assets/js/lib/api/validationErrors.js @@ -1,20 +1,32 @@ import { flow, first } from 'lodash'; -import { get, filter } from 'lodash/fp'; +import { get, filter, map } from 'lodash/fp'; -export const hasError = (keyword, errors) => - errors.some((error) => { - const pointer = get(['source', 'pointer'], error); +const selectField = (keyword) => (error) => { + const pointer = get(['source', 'pointer'], error); - return pointer === `/${keyword}`; - }); + return pointer === `/${keyword}`; +}; + +export const hasError = (keyword, errors) => errors.some(selectField(keyword)); export const getError = (keyword, errors) => - flow([ - filter((error) => { - const pointer = get(['source', 'pointer'], error); + flow([filter(selectField(keyword)), first, get('detail')])(errors); - return pointer === `/${keyword}`; - }), +export const defaultGlobalError = { + title: 'Unexpected error', + detail: 'Something went wrong.', +}; + +export const getGlobalError = (errors) => + flow([ + filter( + (error) => !(typeof error === 'object' && error && 'source' in error) + ), + map((error) => + typeof error === 'object' && error && 'detail' in error + ? error + : defaultGlobalError + ), first, get('detail'), ])(errors); diff --git a/assets/js/lib/api/validationErrors.test.js b/assets/js/lib/api/validationErrors.test.js index d197bb0d8d..0d7fce1ad6 100644 --- a/assets/js/lib/api/validationErrors.test.js +++ b/assets/js/lib/api/validationErrors.test.js @@ -1,4 +1,9 @@ -import { hasError, getError } from './validationErrors'; +import { + hasError, + getError, + getGlobalError, + defaultGlobalError, +} from './validationErrors'; describe('hasError', () => { it('should tell that a list contains an error about a specific field', () => { @@ -91,3 +96,58 @@ describe('getError', () => { expect(getError('url', errors)).toBe(undefined); }); }); + +describe('getGlobalError', () => { + it('should return the first global error', () => { + const errors = [ + { + detail: 'a detail', + title: 'a title', + }, + { + detail: 'another detail', + source: { pointer: '/some_field' }, + title: 'another title', + }, + { + detail: 'do not return this detail', + title: 'do not return this title', + }, + ]; + + expect(getGlobalError(errors)).toBe('a detail'); + }); + + it('should return undefined when there is no global error', () => { + const errors = [ + { + detail: "can't be blank", + source: { pointer: '/some_value' }, + title: 'Invalid value', + }, + ]; + + expect(getGlobalError(errors)).not.toBeDefined(); + }); + + it('should return undefined when no error', () => { + const errors = []; + + expect(getGlobalError(errors)).not.toBeDefined(); + }); + + it.each` + input + ${{ malformed: true }} + ${undefined} + ${null} + ${'string'} + ${1234 /* number */} + ${[12, 34] /* array */} + `( + 'should return the default error if the received error is malformed', + ({ input }) => { + expect(getGlobalError([input])).toBe(defaultGlobalError.detail); + } + ); +}); diff --git a/assets/js/state/sagas/activityLogsSettings.js b/assets/js/state/sagas/activityLogsSettings.js index 6b9e0d5e25..e7fc7aba83 100644 --- a/assets/js/state/sagas/activityLogsSettings.js +++ b/assets/js/state/sagas/activityLogsSettings.js @@ -11,6 +11,7 @@ import { setEditingActivityLogsSettings, setNetworkError, } from '@state/activityLogsSettings'; +import { defaultGlobalError } from '@lib/api/validationErrors'; export function* fetchActivityLogsSettings() { yield put(startLoadingActivityLogsSettings()); @@ -34,7 +35,7 @@ export function* updateActivityLogsSettings({ payload }) { const errors = get( error, ['response', 'data', 'errors'], - ['An error occurred while saving the settings'] + [defaultGlobalError] ); yield put(setActivityLogsSettingsErrors(errors)); } diff --git a/assets/js/state/sagas/activityLogsSettings.test.js b/assets/js/state/sagas/activityLogsSettings.test.js index 32dd163e05..05c8c82aa5 100644 --- a/assets/js/state/sagas/activityLogsSettings.test.js +++ b/assets/js/state/sagas/activityLogsSettings.test.js @@ -13,6 +13,7 @@ import { setNetworkError, } from '@state/activityLogsSettings'; +import { defaultGlobalError } from '@lib/api/validationErrors'; import { fetchActivityLogsSettings, updateActivityLogsSettings, @@ -93,6 +94,44 @@ describe('Activity Logs Settings saga', () => { ]); }); + it('should have generic errors on update (receiving empty body)', async () => { + const axiosMock = new MockAdapter(networkClient); + const payload = activityLogsSettingsFactory.build(); + + axiosMock.onPut('/settings/activity_log', payload).reply(500); + + const dispatched = await recordSaga(updateActivityLogsSettings, { + payload, + }); + + expect(dispatched).toEqual([ + startLoadingActivityLogsSettings(), + setActivityLogsSettingsErrors([defaultGlobalError]), + ]); + }); + + it('should have generic errors on update', async () => { + const axiosMock = new MockAdapter(networkClient); + const payload = activityLogsSettingsFactory.build(); + + axiosMock.onPut('/settings/activity_log', payload).reply(500, { + errors: [ + { title: 'Internal Server Error', detail: 'Something went wrong.' }, + ], + }); + + const dispatched = await recordSaga(updateActivityLogsSettings, { + payload, + }); + + expect(dispatched).toEqual([ + startLoadingActivityLogsSettings(), + setActivityLogsSettingsErrors([ + { title: 'Internal Server Error', detail: 'Something went wrong.' }, + ]), + ]); + }); + it.each([403, 404, 500, 502, 504])( 'should put a network error flag on failed saving', async (status) => { @@ -107,7 +146,7 @@ describe('Activity Logs Settings saga', () => { expect(dispatched).toEqual([ startLoadingActivityLogsSettings(), - setActivityLogsSettingsErrors([expect.any(String)]), + setActivityLogsSettingsErrors([defaultGlobalError]), ]); } );