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

On 28 January 2010 19:51, Abel Deuring <email address hidden> wrote:
> 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...)

+1000000