Hi Brad,

This looks pretty good. I have only a few comments/suggestions and one
request for a missing test.

 review needsfixing

On Fri, 2010-01-08 at 13:36 +0000, Brad Crittenden wrote:
> Brad Crittenden has proposed merging lp:~bac/launchpad/bug-419930 into
> lp:launchpad/devel.
> 
>     Requested reviews:
>     Canonical Launchpad Engineering (launchpad)
> Related bugs:
>   #419930 non-public email address shown after merge request
>   https://bugs.launchpad.net/bugs/419930
> 
> 
> = Summary =
> 
> Users can choose to hide their email addresses and we should always
> honor that
> request.  Currently anyone who attempts to merge a person with hidden
> email addresses
> is shown all addresses for that user.
> 
> Also did a drive-by fix to the person picker vocabulary that was not
> catching an
> Unauthorized exception which prevented people with hidden emails from
> being selected
> in the picker.

Does that mean we can't merge people with hidden email addresses unless
we know their Launchpad ID or email address?

> 
> == Proposed fix ==
> 
> Rather than displaying the addresses and letting the user choose which
> ones to
> contact we should just show a count of addresses and send notification
> to them all.
> 
> == Pre-implementation notes ==
> 
> Quick call with Curtis as a sanity check of the approach.
> 
> == Implementation details ==
> 
> As above.
> 
> == Tests ==
> 
> bin/test -vvm lp.registry -t merge-people.txt
> 

> === modified file 'lib/canonical/launchpad/browser/vocabulary.py'
> --- lib/canonical/launchpad/browser/vocabulary.py       2009-07-31 19:38:18 +0000
> +++ lib/canonical/launchpad/browser/vocabulary.py       2010-01-08 13:36:23 +0000
> @@ -82,7 +82,7 @@
>  def person_to_pickerentry(person):
>      """Adapts IPerson to IPickerEntry."""
>      extra = default_pickerentry_adapter(person)
> -    if person.preferredemail is not None:
> +    if person.preferredemail is not None and not person.hide_email_addresses:

I didn't see a test for this; care to add one?

>          extra.description = person.preferredemail.email
>      return extra
>  
> 
> === modified file 'lib/lp/registry/browser/peoplemerge.py'
> --- lib/lp/registry/browser/peoplemerge.py      2010-01-05 15:44:09 +0000
> +++ lib/lp/registry/browser/peoplemerge.py      2010-01-08 13:36:23 +0000
> @@ -359,8 +359,7 @@
>  
>  
>  class RequestPeopleMergeMultipleEmailsView:
> -    """A view for the page where the user asks a merge and the dupe account
> -    have more than one email address."""
> +    """Merge request view when dupe account has multiple email addresses."""
>  
>      def __init__(self, context, request):
>          self.context = context
> @@ -390,29 +389,38 @@
>          login = getUtility(ILaunchBag).login
>          logintokenset = getUtility(ILoginTokenSet)
>  
> -        emails = self.request.form.get("selected")
> -        if emails is not None:
> -            # We can have multiple email adressess selected, and in this case
> -            # emails will be a list. Otherwise it will be a string and we need
> -            # to make a list with that value to use in the for loop.
> -            if not isinstance(emails, list):
> -                emails = [emails]
> -
> -            for email in emails:
> -                emailaddress = emailaddrset.getByEmail(email)
> -                assert emailaddress in self.dupeemails
> -                token = logintokenset.new(
> -                    user, login, email, LoginTokenType.ACCOUNTMERGE)
> -                token.sendMergeRequestEmail()
> -                self.notified_addresses.append(email)
> -
> -    # XXX: salgado, 2008-07-02: We need to somehow disclose the dupe person's
> -    # email addresses so that the logged in user knows where to look for the
> -    # message with instructions to finish the merge. Since people can choose
> -    # to have their email addresses hidden, we need to remove the security
> -    # proxy here to ensure they can be shown in this page.
> +        email_addresses = []
> +        if self.email_hidden:
> +            # If the email addresses are hidden we must send a merge request
> +            # to each of them.  But first we've got to remove the security
> +            # proxy so we can get to them.
> +            from zope.security.proxy import removeSecurityProxy
> +            email_addresses = [removeSecurityProxy(email).email
> +                               for email in self.dupeemails]
> +        else:
> +            # Otherwise we send a merge request only to the ones the user
> +            # selected.
> +            emails = self.request.form.get("selected")
> +            if emails is not None:
> +                # We can have multiple email adressess selected, and in this

