909368 - Gracefully handle AJAX errors

Status: RESOLVED FIXED

(Tree Management Graveyard :: OrangeFactor, defect)

Description

[Mark Côté [:mcote]]

Occasionally there are problems with the OF back end that result in 500 ISE errors (e.g. bug 909086).  There's currently no way to tell if the UI failed to load something, though, without using tools to view the network requests.  We should at least throw an error up on the UI to let users know that the request errored out.  We could also throw errors or warnings up if a request is on-going but taking much longer than expected.

Comment 1

[gautam.nishchal]

Hello,

I am a new contributor to Mozilla. I just learned about bugzilla and was wandering around a bit. I think I may want to work on this bug. Can anyone help me with some details on how can i work on a bug? Plus Some more details on this specific bug.

Thank you.

Comment 2

[Mark Côté [:mcote]]

Hi, thanks for your interest!  So how you should proceed with this bug will depend on your experience.  Unfortunately, running the back end requires connection to an ElasticSearch database.  The production database is behind a firewall, so you wouldn't be able to run the back end yourself.  However, it wouldn't be too hard to hack up orangefactor/server/woo_server.py to return 500 errors on all calls.  Then you could see the behaviour of the front end when the server is in an error state.  The fix would be adding error callbacks to all AJAX calls that would display a simple error message.  Let me know if you have more questions, or ask around in #ateam on irc.mozilla.org.

Comment 3

[Ed Morley [:emorley]]

Hi! The source is at:
https://hg.mozilla.org/automation/orangefactor/

Instructions for testing locally are at:
https://hg.mozilla.org/automation/orangefactor/file/db7b33a3b05e/README.txt

Comment 4

[Sarvesh( :s1991 )]

Attachment: error handling is accomplished by replacing $getJSON function to $ajax which can support error handling

Comment 5

[Sarvesh( :s1991 )]

Attachment: error handling is accomplished by replacing $getJSON function to $ajax which can support error handling

Comment 6

[Mark Côté [:mcote]]

Comment on attachment 8366086 [details] [diff] [review]
error handling is accomplished by replacing $getJSON function to $ajax which can support error handling

Review of attachment 8366086 [details] [diff] [review]:
-----------------------------------------------------------------

This is pretty good, and very close!  I just have a few comments.  Most are just about coding style (spacing/indentation), and one about code reuse.

