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

Revision history for this message
James Westby (james-w) wrote :

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.

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.

175 + def tearDown(self):

We need an upcall to the super's tearDown.

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.

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

182 + useropenid = UserOpenID(
183 + user=user, claimed_id=self._identity_url,
184 + display_id=self._identity_url)

What does that do?

258 +oidutil.log = lambda msg, level=0: None

Maybe settings.py?

272 +TEMPLATE_CONTEXT_PROCESSORS = (

Is there a reason that's not in default_settings.py?

Overall this looks great though, thanks.

James

review: Needs Information

« Back to merge proposal