Merge lp:~wgrant/launchpad/bugsummary-v2-apg-app into lp:launchpad

[William Grant]

Teach model and test code about BugSummary.access_policy.

BugSummary privacy is a little special. Public bugs show up in a single row with viewed_by=None, private bugs in several rows with viewed_by=subscriber, one for each subscriber. This obviously isn't quite compatible with the new sharing model, which also grants visibility through access policies, not requiring a direct subscription. So BugSummary has grown a new column, access_policy. For public bugs there'll still be just a single row with viewed_by=None,access_policy=None, but private bugs will expand to a one row for each subscriber, plus one for each access policy.

This branch teaches the application code about the new column, removing the last blocker for lp:~wgrant/launchpad/bugsummary-v2-apg-db, which alters the triggers to start setting it. The two production queries and one test query that filter on viewed_by have been adjusted to use a factored out filter method, which knows about access policies. I've verified that they still all perform excellently.

Tests are a bit of an issue, as it can't really be tested until the DB changes are deployed as well. But the query is factored out and well tested.

I also reworked bugsummary.txt a bit. The new column makes the printed tables a bit wide, so I made privacy display optional.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== modified file 'lib/lp/bugs/doc/bugsummary.txt'
--- lib/lp/bugs/doc/bugsummary.txt	2012-07-11 22:31:52 +0000
+++ lib/lp/bugs/doc/bugsummary.txt	2012-07-25 08:05:24 +0000
@@ -19,9 +19,9 @@
 First we should setup some helpers to use in the examples. These will
 let us dump the BugSummary table in a readable format.
 
-    ---------------------------------------------------------------
-    prod ps   dist ds   spn   tag mile status import pa vis    #
-    ---------------------------------------------------------------
+    ----------------------------------------------------------
+    prod ps   dist ds   spn   tag mile status import pa   #
+    ----------------------------------------------------------
 
 The columns are product, productseries, distribution, distroseries,
 sourcepackagename, tag, milestone, status, importance, has_patch,
@@ -40,7 +40,18 @@
     ...         return 'x'
     ...     return object_or_none.name
 
-    >>> def print_result(bugsummary_resultset):
+    >>> def ap_desc(policy_or_none):
+    ...    if policy_or_none is None:
+    ...        return 'x'
+    ...    type_names = {
+    ...        InformationType.PRIVATESECURITY: 'se',
+    ...        InformationType.USERDATA: 'pr',
+    ...        InformationType.PROPRIETARY: 'pp',
+    ...        }
+    ...    return '%-4s/%-2s' % (
+    ...        policy_or_none.pillar.name, type_names[policy_or_none.type])
+
+    >>> def print_result(bugsummary_resultset, include_privacy=False):
     ...     # First, flush and invalidate the cache so we see the effects
     ...     # of the underlying database triggers. Normally you don't want
     ...     # to bother with this as you are only interested in counts of
@@ -59,18 +70,28 @@
     ...         BugSummary.sourcepackagename_id, BugSummary.tag,
     ...         BugSummary.milestone_id, BugSummary.status,
     ...         BugSummary.importance, BugSummary.has_patch,
-    ...         BugSummary.viewed_by_id, BugSummary.id)
+    ...         BugSummary.viewed_by_id, BugSummary.access_policy_id,
+    ...         BugSummary.id)
     ...     fmt = (
     ...         "%-4s %-4s %-4s %-4s %-5s %-3s %-4s "
-    ...         "%-6s %-6s %-2s %-4s %3s")
-    ...     header = fmt % (
+    ...         "%-6s %-6s %-2s")
+    ...     titles = (
     ...         'prod', 'ps', 'dist', 'ds', 'spn', 'tag', 'mile',
-    ...         'status', 'import', 'pa', 'vis', '#')
+    ...         'status', 'import', 'pa')
+    ...     if include_privacy:
+    ...         fmt += ' %-4s %-7s'
+    ...         titles += ('gra', 'pol')
+    ...     fmt += ' %3s'
+    ...     titles += ('#',)
+    ...     header = fmt % titles
     ...     print "-" * len(header)
     ...     print header
     ...     print "-" * len(header)
     ...     for bugsummary in ordered_results:
