Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notify on error - especially re non-published doc (400) #7

Open
rufuspollock opened this issue May 15, 2013 · 6 comments
Open

Notify on error - especially re non-published doc (400) #7

rufuspollock opened this issue May 15, 2013 · 6 comments

Comments

@rufuspollock
Copy link
Contributor

No description provided.

@KrishnaPG
Copy link
Contributor

Not sure if this was implemented - but looks like the problem still exists.

When supplied with invalid urls and non-existent spreadsheet pages, my.fetch() is silently dying with 400 bad-request error (at somewhere in JQuery.js line 9666 xhr.send() etc.).

We (end-developer / caller of dataset.fetch()) are not getting a chance to know about this error to show a user-friendly error. The error is slilently going to console and end-user will never have a clue.

It would be great if the caller of fetch() gets a chance to handle these failures. How about making the fetch() method accept success and failure callbacks which can be passed onto internal ajax calls?

Or, fetch() can be left untcouhed, but inside its code, my.fetch() should forward all .done, .fail and .always calls correctly. Right now .fail() is not getting called. Here is the sample code.

        gDocsDataset.fetch().done(function (results) {
            if (console) { console.log(results.records); }
        })
        .fail(function () {
            console.log("failed");  // <- Never gets called !!
        })
        .always(function () {
            console.log('always');
        });

@rufuspollock
Copy link
Contributor Author

@KrishnaPG you are right this had not got fixed (hence the issue still open). I think the reason .fail() does not get called is that 400 may not trigger a fail (we may need to check status in done). This is just me guessing but if we checked xhr status in done we may find that.

@KrishnaPG
Copy link
Contributor

Thank you @rgrp I have looked into the failure callstack and the problem was with the jQuery.getJSon()'s failure handlers.

I have added the failure cases to reject the fetch promise (please see the code changes of pull request: #12) and now the dataset.fetch() caller gets the failure notification correctly. Here is a sample code that shows an error text in case of failure and gridView in case of success.

    'click a': function (e, template) {
        var gDocsURL = template.find('#gDocsURI').value;
        var gDocsDataset = new recline.Model.Dataset({
            url: gDocsURL,
            backend: 'gdocs'
        });
        var gridView = new recline.View.Grid({ model: gDocsDataset });

        var domGridViewNode = template.find('#gDocsGridView');
        var childNode = domGridViewNode.lastChild;  // cleanup old view nodes and add new view node in DOM
        while ((childNode = domGridViewNode.lastChild) != null) domGridViewNode.removeChild(childNode);
        domGridViewNode.appendChild(gridView.el);

        gDocsDataset.fetch().done(function (results) {
            // nothing to do - view will update automatically
        })
        .fail(function (errObj) {
            if (errObj != null) {
                var div = document.createElement('div'); 
                div.className = 'alert alert-danger'; 
                div.innerHTML = "  <span class=\"glyphicon glyphicon-exclamation-sign\" aria-hidden=\"true\"></span>  <span class=\"sr-only\">Error: </span>" + errObj.statusText + ": " + errObj.responseText + "<br/><ul><li>Url: " + gDocsURL + "</li></ul>";
                domGridViewNode.insertBefore(div, domGridViewNode.firstChild);
            }
        });
    }

I tested this change with the test suite. They all succeeded except for one case I am listing below.

Tests completed in 32 milliseconds.
13 assertions of 14 passed, 1 failed.
Backend GDocs: GDocs Backend (1, 0, 1)Rerun7 ms
Died on test #1     at QUnit.test.global.test (http://okfnlabs.org/recline/test/sinon-qunit/1.0.0/sinon-qunit.js:60:16)
    at http://localhost:8000/Codebase/recline.backend.gdocs/test/backend.gdocs.test.js:275:1
    at http://localhost:8000/Codebase/recline.backend.gdocs/test/backend.gdocs.test.js:344:3: Cannot read property 'fail' of undefined
Source:     
TypeError: Cannot read property 'fail' of undefined
    at http://localhost:8000/Codebase/recline.backend.gdocs/backend.gdocs.js:33:10
    at Object.my.fetch (http://localhost:8000/Codebase/recline.backend.gdocs/backend.gdocs.js:36:6)
    at Object.<anonymous> (http://localhost:8000/Codebase/recline.backend.gdocs/test/backend.gdocs.test.js:292:25)
    at Object.<anonymous> (http://okfnlabs.org/recline/test/sinon/1.7.1/sinon.js:3987:35)

Not sure what it is. It is talking about some sinon.js. Could you please look into it?

@KrishnaPG
Copy link
Contributor

Thank you for the pull /merge.

Now the github code contains the fail handler - but the online version http://okfnlabs.org/recline.backend.gdocs/backend.gdocs.js is still pointing to old code - how to update it?

(Is that link a CDN and ok to point to directly from user code ? or do you have / recommend any CDN for accessing these recline files online (something like cdnjs) ?)

@rufuspollock
Copy link
Contributor Author

We need to merge master into gh-pages. I've done that now!

@KrishnaPG
Copy link
Contributor

Thank you Rufus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants