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

Terms: Include only known terms in rendered selector #5913

Merged
merged 3 commits into from Apr 3, 2018

Conversation

Projects
None yet
4 participants
@aduth
Copy link
Member

aduth commented Mar 30, 2018

This pull request seeks to resolve an issue where posts with multiple non-hierarchical terms assigned will display empty term values in the term selector. Previously, we would render a term even if its name could not be determined (because it had not yet been fetched), resulting in an issue of conflicting key assignments, since the key is derived from the value, an empty string when not yet known.

With these changes, we only include the term in the selector if it's known. Since we explicitly request terms assigned to the post, this should work well enough, though by default this request would only return 10 terms. Therefore, it is possible that a post with more than 10 terms assigned would not display all terms in the selector. With these changes, I have increased this to 100, though this is still not a perfect solution since more than 100 terms for a post is technically possible. A more thorough solution would paginate results as necessary.

Since this component needs to be refactored with consideration of data module improvements (related: #5826), I am inclined to defer this to a future improvement.

Testing instructions:

Verify that with multiple tags assigned to a post, no empty tags appear in tag selector.

@jorgefilipecosta
Copy link
Member

jorgefilipecosta left a comment

I confirmed this change fixes the bug of showing an empty tag in the beginning and the code looks good 👍

@bobbingwide

This comment has been minimized.

Copy link
Contributor

bobbingwide commented Mar 31, 2018

Please can you explain what you mean by

This pull request seeks to resolve an issue where posts with multiple non-hierarchical terms assigned will display empty term values in the term selector.

With 2.5.0 the problem you appear to be referring to occurs when a tag or custom taxonomy contains a blank. e.g. "two words".

The problem occurs when only one multi word tag is assigned to the post.

I also noted that the request to fetch the 'missing' terms appears like this:

https://qw/oikcom/wp-json/wp/v2/tags?per_page=100&orderby=count&order=desc&_fields=id%2Cname&include=449&include=590&include=38

But the response only contains information for the last term.

[{"id":38,"name":"WordPress"}]

This is a backwards compatibility issue.

@bobbingwide

This comment has been minimized.

Copy link
Contributor

bobbingwide commented Mar 31, 2018

This is a backwards compatibility issue.
It's not limited to Chrome.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Mar 31, 2018

Hi @bobbingwide thank you for your debugging and feedback,

Please can you explain what you mean by

This pull request seeks to resolve an issue where posts with multiple non-hierarchical terms assigned will display empty term values in the term selector.

What I tested that this PR solves is:
If we add 3 or more tags e.g: "aaa", "bbb" and "ccc" to a post then we save the post and open it again, without this PR we have a bug where the editor adds empty tags:
image

After this PR the problem does not happen.

@bobbingwide

This comment has been minimized.

Copy link
Contributor

bobbingwide commented Mar 31, 2018

@jorgefilipecosta. I can confirm that I got the same problem that you tested using three one word tags.

And now I don't appear to be able to reproduce the problem that I thought I'd found. The chances are very high that I stumbled across the same problem and attributed it incorrectly. I first noted the problem with this taxonomy block, associated with content last updated 3 years ago.
image

4 multi word tags... but I had no explanation for the three word tag :-)

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Mar 31, 2018

@jorgefilipecosta OK. Well that's just one of the problems then.

Yes, this is one of the problems not directly related to the requests so we were able to separate the improvement as it was needed no matter what changes happen in the request logic. The request logic will need to suffer a refactoring because of the paginated requests (dealing with +100 tags) during this time we will try to check the other bugs in the request system. Thank you for your inputs.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Apr 2, 2018

I also noted that the request to fetch the 'missing' terms appears like this:

But the response only contains information for the last term.

Nice find! It's likely it's only occasionally problematic because we fire requests both for a set of all terms, and those assigned to the post, and between the two, even with only one returned from "assigned to the post", most terms details are likely available for a majority of sites (except those with many tags).

The solution is an easy one, instead of:

&include=449&include=590&include=38

It should be:

include=449,590,38

I'll include a fix for this specific problem here. It highlights a DevEx concern over being an easy oversight, hopefully one which is more easily overcome with abstractions of the core data module planned to be introduced to these components.

Terms: Fetch terms as comma separated
Separated `include` arguments is invalid, and would return only the last of the set of parameters requested for include

@aduth aduth force-pushed the fix/term-selector branch from 07f0fb5 to 9113fcf Apr 2, 2018

@jorgefilipecosta
Copy link
Member

jorgefilipecosta left a comment

Hi @aduth, thank you for including the fix to the requests already in this PR, I confirmed it works as expected. So the changes are still good to 👍

@aduth aduth merged commit 3bb7c09 into master Apr 3, 2018

2 checks passed

codecov/project 44.14% (-0.02%) compared to a4c49cf
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bobbingwide bobbingwide referenced this pull request Apr 3, 2018

Closed

Tags drop down selection list usability issues #5951

0 of 2 tasks complete
@bobbingwide

This comment has been minimized.

Copy link
Contributor

bobbingwide commented Apr 3, 2018

Just to confirm I was able to produce the problem with only 2 terms attached to my post.

@aduth aduth deleted the fix/term-selector branch Apr 3, 2018

@danielbachhuber danielbachhuber added this to the 2.6 milestone Apr 3, 2018

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