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.

Sign up for GitHub

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

CreditCardNumber field should report cardType #189

Merged
merged 2 commits into from
Apr 19, 2017
Merged

CreditCardNumber field should report cardType #189

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a50c943
ta - adding type-reporting and tests for cc number field
TroyAlford Apr 18, 2017
20491b5
ta - minor refactor for legibility, with propagated changes to Credit…
TroyAlford Apr 19, 2017
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
6 changes: 3 additions & 3 deletions src/components/CreditCardNumber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export default class CreditCardNumber extends Component {
setValue = (proposedValue) => {
let value = proposedValue.replace(/[^0-9]/g, '');
if (proposedValue === '') {
this.props.onChange(value, false);
this.props.onChange(value, false, undefined);
this.setState({ value, cardType: undefined });
return;
}
Expand Down Expand Up @@ -71,7 +71,7 @@ export default class CreditCardNumber extends Component {

Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated toI'm thinking this parsing logic might be better outside of the React component - can we make separate JS file in react-gears for this? Would be easier to test and update too.

Copy link
Author

Choose a reason for hiding this comment

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

The parsing logic is already outside the component - it's handled by an external lib. I'm also reticent to refactor just for refactoring sake - is there a use-case that the current implementation doesn't support?

// Only accept the change if we recognize the card type, and it is/may be valid
if (!this.props.restrictInput || cardType && (isValid || isPotentiallyValid)) {
this.props.onChange(value, isValid);
this.props.onChange(value, isValid, card.type);
Copy link
Contributor

Choose a reason for hiding this comment

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

If cardType and card.type are same can we combine or use one or the other here vs both?

Copy link
Author

Choose a reason for hiding this comment

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

They are not the same. cardType is the icon name and card.type is the name returned by the card parsing library. I'll rename cardType to be more clear.

this.setState({ value, cardType, isValid });
}
}
Expand Down Expand Up @@ -103,7 +103,7 @@ CreditCardNumber.defaultProps = {
restrictInput: false,
value: '',

onChange: (cardNumber, isValid) => true, // eslint-disable-line no-unused-vars
onChange: (cardNumber, isValid, cardType) => true, // eslint-disable-line no-unused-vars
};
CreditCardNumber.propTypes = {
allowedBrands: PropTypes.arrayOf(PropTypes.string),
Expand Down
25 changes: 18 additions & 7 deletions test/components/CreditCardNumber.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,23 @@
import React from 'react';
import assert from 'assert';
import { mount, shallow } from 'enzyme';
import sinon from 'sinon';

import CreditCardNumber from '../../src/components/CreditCardNumber';
import Icon from '../../src/components/Icon';

const EXAMPLES = {
'american-express': '378282246310005',
'diners-club': '30569309025904',
amex: '378282246310005',
'master-card': '5555555555554444',
discover: '6011111111111117',
jcb: '3530111333300000',
mastercard: '5555555555554444',
visa: '4111111111111111',
};
const ICON_MAP = {
'american-express': 'amex',
'master-card': 'mastercard',
};

describe('<CreditCardNumber />', () => {
it('should render no icon, by default', () => {
Expand All @@ -30,16 +35,22 @@ describe('<CreditCardNumber />', () => {
assert.equal(component.find(Icon).prop('name'), 'cc-visa');
});

it('should render correct icons for valid card numbers', () => {
it('should report/render icon for correct cardType for valid numbers', () => {
Object.keys(EXAMPLES).forEach(key => {
const expectedIcon = `cc-${key}`;
const cardNumber = EXAMPLES[key];
const onChange = sinon.spy();

const component = mount(<CreditCardNumber />);
const component = mount(<CreditCardNumber onChange={onChange} />);
const input = component.find('input');
input.simulate('change', { target: { value: cardNumber } });

assert.equal(component.find(Icon).prop('name'), expectedIcon);
assert(onChange.called);
const [returnedCardNumber, returnedIsValid, returnedType] = [...onChange.lastCall.args];
assert.equal(returnedCardNumber.replace(/ /g, ''), cardNumber);
assert.equal(returnedIsValid, true);
assert.equal(returnedType, key);

assert.equal(component.find(Icon).prop('name'), `cc-${ICON_MAP[key] || key}`);
});
});

Expand All @@ -65,7 +76,7 @@ describe('<CreditCardNumber />', () => {
it('restrictInput prop should reject numbers for invalid card types', () => {
const component = mount(<CreditCardNumber restrictInput allowedBrands={['visa']} />);
const input = component.find('input');
input.simulate('change', { target: { value: EXAMPLES.mastercard } });
input.simulate('change', { target: { value: EXAMPLES['master-card'] } });

assert.equal(input.get(0).value, '');
assert.equal(component.find(Icon).length, 0);
Expand Down