Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed errors when adding duplicate categories. #4372

Merged
merged 1 commit into from
Jan 11, 2018
Merged

Fixed errors when adding duplicate categories. #4372

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 33 additions & 5 deletions editor/components/post-taxonomies/hierarchical-term-selector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import { connect } from 'react-redux';
import { unescape as unescapeString, without, map, repeat, find } from 'lodash';
import { unescape as unescapeString, without, map, repeat, find, some } from 'lodash';

/**
* WordPress dependencies
Expand All @@ -28,6 +28,7 @@ const DEFAULT_QUERY = {
class HierarchicalTermSelector extends Component {
constructor() {
super( ...arguments );
this.findTerm = this.findTerm.bind( this );
this.onChange = this.onChange.bind( this );
this.onChangeFormName = this.onChangeFormName.bind( this );
this.onChangeFormParent = this.onChangeFormParent.bind( this );
Expand Down Expand Up @@ -69,12 +70,35 @@ class HierarchicalTermSelector extends Component {
} ) );
}

findTerm( terms, parent, name ) {
return find( terms, term => {
return ( ( ! term.parent && ! parent ) || parseInt( term.parent ) === parseInt( parent ) ) &&
term.name === name;
} );
}

onAddTerm( event ) {
event.preventDefault();
const { formName, formParent, adding } = this.state;
const { onUpdateTerms, restBase, terms, slug } = this.props;
const { formName, formParent, adding, availableTerms } = this.state;
if ( formName === '' || adding ) {
return;
}

// check if the term we are adding already exists
const existingTerm = this.findTerm( availableTerms, formParent, formName );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Could this have been consolidated into the findOrCreatePromise as an early return value for the term argument of the resolved promise callback?

if ( existingTerm ) {
// if the term we are adding exists but is not selected select it
if ( ! some( terms, term => term === existingTerm.id ) ) {
onUpdateTerms( [ ...terms, existingTerm.id ], restBase );
}
this.setState( {
formName: '',
formParent: '',
} );
return;
}

const findOrCreatePromise = new Promise( ( resolve, reject ) => {
this.setState( {
adding: true,
Expand All @@ -89,8 +113,13 @@ class HierarchicalTermSelector extends Component {
.then( resolve, ( xhr ) => {
const errorCode = xhr.responseJSON && xhr.responseJSON.code;
if ( errorCode === 'term_exists' ) {
this.addRequest = new Model( { id: xhr.responseJSON.data } ).fetch();
return this.addRequest.then( resolve, reject );
// search the new category created since last fetch
this.addRequest = new Model().fetch(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change the request here? Thinking that requesting using the id is more reliable, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug was that the server when returns the 409 answer does not return the term. So we were generating an invalid request GET http://src.wordpress-develop.test/wp-json/wp/v2/categories/[object%20Object] 404 (Not Found). Requesting by id would be better but I think we can not do that because we don't know the id yet :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try to inspect the 409 response? I think when I did this, the response body used to contain the conflicting id.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I tried, and unfortunately, we are out of luck the response just contains "{"code":"term_exists","message":"A term with the name provided already exists with this parent.","data":{"status":409}}". Maybe something changed in the API meanwhile.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this means we may have an issue here, what guarantees that the received category is the right one? I think we should compare the name once we retrieve the results instead of just picking the first one. Imagine two categories at the same level called "cat" and "cat 2", I believe if you try to insert "cat", the order is not guaranteed in the API response and we may insert "cat 2" instead right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right, we should add a client-side filter on the search results. I will make the change :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change was applied, we should be handling all the cases now.

{ data: { ...DEFAULT_QUERY, parent: formParent || 0, search: formName } }
);
return this.addRequest.then( searchResult => {
resolve( this.findTerm( searchResult, formParent, formName ) );
}, reject );
}
reject( xhr );
} );
Expand All @@ -99,7 +128,6 @@ class HierarchicalTermSelector extends Component {
.then( ( term ) => {
const hasTerm = !! find( this.state.availableTerms, ( availableTerm ) => availableTerm.id === term.id );
const newAvailableTerms = hasTerm ? this.state.availableTerms : [ term, ...this.state.availableTerms ];
const { onUpdateTerms, restBase, terms, slug } = this.props;
const termAddedMessage = slug === 'category' ? __( 'Category added' ) : __( 'Term added' );
this.props.speak( termAddedMessage, 'assertive' );
this.setState( {
Expand Down