Launchpad itself

Code review comment for lp:~bac/launchpad/bug-419930

bug-419930
Merge into devel

Revision history for this message

Brad Crittenden (bac) wrote on 2010-01-08:

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

Thanks for the review.

> > 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?

Currently we cannot use the picker to select people with hidden email addresses. But you can merge them by typing in the ID into the field on the form.

> > === 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?

I have discovered Edwin is working to redo the way these pickers are working and my drive-by change would soon be replaced by his work. Therefore I'm going to back out this portion of the change and just wait for his more thorough fix.

> > === modified file 'lib/lp/registry/browser/peoplemerge.py'

> > + # We can have multiple email adressess selected, and in
> this
>
> Typo: adressess

This typo was pre-existing. I looked to see if it existed elsewhere and found and fixed six other occurrences.

> > === 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. :)

Done

>
> > +
> > +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.

Done.

> > === 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’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’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. ;)

Have not requested a UI review yet. Will do so now.

>
> > + If you don’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?
>

Perhaps. I'll bring that up in the UI review.

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

Thanks for the review.

> > 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?

Currently we cannot use the picker to select people with hidden email addresses.  But you can merge them by typing in the ID into the field on the form.

> > === 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?

I have discovered Edwin is working to redo the way these pickers are working and my drive-by change would soon be replaced by his work.  Therefore I'm going to back out this portion of the change and just wait for his more thorough fix.

> > === modified file 'lib/lp/registry/browser/peoplemerge.py'

> > +                # We can have multiple email adressess selected, and in
> this
> 
> Typo: adressess

This typo was pre-existing. I looked to see if it existed elsewhere and found and fixed six other occurrences.
 
> > === 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('foo.bar@canonical.com')
> > +    >>> dupe = factory.makePerson(name="duplicate-person",
> email="dupe1@example.com",
> > +    ...                           hide_email_addresses=True)
> > +    >>> email = factory.makeEmail(address="dupe2@example.com", 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. :)

Done

> 
> > +
> > +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.

Done.

> > === 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. ;)

Have not requested a UI review yet.  Will do so now.

> 
> > +        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?
>

Perhaps.  I'll bring that up in the UI review.

« Back to merge proposal