Code review comment for lp:~bac/launchpad/bug-432026-person-edit

On Sep 17, 2009, at 15:53 , Curtis Hovey wrote:

> Review: Approve code
> Thanks for doing this Brad.
>
> This branch looks fine to land. I have a few suggestions, but don't
> let them stop you from landing the code today.
>
>> === renamed file 'lib/lp/registry/stories/foaf/xx-addemail.txt' =>
>> 'lib/lp/registry/stories/foaf/xx-add-email.txt'
>
> I like the rename.
>
>> --- lib/lp/registry/stories/foaf/xx-addemail.txt 2009-05-12
>> 01:39:29 +0000
>> +++ lib/lp/registry/stories/foaf/xx-add-email.txt 2009-09-17
>> 16:18:19 +0000
>> @@ -7,15 +7,13 @@
>>
>> Sample Person will now add a couple email addresses to his account.
>>
>> - # Workaround while https://launchpad.net/launchpad/+bug/39016
>> is not
>> - # fixed.
>>>>> from lp.services.mail import stub
>> - >>> stub.test_emails[:] = []
>> + >>> assert not stub.test_emails, (
>> + ... "stub.test_emails should be empty at the start of the
>> test.")
>
> You can remove the assert. The problem was fixed a long time ago and
> lots
> of tests use the stub without clearing it.

Great. We're not so good about cleaning up old XXX work-arounds and
comments.

>
>>>>> browser = setupBrowser(auth='Basic <email address hidden>:test')
>>>>> browser.open('http://launchpad.dev/~name12')
>> - >>> browser.getLink('Change details').click()
>> - >>> browser.getLink('E-mail Settings').click()
>> + >>> browser.getLink(url='+editemails').click()
>
> This is fine as it is, but I usually use the alt text of an image to
> get the
> link.

I'll leave this test as is but keep that in mind.

>
>>>> browser.getLink('Change e-mail settings').click()
>
> ^ Did that not work?

Didn't try it. I'll have a go next time around.