Code review comment for lp:~intellectronica/launchpad/no-patches-message

Michael Hudson <email address hidden> writes:
>Ah, so a prerequisite branch should have been set when proposing the
>branch for merging...

D'oh. Got it now, thanks.

>> Regarding "XXX: Karl Fogel 2010-02-01 bug=515584", I have not done it yet, because we're under time pressure to get this branch into staging in time for Jorge to use it in a demo. Hence the XXX.
>
>Well, OK. I think it would take much much less time than the 2 test
>suite runs it takes to get stuff onto staging, but...

For you, or for a more experienced me, yes. But I'm fully loaded right
now getting the other changes ready in time, and the people I would ask
for help on this are too -- hence deferring the form fix.

>> Regarding your comment about the new factory.doAsUser() method: "Do you really need to commit here? Zope-level users and database transactions are basically orthogonal." I asked Abel, who said: "yes, I think we must commit here. A story test does not access the database directly, but via a web server, which has its own database connection. When we manipulate the database via factroy.makeSomething(), the connetcion of the web server does not 'see' any change, if we don't commit(). Also, I think we get an error message if we don't commit when the web server accesses the DB after a factory.mkeWhatever() call."
>
>As I think we just established on #launchpad-dev, I think Abel is wrong
>in the particulars here, but I guess I don't care all that much.
>
> vote approve
> merge approved

Thanks.

Does that mean if I took out the transaction.commit, everything would
still work? (Would you recommend I do that?)