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

Hi James,

Thanks for the review!

On Thu, 2010-10-14 at 22:44 +0000, James Westby wrote:
> Review: Needs Information
> 60 +
> 61 +class NoCSRFProtectionOnXMLRPCConfigurationTestCase(CSRFTestCase):
> 62 +
>
> How crucial is it to split this test?
>
> I pushed for it all to be one TestClass in order to get more confidence that
> the test was passing because it was working, and not just because django
> disables the checks for tests.

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.

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.

>
> 85 + <table>{{ form.as_table }}</table>
>
> Are you still running under 1.1 on lucid? Have you tried this on 1.2? I
> have a suspicion that you need another tag in there to put the CSRF token
> in the HTML under 1.2.

As we discussed on IRC, launch-control is using the legacy method of
protection against CSRF, which doesn't require a template tag. Once we
drop support for django 1.1 we can move away from the legacy method.
I've added a comment explaining that to the relevant bit of code.

>
> 175 + def tearDown(self):
>
> We need an upcall to the super's tearDown.
>

Good catch; added.

> 177 + settings.OPENID_SSO_SERVER_URL = self.orig_setting
>
> (optional) make the instance variable "orig_sso_server_url" or something,
> in case we have to override a second later.

Renamed.

>
> Will that code handle the SSO option being removed from settings.py?

Probably not, so I've changed it to cope with that.

>
> 182 + useropenid = UserOpenID(
> 183 + user=user, claimed_id=self._identity_url,
> 184 + display_id=self._identity_url)
>
> What does that do?

Associates the newly created user with its identity URL.
(added this as a comment to the code)

>
> 258 +oidutil.log = lambda msg, level=0: None
>
> Maybe settings.py?

WFM.

>
> 272 +TEMPLATE_CONTEXT_PROCESSORS = (
>
> Is there a reason that's not in default_settings.py?

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.

--
Guilherme Salgado <https://launchpad.net/~salgado>