#333 Move the plus-plus views to the contact widget
Merged by abompard. Opened by abompard.
abompard/fedora-hubs plus-plus-views  into  develop

Download 333.patch

The "plus-plus" views are actually specific to the "contact" widget, move them there.

rebased

For PEP8 imports are ordered by standard library, then third-party libraries, then the current package:

from __future__ import unicode_literals
import json
from mock import mock  # I *think* this can just be `import mock` based on its usage below
import flask
from hubs....
import hubs

Not a big deal, but it makes it easy to see what's coming from where at a glance.

Did you mean 404 here?

Does flask not handle unhandled exceptions with an HTTP 500 anyway?

You probably want to add a timeout to this class. It's also worth noting using requests this way won't take advantage of connection pooling. You'll need to create a thread-local session for that.

I think this should be a 404 as well.

Should this be a 400 since it's a bad request?

Pretty much the same comments as the above requests try/except block.

1 new commit added

  • Implement review suggestions by @jcline

I did all that, except the request connection pooling which probably needs to be at a higher level to be useful to all widgets. By the way, Mailman has a different policy on the import order, that's why I got mixed up. Thanks for your review.

Sorry I forgot to mention this during my initial review, but if you wanted to be fancy here, you could do

try:
    req = requests.get(pp_url, timeout=5)
except requests.Timeout:
    return 'The request to {ur} timed out'.format(url=pp_url), 504

That way the client knows the upstream service was the problem.

@abompard, one more comment I didn't think to make first time round (sorry!), and I went ahead and filed an issue about setting up a session. I'll probably take that issue on today.

Oh, that's a good idea, thanks :-)

1 new commit added

  • Contact: return a specific error on timeouts

Cool, looks good to me!

Pull-Request has been merged by abompard

Metadata