-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi Robert, thank you for your branch. You seem to have a lot of "circular fail" instances in here. It looks like a lot of them are owed to the same pattern, right? I only have a fiew minor suggestions that I ask you to consider. review approve Cheers, Henning Am 20.06.2011 09:11, schrieb Robert Collins: > === modified file 'lib/lp/bugs/browser/bugtarget.py' > === modified file 'lib/lp/bugs/browser/bugtask.py' > === modified file 'lib/lp/bugs/interfaces/bugsummary.py' > --- lib/lp/bugs/interfaces/bugsummary.py 2011-06-14 15:06:36 +0000 > +++ lib/lp/bugs/interfaces/bugsummary.py 2011-06-20 07:11:33 +0000 > @@ -4,7 +4,10 @@ > """BugSummary interfaces.""" > > __metaclass__ = type > -__all__ = ['IBugSummary'] > +__all__ = [ > + 'IBugSummary', > + 'IBugSummaryDimension', > + ] > > > from zope.interface import Interface > @@ -70,3 +73,16 @@ > > has_patch = Bool(readonly=True) > fixed_upstream = Bool(readonly=True) > + > + > +class IBugSummaryDimension(Interface): > + """Interface for dimensions used in the BugSummary database class.""" > + > + def _getBugSummaryContextWhereClause(): I don't think "private" methods belong in an interface. I suggest simply making it public. > + """Return a storm clause to filter bugsummaries on this context. > + > + This method is intentended for in-appserver use only. > + > + :return: Either a storm clause to filter bugsummaries, or False if > + there cannot be any matching bug summaries. > + """ > > === modified file 'lib/lp/bugs/interfaces/bugtarget.py' > === modified file 'lib/lp/bugs/interfaces/bugtask.py' > === modified file 'lib/lp/bugs/model/bug.py' > === modified file 'lib/lp/bugs/model/bugsummary.py' > --- lib/lp/bugs/model/bugsummary.py 2011-06-14 15:06:36 +0000 > +++ lib/lp/bugs/model/bugsummary.py 2011-06-20 07:11:33 +0000 > @@ -4,9 +4,13 @@ > """BugSummary Storm database classes.""" > > __metaclass__ = type > -__all__ = ['BugSummary'] > +__all__ = [ > + 'BugSummary' > + 'CombineBugSummaryContraint', > + ] > > from storm.locals import ( > + And, > Bool, > Int, > Reference, > @@ -14,6 +18,7 @@ > Unicode, > ) > from zope.interface import implements > +from zope.security.proxy import removeSecurityProxy I don't see were this is used. > > from canonical.database.enumcol import EnumCol > from lp.bugs.interfaces.bugsummary import IBugSummary > @@ -68,3 +73,21 @@ > > has_patch = Bool() > fixed_upstream = Bool() > + > + > +class CombineBugSummaryContraint(object): AFAIUI you can drop the "(object)" because of the __metaclass__ declaration at the top of the file. > + """A class to combine two separate bug summary constraints. > + > + This is useful for querying on multiple related dimensions (e.g. milestone > + + sourcepackage) - and essential when a dimension is not unique to a > + context. > + """ Missing "implements(IBugSummaryDimension)", is it not? > + > + def __init__(self, *dimensions): > + self.dimensions = map( > + lambda x:removeSecurityProxy(x._getBugSummaryContextWhereClause()), > + dimensions) > + > + def _getBugSummaryContextWhereClause(self): > + """See `IBugSummaryDimension`.""" > + return And(*self.dimensions) > > === modified file 'lib/lp/bugs/model/bugtarget.py' [...] > === modified file 'lib/lp/bugs/model/bugtask.py' > --- lib/lp/bugs/model/bugtask.py 2011-05-28 04:09:11 +0000 > +++ lib/lp/bugs/model/bugtask.py 2011-06-20 07:11:33 +0000 > @@ -45,6 +45,7 @@ > Or, > Select, > SQL, > + Sum, > ) > from storm.info import ClassAlias > from storm.store import ( > @@ -2525,13 +2526,56 @@ > """See `IBugTaskSet`.""" > return self._search(BugTask.bugID, [], None, params).result_set > > - def countBugs(self, params, group_on): > + def countBugs(self, user, contexts, group_on): > """See `IBugTaskSet`.""" > - resultset = self._search( > - group_on + (SQL("COUNT(Distinct BugTask.bug)"),), > - [], None, params).result_set > - # We group on the related field: > + # Circular fail. > + from lp.bugs.model.bugsummary import BugSummary > + assert user is not None Please add a message to the assert or remove it. > + conditions = [] > + # Open bug statuses > + conditions.append(BugSummary.status.is_in(UNRESOLVED_BUGTASK_STATUSES)) > + # BugSummary does not include duplicates so no need to exclude. > + context_conditions = [] > + for context in contexts: > + condition = removeSecurityProxy( > + context._getBugSummaryContextWhereClause()) > + if condition is not False: > + context_conditions.append(condition) > + if not context_conditions: > + return {} > + conditions.append(Or(*context_conditions)) > + # bugsummary by design requires either grouping by tag or excluding > + # non-null tags. > + # This is an awkward way of saying > + # if BugSummary.tag not in group_on: > + # - see bug 799602 > + group_on_tag = False > + for column in group_on: > + if column is BugSummary.tag: > + group_on_tag = True > + if not group_on_tag: > + conditions.append(BugSummary.tag == None) > + else: > + conditions.append(BugSummary.tag != None) > + store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) IMasterStore() ? > + admin_team = getUtility(ILaunchpadCelebrities).admin > + if user is not None and not user.inTeam(admin_team): Can you please add a comment here explaining what this is doing? > + store = store.with_(SQL( > + "teams AS (" > + "SELECT team from TeamParticipation WHERE person=?)", > + (user.id,))) > + # Note that because admins can see every bug regardless of subscription > + # they will see rather inflated counts. Admins get to deal. > + if not user.inTeam(admin_team): > + conditions.append( > + Or( > + BugSummary.viewed_by_id == None, > + BugSummary.viewed_by_id.is_in(SQL("SELECT team FROM teams")) > + )) > + sum_count = Sum(BugSummary.count) > + resultset = store.find(group_on + (sum_count,), *conditions) > resultset.group_by(*group_on) > + resultset.having(sum_count != 0) These could be chained. I think that would read better. > resultset.order_by() What's with the empty order_by clause? Does it have a side effect that is not obvious? > result = {} > for row in resultset: > > === removed file 'lib/lp/bugs/tests/bugtarget-bugcount.txt' Removing code is good! ;-) I assume it is not hurting our test coverage? > === modified file 'lib/lp/bugs/tests/test_bugtarget.py' > === modified file 'lib/lp/bugs/tests/test_bugtask_search.py' > === modified file 'lib/lp/registry/configure.zcml' > === modified file 'lib/lp/registry/model/distribution.py' > --- lib/lp/registry/model/distribution.py 2011-06-08 23:31:41 +0000 > +++ lib/lp/registry/model/distribution.py 2011-06-20 07:11:33 +0000 > @@ -112,7 +112,6 @@ > from lp.bugs.model.bug import ( > BugSet, > get_bug_tags, > - get_bug_tags_open_count, > ) > from lp.bugs.model.bugtarget import ( > BugTargetBase, > @@ -637,9 +636,13 @@ > """See `IBugTarget`.""" > return self.name > > - def _getBugTaskContextWhereClause(self): > + def _getBugSummaryContextWhereClause(self): Missing "implements" statement again? > """See BugTargetBase.""" > - return "BugTask.distribution = %d" % self.id > + # Circular fail. > + from lp.bugs.model.bugsummary import BugSummary > + return And( > + BugSummary.distribution_id == self.id, > + BugSummary.sourcepackagename_id == None) > > def _customizeSearchParams(self, search_params): > """Customize `search_params` for this distribution.""" > @@ -649,15 +652,6 @@ > """See `IBugTarget`.""" > return get_bug_tags("BugTask.distribution = %s" % sqlvalues(self)) > > - def getUsedBugTagsWithOpenCounts(self, user, tag_limit=0, include_tags=None): > - """See IBugTarget.""" > - # Circular fail. > - from lp.bugs.model.bugsummary import BugSummary > - return get_bug_tags_open_count( > - And(BugSummary.distribution_id == self.id, > - BugSummary.sourcepackagename_id == None), > - user, tag_limit=tag_limit, include_tags=include_tags) > - > def getMirrorByName(self, name): > """See `IDistribution`.""" > return Store.of(self).find( > @@ -720,10 +714,6 @@ > bug_params.setBugTarget(distribution=self) > return BugSet().createBug(bug_params) > > - def _getBugTaskContextClause(self): > - """See BugTargetBase.""" > - return 'BugTask.distribution = %s' % sqlvalues(self) > - > @property > def currentseries(self): > """See `IDistribution`.""" > The same comment about "implements" also applies to the model classes in these files: > === modified file 'lib/lp/registry/model/distributionsourcepackage.py' > === modified file 'lib/lp/registry/model/distroseries.py' > === modified file 'lib/lp/registry/model/milestone.py' > === modified file 'lib/lp/registry/model/product.py' > === modified file 'lib/lp/registry/model/productseries.py' > === modified file 'lib/lp/registry/model/projectgroup.py' > === modified file 'lib/lp/registry/model/sourcepackage.py' -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk3/cKEACgkQBT3oW1L17iiNRwCgtJhpCTcGw9dCay9CHpt7Oj// 7NoAnjBt8o1Srv+5KIXDNOruLNpC+S56 =1D8D -----END PGP SIGNATURE-----