-    ...         print fmt % (
+    ...         if not include_privacy:
+    ...             assert bugsummary.viewed_by is None
+    ...             assert bugsummary.access_policy is None
+    ...         data = (
     ...             name(bugsummary.product),
     ...             name(bugsummary.productseries),
     ...             name(bugsummary.distribution),
@@ -80,9 +101,13 @@
     ...             name(bugsummary.milestone),
     ...             str(bugsummary.status)[:6],
     ...             str(bugsummary.importance)[:6],
-    ...             str(bugsummary.has_patch)[:1],
-    ...             name(bugsummary.viewed_by),
-    ...             bugsummary.count)
+    ...             str(bugsummary.has_patch)[:1])
+    ...         if include_privacy:
+    ...             data += (
+    ...                 name(bugsummary.viewed_by),
+    ...                 ap_desc(bugsummary.access_policy),
+    ...                 )
+    ...         print fmt % (data + (bugsummary.count,))
     ...     print " " * (len(header) - 4),
     ...     print "==="
     ...     sum = bugsummary_resultset.sum(BugSummary.count)
@@ -90,8 +115,9 @@
     ...     print "%3s" % sum
 
     >>> def print_find(*bs_query_args, **bs_query_kw):
+    ...     include_privacy = bs_query_kw.pop('include_privacy', False)
     ...     resultset = store.find(BugSummary, *bs_query_args, **bs_query_kw)
-    ...     print_result(resultset)
+    ...     print_result(resultset, include_privacy=include_privacy)
 
 
 /!\ A Note About Privacy in These Examples
@@ -119,12 +145,12 @@
     ...     BugSummary.tag == None)
 
     >>> print_result(bug_summaries)
-    ------------------------------------------------------------
-    prod ps   dist ds   spn   tag mile status import pa vis    #
-    ------------------------------------------------------------
-    pr-a x    x    x    x     x   x    New    Undeci F  x      1
-                                                             ===
-                                                               1
+    -------------------------------------------------------
+    prod ps   dist ds   spn   tag mile status import pa   #
+    -------------------------------------------------------
+    pr-a x    x    x    x     x   x    New    Undeci F    1
+                                                        ===
+                                                          1
 
 There is one row per tag per combination of product, status and milestone.
 If we are interested in all bugs targeted to a product regardless of how
@@ -145,13 +171,13 @@
     ...     BugSummary.product == prod_a,
     ...     BugSummary.tag == None,
     ...     BugSummary.viewed_by == None)
-    ------------------------------------------------------------
-    prod ps   dist ds   spn   tag mile status import pa vis    #
-    ------------------------------------------------------------
-    pr-a x    x    x    x     x   x    New    Undeci F  x      2
-    pr-a x    x    x    x     x   x    Confir Undeci F  x      2
-                                                             ===
-                                                               4
+    -------------------------------------------------------
+    prod ps   dist ds   spn   tag mile status import pa   #
+    -------------------------------------------------------
+    pr-a x    x    x    x     x   x    New    Undeci F    2
+    pr-a x    x    x    x     x   x    Confir Undeci F    2
+                                                        ===
+                                                          4
 
 Here are the rows associated with the 't-a' tag. There is 1 Confirmed
 bug task targetted to the pr-a product who's bug is tagged 't-a'.:
@@ -160,12 +186,12 @@
     ...     BugSummary.product == prod_a,
     ...     BugSummary.tag == u't-a',
     ...     BugSummary.viewed_by == None)
-    ------------------------------------------------------------
-    prod ps   dist ds   spn   tag mile status import pa vis    #
-    ------------------------------------------------------------
-    pr-a x    x    x    x     t-a x    Confir Undeci F  x      1
-                                                             ===
-                                                               1
+    -------------------------------------------------------
+    prod ps   dist ds   spn   tag mile status import pa   #
+    -------------------------------------------------------
+    pr-a x    x    x    x     t-a x    Confir Undeci F    1
+                                                        ===
+                                                          1
 
 You will normally want to get the total count counted in the database
 rather than waste transmission time to calculate the rows client side.
