← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~stevenk/launchpad/populate-previewdiff-merge_proposal into lp:launchpad

 

Steve Kowalik has proposed merging lp:~stevenk/launchpad/populate-previewdiff-merge_proposal into lp:launchpad with lp:~stevenk/launchpad/db-previewdiff-to-bmp as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/populate-previewdiff-merge_proposal/+merge/162913

Populate the new PreviewDiff.{merge_proposal,date_created} columns.
-- 
https://code.launchpad.net/~stevenk/launchpad/populate-previewdiff-merge_proposal/+merge/162913
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/populate-previewdiff-merge_proposal into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2013-03-25 23:39:42 +0000
+++ database/schema/security.cfg	2013-05-08 02:23:34 +0000
@@ -2248,6 +2248,7 @@
 public.openidconsumerassociation        = SELECT, DELETE
 public.openidconsumernonce              = SELECT, DELETE
 public.person                           = SELECT, DELETE
+public.previewdiff                      = SELECT, UPDATE
 public.product                          = SELECT, UPDATE
 public.pofiletranslator                 = SELECT, INSERT, UPDATE, DELETE
 public.potranslation                    = SELECT, DELETE

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2013-02-27 07:28:11 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2013-05-08 02:23:34 +0000
@@ -887,7 +887,7 @@
 
     def setPreviewDiff(self, preview_diff_bytes):
         preview_diff = PreviewDiff.create(
-            preview_diff_bytes, u'a', u'b', None, u'')
+            self.bmp, preview_diff_bytes, u'a', u'b', None, u'')
         removeSecurityProxy(self.bmp).preview_diff = preview_diff
         return preview_diff
 

=== modified file 'lib/lp/code/doc/branch-merge-proposal-notifications.txt'
--- lib/lp/code/doc/branch-merge-proposal-notifications.txt	2010-12-15 22:01:34 +0000
+++ lib/lp/code/doc/branch-merge-proposal-notifications.txt	2013-05-08 02:23:34 +0000
@@ -25,7 +25,8 @@
     >>> import transaction
     >>> login('test@xxxxxxxxxxxxx')
     >>> diff_bytes = ''.join(unified_diff('', 'c'))
-    >>> preview_diff = PreviewDiff.create(diff_bytes, u'a', u'b', None, None)
+    >>> preview_diff = PreviewDiff.create(
+    ...     None, diff_bytes, u'a', u'b', None, None)
     >>> # Make Librarian happy.
     >>> transaction.commit()
     >>> target_owner = factory.makePerson(email='target_owner@xxxxxxxxxxx')

=== modified file 'lib/lp/code/interfaces/diff.py'
--- lib/lp/code/interfaces/diff.py	2012-09-27 05:24:26 +0000
+++ lib/lp/code/interfaces/diff.py	2013-05-08 02:23:34 +0000
@@ -20,6 +20,7 @@
 from zope.schema import (
     Bool,
     Bytes,
+    Datetime,
     Dict,
     Int,
     Text,
@@ -114,12 +115,18 @@
         title=_('Has conflicts'), readonly=True,
         description=_('The previewed merge produces conflicts.'))
 
+    merge_proposal_id = Int(
+        title=_('The branch merge proposal for this diff.'), readonly=True)
+
     # The schema for the Reference gets patched in _schema_circular_imports.
     branch_merge_proposal = exported(
         Reference(
             Interface, readonly=True,
             title=_('The branch merge proposal that diff relates to.')))
 
