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

Framework: Add a stored cards reducer #6413

Merged
merged 15 commits into from
Jul 1, 2016
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
6 changes: 6 additions & 0 deletions client/state/action-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,12 @@ export const SITE_VOUCHERS_REQUEST = 'SITE_VOUCHERS_REQUEST';
export const SITE_VOUCHERS_REQUEST_FAILURE = 'SITE_VOUCHERS_REQUEST_FAILURE';
export const SITE_VOUCHERS_REQUEST_SUCCESS = 'SITE_VOUCHERS_REQUEST_SUCCESS';
export const SUPPORT_USER_ACTIVATE = 'SUPPORT_USER_ACTIVATE';
export const STORED_CARDS_DELETE = 'STORED_CARDS_DELETE';
export const STORED_CARDS_DELETE_COMPLETED = 'STORED_CARDS_DELETE_COMPLETED';
export const STORED_CARDS_DELETE_FAILED = 'STORED_CARDS_DELETE_FAILED';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep this list ordered alphabetically.

export const STORED_CARDS_FETCH = 'STORED_CARDS_FETCH';
export const STORED_CARDS_FETCH_COMPLETED = 'STORED_CARDS_FETCH_COMPLETED';
export const STORED_CARDS_FETCH_FAILED = 'STORED_CARDS_FETCH_FAILED';
export const SUPPORT_USER_ERROR = 'SUPPORT_USER_ERROR';
export const SUPPORT_USER_TOGGLE_DIALOG = 'SUPPORT_USER_TOGGLE_DIALOG';
export const SUPPORT_USER_TOKEN_FETCH = 'SUPPORT_USER_TOKEN_FETCH';
Expand Down
95 changes: 95 additions & 0 deletions client/state/stored-cards/reducer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
/**
* External dependencies
*/
import { combineReducers } from 'redux';

/**
* Internal dependencies
*/
import {
SERIALIZE,
DESERIALIZE,
STORED_CARDS_FETCH,
STORED_CARDS_FETCH_COMPLETED,
STORED_CARDS_FETCH_FAILED,
STORED_CARDS_DELETE,
STORED_CARDS_DELETE_COMPLETED,
STORED_CARDS_DELETE_FAILED
} from 'state/action-types';

/**
* `Reducer` function which handles request/response actions
* concerning stored cards data updates
*
* @param {Array} state Current state
* @param {Object} action storeCard action
* @return {Array} Updated state
*/
export const items = ( state = [], action ) => {
Copy link
Member

Choose a reason for hiding this comment

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

I recommend using our new reducer util: https://github.com/Automattic/wp-calypso/blob/master/client/state/utils.js#L40.
It handles persistent layer properly. In this case you will get all state persisted, and given that we probably still have moment.js instances inside the purchase object, this one can explode when trying to serialize and deserialize :)

Copy link
Member

Choose a reason for hiding this comment

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

I mixed it up with purchases, but still persistence is the feature to take into consideration :)

switch ( action.type ) {
case STORED_CARDS_FETCH_COMPLETED:
return action.list;
case STORED_CARDS_DELETE_COMPLETED:
return state.filter( item => item.stored_details_id !== action.card.stored_details_id );
// return initial state when serializing/deserializing
case SERIALIZE:
case DESERIALIZE:
return [];
}

return state;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this reducer also handle the SERIALIZE and DESERIALIZE actions?

Copy link
Member

Choose a reason for hiding this comment

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

It depends if you want to persist data between page loads, before we didn't persist data so I would say we shouldn't change that behavior :)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I was suggesting :).


/**
* `Reducer` function which handles request/response actions
* concerning stored cards fetching
*
* @param {Object} state - current state
* @param {Object} action - storedCard action
* @return {Object} updated state
*/
export const isFetching = ( state = false, action ) => {
switch ( action.type ) {
case STORED_CARDS_FETCH:
return true;
case STORED_CARDS_FETCH_COMPLETED:
case STORED_CARDS_FETCH_FAILED:
return false;
// return initial state when serializing/deserializing
case SERIALIZE:
case DESERIALIZE:
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

In general if we don't use createReducer we always need to provide handling for SERIALIZE and DESERIALIZE, otherwise it persists data.


return state;
};

/**
* `Reducer` function which handles request/response actions
* concerning stored card deletion
*
* @param {Object} state - current state
* @param {Object} action - storedCard action
* @return {Object} updated state
*/
export const isDeleting = ( state = false, action ) => {
switch ( action.type ) {
case STORED_CARDS_DELETE:
return true;
case STORED_CARDS_DELETE_FAILED:
case STORED_CARDS_DELETE_COMPLETED:
return false;
// return initial state when serializing/deserializing
case SERIALIZE:
case DESERIALIZE:
return false;
}

return state;
};

export default combineReducers( {
items,
isFetching,
isDeleting
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I've missed some previous discussions about this approach. Could you explain me why we split reducers that way, or point me to some documentation? At first glance it seems that having isFetching and isDeleting reducers adds lot of code for no real benefit ...

Copy link
Member

Choose a reason for hiding this comment

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

This is the common patter used in the state code :)
It groups nicely actions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm missing something but with this approach we use the same actions several times in these reducers (e.g. STORED_CARDS_DELETE_COMPLETED is used in items and in isDeleting). They look scattered to me :).

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 find it clearer personally. It applies the separation of concerns principle

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it depends on what concern you want to separate: actions or flags :).

} );
17 changes: 17 additions & 0 deletions client/state/stored-cards/selectors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* Return user's stored cards from state object
*
* @param {Object} state - current state object
* @return {Array} Stored Cards
*/
export const getCards = state => state.storedCards.items;