@@ -205,17 +231,17 @@
     >>> print_find(
     ...     BugSummary.product == prod_a,
     ...     BugSummary.viewed_by == None)
-    ------------------------------------------------------------
-    prod ps   dist ds   spn   tag mile status import pa vis    #
-    ------------------------------------------------------------
-    pr-a x    x    x    x     t-a x    Confir Undeci F  x      1
-    pr-a x    x    x    x     t-b ms-a New    Undeci F  x      1
-    pr-a x    x    x    x     t-c ms-a New    Undeci F  x      1
-    pr-a x    x    x    x     x   ms-a New    Undeci F  x      1
-    pr-a x    x    x    x     x   x    New    Undeci F  x      2
-    pr-a x    x    x    x     x   x    Confir Undeci F  x      2
-                                                             ===
-                                                               8
+    -------------------------------------------------------
+    prod ps   dist ds   spn   tag mile status import pa   #
+    -------------------------------------------------------
+    pr-a x    x    x    x     t-a x    Confir Undeci F    1
+    pr-a x    x    x    x     t-b ms-a New    Undeci F    1
+    pr-a x    x    x    x     t-c ms-a New    Undeci F    1
+    pr-a x    x    x    x     x   ms-a New    Undeci F    1
+    pr-a x    x    x    x     x   x    New    Undeci F    2
+    pr-a x    x    x    x     x   x    Confir Undeci F    2
+                                                        ===
+                                                          8
 
 Number of New bugs not targeted to a milestone. Note the difference
 between selecting records where tag is None, and where milestone is None:
@@ -269,13 +295,13 @@
     ...         BugSummary.productseries == productseries_b,
     ...         BugSummary.product == prod_b),
     ...     BugSummary.viewed_by == None)
-    ------------------------------------------------------------
-    prod ps   dist ds   spn   tag mile status import pa vis    #
-    ------------------------------------------------------------
-    pr-b x    x    x    x     x   x    New    Undeci F  x      1
-    x    ps-b x    x    x     x   x    New    Undeci F  x      1
-                                                             ===
-                                                               2
+    -------------------------------------------------------
+    prod ps   dist ds   spn   tag mile status import pa   #
+    -------------------------------------------------------
+    pr-b x    x    x    x     x   x    New    Undeci F    1
+    x    ps-b x    x    x     x   x    New    Undeci F    1
+                                                        ===
+                                                          2
 
 Distribution Bug Counts
 -----------------------
@@ -297,14 +323,14 @@
     >>> print_find(
     ...     BugSummary.distribution == distribution,
     ...     BugSummary.viewed_by == None)
-    ------------------------------------------------------------
-    prod ps   dist ds   spn   tag mile status import pa vis    #
-    ------------------------------------------------------------
-    x    x    di-a x    sp-a  x   x    New    Undeci F  x      1
-    x    x    di-a x    x     x   x    New    Undeci F  x      1
-    x    x    di-a x    x     x   x    Confir Undeci F  x      1
-                                                             ===
-                                                               3
+    -------------------------------------------------------
+    prod ps   dist ds   spn   tag mile status import pa   #
+    -------------------------------------------------------
+    x    x    di-a x    sp-a  x   x    New    Undeci F    1
+    x    x    di-a x    x     x   x    New    Undeci F    1
+    x    x    di-a x    x     x   x    Confir Undeci F    1
+                                                        ===
+                                                          3
 
 How many bugs targeted to a distribution?
 
@@ -369,12 +395,12 @@
     >>> print_find(
     ...     BugSummary.distroseries == series_c,
     ...     BugSummary.viewed_by == None)
-    ------------------------------------------------------------
-    prod ps   dist ds   spn   tag mile status import pa vis    #
-    ------------------------------------------------------------
-    x    x    x    ds-c x     x   x    New    Undeci F  x      1
-                                                             ===
-                                                               1
+    -------------------------------------------------------
+    prod ps   dist ds   spn   tag mile status import pa   #
+    -------------------------------------------------------
+    x    x    x    ds-c x     x   x    New    Undeci F    1
+                                                        ===
+                                                          1
 
 
 Privacy
