From 95014ea0563253aae4403f52e9bf831abf4694e6 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Tue, 11 Oct 2016 11:33:21 +0100 Subject: [PATCH 1/2] Revert "Revert "Editor: Make the AddTerm Component more reusable (#8588)" (#8643)" This reverts commit 9fabe89568565fcada490728708dcb50ec00a03f. --- .../my-sites/term-tree-selector/add-term.jsx | 99 +++++++---- .../editor-term-selector/index.jsx | 18 +- client/state/index.js | 3 +- client/state/posts/actions.js | 34 +++- client/state/posts/middleware.js | 49 ------ client/state/posts/test/actions.js | 77 ++++++--- client/state/posts/test/middleware.js | 161 ------------------ client/state/terms/actions.js | 10 +- 8 files changed, 170 insertions(+), 281 deletions(-) delete mode 100644 client/state/posts/middleware.js delete mode 100644 client/state/posts/test/middleware.js diff --git a/client/my-sites/term-tree-selector/add-term.jsx b/client/my-sites/term-tree-selector/add-term.jsx index 7b14cb49d7fc59..8cfff1ed4d8779 100644 --- a/client/my-sites/term-tree-selector/add-term.jsx +++ b/client/my-sites/term-tree-selector/add-term.jsx @@ -6,7 +6,7 @@ import ReactDom from 'react-dom'; import classNames from 'classnames'; import { connect } from 'react-redux'; import { localize } from 'i18n-calypso'; -import { get, find } from 'lodash'; +import { get, find, noop } from 'lodash'; /** * Internal dependencies @@ -16,6 +16,7 @@ import TermTreeSelectorTerms from './terms'; import Button from 'components/button'; import Gridicon from 'components/gridicon'; import FormInputValidation from 'components/forms/form-input-validation'; +import FormTextarea from 'components/forms/form-textarea'; import FormTextInput from 'components/forms/form-text-input'; import FormSectionHeading from 'components/forms/form-section-heading'; import FormCheckbox from 'components/forms/form-checkbox'; @@ -26,7 +27,7 @@ import viewport from 'lib/viewport'; import { getSelectedSiteId } from 'state/ui/selectors'; import { getPostTypeTaxonomy } from 'state/post-types/taxonomies/selectors'; import { getTerms } from 'state/terms/selectors'; -import { addTermForPost } from 'state/posts/actions'; +import { addTerm } from 'state/terms/actions'; class TermSelectorAddTerm extends Component { static initialState = { @@ -39,14 +40,21 @@ class TermSelectorAddTerm extends Component { static propTypes = { labels: PropTypes.object, + onSuccess: PropTypes.func, postType: PropTypes.string, postId: PropTypes.number, + showDescriptionInput: PropTypes.bool, siteId: PropTypes.number, terms: PropTypes.array, taxonomy: PropTypes.string, translate: PropTypes.func }; + static defaultProps = { + onSuccess: noop, + showDescriptionInput: false + }; + constructor( props ) { super( props ); this.state = this.constructor.initialState; @@ -92,9 +100,15 @@ class TermSelectorAddTerm extends Component { getFormValues() { const name = ReactDom.findDOMNode( this.refs.termName ).value.trim(); - const parent = this.state.selectedParent.length ? this.state.selectedParent[ 0 ] : 0; + const formValues = { name }; + if ( this.props.isHierarchical ) { + formValues.parent = this.state.selectedParent.length ? this.state.selectedParent[ 0 ] : 0; + } + if ( this.props.showDescriptionInput ) { + formValues.description = ReactDom.findDOMNode( this.refs.termDescription ).value.trim(); + } - return { name, parent }; + return formValues; } isValid() { @@ -109,7 +123,7 @@ class TermSelectorAddTerm extends Component { const lowerCasedTermName = values.name.toLowerCase(); const matchingTerm = find( this.props.terms, ( term ) => { return ( term.name.toLowerCase() === lowerCasedTermName ) && - ( term.parent === values.parent ); + ( ! this.props.isHierarchical || ( term.parent === values.parent ) ); } ); if ( matchingTerm ) { @@ -145,12 +159,43 @@ class TermSelectorAddTerm extends Component { const { postId, siteId, taxonomy } = this.props; - this.props.addTermForPost( siteId, taxonomy, term, postId ); + this.props + .addTerm( siteId, taxonomy, term, postId ) + .then( this.props.onSuccess ); this.closeDialog(); } + renderParentSelector() { + const { labels, siteId, taxonomy, translate } = this.props; + const { searchTerm, selectedParent } = this.state; + const query = {}; + if ( searchTerm && searchTerm.length ) { + query.search = searchTerm; + } + + return ( + + + { labels.parent_item } + + + + { translate( 'Top level', { context: 'Terms: New term being created is top level' } ) } + + + + ); + } + render() { - const { labels, siteId, taxonomy, translate, terms } = this.props; + const { isHierarchical, labels, translate, terms, showDescriptionInput } = this.props; const buttons = [ { action: 'cancel', label: translate( 'Cancel' ) @@ -162,12 +207,6 @@ class TermSelectorAddTerm extends Component { onClick: this.boundSaveTerm } ]; - const { searchTerm, selectedParent } = this.state; - const query = {}; - if ( searchTerm && searchTerm.length ) { - query.search = searchTerm; - } - const isError = this.state.error && this.state.error.length; const totalTerms = terms ? terms.length : 0; const classes = classNames( 'term-tree-selector__add-term', { 'is-compact': totalTerms < 8 } ); @@ -193,23 +232,16 @@ class TermSelectorAddTerm extends Component { onKeyUp={ this.boundValidateInput } /> { isError && } - - - { labels.parent_item } - - - - { translate( 'Top level', { context: 'Terms: New term being created is top level' } ) } - - - + { showDescriptionInput && + + { translate( 'Description', { context: 'Terms: Term description label' } ) } + + + + } + { isHierarchical && this.renderParentSelector() } ); @@ -220,13 +252,16 @@ export default connect( ( state, ownProps ) => { const { taxonomy, postType } = ownProps; const siteId = getSelectedSiteId( state ); - const labels = get( getPostTypeTaxonomy( state, siteId, postType, taxonomy ), 'labels', {} ); + const taxonomyDetails = getPostTypeTaxonomy( state, siteId, postType, taxonomy ); + const labels = get( taxonomyDetails, 'labels', {} ); + const isHierarchical = taxonomyDetails.hierarchical; return { terms: getTerms( state, siteId, taxonomy ), + isHierarchical, labels, siteId }; }, - { addTermForPost } + { addTerm } )( localize( TermSelectorAddTerm ) ); diff --git a/client/post-editor/editor-term-selector/index.jsx b/client/post-editor/editor-term-selector/index.jsx index 53d81b1d3f995f..ceb47cb5c7406f 100644 --- a/client/post-editor/editor-term-selector/index.jsx +++ b/client/post-editor/editor-term-selector/index.jsx @@ -10,7 +10,7 @@ import { cloneDeep, findIndex, map, toArray } from 'lodash'; */ import TermTreeSelector from 'my-sites/term-tree-selector'; import AddTerm from 'my-sites/term-tree-selector/add-term'; -import { editPost } from 'state/posts/actions'; +import { editPost, addTermForPost } from 'state/posts/actions'; import { getSelectedSiteId } from 'state/ui/selectors'; import { getEditorPostId } from 'state/ui/editor/selectors'; import { getEditedPostValue } from 'state/posts/selectors'; @@ -31,6 +31,11 @@ class EditorTermSelector extends Component { this.boundOnTermsChange = this.onTermsChange.bind( this ); } + onAddTerm = ( term ) => { + const { postId, taxonomyName, siteId } = this.props; + this.props.addTermForPost( siteId, taxonomyName, term, postId ); + } + onTermsChange( selectedTerm ) { const { postTerms, taxonomyName, siteId, postId } = this.props; const terms = cloneDeep( postTerms ) || {}; @@ -58,7 +63,7 @@ class EditorTermSelector extends Component { } render() { - const { postType, postId, siteId, taxonomyName, canEditTerms } = this.props; + const { postType, siteId, taxonomyName, canEditTerms } = this.props; return (
@@ -70,7 +75,12 @@ class EditorTermSelector extends Component { siteId={ siteId } multiple={ true } /> - { canEditTerms && } + { canEditTerms && + + }
); } @@ -89,5 +99,5 @@ export default connect( postId }; }, - { editPost } + { editPost, addTermForPost } )( EditorTermSelector ); diff --git a/client/state/index.js b/client/state/index.js index a6072b3c0aa2b5..fc51b871b61201 100644 --- a/client/state/index.js +++ b/client/state/index.js @@ -8,7 +8,6 @@ import { createStore, applyMiddleware, combineReducers, compose } from 'redux'; * Internal dependencies */ import noticesMiddleware from './notices/middleware'; -import postsEditMiddleware from './posts/middleware'; import application from './application/reducer'; import comments from './comments/reducer'; import componentsUsageStats from './components-usage-stats/reducer'; @@ -94,7 +93,7 @@ export const reducer = combineReducers( { wordads } ); -const middleware = [ thunkMiddleware, noticesMiddleware, postsEditMiddleware ]; +const middleware = [ thunkMiddleware, noticesMiddleware ]; if ( typeof window === 'object' ) { // Browser-specific middlewares diff --git a/client/state/posts/actions.js b/client/state/posts/actions.js index 7207a2f0e8c445..338cc31773aa33 100644 --- a/client/state/posts/actions.js +++ b/client/state/posts/actions.js @@ -1,10 +1,14 @@ +/** + * External dependencies + */ +import { isNumber, toArray } from 'lodash'; + /** * Internal dependencies */ import wpcom from 'lib/wp'; -import { extendAction } from 'state/utils'; import { normalizePostForApi } from './utils'; -import { addTerm } from 'state/terms/actions'; +import { getEditedPost } from 'state/posts/selectors'; import { POST_DELETE, POST_DELETE_SUCCESS, @@ -297,9 +301,7 @@ export function restorePost( siteId, postId ) { } /** - * Returns an action thunk which, when dispatched, triggers a network request - * to create a new term. All actions dispatched by the thunk will include meta - * to associate it with the specified post ID. + * Returns an action thunk which, when dispatched, adds a term to the current edited post * * @param {Number} siteId Site ID * @param {String} taxonomy Taxonomy Slug @@ -308,5 +310,25 @@ export function restorePost( siteId, postId ) { * @return {Function} Action thunk */ export function addTermForPost( siteId, taxonomy, term, postId ) { - return extendAction( addTerm( siteId, taxonomy, term ), { postId } ); + return ( dispatch, getState ) => { + const state = getState(); + const post = getEditedPost( state, siteId, postId ); + + // if there is no post, no term, or term is temporary, bail + if ( ! post || ! term || ! isNumber( term.ID ) ) { + return; + } + + const postTerms = post.terms || {}; + + // ensure we have an array since API returns an object + const taxonomyTerms = toArray( postTerms[ taxonomy ] ); + taxonomyTerms.push( term ); + + dispatch( editPost( siteId, postId, { + terms: { + [ taxonomy ]: taxonomyTerms + } + } ) ); + }; } diff --git a/client/state/posts/middleware.js b/client/state/posts/middleware.js deleted file mode 100644 index 0a03df0090df5e..00000000000000 --- a/client/state/posts/middleware.js +++ /dev/null @@ -1,49 +0,0 @@ -/** - * External dependencies - */ -import { isNumber, toArray } from 'lodash'; - -/** - * Internal dependencies - */ -import { TERMS_RECEIVE } from 'state/action-types'; -import { getEditedPost } from 'state/posts/selectors'; -import { editPost } from 'state/posts/actions'; - -/** - * Dispatches editPost when a new term has been added - * that also has a postId on the action - * - * @param {Function} dispatch Dispatch method - * @param {Object} state Global state tree - * @param {Object} action Action object - */ -export function onTermsReceive( dispatch, state, action ) { - const { postId, taxonomy, terms, siteId } = action; - const post = getEditedPost( state, siteId, postId ); - const newTerm = terms[ 0 ]; - - // if there is no post, no term, or term is temporary, bail - if ( ! post || ! newTerm || ! isNumber( newTerm.ID ) ) { - return; - } - - const postTerms = post.terms || {}; - - // ensure we have an array since API returns an object - const taxonomyTerms = toArray( postTerms[ taxonomy ] ); - taxonomyTerms.push( newTerm ); - - dispatch( editPost( siteId, postId, { - terms: { - [ taxonomy ]: taxonomyTerms - } - } ) ); -} - -export default ( { dispatch, getState } ) => ( next ) => ( action ) => { - if ( TERMS_RECEIVE === action.type && action.hasOwnProperty( 'postId' ) ) { - onTermsReceive( dispatch, getState(), action ); - } - return next( action ); -}; diff --git a/client/state/posts/test/actions.js b/client/state/posts/test/actions.js index 0197e7e8aae402..f875466c95ba6b 100644 --- a/client/state/posts/test/actions.js +++ b/client/state/posts/test/actions.js @@ -7,6 +7,7 @@ import { expect } from 'chai'; /** * Internal dependencies */ +import PostQueryManager from 'lib/query-manager/post'; import { POST_DELETE, POST_DELETE_SUCCESS, @@ -25,8 +26,7 @@ import { POSTS_RECEIVE, POSTS_REQUEST, POSTS_REQUEST_SUCCESS, - POSTS_REQUEST_FAILURE, - TERMS_RECEIVE + POSTS_REQUEST_FAILURE } from 'state/action-types'; import { receivePost, @@ -544,31 +544,58 @@ describe( 'actions', () => { } ); describe( 'addTermForPost()', () => { - useNock( ( nock ) => { - nock( 'https://public-api.wordpress.com:443' ) - .persist() - .post( '/rest/v1.1/sites/2916284/taxonomies/jetpack-portfolio/terms/new' ) - .reply( 200, { - ID: 123, - name: 'ribs', - description: '' - } ); - } ); + const postObject = { + ID: 841, + site_ID: 2916284, + global_ID: '3d097cb7c5473c169bba0eb8e3c6cb64', + title: 'Hello World' + }; + const getState = () => { + return { + posts: { + items: { + '3d097cb7c5473c169bba0eb8e3c6cb64': [ 2916284, 841 ] + }, + queries: { + 2916284: new PostQueryManager( { + items: { 841: postObject } + } ) + }, + edits: {} + } + }; + }; - it( 'should dispatch a TERMS_RECEIVE event on success with post meta', () => { - return addTermForPost( 2916284, 'jetpack-portfolio', { name: 'ribs' }, 13640 )( spy ).then( () => { - expect( spy ).to.have.been.calledWith( { - type: TERMS_RECEIVE, - siteId: 2916284, - taxonomy: 'jetpack-portfolio', - terms: [ { - ID: 123, - name: 'ribs', - description: '' - } ], - postId: 13640 - } ); + it( 'should dispatch a EDIT_POST event with the new term', () => { + addTermForPost( 2916284, 'jetpack-portfolio', { ID: 123, name: 'ribs' }, 841 )( spy, getState ); + expect( spy ).to.have.been.calledWith( { + post: { + terms: { + 'jetpack-portfolio': [ { + ID: 123, + name: 'ribs' + } ] + } + }, + postId: 841, + siteId: 2916284, + type: POST_EDIT } ); } ); + + it( 'should not dispatch anything if no post', () => { + addTermForPost( 2916284, 'jetpack-portfolio', { ID: 123, name: 'ribs' }, 3434 )( spy, getState ); + expect( spy ).not.to.have.been.called; + } ); + + it( 'should not dispatch anything if no term', () => { + addTermForPost( 2916284, 'jetpack-portfolio', null, 841 )( spy, getState ); + expect( spy ).not.to.have.been.called; + } ); + + it( 'should not dispatch anything if the term is temporary', () => { + addTermForPost( 2916284, 'jetpack-portfolio', { id: 'temporary' }, 841 )( spy, getState ); + expect( spy ).not.to.have.been.called; + } ); } ); } ); diff --git a/client/state/posts/test/middleware.js b/client/state/posts/test/middleware.js deleted file mode 100644 index 75b31f078650fb..00000000000000 --- a/client/state/posts/test/middleware.js +++ /dev/null @@ -1,161 +0,0 @@ -/** - * External dependencies - */ -import sinon from 'sinon'; -import { expect } from 'chai'; -import deepFreeze from 'deep-freeze'; - -/** - * Internal dependencies - */ -import { - POST_EDIT -} from 'state/action-types'; -import { onTermsReceive } from '../middleware'; -import PostQueryManager from 'lib/query-manager/post'; - -describe( 'middleware', () => { - const spy = sinon.spy(); - - beforeEach( () => { - spy.reset(); - } ); - - describe( 'onTermsReceive()', () => { - it( 'postEdit should not dispatch when no matching post exists', () => { - const action = { - site: 2916284, - taxonomy: 'category', - postId: 111, - terms: [ { - ID: 777, - name: 'chicken' - } ] - }; - onTermsReceive( spy, { posts: { queries: {}, items: {}, edits: {} } }, action ); - expect( spy ).to.not.have.been.called; - } ); - - it( 'should not dispatch a postEdit when a temporary term is added', () => { - const action = { - siteId: 2916284, - taxonomy: 'category', - postId: 841, - terms: [ { - ID: 'temporary1', - name: 'meat' - } ] - }; - - onTermsReceive( spy, { posts: { queries: {}, items: {}, edits: {} } }, action ); - expect( spy ).to.not.have.been.called; - } ); - - it( 'should dispatch a postEdit when a post exists', () => { - const action = { - siteId: 2916284, - taxonomy: 'category', - postId: 841, - terms: [ { - ID: 777, - name: 'meat' - } ] - }; - - const postObject = { - ID: 841, - site_ID: 2916284, - global_ID: '3d097cb7c5473c169bba0eb8e3c6cb64', - title: 'Ribs & Chicken' - }; - - const state = deepFreeze( { - posts: { - items: { - '3d097cb7c5473c169bba0eb8e3c6cb64': [ 2916284, 841 ] - }, - queries: { - 2916284: new PostQueryManager( { - items: { 841: postObject } - } ) - }, - edits: {} - } - } ); - - onTermsReceive( spy, state, action ); - expect( spy ).to.have.been.calledWith( { - type: POST_EDIT, - post: { - terms: { - category: [ { - ID: 777, - name: 'meat' - } ] - } - }, - siteId: 2916284, - postId: 841 - } ); - } ); - - it( 'should dispatch a postEdit with accumulated terms by taxonomy', () => { - const action = { - siteId: 2916284, - taxonomy: 'category', - postId: 841, - terms: [ { - ID: 790, - name: 'yummy' - } ] - }; - - const postObject = { - ID: 841, - site_ID: 2916284, - global_ID: '3d097cb7c5473c169bba0eb8e3c6cb64', - title: 'Ribs & Chicken', - terms: { - category: { - meat: { - ID: 777, - name: 'meat' - } - } - } - }; - - const state = deepFreeze( { - posts: { - items: { - '3d097cb7c5473c169bba0eb8e3c6cb64': [ 2916284, 841 ] - }, - queries: { - 2916284: new PostQueryManager( { - items: { 841: postObject } - } ) - }, - edits: {} - } - } ); - - onTermsReceive( spy, state, action ); - expect( spy ).to.have.been.calledWith( { - type: POST_EDIT, - post: { - terms: { - category: [ { - ID: 777, - name: 'meat' - }, { - ID: 790, - name: 'yummy' - } ] - } - }, - siteId: 2916284, - postId: 841 - } ); - } ); - } ); -} ); diff --git a/client/state/terms/actions.js b/client/state/terms/actions.js index 550328b038c564..b123ba4443c7b4 100644 --- a/client/state/terms/actions.js +++ b/client/state/terms/actions.js @@ -34,9 +34,15 @@ export function addTerm( siteId, taxonomy, term ) { } ) ); return wpcom.site( siteId ).taxonomy( taxonomy ).term().add( term ).then( - ( data ) => dispatch( receiveTerm( siteId, taxonomy, data ) ), + ( data ) => { + dispatch( receiveTerm( siteId, taxonomy, data ) ); + return data; + }, () => Promise.resolve() // Silently ignore failure so we can proceed to remove temporary - ).then( () => dispatch( removeTerm( siteId, taxonomy, temporaryId ) ) ); + ).then( data => { + dispatch( removeTerm( siteId, taxonomy, temporaryId ) ); + return data; + } ); }; } From aadd096e00b7e9fe62d8ee2a9abbd93a20635b17 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Tue, 11 Oct 2016 12:15:54 +0100 Subject: [PATCH 2/2] Check if the taxonomy is loaded before getting the "hierarchical" property --- client/my-sites/term-tree-selector/add-term.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/my-sites/term-tree-selector/add-term.jsx b/client/my-sites/term-tree-selector/add-term.jsx index 8cfff1ed4d8779..9ed05cbc1aaf85 100644 --- a/client/my-sites/term-tree-selector/add-term.jsx +++ b/client/my-sites/term-tree-selector/add-term.jsx @@ -254,7 +254,7 @@ export default connect( const siteId = getSelectedSiteId( state ); const taxonomyDetails = getPostTypeTaxonomy( state, siteId, postType, taxonomy ); const labels = get( taxonomyDetails, 'labels', {} ); - const isHierarchical = taxonomyDetails.hierarchical; + const isHierarchical = get( taxonomyDetails, 'hierarchical', false ); return { terms: getTerms( state, siteId, taxonomy ),