From fe0b7ea9869385de5cce30abc909510be818760a Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Wed, 25 Jul 2018 13:21:49 +0200 Subject: [PATCH] restructure ui for users --- scm-ui/src/components/Checkbox.js | 8 +- .../SecondaryNavigation/NavAction.js | 20 +++ .../components/SecondaryNavigation/NavLink.js | 37 ++++++ .../SecondaryNavigation/Navigation.js | 14 +++ .../components/SecondaryNavigation/Section.js | 21 ++++ .../components/SecondaryNavigation/index.js | 4 + scm-ui/src/containers/Main.js | 10 +- scm-ui/src/users/containers/AddUser.js | 41 ++++-- .../src/users/containers/DeleteUserButton.js | 19 +-- .../users/containers/DeleteUserButton.test.js | 48 +++---- scm-ui/src/users/containers/Details.js | 48 +++++++ scm-ui/src/users/containers/EditUser.js | 46 +------ scm-ui/src/users/containers/SingleUser.js | 118 ++++++++++++++++++ scm-ui/src/users/containers/UserForm.js | 76 ++++++----- scm-ui/src/users/containers/UserRow.js | 29 ++--- scm-ui/src/users/containers/UserTable.js | 17 +-- scm-ui/src/users/containers/Users.js | 10 +- scm-ui/src/users/modules/users.js | 22 ++-- scm-ui/src/users/modules/users.test.js | 39 ++++-- 19 files changed, 426 insertions(+), 201 deletions(-) create mode 100644 scm-ui/src/components/SecondaryNavigation/NavAction.js create mode 100644 scm-ui/src/components/SecondaryNavigation/NavLink.js create mode 100644 scm-ui/src/components/SecondaryNavigation/Navigation.js create mode 100644 scm-ui/src/components/SecondaryNavigation/Section.js create mode 100644 scm-ui/src/components/SecondaryNavigation/index.js create mode 100644 scm-ui/src/users/containers/Details.js create mode 100644 scm-ui/src/users/containers/SingleUser.js diff --git a/scm-ui/src/components/Checkbox.js b/scm-ui/src/components/Checkbox.js index 346ff6bc2a..b37f951e2b 100644 --- a/scm-ui/src/components/Checkbox.js +++ b/scm-ui/src/components/Checkbox.js @@ -2,13 +2,15 @@ import React from "react"; type Props = { - label: string, + label?: string, checked: boolean, - onChange: boolean => void + onChange?: boolean => void }; class Checkbox extends React.Component { onCheckboxChange = (event: SyntheticInputEvent) => { - this.props.onChange(event.target.checked); + if (this.props.onChange) { + this.props.onChange(event.target.checked); + } }; render() { diff --git a/scm-ui/src/components/SecondaryNavigation/NavAction.js b/scm-ui/src/components/SecondaryNavigation/NavAction.js new file mode 100644 index 0000000000..5eacbb7407 --- /dev/null +++ b/scm-ui/src/components/SecondaryNavigation/NavAction.js @@ -0,0 +1,20 @@ +//@flow +import React from "react"; + +type Props = { + label: string, + action: () => void +}; + +class NavAction extends React.Component { + render() { + const { label, action } = this.props; + return ( +
  • + {label} +
    • + ); + } +} + +export default NavAction; diff --git a/scm-ui/src/components/SecondaryNavigation/NavLink.js b/scm-ui/src/components/SecondaryNavigation/NavLink.js new file mode 100644 index 0000000000..3a90fd1fc6 --- /dev/null +++ b/scm-ui/src/components/SecondaryNavigation/NavLink.js @@ -0,0 +1,37 @@ +//@flow +import * as React from "react"; +import { Route, Link } from "react-router-dom"; + +// TODO mostly copy of PrimaryNavigationLink + +type Props = { + to: string, + label: string, + activeOnlyWhenExact?: boolean +}; + +class NavLink extends React.Component { + static defaultProps = { + activeOnlyWhenExact: true + }; + + renderLink = (route: any) => { + const { to, label } = this.props; + return ( +
  • + + {label} + +
    • + ); + }; + + render() { + const { to, activeOnlyWhenExact } = this.props; + return ( + + ); + } +} + +export default NavLink; diff --git a/scm-ui/src/components/SecondaryNavigation/Navigation.js b/scm-ui/src/components/SecondaryNavigation/Navigation.js new file mode 100644 index 0000000000..2b9e85a2c5 --- /dev/null +++ b/scm-ui/src/components/SecondaryNavigation/Navigation.js @@ -0,0 +1,14 @@ +//@flow +import * as React from "react"; + +type Props = { + children?: React.Node +}; + +class Navigation extends React.Component { + render() { + return ; + } +} + +export default Navigation; diff --git a/scm-ui/src/components/SecondaryNavigation/Section.js b/scm-ui/src/components/SecondaryNavigation/Section.js new file mode 100644 index 0000000000..58339529e5 --- /dev/null +++ b/scm-ui/src/components/SecondaryNavigation/Section.js @@ -0,0 +1,21 @@ +//@flow +import * as React from "react"; + +type Props = { + label: string, + children?: React.Node +}; + +class Section extends React.Component { + render() { + const { label, children } = this.props; + return ( +
    +

    {label}

    +
      {children}
    +
    + ); + } +} + +export default Section; diff --git a/scm-ui/src/components/SecondaryNavigation/index.js b/scm-ui/src/components/SecondaryNavigation/index.js new file mode 100644 index 0000000000..af86075056 --- /dev/null +++ b/scm-ui/src/components/SecondaryNavigation/index.js @@ -0,0 +1,4 @@ +export { default as Navigation } from "./Navigation"; +export { default as Section } from "./Section"; +export { default as NavLink } from "./NavLink"; +export { default as NavAction } from "./NavAction"; diff --git a/scm-ui/src/containers/Main.js b/scm-ui/src/containers/Main.js index 0ee8126efd..1b29d985e8 100644 --- a/scm-ui/src/containers/Main.js +++ b/scm-ui/src/containers/Main.js @@ -10,8 +10,8 @@ import Logout from "../containers/Logout"; import { Switch } from "react-router-dom"; import ProtectedRoute from "../components/ProtectedRoute"; -import EditUser from "../users/containers/EditUser"; import AddUser from "../users/containers/AddUser"; +import SingleUser from "../users/containers/SingleUser"; type Props = { authenticated?: boolean @@ -39,13 +39,13 @@ class Main extends React.Component { /> diff --git a/scm-ui/src/users/containers/AddUser.js b/scm-ui/src/users/containers/AddUser.js index e39fb5054e..71d9bf58be 100644 --- a/scm-ui/src/users/containers/AddUser.js +++ b/scm-ui/src/users/containers/AddUser.js @@ -3,33 +3,48 @@ import React from "react"; import { connect } from "react-redux"; import UserForm from "./UserForm"; import type { User } from "../types/User"; - +import type { History } from "history"; import { createUser } from "../modules/users"; +import Page from "../../components/Page"; type Props = { - addUser: User => void, - loading?: boolean + addUser: (user: User, callback?: () => void) => void, + loading?: boolean, + error?: Error, + history: History }; class AddUser extends React.Component { - render() { - const addUser = this.props.addUser; + userCreated = () => { + const { history } = this.props; + history.push("/users"); + }; + createUser = (user: User) => { + this.props.addUser(user, this.userCreated); + }; + + render() { + const { loading, error } = this.props; + + // TODO i18n return ( -
    - addUser(user)} - loading={this.props.loading} - /> -
    + + this.createUser(user)} /> + ); } } const mapDispatchToProps = dispatch => { return { - addUser: (user: User) => { - dispatch(createUser(user)); + addUser: (user: User, callback?: () => void) => { + dispatch(createUser(user, callback)); } }; }; diff --git a/scm-ui/src/users/containers/DeleteUserButton.js b/scm-ui/src/users/containers/DeleteUserButton.js index 31190505a5..9c4c18fed1 100644 --- a/scm-ui/src/users/containers/DeleteUserButton.js +++ b/scm-ui/src/users/containers/DeleteUserButton.js @@ -2,12 +2,11 @@ import React from "react"; import { translate } from "react-i18next"; import type { User } from "../types/User"; -import type { UserEntry } from "../types/UserEntry"; import { confirmAlert } from "../../components/ConfirmAlert"; -import DeleteButton from "../../components/DeleteButton"; +import { NavAction } from "../../components/SecondaryNavigation"; type Props = { - entry: UserEntry, + user: User, confirmDialog?: boolean, t: string => string, deleteUser: (user: User) => void @@ -19,7 +18,7 @@ class DeleteUserButton extends React.Component { }; deleteUser = () => { - this.props.deleteUser(this.props.entry.entry); + this.props.deleteUser(this.props.user); }; confirmDelete = () => { @@ -41,23 +40,17 @@ class DeleteUserButton extends React.Component { }; isDeletable = () => { - return this.props.entry.entry._links.delete; + return this.props.user._links.delete; }; render() { - const { confirmDialog, entry, t } = this.props; + const { confirmDialog, t } = this.props; const action = confirmDialog ? this.confirmDelete : this.deleteUser; if (!this.isDeletable()) { return null; } - return ( - - ); + return ; } } diff --git a/scm-ui/src/users/containers/DeleteUserButton.test.js b/scm-ui/src/users/containers/DeleteUserButton.test.js index b44416056b..a16eba053e 100644 --- a/scm-ui/src/users/containers/DeleteUserButton.test.js +++ b/scm-ui/src/users/containers/DeleteUserButton.test.js @@ -9,61 +9,53 @@ jest.mock("../../components/ConfirmAlert"); describe("DeleteUserButton", () => { it("should render nothing, if the delete link is missing", () => { - const entry = { - entry: { - _links: {} - } + const user = { + _links: {} }; const button = shallow( - {}} /> + {}} /> ); expect(button.text()).toBe(""); }); it("should render the button", () => { - const entry = { - entry: { - _links: { - delete: { - href: "/users" - } + const user = { + _links: { + delete: { + href: "/users" } } }; const button = mount( - {}} /> + {}} /> ); expect(button.text()).not.toBe(""); }); it("should open the confirm dialog on button click", () => { - const entry = { - entry: { - _links: { - delete: { - href: "/users" - } + const user = { + _links: { + delete: { + href: "/users" } } }; const button = mount( - {}} /> + {}} /> ); - button.simulate("click"); + button.find("a").simulate("click"); expect(confirmAlert.mock.calls.length).toBe(1); }); it("should call the delete user function with delete url", () => { - const entry = { - entry: { - _links: { - delete: { - href: "/users" - } + const user = { + _links: { + delete: { + href: "/users" } } }; @@ -75,12 +67,12 @@ describe("DeleteUserButton", () => { const button = mount( ); - button.simulate("click"); + button.find("a").simulate("click"); expect(calledUrl).toBe("/users"); }); diff --git a/scm-ui/src/users/containers/Details.js b/scm-ui/src/users/containers/Details.js new file mode 100644 index 0000000000..233af5759b --- /dev/null +++ b/scm-ui/src/users/containers/Details.js @@ -0,0 +1,48 @@ +//@flow +import React from "react"; +import type { User } from "../types/User"; +import { translate } from "react-i18next"; +import Checkbox from "../../components/Checkbox"; + +type Props = { + user: User, + t: string => string +}; + +class Details extends React.Component { + render() { + const { user, t } = this.props; + return ( + + + + + + + + + + + + + + + + + + + + + + + +
    {t("user.name")}{user.name}
    {t("user.displayName")}{user.displayName}
    {t("user.mail")}{user.mail}
    {t("user.admin")} + +
    {t("user.active")} + +
    + ); + } +} + +export default translate("users")(Details); diff --git a/scm-ui/src/users/containers/EditUser.js b/scm-ui/src/users/containers/EditUser.js index ba1b83c83e..8cdb8f9298 100644 --- a/scm-ui/src/users/containers/EditUser.js +++ b/scm-ui/src/users/containers/EditUser.js @@ -3,50 +3,23 @@ import React from "react"; import { connect } from "react-redux"; import UserForm from "./UserForm"; import type { User } from "../types/User"; -import type { UserEntry } from "../types/UserEntry"; -import Loading from "../../components/Loading"; - -import { modifyUser, fetchUser } from "../modules/users"; +import { modifyUser } from "../modules/users"; type Props = { - name: string, - fetchUser: string => void, - userEntry?: UserEntry, + user: User, updateUser: User => void, loading: boolean }; class EditUser extends React.Component { - componentDidMount() { - this.props.fetchUser(this.props.name); - } - render() { - const submitUser = this.props.updateUser; - - const { userEntry } = this.props; - - if (!userEntry || userEntry.loading) { - return ; - } else { - return ( -
    - submitUser(user)} - user={userEntry.entry} - loading={userEntry.loading} - /> -
    - ); - } + const { user, updateUser } = this.props; + return updateUser(user)} user={user} />; } } const mapDispatchToProps = dispatch => { return { - fetchUser: (name: string) => { - dispatch(fetchUser(name)); - }, updateUser: (user: User) => { dispatch(modifyUser(user)); } @@ -54,16 +27,7 @@ const mapDispatchToProps = dispatch => { }; const mapStateToProps = (state, ownProps) => { - const name = ownProps.match.params.name; - let userEntry; - if (state.users && state.users.usersByNames) { - userEntry = state.users.usersByNames[name]; - } - - return { - name, - userEntry - }; + return {}; }; export default connect( diff --git a/scm-ui/src/users/containers/SingleUser.js b/scm-ui/src/users/containers/SingleUser.js new file mode 100644 index 0000000000..d00af2a4c2 --- /dev/null +++ b/scm-ui/src/users/containers/SingleUser.js @@ -0,0 +1,118 @@ +//@flow +import React from "react"; +import { connect } from "react-redux"; +import Page from "../../components/Page"; +import { Route } from "react-router"; +import Details from "./Details"; +import EditUser from "./EditUser"; +import type { User } from "../types/User"; +import type { UserEntry } from "../types/UserEntry"; +import { fetchUser, deleteUser } from "../modules/users"; +import Loading from "../../components/Loading"; + +import { + Navigation, + Section, + NavLink +} from "../../components/SecondaryNavigation"; +import DeleteUserButton from "./DeleteUserButton"; +import ErrorPage from "../../components/ErrorPage"; + +type Props = { + name: string, + userEntry?: UserEntry, + match: any, + deleteUser: (user: User) => void, + fetchUser: string => void +}; + +class SingleUser extends React.Component { + componentDidMount() { + this.props.fetchUser(this.props.name); + } + + stripEndingSlash = (url: string) => { + if (url.endsWith("/")) { + return url.substring(0, url.length - 2); + } + return url; + }; + + render() { + const { userEntry, match, deleteUser } = this.props; + + if (!userEntry || userEntry.loading) { + return ; + } + + if (userEntry.error) { + return ( + + ); + } + + const user = userEntry.entry; + const url = this.stripEndingSlash(match.url); + + // TODO i18n + + return ( + +
    +
    +
    } /> + } + /> +
    +
    + +
    + + +
    +
    + + +
    +     +
    +
    +     + ); + } +} + +const mapStateToProps = (state, ownProps) => { + const name = ownProps.match.params.name; + let userEntry; + if (state.users && state.users.usersByNames) { + userEntry = state.users.usersByNames[name]; + } + + return { + name, + userEntry + }; +}; + +const mapDispatchToProps = dispatch => { + return { + fetchUser: (name: string) => { + dispatch(fetchUser(name)); + }, + deleteUser: (user: User) => { + dispatch(deleteUser(user)); + } + }; +}; + +export default connect( + mapStateToProps, + mapDispatchToProps +)(SingleUser); diff --git a/scm-ui/src/users/containers/UserForm.js b/scm-ui/src/users/containers/UserForm.js index 2488e6d2e3..3b15ff54c8 100644 --- a/scm-ui/src/users/containers/UserForm.js +++ b/scm-ui/src/users/containers/UserForm.js @@ -42,45 +42,43 @@ class UserForm extends React.Component { const user = this.state; if (user) { return ( -
    -
    - - - - - - - - -
    +
    + + + + + + + + ); } else { return ; diff --git a/scm-ui/src/users/containers/UserRow.js b/scm-ui/src/users/containers/UserRow.js index 84ff684f5b..e161e2b802 100644 --- a/scm-ui/src/users/containers/UserRow.js +++ b/scm-ui/src/users/containers/UserRow.js @@ -1,33 +1,30 @@ // @flow import React from "react"; -import DeleteUserButton from "./DeleteUserButton"; -import EditUserButton from "./EditUserButton"; +import { Link } from "react-router-dom"; import type { User } from "../types/User"; -import type { UserEntry } from "../types/UserEntry"; type Props = { - entry: UserEntry, - deleteUser: User => void + user: User }; export default class UserRow extends React.Component { + renderLink(to: string, label: string) { + return {label}; + } + render() { - const { entry, deleteUser } = this.props; - const user = entry.entry; + const { user } = this.props; + const to = `/user/${user.name}`; return ( - {user.name} - {user.displayName} - {user.mail} + {this.renderLink(to, user.name)} + {this.renderLink(to, user.displayName)} + {user.mail} + + - - - - - - ); } diff --git a/scm-ui/src/users/containers/UserTable.js b/scm-ui/src/users/containers/UserTable.js index 7e321293e8..8435b128fd 100644 --- a/scm-ui/src/users/containers/UserTable.js +++ b/scm-ui/src/users/containers/UserTable.js @@ -2,36 +2,29 @@ import React from "react"; import { translate } from "react-i18next"; import UserRow from "./UserRow"; -import type { User } from "../types/User"; import type { UserEntry } from "../types/UserEntry"; type Props = { t: string => string, - entries: Array, - deleteUser: User => void + entries: Array }; class UserTable extends React.Component { render() { - const { deleteUser, t } = this.props; - const entries = this.props.entries; + const { entries, t } = this.props; return ( - + - - {entries.map((entry, index) => { - return ( - - ); + return ; })}
    {t("user.name")}{t("user.name")} {t("user.displayName")} {t("user.mail")}{t("user.admin")} - + {t("user.admin")}
    diff --git a/scm-ui/src/users/containers/Users.js b/scm-ui/src/users/containers/Users.js index 047369c407..8382852cb0 100644 --- a/scm-ui/src/users/containers/Users.js +++ b/scm-ui/src/users/containers/Users.js @@ -3,7 +3,7 @@ import React from "react"; import { connect } from "react-redux"; import { translate } from "react-i18next"; -import { fetchUsers, deleteUser, getUsersFromState } from "../modules/users"; +import { fetchUsers, getUsersFromState } from "../modules/users"; import Page from "../../components/Page"; import UserTable from "./UserTable"; import type { User } from "../types/User"; @@ -16,7 +16,6 @@ type Props = { t: string => string, userEntries: Array, fetchUsers: () => void, - deleteUser: User => void, canAddUsers: boolean }; @@ -26,7 +25,7 @@ class Users extends React.Component { } render() { - const { userEntries, deleteUser, loading, t, error } = this.props; + const { userEntries, loading, t, error } = this.props; return ( { loading={loading || !userEntries} error={error} > - + {this.renderAddButton()} ); @@ -76,9 +75,6 @@ const mapDispatchToProps = dispatch => { return { fetchUsers: () => { dispatch(fetchUsers()); - }, - deleteUser: (user: User) => { - dispatch(deleteUser(user)); } }; }; diff --git a/scm-ui/src/users/modules/users.js b/scm-ui/src/users/modules/users.js index 3c381146a1..dc5022a3bd 100644 --- a/scm-ui/src/users/modules/users.js +++ b/scm-ui/src/users/modules/users.js @@ -78,7 +78,6 @@ export function fetchUsersFailure(url: string, error: Error): Action { } //fetch user -//TODO: fetchUsersPending and FetchUsersFailure are the wrong functions here! export function fetchUser(name: string) { const userUrl = USERS_URL + "/" + name; return function(dispatch: any) { @@ -98,7 +97,7 @@ export function fetchUser(name: string) { }) .catch(cause => { const error = new Error(`could not fetch user: ${cause.message}`); - dispatch(fetchUserFailure(USERS_URL, error)); + dispatch(fetchUserFailure(name, error)); }); }; } @@ -129,14 +128,16 @@ export function fetchUserFailure(username: string, error: Error): Action { //create user -export function createUser(user: User) { +export function createUser(user: User, callback?: () => void) { return function(dispatch: Dispatch) { dispatch(createUserPending(user)); return apiClient .postWithContentType(USERS_URL, user, CONTENT_TYPE_USER) .then(() => { dispatch(createUserSuccess()); - dispatch(fetchUsers()); + if (callback) { + callback(); + } }) .catch(err => dispatch( @@ -224,7 +225,6 @@ export function deleteUser(user: User) { .delete(user._links.delete.href) .then(() => { dispatch(deleteUserSuccess(user)); - dispatch(fetchUsers()); }) .catch(cause => { const error = new Error( @@ -382,7 +382,7 @@ export default function reducer(state: any = {}, action: any = {}) { case FETCH_USER_FAILURE: return reduceUsersByNames(state, action.payload.username, { - loading: true, + loading: false, error: action.payload.error }); @@ -411,18 +411,12 @@ export default function reducer(state: any = {}, action: any = {}) { }; case DELETE_USER_FAILURE: - const newState = reduceUsersByNames(state, action.payload.user.name, { + return 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 - } - }; + // Add single user cases case CREATE_USER_PENDING: return { diff --git a/scm-ui/src/users/modules/users.test.js b/scm-ui/src/users/modules/users.test.js index fea3bbe540..d217a38deb 100644 --- a/scm-ui/src/users/modules/users.test.js +++ b/scm-ui/src/users/modules/users.test.js @@ -209,15 +209,11 @@ describe("users fetch()", () => { status: 204 }); - // after create, the users are fetched again - fetchMock.getOnce(USERS_URL, response); - const store = mockStore({}); return store.dispatch(createUser(userZaphod)).then(() => { const actions = store.getActions(); expect(actions[0].type).toEqual(CREATE_USER_PENDING); expect(actions[1].type).toEqual(CREATE_USER_SUCCESS); - expect(actions[2].type).toEqual(FETCH_USERS_PENDING); }); }); @@ -235,6 +231,24 @@ describe("users fetch()", () => { }); }); + it("should call the callback after user successfully created", () => { + // unmatched + fetchMock.postOnce(USERS_URL, { + status: 204 + }); + + let callMe = "not yet"; + + const callback = () => { + callMe = "yeah"; + }; + + const store = mockStore({}); + return store.dispatch(createUser(userZaphod, callback)).then(() => { + expect(callMe).toBe("yeah"); + }); + }); + it("successfully update user", () => { fetchMock.putOnce("http://localhost:8081/scm/api/rest/v2/users/zaphod", { status: 204 @@ -269,8 +283,6 @@ describe("users fetch()", () => { fetchMock.deleteOnce("http://localhost:8081/scm/api/rest/v2/users/zaphod", { status: 204 }); - // after update, the users are fetched again - fetchMock.getOnce(USERS_URL, response); const store = mockStore({}); return store.dispatch(deleteUser(userZaphod)).then(() => { @@ -278,7 +290,6 @@ describe("users fetch()", () => { expect(actions[0].type).toEqual(DELETE_USER); expect(actions[0].payload).toBe(userZaphod); expect(actions[1].type).toEqual(DELETE_USER_SUCCESS); - expect(actions[2].type).toEqual(FETCH_USERS_PENDING); }); }); @@ -458,11 +469,19 @@ describe("users reducer", () => { }); it("should update state according to FETCH_USER_FAILURE action", () => { + const error = new Error("kaputt!"); const newState = reducer( - {}, - fetchUserFailure(userFord.name, new Error("kaputt!")) + { + usersByNames: { + ford: { + loading: true + } + } + }, + fetchUserFailure(userFord.name, error) ); - expect(newState.usersByNames["ford"].error).toBeTruthy(); + expect(newState.usersByNames["ford"].error).toBe(error); + expect(newState.usersByNames["ford"].loading).toBeFalsy(); }); it("should update state according to FETCH_USER_SUCCESS action", () => {