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

code refator #80

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 1 addition & 3 deletions App.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ if (!window.Intl) {
})
.then(() => Promise.all([import('intl/locale-data/jsonp/en.js')]))
.then(() => AppRegistry.registerComponent(appName, () => App))
.catch(err => {
throw err;
});
.catch(alert);
himanshu-wedensday marked this conversation as resolved.
Show resolved Hide resolved
} else {
AppRegistry.registerComponent(appName, () => App);
}
Expand Down
4 changes: 2 additions & 2 deletions app/components/molecules/CharacterWithQuote/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from 'react';
import styled from 'styled-components/native';
import { get } from 'lodash';
import get from 'lodash/get';
import PropTypes from 'prop-types';
import { fonts } from '@themes';
import T from '@atoms/T';
Expand All @@ -23,7 +23,7 @@ function CharacterWithQuote({ user }) {
<Result
id="wednesday_lover"
values={{
username: get(user, 'character') || 'character'
username: get(user, 'character', 'character')
}}
/>
<Result id="because" />
Expand Down
23 changes: 13 additions & 10 deletions app/components/molecules/If/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,24 @@
*
*/
// eslint-disable-next-line
import React from 'react'
import Proptypes from 'prop-types';
import PropTypes from 'prop-types';
himanshu-wedensday marked this conversation as resolved.
Show resolved Hide resolved

const If = props => (props.condition ? props.children : props.otherwise);
If.propsTypes = {
condition: Proptypes.bool,
otherwise: Proptypes.oneOfType([
Proptypes.arrayOf(Proptypes.node),
Proptypes.node

If.propTypes = {
condition: PropTypes.bool,
otherwise: PropTypes.oneOfType([
PropTypes.arrayOf(PropTypes.node),
PropTypes.node
]),
children: Proptypes.oneOfType([
Proptypes.arrayOf(Proptypes.node),
Proptypes.node
children: PropTypes.oneOfType([
PropTypes.arrayOf(PropTypes.node),
PropTypes.node
])
};

If.defaultProps = {
otherwise: null
};

export default If;
4 changes: 2 additions & 2 deletions app/components/molecules/If/tests/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import React from 'react';
import { Text } from 'react-native';
import { render } from '@testing-library/react-native';

import set from 'lodash/set';
import If from '../index';

describe('<If />', () => {
Expand All @@ -32,7 +32,7 @@ describe('<If />', () => {
</If>
);
expect(getByText(conditionTrueText)).toBeTruthy();
props.condition = false;
set(props, 'condition', false);
const { getByText: textQueryOnReRender } = render(
<If {...props}>
<TrueConditionComponent />
Expand Down
4 changes: 2 additions & 2 deletions app/components/organisms/SimpsonsLoveWednesday/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import If from '@molecules/If';
import CharacterWithQuote from '@molecules/CharacterWithQuote';
import LogoWithInstructions from '@molecules/LogoWithInstructions';

const Error = styled.Text`
const Err = styled.Text`
${fonts.style.standard()};
text-align: center;
margin-bottom: 5px;
Expand All @@ -25,7 +25,7 @@ function SimpsonsLoveWednesday({ instructions, user, userErrorMessage }) {
<LogoWithInstructions instructions={instructions} />
<If
condition={!userErrorMessage}
otherwise={<Error>{userErrorMessage}</Error>}
otherwise={<Err>{userErrorMessage}</Err>}
>
<SeparatedView>
<CharacterWithQuote user={user} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
import React from 'react';
import { renderWithIntl } from '@utils/testUtils';
import { rerender } from '@testing-library/react-native';
import set from 'lodash/set';
import SimpsonsLoveWednesday from '../index';

describe('<SimpsonsLoveWednesday />', () => {
it('Should render and match the snapshot', () => {
const baseElement = renderWithIntl(<SimpsonsLoveWednesday />);
Expand All @@ -29,7 +29,7 @@ describe('<SimpsonsLoveWednesday />', () => {
};
const { getByText } = renderWithIntl(<SimpsonsLoveWednesday {...props} />);
expect(getByText(props.userErrorMessage)).toBeTruthy();
props.userErrorMessage = null;
set(props, 'userErrorMessage', null);
Copy link

Choose a reason for hiding this comment

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

Consider using direct assignment instead of lodash/set for modifying props.userErrorMessage in tests for clarity and maintainability.

- set(props, 'userErrorMessage', null);
+ props.userErrorMessage = null;

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
set(props, 'userErrorMessage', null);
props.userErrorMessage = null;

Copy link
Contributor

Choose a reason for hiding this comment

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

why do we set here?

const { getByText: textQueryOnReRender } = renderWithIntl(
<SimpsonsLoveWednesday {...props} />,
rerender
Expand Down
16 changes: 10 additions & 6 deletions app/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@
* script `extract-intl`, and must use CommonJS module syntax
* You CANNOT use import/export in this file.
*/
import get from 'lodash/get';
const addLocaleData = require('react-intl').addLocaleData; //eslint-disable-line

const enLocaleData = require('react-intl/locale-data/en');

const enTranslationMessages = require('./translations/en.json');

addLocaleData(enLocaleData);

export const DEFAULT_LOCALE = 'en';
Expand All @@ -28,11 +28,15 @@ export const formatTranslationMessages = (locale, messages) => {
? formatTranslationMessages(DEFAULT_LOCALE, enTranslationMessages)
: {};
const flattenFormattedMessages = (formattedMessages, key) => {
const formattedMessage =
!messages[key] && locale !== DEFAULT_LOCALE
? defaultFormattedMessages[key]
: messages[key];
return Object.assign(formattedMessages, { [key]: formattedMessage });
const formattedMessageOptions = {
true: get(defaultFormattedMessages, key),
false: get(messages, key)
};
const formattedCondition = !get(messages, key) && locale !== DEFAULT_LOCALE;
return {
...formattedMessages,
[key]: get(formattedMessageOptions, formattedCondition)
};
Comment on lines +31 to +39
Copy link

Choose a reason for hiding this comment

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

Consider simplifying the logic in formatTranslationMessages to enhance readability. Using a ternary operator directly might be more straightforward than using a condition object.

- const formattedMessageOptions = {
-   true: get(defaultFormattedMessages, key),
-   false: get(messages, key)
- };
- const formattedCondition = !get(messages, key) && locale !== DEFAULT_LOCALE;
- return {
-   ...formattedMessages,
-   [key]: get(formattedMessageOptions, formattedCondition)
- };
+ return {
+   ...formattedMessages,
+   [key]: !get(messages, key) && locale !== DEFAULT_LOCALE ? get(defaultFormattedMessages, key) : get(messages, key)
+ };

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
const formattedMessageOptions = {
true: get(defaultFormattedMessages, key),
false: get(messages, key)
};
const formattedCondition = !get(messages, key) && locale !== DEFAULT_LOCALE;
return {
...formattedMessages,
[key]: get(formattedMessageOptions, formattedCondition)
};
return {
...formattedMessages,
[key]: !get(messages, key) && locale !== DEFAULT_LOCALE ? get(defaultFormattedMessages, key) : get(messages, key)
};

};
return Object.keys(messages).reduce(flattenFormattedMessages, {});
};
Expand Down
40 changes: 28 additions & 12 deletions app/utils/apiUtils.js
Original file line number Diff line number Diff line change
@@ -1,41 +1,57 @@
import { create } from 'apisauce';
import mapKeysDeep from 'map-keys-deep';
import { camelCase, snakeCase } from 'lodash';
import camelCase from 'lodash/camelCase';
import snakeCase from 'lodash/snakeCase';
import set from 'lodash/set';
import get from 'lodash/get';
import { Config } from '@app/config/index';

export const apiClients = {
configApi: null,
default: null
};

Check warning on line 11 in app/utils/apiUtils.js

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🌿 Branch is not covered

Warning! Not covered branch
export const getApiClient = (type = 'configApi') => apiClients[type];
export const getApiClient = (type = 'configApi') => get(apiClients, type);
export const generateApiClient = (type = 'configApi') => {
switch (type) {
case 'configApi':
apiClients[type] = createApiClientWithTransForm(Config.API_URL);
return apiClients[type];
default:
apiClients.default = createApiClientWithTransForm(Config.API_URL);
const apiClientOption = {
configApi: () => {
set(apiClients, type, createApiClientWithTransForm(Config.API_URL));
return get(apiClients, type);

Check warning on line 17 in app/utils/apiUtils.js

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered

Warning! Not covered statement
himanshu-wedensday marked this conversation as resolved.
Show resolved Hide resolved
},

Check warning on line 18 in app/utils/apiUtils.js

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered

Warning! Not covered statement

Check warning on line 18 in app/utils/apiUtils.js

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🌿 Branch is not covered

Warning! Not covered branch
default: () => {
set(apiClients, 'default', createApiClientWithTransForm(Config.API_URL));
return apiClients.default;
}
}
};
const clientGenerator = get(apiClientOption, type, apiClientOption.default);
return clientGenerator();
Copy link

Choose a reason for hiding this comment

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

Ensure the new logic in generateApiClient is covered by tests, especially since it involves dynamic property access which can be prone to errors if not handled correctly.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

};

export const createApiClientWithTransForm = baseURL => {
const api = create({
baseURL,
headers: { 'Content-Type': 'application/json' }

Check warning on line 31 in app/utils/apiUtils.js

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🌿 Branch is not covered

Warning! Not covered branch
});

// eslint-disable-next-line complexity
api.addResponseTransform(response => {
const { ok, data } = response;
if (ok && data) {
response.data = mapKeysDeep(data, keys => camelCase(keys));
set(

Check warning on line 38 in app/utils/apiUtils.js

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered

Warning! Not covered statement

Check warning on line 38 in app/utils/apiUtils.js

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered

Warning! Not covered statement

Check warning on line 38 in app/utils/apiUtils.js

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🕹 Function is not covered

Warning! Not covered function
response,

Check warning on line 39 in app/utils/apiUtils.js

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🌿 Branch is not covered

Warning! Not covered branch
'data',
mapKeysDeep(data, keys => camelCase(keys))
);
Comment on lines +33 to +37
Copy link

Choose a reason for hiding this comment

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

The transformation logic in createApiClientWithTransForm uses set and mapKeysDeep effectively for handling API response data. However, ensure this part of the code is covered by tests to prevent potential issues with data handling.

Would you like assistance in creating unit tests for this transformation logic?

}
return response;
});

api.addRequestTransform(request => {
const { data } = request;
if (data) {
request.data = mapKeysDeep(data, keys => snakeCase(keys));
set(
request,
'data',
mapKeysDeep(data, keys => snakeCase(keys))
);
Comment on lines +45 to +49
Copy link

Choose a reason for hiding this comment

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

Similarly, the request transformation logic is well implemented using set and mapKeysDeep. Coverage for this function should also be ensured.

I can help with writing tests for this part as well. Let me know if you need it.

}
return request;
});
Expand Down
10 changes: 3 additions & 7 deletions app/utils/createStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,10 @@ const persistConfig = {
};

export default (rootReducer, rootSaga) => {
const middleware = [];
const enhancers = [];

// Connect the sagas to the redux store
// connect sagas to redux store
const sagaMiddleware = createSagaMiddleware();
middleware.push(sagaMiddleware);

enhancers.push(applyMiddleware(...middleware));
const middleware = [sagaMiddleware];
const enhancers = [applyMiddleware(...middleware)];

// Redux persist
const persistedReducer = persistReducer(persistConfig, rootReducer);
Expand Down
3 changes: 2 additions & 1 deletion app/utils/testUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { IntlProvider } from 'react-intl';
import { render } from '@testing-library/react-native';
import { Provider } from 'react-redux';
import createStore from 'app/rootReducer';
import get from 'lodash/get';
import { DEFAULT_LOCALE, translationMessages } from '@app/i18n';
import ConnectedLanguageProvider from '@atoms/LanguageProvider';

Expand All @@ -14,7 +15,7 @@ export const renderWithIntl = (children, renderFunction = render) =>
renderFunction(
<IntlProvider
locale={DEFAULT_LOCALE}
messages={translationMessages[DEFAULT_LOCALE]}
messages={get(translationMessages, DEFAULT_LOCALE)}
>
{children}
</IntlProvider>
Expand Down
2 changes: 1 addition & 1 deletion babel.config.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
module.exports = function(api = { cache: () => {} }) {
module.exports = (api = { cache: () => {} }) => {
api.cache(true);
return {
presets: ['babel-preset-expo'],
Expand Down
4 changes: 2 additions & 2 deletions setupTests.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import mockAsyncStorage from '@react-native-async-storage/async-storage/jest/async-storage-mock';

import { LogBox } from 'react-native';
jest.mock('@react-native-async-storage/async-storage', () => mockAsyncStorage);
console.disableYellowBox = true;
LogBox.ignoreAllLogs();
Loading