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

On Sun, Sep 22, 2013 at 11:12 PM, Paul Sokolovsky
<email address hidden> wrote:
> Review: Approve
>
> I have few concerns with crowd.py:
>
> 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
CrowdForbiddenException.

> 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
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs