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

[Hongliang Wang]

On 02/27/2014 10:44 AM, Adam King wrote:
> 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.
OK. I see. It should be:

ajaxSettings['originalError'](jqXHR, textStatus, errorThrown);
>>>           });
>>>
>>>           kimchi.user.showUser(true);
>>
>