Merge lp:~adeuring/launchpad/security-guarded-test-object-factory-3 into lp:launchpad

[Abel Deuring]

this branch is another sequel to ensure that objects returned by methods of LPObjectFactory are wrapped in security proxies. It changes the methods makeBranchMergeProposal(), makeCodeReviewVoteReference(), makeBinaryPackagePublishingHistory(), makeDistributionSourcePackage() and affected tests.

See also the branches lp:~adeuring/launchpad/security-guarded-test-object-factory-1 and ~adeuring/launchpad/security-guarded-test-object-factory-2 and bug 607236

[Abel Deuring (adeuring)]

erm, tests: All those that I changed.

No newly introduced lint.

[Aaron Bentley (abentley)]

This is a team-wide change. Was there a preimplementation call for this?

I certainly don't remember being consulted, and I don't think shouting at anyone is an improvement. We all already know that removeSecurityProxy is a warning sign, but it has legitimate uses.

[Henning Eggers (henninge)]

With the ongoing discussion in mind, I refrain from approving this change.

review: Abstain (code)

[Abel Deuring (adeuring)]

Let's wait for the result of the related discussion on the mailing list

Unmerged revisions

11164. By Abel Deuring on 2010-07-25

trunk merged

11163. By Abel Deuring on 2010-07-22

fixed test failures in bug-heat.txt

11162. By Abel Deuring on 2010-07-22

fixed test failures in test_bugheat.py

11161. By Abel Deuring on 2010-07-22

trunk merged

11160. By Abel Deuring on 2010-07-22

parent branch merged

11159. By Abel Deuring on 2010-07-22

add securoty proxies to the result returned by LaunchpadObjectFactory.makeBranchMergeProposal() and LaunchpadObjectFactory.makeDistributionSourcePackage()

11158. By Abel Deuring on 2010-07-22

fixed a failing test in test_braachmergeproposal.py

11157. By Abel Deuring on 2010-07-21

let LaunchpadObjectFactory.makeCodeReviewVoteReference() and LaunchpadObjectFactory.makeBinaryPackagePublishingHistory() return security proxied objects

=== modified file 'lib/lp/bugs/doc/bug-heat.txt'
--- lib/lp/bugs/doc/bug-heat.txt	2010-06-22 16:08:05 +0000
+++ lib/lp/bugs/doc/bug-heat.txt	2010-07-26 08:41:50 +0000
@@ -311,15 +311,18 @@
 A DistributionSourcePackage also has a cached value for bug_count and
 total_bug_heat.
 
-    >>> print dsp.bug_count
+    >>> from lp.testing.factory import (
+    ...     remove_security_proxy_and_shout_at_engineer)
+    >>> naked_dsp = remove_security_proxy_and_shout_at_engineer(dsp)
+    >>> print naked_dsp.bug_count
     1
-    >>> print dsp.total_bug_heat
+    >>> print naked_dsp.total_bug_heat
     123
     >>> dsp_task2 = factory.makeBugTask(target=dsp)
     >>> dsp_task2.bug.setHeat(7)
-    >>> print dsp.bug_count
+    >>> print naked_dsp.bug_count
     2
-    >>> print dsp.total_bug_heat
+    >>> print naked_dsp.total_bug_heat
     130
 
 Transitioning from one target to another, calculates the value for the new

=== modified file 'lib/lp/bugs/tests/test_bugheat.py'
--- lib/lp/bugs/tests/test_bugheat.py	2010-07-18 00:55:24 +0000
+++ lib/lp/bugs/tests/test_bugheat.py	2010-07-26 08:41:50 +0000
@@ -12,7 +12,8 @@
 from canonical.testing import LaunchpadZopelessLayer
 
 from lp.testing import TestCaseWithFactory
-from lp.testing.factory import LaunchpadObjectFactory
+from lp.testing.factory import (
+    LaunchpadObjectFactory, remove_security_proxy_and_shout_at_engineer)
 
 
 class MaxHeatByTargetBase:
@@ -75,10 +76,14 @@
         self.assertEqual(None, self.target.max_bug_heat)
 
     def test_null_total_bug_heat(self):
-        self.assertEqual(None, self.target.total_bug_heat)
+        naked_target = remove_security_proxy_and_shout_at_engineer(
+            self.target)
+        self.assertEqual(None, naked_target.total_bug_heat)
 
     def test_null_bug_count(self):
