Code review comment for lp:~salgado/lava-dashboard/openid

On Fri, 2010-10-15 at 15:28 +0000, James Westby wrote:
> > I did that because the other one has the urls property, and that doesn't
> > include any of the app's urlpatterns as it only uses the test view
> > created for the test. That means the test method here can't post to the
> > 'xml-rpc' endpoint. One benefit of splitting, though, is that this test
> > doesn't need to use CSRFTestCase -- the regular TestCase will do.
>
> It does need CSRFTestCase, otherwise Django disables the csrf checks
> globally, meaning that it is not testing in the right environment. That's
> the sort of issue that means I want to stick to one test class.
>
> > I could try and combine them together again, by having the urls property
> > include all the app's urls plus the one that is specific for testing, if
> > you think it's worth the effort.
>
> I'm don't feel strongly about it, but I would have more confidence in the
> test. Doesn't it just need the xml-rpc view hooked up?
>
> Maybe the test urls can be loaded and extended in the property, rather than
> overwritten?

That's what I was thinking. It was easy to do except for the fact that
reverse("xml-rpc") stopped working, so I had to do
reverse(dashboard_xml_rpc_handler) instead.

> > Mostly ignorance. I know close to nothing about what should go in each
> > of the settings files of a django project, so I added it to the same
> > file that Michael added the lexbuilder one. Happy to move that to
> > default_settings.py, though.
>
> launch-control doesn't really use a django standard I don't think.
>
> It has three files two of which can override the defaults. What you
> have done is set that variable such that it can't be overriden in
> local_settings or deployment_settings like the others can. I would
> suggest moving it to default_settings for consistency.

Fair enough; I've moved it there.

I've also re-added the LOGIN_REDIRECT_URL line as Zygmunt requested.

Now we just need to decide whether we want to have only-OpenID login
enabled or both OpenID and password.