From 15d774819c2631898ae8fa70d424dcd1c6dad480 Mon Sep 17 00:00:00 2001 From: Waheed Ahmed Date: Thu, 4 Feb 2021 20:10:10 +0500 Subject: [PATCH] Replaced all throw (e) calls with logError(). (#122) Also removed unused utility functions. VAN-49 --- .../__snapshots__/reduxUtils.test.js.snap | 3 - src/data/utils/dataUtils.js | 43 +-------- src/data/utils/dataUtils.test.js | 94 ++----------------- src/data/utils/index.js | 14 +-- src/data/utils/reduxUtils.js | 34 +------ src/data/utils/reduxUtils.test.js | 40 +------- src/data/utils/sagaUtils.js | 16 ---- src/data/utils/serviceUtils.js | 48 ---------- src/forgot-password/data/sagas.js | 2 + src/login/LoginFailure.jsx | 2 +- src/register/RegistrationPage.jsx | 2 +- src/reset-password/data/sagas.js | 3 + src/reset-password/data/tests/sagas.test.js | 7 ++ 13 files changed, 30 insertions(+), 278 deletions(-) delete mode 100644 src/data/utils/__snapshots__/reduxUtils.test.js.snap delete mode 100644 src/data/utils/sagaUtils.js delete mode 100644 src/data/utils/serviceUtils.js diff --git a/src/data/utils/__snapshots__/reduxUtils.test.js.snap b/src/data/utils/__snapshots__/reduxUtils.test.js.snap deleted file mode 100644 index 5571ec8f..00000000 --- a/src/data/utils/__snapshots__/reduxUtils.test.js.snap +++ /dev/null @@ -1,3 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`getModuleState should throw an exception on a bad path 1`] = `"Unexpected state key uhoh given to getModuleState. Is your state path set up correctly?"`; diff --git a/src/data/utils/dataUtils.js b/src/data/utils/dataUtils.js index 65693a7a..e94be8e9 100644 --- a/src/data/utils/dataUtils.js +++ b/src/data/utils/dataUtils.js @@ -1,46 +1,9 @@ -import camelCase from 'lodash.camelcase'; -import snakeCase from 'lodash.snakecase'; +// Utility functions -export function modifyObjectKeys(object, modify) { - // If the passed in object is not an object, return it. - if ( - object === undefined - || object === null - || (typeof object !== 'object' && !Array.isArray(object)) - ) { - return object; - } - - if (Array.isArray(object)) { - return object.map(value => modifyObjectKeys(value, modify)); - } - - // Otherwise, process all its keys. - const result = {}; - Object.entries(object).forEach(([key, value]) => { - result[modify(key)] = modifyObjectKeys(value, modify); - }); - return result; -} - -export function camelCaseObject(object) { - return modifyObjectKeys(object, camelCase); -} - -export function snakeCaseObject(object) { - return modifyObjectKeys(object, snakeCase); -} - -export function convertKeyNames(object, nameMap) { - const transformer = key => (nameMap[key] === undefined ? key : nameMap[key]); - - return modifyObjectKeys(object, transformer); -} - -export const processLink = (link) => { +export default function processLink(link) { let matches; link.replace(/(.*?)([^<]+)<\/a>(.*)/g, function () { // eslint-disable-line func-names matches = Array.prototype.slice.call(arguments, 1, 5); // eslint-disable-line prefer-rest-params }); return matches; -}; +} diff --git a/src/data/utils/dataUtils.test.js b/src/data/utils/dataUtils.test.js index dee7558d..d37e31f2 100644 --- a/src/data/utils/dataUtils.test.js +++ b/src/data/utils/dataUtils.test.js @@ -1,90 +1,14 @@ -import { - modifyObjectKeys, - camelCaseObject, - snakeCaseObject, - convertKeyNames, -} from './dataUtils'; +import processLink from './dataUtils'; -describe('modifyObjectKeys', () => { - it('should use the provided modify function to change all keys in and object and its children', () => { - function meowKeys(key) { - return `${key}Meow`; - } +describe('processLink', () => { + it('should use the provided processLink function to', () => { + const expectedHref = 'http://test.server.com/'; + const expectedText = 'test link'; + const link = `${expectedText}`; - const result = modifyObjectKeys( - { - one: undefined, - two: null, - three: '', - four: 0, - five: NaN, - six: [1, 2, { seven: 'woof' }], - eight: { nine: { ten: 'bark' }, eleven: true }, - }, - meowKeys, - ); + const matches = processLink(link); - expect(result).toEqual({ - oneMeow: undefined, - twoMeow: null, - threeMeow: '', - fourMeow: 0, - fiveMeow: NaN, - sixMeow: [1, 2, { sevenMeow: 'woof' }], - eightMeow: { nineMeow: { tenMeow: 'bark' }, elevenMeow: true }, - }); - }); -}); - -describe('camelCaseObject', () => { - it('should make everything camelCase', () => { - const result = camelCaseObject({ - what_now: 'brown cow', - but_who: { says_you_people: 'okay then', but_how: { will_we_even_know: 'the song is over' } }, - 'dot.dot.dot': 123, - }); - - expect(result).toEqual({ - whatNow: 'brown cow', - butWho: { saysYouPeople: 'okay then', butHow: { willWeEvenKnow: 'the song is over' } }, - dotDotDot: 123, - }); - }); -}); - -describe('snakeCaseObject', () => { - it('should make everything snake_case', () => { - const result = snakeCaseObject({ - whatNow: 'brown cow', - butWho: { saysYouPeople: 'okay then', butHow: { willWeEvenKnow: 'the song is over' } }, - 'dot.dot.dot': 123, - }); - - expect(result).toEqual({ - what_now: 'brown cow', - but_who: { says_you_people: 'okay then', but_how: { will_we_even_know: 'the song is over' } }, - dot_dot_dot: 123, - }); - }); -}); - -describe('convertKeyNames', () => { - it('should replace the specified keynames', () => { - const result = convertKeyNames( - { - one: { two: { three: 'four' } }, - five: 'six', - }, - { - two: 'blue', - five: 'alive', - seven: 'heaven', - }, - ); - - expect(result).toEqual({ - one: { blue: { three: 'four' } }, - alive: 'six', - }); + expect(matches[1]).toEqual(expectedHref); + expect(matches[2]).toEqual(expectedText); }); }); diff --git a/src/data/utils/index.js b/src/data/utils/index.js index e8c75a28..7bca6fc4 100644 --- a/src/data/utils/index.js +++ b/src/data/utils/index.js @@ -1,12 +1,2 @@ -export { - camelCaseObject, - convertKeyNames, - modifyObjectKeys, - snakeCaseObject, -} from './dataUtils'; -export { - AsyncActionType, - getModuleState, -} from './reduxUtils'; -export { default as handleFailure } from './sagaUtils'; -export { unpackFieldErrors, handleRequestError } from './serviceUtils'; +export { default } from './dataUtils'; +export { default as AsyncActionType } from './reduxUtils'; diff --git a/src/data/utils/reduxUtils.js b/src/data/utils/reduxUtils.js index 7cab9e91..45b0d762 100644 --- a/src/data/utils/reduxUtils.js +++ b/src/data/utils/reduxUtils.js @@ -2,7 +2,7 @@ * Helper class to save time when writing out action types for asynchronous methods. Also helps * ensure that actions are namespaced. */ -export class AsyncActionType { +export default class AsyncActionType { constructor(topic, name) { this.topic = topic; this.name = name; @@ -32,35 +32,3 @@ export class AsyncActionType { return `${this.topic}__${this.name}__FORBIDDEN`; } } - -/** - * Given a state tree and an array representing a set of keys to traverse in that tree, returns - * the portion of the tree at that key path. - * - * Example: - * - * const result = getModuleState( - * { - * first: { red: { awesome: 'sauce' }, blue: { weak: 'sauce' } }, - * second: { other: 'data', } - * }, - * ['first', 'red'] - * ); - * - * result will be: - * - * { - * awesome: 'sauce' - * } - */ -export function getModuleState(state, originalPath) { - const path = [...originalPath]; // don't modify your argument - if (path.length < 1) { - return state; - } - const key = path.shift(); - if (state[key] === undefined) { - throw new Error(`Unexpected state key ${key} given to getModuleState. Is your state path set up correctly?`); - } - return getModuleState(state[key], path); -} diff --git a/src/data/utils/reduxUtils.test.js b/src/data/utils/reduxUtils.test.js index e07adf45..3e5a3d80 100644 --- a/src/data/utils/reduxUtils.test.js +++ b/src/data/utils/reduxUtils.test.js @@ -1,7 +1,4 @@ -import { - AsyncActionType, - getModuleState, -} from './reduxUtils'; +import AsyncActionType from './reduxUtils'; describe('AsyncActionType', () => { it('should return well formatted action strings', () => { @@ -15,38 +12,3 @@ describe('AsyncActionType', () => { expect(actionType.FORBIDDEN).toBe('HOUSE_CATS__START_THE_RACE__FORBIDDEN'); }); }); - -describe('getModuleState', () => { - const state = { - first: { red: { awesome: 'sauce' }, blue: { weak: 'sauce' } }, - second: { other: 'data' }, - }; - - it('should return everything if given an empty path', () => { - expect(getModuleState(state, [])).toEqual(state); - }); - - it('should resolve paths correctly', () => { - expect(getModuleState( - state, - ['first'], - )).toEqual({ red: { awesome: 'sauce' }, blue: { weak: 'sauce' } }); - - expect(getModuleState( - state, - ['first', 'red'], - )).toEqual({ awesome: 'sauce' }); - - expect(getModuleState(state, ['second'])).toEqual({ other: 'data' }); - }); - - it('should throw an exception on a bad path', () => { - expect(() => { - getModuleState(state, ['uhoh']); - }).toThrowErrorMatchingSnapshot(); - }); - - it('should return non-objects correctly', () => { - expect(getModuleState(state, ['first', 'red', 'awesome'])).toEqual('sauce'); - }); -}); diff --git a/src/data/utils/sagaUtils.js b/src/data/utils/sagaUtils.js deleted file mode 100644 index 0ce3ecd0..00000000 --- a/src/data/utils/sagaUtils.js +++ /dev/null @@ -1,16 +0,0 @@ -import { put } from 'redux-saga/effects'; -import { logError } from '@edx/frontend-platform/logging'; -import { history } from '@edx/frontend-platform'; - -export default function* handleFailure(error, failureAction = null, failureRedirectPath = null) { - if (error.fieldErrors && failureAction !== null) { - yield put(failureAction({ fieldErrors: error.fieldErrors })); - } - logError(error); - if (failureAction !== null) { - yield put(failureAction(error.message)); - } - if (failureRedirectPath !== null) { - history.push(failureRedirectPath); - } -} diff --git a/src/data/utils/serviceUtils.js b/src/data/utils/serviceUtils.js deleted file mode 100644 index ea22c04c..00000000 --- a/src/data/utils/serviceUtils.js +++ /dev/null @@ -1,48 +0,0 @@ -/** - * Turns field errors of the form: - * - * { - * "name":{ - * "developer_message": "Nerdy message here", - * "user_message": "This value is invalid." - * }, - * "other_field": { - * "developer_message": "Other Nerdy message here", - * "user_message": "This other value is invalid." - * } - * } - * - * Into: - * - * { - * "name": "This value is invalid.", - * "other_field": "This other value is invalid" - * } - */ -export function unpackFieldErrors(fieldErrors) { - return Object.entries(fieldErrors).reduce((acc, [k, v]) => { - acc[k] = v.user_message; - return acc; - }, {}); -} - -/** - * Processes and re-throws request errors. If the response contains a field_errors field, will - * massage the data into a form expected by the client. - * - * Field errors will be packaged as an api error with a fieldErrors field usable by the client. - * Takes an optional unpack function which is used to process the field errors, - * otherwise uses the default unpackFieldErrors function. - * - * @param error The original error object. - * @param unpackFunction (Optional) A function to use to unpack the field errors as a replacement - * for the default. - */ -export function handleRequestError(error, unpackFunction = unpackFieldErrors) { - if (error.response && error.response.data.field_errors) { - const apiError = Object.create(error); - apiError.fieldErrors = unpackFunction(error.response.data.field_errors); - throw apiError; - } - throw error; -} diff --git a/src/forgot-password/data/sagas.js b/src/forgot-password/data/sagas.js index 636bb8c3..b9749c10 100644 --- a/src/forgot-password/data/sagas.js +++ b/src/forgot-password/data/sagas.js @@ -1,3 +1,4 @@ +import { logError } from '@edx/frontend-platform/logging'; import { call, put, takeEvery } from 'redux-saga/effects'; // Actions @@ -25,6 +26,7 @@ export function* handleForgotPassword(action) { } else { yield put(forgotPasswordServerError()); } + logError(e); } } diff --git a/src/login/LoginFailure.jsx b/src/login/LoginFailure.jsx index a3a2cc23..85d0eeba 100644 --- a/src/login/LoginFailure.jsx +++ b/src/login/LoginFailure.jsx @@ -4,7 +4,7 @@ import { FormattedMessage, injectIntl, intlShape } from '@edx/frontend-platform/ import { Alert } from '@edx/paragon'; import PropTypes from 'prop-types'; -import { processLink } from '../data/utils/dataUtils'; +import processLink from '../data/utils'; import { FORBIDDEN_REQUEST, INACTIVE_USER, diff --git a/src/register/RegistrationPage.jsx b/src/register/RegistrationPage.jsx index e52b6229..4b912184 100644 --- a/src/register/RegistrationPage.jsx +++ b/src/register/RegistrationPage.jsx @@ -42,7 +42,7 @@ import { REGISTRATION_EXTRA_FIELDS, } from '../data/constants'; import messages from './messages'; -import { processLink } from '../data/utils/dataUtils'; +import processLink from '../data/utils'; class RegistrationPage extends React.Component { constructor(props, context) { diff --git a/src/reset-password/data/sagas.js b/src/reset-password/data/sagas.js index 38debd34..53185963 100644 --- a/src/reset-password/data/sagas.js +++ b/src/reset-password/data/sagas.js @@ -1,4 +1,5 @@ import { call, put, takeEvery } from 'redux-saga/effects'; +import { logError } from '@edx/frontend-platform/logging'; // Actions import { @@ -27,6 +28,7 @@ export function* handleValidateToken(action) { } } catch (err) { yield put(validateTokenFailure(err)); + logError(err); } } @@ -44,6 +46,7 @@ export function* handleResetPassword(action) { } } catch (err) { yield put(resetPasswordFailure(err)); + logError(err); } } diff --git a/src/reset-password/data/tests/sagas.test.js b/src/reset-password/data/tests/sagas.test.js index 9541d8f6..7c49b780 100644 --- a/src/reset-password/data/tests/sagas.test.js +++ b/src/reset-password/data/tests/sagas.test.js @@ -7,6 +7,9 @@ import { } from '../actions'; import { handleResetPassword } from '../sagas'; import * as api from '../service'; +import initializeMockLogging from '../../../setupTest'; + +const { loggingService } = initializeMockLogging(); describe('handleResetPassword', () => { const params = { @@ -25,6 +28,10 @@ describe('handleResetPassword', () => { err_msg: '', }; + beforeEach(() => { + loggingService.logError.mockReset(); + }); + it('should call service and dispatch success action', async () => { const resetPassword = jest.spyOn(api, 'resetPassword') .mockImplementation(() => Promise.resolve(responseData));