@@ -440,19 +466,19 @@
     >>> distro_or_series = Or(
     ...     BugSummary.distribution == distro_p,
     ...     BugSummary.distroseries == series_p)
-    >>> print_find(distro_or_series)
-    ------------------------------------------------------------
-    prod ps   dist ds   spn   tag mile status import pa vis    #
-    ------------------------------------------------------------
-    x    x    di-p x    x     x   x    New    Undeci F  p-b    1
-    x    x    di-p x    x     x   x    New    Undeci F  own    3
-    x    x    di-p x    x     x   x    New    Undeci F  t-a    1
-    x    x    di-p x    x     x   x    New    Undeci F  t-c    1
-    x    x    di-p x    x     x   x    New    Undeci F  x      1
-    x    x    x    ds-p x     x   x    New    Undeci F  own    1
-    x    x    x    ds-p x     x   x    New    Undeci F  t-c    1
-                                                             ===
-                                                               9
+    >>> print_find(distro_or_series, include_privacy=True)
+    --------------------------------------------------------------------
+    prod ps   dist ds   spn   tag mile status import pa gra  pol       #
+    --------------------------------------------------------------------
+    x    x    di-p x    x     x   x    New    Undeci F  p-b  x         1
+    x    x    di-p x    x     x   x    New    Undeci F  own  x         3
+    x    x    di-p x    x     x   x    New    Undeci F  t-a  x         1
+    x    x    di-p x    x     x   x    New    Undeci F  t-c  x         1
+    x    x    di-p x    x     x   x    New    Undeci F  x    x         1
+    x    x    x    ds-p x     x   x    New    Undeci F  own  x         1
+    x    x    x    ds-p x     x   x    New    Undeci F  t-c  x         1
+                                                                     ===
+                                                                       9
 
 So how many public bugs are there on the distro?
 
@@ -460,12 +486,14 @@
     ...     BugSummary,
     ...     BugSummary.distribution == distro_p,
     ...     BugSummary.viewed_by == None, # Public bugs only
+    ...     BugSummary.access_policy == None, # Public bugs only
     ...     BugSummary.sourcepackagename == None,
     ...     BugSummary.tag == None).sum(BugSummary.count) or 0
     1
 
 But how many can the owner see?
 
+    >>> from storm.expr import And
     >>> join = LeftJoin(
     ...     BugSummary, TeamParticipation,
     ...     BugSummary.viewed_by_id == TeamParticipation.teamID)
@@ -473,7 +501,8 @@
     ...     BugSummary,
     ...     BugSummary.distribution == distro_p,
     ...     Or(
-    ...         BugSummary.viewed_by == None,
+    ...         And(BugSummary.viewed_by == None,
+    ...             BugSummary.access_policy == None),
     ...         TeamParticipation.person == owner),
     ...     BugSummary.sourcepackagename == None,
     ...     BugSummary.tag == None).sum(BugSummary.count) or 0

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-07-19 04:40:03 +0000
+++ lib/lp/bugs/model/bug.py	2012-07-25 08:05:24 +0000
@@ -227,12 +227,7 @@
     get_property_cache,
     )
 from lp.services.webapp.authorization import check_permission
-from lp.services.webapp.interfaces import (
-    DEFAULT_FLAVOR,
-    ILaunchBag,
-    IStoreSelector,
-    MAIN_STORE,
-    )
+from lp.services.webapp.interfaces import ILaunchBag
 
 
 _bug_tag_query_template = """
@@ -291,29 +286,26 @@
         (and {} returned).
     """
     # Circular fail.
-    from lp.bugs.model.bugsummary import BugSummary
+    from lp.bugs.model.bugsummary import (
+        BugSummary,
+        get_bugsummary_filter_for_user,
+        )
     tags = {}
     if include_tags:
         tags = dict((tag, 0) for tag in include_tags)
