Show an error message when an error response does not have JSON #157

sh19910711 · 2015-08-11T04:47:22Z

After stopping a web server, the console is not doing anything even if a user types commands. It might cause a little confusion to the users.

This pull request is to show an error message in such that case, and also show a HTTP status code when an error response does not have a JSON-formatted text (see the test case: fb979f8).

Example

>> input
=> result
( stop web server )
>> input
ERROR: connection is refused

GIF: start server => stop server => restart server

Thanks.

sh19910711 · 2015-08-11T04:48:09Z

extensions/chrome/js/background.js

  function handleMessage(req, sender, sendResponse) {
    if (req.type === 'request') {
      var url = tabInfo[req.tabId].remoteHost + '/' + req.url;
      REPLConsole.request(req.method, url, req.params, function(xhr) {
-        sendResponse({ status: xhr.status, responseText: xhr.responseText });
+        sendResponse(extractProps(xhr));


This change is to use xhr.statusText.

gsamokovarov · 2015-08-11T19:34:47Z

Didn't we issue the message before? Also most of the messages we currently have are pretty prosaic. I think its okay to drop the ERROR: prefix and just drop a sentence with a hint of what's going on.

sh19910711 · 2015-08-12T03:40:06Z

OK, updated:

Edit the error message (diff: fb979f8...f392b1a)
Add a fix for the test:templates rake task (ref: 2871284)

And here is a diff of the message:

- ERROR: connection is refused
+ Oops! Failed to connect to Web Console middleware.
+ Please make sure that web server is running.

Thanks!

gsamokovarov · 2015-08-12T15:57:30Z

Add a fix for the test:templates rake task (ref

Can we move this to its own PR?

sh19910711 · 2015-08-13T04:38:50Z

Can we move this to its own PR?

OK, I have moved it.

gsamokovarov · 2015-08-13T13:35:18Z

lib/web_console/templates/console.js.erb

+
+  function getErrorText(xhr) {
+    if (!xhr.status) {
+      return "Oops! Failed to connect to Web Console middleware.\n" +


It may be a good idea to collect all the messages we have and put them in config/locale/en.yml or something like this. It will open the door for internationalization and will get rid of those hard-coded messages from the code-base. Just some food for thought.

Yeah, let's do it.

It may be my native language parasite skills kicking in, but maybe we can reword the following sections:

- connect to Web Console + connect to the Web Console

... and:

- Please make sure that web server is running. + Please make sure a rails development server is running.

It's a valuable point to me :-)

gsamokovarov · 2015-08-13T15:26:02Z

Let's make a final message pass and this PR is good to go. 👍

sh19910711 · 2015-08-14T07:55:06Z

Updated:

Move WebConsole::Template::Context into WebConsole::View (ref: ad17a5a)
- We can reuse it on the FakeMiddleware class as well.
Use I18n module for system messages instead of hard-coded value (ref: fb55d48)
- Put the messages in config/locales/en.yml

Thank you!

gsamokovarov · 2015-08-14T08:10:44Z

lib/web_console/locale.rb

@@ -0,0 +1,2 @@
+require 'active_support/core_ext/string/access'


Hey, thanks for doing this!

You can actually put it inside an initializer in the Railtie.

You can actually put it inside an initializer in the Railtie.

OK, I will change it.

sh19910711 · 2015-08-14T09:40:11Z

Updated again ♻️

Move lib/web_console/config/locales into lib/config/locales
Move the i18n configuration into railtie.rb
- but I'm not sure I'm doing it correctly, so let me know if I'm mistaking.

Thanks.

gsamokovarov · 2015-08-14T10:43:56Z

lib/web_console/railtie.rb

@@ -66,5 +66,9 @@ class Railtie < ::Rails::Railtie
        Middleware.whiny_requests = config.web_console.whiny_requests
      end
    end
+
+    initializer 'i18n.load_path' do


Exactly that, yes! 👍

gsamokovarov · 2015-08-14T10:44:05Z

Thanks a lot!

Show an error message when an error response does not have JSON

sh19910711 reviewed Aug 11, 2015
View reviewed changes

sh19910711 force-pushed the patch/0020/show-error-message branch 3 times, most recently from f490f58 to 2871284 Compare August 12, 2015 03:20

gsamokovarov added the gsoc label Aug 12, 2015

sh19910711 force-pushed the patch/0020/show-error-message branch from 2871284 to 87ab763 Compare August 13, 2015 04:31

sh19910711 mentioned this pull request Aug 13, 2015

Implicitly return exit status on the "test:templates" rake task #158

Merged

gsamokovarov reviewed Aug 13, 2015
View reviewed changes

sh19910711 force-pushed the patch/0020/show-error-message branch from 87ab763 to 4575a2b Compare August 14, 2015 07:48

gsamokovarov reviewed Aug 14, 2015
View reviewed changes

sh19910711 added 4 commits August 14, 2015 17:48

Move WebConsole::Template::Context into WebConsole::View

7d1f547

Use I18n module for system messages instead of hard-coded value

25889be

Show an error message when a response does not have a JSON text

cd0cc02

Add test for the error response without JSON output

0f47ed9

sh19910711 force-pushed the patch/0020/show-error-message branch from 4575a2b to 0f47ed9 Compare August 14, 2015 09:20

gsamokovarov reviewed Aug 14, 2015
View reviewed changes

gsamokovarov added a commit that referenced this pull request Aug 14, 2015

Merge pull request #157 from sh19910711/patch/0020/show-error-message

ba5a850

Show an error message when an error response does not have JSON

gsamokovarov merged commit ba5a850 into rails:master Aug 14, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show an error message when an error response does not have JSON #157

Show an error message when an error response does not have JSON #157

sh19910711 commented Aug 11, 2015

sh19910711 Aug 11, 2015

gsamokovarov commented Aug 11, 2015

sh19910711 commented Aug 12, 2015

gsamokovarov commented Aug 12, 2015

sh19910711 commented Aug 13, 2015

gsamokovarov Aug 13, 2015

sh19910711 Aug 13, 2015

gsamokovarov Aug 13, 2015

sh19910711 Aug 14, 2015

gsamokovarov commented Aug 13, 2015

sh19910711 commented Aug 14, 2015

gsamokovarov Aug 14, 2015

sh19910711 Aug 14, 2015

sh19910711 commented Aug 14, 2015

gsamokovarov Aug 14, 2015

gsamokovarov commented Aug 14, 2015

		@@ -0,0 +1,2 @@
		require 'active_support/core_ext/string/access'

Show an error message when an error response does not have JSON #157

Show an error message when an error response does not have JSON #157

Conversation

sh19910711 commented Aug 11, 2015

Example

GIF: start server => stop server => restart server

Choose a reason for hiding this comment

gsamokovarov commented Aug 11, 2015

sh19910711 commented Aug 12, 2015

gsamokovarov commented Aug 12, 2015

sh19910711 commented Aug 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsamokovarov commented Aug 13, 2015

sh19910711 commented Aug 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sh19910711 commented Aug 14, 2015

Choose a reason for hiding this comment

gsamokovarov commented Aug 14, 2015