From 4cbbc16b6ae65017e6dfad3af383fee729ca9954 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Wed, 17 Oct 2018 10:38:46 +0200 Subject: [PATCH] simplify changesets module --- scm-ui/src/repos/containers/Changesets.js | 74 ++++------------ scm-ui/src/repos/modules/changesets.js | 98 +++++---------------- scm-ui/src/repos/modules/changesets.test.js | 69 ++++++--------- 3 files changed, 64 insertions(+), 177 deletions(-) diff --git a/scm-ui/src/repos/containers/Changesets.js b/scm-ui/src/repos/containers/Changesets.js index 8df046e21a..21417b6715 100644 --- a/scm-ui/src/repos/containers/Changesets.js +++ b/scm-ui/src/repos/containers/Changesets.js @@ -9,13 +9,13 @@ import type { Repository } from "@scm-manager/ui-types"; import { - fetchChangesetsByBranch, - fetchChangesetsByBranchAndPage, + fetchChangesets, getChangesets, getFetchChangesetsFailure, isFetchChangesetsPending, selectListAsCollection } from "../modules/changesets"; + import { connect } from "react-redux"; import ChangesetList from "../components/changesets/ChangesetList"; import { ErrorPage, LinkPaginator, Loading } from "@scm-manager/ui-components"; @@ -24,66 +24,28 @@ import { translate } from "react-i18next"; type Props = { repository: Repository, //TODO: Do we really need/want this here? branch: Branch, + page: number, // State props changesets: Changeset[], - loading: boolean, list: PagedCollection, + loading: boolean, error: Error, // Dispatch props - fetchChangesetsByBranch: (Repository, Branch) => void, - fetchChangesetsByBranchAndPage: (Repository, Branch, number) => void, + fetchChangesets: (Repository, Branch, number) => void, // Context Props - match: any, t: string => string }; -type State = {}; - -class Changesets extends React.Component { +class Changesets extends React.Component { componentDidMount() { - const { - fetchChangesetsByBranch, - fetchChangesetsByBranchAndPage, - repository, - branch, - match - } = this.props; + const { fetchChangesets, repository, branch, page } = this.props; - const { page } = match.params; - if (!branch) { - return; - } - if (!page) { - fetchChangesetsByBranch(repository, branch); - } else { - fetchChangesetsByBranchAndPage(repository, branch, page); - } + fetchChangesets(repository, branch, page); } - // componentDidUpdate(prevProps: Props) { - // const { - // match, - // repository, - // branch, - // fetchChangesetsByBranch, - // fetchChangesetsByBranchAndPage - // } = this.props; - // const { page } = match.params; - // - // if (branch === prevProps.branch) { - // return; - // } - // - // if (!page) { - // fetchChangesetsByBranch(repository, branch); - // } else { - // fetchChangesetsByBranchAndPage(repository, branch, page); - // } - // } - render() { const { changesets, loading, error, t } = this.props; @@ -127,27 +89,25 @@ class Changesets extends React.Component { const mapDispatchToProps = dispatch => { return { - fetchChangesetsByBranch: (repo: Repository, branch: Branch) => { - dispatch(fetchChangesetsByBranch(repo, branch)); - }, - fetchChangesetsByBranchAndPage: ( - repo: Repository, - branch: Branch, - page: number - ) => { - dispatch(fetchChangesetsByBranchAndPage(repo, branch, page)); + fetchChangesets: (repo: Repository, branch: Branch, page: number) => { + dispatch(fetchChangesets(repo, branch, page)); } }; }; const mapStateToProps = (state: any, ownProps: Props) => { - const { repository, branch } = ownProps; + const { repository, branch, match } = ownProps; const changesets = getChangesets(state, repository, branch); const loading = isFetchChangesetsPending(state, repository, branch); const error = getFetchChangesetsFailure(state, repository, branch); const list = selectListAsCollection(state, repository, branch); - return { changesets, list, loading, error }; + + // TODO + const page = parseInt(match.params.page || "1"); + + return { changesets, list, page, loading, error }; }; + export default withRouter( connect( mapStateToProps, diff --git a/scm-ui/src/repos/modules/changesets.js b/scm-ui/src/repos/modules/changesets.js index 925f37af2e..53df4a1099 100644 --- a/scm-ui/src/repos/modules/changesets.js +++ b/scm-ui/src/repos/modules/changesets.js @@ -22,33 +22,34 @@ export const FETCH_CHANGESETS_PENDING = `${FETCH_CHANGESETS}_${PENDING_SUFFIX}`; export const FETCH_CHANGESETS_SUCCESS = `${FETCH_CHANGESETS}_${SUCCESS_SUFFIX}`; export const FETCH_CHANGESETS_FAILURE = `${FETCH_CHANGESETS}_${FAILURE_SUFFIX}`; -const REPO_URL = "repositories"; //TODO: Content type // actions -export function fetchChangesetsByLink( +export function fetchChangesets( repository: Repository, - link: string, - branch?: Branch + branch?: Branch, + page: number ) { + const link = createChangesetsLink(repository, branch, page); + return function(dispatch: any) { dispatch(fetchChangesetsPending(repository, branch)); return apiClient .get(link) .then(response => response.json()) .then(data => { - dispatch(fetchChangesetsSuccess(data, repository, branch)); + dispatch(fetchChangesetsSuccess(repository, branch, data)); }) .catch(cause => { - dispatch(fetchChangesetsFailure(repository, cause, branch)); + dispatch(fetchChangesetsFailure(repository, branch, cause)); }); }; } -export function fetchChangesetsWithOptions( +function createChangesetsLink( repository: Repository, branch?: Branch, - suffix?: string + page: number ) { let link = repository._links.changesets.href; @@ -56,45 +57,10 @@ export function fetchChangesetsWithOptions( link = branch._links.history.href; } - if (suffix) { - link = link + `${suffix}`; + if (page) { + link = link + `?page=${page - 1}`; } - - return function(dispatch: any) { - dispatch(fetchChangesetsPending(repository, branch)); - return apiClient - .get(link) - .then(response => response.json()) - .then(data => { - dispatch(fetchChangesetsSuccess(data, repository, branch)); - }) - .catch(cause => { - dispatch(fetchChangesetsFailure(repository, cause, branch)); - }); - }; -} - -export function fetchChangesets(repository: Repository) { - return fetchChangesetsWithOptions(repository); -} - -export function fetchChangesetsByPage(repository: Repository, page: number) { - return fetchChangesetsWithOptions(repository, undefined, `?page=${page - 1}`); -} - -export function fetchChangesetsByBranchAndPage( - repository: Repository, - branch: Branch, - page: number -) { - return fetchChangesetsWithOptions(repository, branch, `?page=${page - 1}`); -} - -export function fetchChangesetsByBranch( - repository: Repository, - branch: Branch -) { - return fetchChangesetsWithOptions(repository, branch); + return link; } export function fetchChangesetsPending( @@ -105,15 +71,14 @@ export function fetchChangesetsPending( return { type: FETCH_CHANGESETS_PENDING, - payload: { repository, branch }, itemId }; } export function fetchChangesetsSuccess( - changesets: any, repository: Repository, - branch?: Branch + branch?: Branch, + changesets: any ): Action { return { type: FETCH_CHANGESETS_SUCCESS, @@ -124,8 +89,8 @@ export function fetchChangesetsSuccess( function fetchChangesetsFailure( repository: Repository, - error: Error, - branch?: Branch + branch?: Branch, + error: Error ): Action { return { type: FETCH_CHANGESETS_FAILURE, @@ -141,14 +106,14 @@ function fetchChangesetsFailure( function createItemId(repository: Repository, branch?: Branch): string { const { namespace, name } = repository; let itemId = namespace + "/" + name; - if (branch && branch !== "") { + if (branch) { itemId = itemId + "/" + branch.name; } return itemId; } // reducer -function byKeyReducer( +export default function reducer( state: any = {}, action: Action = { type: "UNKNOWN" } ): Object { @@ -186,10 +151,6 @@ function byKeyReducer( } } -export default combineReducers({ - byKey: byKeyReducer -}); - function extractChangesetsByIds(changesets: any) { const changesetsByIds = {}; @@ -207,10 +168,10 @@ export function getChangesets( branch?: Branch ) { const key = createItemId(repository, branch); - if (!state.changesets.byKey[key]) { + if (!state.changesets[key]) { return null; } - return Object.values(state.changesets.byKey[key].byId); + return Object.values(state.changesets[key].byId); } export function isFetchChangesetsPending( @@ -231,8 +192,8 @@ export function getFetchChangesetsFailure( const selectList = (state: Object, repository: Repository, branch?: Branch) => { const itemId = createItemId(repository, branch); - if (state.changesets.byKey[itemId] && state.changesets.byKey[itemId].list) { - return state.changesets.byKey[itemId].list; + if (state.changesets[itemId] && state.changesets[itemId].list) { + return state.changesets[itemId].list; } return {}; }; @@ -256,18 +217,3 @@ export const selectListAsCollection = ( ): PagedCollection => { return selectListEntry(state, repository, branch); }; - -export function getChangesetsFromState(state: Object, repository: Repository) { - const itemId = createItemId(repository); - const changesetIds = selectList(state, repository).entries; - if (!changesetIds) { - return null; - } - const changesetEntries: Changeset[] = []; - - for (let id of changesetIds) { - changesetEntries.push(state.changesets.byKey[itemId].byId[id]); - } - - return changesetEntries; -} diff --git a/scm-ui/src/repos/modules/changesets.test.js b/scm-ui/src/repos/modules/changesets.test.js index fd33efbc7b..6d88315b04 100644 --- a/scm-ui/src/repos/modules/changesets.test.js +++ b/scm-ui/src/repos/modules/changesets.test.js @@ -9,9 +9,6 @@ import reducer, { FETCH_CHANGESETS_PENDING, FETCH_CHANGESETS_SUCCESS, fetchChangesets, - fetchChangesetsByBranch, - fetchChangesetsByBranchAndPage, - fetchChangesetsByPage, fetchChangesetsSuccess, getChangesets, getFetchChangesetsFailure, @@ -68,7 +65,6 @@ describe("changesets", () => { const expectedActions = [ { type: FETCH_CHANGESETS_PENDING, - payload: { repository }, itemId: "foo/bar" }, { @@ -91,7 +87,6 @@ describe("changesets", () => { const expectedActions = [ { type: FETCH_CHANGESETS_PENDING, - payload: { repository, branch }, itemId }, { @@ -102,11 +97,9 @@ describe("changesets", () => { ]; const store = mockStore({}); - return store - .dispatch(fetchChangesetsByBranch(repository, branch)) - .then(() => { - expect(store.getActions()).toEqual(expectedActions); - }); + return store.dispatch(fetchChangesets(repository, branch)).then(() => { + expect(store.getActions()).toEqual(expectedActions); + }); }); it("should fail fetching changesets on error", () => { @@ -116,7 +109,6 @@ describe("changesets", () => { const expectedActions = [ { type: FETCH_CHANGESETS_PENDING, - payload: { repository }, itemId } ]; @@ -136,19 +128,16 @@ describe("changesets", () => { const expectedActions = [ { type: FETCH_CHANGESETS_PENDING, - payload: { repository, branch }, itemId } ]; const store = mockStore({}); - return store - .dispatch(fetchChangesetsByBranch(repository, branch)) - .then(() => { - expect(store.getActions()[0]).toEqual(expectedActions[0]); - expect(store.getActions()[1].type).toEqual(FETCH_CHANGESETS_FAILURE); - expect(store.getActions()[1].payload).toBeDefined(); - }); + return store.dispatch(fetchChangesets(repository, branch)).then(() => { + expect(store.getActions()[0]).toEqual(expectedActions[0]); + expect(store.getActions()[1].type).toEqual(FETCH_CHANGESETS_FAILURE); + expect(store.getActions()[1].payload).toBeDefined(); + }); }); it("should fetch changesets by page", () => { @@ -157,7 +146,6 @@ describe("changesets", () => { const expectedActions = [ { type: FETCH_CHANGESETS_PENDING, - payload: { repository }, itemId: "foo/bar" }, { @@ -168,9 +156,11 @@ describe("changesets", () => { ]; const store = mockStore({}); - return store.dispatch(fetchChangesetsByPage(repository, 5)).then(() => { - expect(store.getActions()).toEqual(expectedActions); - }); + return store + .dispatch(fetchChangesets(repository, undefined, 5)) + .then(() => { + expect(store.getActions()).toEqual(expectedActions); + }); }); it("should fetch changesets by branch and page", () => { @@ -179,7 +169,6 @@ describe("changesets", () => { const expectedActions = [ { type: FETCH_CHANGESETS_PENDING, - payload: { repository, branch }, itemId: "foo/bar/specific" }, { @@ -190,11 +179,9 @@ describe("changesets", () => { ]; const store = mockStore({}); - return store - .dispatch(fetchChangesetsByBranchAndPage(repository, branch, 5)) - .then(() => { - expect(store.getActions()).toEqual(expectedActions); - }); + return store.dispatch(fetchChangesets(repository, branch, 5)).then(() => { + expect(store.getActions()).toEqual(expectedActions); + }); }); }); @@ -220,19 +207,15 @@ describe("changesets", () => { it("should set state to received changesets", () => { const newState = reducer( {}, - fetchChangesetsSuccess(responseBody, repository) + fetchChangesetsSuccess(repository, undefined, responseBody) ); expect(newState).toBeDefined(); - expect(newState.byKey["foo/bar"].byId["changeset1"].author.mail).toEqual( + expect(newState["foo/bar"].byId["changeset1"].author.mail).toEqual( "z@phod.com" ); - expect(newState.byKey["foo/bar"].byId["changeset2"].description).toEqual( - "foo" - ); - expect(newState.byKey["foo/bar"].byId["changeset3"].description).toEqual( - "bar" - ); - expect(newState.byKey["foo/bar"].list).toEqual({ + expect(newState["foo/bar"].byId["changeset2"].description).toEqual("foo"); + expect(newState["foo/bar"].byId["changeset3"].description).toEqual("bar"); + expect(newState["foo/bar"].list).toEqual({ entry: { page: 1, pageTotal: 10, @@ -249,12 +232,10 @@ describe("changesets", () => { it("should get all changesets for a given repository", () => { const state = { changesets: { - byKey: { - "foo/bar": { - byId: { - id1: { id: "id1" }, - id2: { id: "id2" } - } + "foo/bar": { + byId: { + id1: { id: "id1" }, + id2: { id: "id2" } } } }