Code review comment for lp:~kfogel/launchpad/506018-patch-report

On 28.01.2010 19:23, Tom Berger wrote:
>> === added directory 'lib/lp/bugs/stories/patches-view'
>> === added file 'lib/lp/bugs/stories/patches-view/patches-view.txt'
>> --- lib/lp/bugs/stories/patches-view/patches-view.txt 1970-01-01 00:00:00 +0000
>> +++ lib/lp/bugs/stories/patches-view/patches-view.txt 2010-01-28 17:24:20 +0000
>> @@ -0,0 +1,301 @@
>> +Patches View
>> +============
>> +
>> +Patches View by Product
>> +-----------------------
>> +
>> +We have a view listing patches attached to bugs that target a given
>> +product. At first, the product is new and has no bugs.
>> +
>> + >>> def make_thing(factory_method, **thing_args):
>> + ... login('<email address hidden>')
>> + ... result = factory_method(**thing_args)
>> + ... transaction.commit()
>> + ... logout()
>> + ... return result
>
> That's a nice helper function. Let's extract it out of this test and make it
> generally available to all tests. It probably deserves a more descriptive name,
> though.

A generally available function should have a way to specify the user
that should login: Some objects can only be created by script users.

And this is actually a good candidate for using the "with" statement --
provided that Jeroen does not object to using it for "transaction
wrapping". (see the notes for last reviewers meeting. Has Barry posted
them already?)

Something like

    with factory.database_login('<email address hidden>'):
 factory.makePerson(k1=v1, k2=v2)

would look much nicer than

    make_thing(factory.makePerson, k1=v1, k2=v2...)

Abel