Framework: Add a stored cards reducer #6413
Conversation
Tug
added
Framework
[Status] In Progress
[Feature] Purchase Management
Tug
added
[Status] Needs Review
| export const STORED_CARDS_FETCH_FAILED = 'STORED_CARDS_FETCH_FAILED'; | ||
| 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'; |
There was a problem hiding this comment.
We should keep this list ordered alphabetically.
|
Fun fact is that we probably never use |
| import { createStoredCardsArray } from './assembler.js'; | ||
|
|
||
| /** | ||
| * List all known stored cards of the current user at /me/stored-cards. |
There was a problem hiding this comment.
This description doesn't seem to correspond to what this reducer function is actually doing.
There was a problem hiding this comment.
Why is that? Maybe the at /me/stored-cards is confusing here. I added it because I found the info valuable (since you rarely know from looking at the reducer from which endpoint the data is pulled from)
There was a problem hiding this comment.
So yes we should probably avoid mentioning the /me/stored-cards API endpoint since it doesn't really matter where that data is fetched from - it's a different concern.
To answer your question I found it confusing for several reasons:
- This function doesn't only return a list of cards but also flags
- This function also takes care of deleting cards
- The docblock comment of this function is not in line with the ones of other reducer functions below:
`Reducer` function which handles request/response actions concerning stored cards fetching
There was a problem hiding this comment.
This function doesn't only return a list of cards but also flags
Nop only a list of cards, flags are handled by the other reducers: isFetching and isDeleting.
This function also takes care of deleting cards
The docblock comment of this function is not in line with the ones of other reducer functions below:
True 👍
There was a problem hiding this comment.
Nop only a list of cards, flags are handled by the other reducers: isFetching and isDeleting.
You're indeed right 👍.
stephanethomas
added
[Status] Needs Author Reply
and removed
[Status] Needs Review
| describe( 'selectors', () => { | ||
| describe( 'getCards', () => { | ||
| it( 'should return a purchase by its ID, preserving the top-level flags', () => { | ||
| const state = { |
There was a problem hiding this comment.
I almost forgot, we are wrapping state with deepFreeze to make sure it doesn't get mutated.
|
Thanks @stephanethomas and @gziolo for reviewing, this is ready for another round ;) |
Tug
added
[Status] Needs Review
|
LGTM. I added a commit to ensure that we don't mutate |
drewblaisdell
added
[Status] Ready to Merge
and removed
[Status] Needs Review
Part of #5046
This PR copies the reducer from
lib/purchases/stored-cards/intostate/stored-cards/, refactors it into 3 combined reducers and adds tests for each module.This PR does not remove the old reducer or Flux store. Nor does it import the newly updated reducer into the combined global reducer. This will happen in a separate PR to keep the PR size manageable.
Testing instructions
Apart from the changes to action-types, the changes here won't affect the actual build, so this only needs a code review.
Reviews
@Automattic/sdev-feed
Test live: https://calypso.live/?branch=add/stored-cards-redux-reducer