+    date_created = Datetime(
+        title=_("When this diff was created."), readonly=True)
+
     stale = exported(
         Bool(readonly=True, description=_(
                 'If the preview diff is stale, it is out of date when '

=== modified file 'lib/lp/code/mail/tests/test_branchmergeproposal.py'
--- lib/lp/code/mail/tests/test_branchmergeproposal.py	2012-09-19 13:22:42 +0000
+++ lib/lp/code/mail/tests/test_branchmergeproposal.py	2013-05-08 02:23:34 +0000
@@ -52,20 +52,9 @@
     def setUp(self):
         super(TestMergeProposalMailing, self).setUp('admin@xxxxxxxxxxxxx')
 
-    def makeProposalWithSubscriber(self, diff_text=None,
-                                   initial_comment=None,
-                                   prerequisite=False,
-                                   needs_review=True,
+    def makeProposalWithSubscriber(self, diff_text=None, initial_comment=None,
+                                   prerequisite=False, needs_review=True,
                                    reviewer=None):
-        if diff_text is not None:
-            preview_diff = PreviewDiff.create(
-                diff_text,
-                unicode(self.factory.getUniqueString('revid')),
-                unicode(self.factory.getUniqueString('revid')),
-                None, None)
-            transaction.commit()
-        else:
-            preview_diff = None
         registrant = self.factory.makePerson(
             name='bazqux', displayname='Baz Qux', email='baz.qux@xxxxxxxxxxx')
         product = self.factory.makeProduct(name='super-product')
@@ -81,8 +70,14 @@
             registrant=registrant, product=product,
             set_state=initial_status,
             prerequisite_branch=prerequisite_branch,
-            preview_diff=preview_diff, initial_comment=initial_comment,
+            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')),
+                unicode(self.factory.getUniqueString('revid')), None, None)
+            transaction.commit()
         subscriber = self.factory.makePerson(displayname='Baz Quxx',
             email='baz.quxx@xxxxxxxxxxx')
         bmp.source_branch.subscribe(subscriber,

=== modified file 'lib/lp/code/mail/tests/test_codehandler.py'
--- lib/lp/code/mail/tests/test_codehandler.py	2012-06-14 05:18:22 +0000
+++ lib/lp/code/mail/tests/test_codehandler.py	2013-05-08 02:23:34 +0000
@@ -383,10 +383,9 @@
         """Requesting a review with a diff works."""
         diff_text = ''.join(unified_diff('', 'Fake diff'))
         preview_diff = PreviewDiff.create(
-            diff_text,
-            unicode(self.factory.getUniqueString('revid')),
-            unicode(self.factory.getUniqueString('revid')),
-            None, None)
+            None, diff_text,
+            unicode(self.factory.getUniqueString('revid')),
+            unicode(self.factory.getUniqueString('revid')), None, None)
         # To record the diff in the librarian.
         transaction.commit()
         bmp = make_merge_proposal_without_reviewers(

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2013-05-02 00:40:14 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2013-05-08 02:23:34 +0000
@@ -862,7 +862,7 @@
         """See `IBranchMergeProposal`."""
         # Create the PreviewDiff.
         self.preview_diff = PreviewDiff.create(
-            diff_content, source_revision_id, target_revision_id,
+            self, diff_content, source_revision_id, target_revision_id,
             prerequisite_revision_id, conflicts)
 
         # XXX: TimPenhey 2009-02-19 bug 324724

=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py	2012-12-01 07:41:41 +0000
+++ lib/lp/code/model/diff.py	2013-05-08 02:23:34 +0000
@@ -49,6 +49,8 @@
 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
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.librarian.interfaces.client import (
@@ -362,6 +364,11 @@
 
     prerequisite_revision_id = Unicode(name='dependent_revision_id')
 
+    merge_proposal_id = Int(name='merge_proposal')
+    merge_proposal = Reference(merge_proposal_id, 'BranchMergeProposal.id')
+
+    date_created = UtcDateTimeCol(dbName='date_created', default=UTC_NOW)
+
     conflicts = Unicode()
 
     @property
@@ -385,6 +392,9 @@
 
     @cachedproperty
     def branch_merge_proposal(self):
+        # This can turn into return self.merge_proposal when it's populated.
+        if self.merge_proposal:
+            return self.merge_proposal
         return self._branch_merge_proposal
 
     @classmethod
@@ -399,25 +409,29 @@
         source_revision = source_branch.last_revision()
         target_branch = bmp.target_branch.getBzrBranch()
         target_revision = target_branch.last_revision()
-        preview = cls()
-        preview.source_revision_id = source_revision.decode('utf-8')
-        preview.target_revision_id = target_revision.decode('utf-8')
         if bmp.prerequisite_branch is not None:
             prerequisite_branch = bmp.prerequisite_branch.getBzrBranch()
         else:
             prerequisite_branch = None
-        preview.diff, conflicts = Diff.mergePreviewFromBranches(
+        diff, conflicts = Diff.mergePreviewFromBranches(
             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')
+        preview.merge_proposal = bmp
+        preview.diff = diff
         preview.conflicts = u''.join(
             unicode(conflict) + '\n' for conflict in conflicts)
         return preview
 
     @classmethod
-    def create(cls, diff_content, source_revision_id, target_revision_id,
+    def create(cls, bmp, diff_content, source_revision_id, target_revision_id,
                prerequisite_revision_id, conflicts):
         """Create a PreviewDiff with specified values.
 
+        :param bmp: The `BranchMergeProposal` this diff references.
         :param diff_content: The text of the dift, as bytes.
         :param source_revision_id: The revision_id of the source branch.
         :param target_revision_id: The revision_id of the target branch.
@@ -426,15 +440,18 @@
         :param conflicts: The conflicts, as text.
         :return: A `PreviewDiff` with specified values.
         """
+        filename = str(uuid1()) + '.txt'
+        size = len(diff_content)
+        diff = Diff.fromFile(StringIO(diff_content), size, filename)
+
         preview = cls()
+        preview.merge_proposal = bmp
         preview.source_revision_id = source_revision_id
         preview.target_revision_id = target_revision_id
         preview.prerequisite_revision_id = prerequisite_revision_id
         preview.conflicts = conflicts
+        preview.diff = diff
 
-        filename = str(uuid1()) + '.txt'
-        size = len(diff_content)
-        preview.diff = Diff.fromFile(StringIO(diff_content), size, filename)
         return preview
 
     @property

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2013-02-27 07:28:11 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2013-05-08 02:23:34 +0000
@@ -2055,8 +2055,9 @@
             transaction.commit()
             target = ws_object(launchpad, mp.target_branch)
             mp = ws_object(launchpad, mp)
-        self.assertEqual([mp], list(target.getMergeProposals(
-            status=['Merged'], merged_revnos=[123])))
+        self.assertEqual(
+            [mp], list(target.getMergeProposals(
+                status=['Merged'], merged_revnos=[123])))
 
     def test_getRelatedBugTasks(self):
         """Test the getRelatedBugTasks API."""
@@ -2066,8 +2067,7 @@
         transaction.commit()
         bmp = self.wsObject(db_bmp)
         bugtask = self.wsObject(db_bug.default_bugtask)
-        self.assertEqual(
-            [bugtask], list(bmp.getRelatedBugTasks()))
+        self.assertEqual([bugtask], list(bmp.getRelatedBugTasks()))
 
     def test_setStatus_invalid_transition(self):
         """Emit BadRequest when an invalid transition is requested."""
@@ -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.branch_merge_proposal.target_branch.owner
+        user = previewdiff.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	2012-10-04 01:09:26 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt	2013-05-08 02:23:34 +0000
@@ -176,7 +176,8 @@
     >>> from lp.code.model.diff import PreviewDiff
     >>> login('foo.bar@xxxxxxxxxxxxx')
     >>> removeSecurityProxy(proposal).preview_diff = PreviewDiff.create(
-    ...   'a\n\b\n', u'source-id', u'target-id', u'prereq-id', u'conflicts')
+    ...    proposal, 'a\n\b\n', u'source-id', u'target-id', u'prereq-id',
+    ...    u'conflicts')
     >>> logout()
     >>> browser.open(url)
     >>> print_tag_with_id(browser.contents, 'proposals')

=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2013-02-27 07:28:11 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2013-05-08 02:23:34 +0000
@@ -609,7 +609,8 @@
     >>> from lp.code.model.diff import PreviewDiff
     >>> diff_text = ''.join(
     ...     unified_diff('', ['Fake Diff' + u'\u1010'.encode('utf-8')]))
-    >>> preview_diff = PreviewDiff.create(diff_text, u'a', u'b', None, u'')
+    >>> preview_diff = PreviewDiff.create(
+    ...     None, diff_text, u'a', u'b', None, u'')
     >>> transaction.commit()
     >>> bmp = factory.makeBranchMergeProposal()
     >>> removeSecurityProxy(bmp).preview_diff = preview_diff
@@ -651,7 +652,7 @@
 
     >>> login('admin@xxxxxxxxxxxxx')
     >>> removeSecurityProxy(bmp).preview_diff = PreviewDiff.create(
-    ...     '', u'c', u'd', None, u'')
+    ...     bmp, '', u'c', u'd', None, u'')
     >>> logout()
     >>> print extract_text(get_review_diff())
     Preview Diff

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2013-03-21 05:00:52 +0000
+++ lib/lp/scripts/garbo.py	2013-05-08 02:23:34 +0000
@@ -58,8 +58,13 @@
     MAX_SAMPLE_SIZE,
     )
 from lp.code.interfaces.revision import IRevisionSet
+from lp.code.model.branchmergeproposal import BranchMergeProposal
 from lp.code.model.codeimportevent import CodeImportEvent
 from lp.code.model.codeimportresult import CodeImportResult
+from lp.code.model.diff import (
+    Diff,
+    PreviewDiff,
+    )
 from lp.code.model.revision import (
     RevisionAuthor,
     RevisionCache,
@@ -101,7 +106,10 @@
 from lp.services.identity.model.account import Account
 from lp.services.identity.model.emailaddress import EmailAddress
 from lp.services.job.model.job import Job
-from lp.services.librarian.model import TimeLimitedToken
+from lp.services.librarian.model import (
+    LibraryFileAlias,
+    TimeLimitedToken,
+    )
 from lp.services.log.logger import PrefixFilter
 from lp.services.looptuner import TunableLoop
 from lp.services.oauth.model import OAuthNonce
@@ -1335,6 +1343,43 @@
         transaction.commit()
 
 
+class PopulatePreviewDiffMergeProposal(TunableLoop):
+
+    maximum_chunk_size = 5000
+
+    def __init__(self, log, abort_time=None):
+        super(PopulatePreviewDiffMergeProposal, self).__init__(log, abort_time)
+        self.start_at = 1
+        self.store = IMasterStore(BranchMergeProposal)
+
+    def findBranchMergeProposalIDs(self):
+        return self.store.find(
+            (BranchMergeProposal.id),
+            BranchMergeProposal.preview_diff_id != None,
+            BranchMergeProposal.id >= self.start_at).order_by(
+                BranchMergeProposal.id)
+
+    def isDone(self):
+        return self.findBranchMergeProposalIDs().is_empty()
+
+    def __call__(self, chunk_size):
+        bmp_ids = list(self.findBranchMergeProposalIDs()[:chunk_size])
+        columns = dict(
+            [(PreviewDiff.merge_proposal_id, BranchMergeProposal.id),
+             (PreviewDiff.date_created, LibraryFileAlias.date_created)])
+        self.store.execute(
+            BulkUpdate(
+                columns, table=PreviewDiff,
+                values=[BranchMergeProposal, Diff, LibraryFileAlias],
+                where=And(
+                    BranchMergeProposal.id.is_in(bmp_ids),
+                    PreviewDiff.diff_id == Diff.id,
+                    Diff.diff_text == LibraryFileAlias.id,
+                    PreviewDiff.id == BranchMergeProposal.preview_diff_id)))
+        self.start_at = bmp_ids[-1] + 1
+        transaction.commit()
+
+
 class BaseDatabaseGarbageCollector(LaunchpadCronScript):
     """Abstract base class to run a collection of TunableLoops."""
     script_name = None  # Script name for locking and database user. Override.
@@ -1589,6 +1634,7 @@
         UnusedSessionPruner,
         DuplicateSessionPruner,
         BugHeatUpdater,
+        PopulatePreviewDiffMergeProposal,
         ]
     experimental_tunable_loops = []
 

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2013-02-01 03:45:53 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2013-05-08 02:23:34 +0000
@@ -1167,6 +1167,21 @@
             [InformationType.PRIVATESECURITY, InformationType.PROPRIETARY],
             self.getAccessPolicyTypes(product))
 
+    def test_PopulatePreviewDiffMergeProposal(self):
+        switch_dbuser('testadmin')
+        diffs = [self.factory.makePreviewDiff() for i in range(5)]
+        expected_bmps = [diff.merge_proposal_id for diff in diffs]
+        expected_dates = [diff.diff.diff_text.date_created for diff in diffs]
+        for diff in diffs:
+            diff.merge_proposal.preview_diff = diff
+            diff.merge_proposal = None
+            diff.date_created = None
+        self.runHourly()
+        self.assertContentEqual(
+            expected_bmps, [diff.merge_proposal_id for diff in diffs])
+        self.assertEqual(
+            expected_dates, [diff.date_created for diff in diffs])
+
     def test_PopulateLatestPersonSourcePackageReleaseCache(self):
         switch_dbuser('testadmin')
         # Make some same test data - we create published source package

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2013-05-04 00:37:58 +0000
+++ lib/lp/testing/factory.py	2013-05-08 02:23:34 +0000
@@ -1573,11 +1573,12 @@
         if merge_proposal is None:
             merge_proposal = self.makeBranchMergeProposal()
         preview_diff = PreviewDiff()
-        preview_diff._branch_merge_proposal = merge_proposal
+        preview_diff.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
         return preview_diff
 
     def makeIncrementalDiff(self, merge_proposal=None, old_revision=None,