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.

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

Conversation

Projects
None yet
3 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Jan 9, 2018

Now before issuing requests to the server, we verify if the category being added already exists. If yes we try to select it (if it is not already select).
If the category does exist we issue a request to the server, if we get an error saying we have a duplicate category (category may have been added by other used since last time we fetched), we search the server for the new category and we add it to our list.
JS errors are fixed, a 409 HTTP request may appear in the console but that is expected if the category was added in simulations since last time we fetched and we are handling that situation.

Fixes: #3910

How Has This Been Tested?

Add a category verify things work as expected.
Add a category that is repeated but not selected for the post, verify the category was selected and no error appeared in the console.
Open/create a new post, open another window with a different post, add a category in this new window. Go back to the first window, add the same category verify no error besides HTTP 409 appeared on the console and the category was added to the list of available categories.

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(

This comment has been minimized.

@youknowriad

youknowriad Jan 10, 2018

Contributor

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

@youknowriad

youknowriad Jan 10, 2018

Contributor

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

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jan 10, 2018

Member

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 :(

@jorgefilipecosta

jorgefilipecosta Jan 10, 2018

Member

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 :(

This comment has been minimized.

@youknowriad

youknowriad Jan 10, 2018

Contributor

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

@youknowriad

youknowriad Jan 10, 2018

Contributor

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

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jan 10, 2018

Member

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.

@jorgefilipecosta

jorgefilipecosta Jan 10, 2018

Member

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.

This comment has been minimized.

@youknowriad

youknowriad Jan 10, 2018

Contributor

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?

@youknowriad

youknowriad Jan 10, 2018

Contributor

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?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jan 10, 2018

Member

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

@jorgefilipecosta

jorgefilipecosta Jan 10, 2018

Member

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

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jan 10, 2018

Member

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

@jorgefilipecosta

jorgefilipecosta Jan 10, 2018

Member

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

Fixed errors when adding duplicate categories.
Now before issuing requests to the server, we verify if the category being added already exists. If yes we try to select it  (if it is not already select).
If the category does exist we issue a request to the server, if we get an error saying we have a duplicate category (category may have been added by other used since last time we fetched), we search the server for the new category and we add it to our list.
@youknowriad

LGTM 👍

What about the non hierarchical selector, do we have a similar issue there?

@jorgefilipecosta jorgefilipecosta merged commit 6e82947 into master Jan 11, 2018

3 checks passed

codecov/project 39.43% (-0.38%) compared to 6a7241a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/errors-adding-duplicate-categories branch Jan 11, 2018

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Jan 12, 2018

Member

What about the non hierarchical selector, do we have a similar issue there?

Good point @youknowriad, I checked the code and we have the same problem there. I will address it in a new PR.

Member

jorgefilipecosta commented Jan 12, 2018

What about the non hierarchical selector, do we have a similar issue there?

Good point @youknowriad, I checked the code and we have the same problem there. I will address it in a new PR.

if ( formName === '' || adding ) {
return;
}
// check if the term we are adding already exists
const existingTerm = this.findTerm( availableTerms, formParent, formName );

This comment has been minimized.

@aduth

aduth Jan 16, 2018

Member

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

@aduth

aduth Jan 16, 2018

Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment