Code review comment for lp:~rockstar/launchpad/recipe-security

On Wed, Jun 30, 2010 at 04:35:40PM -0000, Paul Hummer wrote:
> We've always used transaction.commit instead of Store.flush.
> transaction.commit apparently has some hooks in it.
>
> I'd be curious to know where you found tests pass that don't need
> transaction.commit. I'm pretty sure it's only in places where I needed
> to get the tests to pass.

I'd like to know more, why you need a transaction.commit() in browser
code. In tests it's ok, but it should be avoided in real code, since
it breaks the 'one transaction per request' model that is used
everywhere else. By doing commits you have the risk of errors happening
after the commit causing the DB to have an inconsistent state.

If the commit would be necessary, it should definitely have a big
comment, explaining why. But it seems odd that we need it.

    vote needsinformation

--
Björn Tillenius | https://launchpad.net/~bjornt