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

[Adam King]

Thanks for taking the time to review....
A few comment responses inline below.

On 2/26/2014 9:15 PM, Hongliang Wang wrote:
> 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.

A good point. We do need to improve the overall error handling.
>
>>           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.
I thought about leaving it out, or altering the retry interval. I 
ultimately decided to keep it as is because it allows the user to begin 
using the server again almost as soon as it comes back up. The flashing 
error message is annoying, but I put that under the heading of improved 
error feedback. Its no worse than the other error feedback, just no better.

We should probably think about improving error feedback on two axes:
     1) improving  the errors we can detect and the messages that 
describe them
     2) improving how we communicate errors to the user.
>>       });
>>   };
>> 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.
I see how it looks redundant, but it isn't. The previous code only 
passed the single argument to the original error handler.
If the response were not back, you are correct it would rebind the error 
handler.
In this situation the response is back, so the original error handler is 
called immediately, but all the parameters are passed.
>>           });
>>
>>           kimchi.user.showUser(true);
>

-- 
Adam King <rak at linux.vnet.ibm.com>
IBM C&SI