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

Revision history for this message
Milo Casagrande (milo) wrote :

On Sun, Sep 22, 2013 at 11:12 PM, Paul Sokolovsky
<email address hidden> wrote:
> Review: Approve
> 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).

Addressed this, even if I think more refactoring might be needed.

> 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.
> 3.
> 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.

Treated 401 and 403 separately: CrowdUnauthorizedException and

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

It is not, and I hope we can reuse it in some way. I didn't want to
jump into it right away, because I would have had to go through the
same studies and tests you have done, spending more time on it.

Milo Casagrande | Automation Engineer <> │ Open source software for ARM SoCs

« Back to merge proposal