Typo: adressess

> +                # case emails will be a list. Otherwise it will be a string
> +                # and we need to make a list with that value to use in the for
> +                # loop.
> +                if not isinstance(emails, list):
> +                    emails = [emails]
> +
> +                for emailaddress in emails:
> +                    email = emailaddrset.getByEmail(emailaddress)
> +                    assert email in self.dupeemails
> +                    email_addresses.append(emailaddress)
> +
> +        for emailaddress in email_addresses:
> +            token = logintokenset.new(
> +                user, login, emailaddress, LoginTokenType.ACCOUNTMERGE)
> +            token.sendMergeRequestEmail()
> +            self.notified_addresses.append(emailaddress)
> +
>      @property
> -    def naked_dupeemails(self):
> -        """Non-security-proxied email addresses of the dupe person."""
> -        from zope.security.proxy import removeSecurityProxy
> -        return [removeSecurityProxy(email) for email in self.dupeemails]
> +    def email_hidden(self):
> +        """Does the duplicate account hide email addresses?"""
> +        return self.dupe.hide_email_addresses
> 

> === modified file 'lib/lp/registry/stories/person/merge-people.txt'
> --- lib/lp/registry/stories/person/merge-people.txt     2009-07-31 22:34:24 +0000
> +++ lib/lp/registry/stories/person/merge-people.txt     2010-01-08 13:36:23 +0000
> @@ -172,3 +172,56 @@
>      #>>> browser.open('http://launchpad.dev/people/+mergerequest-sent?dupe=55')
>      #>>> browser.url
>      #'http://launchpad.dev/~no-priv'
> +
> +If the duplicate account has multiple email addresses and has chosen
> +to hide them the process is slightly different.  We cannot display the
> +hidden addresses so instead we just inform the user to check all of
> +them (and hope they know which ones) and we send merge request
> +messages to them all.
> +
> +    >>> login('<email address hidden>')
> +    >>> dupe = factory.makePerson(name="duplicate-person", <email address hidden>",
> +    ...                           hide_email_addresses=True)
> +    >>> email = factory.makeEmail(<email address hidden>", person=dupe)
> +    >>> logout()
> +
> +Ensure there are no leftover emails.
> +
> +    >>> len(stub.test_emails)
> +    0
> +
> +Open the merge page and merge our duplicate account with hidden email
> +addresses.
> +
> +    >>> browser.open('http://launchpad.dev/people/+requestmerge')
> +    >>> browser.getControl(
> +    ...     'Duplicated Account').value = 'duplicate-person'
> +    >>> browser.getControl('Continue').click()
> +
> +    >>> maincontent = extract_text(find_main_content(browser.contents))
> +    >>> maincontent = ' '.join(maincontent.split())
> +    >>> 'has 2 registered e-mail addresses' in maincontent
> +    True

It'd be nicer if you had an ID for the <p> that holds the information
you care about and then just show the text of that element here. That'd
make the test a bit more readable and, more importantly, when it fails
we'll see what text is in that element, whereas in the above test we'll
only be told that it failed. A good (IMO) rule of thumb is to always
avoid the 'in' keyword in doctests -- it's nearly always possible to do
better than that. :)

> +
> +Since the addresses are hidden we cannot display them therefore there
> +are no controls to select.
> +
> +    >>> email_select_control = browser.getControl(name='selected')
> +    Traceback (most recent call last):
> +    ...
> +    LookupError: name 'selected'
> +
> +    >>> browser.getControl('Submit').click()
> +
> +    >>> len(stub.test_emails)
> +    2
> +
> +    >>> maincontent = extract_text(find_main_content(browser.contents))
> +    >>> maincontent = ' '.join(maincontent.split())
> +
> +    >>> 'messages were sent to the 2 registered e-mail' in maincontent
> +    True


