From 2c82026e86359ec1feeb0c022751c7b6358596c6 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 8 Apr 2019 15:16:37 +0200 Subject: [PATCH] refactoring branches state --- scm-ui/src/repos/branches/modules/branches.js | 110 +++++----- .../repos/branches/modules/branches.test.js | 191 ++++++++++++------ 2 files changed, 186 insertions(+), 115 deletions(-) diff --git a/scm-ui/src/repos/branches/modules/branches.js b/scm-ui/src/repos/branches/modules/branches.js index ffe257a94c..eb1b2bb665 100644 --- a/scm-ui/src/repos/branches/modules/branches.js +++ b/scm-ui/src/repos/branches/modules/branches.js @@ -3,7 +3,6 @@ import { FAILURE_SUFFIX, PENDING_SUFFIX, SUCCESS_SUFFIX, - default as types } from "../../../modules/types"; import { apiClient } from "@scm-manager/ui-components"; import type { Action, Branch, Repository } from "@scm-manager/ui-types"; @@ -103,15 +102,24 @@ export function createBranch( // Selectors export function getBranches(state: Object, repository: Repository) { - const key = createKey(repository); - if (state.branches[key]) { - return state.branches[key]; + const repoState = getRepoState(state, repository); + if (repoState && repoState.list) { + return repoState.list._embedded.branches.map(name => repoState.byName[name]); } - return null; } -export const isPermittedToCreateBranches = (state: Object): boolean => { - return false; // TODO +function getRepoState(state: Object, repository: Repository) { + const key = createKey(repository); + const repoState = state.branches[key]; + if (repoState && repoState.byName) { + return repoState; + } +} + +export const isPermittedToCreateBranches = (state: Object, repository: Repository): boolean => { + const repoState = getRepoState(state, repository); + return !!(repoState && repoState.list && repoState.list._links && repoState.list._links.create); + }; export function getBranch( @@ -119,11 +127,10 @@ export function getBranch( repository: Repository, name: string ): ?Branch { - const key = createKey(repository); - if (state.branches[key]) { - return state.branches[key].find((b: Branch) => b.name === name); + const repoState = getRepoState(state, repository); + if (repoState) { + return repoState.byName[name]; } - return null; } // Action creators @@ -252,58 +259,61 @@ export function fetchBranchFailure( // Reducers -function extractBranchesFromPayload(payload: any) { - if (payload._embedded && payload._embedded.branches) { - return payload._embedded.branches; - } - return []; -} +const reduceByBranchesSuccess = (state, payload) => { + const repository = payload.repository; + const response = payload.data; -function reduceBranchSuccess(state, repositoryName, newBranch) { - const newBranches = []; - // we do not use filter, because we try to keep the current order - let found = false; - for (const branch of state[repositoryName] || []) { - if (branch.name === newBranch.name) { - newBranches.push(newBranch); - found = true; - } else { - newBranches.push(branch); + const key = createKey(repository); + const repoState = state[key] || {}; + const byName = repoState.byName || {}; + repoState.byName = byName; + + const branches = response._embedded.branches; + const names = branches.map(b => b.name); + response._embedded.branches = names; + for (let branch of branches) { + byName[branch.name] = branch; + } + return { + [key]: { + list: response, + byName } - } - if (!found) { - newBranches.push(newBranch); - } - return newBranches; -} + }; +}; -type State = { [string]: Branch[] }; +const reduceByBranchSuccess = (state, payload) => { + const repository = payload.repository; + const branch = payload.branch; + + const key = createKey(repository); + + const repoState = state[key] || {}; + + const byName = repoState.byName || {}; + byName[branch.name] = branch; + + repoState.byName = byName; + + return { + ...state, + [key]: repoState + }; +}; export default function reducer( - state: State = {}, + state: {}, action: Action = { type: "UNKNOWN" } -): State { +): Object { if (!action.payload) { return state; } const payload = action.payload; switch (action.type) { case FETCH_BRANCHES_SUCCESS: - const key = createKey(payload.repository); - return { - ...state, - [key]: extractBranchesFromPayload(payload.data) - }; + return reduceByBranchesSuccess(state, payload); case FETCH_BRANCH_SUCCESS: - if (!action.payload.repository || !action.payload.branch) { - return state; - } - const newBranch = action.payload.branch; - const repositoryName = createKey(action.payload.repository); - return { - ...state, - [repositoryName]: reduceBranchSuccess(state, repositoryName, newBranch) - }; + return reduceByBranchSuccess(state, payload); default: return state; } diff --git a/scm-ui/src/repos/branches/modules/branches.test.js b/scm-ui/src/repos/branches/modules/branches.test.js index 4e5c1d96a9..3f81d8bb5b 100644 --- a/scm-ui/src/repos/branches/modules/branches.test.js +++ b/scm-ui/src/repos/branches/modules/branches.test.js @@ -22,7 +22,7 @@ import reducer, { isFetchBranchesPending, createBranch, isCreateBranchPending, - getCreateBranchFailure + getCreateBranchFailure, isPermittedToCreateBranches } from "./branches"; const namespace = "foo"; @@ -192,6 +192,14 @@ describe("branches", () => { const branches = { _embedded: { branches: [branch1, branch2] + }, + _links: { + self: { + href: "/self" + }, + create: { + href: "/create" + } } }; const action = { @@ -202,82 +210,96 @@ describe("branches", () => { } }; - it("should update state according to successful fetch", () => { + it("should store the branches", () => { const newState = reducer({}, action); - expect(newState).toBeDefined(); - expect(newState[key]).toBeDefined(); - expect(newState[key]).toContain(branch1); - expect(newState[key]).toContain(branch2); + const repoState = newState["foo/bar"]; + + expect(repoState.list._links.create.href).toEqual("/create"); + expect(repoState.list._embedded.branches).toEqual([ "branch1", "branch2" ]); + + expect(repoState.byName.branch1).toEqual(branch1); + expect(repoState.byName.branch2).toEqual(branch2); }); - it("should not delete existing branches from state", () => { - const oldState = { - "hitchhiker/heartOfGold": [branch3] + it("should store a single branch", () => { + const newState = reducer({}, fetchBranchSuccess(repository, branch1)); + const repoState = newState["foo/bar"]; + + expect(repoState.list).toBeUndefined(); + expect(repoState.byName.branch1).toEqual(branch1); + }); + + it("should add a single branch", () => { + const state = { + "foo/bar": { + list: { + _links: { + + }, + _embedded: { + branches: ["branch1"] + } + }, + byName: { + "branch1": branch1 + } + } }; - const newState = reducer(oldState, action); - expect(newState[key]).toContain(branch1); - expect(newState[key]).toContain(branch2); - expect(newState["hitchhiker/heartOfGold"]).toContain(branch3); + const newState = reducer(state, fetchBranchSuccess(repository, branch2)); + const repoState = newState["foo/bar"]; + const byName = repoState.byName; + + expect(repoState.list._embedded.branches).toEqual(["branch1"]); + expect(byName.branch1).toEqual(branch1); + expect(byName.branch2).toEqual(branch2); }); - it("should update state according to FETCH_BRANCH_SUCCESS action", () => { - const newState = reducer({}, fetchBranchSuccess(repository, branch3)); - expect(newState["foo/bar"]).toEqual([branch3]); - }); - - it("should not delete existing branch from state", () => { - const oldState = { - "foo/bar": [branch1] + it("should not overwrite non related repositories", () => { + const state = { + "scm/core": { + byName: { + "branch1": branch1 + } + } }; - const newState = reducer( - oldState, - fetchBranchSuccess(repository, branch2) - ); - expect(newState["foo/bar"]).toEqual([branch1, branch2]); + const newState = reducer(state, fetchBranchSuccess(repository, branch1)); + const byName = newState["scm/core"].byName; + + expect(byName.branch1).toEqual(branch1); }); - it("should update required branch from state", () => { - const oldState = { - "foo/bar": [branch1] + it("should overwrite existing branch", () => { + const state = { + "foo/bar": { + byName: { + "branch1": { + "name": "branch1", + "revision": "xyz" + } + } + } }; - const newBranch1 = { name: "branch1", revision: "revision2" }; - const newState = reducer( - oldState, - fetchBranchSuccess(repository, newBranch1) - ); - expect(newState["foo/bar"]).toEqual([newBranch1]); + const newState = reducer(state, fetchBranchSuccess(repository, branch1)); + const byName = newState["foo/bar"].byName; + + expect(byName.branch1.revision).toEqual("revision1"); }); - it("should update required branch from state and keeps old repo", () => { - const oldState = { - "ns/one": [branch1] + it("should not overwrite existing branches", () => { + const state = { + "foo/bar": { + byName: { + 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 }); - expect(newState).toBe(state); - }); - - it("should return the oldState, if payload has no branch", () => { - const action = { - type: FETCH_BRANCH_SUCCESS, - payload: { - repository - }, - itemId: "foo/bar/" - }; - const state = {}; const newState = reducer(state, action); - expect(newState).toBe(state); + expect(newState["foo/bar"].byName.branch1).toEqual(branch1); + expect(newState["foo/bar"].byName.branch2).toEqual(branch2); + expect(newState["foo/bar"].byName.branch3).toEqual(branch3); }); + }); describe("branch selectors", () => { @@ -285,7 +307,20 @@ describe("branches", () => { const state = { branches: { - [key]: [branch1, branch2] + "foo/bar": { + list: { + _links: { + + }, + _embedded: { + branches: ["branch1", "branch2"] + } + }, + byName: { + "branch1": branch1, + "branch2": branch2 + } + } } }; @@ -312,9 +347,9 @@ describe("branches", () => { expect(one).toBe(two); }); - it("should return null, if no branches for the repository available", () => { + it("should return undefined, if no branches for the repository available", () => { const branches = getBranches({ branches: {} }, repository); - expect(branches).toBeNull(); + expect(branches).toBeUndefined(); }); it("should return single branch by name", () => { @@ -333,6 +368,32 @@ describe("branches", () => { expect(branch).toBeUndefined(); }); + it("should return true if the branches list contains the create link", () => { + const stateWithLink = { + branches: { + "foo/bar": { + ...state.branches["foo/bar"], + list: { + ...state.branches["foo/bar"].list, + _links: { + create: { + href: "http://create-it" + } + } + }, + } + } + }; + + const permitted = isPermittedToCreateBranches(stateWithLink, repository); + expect(permitted).toBe(true); + }); + + it("should return false if the create link is missing", () => { + const permitted = isPermittedToCreateBranches(state, repository); + expect(permitted).toBe(false); + }); + it("should return error if fetching branches failed", () => { const state = { failure: { @@ -374,7 +435,7 @@ describe("branches", () => { expect(getCreateBranchFailure(state)).toEqual(error); }); - it("shouls return undefined when create branch did not fail", () => { + it("should return undefined when create branch did not fail", () => { expect(getCreateBranchFailure({})).toBe(undefined); }); });