Merge into db-devel : bug-793848 : Code : Launchpad itself

Status:

Merged

Approved by:

Robert Collins on 2011-06-21

Approved revision:

no longer in the source branch.

Merged at revision:

10699

Proposed branch:

lp://staging/~lifeless/launchpad/bug-793848

Merge into:

lp://staging/launchpad/db-devel

Diff against target:

1302 lines (+256/-387)

22 files modified

lib/lp/bugs/browser/bugtarget.py (+22/-12)
lib/lp/bugs/browser/bugtask.py (+5/-10)
lib/lp/bugs/interfaces/bugsummary.py (+17/-1)
lib/lp/bugs/interfaces/bugtarget.py (+0/-9)
lib/lp/bugs/interfaces/bugtask.py (+10/-5)
lib/lp/bugs/model/bug.py (+3/-2)
lib/lp/bugs/model/bugsummary.py (+30/-2)
lib/lp/bugs/model/bugtarget.py (+16/-35)
lib/lp/bugs/model/bugtask.py (+53/-5)
lib/lp/bugs/stories/bugs/xx-portlets-bug-milestones.txt (+12/-34)
lib/lp/bugs/tests/bugtarget-bugcount.txt (+0/-137)
lib/lp/bugs/tests/test_bugtarget.py (+1/-11)
lib/lp/bugs/tests/test_bugtask_search.py (+12/-9)
lib/lp/registry/configure.zcml (+8/-1)
lib/lp/registry/model/distribution.py (+10/-20)
lib/lp/registry/model/distributionsourcepackage.py (+9/-21)
lib/lp/registry/model/distroseries.py (+10/-17)
lib/lp/registry/model/milestone.py (+8/-1)
lib/lp/registry/model/product.py (+8/-15)
lib/lp/registry/model/productseries.py (+6/-12)
lib/lp/registry/model/projectgroup.py (+8/-13)
lib/lp/registry/model/sourcepackage.py (+8/-15)

To merge this branch:

bzr merge lp://staging/~lifeless/launchpad/bug-793848

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Henning Eggers (community)		2011-06-20	Approve on 2011-06-20
Review via email: mp+65158@code.staging.launchpad.net

Commit message

[r=henninge][bug=793848] Make use of the new bugsummary to handle the bug stats portlet.

Description of the change

This branch gets the table scan component of the bug stats portlet using bugsummary.

It needs the new columns only available in db-devel; along the way I saw dead code that confused me, so I deleted it.

I've added a interface for things that are dimensions in the bugsummary, this lets me avoid code duplication between the tags counting code and countBugs. In fact, in a future branch we probably want to delete the special cased tag counting code.

Oh, and I removed code duplication in the tags counting code, making the only variation the joining condition logic. The previous duplicate code I pulled up to an appropriate common base class.

Revision history for this message

Henning Eggers (henninge) wrote on 2011-06-20:

#

Download full text (9.9 KiB)

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

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

review: Approve

Revision history for this message

Robert Collins (lifeless) wrote on 2011-06-20:

#

Thanks for the review Henninge.
All your suggestions are useful and I'll act on them before landing this branch. The one about chaining however I won't - we end up with 3 lines with poor indenting (its too long).

The implements thing is interesting; it turns out that the security rules are not cross checking the implements() - the interfaces primary job is then to abstract out the method signature. I'll add them though we don't strictly need them - and I bet we're missing other implements within the code base.

The admin team checking is doing visibility checks - same as private bugs. The order_by thing nukes the default order_by - storm sets a query to find_spec.default_order, and we really want no order by to ensure we get the best query plan. (yes, its weird that the default is possibly ordered) - I'm just using belts and bracers to avoid surprises.

Revision history for this message

Robert Collins (lifeless) wrote on 2011-06-20:

#

>> + store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)

>IMasterStore() ?

No - we should use the default store not force it to the master; IStore() can be used in this case.

Launchpad itself

Merge lp://staging/~lifeless/launchpad/bug-793848 into lp://staging/launchpad/db-devel

Commit message

Description of the change

Preview Diff

Subscribers