Same here.

> +    >>> '<email address hidden>' in maincontent
> +    False
> +    >>> '<email address hidden>' in maincontent
> +    False

Here I think it's better to leave it as is.

> 
> === modified file 'lib/lp/registry/templates/people-requestmerge-multiple.pt'
> --- lib/lp/registry/templates/people-requestmerge-multiple.pt   2009-08-31 19:53:54 +0000
> +++ lib/lp/registry/templates/people-requestmerge-multiple.pt   2010-01-08 13:36:23 +0000
> @@ -19,6 +19,7 @@
>          <tal:block tal:condition="not: view/form_processed">
>            <h1>Merge accounts</h1>
>  
> +      <tal:email_visible condition="not: view/email_hidden">
>        <p>
>          The account <code tal:content="view/dupe/name">foo</code>,
>          which you&#8217;re trying to merge,
> @@ -34,7 +35,7 @@
>        <form name="mergerequest" action="" method="POST">
>        <input type="hidden" name="dupe" tal:attributes="value request/dupe" />
>        <table>
> -        <tr tal:repeat="email view/naked_dupeemails">
> +        <tr tal:repeat="email view/dupeemails">
>            <td>
>              <input type="checkbox" name="selected" checked="checked"
>                     tal:attributes="value email/email" />
> @@ -49,23 +50,60 @@
>          </tr>
>        </table>
>        </form>
> +      </tal:email_visible>
> +
> +      <tal:email_hidden condition="view/email_hidden">
> +      <p>
> +        The account <code tal:content="view/dupe/name">foo</code>,
> +        which you&#8217;re trying to merge,
> +        has <span tal:replace="view/dupeemails/count" /> registered
> +        e-mail addresses but they are not publicly visible.
> +        To complete the merge, you need to prove that you have access to
> +        all e-mail addresses registered for this account.
> +      </p>
> +      <p>
> +        To do so, click <samp>Submit</samp>.  An email will be sent to each
> +        address on file for the duplicate account.  You must follow the
> +        instructions for each email.

Did you run this past a UI reviewer?  I'm sure they'll have something to
say about the 'click Submit' part. ;)

> +        If you don&#8217;t have access to one or more of these addresses,
> +        Launchpad will not be able to merge the account into your existing one,
> +        but the addresses you verify will be transferred to your account.

Would it be worth mentioning that they can ask a question when they
don't have access to all email addresses anymore?

> +      </p>
> +
> +      <form name="mergerequest" action="" method="POST">
> +        <input type="hidden" name="dupe" tal:attributes="value request/dupe" />
> +        <input type="submit" value="Submit" />
> +      </form>
> +      </tal:email_hidden>
> +
>      </tal:block>
>  
>      <tal:block tal:condition="view/form_processed">
>        <h1>Confirmation mail sent</h1>
>  
> -      <p>Individual email messages were sent to
> +      <tal:email_visible condition="not: view/email_hidden">
> +      <p>Individual email messages were sent to:</p>
>          <tal:block repeat="email view/notified_addresses">
> -          <strong tal:content="email"
> -            /><span tal:condition="not: repeat/email/end">, </span>
> -        </tal:block>.
> +          <p><strong tal:content="email"/></p>
> +        </tal:block>
> +      </tal:email_visible>
> +
> +      <tal:email_hidden condition="view/email_hidden">
> +      <p>Individual email messages were sent to the
> +        <span tal:content="view/dupeemails/count" /> registered
> +        e-mail addresses for
> +        <code tal:content="view/dupe/name">foo</code>.
> +      </p>
> +      </tal:email_hidden>
> +
> +      <p>
>          Please follow the instructions on each of those messages to complete
>          the merge.
>        </p>
>      </tal:block>
>  
>    </tal:duplicate>
> -</div> 
> +</div>
>  
>  </body>
>  </html>
> 
> 
> -- 
> launchpad-reviews mailing list
> <email address hidden>
> https://lists.ubuntu.com/mailman/listinfo/launchpad-reviews
> 
> 

-- 
Guilherme Salgado <email address hidden>