::: html/scripts/woo.bugs.js
@@ +60,5 @@
>        var self = this;
> +      $.ajax({
> +        url: es_server + "/bugdetails?bugid=" + unknownBugIds.join(","),
> +        dataType: 'json',
> +        success: function( data ) {

To match the style of the rest of the files, please don't put extra spaces around function parameters.

@@ +217,5 @@
>    es_args += esTreeArg();
> +  $.ajax({
> +    url:es_server + "/bybug" + es_args,
> +    dataType: 'json',
> +    success: function(data) {

Please fix indentation of the lines below.  It will make the patch bigger, but I'd prefer to keep a somewhat clean coding style.

@@ +342,5 @@
>                  }
>                  $("#" + spanId + " .detailslinks").html(details);
>                }
> +            },
> +            error: function( data ){

Spacing.

@@ +420,5 @@
>    var bugdetails = {};
> +  $.ajax({
> +    url:es_server + "/bybug" + es_args,
> +    dataType: 'json',
> +    sunccess: function(data) {

Identation.

@@ +719,5 @@
>    es_args += esTreeArg();
> +  $.ajax({
> +    url: es_server + "/bybug" + es_args,
> +    dataType: 'json',
> +    success: function(data) {

Indentation.

@@ +807,5 @@
>    es_args += esTreeArg();
> +    $.ajax({
> +    url: es_server + '/correlations' + es_args,
> +    dataType: 'json',
> +    success: function( data ) {

Spacing.  Also the subsequent line is indented 4 when it should be 2 to match the rest of the file.

::: html/scripts/woo.dialog.js
@@ +33,5 @@
> +    return '[<a href="#" onclick="$(\'#' + dlgName + '\').dialog(\'open\'); return false;">?</a>]';
> +  };
> +
> +  return { helpDlgText: helpDlgText };
> +}();

This works, but it's very redundant with the Dialog function.  Do you think you could refactor it?  There are a couple ways to do it, but the easiest would probably be to make a function that takes the few parameters that change here (element ID, title, and width) and return a function using those parameters.  Ping me on IRC if you need me to explain more.

::: html/scripts/woo.main.js
@@ +27,5 @@
> +
> +  $.ajax({
> +    url: es_server + "/count" + es_args,
> +    dataType: 'json',
> +    success: function( data ) {

Spacing.

@@ +123,5 @@
>    es_args += esTreeArg();
> +  $.ajax({
> +    url:es_server + "/bybug" + es_args,
> +    dataType: 'json',
> +    success: function(data) {

Indentation.

::: html/scripts/woo.utils.js
@@ +1036,5 @@
>    var getQuicksearchResponse = function(filterId) {
> +    $.ajax({
> +      url: es_server + "/bugquery" + "?bzquery=" + filterSettings[filterId].filterDetails.quicksearch,
> +      dataType: 'json',
> +      success: function(data) {

Indentation.

Comment 7

[Sarvesh( :s1991 )]

Attachment: error handling is accomplished by replacing $getJSON function to $ajax which can support error handling and creating Dialog function

dialog function added

Comment 8

[Mark Côté [:mcote]]

Comment on attachment 8366698 [details] [diff] [review]
error handling is accomplished by replacing $getJSON function to $ajax which can support error handling and creating Dialog function

Review of attachment 8366698 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there! :)

::: html/scripts/woo.bugs.js
@@ +420,5 @@
>    var bugdetails = {};
> +  $.ajax({
> +    url: es_server + "/bybug" + es_args,
> +    dataType: 'json',
> +    sunccess: function(data) {

Typo spelling "success"!

@@ +808,5 @@
> +  $.ajax({
> +    url: es_server + '/correlations' + es_args,
> +    dataType: 'json',
> +    success: function(data) {
> +        data.minoccurs = 3;

Indented too far.

::: html/scripts/woo.dialog.js
@@ +21,5 @@
> +}
> +
> +Dialog("dlgHelp", "Help", 800);
> +
> +Dialog("dlgErrorHandler", "Error", 300);
\ No newline at end of file

Hm this isn't quite what I meant, since changing Dialog from an object to a function will break the use of the Dialog object in woo.utils.js.  I meant that you should make a separate function that returns an object, which you can still assign to the Dialog variable (and to a new errorDialog object).

However I've thought more about this, and you don't actually need that object for error dialogs.  The usages of the existing Dialog object and your new error dialog are different, since the user can summon a Dialog object by clicking on the [?] link, but the error dialog is always opened in code, never by the user.  Thus the error dialog doesn't need the helpDlgText function.

So, how about you go back to the way it was originally, but instead of creating an errorDialog object, just call the first part (the .each()) for the dlgErrorHandler divs after initializing the Dialog object.  In other words, you're just calling the jquery dialog function on your dlgErrorHandler, but you aren't making a proper object out of it, since you don't need the extra functionality.  This will be a bit redundant, but little enough that I don't care. :)

Sorry for the confusion!

Comment 9

[Sarvesh( :s1991 )]

Attachment: error handling is accomplished by replacing $getJSON function to $ajax which can support error handling and creating Dialog function

Comment 10

[Mark Côté [:mcote]]

Comment on attachment 8366771 [details] [diff] [review]
error handling is accomplished by replacing $getJSON function to $ajax which can support error handling and creating Dialog function

Review of attachment 8366771 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  But I realized we need one more modification: we should construct the error message dynamically from the contents of the data object passed to the $.ajax() error callback.  I will commit this as is, since it is an improvement, and the errors are usually 500, but we should make sure the error text is accurate as a separate patch.

Comment 11

[Mark Côté [:mcote]]

http://hg.mozilla.org/automation/orangefactor/rev/1deab2f0b812

Comment 12

[Sarvesh( :s1991 )]

Attachment: Error function added to call error dialog

Comment 13

[Mark Côté [:mcote]]

Comment on attachment 8366816 [details] [diff] [review]
Error function added to call error dialog

Review of attachment 8366816 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  This works but just needs a couple little changes to make it a bit cleaner.

::: html/index.html
@@ +176,5 @@
>  
>                <p>If necessary, you can get more complicated: if you choose to include the top 10 bugs and exclude the
>                top 15, you will get information on the 16th to 25th highest-ranking bugs, inclusive.</p>
>  	    </div>
>        <div id="dlgErrorHandler500">

This should now be called dlgErrorHandler, since it can represent any error.

::: html/scripts/woo.bugs.js
@@ +349,1 @@
>                  $('#dlgErrorHandler500').dialog('open');

Since you made a function to update the dlgErrorHandler div, you should move this line there too, so that all the dialog-manipulation code is in the same place.  It's cleaner and easier to change in the future.

Same with the other error callbacks that follow.

Comment 14

[Sarvesh( :s1991 )]

Attachment: Error function added to call error dialog

Comment 15

[Mark Côté [:mcote]]

Comment on attachment 8367170 [details] [diff] [review]
Error function added to call error dialog

Review of attachment 8367170 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!  I'll commit this and push it all out to production shortly.  Thanks!

Comment 16

[Mark Côté [:mcote]]

Pushed and deployed!

http://hg.mozilla.org/automation/orangefactor/rev/368d1befd741