launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08582
lp:~wallyworld/launchpad/branchcollection-filter-infotype-1001042 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/branchcollection-filter-infotype-1001042 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1001042 in Launchpad itself: "Update BranchCollection filtering to use branch information_type"
https://bugs.launchpad.net/launchpad/+bug/1001042
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/branchcollection-filter-infotype-1001042/+merge/108886
== Implementation ==
This branch removes the uses of branch transitively_private and explicitly_private from model code. Those attributes are still defined on Branch, and set correctly when Branch is constructed and when information_type is changed. But the model code is ported to use information_type when doing branch queries etc.
Next steps will remove triggers which maintain transitively_private, remove the attributes from the model, delete the columns.
== Tests ==
Change tests to also use information_tyoe.
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/model/branchcollection.py
lib/lp/code/model/branchlookup.py
lib/lp/code/model/branchmergeproposal.py
lib/lp/code/model/revision.py
lib/lp/code/model/tests/test_branch.py
lib/lp/code/model/tests/test_branchcollection.py
lib/lp/code/model/tests/test_branchmergeproposal.py
lib/lp/code/model/tests/test_revision.py
lib/lp/code/stories/webservice/xx-branch.txt
lib/lp/code/stories/webservice/xx-branchmergeproposal.txt
lib/lp/registry/model/distribution.py
--
https://code.launchpad.net/~wallyworld/launchpad/branchcollection-filter-infotype-1001042/+merge/108886
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/branchcollection-filter-infotype-1001042 into lp:launchpad.
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2012-05-15 08:16:09 +0000
+++ lib/lp/code/model/branchcollection.py 2012-06-07 04:55:24 +0000
@@ -56,6 +56,10 @@
from lp.code.model.codereviewcomment import CodeReviewComment
from lp.code.model.codereviewvote import CodeReviewVoteReference
from lp.code.model.seriessourcepackagebranch import SeriesSourcePackageBranch
+from lp.registry.enums import (
+ PRIVATE_INFORMATION_TYPES,
+ PUBLIC_INFORMATION_TYPES,
+ )
from lp.registry.model.distribution import Distribution
from lp.registry.model.distroseries import DistroSeries
from lp.registry.model.person import (
@@ -71,7 +75,10 @@
)
from lp.services.database.decoratedresultset import DecoratedResultSet
from lp.services.database.lpstorm import IStore
-from lp.services.database.sqlbase import quote
+from lp.services.database.sqlbase import (
+ quote,
+ sqlvalues,
+ )
from lp.services.propertycache import get_property_cache
from lp.services.searchbuilder import any
from lp.services.webapp.interfaces import (
@@ -381,7 +388,7 @@
scope_tables = [Branch] + self._tables.values()
scope_expressions = self._branch_filter_expressions
select = self.store.using(*scope_tables).find(
- (Branch.id, Branch.transitively_private, Branch.ownerID),
+ (Branch.id, Branch.information_type, Branch.ownerID),
*scope_expressions)
branches_query = select._get_select()
with_expr = [With("scope_branches", branches_query)
@@ -750,7 +757,8 @@
def _getBranchVisibilityExpression(self, branch_class=Branch):
"""Return the where clauses for visibility."""
- return [branch_class.transitively_private == False]
+ return [
+ branch_class.information_type.is_in(PUBLIC_INFORMATION_TYPES)]
def _getCandidateBranchesWith(self):
"""Return WITH clauses defining candidate branches.
@@ -762,7 +770,8 @@
return [
With("candidate_branches",
SQL("""select id from scope_branches
- where not transitively_private"""))
+ where information_type in %s"""
+ % sqlvalues(PUBLIC_INFORMATION_TYPES)))
]
@@ -842,7 +851,8 @@
Select(Branch.id,
And(Branch.owner == TeamParticipation.teamID,
TeamParticipation.person == person,
- Branch.transitively_private == True)),
+ Branch.information_type.is_in(
+ PRIVATE_INFORMATION_TYPES))),
# Private branches the person is subscribed to, either directly or
# indirectly.
Select(Branch.id,
@@ -850,7 +860,8 @@
BranchSubscription.person ==
TeamParticipation.teamID,
TeamParticipation.person == person,
- Branch.transitively_private == True)))
+ Branch.information_type.is_in(
+ PRIVATE_INFORMATION_TYPES))))
return private_branches
def _getBranchVisibilityExpression(self, branch_class=Branch):
@@ -859,7 +870,8 @@
:param branch_class: The Branch class to use - permits using
ClassAliases.
"""
- public_branches = branch_class.transitively_private == False
+ public_branches = branch_class.information_type.is_in(
+ PUBLIC_INFORMATION_TYPES)
if self._private_branch_ids is None:
# Public only.
return [public_branches]
@@ -880,24 +892,26 @@
# Really an anonymous sitation
return [
With("candidate_branches",
- SQL("""
- select id from scope_branches
- where not transitively_private"""))
+ SQL("""select id from scope_branches
+ where information_type in %s"""
+ % sqlvalues(PUBLIC_INFORMATION_TYPES)))
]
return [
With("teams", self.store.find(TeamParticipation.teamID,
TeamParticipation.personID == person.id)._get_select()),
With("private_branches", SQL("""
SELECT scope_branches.id FROM scope_branches WHERE
- scope_branches.transitively_private AND (
+ scope_branches.information_type in %s AND (
(scope_branches.owner in (select team from teams) OR
EXISTS(SELECT true from BranchSubscription, teams WHERE
branchsubscription.branch = scope_branches.id AND
- branchsubscription.person = teams.team)))""")),
+ branchsubscription.person = teams.team)))"""
+ % sqlvalues(PRIVATE_INFORMATION_TYPES))),
With("candidate_branches", SQL("""
(SELECT id FROM private_branches) UNION
(select id FROM scope_branches
- WHERE not transitively_private)"""))
+ WHERE information_type in %s)"""
+ % sqlvalues(PUBLIC_INFORMATION_TYPES)))
]
def visibleByUser(self, person):
=== modified file 'lib/lp/code/model/branchlookup.py'
--- lib/lp/code/model/branchlookup.py 2012-02-16 23:07:25 +0000
+++ lib/lp/code/model/branchlookup.py 2012-06-07 04:55:24 +0000
@@ -42,6 +42,7 @@
from lp.code.interfaces.codehosting import BRANCH_ID_ALIAS_PREFIX
from lp.code.interfaces.linkedbranch import get_linked_to_branch
from lp.code.model.branch import Branch
+from lp.registry.enums import PUBLIC_INFORMATION_TYPES
from lp.registry.errors import (
NoSuchDistroSeries,
NoSuchSourcePackageName,
@@ -303,7 +304,7 @@
result = store.find(
(Branch.id),
Branch.id == branch_id,
- Branch.transitively_private == False).one()
+ Branch.information_type.is_in(PUBLIC_INFORMATION_TYPES)).one()
if result is None:
return None, None
else:
@@ -320,7 +321,7 @@
result = store.find(
(Branch.id, Branch.unique_name),
Branch.unique_name.is_in(prefixes),
- Branch.transitively_private == False).one()
+ Branch.information_type.is_in(PUBLIC_INFORMATION_TYPES)).one()
if result is None:
return None, None
else:
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2012-04-12 15:16:12 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2012-06-07 04:55:24 +0000
@@ -76,6 +76,7 @@
IncrementalDiff,
PreviewDiff,
)
+from lp.registry.enums import PRIVATE_INFORMATION_TYPES
from lp.registry.interfaces.person import (
IPerson,
IPersonSet,
@@ -194,10 +195,13 @@
@property
def private(self):
return (
- self.source_branch.transitively_private or
- self.target_branch.transitively_private or
+ (self.source_branch.information_type
+ in PRIVATE_INFORMATION_TYPES) or
+ (self.target_branch.information_type
+ in PRIVATE_INFORMATION_TYPES) or
(self.prerequisite_branch is not None and
- self.prerequisite_branch.transitively_private))
+ (self.prerequisite_branch.information_type in
+ PRIVATE_INFORMATION_TYPES)))
reviewer = ForeignKey(
dbName='reviewer', foreignKey='Person',
=== modified file 'lib/lp/code/model/revision.py'
--- lib/lp/code/model/revision.py 2012-02-24 22:00:41 +0000
+++ lib/lp/code/model/revision.py 2012-06-07 04:55:24 +0000
@@ -56,6 +56,7 @@
IRevisionProperty,
IRevisionSet,
)
+from lp.registry.enums import PUBLIC_INFORMATION_TYPES
from lp.registry.interfaces.person import validate_public_person
from lp.registry.interfaces.product import IProduct
from lp.registry.interfaces.projectgroup import IProjectGroup
@@ -159,7 +160,8 @@
self.id == BranchRevision.revision_id,
BranchRevision.branch_id == Branch.id)
if not allow_private:
- query = And(query, Not(Branch.transitively_private))
+ query = And(
+ query, Branch.information_type.is_in(PUBLIC_INFORMATION_TYPES))
if not allow_junk:
query = And(
query,
@@ -504,7 +506,7 @@
Revision,
And(revision_time_limit(day_limit),
person_condition,
- Not(Branch.transitively_private)))
+ Branch.information_type.is_in(PUBLIC_INFORMATION_TYPES)))
result_set.config(distinct=True)
return result_set.order_by(Desc(Revision.revision_date))
@@ -522,8 +524,9 @@
Join(Branch, BranchRevision.branch == Branch.id),
]
- conditions = And(revision_time_limit(day_limit),
- Not(Branch.transitively_private))
+ conditions = And(
+ revision_time_limit(day_limit),
+ Branch.information_type.is_in(PUBLIC_INFORMATION_TYPES))
if IProduct.providedBy(obj):
conditions = And(conditions, Branch.product == obj)
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py 2012-06-04 09:41:48 +0000
+++ lib/lp/code/model/tests/test_branch.py 2012-06-07 04:55:24 +0000
@@ -2350,19 +2350,9 @@
self.assertTrue(branch.private)
self.assertEqual(
stacked_on.information_type, branch.information_type)
- self.assertTrue(removeSecurityProxy(branch).transitively_private)
- self.assertTrue(branch.explicitly_private)
-
- def test_private_stacked_on_public_is_private(self):
- # A private branch stacked on a public branch is private.
- stacked_on = self.factory.makeBranch()
- branch = self.factory.makeBranch(
- stacked_on=stacked_on, information_type=InformationType.USERDATA)
- self.assertTrue(branch.private)
- self.assertNotEqual(
- stacked_on.information_type, branch.information_type)
- self.assertTrue(removeSecurityProxy(branch).transitively_private)
- self.assertTrue(branch.explicitly_private)
+ self.assertEqual(
+ InformationType.USERDATA,
+ removeSecurityProxy(branch).information_type)
def test_personal_branches_for_private_teams_are_private(self):
team = self.factory.makeTeam(visibility=PersonVisibility.PRIVATE)
@@ -2406,8 +2396,7 @@
self.assertFalse(branch.private)
branch.setPrivate(False, branch.owner)
self.assertFalse(branch.private)
- self.assertFalse(removeSecurityProxy(branch).transitively_private)
- self.assertFalse(branch.explicitly_private)
+ self.assertEqual(InformationType.PUBLIC, branch.information_type)
def test_public_to_private_allowed(self):
# If there is a privacy policy allowing the branch owner to have
@@ -2417,8 +2406,6 @@
branch.owner, BranchVisibilityRule.PRIVATE)
branch.setPrivate(True, branch.owner)
self.assertTrue(branch.private)
- self.assertTrue(removeSecurityProxy(branch).transitively_private)
- self.assertTrue(branch.explicitly_private)
self.assertEqual(InformationType.USERDATA, branch.information_type)
def test_public_to_private_not_allowed(self):
@@ -2438,8 +2425,9 @@
admins = getUtility(ILaunchpadCelebrities).admin
branch.setPrivate(True, admins.teamowner)
self.assertTrue(branch.private)
- self.assertTrue(removeSecurityProxy(branch).transitively_private)
- self.assertTrue(branch.explicitly_private)
+ self.assertEqual(
+ InformationType.USERDATA,
+ removeSecurityProxy(branch).information_type)
def test_private_to_private(self):
# Setting a private branch to be private is a no-op.
@@ -2448,8 +2436,9 @@
self.assertTrue(branch.private)
branch.setPrivate(True, branch.owner)
self.assertTrue(branch.private)
- self.assertTrue(removeSecurityProxy(branch).transitively_private)
- self.assertTrue(branch.explicitly_private)
+ self.assertEqual(
+ InformationType.USERDATA,
+ removeSecurityProxy(branch).information_type)
def test_private_to_public_allowed(self):
# If the namespace policy allows public branches, then changing from
@@ -2458,8 +2447,6 @@
information_type=InformationType.USERDATA)
branch.setPrivate(False, branch.owner)
self.assertFalse(branch.private)
- self.assertFalse(removeSecurityProxy(branch).transitively_private)
- self.assertFalse(branch.explicitly_private)
self.assertEqual(InformationType.PUBLIC, branch.information_type)
def test_private_to_public_not_allowed(self):
=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py 2012-05-31 03:54:13 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py 2012-06-07 04:55:24 +0000
@@ -890,8 +890,10 @@
def test_target_branch_private(self):
# The target branch must be in the branch collection, as must the
# source branch.
- mp1 = self.factory.makeBranchMergeProposal()
- removeSecurityProxy(mp1.target_branch).explicitly_private = True
+ registrant = self.factory.makePerson()
+ mp1 = self.factory.makeBranchMergeProposal(registrant=registrant)
+ removeSecurityProxy(mp1.target_branch).transitionToInformationType(
+ InformationType.USERDATA, registrant, verify_policy=False)
collection = self.all_branches.visibleByUser(None)
proposals = collection.getMergeProposals()
self.assertEqual([], list(proposals))
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2012-05-31 03:54:13 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2012-06-07 04:55:24 +0000
@@ -1160,7 +1160,9 @@
# proposals that the logged in user is able to see.
proposal = self._make_merge_proposal('albert', 'november', 'work')
# Mark the source branch private.
- removeSecurityProxy(proposal.source_branch).explicitly_private = True
+ proposal.source_branch.transitionToInformationType(
+ InformationType.USERDATA, proposal.source_branch.owner,
+ verify_policy=False)
self._make_merge_proposal('albert', 'mike', 'work')
albert = getUtility(IPersonSet).getByName('albert')
@@ -1198,10 +1200,10 @@
proposal = self._make_merge_proposal(
'xray', 'november', 'work', registrant=albert)
- # Mark the source branch private by making it's stacked on branch
- # private.
- removeSecurityProxy(
- proposal.source_branch.stacked_on).explicitly_private = True
+ # Mark the source branch private.
+ proposal.source_branch.transitionToInformationType(
+ InformationType.USERDATA, proposal.source_branch.owner,
+ verify_policy=False)
november = getUtility(IProductSet).getByName('november')
# The proposal is visible to charles.
=== modified file 'lib/lp/code/model/tests/test_revision.py'
--- lib/lp/code/model/tests/test_revision.py 2012-05-31 03:54:13 +0000
+++ lib/lp/code/model/tests/test_revision.py 2012-06-07 04:55:24 +0000
@@ -340,7 +340,8 @@
# Only public branches are returned.
b1 = self.makeBranchWithRevision(1)
b2 = self.makeBranchWithRevision(1, owner=self.author)
- removeSecurityProxy(b2).explicitly_private = True
+ removeSecurityProxy(b2).transitionToInformationType(
+ InformationType.USERDATA, b2.owner, verify_policy=False)
self.assertEqual(b1, self.revision.getBranch())
def testAllowPrivateReturnsPrivateBranch(self):
@@ -348,7 +349,8 @@
# returned if they are the best match.
self.makeBranchWithRevision(1)
b2 = self.makeBranchWithRevision(1, owner=self.author)
- removeSecurityProxy(b2).explicitly_private = True
+ removeSecurityProxy(b2).transitionToInformationType(
+ InformationType.USERDATA, b2.owner, verify_policy=False)
self.assertEqual(b2, self.revision.getBranch(allow_private=True))
def testAllowPrivateCanReturnPublic(self):
@@ -356,7 +358,8 @@
# the branches.
b1 = self.makeBranchWithRevision(1)
b2 = self.makeBranchWithRevision(1, owner=self.author)
- removeSecurityProxy(b1).explicitly_private = True
+ removeSecurityProxy(b1).transitionToInformationType(
+ InformationType.USERDATA, b1.owner, verify_policy=False)
self.assertEqual(b2, self.revision.getBranch(allow_private=True))
def testGetBranchNotJunk(self):
@@ -464,7 +467,9 @@
rev1 = self._makeRevision()
b = self._makeBranch()
b.createBranchRevision(1, rev1)
- removeSecurityProxy(b).explicitly_private = True
+
+ removeSecurityProxy(b).transitionToInformationType(
+ InformationType.USERDATA, b.owner, verify_policy=False)
self.assertEqual([], self._getRevisions())
def testRevisionDateRange(self):
=== modified file 'lib/lp/code/stories/webservice/xx-branch.txt'
--- lib/lp/code/stories/webservice/xx-branch.txt 2012-05-04 10:39:00 +0000
+++ lib/lp/code/stories/webservice/xx-branch.txt 2012-06-07 04:55:24 +0000
@@ -262,17 +262,14 @@
>>> response = webservice.named_post(
... branch_url, 'setPrivate', api_version='beta', private=True)
>>> branch = webservice.get(branch_url).jsonBody()
- >>> print branch['explicitly_private']
- True
- >>> print branch['private']
- True
+ >>> print branch['information_type']
+ User Data
-In subsequent versions, 'setPrivate' is gone; you have to set the
-'explicitly_private' field directly.
+In subsequent versions, 'setPrivate' is gone; you have to use the
+'transitionToInformationType' method.
>>> print webservice.named_post(
- ... branch_url, 'setPrivate', api_version='devel',
- ... explicitly_private=True)
+ ... branch_url, 'setPrivate', api_version='devel', private=True)
HTTP/1.1 400 Bad Request
...
No such operation: setPrivate
=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2012-03-07 07:15:00 +0000
+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2012-06-07 04:55:24 +0000
@@ -420,8 +420,11 @@
>>> login('admin@xxxxxxxxxxxxx')
>>> from zope.security.proxy import removeSecurityProxy
- >>> removeSecurityProxy(fixit_proposal.source_branch).explicitly_private = True
+ >>> from lp.registry.enums import InformationType
>>> branch_owner = fixit_proposal.source_branch.owner
+ >>> removeSecurityProxy(
+ ... fixit_proposal.source_branch).transitionToInformationType(
+ ... InformationType.USERDATA, branch_owner, verify_policy=False)
>>> logout()
>>> print_proposals(
@@ -439,7 +442,9 @@
http://.../~source/fooix/fix-it/+merge/... - Approved
>>> login('admin@xxxxxxxxxxxxx')
- >>> removeSecurityProxy(fixit_proposal.source_branch).explicitly_private = False
+ >>> removeSecurityProxy(
+ ... fixit_proposal.source_branch).transitionToInformationType(
+ ... InformationType.PUBLIC, branch_owner)
>>> logout()
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py 2012-06-05 11:40:48 +0000
+++ lib/lp/registry/model/distribution.py 2012-06-07 04:55:24 +0000
@@ -98,6 +98,7 @@
)
from lp.registry.enums import (
InformationType,
+ PRIVATE_INFORMATION_TYPES,
PUBLIC_INFORMATION_TYPES,
)
from lp.registry.errors import NoSuchDistroSeries
@@ -639,7 +640,7 @@
Branch.last_scanned_id,
SPBDS.name AS distro_series_name,
Branch.id,
- Branch.transitively_private,
+ Branch.information_type,
Branch.owner
FROM Branch
JOIN DistroSeries
@@ -659,7 +660,9 @@
# Now we see just a touch of privacy concerns.
# If the current user is anonymous, they cannot see any private
# branches.
- base_query += (' AND NOT Branch.transitively_private\n')
+ base_query += (
+ ' AND Branch.information_type in %s\n'
+ % sqlvalues(PUBLIC_INFORMATION_TYPES))
# We want to order the results, in part for easier grouping at the
# end.
base_query += 'ORDER BY unique_name, last_scanned_id'
@@ -693,7 +696,7 @@
id,
owner
FROM all_branches
- WHERE transitively_private
+ WHERE information_type in %(private_branches)s
), owned_branch_ids AS (
SELECT private_branches.id
FROM private_branches
@@ -708,10 +711,14 @@
)
SELECT unique_name, last_scanned_id, distro_series_name
FROM all_branches
- WHERE NOT transitively_private OR
+ WHERE information_type in %(public_branches)s OR
id IN (SELECT id FROM owned_branch_ids) OR
id IN (SELECT id FROM subscribed_branch_ids)
- """ % dict(base_query=base_query, user=quote(user.id))
+ """ % dict(
+ base_query=base_query,
+ user=quote(user.id),
+ private_branches=sqlvalues(PRIVATE_INFORMATION_TYPES),
+ public_branches=sqlvalues(PUBLIC_INFORMATION_TYPES))
data = Store.of(self).execute(query + ';')
Follow ups
