Code review comment for lp:~milo/linaro-patchmetrics/crowd-teams

Revision history for this message
Paul Sokolovsky (pfalcon) wrote :

I have few concerns with

1. it doesn't follow DRY, for example has "/crowd/rest/usermanagement/1" twice (even rough example for Crowd API access addressed that providing layered functions for API calls).

2. I find "login_with" to be pretty confusing name for a factory method. It rather would be called something like "connect", though I don't understand why that work can't be done in old good constructor.

264 + elif resp.status == 401 or resp.status == 403:
265 + raise CrowdException("Impossible to fulfill the request")

403 is rather important HTTP result code to shove it together with any other code, especially not mentioning "forbidden" explicitly in a result.

4. Crowd credentials storage scheme is not compatible with django-crowd-backend which would be used for user auth support.

I'd suggest to do something about 3. before merging. Otherwise, based on the discussion in the email (i.e. confirming it was tested), this can go in, so we'll definitely need to address 4 later.

review: Approve

« Back to merge proposal