From 2689a20ee8a722497cd9651d27ea6ce7b9f946f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maren=20S=C3=BCwer?= Date: Thu, 19 Jul 2018 09:39:57 +0200 Subject: [PATCH 1/5] added global error state for delete user failure --- scm-ui/src/users/modules/users.js | 21 +++++-- scm-ui/src/users/modules/users.test.js | 80 ++++++++++++++++++++------ 2 files changed, 77 insertions(+), 24 deletions(-) diff --git a/scm-ui/src/users/modules/users.js b/scm-ui/src/users/modules/users.js index e198cccfb3..148cc2ece4 100644 --- a/scm-ui/src/users/modules/users.js +++ b/scm-ui/src/users/modules/users.js @@ -88,7 +88,9 @@ function fetchUsersSuccess(users: any) { function requestUser(name: string) { return { type: FETCH_USER, - payload: { name } + payload: { + name + } }; } @@ -186,7 +188,7 @@ function updateUserSuccess() { }; } -function updateUserFailure(user: User, error: Error) { +export function updateUserFailure(user: User, error: Error) { return { type: UPDATE_USER_FAILURE, payload: error, @@ -294,8 +296,10 @@ export default function reducer(state: any = {}, action: any = {}) { switch (action.type) { case FETCH_USERS: return { - loading: true, - error: null + ...state, + users: { + loading: true + } }; case DELETE_USER: return reduceUsersByNames(state, action.payload.name, { @@ -344,11 +348,18 @@ export default function reducer(state: any = {}, action: any = {}) { case FETCH_USERS_FAILURE: case DELETE_USER_FAILURE: - return reduceUsersByNames(state, action.payload.user.name, { + const newState = reduceUsersByNames(state, action.payload.user.name, { loading: false, error: action.payload.error, entry: action.payload.user }); + return { + ...newState, + users: { + ...newState.users, + error: action.payload.error + } + }; case EDIT_USER: return { ...state, diff --git a/scm-ui/src/users/modules/users.test.js b/scm-ui/src/users/modules/users.test.js index 56e2a9e0de..7ec5b8c409 100644 --- a/scm-ui/src/users/modules/users.test.js +++ b/scm-ui/src/users/modules/users.test.js @@ -22,7 +22,8 @@ import { DELETE_USER, DELETE_USER_SUCCESS, DELETE_USER_FAILURE, - deleteUser + deleteUser, + updateUserFailure } from "./users"; import reducer from "./users"; @@ -118,7 +119,9 @@ const responseBody = { }; const response = { - headers: { "content-type": "application/json" }, + headers: { + "content-type": "application/json" + }, responseBody }; @@ -132,8 +135,9 @@ describe("users fetch()", () => { it("should successfully fetch users", () => { fetchMock.getOnce("/scm/api/rest/v2/users", response); - const expectedActions = [ - { type: FETCH_USERS }, + const expectedActions = [{ + type: FETCH_USERS + }, { type: FETCH_USERS_SUCCESS, payload: response @@ -257,17 +261,20 @@ describe("users fetch()", () => { }); describe("users reducer", () => { - test("should update state correctly according to FETCH_USERS action", () => { - const newState = reducer({}, { type: FETCH_USERS }); - expect(newState.loading).toBeTruthy(); - expect(newState.error).toBeNull(); + + it("should update state correctly according to FETCH_USERS action", () => { + const newState = reducer({}, { + type: FETCH_USERS + }); + expect(newState.users.loading).toBeTruthy(); + expect(newState.users.error).toBeFalsy(); }); it("should update state correctly according to FETCH_USERS_SUCCESS action", () => { - const newState = reducer( - {}, - { type: FETCH_USERS_SUCCESS, payload: responseBody } - ); + const newState = reducer({}, { + type: FETCH_USERS_SUCCESS, + payload: responseBody + }); expect(newState.users).toEqual({ entries: ["zaphod", "ford"], @@ -358,6 +365,44 @@ describe("users reducer", () => { expect(ford.loading).toBeFalsy(); }); + it("should set global error state if one user could not be deleted", () => { + const state = { + users: { + error: null + }, + usersByNames: { + zaphod: { + loading: false, + error: null, + entry: userZaphod + } + } + }; + + const error = new Error("could not delete user zaphod: .."); + const newState = reducer(state, deleteUserFailure(userZaphod, error)); + expect(newState.users.error).toBe(error); + }); + + it("should not set global error state if one user could not be edited", () => { + const state = { + users: { + error: null + }, + usersByNames: { + zaphod: { + loading: false, + error: null, + entry: userZaphod + } + } + }; + + const error = new Error("could not edit user zaphod: .."); + const newState = reducer(state, updateUserFailure(userZaphod, error)); + expect(newState.users.error).toBe(null); + }); + it("should not replace whole usersByNames map when fetching users", () => { const oldState = { usersByNames: { @@ -376,13 +421,10 @@ describe("users reducer", () => { }); it("should update state correctly according to EDIT_USER action", () => { - const newState = reducer( - {}, - { - type: EDIT_USER, - user: userZaphod - } - ); + const newState = reducer({}, { + type: EDIT_USER, + user: userZaphod + }); expect(newState.editUser).toEqual(userZaphod); }); }); From 9593a18a5551e307f70ca50eecb319e277b03736 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maren=20S=C3=BCwer?= Date: Thu, 19 Jul 2018 11:23:20 +0200 Subject: [PATCH 2/5] add error notification if user deletion is unsuccessful and add DELETE_USER_SUCCESS --- scm-ui/src/users/containers/Users.js | 6 ++- scm-ui/src/users/modules/users.js | 73 ++++++++++++++++++++------ scm-ui/src/users/modules/users.test.js | 41 ++++++++++++++- 3 files changed, 101 insertions(+), 19 deletions(-) diff --git a/scm-ui/src/users/containers/Users.js b/scm-ui/src/users/containers/Users.js index 6c9793b89c..2d075717f4 100644 --- a/scm-ui/src/users/containers/Users.js +++ b/scm-ui/src/users/containers/Users.js @@ -51,9 +51,13 @@ class Users extends React.Component { const mapStateToProps = state => { const userEntries = getUsersFromState(state); + let error = null; + if (state.users && state.users.users) { + error = state.users.users.error + } return { userEntries, - error: state.users.error + error }; }; diff --git a/scm-ui/src/users/modules/users.js b/scm-ui/src/users/modules/users.js index 148cc2ece4..415b1d9e9a 100644 --- a/scm-ui/src/users/modules/users.js +++ b/scm-ui/src/users/modules/users.js @@ -1,8 +1,17 @@ // @flow -import { apiClient, NOT_FOUND_ERROR } from "../../apiclient"; -import type { User } from "../types/User"; -import type { UserEntry } from "../types/UserEntry"; -import { Dispatch } from "redux"; +import { + apiClient, + NOT_FOUND_ERROR +} from "../../apiclient"; +import type { + User +} from "../types/User"; +import type { + UserEntry +} from "../types/UserEntry"; +import { + Dispatch +} from "redux"; export const FETCH_USERS = "scm/users/FETCH"; export const FETCH_USERS_SUCCESS = "scm/users/FETCH_SUCCESS"; @@ -53,7 +62,7 @@ function usersNotFound(url: string) { } export function fetchUsers() { - return function(dispatch: any) { + return function (dispatch: any) { dispatch(requestUsers()); return apiClient .get(USERS_URL) @@ -96,7 +105,7 @@ function requestUser(name: string) { export function fetchUser(name: string) { const userUrl = USER_URL + name; - return function(dispatch: any) { + return function (dispatch: any) { dispatch(requestUsers()); return apiClient .get(userUrl) @@ -136,7 +145,7 @@ function requestAddUser(user: User) { } export function addUser(user: User) { - return function(dispatch: Dispatch) { + return function (dispatch: Dispatch) { dispatch(requestAddUser(user)); return apiClient .postWithContentType(USERS_URL, user, CONTENT_TYPE_USER) @@ -170,7 +179,7 @@ function requestUpdateUser(user: User) { } export function updateUser(user: User) { - return function(dispatch: Dispatch) { + return function (dispatch: Dispatch) { dispatch(requestUpdateUser(user)); return apiClient .putWithContentType(user._links.update.href, user, CONTENT_TYPE_USER) @@ -203,7 +212,7 @@ export function requestDeleteUser(user: User) { }; } -function deleteUserSuccess(user: User) { +export function deleteUserSuccess(user: User) { return { type: DELETE_USER_SUCCESS, payload: user @@ -221,7 +230,7 @@ export function deleteUserFailure(user: User, error: Error) { } export function deleteUser(user: User) { - return function(dispatch: any) { + return function (dispatch: any) { dispatch(requestDeleteUser(user)); return apiClient .delete(user._links.delete.href) @@ -229,7 +238,10 @@ export function deleteUser(user: User) { dispatch(deleteUserSuccess(user)); dispatch(fetchUsers()); }) - .catch(err => dispatch(deleteUserFailure(user, err))); + .catch(cause => { + const error = new Error(`could not delete user ${user.name}: ${cause.message}`); + dispatch(deleteUserFailure(user, error)); + }); }; } @@ -241,7 +253,7 @@ export function getUsersFromState(state) { if (!userNames) { return null; } - const userEntries: Array = []; + const userEntries: Array < UserEntry > = []; for (let userName of userNames) { userEntries.push(state.users.usersByNames[userName]); @@ -251,8 +263,8 @@ export function getUsersFromState(state) { } function extractUsersByNames( - users: Array, - userNames: Array, + users: Array < User > , + userNames: Array < string > , oldUsersByNames: {} ) { const usersByNames = {}; @@ -276,6 +288,24 @@ export function editUser(user: User) { }; } +function deleteUserInUsersByNames(users:{}, userName: any){ + let newUsers = {}; + for(let username in users){ + if(username != userName) + newUsers[username] = users[username]; + } + return newUsers; +} + +function deleteUserInEntries(users:[], userName: any){ + let newUsers = []; + for(let user of users){ + if(user != userName) + newUsers.push(user); + } + return newUsers; +} + const reduceUsersByNames = ( state: any, username: string, @@ -332,8 +362,7 @@ export default function reducer(state: any = {}, action: any = {}) { }; case FETCH_USER_SUCCESS: const ubn = extractUsersByNames( - [action.payload], - [action.payload.name], + [action.payload], [action.payload.name], state.usersByNames ); return { @@ -345,7 +374,17 @@ export default function reducer(state: any = {}, action: any = {}) { }, usersByNames: ubn }; - + case DELETE_USER_SUCCESS: + const newUserByNames = deleteUserInUsersByNames(state.usersByNames, [action.payload.name]); + const newUserEntries = deleteUserInEntries(state.users.entries, [action.payload.name]); + return { + ...state, + users: { + ...state.users, + entries: newUserEntries + }, + usersByNames: newUserByNames + } case FETCH_USERS_FAILURE: case DELETE_USER_FAILURE: const newState = reduceUsersByNames(state, action.payload.user.name, { diff --git a/scm-ui/src/users/modules/users.test.js b/scm-ui/src/users/modules/users.test.js index 7ec5b8c409..1eb38ca2fe 100644 --- a/scm-ui/src/users/modules/users.test.js +++ b/scm-ui/src/users/modules/users.test.js @@ -23,7 +23,8 @@ import { DELETE_USER_SUCCESS, DELETE_USER_FAILURE, deleteUser, - updateUserFailure + updateUserFailure, + deleteUserSuccess } from "./users"; import reducer from "./users"; @@ -365,6 +366,44 @@ describe("users reducer", () => { expect(ford.loading).toBeFalsy(); }); + it("should delete deleted user in users in state if delete user action is successful", () => { + const state = { + users: { + entries: [ + "zaphod", + "ford" + ] + } + }; + const newState = reducer(state, deleteUserSuccess(userZaphod)); + expect(newState.users.entries).toContain("ford"); + expect(newState.users.entries).not.toContain("zaphod"); + }); + + it("should delete deleted user in usersByNames in state if delete user action is successful", () => { + const state = { + users: { + entries: [ + "zaphod", + "ford" + ] + }, + usersByNames: { + zaphod: { + entry: userZaphod + }, + ford: { + entry: userFord + } + } + }; + const newState = reducer(state, deleteUserSuccess(userZaphod)); + const ford = newState.usersByNames["ford"]; + const zaphod = newState.usersByNames["zaphod"]; + expect(zaphod).toBeUndefined(); + expect(ford.entry).toBe(userFord); + }); + it("should set global error state if one user could not be deleted", () => { const state = { users: { From 8562b9d65e5c9352112fac8691fd030bd0b156f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maren=20S=C3=BCwer?= Date: Thu, 19 Jul 2018 11:57:59 +0200 Subject: [PATCH 3/5] add error handling for fetching users --- scm-ui/src/users/containers/Users.js | 9 ++++++- scm-ui/src/users/modules/users.js | 34 ++++++++++++++------------ scm-ui/src/users/modules/users.test.js | 17 ++++++++++++- 3 files changed, 43 insertions(+), 17 deletions(-) diff --git a/scm-ui/src/users/containers/Users.js b/scm-ui/src/users/containers/Users.js index 2d075717f4..fa44e95d70 100644 --- a/scm-ui/src/users/containers/Users.js +++ b/scm-ui/src/users/containers/Users.js @@ -43,7 +43,14 @@ class Users extends React.Component { ); - } else { + } else if(error){ + return ( +
+ +
+ ); + } + else { return ; } } diff --git a/scm-ui/src/users/modules/users.js b/scm-ui/src/users/modules/users.js index 415b1d9e9a..aa537ab215 100644 --- a/scm-ui/src/users/modules/users.js +++ b/scm-ui/src/users/modules/users.js @@ -46,11 +46,13 @@ function requestUsers() { }; } -function failedToFetchUsers(url: string, err: Error) { +export function failedToFetchUsers(url: string, error: Error) { return { type: FETCH_USERS_FAILURE, - payload: err, - url + payload: { + error, + url + } }; } @@ -77,12 +79,9 @@ export function fetchUsers() { .then(data => { dispatch(fetchUsersSuccess(data)); }) - .catch(err => { - if (err === NOT_FOUND_ERROR) { - dispatch(usersNotFound(USERS_URL)); - } else { - dispatch(failedToFetchUsers(USERS_URL, err)); - } + .catch(cause => { + const error = new Error(`could not fetch users: ${cause.message}`); + dispatch(failedToFetchUsers(USERS_URL, error)); }); }; } @@ -120,12 +119,9 @@ export function fetchUser(name: string) { .then(data => { dispatch(fetchUserSuccess(data)); }) - .catch(err => { - if (err === NOT_FOUND_ERROR) { - dispatch(usersNotFound(userUrl)); - } else { - dispatch(failedToFetchUsers(userUrl, err)); - } + .catch(cause => { + const error = new Error(`could not fetch user: ${cause.message}`); + dispatch(failedToFetchUsers(USERS_URL, error)); }); }; } @@ -386,6 +382,14 @@ export default function reducer(state: any = {}, action: any = {}) { usersByNames: newUserByNames } case FETCH_USERS_FAILURE: + return { + ...state, + users: { + ...state.users, + loading: false, + error: action.payload.error + } + }; case DELETE_USER_FAILURE: const newState = reduceUsersByNames(state, action.payload.user.name, { loading: false, diff --git a/scm-ui/src/users/modules/users.test.js b/scm-ui/src/users/modules/users.test.js index 1eb38ca2fe..0625477690 100644 --- a/scm-ui/src/users/modules/users.test.js +++ b/scm-ui/src/users/modules/users.test.js @@ -24,7 +24,8 @@ import { DELETE_USER_FAILURE, deleteUser, updateUserFailure, - deleteUserSuccess + deleteUserSuccess, + failedToFetchUsers } from "./users"; import reducer from "./users"; @@ -442,6 +443,20 @@ describe("users reducer", () => { expect(newState.users.error).toBe(null); }); + it("should set error state if users could not be fetched", () => { + const state = { + users: { + error: null, + loading: true + } + }; + + const error = new Error("could not edit user zaphod: .."); + const newState = reducer(state, failedToFetchUsers('/users', error)); + expect(newState.users.error).toBe(error); + expect(newState.users.loading).toBeFalsy(); + }); + it("should not replace whole usersByNames map when fetching users", () => { const oldState = { usersByNames: { From 06d6f1d3dd4a7811981fafe25aad4d0550631d84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maren=20S=C3=BCwer?= Date: Thu, 19 Jul 2018 11:59:58 +0200 Subject: [PATCH 4/5] renaming test to it --- scm-ui/src/users/modules/users.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scm-ui/src/users/modules/users.test.js b/scm-ui/src/users/modules/users.test.js index 0625477690..0f389c0b64 100644 --- a/scm-ui/src/users/modules/users.test.js +++ b/scm-ui/src/users/modules/users.test.js @@ -294,7 +294,7 @@ describe("users reducer", () => { }); }); - test("should update state correctly according to DELETE_USER action", () => { + it("should update state correctly according to DELETE_USER action", () => { const state = { usersByNames: { zaphod: { From ef8000b3805b31986484d434d155d7fd04bed7e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maren=20S=C3=BCwer?= Date: Thu, 19 Jul 2018 12:05:17 +0200 Subject: [PATCH 5/5] remove unused function --- scm-ui/src/users/modules/users.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/scm-ui/src/users/modules/users.js b/scm-ui/src/users/modules/users.js index aa537ab215..a0ea344834 100644 --- a/scm-ui/src/users/modules/users.js +++ b/scm-ui/src/users/modules/users.js @@ -56,13 +56,6 @@ export function failedToFetchUsers(url: string, error: Error) { }; } -function usersNotFound(url: string) { - return { - type: FETCH_USERS_NOTFOUND, - url - }; -} - export function fetchUsers() { return function (dispatch: any) { dispatch(requestUsers());