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.

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

Add error message from api request. #15657

Merged
merged 10 commits into from Jun 28, 2019

Conversation

@spacedmonkey
Copy link
Contributor

commented May 15, 2019

Description

Improve error message when saving posts. More detail #14644

Fixes #14644.

How has this been tested?

  1. Create a post.
  2. Delete cookies.
  3. Save post.

Screenshots

Screenshot 2019-05-15 at 14 35 41

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
__( 'Updating failed' );
__( 'Updating failed.' );

if ( ! ( /<\/?[^>]*>/.test( error.message ) ) ) {

This comment has been minimized.

Copy link
@aduth

aduth May 15, 2019

Member

Is the idea with this to exclude messages with HTML? I think it may be agreeable as a first-pass improvement, but is there a fundamental reason we should want to exclude messages containing HTML?

Stripping the HTML tags seems reasonable to me, though the specifics of doing so are some combination of challenging and risky (example solution), depending if we consider the REST response to be "safe".

It might also help to provide some context to the future maintainer what's actually happening here, either with an inline code comment or extracting as a separate, helpfully-named function (e.g. stripTags), in scope or proposed as part of a relevant package (closest might be escape-html).

This comment has been minimized.

Copy link
@spacedmonkey

spacedmonkey May 17, 2019

Author Contributor

There reason we don't want html tags here, as some responses may contain hyperlinks or bullet points. The content may not make sense anymore if it says for example "Click on this link" and it is no longer a link.

I didn't make the tag check into a function, as I didn't seem like it would be useful to do so. But I can, if you so wish.

This comment has been minimized.

Copy link
@aduth

aduth May 21, 2019

Member

There reason we don't want html tags here, as some responses may contain hyperlinks or bullet points. The content may not make sense anymore if it says for example "Click on this link" and it is no longer a link.

I think it's okay to be conservative here as you propose, as long as there's the fallback messaging of "Publishing failed".

I didn't make the tag check into a function, as I didn't seem like it would be useful to do so. But I can, if you so wish.

My concern here is whether it will be obvious here to a future maintainer what this logic is intended to do. Whether that's an inline code comment or function, I have no strong preference. But as implemented currently, I foresee it being not immediately obvious to its purpose.

This comment has been minimized.

Copy link
@aduth

aduth Jun 12, 2019

Member

My concern here is whether it will be obvious here to a future maintainer what this logic is intended to do. Whether that's an inline code comment or function, I have no strong preference. But as implemented currently, I foresee it being not immediately obvious to its purpose.

Can we add a code comment here?

__( 'Updating failed.' );

if ( ! ( /<\/?[^>]*>/.test( error.message ) ) ) {
noticeMessage = noticeMessage + ' ' + error.message;

This comment has been minimized.

Copy link
@aduth

aduth May 15, 2019

Member

The concatenation† here has me thinking about localization: Based on the examples you shared at #14644 (comment), it seems we can generally expect that the messages are run through __ on the server, but it's worth checking that the user's locale preferences are in-fact represented in the UI (that the user context is known for these requests, the locale data is loaded for the user, the messages themselves are localized).

† Concatenation can be discouraged, though typically more problematic in cases of fragments within a sentence, rather than as two separate sentences.

This comment has been minimized.

Copy link
@spacedmonkey

spacedmonkey May 17, 2019

Author Contributor

It really depends when the requests dies, to if the users language is selected. If for example, there is a PHP fatal error or database error, it will load wp_load_translations_early, which will it's best to work users language but doesn't work 100% of the time.

Maybe it is better to just use a mixture of the error code and the error status and keep the translations in javascript. This may also make gutenberg a little more platform agnostic.

This comment has been minimized.

Copy link
@aduth

aduth May 21, 2019

Member

Maybe it is better to just use a mixture of the error code and the error status and keep the translations in javascript. This may also make gutenberg a little more platform agnostic.

Combined with the above discussion about how not all messages can be used directly from the response, this sounds promising. I'm not as sure that it benefits platform agnosticism, if the error codes are quite specific to WordPress (moreso than a message, which could be any text).

This comment has been minimized.

Copy link
@spacedmonkey

spacedmonkey Jun 4, 2019

Author Contributor

@aduth Updated PR to use sprintf 28579ed

@aduth

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

I requested some advice on the translateability of these strings in #core-i18n yesterday, since I wasn't personally too confident about guidelines concerning the updated strings and sprintf usage (link requires registration):

https://wordpress.slack.com/archives/C04KRK0KA/p1560181188005600

I just asked some follow-up questions, but it may be fine as-is. I don't think the use of sprintf in concatenating the two strings provides any benefit, but also not exactly harmful (aside from an additional performance cost in interpolating the string, maybe not as readable for other developers).

@spacedmonkey spacedmonkey force-pushed the spacedmonkey:feature/improve-error-message branch from de0cf93 to d7a1034 Jun 25, 2019

spacedmonkey added some commits Jun 25, 2019

@spacedmonkey

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

@aduth Update the PR with translations feedback and comment.

I also fixed some unit tests. The e2e do not seem to be running, but this seems to be unrelated.

@aduth

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

I restarted the build. There was one which was an intermittent failure, but the other looks like a genuine issue. The test case tries to look for the text, which was updated in this pull request:

'//*[contains(@class, "components-notice") and contains(@class, "is-error")]/*[text()="Updating failed"]'

@spacedmonkey

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

I updated tests to include a message.

That test is test failing, but I am not sure why...

@aduth

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

That test is test failing, but I am not sure why...

Manually repeating the steps of the test case, I can see that per the changes here, the notice also includes the error message, so the updated text query isn't enough to match the element.

image

I'm not very familiar with XPath, but we could update the query to query for a notice starting with the common "Updating failed." message (reference):

'//*[contains(@class, "components-notice") and contains(@class, "is-error")]/*[starts-with(text(),"Updating failed.")]'

Otherwise, just specifying the full text of the message:

'//*[contains(@class, "components-notice") and contains(@class, "is-error")]/*[text()="Updating failed. Error message: The response is not a valid JSON response."]'
@spacedmonkey

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Tests now pass 😄

@aduth

aduth approved these changes Jun 26, 2019

Copy link
Member

left a comment

Looks good 👍 Thanks for the work here.

Show resolved Hide resolved packages/editor/src/store/utils/notice-builder.js Outdated
Update packages/editor/src/store/utils/notice-builder.js
Change comment.

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
@aduth

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Thanks again 👍

@aduth aduth merged commit ffa4a27 into WordPress:master Jun 28, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@github-actions github-actions bot added this to the Gutenberg 6.1 milestone Jun 28, 2019

// Check if message string contains HTML. Notice text is currently only
// supported as plaintext, and stripping the tags may muddle the meaning.
if ( error.message && ! ( /<\/?[^>]*>/.test( error.message ) ) ) {
noticeMessage = sprintf( __( '%1$s Error message: %2$s' ), noticeMessage, error.message );

This comment has been minimized.

Copy link
@swissspidy

swissspidy Jul 1, 2019

Member

This is missing a translator comment to explain the parameters.

This comment has been minimized.

Copy link
@spacedmonkey

spacedmonkey Jul 1, 2019

Author Contributor

Should this be another PR?

This comment has been minimized.

Copy link
@swissspidy
@swissspidy

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

I'm really missing a translator comment here for the notice message. The placement of the placeholders is a bit unfortunate, one could easily think those are for opening and closing tags or something. Wasn't there originally a period after the first one?

@spacedmonkey

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

There was a full stop added to the end of each message, instead of in the concatting string, as that is confusing.

Tug added a commit that referenced this pull request Jul 9, 2019

Add error message from api request. (#15657)
* Add error message from api request.

* Use sprintf for error message.

* Add comment

* Fix tests for new error message

* More linting fixes

* Fix phpunit tests

* Improve the tests, to reflect message in error.

* Fix text of message in e2e.

* Update packages/editor/src/store/utils/notice-builder.js

Change comment.

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

jg314 added a commit to jg314/gutenberg that referenced this pull request Jul 19, 2019

Add error message from api request. (WordPress#15657)
* Add error message from api request.

* Use sprintf for error message.

* Add comment

* Fix tests for new error message

* More linting fixes

* Fix phpunit tests

* Improve the tests, to reflect message in error.

* Fix text of message in e2e.

* Update packages/editor/src/store/utils/notice-builder.js

Change comment.

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

Add error message from api request. (WordPress#15657)
* Add error message from api request.

* Use sprintf for error message.

* Add comment

* Fix tests for new error message

* More linting fixes

* Fix phpunit tests

* Improve the tests, to reflect message in error.

* Fix text of message in e2e.

* Update packages/editor/src/store/utils/notice-builder.js

Change comment.

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.