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

Ian, thanks for this. I had some trouble initially going through it and so I've got some feedback, much is optional, but I wanted to share and see if any of it is useful to you.

First, just a couple of points. Everything here seems like it should work out ok, so I'm marking approved with some minor notes:

In your tests you setup a LP = {}, but the test write out a window.LP. Then the code uses just LP. If you reference things as window. I'd find it clearer for others to see all occurances using that so that it's explicit vs trusting that LP is found up the scope chain.

In the tests you use var reviewer = Y.one("[name='field.reviewer']"); where as the rest of the code is all referring to things by the id. Is there a reason you went with an attribute search?

The validation namespace is ENV., but you're using YUI.namespace which the docs seem to indicate is global already? I know it has to match what's already there, so this is more a question at this point than a request for change.

There's enough html generation in there that I think it'd be nice to clean it up using Mustache. You could use it to build the content, place the person name, and loop through the branches.

Now onto the optional stuff. I tend to try to set this up in a YUI object to make it a bit easier to follow and track the various functions defined and used. They're all placed upon the namespace, and I understand that for testing, but then it makes them all potential calling/entry points for code elsewhere.

I've mocked up (completely not run through/tested/etc) a YUI object with a normal YUI initialize and a setup() method to mimic what it appears you've got there. I also added the Mustache templating to kind of show how confirm_reviewer simplifies a little bit. I've tried to indicate "private" methods with _ vs the ones that appear to be externally callable for real use.

https://pastebin.canonical.com/59299/

Consider this just a transfer of technique and take from it what you will. Let me know if you have any questions or concerns. I understand this is hooking into existing picker code.