Code review comment for lp:~mikemc/canonical-identity-provider/fix-1169632

Revision history for this message
Maximiliano Bertacchini (maxiberta) wrote :

Hi, I'm sorry I could not review this before. I've just run this branch and it worked as expected :-D. That said:

* There's a couple of hidden code conflicts (see comments inline). Please merge trunk, fix and run all tests again.
* I think the new "OpenID + mail verification" flow may be a little bit confusing for users as it redirects from the email verification view directly back to the RP. It'd be great if we could show the /+decide view as usual at the end of the OpenID dance instead of merging it into the email verification view.
* I think the email verification view should make that redirection more explicit. Maybe just show a warning (eg. see how it is handled in the OpenID username required case: https://bazaar.launchpad.net/~canonical-isd-hackers/canonical-identity-provider/trunk/view/head:/src/webui/views/account.py#L197).

Haven't checked the tests yet.

review: Needs Fixing

« Back to merge proposal