From 148f05daeeb3eeb5087923c70af90f515bb6922e Mon Sep 17 00:00:00 2001 From: Florian Scholdei Date: Wed, 3 Apr 2019 10:11:43 +0200 Subject: [PATCH] review changes have been implemented: integrated createLink functionalities in BranchButtonGroup, outsourced orderBraches and wrote unit tests --- .../branches/components/BranchButtonGroup.js | 9 +- .../repos/branches/containers/BranchView.js | 8 +- .../branches/containers/BranchesOverview.js | 32 +-- scm-ui/src/repos/branches/modules/branches.js | 235 +++++++++--------- .../repos/branches/modules/branches.test.js | 59 ++++- 5 files changed, 189 insertions(+), 154 deletions(-) diff --git a/scm-ui/src/repos/branches/components/BranchButtonGroup.js b/scm-ui/src/repos/branches/components/BranchButtonGroup.js index dc170963e9..424d4fd156 100644 --- a/scm-ui/src/repos/branches/components/BranchButtonGroup.js +++ b/scm-ui/src/repos/branches/components/BranchButtonGroup.js @@ -3,7 +3,6 @@ import React from "react"; import type { Repository, Branch } from "@scm-manager/ui-types"; import { ButtonGroup, Button } from "@scm-manager/ui-components"; import { translate } from "react-i18next"; -import { createChangesetLink, createSourcesLink } from "../modules/branches"; type Props = { repository: Repository, @@ -17,8 +16,12 @@ class BranchButtonGroup extends React.Component { render() { const { repository, branch, t } = this.props; - const changesetLink = createChangesetLink(repository, branch); - const sourcesLink = createSourcesLink(repository, branch); + const changesetLink = `/repo/${repository.namespace}/${ + repository.name + }/branch/${encodeURIComponent(branch.name)}/changesets/`; + const sourcesLink = `/repo/${repository.namespace}/${ + repository.name + }/sources/${encodeURIComponent(branch.name)}/`; return ( diff --git a/scm-ui/src/repos/branches/containers/BranchView.js b/scm-ui/src/repos/branches/containers/BranchView.js index 542cbd67d2..5e7fa9610a 100644 --- a/scm-ui/src/repos/branches/containers/BranchView.js +++ b/scm-ui/src/repos/branches/containers/BranchView.js @@ -18,8 +18,8 @@ type Props = { repository: Repository, branchName: string, loading: boolean, - error: Error, - branch: Branch, + error?: Error, + branch?: Branch, // dispatch functions fetchBranch: (repository: Repository, branchName: string) => void, @@ -72,8 +72,8 @@ const mapStateToProps = (state, ownProps) => { const { repository } = ownProps; const branchName = decodeURIComponent(ownProps.match.params.branch); const branch = getBranch(state, repository, branchName); - const loading = isFetchBranchPending(state, branchName); - const error = getFetchBranchFailure(state, branchName); + const loading = isFetchBranchPending(state, repository, branchName); + const error = getFetchBranchFailure(state, repository, branchName); return { repository, branchName, diff --git a/scm-ui/src/repos/branches/containers/BranchesOverview.js b/scm-ui/src/repos/branches/containers/BranchesOverview.js index 4f4ca4a75d..3bc92deab4 100644 --- a/scm-ui/src/repos/branches/containers/BranchesOverview.js +++ b/scm-ui/src/repos/branches/containers/BranchesOverview.js @@ -4,7 +4,8 @@ import { fetchBranches, getBranches, getFetchBranchesFailure, - isFetchBranchesPending + isFetchBranchesPending, + orderBranches } from "../modules/branches"; import { connect } from "react-redux"; import type { Branch, Repository } from "@scm-manager/ui-types"; @@ -36,35 +37,6 @@ type Props = { t: string => string }; -// master, default should always be the first one, -// followed by develop the rest should be ordered by its name -export function orderBranches(branches: Branch[]) { - branches.sort((a, b) => { - if (a.defaultBranch && !b.defaultBranch) { - return -20; - } else if (!a.defaultBranch && b.defaultBranch) { - return 20; - } else if (a.name === "master" && b.name !== "master") { - return -10; - } else if (a.name !== "master" && b.name === "master") { - return 10; - } else if (a.name === "default" && b.name !== "default") { - return -10; - } else if (a.name !== "default" && b.name === "default") { - return 10; - } else if (a.name === "develop" && b.name !== "develop") { - return -5; - } else if (a.name !== "develop" && b.name === "develop") { - return 5; - } else if (a.name < b.name) { - return -1; - } else if (a.name > b.name) { - return 1; - } - return 0; - }); -} - class BranchesOverview extends React.Component { componentDidMount() { const { fetchBranches, repository } = this.props; diff --git a/scm-ui/src/repos/branches/modules/branches.js b/scm-ui/src/repos/branches/modules/branches.js index dd542d0183..a5f676eced 100644 --- a/scm-ui/src/repos/branches/modules/branches.js +++ b/scm-ui/src/repos/branches/modules/branches.js @@ -21,41 +21,26 @@ export const FETCH_BRANCH_FAILURE = `${FETCH_BRANCH}_${FAILURE_SUFFIX}`; // Fetching branches -function createIdentifier(repository: Repository) { - return repository.namespace + "/" + repository.name; -} +export function fetchBranches(repository: Repository) { + if (!repository._links.branches) { + return { + type: FETCH_BRANCHES_SUCCESS, + payload: { repository, data: {} }, + itemId: createKey(repository) + }; + } -export function fetchBranchPending( - repository: Repository, - name: string -): Action { - return { - type: FETCH_BRANCH_PENDING, - payload: { repository, name }, - itemId: createIdentifier(repository) + "/" + name - }; -} - -export function fetchBranchSuccess( - repository: Repository, - branch: Branch -): Action { - return { - type: FETCH_BRANCH_SUCCESS, - payload: { repository, branch }, - itemId: createIdentifier(repository) + "/" + branch.name - }; -} - -export function fetchBranchFailure( - repository: Repository, - name: string, - error: Error -): Action { - return { - type: FETCH_BRANCH_FAILURE, - payload: { error, repository, name }, - itemId: createIdentifier(repository) + "/" + name + return function(dispatch: any) { + dispatch(fetchBranchesPending(repository)); + return apiClient + .get(repository._links.branches.href) + .then(response => response.json()) + .then(data => { + dispatch(fetchBranchesSuccess(data, repository)); + }) + .catch(error => { + dispatch(fetchBranchesFailure(repository, error)); + }); }; } @@ -84,38 +69,48 @@ export function fetchBranch( }; } -export function isFetchBranchPending(state: Object, name: string) { - return isPending(state, FETCH_BRANCH, name); -} +// Selectors -export function getFetchBranchFailure(state: Object, name: string) { - return getFailure(state, FETCH_BRANCH, name); -} - -export function fetchBranches(repository: Repository) { - if (!repository._links.branches) { - return { - type: FETCH_BRANCHES_SUCCESS, - payload: { repository, data: {} }, - itemId: createKey(repository) - }; +export function getBranches(state: Object, repository: Repository) { + const key = createKey(repository); + if (state.branches[key]) { + return state.branches[key]; } + return null; +} - return function(dispatch: any) { - dispatch(fetchBranchesPending(repository)); - return apiClient - .get(repository._links.branches.href) - .then(response => response.json()) - .then(data => { - dispatch(fetchBranchesSuccess(data, repository)); - }) - .catch(error => { - dispatch(fetchBranchesFailure(repository, error)); - }); - }; +export function getBranch( + state: Object, + repository: Repository, + name: string +): ?Branch { + const key = createKey(repository); + if (state.branches[key]) { + return state.branches[key].find((b: Branch) => b.name === name); + } + return null; } // Action creators +export function isFetchBranchesPending( + state: Object, + repository: Repository +): boolean { + return isPending(state, FETCH_BRANCHES, createKey(repository)); +} + +export function getFetchBranchesFailure(state: Object, repository: Repository) { + return getFailure(state, FETCH_BRANCHES, createKey(repository)); +} + +export function isFetchBranchPending(state: Object, repository: Repository, name: string) { + return isPending(state, FETCH_BRANCH, createKey(repository) + "/" + name); +} + +export function getFetchBranchFailure(state: Object, repository: Repository, name: string) { + return getFailure(state, FETCH_BRANCH, createKey(repository) + "/" + name); +} + export function fetchBranchesPending(repository: Repository) { return { type: FETCH_BRANCHES_PENDING, @@ -140,8 +135,49 @@ export function fetchBranchesFailure(repository: Repository, error: Error) { }; } +export function fetchBranchPending( + repository: Repository, + name: string +): Action { + return { + type: FETCH_BRANCH_PENDING, + payload: { repository, name }, + itemId: createKey(repository) + "/" + name + }; +} + +export function fetchBranchSuccess( + repository: Repository, + branch: Branch +): Action { + return { + type: FETCH_BRANCH_SUCCESS, + payload: { repository, branch }, + itemId: createKey(repository) + "/" + branch.name + }; +} + +export function fetchBranchFailure( + repository: Repository, + name: string, + error: Error +): Action { + return { + type: FETCH_BRANCH_FAILURE, + payload: { error, repository, name }, + itemId: createKey(repository) + "/" + name + }; +} + // Reducers +function extractBranchesFromPayload(payload: any) { + if (payload._embedded && payload._embedded.branches) { + return payload._embedded.branches; + } + return []; +} + function reduceBranchSuccess(state, repositoryName, newBranch) { const newBranches = []; // we do not use filter, because we try to keep the current order @@ -182,7 +218,7 @@ export default function reducer( return state; } const newBranch = action.payload.branch; - const repositoryName = createIdentifier(action.payload.repository); + const repositoryName = createKey(action.payload.repository); return { ...state, [repositoryName]: reduceBranchSuccess(state, repositoryName, newBranch) @@ -192,59 +228,36 @@ export default function reducer( } } -function extractBranchesFromPayload(payload: any) { - if (payload._embedded && payload._embedded.branches) { - return payload._embedded.branches; - } - return []; -} - -// Selectors - -export function getBranches(state: Object, repository: Repository) { - const key = createKey(repository); - if (state.branches[key]) { - return state.branches[key]; - } - return null; -} - -export function getBranch( - state: Object, - repository: Repository, - name: string -): ?Branch { - const key = createKey(repository); - if (state.branches[key]) { - return state.branches[key].find((b: Branch) => b.name === name); - } - return null; -} - -export function isFetchBranchesPending( - state: Object, - repository: Repository -): boolean { - return isPending(state, FETCH_BRANCHES, createKey(repository)); -} - -export function getFetchBranchesFailure(state: Object, repository: Repository) { - return getFailure(state, FETCH_BRANCHES, createKey(repository)); -} - function createKey(repository: Repository): string { const { namespace, name } = repository; return `${namespace}/${name}`; } -export function createChangesetLink(repository: Repository, branch: Branch) { - return `/repo/${repository.namespace}/${ - repository.name - }/branch/${encodeURIComponent(branch.name)}/changesets/`; -} - -export function createSourcesLink(repository: Repository, branch: Branch) { - return `/repo/${repository.namespace}/${ - repository.name - }/sources/${encodeURIComponent(branch.name)}/`; +// master, default should always be the first one, +// followed by develop the rest should be ordered by its name +export function orderBranches(branches: Branch[]) { + branches.sort((a, b) => { + if (a.defaultBranch && !b.defaultBranch) { + return -20; + } else if (!a.defaultBranch && b.defaultBranch) { + return 20; + } else if (a.name === "master" && b.name !== "master") { + return -10; + } else if (a.name !== "master" && b.name === "master") { + return 10; + } else if (a.name === "default" && b.name !== "default") { + return -10; + } else if (a.name !== "default" && b.name === "default") { + return 10; + } else if (a.name === "develop" && b.name !== "develop") { + return -5; + } else if (a.name !== "develop" && b.name === "develop") { + return 5; + } else if (a.name < b.name) { + return -1; + } else if (a.name > b.name) { + return 1; + } + return 0; + }); } diff --git a/scm-ui/src/repos/branches/modules/branches.test.js b/scm-ui/src/repos/branches/modules/branches.test.js index 30b81975d6..8db4f6f83a 100644 --- a/scm-ui/src/repos/branches/modules/branches.test.js +++ b/scm-ui/src/repos/branches/modules/branches.test.js @@ -16,7 +16,8 @@ import reducer, { getBranch, getBranches, getFetchBranchesFailure, - isFetchBranchesPending + isFetchBranchesPending, + orderBranches } from "./branches"; const namespace = "foo"; @@ -34,7 +35,18 @@ const repository = { const branch1 = { name: "branch1", revision: "revision1" }; const branch2 = { name: "branch2", revision: "revision2" }; -const branch3 = { name: "branch3", revision: "revision3" }; +const branch3 = { name: "branch3", revision: "revision3", defaultBranch: true }; +const defaultBranch = { + name: "default", + revision: "revision4", + defaultBranch: false +}; +const developBranch = { + name: "develop", + revision: "revision5", + defaultBranch: false +}; +const masterBranch = { name: "master", revision: "revision6", defaultBranch: false }; describe("branches", () => { describe("fetch branches", () => { @@ -163,7 +175,10 @@ describe("branches", () => { const oldState = { "foo/bar": [branch1] }; - const newState = reducer(oldState, fetchBranchSuccess(repository, branch2)); + const newState = reducer( + oldState, + fetchBranchSuccess(repository, branch2) + ); expect(newState["foo/bar"]).toEqual([branch1, branch2]); }); @@ -172,7 +187,10 @@ describe("branches", () => { "foo/bar": [branch1] }; const newBranch1 = { name: "branch1", revision: "revision2" }; - const newState = reducer(oldState, fetchBranchSuccess(repository, newBranch1)); + const newState = reducer( + oldState, + fetchBranchSuccess(repository, newBranch1) + ); expect(newState["foo/bar"]).toEqual([newBranch1]); }); @@ -180,14 +198,17 @@ describe("branches", () => { const oldState = { "ns/one": [branch1] }; - const newState = reducer(oldState, fetchBranchSuccess(repository, branch3)); + const newState = reducer( + oldState, + fetchBranchSuccess(repository, branch3) + ); expect(newState["ns/one"]).toEqual([branch1]); expect(newState["foo/bar"]).toEqual([branch3]); }); it("should return the oldState, if action has no payload", () => { const state = {}; - const newState = reducer(state, {type: FETCH_BRANCH_SUCCESS}); + const newState = reducer(state, { type: FETCH_BRANCH_SUCCESS }); expect(newState).toBe(state); }); @@ -272,4 +293,30 @@ describe("branches", () => { expect(getFetchBranchesFailure({}, repository)).toBeUndefined(); }); }); + + describe("sort branches", () => { + it("should return branches", () => { + let branches = [branch1, branch2]; + orderBranches(branches); + expect(branches).toEqual([branch1, branch2]); + }); + + it("should return defaultBranch first", () => { + let branches = [branch1, branch2, branch3]; + orderBranches(branches); + expect(branches).toEqual([branch3, branch1, branch2]); + }); + + it("should order special branches as follows: master > default > develop", () => { + let branches = [defaultBranch, developBranch, masterBranch]; + orderBranches(branches); + expect(branches).toEqual([masterBranch, defaultBranch, developBranch]); + }); + + it("should order special branches but starting with defaultBranch", () => { + let branches = [masterBranch, developBranch, defaultBranch, branch3]; + orderBranches(branches); + expect(branches).toEqual([branch3, masterBranch, defaultBranch, developBranch]); + }); + }); });