Code review comment for lp:~jamestait/canonical-identity-provider/add-ax-fetch

> - missing commit message

Fixed. :)

> - instead of *_EMAIL_VERIFIED I'd use *_ACCOUNT_VERIFIED (as we should care
> about the status of the account, not the email; if the account is verified,
> the returned email is also verified).

This came about as a result of a misunderstanding on my part about the difference between a verified account and an account with a verified e-mail address. Happy to fix this.

> - l. 96-99: I'd extract this into a helper method for readability
> - l. 101-105: ditto

Yes, that would clean things up nicely.

> - l. 114-135: since these values (except email_verified) are the same for AX
> or SREG, why not have a common helper function to get them (DRY)?

Ditto.

> - l. 462-463: you can use {% elif ... %} here I believe?

Alas, not in Django 1.3. ;)

> - l. 516: why not get the entry again and confirm it has the fields you just
> posted?

Yes, that should work. I'll look into adding that extra verification, and maybe do the same for the SReg tests.

> - l. 531: this could match any checkbox in the form? why not rather add a
> specific test to verify email is listed as an ax field?

Indeed, this should be made more robust. I'll shore this up and again look to do the same for the SReg tests.

> - l. 613-614: you could also use assertIn, assertNotIn

Good catch!

> - l. 818-819,870-894,930-954,988-1024,1065-1086: we now tend to prefer using
> pyquery for asserting against the DOM

I'll look into this. I have to admit I was cringing when writing the tests, so I'm grateful for the suggestion of a better method.

> - l. 1166: can we call _get_openid_request with named params? it's hard to
> understand what each param means otherwise

Yes, that would be an improvement.

> - l. 1250: why not just do: if ax_form.data: ?
> - l. 1250-1253: or maybe add a helper to the form to get the values without
> having to know that data is a dict?

I prefer the latter approach. It had occurred to me to do something like this, but I wanted to get eyes on the code before I made any further changes.

Thanks for the comments!