review changes have been implemented: integrated createLink functionalities in BranchButtonGroup, outsourced orderBraches and wrote unit tests

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<Props> {
   render() {
     const { repository, branch, t } = this.props;
 
-    const changesetLink = createChangesetLink(repository, branch);
+    const changesetLink = `/repo/${repository.namespace}/${
-    const sourcesLink = createSourcesLink(repository, branch);
+      repository.name
+    }/branch/${encodeURIComponent(branch.name)}/changesets/`;
+    const sourcesLink = `/repo/${repository.namespace}/${
+      repository.name
+    }/sources/${encodeURIComponent(branch.name)}/`;
 
     return (
       <ButtonGroup>

scm-ui/src/repos/branches/containers/BranchView.js

@@ -18,8 +18,8 @@ type Props = {
   repository: Repository,
   branchName: string,
   loading: boolean,
-  error: Error,
+  error?: Error,
-  branch: Branch,
+  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 loading = isFetchBranchPending(state, repository, branchName);
-  const error = getFetchBranchFailure(state, branchName);
+  const error = getFetchBranchFailure(state, repository, branchName);
   return {
     repository,
     branchName,

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<Props> {
   componentDidMount() {
     const { fetchBranches, repository } = this.props;

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) {
+export function fetchBranches(repository: Repository) {
-  return repository.namespace + "/" + repository.name;
+  if (!repository._links.branches) {
-}
-
-export function fetchBranchPending(
-  repository: Repository,
-  name: string
-): Action {
     return {
-    type: FETCH_BRANCH_PENDING,
+      type: FETCH_BRANCHES_SUCCESS,
-    payload: { repository, name },
+      payload: { repository, data: {} },
-    itemId: createIdentifier(repository) + "/" + name
+      itemId: createKey(repository)
     };
-}
+  }
 
-export function fetchBranchSuccess(
+  return function(dispatch: any) {
-  repository: Repository,
+    dispatch(fetchBranchesPending(repository));
-  branch: Branch
+    return apiClient
-): Action {
+      .get(repository._links.branches.href)
-  return {
+      .then(response => response.json())
-    type: FETCH_BRANCH_SUCCESS,
+      .then(data => {
-    payload: { repository, branch },
+        dispatch(fetchBranchesSuccess(data, repository));
-    itemId: createIdentifier(repository) + "/" + branch.name
+      })
-  };
+      .catch(error => {
-}
+        dispatch(fetchBranchesFailure(repository, error));
-
+      });
-export function fetchBranchFailure(
-  repository: Repository,
-  name: string,
-  error: Error
-): Action {
-  return {
-    type: FETCH_BRANCH_FAILURE,
-    payload: { error, repository, name },
-    itemId: createIdentifier(repository) + "/" + name
   };
 }
 
@@ -84,38 +69,48 @@ export function fetchBranch(
   };
 }
 
-export function isFetchBranchPending(state: Object, name: string) {
+// Selectors
-  return isPending(state, FETCH_BRANCH, name);
-}
 
-export function getFetchBranchFailure(state: Object, name: string) {
+export function getBranches(state: Object, repository: Repository) {
-  return getFailure(state, FETCH_BRANCH, name);
+  const key = createKey(repository);
-}
+  if (state.branches[key]) {
-
+    return state.branches[key];
-export function fetchBranches(repository: Repository) {
-  if (!repository._links.branches) {
-    return {
-      type: FETCH_BRANCHES_SUCCESS,
-      payload: { repository, data: {} },
-      itemId: createKey(repository)
-    };
   }
+  return null;
+}
 
-  return function(dispatch: any) {
+export function getBranch(
-    dispatch(fetchBranchesPending(repository));
+  state: Object,
-    return apiClient
+  repository: Repository,
-      .get(repository._links.branches.href)
+  name: string
-      .then(response => response.json())
+): ?Branch {
-      .then(data => {
+  const key = createKey(repository);
-        dispatch(fetchBranchesSuccess(data, repository));
+  if (state.branches[key]) {
-      })
+    return state.branches[key].find((b: Branch) => b.name === name);
-      .catch(error => {
+  }
-        dispatch(fetchBranchesFailure(repository, error));
+  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) {
+// master, default should always be the first one,
-  return `/repo/${repository.namespace}/${
+// followed by develop the rest should be ordered by its name
-    repository.name
+export function orderBranches(branches: Branch[]) {
-  }/branch/${encodeURIComponent(branch.name)}/changesets/`;
+  branches.sort((a, b) => {
-}
+    if (a.defaultBranch && !b.defaultBranch) {
-
+      return -20;
-export function createSourcesLink(repository: Repository, branch: Branch) {
+    } else if (!a.defaultBranch && b.defaultBranch) {
-  return `/repo/${repository.namespace}/${
+      return 20;
-    repository.name
+    } else if (a.name === "master" && b.name !== "master") {
-  }/sources/${encodeURIComponent(branch.name)}/`;
+      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;
+  });
 }

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]);
+    });
+  });
 });