Launchpad itself

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

no-patches-message
Merge into devel

Revision history for this message

Karl Fogel (kfogel) wrote on 2010-02-02:

Michael, thanks. It turns out all your comments were on changes from my https://code.edge.launchpad.net/~kfogel/launchpad/506018-patch-report branch, which Tom merged into his branch to have the right context in which to make his changes. Abel had already reviewed my branch once, and I'd updated it in response, so you effectively did the second review.

Anyway: Tom's changes are fine, presumably :-).

As for mine, I've addressed most of your concerns. See these revisions:

* http://bazaar.launchpad.net/~kfogel/launchpad/506018-patch-report/revision/10190
* http://bazaar.launchpad.net/~kfogel/launchpad/506018-patch-report/revision/10191
* http://bazaar.launchpad.net/~kfogel/launchpad/506018-patch-report/revision/10192
* http://bazaar.launchpad.net/~kfogel/launchpad/506018-patch-report/revision/10193
* http://bazaar.launchpad.net/~kfogel/launchpad/506018-patch-report/revision/10194

Regarding things not addressed in the above commits:

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.

The copyright year changes follow up to other changes already landed.

We still don't have at est for patch view on a project group; I have left an "XXX" about that due to the time constraints. However, I have fixed it so the column name is "Project" (meaning Product) not "Package", when doing patches view on a project group -- thanks for the excellent suggestion.

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."

Michael, thanks.  It turns out all your comments were on changes from my https://code.edge.launchpad.net/~kfogel/launchpad/506018-patch-report branch, which Tom merged into his branch to have the right context in which to make his changes.  Abel had already reviewed my branch once, and I'd updated it in response, so you effectively did the second review.

Anyway: Tom's changes are fine, presumably :-).

As for mine, I've addressed most of your concerns.  See these revisions:

* http://bazaar.launchpad.net/~kfogel/launchpad/506018-patch-report/revision/10190
 * http://bazaar.launchpad.net/~kfogel/launchpad/506018-patch-report/revision/10191
 * http://bazaar.launchpad.net/~kfogel/launchpad/506018-patch-report/revision/10192
 * http://bazaar.launchpad.net/~kfogel/launchpad/506018-patch-report/revision/10193
 * http://bazaar.launchpad.net/~kfogel/launchpad/506018-patch-report/revision/10194
  
Regarding things not addressed in the above commits:

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.

The copyright year changes follow up to other changes already landed.

We still don't have at est for patch view on a project group; I have left an "XXX" about that due to the time constraints.  However, I have fixed it so the column name is "Project" (meaning Product) not "Package", when doing patches view on a project group -- thanks for the excellent suggestion.

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."

« Back to merge proposal