-    store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
-    admin_team = getUtility(ILaunchpadCelebrities).admin
-    if user is not None and not user.inTeam(admin_team):
-        store = store.with_(SQL(
-            "teams AS ("
-            "SELECT team from TeamParticipation WHERE person=?)", (user.id,)))
     where_conditions = [
         BugSummary.status.is_in(UNRESOLVED_BUGTASK_STATUSES),
         BugSummary.tag != None,
         context_condition,
         ]
-    if user is None:
-        where_conditions.append(BugSummary.viewed_by_id == None)
-    elif not user.inTeam(admin_team):
-        where_conditions.append(
-            Or(
-                BugSummary.viewed_by_id == None,
-                BugSummary.viewed_by_id.is_in(SQL("SELECT team FROM teams"))
-                ))
+
+    # Apply the privacy filter.
+    store = IStore(BugSummary)
+    user_with, user_where = get_bugsummary_filter_for_user(user)
+    if user_with:
+        store = store.with_(user_with)
+    where_conditions.extend(user_where)
+
     sum_count = Sum(BugSummary.count)
     tag_count_columns = (BugSummary.tag, sum_count)
 

=== modified file 'lib/lp/bugs/model/bugsummary.py'
--- lib/lp/bugs/model/bugsummary.py	2012-06-25 09:45:56 +0000
+++ lib/lp/bugs/model/bugsummary.py	2012-07-25 08:05:24 +0000
@@ -7,16 +7,23 @@
 __all__ = [
     'BugSummary',
     'CombineBugSummaryConstraint',
+    'get_bugsummary_filter_for_user',
     ]
 
-from storm.locals import (
+from storm.base import Storm
+from storm.expr import (
     And,
+    Or,
+    Select,
+    SQL,
+    With,
+    )
+from storm.properties import (
     Bool,
     Int,
-    Reference,
-    Storm,
     Unicode,
     )
+from storm.references import Reference
 from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
 
@@ -29,6 +36,11 @@
     BugTaskStatus,
     BugTaskStatusSearch,
     )
+from lp.registry.interfaces.role import IPersonRoles
+from lp.registry.model.accesspolicy import (
+    AccessPolicy,
+    AccessPolicyGrant,
+    )
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.distroseries import DistroSeries
 from lp.registry.model.milestone import Milestone
@@ -36,6 +48,7 @@
 from lp.registry.model.product import Product
 from lp.registry.model.productseries import ProductSeries
 from lp.registry.model.sourcepackagename import SourcePackageName
+from lp.registry.model.teammembership import TeamParticipation
 from lp.services.database.enumcol import EnumCol
 
 
@@ -76,6 +89,8 @@
 
     viewed_by_id = Int(name='viewed_by')
     viewed_by = Reference(viewed_by_id, Person.id)
+    access_policy_id = Int(name='access_policy')
+    access_policy = Reference(access_policy_id, AccessPolicy.id)
 
     has_patch = Bool()
 
@@ -99,3 +114,46 @@
     def getBugSummaryContextWhereClause(self):
         """See `IBugSummaryDimension`."""
         return And(*self.dimensions)
+
+
+def get_bugsummary_filter_for_user(user):
+    """Build a Storm expression to filter BugSummary by visibility.
+
+    :param user: The user for which visible rows should be calculated.
+    :return: (with_clauses, where_clauses)
+    """
+    # Admins get to see every bug, everyone else only sees bugs
+    # viewable by them-or-their-teams.
+    # Note that because admins can see every bug regardless of
+    # subscription they will see rather inflated counts. Admins get to
+    # deal.
+    public_filter = And(
+        BugSummary.viewed_by_id == None,
+        BugSummary.access_policy_id == None)
+    if user is None:
+        return [], [public_filter]
+    elif IPersonRoles(user).in_admin:
+        return [], []
+    else:
+        with_clauses = [
+            With(
+                'teams',
+                Select(
+                    TeamParticipation.teamID, tables=[TeamParticipation],
+                    where=(TeamParticipation.personID == user.id))),
+            With(
+                'policies',
+                Select(
+                    AccessPolicyGrant.policy_id,
+                    tables=[AccessPolicyGrant],
+                    where=(
+                        AccessPolicyGrant.grantee_id.is_in(
+                            SQL("SELECT team FROM teams"))))),
+            ]
+        where_clauses = [Or(
+            public_filter,
+            BugSummary.viewed_by_id.is_in(
+                SQL("SELECT team FROM teams")),
+            BugSummary.access_policy_id.is_in(
+                SQL("SELECT policy FROM policies")))]
+        return with_clauses, where_clauses

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2012-07-24 10:03:32 +0000
+++ lib/lp/bugs/model/bugtask.py	2012-07-25 08:05:24 +0000
@@ -1549,7 +1549,10 @@
     def countBugs(self, user, contexts, group_on):
         """See `IBugTaskSet`."""
         # Circular fail.
