From 34ac4cf9cd43043689bf892215a5579414e13b8c Mon Sep 17 00:00:00 2001 From: William Durand Date: Wed, 13 Mar 2019 13:06:09 +0100 Subject: [PATCH] render diffs --- src/pages/Compare/index.spec.tsx | 296 ++++++++++++++++++--------- src/pages/Compare/index.tsx | 83 ++++++-- src/pages/Compare/styles.module.scss | 0 src/pages/Index/index.tsx | 2 +- 4 files changed, 256 insertions(+), 125 deletions(-) create mode 100644 src/pages/Compare/styles.module.scss diff --git a/src/pages/Compare/index.spec.tsx b/src/pages/Compare/index.spec.tsx index ed69e527d..9023db2b7 100644 --- a/src/pages/Compare/index.spec.tsx +++ b/src/pages/Compare/index.spec.tsx @@ -5,18 +5,21 @@ import { History } from 'history'; import { createFakeHistory, createFakeThunk, - fakeVersion, + fakeVersionWithDiff, shallowUntilTarget, spyOn, } from '../../test-helpers'; import configureStore from '../../configureStore'; import { - actions as versionActions, + actions as versionsActions, + createInternalDiffs, createInternalVersion, } from '../../reducers/versions'; import FileTree from '../../components/FileTree'; import DiffView from '../../components/DiffView'; import Loading from '../../components/Loading'; +import VersionChooser from '../../components/VersionChooser'; +import styles from './styles.module.scss'; import Compare, { CompareBase, PublicProps } from '.'; @@ -43,7 +46,7 @@ describe(__filename, () => { }; type RenderParams = { - _fetchVersion?: PublicProps['_fetchVersion']; + _fetchDiff?: PublicProps['_fetchDiff']; addonId?: string; baseVersionId?: string; headVersionId?: string; @@ -53,7 +56,7 @@ describe(__filename, () => { }; const render = ({ - _fetchVersion, + _fetchDiff, addonId = '999', baseVersionId = '1', headVersionId = '2', @@ -66,7 +69,7 @@ describe(__filename, () => { history, params: { lang, addonId, baseVersionId, headVersionId }, }), - _fetchVersion, + _fetchDiff, }; return shallowUntilTarget(, CompareBase, { @@ -94,248 +97,337 @@ describe(__filename, () => { }; }; - it('renders a page with a loading message', () => { - const root = render(); + const _loadDiff = ({ + addonId = 1, + baseVersionId = 2, + headVersionId = 3, + store = configureStore(), + version = fakeVersionWithDiff, + }) => { + store.dispatch(versionsActions.loadVersionInfo({ version })); + store.dispatch( + versionsActions.loadDiff({ + addonId, + baseVersionId, + headVersionId, + version, + }), + ); + }; + + it('renders loading messages when no diff has been loaded', () => { + const addonId = 123; + const root = render({ addonId: String(addonId) }); - expect(root.find(Loading)).toHaveLength(1); - expect(root.find(Loading)).toHaveProp('message', 'Loading version...'); + expect(root.find(Loading)).toHaveLength(2); + expect(root.find(Loading).at(0)).toHaveProp( + 'message', + 'Loading file tree...', + ); + expect(root.find(Loading).at(1)).toHaveProp('message', 'Loading diff...'); + // This component is always displayed. + expect(root.find(VersionChooser)).toHaveLength(1); + expect(root.find(VersionChooser)).toHaveProp('addonId', addonId); }); - it('renders a FileTree component when a version has been loaded', () => { - const baseVersion = fakeVersion; - const headVersion = { ...fakeVersion, id: baseVersion.id + 1 }; + it('renders a FileTree component when a diff has been loaded', () => { + const addonId = 9999; + const baseVersionId = 1; + const version = { ...fakeVersionWithDiff, id: baseVersionId + 1 }; const store = configureStore(); - store.dispatch(versionActions.loadVersionInfo({ version: baseVersion })); - store.dispatch(versionActions.loadVersionInfo({ version: headVersion })); + _loadDiff({ + addonId, + baseVersionId, + headVersionId: version.id, + store, + version, + }); const root = render({ store, - baseVersionId: String(baseVersion.id), - headVersionId: String(headVersion.id), + baseVersionId: String(baseVersionId), + headVersionId: String(version.id), }); expect(root.find(FileTree)).toHaveLength(1); expect(root.find(FileTree)).toHaveProp( 'version', - createInternalVersion(baseVersion), + createInternalVersion(version), ); }); it('renders a DiffView', () => { - const baseVersion = fakeVersion; - const headVersion = { ...fakeVersion, id: baseVersion.id + 1 }; + const addonId = 999; + const baseVersionId = 1; + const path = 'manifest.json'; + const mimeType = 'mime/type'; + + const version = { + ...fakeVersionWithDiff, + id: baseVersionId + 1, + file: { + ...fakeVersionWithDiff.file, + entries: { + ...fakeVersionWithDiff.file.entries, + [path]: { + ...fakeVersionWithDiff.file.entries[path], + mimetype: mimeType, + }, + }, + // eslint-disable-next-line @typescript-eslint/camelcase + selected_file: path, + }, + }; const store = configureStore(); - store.dispatch(versionActions.loadVersionInfo({ version: baseVersion })); - store.dispatch(versionActions.loadVersionInfo({ version: headVersion })); + _loadDiff({ + addonId, + baseVersionId, + headVersionId: version.id, + store, + version, + }); const root = render({ store, - baseVersionId: String(baseVersion.id), - headVersionId: String(headVersion.id), + baseVersionId: String(baseVersionId), + headVersionId: String(version.id), }); expect(root.find(DiffView)).toHaveLength(1); + expect(root.find(DiffView)).toHaveProp( + 'diffs', + createInternalDiffs({ + baseVersionId, + headVersionId: version.id, + version, + }), + ); + expect(root.find(DiffView)).toHaveProp('mimeType', mimeType); }); - it('dispatches fetchVersion() on mount', () => { - const addonId = 123456; - const baseVersion = fakeVersion; + it('renders an error when fetching a diff has failed', () => { + const store = configureStore(); + store.dispatch(versionsActions.abortFetchDiff()); + + const root = render({ store }); + + expect(root.find(`.${styles.error}`)).toHaveLength(2); + }); + + it('dispatches fetchDiff() on mount', () => { + const addonId = 9999; + const baseVersionId = 1; + const headVersionId = baseVersionId + 1; const store = configureStore(); const dispatch = spyOn(store, 'dispatch'); const fakeThunk = createFakeThunk(); - const _fetchVersion = fakeThunk.createThunk; + const _fetchDiff = fakeThunk.createThunk; render({ - _fetchVersion, + ...getRouteParams({ addonId, baseVersionId, headVersionId }), + _fetchDiff, store, - ...getRouteParams({ - addonId, - baseVersionId: baseVersion.id, - }), }); expect(dispatch).toHaveBeenCalledWith(fakeThunk.thunk); - expect(_fetchVersion).toHaveBeenCalledWith({ + expect(_fetchDiff).toHaveBeenCalledWith({ addonId, - versionId: baseVersion.id, + baseVersionId, + headVersionId, }); }); it('redirects to a new compare url when the "old" version is newer than the "new" version', () => { const addonId = 123456; - const baseVersion = fakeVersion; - const headVersion = { ...fakeVersion, id: baseVersion.id - 1 }; + const baseVersionId = 2; + const headVersionId = baseVersionId - 1; const lang = 'es'; const store = configureStore(); const dispatch = spyOn(store, 'dispatch'); const fakeThunk = createFakeThunk(); - const _fetchVersion = fakeThunk.createThunk; + const _fetchDiff = fakeThunk.createThunk; const history = createFakeHistory(); const push = spyOn(history, 'push'); render({ - _fetchVersion, - store, - ...getRouteParams({ - addonId, - baseVersionId: baseVersion.id, - headVersionId: headVersion.id, - }), + ...getRouteParams({ addonId, baseVersionId, headVersionId }), + _fetchDiff, history, lang, + store, }); expect(push).toHaveBeenCalledWith( - `/${lang}/compare/${addonId}/versions/${headVersion.id}...${ - baseVersion.id - }/`, + `/${lang}/compare/${addonId}/versions/${headVersionId}...${baseVersionId}/`, ); expect(dispatch).not.toHaveBeenCalled(); }); - it('does not dispatch fetchVersion() on update if no parameter has changed', () => { + it('does not dispatch fetchDiff() on update if no parameter has changed', () => { const addonId = 123456; - const baseVersion = fakeVersion; - const headVersion = { ...fakeVersion, id: baseVersion.id + 1 }; + const baseVersionId = 1; + const headVersionId = baseVersionId + 1; + const params = getRouteParams({ addonId, baseVersionId, headVersionId }); const store = configureStore(); const fakeThunk = createFakeThunk(); const dispatch = spyOn(store, 'dispatch'); const root = render({ - _fetchVersion: fakeThunk.createThunk, - addonId: String(addonId), - baseVersionId: String(baseVersion.id), - headVersionId: String(headVersion.id), + ...params, + _fetchDiff: fakeThunk.createThunk, store, }); dispatch.mockClear(); - root.setProps({ - match: { - params: { - addonId: String(addonId), - baseVersionId: String(baseVersion.id), - headVersionId: String(headVersion.id), - }, - }, - }); + root.setProps({ match: { params } }); expect(dispatch).not.toHaveBeenCalled(); }); - it('dispatches fetchVersion() on update if base version is different', () => { + it('dispatches fetchDiff() on update if base version is different', () => { const addonId = 123456; - const baseVersion = fakeVersion; + const baseVersionId = 10; + const headVersionId = baseVersionId + 1; const store = configureStore(); const fakeThunk = createFakeThunk(); - const _fetchVersion = fakeThunk.createThunk; + const _fetchDiff = fakeThunk.createThunk; const dispatch = spyOn(store, 'dispatch'); const root = render({ - _fetchVersion, ...getRouteParams({ addonId, - baseVersionId: baseVersion.id - 10, + baseVersionId: baseVersionId - 1, + headVersionId, }), + _fetchDiff, store, }); dispatch.mockClear(); - _fetchVersion.mockClear(); + _fetchDiff.mockClear(); root.setProps({ match: { - params: getRouteParams({ - addonId, - baseVersionId: baseVersion.id, - }), + params: getRouteParams({ addonId, baseVersionId, headVersionId }), }, }); expect(dispatch).toHaveBeenCalledWith(fakeThunk.thunk); - expect(_fetchVersion).toHaveBeenCalledWith({ + expect(_fetchDiff).toHaveBeenCalledWith({ addonId, - versionId: baseVersion.id, + baseVersionId, + headVersionId, }); }); - it('dispatches fetchVersion() on update if head version is different', () => { + it('dispatches fetchDiff() on update if head version is different', () => { const addonId = 123456; - const baseVersion = fakeVersion; - const headVersion = { ...fakeVersion, id: baseVersion.id + 1 }; + const baseVersionId = 1; + const headVersionId = baseVersionId + 1; const store = configureStore(); const fakeThunk = createFakeThunk(); - const _fetchVersion = fakeThunk.createThunk; + const _fetchDiff = fakeThunk.createThunk; const dispatch = spyOn(store, 'dispatch'); const root = render({ - _fetchVersion, ...getRouteParams({ addonId, - baseVersionId: baseVersion.id, - headVersionId: headVersion.id + 10, + baseVersionId, + headVersionId: headVersionId + 1, }), + _fetchDiff, store, }); dispatch.mockClear(); - _fetchVersion.mockClear(); + _fetchDiff.mockClear(); root.setProps({ match: { - params: getRouteParams({ - addonId, - baseVersionId: baseVersion.id, - headVersionId: headVersion.id, - }), + params: getRouteParams({ addonId, baseVersionId, headVersionId }), }, }); expect(dispatch).toHaveBeenCalledWith(fakeThunk.thunk); - expect(_fetchVersion).toHaveBeenCalledWith({ + expect(_fetchDiff).toHaveBeenCalledWith({ addonId, - versionId: baseVersion.id, + baseVersionId, + headVersionId, }); }); - it('dispatches fetchVersion() on update if addon ID is different', () => { + it('dispatches fetchDiff() on update if addon ID is different', () => { const addonId = 123456; - const baseVersion = fakeVersion; + const baseVersionId = 1; + const headVersionId = baseVersionId + 1; const store = configureStore(); const fakeThunk = createFakeThunk(); - const _fetchVersion = fakeThunk.createThunk; + const _fetchDiff = fakeThunk.createThunk; const dispatch = spyOn(store, 'dispatch'); const root = render({ - _fetchVersion, + _fetchDiff, ...getRouteParams({ addonId: addonId + 10, - baseVersionId: baseVersion.id, }), store, }); dispatch.mockClear(); - _fetchVersion.mockClear(); + _fetchDiff.mockClear(); root.setProps({ match: { - params: getRouteParams({ - addonId, - baseVersionId: baseVersion.id, - }), + params: getRouteParams({ addonId, baseVersionId, headVersionId }), }, }); expect(dispatch).toHaveBeenCalledWith(fakeThunk.thunk); - expect(_fetchVersion).toHaveBeenCalledWith({ + expect(_fetchDiff).toHaveBeenCalledWith({ + addonId, + baseVersionId, + headVersionId, + }); + }); + + it('dispatches fetchDiff() when a file is selected', () => { + const addonId = 123456; + const baseVersionId = 1; + const headVersionId = baseVersionId + 1; + const path = 'new-path.js'; + + const store = configureStore(); + const fakeThunk = createFakeThunk(); + const _fetchDiff = fakeThunk.createThunk; + const dispatch = spyOn(store, 'dispatch'); + + const root = render({ + ...getRouteParams({ + addonId, + baseVersionId, + headVersionId: headVersionId + 1, + }), + _fetchDiff, + store, + }); + + dispatch.mockClear(); + _fetchDiff.mockClear(); + + const onSelectFile = root.find(FileTree).prop('onSelect'); + onSelectFile(path); + + expect(dispatch).toHaveBeenCalledWith(fakeThunk.thunk); + expect(_fetchDiff).toHaveBeenCalledWith({ addonId, - versionId: baseVersion.id, + baseVersionId, + headVersionId, + path, }); }); }); diff --git a/src/pages/Compare/index.tsx b/src/pages/Compare/index.tsx index 8d4ead1ef..71f27ae06 100644 --- a/src/pages/Compare/index.tsx +++ b/src/pages/Compare/index.tsx @@ -2,19 +2,23 @@ import * as React from 'react'; import { Col, Row } from 'react-bootstrap'; import { RouteComponentProps } from 'react-router-dom'; import { connect } from 'react-redux'; -import { parseDiff } from 'react-diff-view'; import { ApplicationState, ConnectedReduxProps } from '../../configureStore'; import FileTree from '../../components/FileTree'; import DiffView from '../../components/DiffView'; import Loading from '../../components/Loading'; import VersionChooser from '../../components/VersionChooser'; -import { Version, fetchVersion, getVersionInfo } from '../../reducers/versions'; +import { + CompareInfo, + Version, + fetchDiff, + getVersionInfo, +} from '../../reducers/versions'; import { gettext } from '../../utils'; -import diffWithDeletions from '../../components/DiffView/fixtures/diffWithDeletions'; +import styles from './styles.module.scss'; export type PublicProps = { - _fetchVersion: typeof fetchVersion; + _fetchDiff: typeof fetchDiff; }; type PropsFromRouter = { @@ -26,6 +30,8 @@ type PropsFromRouter = { type PropsFromState = { addonId: number; + compareInfo: CompareInfo | null | void; + isLoading: boolean; version: Version; }; @@ -36,7 +42,7 @@ type Props = RouteComponentProps & export class CompareBase extends React.Component { static defaultProps = { - _fetchVersion: fetchVersion, + _fetchDiff: fetchDiff, }; componentDidMount() { @@ -71,34 +77,57 @@ export class CompareBase extends React.Component { baseVersionId !== prevProps.match.params.baseVersionId || headVersionId !== prevProps.match.params.headVersionId ) { - const { dispatch, _fetchVersion } = this.props; + const { dispatch, _fetchDiff } = this.props; dispatch( - _fetchVersion({ + _fetchDiff({ addonId: parseInt(addonId, 10), - versionId: parseInt(baseVersionId, 10), + baseVersionId: parseInt(baseVersionId, 10), + headVersionId: parseInt(headVersionId, 10), }), ); } } - onSelectFile = () => {}; + onSelectFile = (path: string) => { + const { _fetchDiff, dispatch, match } = this.props; + const { addonId, baseVersionId, headVersionId } = match.params; - render() { - const { addonId, version } = this.props; + dispatch( + _fetchDiff({ + addonId: parseInt(addonId, 10), + baseVersionId: parseInt(baseVersionId, 10), + headVersionId: parseInt(headVersionId, 10), + path, + }), + ); + }; + + renderLoadingMessageOrError(message: string) { + const { isLoading } = this.props; - if (!version) { + if (!isLoading) { return ( - - - +

+ {gettext('Ooops, an error has occured.')} +

); } + return ; + } + + render() { + const { addonId, compareInfo, version } = this.props; + return ( - + {version ? ( + + ) : ( + this.renderLoadingMessageOrError(gettext('Loading file tree...')) + )} @@ -108,10 +137,14 @@ export class CompareBase extends React.Component { - + {compareInfo ? ( + + ) : ( + this.renderLoadingMessageOrError(gettext('Loading diff...')) + )} @@ -126,12 +159,18 @@ const mapStateToProps = ( ): PropsFromState => { const { match } = ownProps; const addonId = parseInt(match.params.addonId, 10); - const baseVersionId = parseInt(match.params.baseVersionId, 10); + const headVersionId = parseInt(match.params.headVersionId, 10); + + const { compareInfo } = state.versions; + const isLoading = compareInfo === undefined; - const version = getVersionInfo(state.versions, baseVersionId); + // The Compare API returns the version info of the head/newest version. + const version = getVersionInfo(state.versions, headVersionId); return { addonId, + compareInfo, + isLoading, version, }; }; diff --git a/src/pages/Compare/styles.module.scss b/src/pages/Compare/styles.module.scss new file mode 100644 index 000000000..e69de29bb diff --git a/src/pages/Index/index.tsx b/src/pages/Index/index.tsx index 27f637dbc..6386dd302 100644 --- a/src/pages/Index/index.tsx +++ b/src/pages/Index/index.tsx @@ -14,7 +14,7 @@ export class IndexBase extends React.Component { browsing this add-on version {' '} or{' '} - + look at this compare view.