/**
* Returns a Stored Card
* @param {Object} state global state
* @param {Number} cardId the card id
* @return {Object} the matching card if there is one
*/
export const getByCardId = ( state, cardId ) => (
getCards( state ).filter( card => card.stored_details_id === cardId ).shift()
);
32 changes: 32 additions & 0 deletions client/state/stored-cards/test/fixture.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
export const STORED_CARDS_FROM_API = [
{
user_id: 12345678,
stored_details_id: 1234567,
expiry: '2016-01-31',
card: 1234,
card_type: 'visa',
mp_ref: '8qkGjuMJJbRhyrwq8qkGjuMJJbRhyrwq',
payment_partner: 'moneypress',
name: 'John Doe',
email: 'john@example.com',
remember: 1,
meta: [],
added: '2015-10-22 11:14:05',
last_used: '2015-10-22 11:14:05'
},
{
user_id: 12345678,
stored_details_id: 12345,
expiry: '2016-11-30',
card: 2596,
card_type: 'amex',
mp_ref: 'Cb9S1bxEZDhl20cfCb9S1bxEZDhl20cf',
payment_partner: 'moneypress',
name: 'Jane Doe',
email: 'jane@example.com',
remember: 1,
meta: [],
added: '2015-02-06 20:28:11',
last_used: '2015-10-22 11:10:10'
}
];
112 changes: 112 additions & 0 deletions client/state/stored-cards/test/reducer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/**
* External dependencies
*/
import { expect } from 'chai';
import deepFreeze from 'deep-freeze';

/**
* Internal dependencies
*/
import {
STORED_CARDS_FETCH,
STORED_CARDS_FETCH_COMPLETED,
STORED_CARDS_FETCH_FAILED,
STORED_CARDS_DELETE,
STORED_CARDS_DELETE_COMPLETED,
STORED_CARDS_DELETE_FAILED
} from 'state/action-types';
import reducer from '../reducer';
import { STORED_CARDS_FROM_API } from './fixture';

describe( 'items', () => {
it( 'should return an object with the initial state', () => {
expect( reducer( undefined, { type: 'UNRELATED' } ) ).to.be.eql( {
items: [],
isFetching: false,
isDeleting: false
} );
} );

it( 'should return an object with an empty list and fetching enabled when fetching is triggered', () => {
expect( reducer( undefined, { type: STORED_CARDS_FETCH } ) ).to.be.eql( {
items: [],
isFetching: true,
isDeleting: false
} );
} );

it( 'should return an object with the list of stored cards when fetching completed', () => {
const state = reducer( undefined, {
type: STORED_CARDS_FETCH_COMPLETED,
list: STORED_CARDS_FROM_API
} );

expect( state ).to.be.eql( {
items: STORED_CARDS_FROM_API,
isFetching: false,
isDeleting: false
} );
} );

it( 'should return an object with an empty list of stored cards when fetching failed', () => {
const state = reducer( undefined, {
type: STORED_CARDS_FETCH_FAILED
} );

expect( state ).to.be.eql( {
items: [],
isFetching: false,
isDeleting: false
} );
} );

it( 'should keep the current state and enable isDeleting when requesting a stored card deletion', () => {
const state = reducer( deepFreeze( {
items: STORED_CARDS_FROM_API,
isFetching: false,
isDeleting: false
} ), {
type: STORED_CARDS_DELETE,
card: STORED_CARDS_FROM_API[ 0 ]
} );

expect( state ).to.be.eql( {
items: STORED_CARDS_FROM_API,
isFetching: false,
isDeleting: true
} );
} );

it( 'should remove a stored card from the list if the stored card deletion request succeeded', () => {
const state = reducer( deepFreeze( {
items: STORED_CARDS_FROM_API,
isFetching: false,
isDeleting: true
} ), {
type: STORED_CARDS_DELETE_COMPLETED,
card: STORED_CARDS_FROM_API[ 0 ]
} );

expect( state ).to.be.eql( {
items: [ STORED_CARDS_FROM_API[ 1 ] ],
isFetching: false,
isDeleting: false
} );
} );

it( 'should not change the list of items if the stored card deletion request failed', () => {
const state = reducer( deepFreeze( {
items: STORED_CARDS_FROM_API,
isFetching: false,
isDeleting: true
} ), {
type: STORED_CARDS_DELETE_FAILED
} );

expect( state ).to.be.eql( {
items: STORED_CARDS_FROM_API,
isFetching: false,
isDeleting: false
} );
} );
} );
37 changes: 37 additions & 0 deletions client/state/stored-cards/test/selectors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// External dependencies
import deepFreeze from 'deep-freeze';
import { expect } from 'chai';

// Internal dependencies
import { getCards, getByCardId } from '../selectors';
import { STORED_CARDS_FROM_API } from './fixture';

describe( 'selectors', () => {
describe( 'getCards', () => {
it( 'should return a purchase by its ID, preserving the top-level flags', () => {
const state = deepFreeze( {
storedCards: {
isFetching: false,
isDeleting: false,
items: STORED_CARDS_FROM_API
}
} );

expect( getCards( state ) ).to.be.eql( STORED_CARDS_FROM_API );
} );
} );

describe( 'getByCardId', () => {
it( 'should return a purchase by its ID, preserving the top-level flags', () => {
const state = deepFreeze( {
storedCards: {
isFetching: false,
isDeleting: false,
items: STORED_CARDS_FROM_API
}
} );

expect( getByCardId( state, 12345 ) ).to.be.eql( STORED_CARDS_FROM_API[ 1 ] );
} );
} );
} );