launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10262
[Merge] lp:~wgrant/launchpad/bugsummary-v2-apg-app into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bugsummary-v2-apg-app into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1017748 in Launchpad itself: "BugSummary assumes that subscription == visibility"
https://bugs.launchpad.net/launchpad/+bug/1017748
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bugsummary-v2-apg-app/+merge/116594
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.
--
https://code.launchpad.net/~wgrant/launchpad/bugsummary-v2-apg-app/+merge/116594
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bugsummary-v2-apg-app into lp:launchpad.
=== 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))
Follow ups