-        self.assertEqual(None, self.target.bug_count)
+        naked_target = remove_security_proxy_and_shout_at_engineer(
+            self.target)
+        self.assertEqual(None, naked_target.bug_count)
 
 
 class DistributionSourcePackageZeroRecalculateBugHeatCacheTest(
@@ -96,10 +101,14 @@
         self.assertEqual(0, self.target.max_bug_heat)
 
     def test_zero_total_bug_heat(self):
-        self.assertEqual(0, self.target.total_bug_heat)
+        naked_target = remove_security_proxy_and_shout_at_engineer(
+            self.target)
+        self.assertEqual(0, naked_target.total_bug_heat)
 
     def test_zero_bug_count(self):
-        self.assertEqual(0, self.target.bug_count)
+        naked_target = remove_security_proxy_and_shout_at_engineer(
+            self.target)
+        self.assertEqual(0, naked_target.bug_count)
 
 
 class DistributionSourcePackageMultipleBugsRecalculateBugHeatCacheTest(
@@ -131,14 +140,18 @@
         self.assertEqual(self.max_heat, self.target.max_bug_heat)
 
     def test_total_bug_heat(self):
-        self.assertEqual(self.total_heat, self.target.total_bug_heat)
+        naked_target = remove_security_proxy_and_shout_at_engineer(
+            self.target)
+        self.assertEqual(self.total_heat, naked_target.total_bug_heat)
         self.failUnless(
-            self.target.total_bug_heat > self.target.max_bug_heat,
+            naked_target.total_bug_heat > naked_target.max_bug_heat,
             "Total bug heat should be more than the max bug heat, "
             "since we know that multiple bugs have nonzero heat.")
 
     def test_bug_count(self):
-        self.assertEqual(2, self.target.bug_count)
+        naked_target = remove_security_proxy_and_shout_at_engineer(
+            self.target)
+        self.assertEqual(2, naked_target.bug_count)
 
 
 class SourcePackageMaxHeatByTargetTest(

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2010-03-11 20:54:36 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2010-07-26 08:41:50 +0000
@@ -28,6 +28,7 @@
 from lp.code.tests.helpers import add_revision_to_branch
 from lp.testing import (
     login_person, TestCaseWithFactory, time_counter)
+from lp.testing.factory import remove_security_proxy_and_shout_at_engineer
 from lp.testing.views import create_initialized_view
 from lp.code.model.diff import PreviewDiff, StaticDiff
 from canonical.launchpad.database.message import MessageSet
@@ -78,7 +79,8 @@
         """It should be possible to review an unmergeable proposal."""
         request = self.factory.makeCodeReviewVoteReference()
         bmp = request.branch_merge_proposal
-        bmp.rejectBranch(bmp.target_branch.owner, 'foo')
+        naked_bmp = remove_security_proxy_and_shout_at_engineer(bmp)
+        naked_bmp.rejectBranch(bmp.target_branch.owner, 'foo')
         d = DecoratedCodeReviewVoteReference(request, request.reviewer, None)
         self.assertTrue(d.user_can_review)
         self.assertTrue(d.can_change_review)
@@ -752,7 +754,6 @@
 
     layer = LaunchpadFunctionalLayer
 
-
     def _makeCommentFromEmailWithAttachment(self, attachment_body):
         # Make an email message with an attachment, and create a code
         # review comment from it.

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-07-26 06:21:45 +0000
+++ lib/lp/testing/factory.py	2010-07-26 08:41:50 +0000
@@ -1026,7 +1026,7 @@
         else:
             raise AssertionError('Unknown status: %s' % set_state)
 
-        return proposal
+        return ProxyFactory(proposal)
 
     def makeBranchSubscription(self, branch=None, person=None,
                                subscribed_by=None):
@@ -1586,7 +1586,7 @@
     def makeCodeReviewVoteReference(self):
         bmp = removeSecurityProxy(self.makeBranchMergeProposal())
         candidate = self.makePerson()
-        return bmp.nominateReviewer(candidate, bmp.registrant)
+        return ProxyFactory(bmp.nominateReviewer(candidate, bmp.registrant))
 
     def makeMessage(self, subject=None, content=None, parent=None,
                     owner=None):
@@ -2466,7 +2466,7 @@
             section_name=section_name,
             priority=priority)
 
-        return BinaryPackagePublishingHistory(
+        return ProxyFactory(BinaryPackagePublishingHistory(
             distroarchseries=distroarchseries,
             binarypackagerelease=bpr,
             component=bpr.component,
@@ -2476,7 +2476,7 @@
             scheduleddeletiondate=scheduleddeletiondate,
             pocket=pocket,
             priority=priority,
-            archive=archive)
+            archive=archive))
 
     def makeBinaryPackageName(self, name=None):
         if name is None:
@@ -2565,7 +2565,8 @@
         if distribution is None:
             distribution = self.makeDistribution()
 
-        return DistributionSourcePackage(distribution, sourcepackagename)
+        return ProxyFactory(
+            DistributionSourcePackage(distribution, sourcepackagename))
 
     def makeEmailMessage(self, body=None, sender=None, to=None,
                          attachments=None, encode_attachments=False):