Extract authentication logic to redux state #517
Conversation
| @@ -1,2401 +1,156 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
There was a problem hiding this comment.
I think I don't fully understand the purpose of snapshots. It seems that it's something that you just replace with the current state when it fails. Apparently it's not a very useful test. Am I missing something?
migmartri
force-pushed
the
294-authorization-errors
branch
from
efacd26 to
f2fbd70
Compare
migmartri
force-pushed
the
294-authorization-errors
branch
from
0b5cd22 to
d19ecbe
Compare
migmartri
changed the title
| const token = "abcd"; | ||
| const validationErrorMsg = "Validation error"; | ||
|
|
||
| let store: any; |
There was a problem hiding this comment.
We should see if Partial<IStoreState> works here, if not we can try casting the mockStore thing to IStoreState
|
|
||
| describe("LoginFormContainer props", () => { | ||
| it("maps authentication redux states to props", () => { | ||
| const store = makeStore(true, true, "It's a trap"); |
There was a problem hiding this comment.
why not just pass the IAuthState object directly? not sure why this helper is valuable, it's also not easy to read what the first two parameters relate to
There was a problem hiding this comment.
also, shouldn't we test different variations?
There was a problem hiding this comment.
I do not see much value on passing variations since the mapStateToProps just takes the values, there are not defaults or anything.
| declare namespace JSX { | ||
| interface IntrinsicAttributes { | ||
| // Add store option for testing with mocked-store | ||
| store?: any; |
There was a problem hiding this comment.
again check if this can be IStoreState or Partial
| expect( | ||
| authReducer(undefined, { | ||
| authenticated: e, | ||
| type: actionTypes.setAuthenticated as any, |
There was a problem hiding this comment.
add a TODO comment to figure out why we need any here
First step wrt to the 401 error shown here #294
This patch sets the stage by extracting the authentication information to the Redux state instead of the local state of the login component. This will allow us to:
This patch also adds some tests for the modified/added files as well as some related libraries for such tests.
NOTE: This patch removes the ability of closing the error message shown during login. I decided to do so for simplicity since now the state comes from the props and we will need to make local and external state to interact to make it work. Things that is possible but I do not think it's worth it more taking into account that with this change we will be able to use external libraries like https://tomchentw.github.io/react-toastr/ which handle all this for free.
TODO