Launchpad itself

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

no-patches-message
Merge into devel

Revision history for this message

Michael Hudson-Doyle (mwhudson) wrote on 2010-02-02:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Karl Fogel wrote:
> 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.

Ah, so a prerequisite branch should have been set when proposing the
branch for merging...

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

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

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

Thanks!

> 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

Cheers,
mwh
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAktohtcACgkQeTTOPm7A7kg4WACgllYxd55yDVeYZMqUSonaJw/0
3+0AoIR/1xBaEnJlOilQ1nLIC1ISjwND
=7jeF
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Karl Fogel wrote:
> 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.

Ah, so a prerequisite branch should have been set when proposing the
branch for merging...

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

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

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

Thanks!

> 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

Cheers,
mwh
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAktohtcACgkQeTTOPm7A7kg4WACgllYxd55yDVeYZMqUSonaJw/0
3+0AoIR/1xBaEnJlOilQ1nLIC1ISjwND
=7jeF
-----END PGP SIGNATURE-----

review: Approve

« Back to merge proposal