Fixed issues noticed in review

2025-11-16 10:16:16 +01:00 · 2018-10-08 17:34:11 +02:00
parent e58269444b
commit dbe9ee59a8
14 changed files with 160 additions and 104 deletions
--- a/scm-ui/src/repos/components/changesets/ChangesetAuthor.js
+++ b/scm-ui/src/repos/components/changesets/ChangesetAuthor.js
@@ -9,19 +9,25 @@ type Props = {

 export default class ChangesetAuthor extends React.Component<Props> {
  render() {
-    const { changeset } = this.props;
+    const { name } = this.props.changeset.author;
+
    return (
      <>
-        {changeset.author.name}{" "}
-        <a
-          className="is-hidden-mobile"
-          href={"mailto:" + changeset.author.mail}
-        >
-          &lt;
-          {changeset.author.mail}
-          &gt;
-        </a>
+        {name} {this.renderMail()}
      </>
    );
  }
+
+  renderMail() {
+    const { mail } = this.props.changeset.author;
+    if (mail) {
+      return (
+        <a className="is-hidden-mobile" href={"mailto:" + mail}>
+          &lt;
+          {mail}
+          &gt;
+        </a>
+      );
+    }
+  }
 }
--- a/scm-ui/src/repos/components/changesets/ChangesetAvatar.js
+++ b/scm-ui/src/repos/components/changesets/ChangesetAvatar.js
@@ -1,8 +1,7 @@
 //@flow
 import React from "react";
 import { ExtensionPoint } from "@scm-manager/ui-extensions";
-import type { Changeset } from "../../../../../scm-ui-components/packages/ui-types/src/index";
-import { Image } from "../../../../../scm-ui-components/packages/ui-components/src/index";
+import type { Changeset } from "@scm-manager/ui-types";

 type Props = {
  changeset: Changeset
--- a/scm-ui/src/repos/components/changesets/ChangesetList.js
+++ b/scm-ui/src/repos/components/changesets/ChangesetList.js
@@ -1,7 +1,7 @@
 // @flow
 import ChangesetRow from "./ChangesetRow";
 import React from "react";
-import type { Changeset, Repository } from "../../../../../scm-ui-components/packages/ui-types/src/index";
+import type { Changeset, Repository } from "@scm-manager/ui-types";
 import classNames from "classnames";

 type Props = {
@@ -13,7 +13,13 @@ class ChangesetList extends React.Component<Props> {
  render() {
    const { repository, changesets } = this.props;
    const content = changesets.map((changeset, index) => {
-      return <ChangesetRow key={index} repository={repository} changeset={changeset} />;
+      return (
+        <ChangesetRow
+          key={index}
+          repository={repository}
+          changeset={changeset}
+        />
+      );
    });
    return <div className={classNames("box")}>{content}</div>;
  }
--- a/scm-ui/src/repos/components/changesets/ChangesetRow.js
+++ b/scm-ui/src/repos/components/changesets/ChangesetRow.js
@@ -1,15 +1,12 @@
 //@flow
 import React from "react";
-import type {
-  Changeset,
-  Repository
-} from "../../../../../scm-ui-components/packages/ui-types/src/index";
+import type { Changeset, Repository } from "@scm-manager/ui-types";
 import classNames from "classnames";
 import { translate, Interpolate } from "react-i18next";
 import ChangesetAvatar from "./ChangesetAvatar";
 import ChangesetId from "./ChangesetId";
 import injectSheet from "react-jss";
-import { DateFromNow } from "../../../../../scm-ui-components/packages/ui-components/src/index";
+import { DateFromNow } from "@scm-manager/ui-components";
 import ChangesetAuthor from "./ChangesetAuthor";

 const styles = {
--- a/scm-ui/src/repos/containers/BranchChooser.js
+++ b/scm-ui/src/repos/containers/BranchChooser.js
@@ -1,18 +1,22 @@
 // @flow

 import React from "react";
-import type {Repository} from "@scm-manager/ui-types";
-import {connect} from "react-redux";
-import {fetchBranches, getBranches, isFetchBranchesPending} from "../modules/branches";
+import type { Repository, Branch } from "@scm-manager/ui-types";
+import { connect } from "react-redux";
+import {
+  fetchBranches,
+  getBranches,
+  isFetchBranchesPending
+} from "../modules/branches";

-import {Loading} from "@scm-manager/ui-components";
+import { Loading } from "@scm-manager/ui-components";
 import DropDown from "../components/DropDown";

 type Props = {
  repository: Repository,
  fetchBranches: Repository => void,
-  callback: Branch => void, //TODO use correct branch type
-  branches: Branch[], //TODO use correct branch type
+  callback: (?Branch) => void,
+  branches: Branch[],
  selectedBranchName: string,
  loading: boolean
 };
--- a/scm-ui/src/repos/containers/Changesets.js
+++ b/scm-ui/src/repos/containers/Changesets.js
@@ -1,8 +1,12 @@
 // @flow
 import React from "react";
-import {connect} from "react-redux";
-import {translate} from "react-i18next";
-import {ErrorNotification, Loading, Paginator} from "@scm-manager/ui-components";
+import { connect } from "react-redux";
+import { translate } from "react-i18next";
+import {
+  ErrorNotification,
+  Loading,
+  Paginator
+} from "@scm-manager/ui-components";

 import {
  fetchChangesets,
@@ -14,11 +18,16 @@ import {
  isFetchChangesetsPending,
  selectListAsCollection
 } from "../modules/changesets";
-import type {History} from "history";
-import type {Changeset, PagedCollection, Repository} from "@scm-manager/ui-types";
+import type { History } from "history";
+import type {
+  Changeset,
+  PagedCollection,
+  Repository,
+  Branch
+} from "@scm-manager/ui-types";
 import ChangesetList from "../components/changesets/ChangesetList";
-import {withRouter} from "react-router-dom";
-import {fetchBranches, getBranch, getBranchNames} from "../modules/branches";
+import { withRouter } from "react-router-dom";
+import { fetchBranches, getBranch, getBranchNames } from "../modules/branches";
 import BranchChooser from "./BranchChooser";

 type Props = {
@@ -34,7 +43,7 @@ type Props = {
  list: PagedCollection,
  fetchChangesetsByLink: (Repository, string, Branch) => void,
  fetchChangesetsByPage: (Repository, number) => void,
-  fetchChangesetsByBranchAndPage: (Repository, string, number) => void,
+  fetchChangesetsByBranchAndPage: (Repository, Branch, number) => void,
  fetchBranches: Repository => void,
  page: number,
  t: string => string,
@@ -63,7 +72,9 @@ class Changesets extends React.PureComponent<Props, State> {
  };

  componentDidMount() {
-    this.updateContent();
+    if (!this.props.loading) {
+      this.updateContent();
+    }
  }

  updateContent() {
@@ -81,15 +92,14 @@ class Changesets extends React.PureComponent<Props, State> {
    }
  }

-  componentDidUpdate(prevProps: Props, prevState: State, snapshot: any) {
+  componentDidUpdate(prevProps: Props) {
    const { page, list, repository, match } = this.props;
    const { namespace, name } = repository;
-    const branch = match.params.branch;
-
+    const branch = decodeURIComponent(match.params.branch);
    if (!this.props.loading) {
      if (prevProps.branch !== this.props.branch) {
-        this.updateContent();
        this.setState({ branch });
+        this.updateContent();
      }

      if (list && (list.page || list.page === 0)) {
@@ -98,11 +108,11 @@ class Changesets extends React.PureComponent<Props, State> {
        if (page !== statePage) {
          if (branch) {
            this.props.history.push(
-              `/repo/${namespace}/${name}/${branch}/history/${statePage}`
+              `/repo/${namespace}/${name}/${branch}/changesets/${statePage}`
            );
          } else {
            this.props.history.push(
-              `/repo/${namespace}/${name}/history/${statePage}`
+              `/repo/${namespace}/${name}/changesets/${statePage}`
            );
          }
        }
@@ -129,7 +139,7 @@ class Changesets extends React.PureComponent<Props, State> {
  }

  renderList = () => {
-    const branch = this.props.match.params.branch;
+    const branch = decodeURIComponent(this.props.match.params.branch);
    const { repository, changesets, t } = this.props;

    return (
@@ -160,12 +170,16 @@ class Changesets extends React.PureComponent<Props, State> {
  branchChanged = (branch: Branch): void => {
    const { history, repository } = this.props;
    if (branch === undefined) {
-      history.push(`/repo/${repository.namespace}/${repository.name}/history`);
+      history.push(
+        `/repo/${repository.namespace}/${repository.name}/changesets`
+      );
    } else {
-      const branchName = branch.name;
+      const branchName = encodeURIComponent(branch.name);
      this.setState({ branch: branchName });
      history.push(
-        `/repo/${repository.namespace}/${repository.name}/${branchName}/history`
+        `/repo/${repository.namespace}/${
+          repository.name
+        }/${branchName}/changesets`
      );
    }
  };
@@ -185,12 +199,11 @@ const mapStateToProps = (state, ownProps: Props) => {
  const { repository } = ownProps;
  const branchName = ownProps.match.params.branch;
  const branch = getBranch(state, repository, branchName);
-  const key = createKey(repository, branch);
  const loading = isFetchChangesetsPending(state, repository, branch);
-  const changesets = getChangesetsFromState(state, key);
+  const changesets = getChangesetsFromState(state, repository);
  const branchNames = getBranchNames(state, repository);
  const error = getFetchChangesetsFailure(state, repository, branch);
-  const list = selectListAsCollection(state, key);
+  const list = selectListAsCollection(state, repository);
  const page = getPageFromProps(ownProps);

  return {
@@ -204,15 +217,6 @@ const mapStateToProps = (state, ownProps: Props) => {
  };
 };

-const createKey = (repository: Repository, branch?: Branch): string => {
-  const { namespace, name } = repository;
-  let key = `${namespace}/${name}`;
-  if (branch && branch.name) {
-    key = key + `/${branch.name}`;
-  }
-  return key;
-};
-
 const mapDispatchToProps = dispatch => {
  return {
    fetchBranches: (repository: Repository) => {
@@ -226,7 +230,7 @@ const mapDispatchToProps = dispatch => {
    },
    fetchChangesetsByBranchAndPage: (
      repository,
-      branch: string,
+      branch: Branch,
      page: number
    ) => {
      dispatch(fetchChangesetsByBranchAndPage(repository, branch, page));
@@ -234,7 +238,7 @@ const mapDispatchToProps = dispatch => {
    fetchChangesetsByLink: (
      repository: Repository,
      link: string,
-      branch?: string
+      branch?: Branch
    ) => {
      dispatch(fetchChangesetsByLink(repository, link, branch));
    }
--- a/scm-ui/src/repos/containers/RepositoryRoot.js
+++ b/scm-ui/src/repos/containers/RepositoryRoot.js
@@ -104,22 +104,22 @@ class RepositoryRoot extends React.Component<Props> {
            />
            <Route
              exact
-              path={`${url}/history`}
+              path={`${url}/changesets`}
              render={() => <Changesets repository={repository} />}
            />
            <Route
              exact
-              path={`${url}/history/:page`}
+              path={`${url}/changesets/:page`}
              render={() => <Changesets repository={repository} />}
            />
            <Route
              exact
-              path={`${url}/:branch/history`}
+              path={`${url}/:branch/changesets`}
              render={() => <Changesets repository={repository} />}
            />
            <Route
              exact
-              path={`${url}/:branch/history/:page`}
+              path={`${url}/:branch/changesets/:page`}
              render={() => <Changesets repository={repository} />}
            />
          </div>
@@ -129,7 +129,7 @@ class RepositoryRoot extends React.Component<Props> {
                <NavLink to={url} label={t("repository-root.information")} />
                <NavLink
                  activeOnlyWhenExact={false}
-                  to={`${url}/history`}
+                  to={`${url}/changesets`}
                  label={t("repository-root.history")}
                />
                <EditNavLink repository={repository} editUrl={`${url}/edit`} />
--- a/scm-ui/src/repos/modules/branches.js
+++ b/scm-ui/src/repos/modules/branches.js
@@ -5,7 +5,7 @@ import {
  SUCCESS_SUFFIX
 } from "../../modules/types";
 import { apiClient } from "@scm-manager/ui-components";
-import type { Repository } from "@scm-manager/ui-types";
+import type { Repository, Action, Branch } from "@scm-manager/ui-types";
 import { isPending } from "../../modules/pending";

 export const FETCH_BRANCHES = "scm/repos/FETCH_BRANCHES";
@@ -16,6 +16,14 @@ export const FETCH_BRANCHES_FAILURE = `${FETCH_BRANCHES}_${FAILURE_SUFFIX}`;
 // Fetching branches

 export function fetchBranches(repository: Repository) {
+  if (!repository._links.branches) {
+    return {
+      type: FETCH_BRANCHES_SUCCESS,
+      payload: { repository, data: {} },
+      itemId: createKey(repository)
+    };
+  }
+
  return function(dispatch: any) {
    dispatch(fetchBranchesPending(repository));
    return apiClient
@@ -81,7 +89,10 @@ export default function reducer(
  }
 }

-function extractBranchesByNames(data: any, oldBranchesByNames: any): Branch[] {
+function extractBranchesByNames(data: any, oldBranchesByNames: any): ?Object {
+  if (!data._embedded || !data._embedded.branches) {
+    return {};
+  }
  const branches = data._embedded.branches;
  const branchesByNames = {};

@@ -97,10 +108,13 @@ function extractBranchesByNames(data: any, oldBranchesByNames: any): Branch[] {

 // Selectors

-export function getBranchNames(state: Object, repository: Repository) {
+export function getBranchNames(
+  state: Object,
+  repository: Repository
+): ?Array<Branch> {
  const key = createKey(repository);
  if (!state.branches[key] || !state.branches[key].byNames) {
-    return null;
+    return [];
  }
  return Object.keys(state.branches[key].byNames);
 }
@@ -114,7 +128,11 @@ export function getBranches(state: Object, repository: Repository) {
  }
 }

-export function getBranch(state: Object, repository: Repository, name: string) {
+export function getBranch(
+  state: Object,
+  repository: Repository,
+  name: string
+): ?Branch {
  const key = createKey(repository);
  if (state.branches[key]) {
    if (state.branches[key].byNames[name]) {
@@ -124,11 +142,14 @@ export function getBranch(state: Object, repository: Repository, name: string) {
  return undefined;
 }

-export function isFetchBranchesPending(state: Object, repository: Repository) {
+export function isFetchBranchesPending(
+  state: Object,
+  repository: Repository
+): boolean {
  return isPending(state, FETCH_BRANCHES, createKey(repository));
 }

-function createKey(repository: Repository) {
+function createKey(repository: Repository): string {
  const { namespace, name } = repository;
  return `${namespace}/${name}`;
 }
--- a/scm-ui/src/repos/modules/changesets.js
+++ b/scm-ui/src/repos/modules/changesets.js
@@ -1,11 +1,21 @@
 // @flow

-import {FAILURE_SUFFIX, PENDING_SUFFIX, SUCCESS_SUFFIX} from "../../modules/types";
-import {apiClient} from "@scm-manager/ui-components";
-import {isPending} from "../../modules/pending";
-import {getFailure} from "../../modules/failure";
-import {combineReducers} from "redux";
-import type {Action, Changeset, PagedCollection, Repository} from "@scm-manager/ui-types";
+import {
+  FAILURE_SUFFIX,
+  PENDING_SUFFIX,
+  SUCCESS_SUFFIX
+} from "../../modules/types";
+import { apiClient } from "@scm-manager/ui-components";
+import { isPending } from "../../modules/pending";
+import { getFailure } from "../../modules/failure";
+import { combineReducers } from "redux";
+import type {
+  Action,
+  Changeset,
+  PagedCollection,
+  Repository,
+  Branch
+} from "@scm-manager/ui-types";

 export const FETCH_CHANGESETS = "scm/repos/FETCH_CHANGESETS";
 export const FETCH_CHANGESETS_PENDING = `${FETCH_CHANGESETS}_${PENDING_SUFFIX}`;
@@ -69,7 +79,7 @@ export function fetchChangesets(repository: Repository) {
 }

 export function fetchChangesetsByPage(repository: Repository, page: number) {
-  return fetchChangesetsWithOptions(repository, "", `?page=${page - 1}`);
+  return fetchChangesetsWithOptions(repository, undefined, `?page=${page - 1}`);
 }

 // TODO: Rewrite code to fetch changesets by branches, adjust tests and let BranchChooser fetch branches
@@ -93,9 +103,7 @@ export function fetchChangesetsPending(
  branch?: Branch
 ): Action {
  const itemId = createItemId(repository, branch);
-  if (!branch) {
-    branch = "";
-  }
+
  return {
    type: FETCH_CHANGESETS_PENDING,
    payload: { repository, branch },
@@ -150,8 +158,13 @@ function byKeyReducer(
      const changesets = action.payload._embedded.changesets;
      const changesetIds = changesets.map(c => c.id);
      const key = action.itemId;
+
+      if (!key) {
+        return state;
+      }
+
      let oldChangesets = { [key]: {} };
-      if (state[key] !== undefined) {
+      if (state[key]) {
        oldChangesets[key] = state[key];
      }
      const byIds = extractChangesetsByIds(changesets, oldChangesets[key].byId);
@@ -196,7 +209,7 @@ function extractChangesetsByIds(changesets: any, oldChangesetsByIds: any) {
 export function getChangesets(
  state: Object,
  repository: Repository,
-  branch?: string
+  branch?: Branch
 ) {
  const key = createItemId(repository, branch);
  if (!state.changesets.byKey[key]) {
@@ -208,7 +221,7 @@ export function getChangesets(
 export function isFetchChangesetsPending(
  state: Object,
  repository: Repository,
-  branch?: string
+  branch?: Branch
 ) {
  return isPending(state, FETCH_CHANGESETS, createItemId(repository, branch));
 }
@@ -216,20 +229,21 @@ export function isFetchChangesetsPending(
 export function getFetchChangesetsFailure(
  state: Object,
  repository: Repository,
-  branch?: string
+  branch?: Branch
 ) {
  return getFailure(state, FETCH_CHANGESETS, createItemId(repository, branch));
 }

-const selectList = (state: Object, key: string) => {
-  if (state.changesets.byKey[key] && state.changesets.byKey[key].list) {
-    return state.changesets.byKey[key].list;
+const selectList = (state: Object, repository: Repository) => {
+  const itemId = createItemId(repository);
+  if (state.changesets.byKey[itemId] && state.changesets.byKey[itemId].list) {
+    return state.changesets.byKey[itemId].list;
  }
  return {};
 };

-const selectListEntry = (state: Object, key: string): Object => {
-  const list = selectList(state, key);
+const selectListEntry = (state: Object, repository: Repository): Object => {
+  const list = selectList(state, repository);
  if (list.entry) {
    return list.entry;
  }
@@ -238,20 +252,21 @@ const selectListEntry = (state: Object, key: string): Object => {

 export const selectListAsCollection = (
  state: Object,
-  key: string
+  repository: Repository
 ): PagedCollection => {
-  return selectListEntry(state, key);
+  return selectListEntry(state, repository);
 };

-export function getChangesetsFromState(state: Object, key: string) {
-  const changesetIds = selectList(state, key).entries;
+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[key].byId[id]);
+    changesetEntries.push(state.changesets.byKey[itemId].byId[id]);
  }

  return changesetEntries;
--- a/scm-ui/src/repos/modules/changesets.test.js
+++ b/scm-ui/src/repos/modules/changesets.test.js
@@ -20,6 +20,7 @@ import {
 import reducer from "./changesets";
 const branch = {
  name: "specific",
+  revision: "123",
  _links: {
    history: {
      href:
@@ -31,6 +32,7 @@ const branch = {
 const repository = {
  namespace: "foo",
  name: "bar",
+  type: "GIT",
  _links: {
    self: {
      href: "http://scm/api/rest/v2/repositories/foo/bar"
@@ -63,7 +65,7 @@ describe("changesets", () => {
      const expectedActions = [
        {
          type: FETCH_CHANGESETS_PENDING,
-          payload: { repository, branch: "" },
+          payload: { repository },
          itemId: "foo/bar"
        },
        {
@@ -111,7 +113,7 @@ describe("changesets", () => {
      const expectedActions = [
        {
          type: FETCH_CHANGESETS_PENDING,
-          payload: { repository, branch: "" },
+          payload: { repository },
          itemId
        }
      ];
@@ -152,7 +154,7 @@ describe("changesets", () => {
      const expectedActions = [
        {
          type: FETCH_CHANGESETS_PENDING,
-          payload: { repository, branch: "" },
+          payload: { repository },
          itemId: "foo/bar"
        },
        {