Code review comment for lp:~wallyworld/launchpad/confirm-reviewer-subscription-330290

Ian--

Thanks for this branch, I know it's been a real pain.

I think Rick's suggestion for mustache would clean up that html generation block a good bit, but I wouldn't spend more than an hour trying to get it to work--just make an XXX after that amount of time if not, because we do need to get this part of disclosure over with.

I like Rick's mock, but I can see that you're dealing with the existing issues of the picker in this code. At a minimum, I would ask that if you not take his suggestions at this time, you file a bug and link in his mock as an approach in a bug comment, so that someone on purple can do more cleaning of the pickers when we're on maintenance.

I do wonder at the need for the global lp_client variable; I'm guessing it's so you can pass the mock io in the conf in testing? It looks like a possible problem waiting to happen. Perhaps you could create a function factory that sets up the necessary lp.client for check_reviewer_can_see_branches?

I'm marking this as approved, b/c again I don't see this as a blocker or as something that more than an hour of your day should be spent in trying to fix--there's nothing actually wrong with it per se, but I'm leery of it.