Merge lp:~stevenk/launchpad/switch-bmp-to-previewdiff-merge_proposal into lp:launchpad

[Steve Kowalik]

Switch everything over to using PreviewDiff.branch_merge_proposal, and remove BranchMergeProposal.preview_diff in preparation for dropping the column.

Switch everything over to using PreviewDiff.merge_propsal, and remove BranchMergeProposal.preview_diff in preparation for dropping the column.

[William Grant (wgrant)]

207 + @cachedproperty
208 + def preview_diffs(self):
209 + return Store.of(self).find(
210 + PreviewDiff, PreviewDiff.merge_proposal_id == self.id).order_by(
211 + PreviewDiff.date_created)

Caching a ResultSet achieves nothing. You probably want to cache a listified version instead.

213 + @property
214 + def preview_diff(self):
215 + diffs = list(self.preview_diffs)
216 + if diffs:
217 + return diffs[-1]
218 + return None

Materialising all of the PreviewDiffs just for this is probably pointless, particularly given how cheap it is to find just the latest. I'd probably move the ResultSet into _preview_diffs, then have preview_diffs listify and cache it, and preview_diff grab the first and cache it. If you wanted to be excessively performant you could even have preview_diff check is preview_diffs was cached, and if so just grab the first from there, but that's probably pointless.

292 + for previewdiff in preview_diffs:
293 + cache = get_property_cache(previewdiff.merge_proposal)
294 + cache.preview_diffs.append(previewdiff)

Is preloading preview_diffs valuable here, or would it be sufficient to simply preload the latest, preview_diff?

371 + @property
372 def branch_merge_proposal(self):
373 - # This can turn into return self.merge_proposal when it's populated.
374 - if self.merge_proposal:
375 - return self.merge_proposal
376 - return self._branch_merge_proposal
377 + return self.merge_proposal

Why does this still exist?

review: Approve (code)

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2013-05-13 06:57:37 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2013-05-13 06:57:38 +0000
@@ -871,25 +871,23 @@
         # librarian content.
         text = ''.join(chr(x) for x in range(255))
         diff_bytes = ''.join(unified_diff('', text))
-        self.setPreviewDiff(diff_bytes)
+        preview_diff = self.setPreviewDiff(diff_bytes)
         transaction.commit()
 
         def fake_open(*args):
             raise LibrarianServerError
 
-        lfa = removeSecurityProxy(self.bmp.preview_diff).diff.diff_text
+        lfa = preview_diff.diff.diff_text
         with monkey_patch(lfa, open=fake_open):
-            view = create_initialized_view(self.bmp.preview_diff, '+diff')
+            view = create_initialized_view(preview_diff, '+diff')
             self.assertEqual('', view.preview_diff_text)
             self.assertFalse(view.diff_available)
             markup = view()
             self.assertIn('The diff is not available at this time.', markup)
 
     def setPreviewDiff(self, preview_diff_bytes):
