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

667/1 To square a bit error handling on the API side of our service. Fix #667 #678

Merged
merged 17 commits into from
Aug 20, 2015

Conversation

karlcow
Copy link
Member

@karlcow karlcow commented Aug 18, 2015

  • Added a couple of tests
  • Added error messages in JSON. Handled at the same level than HTML in views.py
  • Removed some hard code failures with some URIs

r? @miketaylr

@miketaylr
Copy link
Member

It looks like this breaks the following:

FAIL: main - issues - non-existant issues go to 404 (10532ms)
NoSuchElement: [POST http://localhost:4444/wd/hub/session/dfa77d4d-9275-489d-a394-4177c50ba6b2/element / {"using":"css selector","value":"#pageerror h1"}] Unable to locate element: {"method":"css selector","selector":"#pageerror h1"}

Can you investigate?

The expected behavior is that going to /issues/484748493945 (or whatever) ends up at the 404 page.

@miketaylr
Copy link
Member

Instead we get a 404 on the response, which means we end up with an empty page.

@karlcow
Copy link
Member Author

karlcow commented Aug 18, 2015

in https://github.com/webcompat/webcompat.com/blob/master/tests/functional/issues.js#L93-L101

    'non-existant issues go to 404': function() {
      return this.remote
        .setFindTimeout(intern.config.wc.pageLoadTimeout)
        // TODO: uh, update this in the future
        .get(require.toUrl(url(999999)))
        .findByCssSelector('#pageerror h1').getVisibleText()
        .then(function (text) {
          assert.include(text, '(404)', 'We\'re at the 404.');
        });
    },

On the CLI, a request returns a 200 HTML file, which it should not, so I guess it's why it's failing.

→ http GET http://localhost:5000/issues/1000000
HTTP/1.0 200 OK
Content-Length: 10291
Content-Type: text/html; charset=utf-8
Date: Tue, 18 Aug 2015 21:33:01 GMT
Server: Werkzeug/0.9.4 Python/2.7.6

I will investigate later today.

@karlcow
Copy link
Member Author

karlcow commented Aug 18, 2015

This is what is happening today on webcompat.com (which is somehow bad ;) )

capture d ecran 2015-08-19 a 06 49 57

And this is what is happening with this PR (which is equally bad HTTP wise)

capture d ecran 2015-08-19 a 06 53 27

I guess the JavaScript client receiving the 404 answer was expecting something different to send back the html 404.

@miketaylr
Copy link
Member

I think we just have to change the message here: https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/issues.js#L223-L225 to the new message that you're sending ("API call not found" (or whatever it is).

@karlcow
Copy link
Member Author

karlcow commented Aug 18, 2015

Ah yes! That's it! Thanks miketaylr. I was already going through the line of codes. Excellent.
Thanks. I will add a commit for this.

@karlcow
Copy link
Member Author

karlcow commented Aug 18, 2015

Ok hopefully the parsing is fine now.

@karlcow
Copy link
Member Author

karlcow commented Aug 19, 2015

Locally I get now the 404 http://localhost:5000/404

@miketaylr
Copy link
Member

Awesome! Looks good now, thanks @karlcow.

miketaylr pushed a commit that referenced this pull request Aug 20, 2015
667/1 To square a bit error handling on the API side of our service. Fix #667
@miketaylr miketaylr merged commit ce4d83e into webcompat:master Aug 20, 2015
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

Successfully merging this pull request may close these issues.

2 participants