-        from lp.bugs.model.bugsummary import BugSummary
+        from lp.bugs.model.bugsummary import (
+            BugSummary,
+            get_bugsummary_filter_for_user,
+            )
         conditions = []
         # Open bug statuses
         conditions.append(
@@ -1577,27 +1580,14 @@
             conditions.append(BugSummary.tag == None)
         else:
             conditions.append(BugSummary.tag != None)
+
+        # Apply the privacy filter.
         store = IStore(BugSummary)
-        admin_team = getUtility(ILaunchpadCelebrities).admin
-        if user is not None and not user.inTeam(admin_team):
-            # admins get to see every bug, everyone else only sees bugs
-            # viewable by them-or-their-teams.
-            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 user is None:
-            conditions.append(BugSummary.viewed_by_id == None)
-        elif not user.inTeam(admin_team):
-            conditions.append(
-                Or(
-                    BugSummary.viewed_by_id == None,
-                    BugSummary.viewed_by_id.is_in(
-                        SQL("SELECT team FROM teams"))
-                    ))
+        user_with, user_where = get_bugsummary_filter_for_user(user)
+        if user_with:
+            store = store.with_(user_with)
+        conditions.extend(user_where)
+
         sum_count = Sum(BugSummary.count)
         resultset = store.find(group_on + (sum_count,), *conditions)
         resultset.group_by(*group_on)

=== modified file 'lib/lp/bugs/model/tests/test_bugsummary.py'
--- lib/lp/bugs/model/tests/test_bugsummary.py	2012-05-24 22:37:33 +0000
+++ lib/lp/bugs/model/tests/test_bugsummary.py	2012-07-25 08:05:24 +0000
@@ -16,10 +16,12 @@
     BugTaskStatus,
     )
 from lp.bugs.model.bug import BugTag
-from lp.bugs.model.bugsummary import BugSummary
+from lp.bugs.model.bugsummary import (
+    BugSummary,
+    get_bugsummary_filter_for_user,
+    )
 from lp.bugs.model.bugtask import BugTask
 from lp.registry.enums import InformationType
-from lp.registry.model.teammembership import TeamParticipation
 from lp.services.database.lpstorm import IMasterStore
 from lp.testing import TestCaseWithFactory
 from lp.testing.dbuser import switch_dbuser
@@ -41,21 +43,14 @@
 
     def getCount(self, person, **kw_find_expr):
         self._maybe_rollup()
-
-        public_summaries = self.store.find(
-            BugSummary,
-            BugSummary.viewed_by == None,
-            **kw_find_expr)
-        private_summaries = self.store.find(
-            BugSummary,
-            BugSummary.viewed_by_id == TeamParticipation.teamID,
-            TeamParticipation.person == person,
-            **kw_find_expr)
-        all_summaries = public_summaries.union(private_summaries, all=True)
-
+        store = self.store
+        user_with, user_where = get_bugsummary_filter_for_user(person)
+        if user_with:
+            store = store.with_(user_with)
+        summaries = store.find(BugSummary, *user_where, **kw_find_expr)
         # Note that if there a 0 records found, sum() returns None, but
         # we prefer to return 0 here.
-        return all_summaries.sum(BugSummary.count) or 0
+        return summaries.sum(BugSummary.count) or 0
 
     def assertCount(self, count, user=None, **kw_find_expr):
         self.assertEqual(count, self.getCount(user, **kw_find_expr))