-        preview_diff = PreviewDiff.create(
+        return PreviewDiff.create(
             self.bmp, preview_diff_bytes, u'a', u'b', None, u'')
-        removeSecurityProxy(self.bmp).preview_diff = preview_diff
-        return preview_diff
 
     def test_linked_bugs_excludes_mutual_bugs(self):
         """List bugs that are linked to the source only."""

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py	2013-03-15 01:27:19 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py	2013-05-13 06:57:38 +0000
@@ -207,7 +207,7 @@
         with StormStatementRecorder() as recorder:
             self.getViewBrowser(
                 product, '+merges', rootsite='code', user=product.owner)
-        self.assertThat(recorder, HasQueryCount(Equals(40)))
+        self.assertThat(recorder, HasQueryCount(Equals(41)))
 
     def test_productseries(self):
         target = self.factory.makeBranch()

=== modified file 'lib/lp/code/doc/branch-merge-proposal-notifications.txt'
--- lib/lp/code/doc/branch-merge-proposal-notifications.txt	2013-05-13 06:57:37 +0000
+++ lib/lp/code/doc/branch-merge-proposal-notifications.txt	2013-05-13 06:57:38 +0000
@@ -19,20 +19,16 @@
     >>> from lp.code.interfaces.branchmergeproposal import (
     ...     IBranchMergeProposalJobSource)
     >>> from lp.code.model.diff import PreviewDiff
-    >>> from lp.code.tests.helpers import (
-    ...     make_merge_proposal_without_reviewers)
+    >>> from lp.code.tests.helpers import make_merge_proposal_without_reviewers
     >>> from lp.testing.mail_helpers import pop_notifications
     >>> import transaction
     >>> login('[e-mail]')
-    >>> diff_bytes = ''.join(unified_diff('', 'c'))
-    >>> preview_diff = PreviewDiff.create(
-    ...     None, diff_bytes, u'a', u'b', None, None)
-    >>> # Make Librarian happy.
-    >>> transaction.commit()
     >>> target_owner = factory.makePerson(email='[e-mail]')
     >>> target_branch = factory.makeBranch(owner=target_owner)
     >>> bmp = make_merge_proposal_without_reviewers(
     ...     factory, target_branch=target_branch)
+    >>> previewdiff = factory.makePreviewDiff(merge_proposal=bmp)
+    >>> transaction.commit()
     >>> source_subscriber = factory.makePerson(
     ...     email='[e-mail]', displayname='Source Subscriber')
     >>> _unused = bmp.source_branch.subscribe(source_subscriber,
@@ -111,7 +107,8 @@
     >>> # To avoid needing to access branches, pre-populate diffs.
     >>> bmp = source_branch.addLandingTarget(
     ...     registrant, target_branch, needs_review=True)
-    >>> removeSecurityProxy(bmp).preview_diff = preview_diff
+    >>> previewdiff = factory.makePreviewDiff(merge_proposal=bmp)
+    >>> transaction.commit()
     >>> # Fake the update preview diff as done.
     >>> bmp.next_preview_diff_job.start()
     >>> bmp.next_preview_diff_job.complete()
@@ -165,10 +162,10 @@
     ...     """)
     >>> bmp.deleteProposal()
     >>> bmp = source_branch.addLandingTarget(
-    ...     registrant, target_branch,
-    ...     description=initial_comment, review_requests=reviewers,
-    ...     needs_review=True)
-    >>> removeSecurityProxy(bmp).preview_diff = preview_diff
+    ...     registrant, target_branch, description=initial_comment,
+    ...     review_requests=reviewers, needs_review=True)
+    >>> previewdiff = factory.makePreviewDiff(merge_proposal=bmp)
+    >>> transaction.commit()
     >>> # Fake the update preview diff as done.
     >>> bmp.next_preview_diff_job.start()
     >>> bmp.next_preview_diff_job.complete()

=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py	2013-02-28 01:02:50 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py	2013-05-13 06:57:38 +0000
@@ -174,6 +174,8 @@
     next_preview_diff_job = Attribute(
         'The next BranchMergeProposalJob that will update a preview diff.')
 
+    preview_diffs = Attribute('All preview diffs for this merge proposal.')
+
     preview_diff = exported(
         Reference(
             IPreviewDiff,

=== modified file 'lib/lp/code/mail/tests/test_branchmergeproposal.py'
--- lib/lp/code/mail/tests/test_branchmergeproposal.py	2013-05-13 06:57:37 +0000
+++ lib/lp/code/mail/tests/test_branchmergeproposal.py	2013-05-13 06:57:38 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for BranchMergeProposal mailings"""
@@ -67,15 +67,12 @@
         else:
             initial_status = BranchMergeProposalStatus.WORK_IN_PROGRESS
         bmp = self.factory.makeBranchMergeProposal(
-            registrant=registrant, product=product,
-            set_state=initial_status,
+            registrant=registrant, product=product, set_state=initial_status,
             prerequisite_branch=prerequisite_branch,
-            initial_comment=initial_comment,
-            reviewer=reviewer)
-        if diff_text is not None:
-            removeSecurityProxy(bmp).preview_diff = PreviewDiff.create(
-                bmp, diff_text,
-                unicode(self.factory.getUniqueString('revid')),
+            initial_comment=initial_comment, reviewer=reviewer)
+        if diff_text:
+            PreviewDiff.create(
+                bmp, diff_text, unicode(self.factory.getUniqueString('revid')),
                 unicode(self.factory.getUniqueString('revid')), None, None)
             transaction.commit()
         subscriber = self.factory.makePerson(displayname='Baz Quxx',

=== modified file 'lib/lp/code/mail/tests/test_codehandler.py'
--- lib/lp/code/mail/tests/test_codehandler.py	2013-05-13 06:57:37 +0000
+++ lib/lp/code/mail/tests/test_codehandler.py	2013-05-13 06:57:38 +0000
@@ -381,15 +381,10 @@
 
     def test_reviewer_with_diff(self):
         """Requesting a review with a diff works."""
-        diff_text = ''.join(unified_diff('', 'Fake diff'))
-        preview_diff = PreviewDiff.create(
-            None, diff_text,
-            unicode(self.factory.getUniqueString('revid')),
-            unicode(self.factory.getUniqueString('revid')), None, None)
+        bmp = make_merge_proposal_without_reviewers(self.factory)
+        preview_diff = self.factory.makePreviewDiff(merge_proposal=bmp)
         # To record the diff in the librarian.
         transaction.commit()
-        bmp = make_merge_proposal_without_reviewers(
-            self.factory, preview_diff=preview_diff)
         eric = self.factory.makePerson(name="eric", email="[e-mail]")
         mail = self.factory.makeSignedMessage(body=' reviewer eric')
         email_addr = bmp.address

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2013-05-13 06:57:37 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2013-05-13 06:57:38 +0000
@@ -27,10 +27,7 @@
     Select,
     SQL,
     )
-from storm.locals import (
-    Int,
-    Reference,
-    )
+from storm.locals import Reference
 from storm.store import Store
 from zope.component import getUtility
 from zope.event import notify
@@ -104,6 +101,10 @@
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
 from lp.services.mail.sendmail import validate_message
+from lp.services.propertycache import (
+    cachedproperty,
+    get_property_cache,
+    )
 
 
 def is_valid_transition(proposal, from_state, next_state, user=None):
@@ -226,9 +227,6 @@
         else:
             return None
 
-    preview_diff_id = Int(name='merge_diff')
-    preview_diff = Reference(preview_diff_id, 'PreviewDiff.id')
-
     reviewed_revision_id = StringCol(default=None)
 
     commit_message = StringCol(default=None)
@@ -309,6 +307,21 @@
             raise WrongBranchMergeProposal
         return vote
 
+    @property
+    def _preview_diffs(self):
+        return Store.of(self).find(
+            PreviewDiff,
+            PreviewDiff.branch_merge_proposal_id == self.id).order_by(
+                PreviewDiff.date_created)
+
+    @cachedproperty
+    def preview_diffs(self):
+        return list(self._preview_diffs)
+
+    @cachedproperty
+    def preview_diff(self):
+        return self._preview_diffs.first()
+
     date_queued = UtcDateTimeCol(notNull=False, default=None)
 
     votes = SQLMultipleJoin(
@@ -706,11 +719,11 @@
             comment.destroySelf()
         # Delete all jobs referring to the BranchMergeProposal, whether
         # or not they have completed.
-        from lp.code.model.branchmergeproposaljob import (
-            BranchMergeProposalJob)
+        from lp.code.model.branchmergeproposaljob import BranchMergeProposalJob
         for job in BranchMergeProposalJob.selectBy(
             branch_merge_proposal=self.id):
             job.destroySelf()
+        self.preview_diffs.remove()
         self.destroySelf()
 
     def getUnlandedSourceBranchRevisions(self):
@@ -860,18 +873,10 @@
                           target_revision_id, prerequisite_revision_id=None,
                           conflicts=None):
         """See `IBranchMergeProposal`."""
-        # Create the PreviewDiff.
-        self.preview_diff = PreviewDiff.create(
+        return PreviewDiff.create(
             self, diff_content, source_revision_id, target_revision_id,
             prerequisite_revision_id, conflicts)
 
-        # XXX: TimPenhey 2009-02-19 bug 324724
-        # Since the branch_merge_proposal attribute of the preview_diff
-        # is a on_remote reference, it may not be found unless we flush
-        # the storm store.
-        Store.of(self).flush()
-        return self.preview_diff
-
     def getIncrementalDiffRanges(self):
         groups = self.getRevisionsSinceReviewStart()
         return [
@@ -965,14 +970,14 @@
         from lp.registry.model.product import Product
         from lp.registry.model.distroseries import DistroSeries
 
+        ids = set()
         source_branch_ids = set()
         person_ids = set()
-        diff_ids = set()
         for mp in branch_merge_proposals:
+            ids.add(mp.id)
             source_branch_ids.add(mp.source_branchID)
             person_ids.add(mp.registrantID)
             person_ids.add(mp.merge_reporterID)
-            diff_ids.add(mp.preview_diff_id)
 
         branches = load_related(
             Branch, branch_merge_proposals, (
@@ -985,16 +990,17 @@
         if len(branches) == 0:
             return
 
-        store = IStore(BranchMergeProposal)
-
         # Pre-load PreviewDiffs and Diffs.
-        preview_diffs_and_diffs = list(store.find(
-            (PreviewDiff, Diff),
-            PreviewDiff.id.is_in(diff_ids),
-            Diff.id == PreviewDiff.diff_id))
-        PreviewDiff.preloadData(
-            [preview_diff_and_diff[0] for preview_diff_and_diff
-                in preview_diffs_and_diffs])
+        preview_diffs = IStore(BranchMergeProposal).find(
+            PreviewDiff,
+            PreviewDiff.branch_merge_proposal_id.is_in(ids)).order_by(
+                PreviewDiff.branch_merge_proposal_id,
+                Desc(PreviewDiff.date_created)).config(
+                    distinct=[PreviewDiff.branch_merge_proposal_id])
+        load_related(Diff, preview_diffs, ['diff_id'])
+        for previewdiff in preview_diffs:
+            cache = get_property_cache(previewdiff.branch_merge_proposal)
+            cache.preview_diff = previewdiff
 
         # Add source branch owners' to the list of pre-loaded persons.
         person_ids.update(

=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
--- lib/lp/code/model/branchmergeproposaljob.py	2012-11-30 05:10:47 +0000
+++ lib/lp/code/model/branchmergeproposaljob.py	2013-05-13 06:57:38 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Job classes related to BranchMergeProposals are in here.
@@ -371,10 +371,8 @@
         """See `IRunnableJob`."""
         self.checkReady()
         with server(get_ro_server(), no_replace=True):
-            preview = PreviewDiff.fromBranchMergeProposal(
-                self.branch_merge_proposal)
-        with BranchMergeProposalDelta.monitor(self.branch_merge_proposal):
-            self.branch_merge_proposal.preview_diff = preview
+            with BranchMergeProposalDelta.monitor(self.branch_merge_proposal):
+                PreviewDiff.fromBranchMergeProposal(self.branch_merge_proposal)
 
     def getOperationDescription(self):
         return ('generating the diff for a merge proposal')

=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py	2013-05-13 06:57:37 +0000
+++ lib/lp/code/model/diff.py	2013-05-13 06:57:38 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementation classes for IDiff, etc."""
@@ -48,7 +48,6 @@
     )
 from lp.codehosting.bzrutils import read_locked
 from lp.services.config import config
-from lp.services.database.bulk import load_referencing
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.datetimecol import UtcDateTimeCol
 from lp.services.database.sqlbase import SQLBase
@@ -56,10 +55,7 @@
 from lp.services.librarian.interfaces.client import (
     LIBRARIAN_SERVER_DEFAULT_TIMEOUT,
     )
-from lp.services.propertycache import (
-    cachedproperty,
-    get_property_cache,
-    )
+from lp.services.propertycache import get_property_cache
 from lp.services.webapp.adapter import get_request_remaining_seconds
 
 
@@ -364,11 +360,13 @@
 
     prerequisite_revision_id = Unicode(name='dependent_revision_id')
 
-    branch_merge_proposal_id = Int(name='branch_merge_proposal')
-    _new_branch_merge_proposal = Reference(
+    branch_merge_proposal_id = Int(
+        name='branch_merge_proposal', allow_none=False)
+    branch_merge_proposal = Reference(
         branch_merge_proposal_id, 'BranchMergeProposal.id')
 
-    date_created = UtcDateTimeCol(dbName='date_created', default=UTC_NOW)
+    date_created = UtcDateTimeCol(
+        dbName='date_created', default=UTC_NOW, notNull=True)
 
     conflicts = Unicode()
 
@@ -376,29 +374,6 @@
     def has_conflicts(self):
         return self.conflicts is not None and self.conflicts != ''
 
-    @staticmethod
-    def preloadData(preview_diffs):
-        # Circular imports.
-        from lp.code.model.branchmergeproposal import BranchMergeProposal
-        bmps = load_referencing(
-            BranchMergeProposal, preview_diffs, ['preview_diff_id'])
-        bmps_preview = dict((bmp.preview_diff_id, bmp) for bmp in bmps)
-        for preview_diff in preview_diffs:
-            cache = get_property_cache(preview_diff)
-            cache.branch_merge_proposal = bmps_preview[preview_diff.id]
-
-    _branch_merge_proposal = Reference(
-        "PreviewDiff.id", "BranchMergeProposal.preview_diff_id",
-        on_remote=True)
-
-    @cachedproperty
-    def branch_merge_proposal(self):
-        # This property can die when self._new_branch_merge_proposal is
-        # populated.
-        if self._new_branch_merge_proposal:
-            return self._new_branch_merge_proposal
-        return self._branch_merge_proposal
-
     @classmethod
     def fromBranchMergeProposal(cls, bmp):
         """Create a `PreviewDiff` from a `BranchMergeProposal`.
@@ -416,9 +391,7 @@
         else:
             prerequisite_branch = None
         diff, conflicts = Diff.mergePreviewFromBranches(
-            source_branch, source_revision, target_branch,
-            prerequisite_branch)
-
+            source_branch, source_revision, target_branch, prerequisite_branch)
         preview = cls()
         preview.source_revision_id = source_revision.decode('utf-8')
         preview.target_revision_id = target_revision.decode('utf-8')
@@ -426,6 +399,7 @@
         preview.diff = diff
         preview.conflicts = u''.join(
             unicode(conflict) + '\n' for conflict in conflicts)
+        del get_property_cache(bmp).preview_diffs
         return preview
 
     @classmethod

=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py	2013-02-14 03:53:10 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py	2013-05-13 06:57:38 +0000
@@ -5,10 +5,16 @@
 
 __metaclass__ = type
 
-from datetime import datetime
+from datetime import (
+    datetime,
+    timedelta,
+    )
 
 import pytz
-from storm.store import EmptyResultSet
+from storm.store import (
+    EmptyResultSet,
+    Store,
+    )
 from testtools.matchers import Equals
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -36,11 +42,7 @@
 from lp.registry.enums import PersonVisibility
 from lp.registry.interfaces.person import TeamMembershipPolicy
 from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.services.database.interfaces import (
-    DEFAULT_FLAVOR,
-    IStoreSelector,
-    MAIN_STORE,
-    )
+from lp.services.database.lpstorm import IStore
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
     person_logged_in,
@@ -48,7 +50,10 @@
     StormStatementRecorder,
     TestCaseWithFactory,
     )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.testing.matchers import HasQueryCount
 
 
@@ -94,10 +99,9 @@
     layer = DatabaseFunctionalLayer
 
     def setUp(self):
-        TestCaseWithFactory.setUp(self)
+        super(TestGenericBranchCollection, self).setUp()
         remove_all_sample_data_branches()
-        self.store = getUtility(IStoreSelector).get(
-            MAIN_STORE, DEFAULT_FLAVOR)
+        self.store = IStore(Branch)
 
     def test_provides_branchcollection(self):
         # `GenericBranchCollection` provides the `IBranchCollection`
@@ -879,7 +883,7 @@
 
 class TestBranchMergeProposals(TestCaseWithFactory):
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def setUp(self):
         TestCaseWithFactory.setUp(self)
@@ -940,6 +944,35 @@
         proposals = collection.getMergeProposals()
         self.assertEqual(sorted([mp1, mp2]), sorted(proposals))
 
+    def test_preloading_for_previewdiff(self):
+        product = self.factory.makeProduct()
+        target = self.factory.makeBranch(product=product)
+        owner = self.factory.makePerson()
+        branch1 = self.factory.makeBranch(product=product, owner=owner)
+        branch2 = self.factory.makeBranch(product=product, owner=owner)
+        bmp1 = self.factory.makeBranchMergeProposal(
+            target_branch=target, source_branch=branch1)
+        bmp2 = self.factory.makeBranchMergeProposal(
+            target_branch=target, source_branch=branch2)
+        old_date = datetime.now(pytz.UTC) - timedelta(hours=1)
+        self.factory.makePreviewDiff(
+            merge_proposal=bmp1, date_created=old_date)
+        previewdiff1 = self.factory.makePreviewDiff(merge_proposal=bmp1)
+        self.factory.makePreviewDiff(
+            merge_proposal=bmp2, date_created=old_date)
+        previewdiff2 = self.factory.makePreviewDiff(merge_proposal=bmp2)
+        Store.of(bmp1).flush()
+        Store.of(bmp1).invalidate()
+        collection = self.all_branches.ownedBy(owner)
+        [pre_bmp1, pre_bmp2] = sorted(
+            collection.getMergeProposals(eager_load=True))
+        with StormStatementRecorder() as recorder:
+            self.assertEqual(
+                removeSecurityProxy(pre_bmp1.preview_diff).id, previewdiff1.id)
+            self.assertEqual(
+                removeSecurityProxy(pre_bmp2.preview_diff).id, previewdiff2.id)
+        self.assertThat(recorder, HasQueryCount(Equals(0)))
+
     def test_merge_proposals_in_product(self):
         mp1 = self.factory.makeBranchMergeProposal()
         self.factory.makeBranchMergeProposal()

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2013-05-13 06:57:37 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2013-05-13 06:57:38 +0000
@@ -2085,6 +2085,6 @@
         # A previewdiff with an empty diffstat doesn't crash when fetched.
         previewdiff = self.factory.makePreviewDiff()
         previewdiff.diff.diffstat = None
-        user = previewdiff._new_branch_merge_proposal.target_branch.owner
+        user = previewdiff.branch_merge_proposal.target_branch.owner
         ws_previewdiff = self.wsObject(previewdiff, user=user)
         self.assertIsNone(ws_previewdiff.diffstat)

=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt	2013-05-13 06:57:37 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt	2013-05-13 06:57:38 +0000
@@ -173,14 +173,11 @@
 
 After a preview diff is added, it has a line count.
 
-    >>> from lp.code.model.diff import PreviewDiff
     >>> login('[e-mail]')
-    >>> removeSecurityProxy(proposal).preview_diff = PreviewDiff.create(
-    ...    proposal, 'a\n\b\n', u'source-id', u'target-id', u'prereq-id',
-    ...    u'conflicts')
+    >>> diff = factory.makePreviewDiff(merge_proposal=proposal)
     >>> logout()
     >>> browser.open(url)
     >>> print_tag_with_id(browser.contents, 'proposals')
     Other reviews I am not actively reviewing
     Branch Merge Proposal             Requested By    Lines Activity
-    lp://dev/...                      ...             2     None
+    lp://dev/...                      ...             14    None

=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2013-05-13 06:57:37 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2013-05-13 06:57:38 +0000
@@ -609,11 +609,10 @@
     >>> from lp.code.model.diff import PreviewDiff
     >>> diff_text = ''.join(
     ...     unified_diff('', ['Fake Diff' + u'\u1010'.encode('utf-8')]))
+    >>> bmp = factory.makeBranchMergeProposal()
     >>> preview_diff = PreviewDiff.create(
-    ...     None, diff_text, u'a', u'b', None, u'')
+    ...     bmp, diff_text, u'a', u'b', None, u'')
     >>> transaction.commit()
-    >>> bmp = factory.makeBranchMergeProposal()
-    >>> removeSecurityProxy(bmp).preview_diff = preview_diff
     >>> url = canonical_url(bmp)
     >>> logout()
     >>> def get_review_diff():
@@ -642,17 +641,17 @@
 
 If no diff is present, nothing is shown.
 
-    >>> login('[e-mail]')
-    >>> removeSecurityProxy(bmp).preview_diff = None
-    >>> logout()
+    >>> from storm.store import Store
+    >>> Store.of(preview_diff).remove(preview_diff)
+    >>> from lp.services.propertycache import get_property_cache
+    >>> del get_property_cache(bmp).preview_diffs
     >>> print get_review_diff()
     None
 
 If the review diff is empty, then we say it is empty.
 
     >>> login('[e-mail]')
-    >>> removeSecurityProxy(bmp).preview_diff = PreviewDiff.create(
-    ...     bmp, '', u'c', u'd', None, u'')
+    >>> preview_diff = PreviewDiff.create(bmp, '', u'c', u'd', None, u'')
     >>> logout()
     >>> print extract_text(get_review_diff())
     Preview Diff
@@ -662,8 +661,7 @@
 Preview diff generation status
 ------------------------------
 
-    >>> update = find_tag_by_id(
-    ...     nopriv_browser.contents, 'diff-pending-update')
+    >>> update = find_tag_by_id(nopriv_browser.contents, 'diff-pending-update')
     >>> print extract_text(update)
     Updating diff...
     An updated diff will be available in a few minutes.  Reload to see the

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2013-05-13 06:57:37 +0000
+++ lib/lp/testing/factory.py	2013-05-13 06:57:38 +0000
@@ -1483,9 +1483,9 @@
     def makeBranchMergeProposal(self, target_branch=None, registrant=None,
                                 set_state=None, prerequisite_branch=None,
                                 product=None, initial_comment=None,
-                                source_branch=None, preview_diff=None,
-                                date_created=None, description=None,
-                                reviewer=None, merged_revno=None):
+                                source_branch=None, date_created=None,
+                                description=None, reviewer=None,
+                                merged_revno=None):
         """Create a proposal to merge based on anonymous branches."""
         if target_branch is not None:
             target_branch = removeSecurityProxy(target_branch)
@@ -1521,8 +1521,6 @@
 
         unsafe_proposal = removeSecurityProxy(proposal)
         unsafe_proposal.merged_revno = merged_revno
-        if preview_diff is not None:
-            unsafe_proposal.preview_diff = preview_diff
         if (set_state is None or
             set_state == BranchMergeProposalStatus.WORK_IN_PROGRESS):
             # The initial state is work in progress, so do nothing.
@@ -1568,17 +1566,19 @@
         return ProxyFactory(
             Diff.fromFile(StringIO(diff_text), len(diff_text)))
 
-    def makePreviewDiff(self, conflicts=u'', merge_proposal=None):
+    def makePreviewDiff(self, conflicts=u'', merge_proposal=None,
+                        date_created=None):
         diff = self.makeDiff()
         if merge_proposal is None:
             merge_proposal = self.makeBranchMergeProposal()
         preview_diff = PreviewDiff()
-        preview_diff._new_branch_merge_proposal = merge_proposal
+        preview_diff.branch_merge_proposal = merge_proposal
         preview_diff.conflicts = conflicts
         preview_diff.diff = diff
         preview_diff.source_revision_id = self.getUniqueUnicode()
         preview_diff.target_revision_id = self.getUniqueUnicode()
-        removeSecurityProxy(merge_proposal).preview_diff = preview_diff
+        if date_created:
+            preview_diff.date_created = date_created
         return preview_diff
 
     def makeIncrementalDiff(self, merge_proposal=None, old_revision=None,