[Kimchi-devel] [PATCH 3/4] Pass ajaxError information on to original requester on ajaxError event

[Hongliang Wang]

On 02/27/2014 08:47 AM, Adam King wrote:
> The previous implementation on the ajaxError event did not pass along all the
> available information to the original caller.  Corrected with this patch.
> Updated guest vms processing to use the additional information.
> Corrected the error handler to show the JSON response reason when available,
> and show the new error message whenever a response is not avail.
>
> Signed-off-by: Adam King <rak at linux.vnet.ibm.com>
> ---
>   ui/js/src/kimchi.guest_main.js | 9 +++++++--
>   ui/js/src/kimchi.main.js       | 4 +++-
>   2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/ui/js/src/kimchi.guest_main.js b/ui/js/src/kimchi.guest_main.js
> index bbc8051..74bf7ad 100644
> --- a/ui/js/src/kimchi.guest_main.js
> +++ b/ui/js/src/kimchi.guest_main.js
> @@ -148,8 +148,13 @@ kimchi.listVmsAuto = function() {
>           }
>
>           kimchi.vmTimeout = window.setTimeout("kimchi.listVmsAuto();", 5000);
> -    }, function(err) {
> -        kimchi.message.error(err.responseJSON.reason);
> +    }, function(errorResponse, textStatus, errorThrown) {

> +        if(errorResponse.responseJSON && errorResponse.responseJSON.reason) {
> +            kimchi.message.error(errorResponse.responseJSON.reason);
> +        }
> +        else {
> +            kimchi.message.error(i18n['KCHAPI6007E'].replace("%1", errorResponse.state()));
> +        }
Good point to include network failure here! Though seems need 
improvement. Consider this: server encounters an internal error which 
will cause a 5xx response code and our code will run into else{} block, 
because response body has no responseJSON defined for 5xx error. And in 
your last patch,

KCHAPI6007E tells user "Can not contact the host system. Verify the host system is up and that you have network connectivity to it. HTTP request response %1. " which is wrong. Kimchi runs  well and network is connected. In this case, user should be told "Some internal error occurs." or something likewise.

     In fact, I considered the error message improvement at a global viewpoint but still have no time to work on it. It's should be handled in every Ajax requests including this listVMsAuto() function. You can put this kind of code into the global Ajax error listener $(document).bind('ajaxError', ...) which is in kimchi.main.js as you touched below.

>           kimchi.vmTimeout = window.setTimeout("kimchi.listVmsAuto();", 5000);
As we've known there's error with this part, why do we still 
continuously send this request that will continuously tell the user the 
same error message?

     Suggest not include this line as previous logic does.
>       });
>   };
> diff --git a/ui/js/src/kimchi.main.js b/ui/js/src/kimchi.main.js
> index c9d56e4..6bae95f 100644
> --- a/ui/js/src/kimchi.main.js
> +++ b/ui/js/src/kimchi.main.js
> @@ -163,7 +163,9 @@ kimchi.main = function() {
>                   return;
>               }
>
> -            ajaxSettings['originalError'] && ajaxSettings['originalError'](jqXHR);

> +            if(ajaxSettings['originalError']) {
> +                jqXHR.fail(ajaxSettings['originalError']);
> +            }
Seems it's unnecessary to make this change. Take a look at this whole block:

$(document).bind('ajaxError', function(event, jqXHR, ajaxSettings, 
errorThrown) {
     // ... code above

     ajaxSettings['originalError'] && ajaxSettings['originalError'](jqXHR);
});

It will run into this block only when some Ajax error occurs that we 
bind "ajaxError" event. So we only need to run the error callback 
directly without to bind it again to jqXHR.fail() function. it's a 
redundancy.

     Suggest keep this code without any change.
>           });
>
>           kimchi.user.showUser(true);