Code review comment for lp:~thumper/launchpad/branch-subscription-subscribed-by

On Fri, 28 May 2010 13:58:22 you wrote:
> Review: Needs Information release-critical

Thanks Curtis,

> > === modified file 'lib/lp/code/browser/branch.py'
> > --- lib/lp/code/browser/branch.py 2010-05-20 04:01:34 +0000
> > +++ lib/lp/code/browser/branch.py 2010-05-27 04:47:25 +0000
> > @@ -354,6 +354,7 @@
> >
> > def initialize(self):
> > self.notices = []
> >
> > + # TODO: FIXME - this is almost certainly not what we want.
> >
> > self._add_subscription_notice()
>
> What do you want to do here?

Haha, that was my own personal reminder in that I *think* that the following
function call isn't needed, nor called, nor would do what is really wanted,
and it was a reminder to check.

> > === modified file 'lib/lp/code/model/branchsubscription.py'
> > --- lib/lp/code/model/branchsubscription.py 2009-06-25 04:06:00 +0000
> > +++ lib/lp/code/model/branchsubscription.py 2010-05-27 04:47:25 +0000
> > @@ -41,6 +41,9 @@
> >
> > notNull=False, default=DEFAULT)
> >
> > review_level = EnumCol(enum=CodeReviewNotificationLevel,
> >
> > notNull=True, default=DEFAULT)
> >
> > + subscribed_by = ForeignKey(
> > + dbName='subscribed_by', foreignKey='Person',
> > + storm_validator=validate_person_not_private_membership,
> > notNull=True)
>
> I was suggested in a recent review to drop the dbName since it is identical
> to the column name. I did so, but now I ponder explicit is better than
> implicit again.

I'm relatively unconcerned about this point. I think at one stage the dbName
was required for ForeignKey but now may not be needed. I'm happy to remove
the dbName param if you think it best.

> > === modified file 'lib/lp/registry/browser/tests/packaging-views.txt'
> > --- lib/lp/registry/browser/tests/packaging-views.txt 2010-04-16 18:00:31
> > +0000 +++ lib/lp/registry/browser/tests/packaging-views.txt 2010-05-27
> > 04:47:25 +0000 @@ -350,5 +350,5 @@
> >
> > cnews
> > libstdc++
> > linux-source-2.6.15
> >
> > + hot
> >
> > thunderbird
> >
> > - hot
>
> What caused this? This look like me from last week?

Caused by someone.

> > === modified file 'lib/lp/registry/model/distroseries.py'
> > --- lib/lp/registry/model/distroseries.py 2010-05-12 23:23:19 +0000
> > +++ lib/lp/registry/model/distroseries.py 2010-05-27 04:47:25 +0000
> > @@ -332,7 +332,7 @@
> >
> > origin = SQL(joins)
> > condition = SQL(conditions + "AND packaging.id IS NULL")
> > results = IStore(self).using(origin).find(find_spec, condition)
> >
> > - results = results.order_by('score DESC')
> > + results = results.order_by('score DESC', SourcePackageName.name)
> >
> > return [{
> >
> > 'package': SourcePackage(
> >
> > sourcepackagename=spn, distroseries=self),
>
> This is me from last week.

Yes this was the source of it. It was failing locally for me consistently.

I'm back on to this branch now to add the extra tests and model validation.