Merge lp:~abentley/launchpad/no-review-diff into lp:launchpad

[Aaron Bentley]

[Aaron Bentley (abentley)]

= Summary =
Review diffs are supplanted by preview diffs, so stop generating them.

== Proposed fix ==
This patch changes MergeProposalCreatedJob to stop generating review diffs, while still generating preview diffs.

== Pre-implementation notes ==
Pre-implementation was with thumper.

== Implementation details ==
The APIs related to review and preview diffs are somewhat different, so
some changes were necessary, such as using unicode rather than string revision ids. Also, BranchMergeProposal.preview_diff is not normally writable, so the security proxy must be removed for some tests.

== Tests ==
bin/test -vt branch-merge -t branchmerge -t test_mpcreationjobs

== Demo and Q/A ==
Not applicable; no user-visible changes.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/model/tests/test_branchmergeproposals.py
  lib/lp/code/browser/tests/test_branchmergeproposal.py
  lib/lp/code/scripts/tests/test_mp_creationjob.py
  lib/lp/code/stories/branches/xx-branch-merge-proposals.txt
  lib/lp/code/model/branchmergeproposaljob.py
  lib/lp/code/doc/branch-merge-proposal-notifications.txt

== Pylint notices ==

lib/lp/code/model/branchmergeproposaljob.py
    21: [F0401] Unable to import 'lazr.delegates' (No module named delegates)
    22: [F0401] Unable to import 'lazr.enum' (No module named enum)

[Edwin Grubbs (edwin-grubbs)]

Hi Aaron,

This branch looks good. I would just like the two docstrings changed as discussed on IRC.

merge-conditional

-Edwin

IRC Log:

[11:02 AM] <EdwinGrubbs> abentley the docstring for test_preview_diff_text_with_no_diff() says that the review_diff should be None when there is no context.preview_diff. Is that right? I don't really understand the difference between a review diff and a preview diff or whether all the objects treat them as separate.
[11:03 AM] <abentley> EdwinGrubbs: The confusion between review diffs and preview diffs was one of the reasons we got rid of review diffs. A review diff shows the changes the branch author made. A preview diff shows what would happen if you merged that branch.
[11:04 AM] <abentley> EdwinGrubbs: The review_diff field on the view class now actually refers to the preview diff, so the docstring is accurate.
[11:05 AM] <EdwinGrubbs> abentley: is that first docstring in the diff correct, or should "review_diff" be "preview_diff". In the test you are checking view.preview_diff_text.
[11:06 AM] <abentley> EdwinGrubbs: I think review_diff should be preview_diff_text.
[11:06 AM] <EdwinGrubbs> abentley: The docstring for test_preview_diff_utf8 doesn't make any sense.
[11:07 AM] <abentley> EdwinGrubbs: It means that if the bytes are utf-8-encoded, they should be decoded to unicode as utf-8.
[11:08 AM] <EdwinGrubbs> abentley: can you clarify that in the docstring?
[11:08 AM] Activating Flood Protection
[11:08 AM] Deactivating Flood Protection
[11:08 AM] <abentley> EdwinGrubbs: "A preview_diff in utf-8 should be decoded as utf-8" ?
[11:13 AM] <EdwinGrubbs> abentley: yes, but there is no reason to say utf-8 twice, can you say "decoded into a unicode object"?
[11:14 AM] <abentley> EdwinGrubbs: The reason to say it twice is because it should not be decoded using a different encoding.
[11:14 AM] <abentley> e.g. "A preview_diff in utf-8 should be decoded as utf-8, not utf-16"
[11:15 AM] <EdwinGrubbs> abentley: ok, that makes sense, since that's what you are really trying to test.
[11:18 AM] <EdwinGrubbs> abentley: in xx-branch-merge-proposals.txt, a tag with id='review-diff' is retrieved. Is that just a case where the UI hasn't yet been updated to match the name of the attribute that is now used?
[11:19 AM] <abentley> EdwinGrubbs: Yes.

review: Approve (code)

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2009-12-10 20:04:49 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2010-01-07 13:40:34 +0000
@@ -488,28 +488,26 @@
                           {'review_id': review.id})
 
     def test_preview_diff_text_with_no_diff(self):
-        """review_diff should be None when there is no context.review_diff."""
+        """preview_diff_text should be None if context has no preview_diff."""
         view = create_initialized_view(self.bmp, '+index')
         self.assertIs(None, view.preview_diff_text)
 
-    def test_review_diff_utf8(self):
-        """A review_diff in utf-8 should be converted to utf-8."""
+    def test_preview_diff_utf8(self):
+        """A preview_diff in utf-8 should decoded as utf-8."""
         text = ''.join(unichr(x) for x in range(255))
         diff_bytes = ''.join(unified_diff('', text)).encode('utf-8')
-        diff = StaticDiff.acquireFromText('x', 'y', diff_bytes)
+        self.setPreviewDiff(diff_bytes)
         transaction.commit()
-        self.bmp.review_diff = diff
         view = create_initialized_view(self.bmp, '+index')
         self.assertEqual(diff_bytes.decode('utf-8'),
                          view.preview_diff_text)
 
-    def test_review_diff_all_chars(self):
-        """review_diff should work on diffs containing all possible bytes."""
+    def test_preview_diff_all_chars(self):
+        """preview_diff should work on diffs containing all possible bytes."""
         text = ''.join(chr(x) for x in range(255))
         diff_bytes = ''.join(unified_diff('', text))
-        diff = StaticDiff.acquireFromText('x', 'y', diff_bytes)
+        self.setPreviewDiff(diff_bytes)
         transaction.commit()
-        self.bmp.review_diff = diff
         view = create_initialized_view(self.bmp, '+index')
         self.assertEqual(diff_bytes.decode('windows-1252', 'replace'),
                          view.preview_diff_text)
@@ -523,6 +521,9 @@
     def addBothDiffs(self):
         self.addReviewDiff()
         preview_diff_bytes = ''.join(unified_diff('', 'preview'))
+        return self.setPreviewDiff(preview_diff_bytes)
+
+    def setPreviewDiff(self, preview_diff_bytes):
         preview_diff = PreviewDiff.create(
             preview_diff_bytes, u'a', u'b', None, u'')
         removeSecurityProxy(self.bmp).preview_diff = preview_diff

=== modified file 'lib/lp/code/doc/branch-merge-proposal-notifications.txt'
--- lib/lp/code/doc/branch-merge-proposal-notifications.txt	2009-09-16 13:47:41 +0000
+++ lib/lp/code/doc/branch-merge-proposal-notifications.txt	2010-01-07 13:40:34 +0000
@@ -16,14 +16,11 @@
     ...     CodeReviewNotificationLevel)
     >>> from lp.code.interfaces.branchmergeproposal import (
     ...     IMergeProposalCreatedJobSource)
-    >>> from lp.code.interfaces.diff import IStaticDiffSource
     >>> from lp.code.model.diff import PreviewDiff
     >>> from lp.testing.mail_helpers import pop_notifications
     >>> import transaction
     >>> login('[e-mail]')
     >>> diff_bytes = ''.join(unified_diff('', 'c'))
-    >>> review_diff = getUtility(IStaticDiffSource).acquireFromText(
-    ...     'a', 'b', diff_bytes)
     >>> preview_diff = PreviewDiff.create(diff_bytes, u'a', u'b', None, None)
     >>> # Make Librarian happy.
     >>> transaction.commit()
@@ -104,10 +101,10 @@
     ...     displayname="Eric", email="[e-mail]")
     >>> # To avoid needing to access branches, pre-populate diffs.
     >>> bmp = source_branch.addLandingTarget(
-    ...     registrant, target_branch, review_diff=review_diff)
+    ...     registrant, target_branch)
     >>> removeSecurityProxy(bmp).preview_diff = preview_diff
     >>> [job,] = list(getUtility(IMergeProposalCreatedJobSource).iterReady())
-    >>> _unused_diff = job.run(_create_preview=False)
+    >>> job.run(_create_preview=False)
     >>> notifications = pop_notifications(
     ...     sort_key=lambda n: n.get('X-Envelope-To'))
 
@@ -156,11 +153,10 @@
     >>> bmp.deleteProposal()
     >>> bmp = source_branch.addLandingTarget(
     ...     registrant, target_branch,
-    ...     initial_comment=initial_comment, review_requests=reviewers,
-    ...     review_diff=review_diff)
+    ...     initial_comment=initial_comment, review_requests=reviewers)
     >>> removeSecurityProxy(bmp).preview_diff = preview_diff
     >>> [job,] = list(getUtility(IMergeProposalCreatedJobSource).iterReady())
-    >>> _unused_diff = job.run(_create_preview=False)
+    >>> job.run(_create_preview=False)
     >>> notifications = pop_notifications(
     ...     sort_key=lambda n: n.get('X-Envelope-To'))
     >>> for notification in notifications:

=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
--- lib/lp/code/model/branchmergeproposaljob.py	2009-12-10 18:20:09 +0000
+++ lib/lp/code/model/branchmergeproposaljob.py	2010-01-07 13:40:34 +0000
@@ -15,7 +15,7 @@
     ]
 
 import contextlib
-from email.Utils import parseaddr
+from email.utils import parseaddr
 import transaction
 
 from lazr.delegates import delegates
@@ -45,7 +45,7 @@
     )
 from lp.code.mail.branchmergeproposal import BMPMailer
 from lp.code.model.branchmergeproposal import BranchMergeProposal
-from lp.code.model.diff import PreviewDiff, StaticDiff
+from lp.code.model.diff import PreviewDiff
 from lp.codehosting.vfs import get_multi_server, get_scanner_server
 from lp.services.job.model.job import Job
 from lp.services.job.interfaces.job import IRunnableJob
@@ -223,41 +223,14 @@
     def run(self, _create_preview=True):
         """See `IMergeProposalCreatedJob`."""
         # _create_preview can be set False for testing purposes.
-        diff_created = False
-        if self.branch_merge_proposal.review_diff is None:
-            self.branch_merge_proposal.review_diff = self._makeReviewDiff()
-            diff_created = True
         if _create_preview:
             preview_diff = PreviewDiff.fromBranchMergeProposal(
                 self.branch_merge_proposal)
             self.branch_merge_proposal.preview_diff = preview_diff
-            diff_created = True
-        if diff_created:
             transaction.commit()
         mailer = BMPMailer.forCreation(
             self.branch_merge_proposal, self.branch_merge_proposal.registrant)
         mailer.sendAll()
-        return self.branch_merge_proposal.review_diff
-
-    def _makeReviewDiff(self):
-        """Return a StaticDiff to be used as a review diff."""
-        cleanups = []
-        def get_branch(branch):
-            bzr_branch = branch.getBzrBranch()
-            bzr_branch.lock_read()
-            cleanups.append(bzr_branch.unlock)
-            return bzr_branch
-        try:
-            bzr_source = get_branch(self.branch_merge_proposal.source_branch)
-            bzr_target = get_branch(self.branch_merge_proposal.target_branch)
-            lca, source_revision = self._findRevisions(
-                bzr_source, bzr_target)
-            diff = StaticDiff.acquire(
-                lca, source_revision, bzr_source.repository)
-        finally:
-            for cleanup in reversed(cleanups):
-                cleanup()
-        return diff
 
     @staticmethod
     def _findRevisions(bzr_source, bzr_target):

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
--- lib/lp/code/model/tests/test_branchmergeproposals.py	2009-12-18 15:02:34 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposals.py	2010-01-07 13:40:34 +0000
@@ -48,7 +48,6 @@
     MergeProposalCreatedJob, UpdatePreviewDiffJob)
 from lp.code.model.branchmergeproposal import (
     BranchMergeProposal, BranchMergeProposalGetter, is_valid_transition)
-from lp.code.model.diff import StaticDiff
 from lp.code.model.tests.test_diff import DiffTestCase
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
@@ -1292,50 +1291,29 @@
         job = MergeProposalCreatedJob.create(bmp)
         transaction.commit()
         self.layer.switchDbUser(config.mpcreationjobs.dbuser)
-        diff = job.run()
-        self.assertIsNot(None, diff)
-        self.assertEqual(diff, bmp.review_diff)
+        job.run()
+        self.assertIs(None, bmp.review_diff)
         self.assertIsNot(None, bmp.preview_diff)
         transaction.commit()
-        self.checkDiff(diff)
         self.checkDiff(bmp.preview_diff)
 
     def checkDiff(self, diff):
         self.assertNotIn('+bar', diff.diff.text)
         self.assertIn('+qux', diff.diff.text)
 
-    def createProposalWithEmptyBranches(self, review_diff=None):
+    def createProposalWithEmptyBranches(self):
         target_branch, tree = self.create_branch_and_tree()
         tree.commit('test')
         source_branch = self.factory.makeProductBranch(
             product=target_branch.product)
         self.createBzrBranch(source_branch, tree.branch)
         return self.factory.makeBranchMergeProposal(
-            source_branch=source_branch, target_branch=target_branch,
-            review_diff=review_diff)
-
-    def test_run_skips_diff_if_present(self):
-        """The review diff is only generated if not already assigned."""
-        # We want to make sure that we don't try to do anything with the
-        # bzr branch if there's already a diff.  So here, we create a
-        # database branch that has no bzr branch.
-        self.useBzrBranches()
-        bmp = self.createProposalWithEmptyBranches()
-        job = MergeProposalCreatedJob.create(bmp)
-        diff_bytes = ''.join(unified_diff('', 'foo'))
-        review_diff = StaticDiff.acquireFromText('rev1', 'rev2', diff_bytes)
-        transaction.commit()
-        removeSecurityProxy(bmp).review_diff = review_diff
-        job.run()
-        self.assertEqual(review_diff, bmp.review_diff)
+            source_branch=source_branch, target_branch=target_branch)
 
     def test_run_sends_email(self):
         """MergeProposalCreationJob.run sends an email."""
         self.useBzrBranches()
-        diff_bytes = ''.join(unified_diff('', 'foo'))
-        review_diff = StaticDiff.acquireFromText('rev1', 'rev2', diff_bytes)
-        transaction.commit()
-        bmp = self.createProposalWithEmptyBranches(review_diff)
+        bmp = self.createProposalWithEmptyBranches()
         job = MergeProposalCreatedJob.create(bmp)
         self.assertEqual([], pop_notifications())
         job.run()

=== modified file 'lib/lp/code/scripts/tests/test_mp_creationjob.py'
--- lib/lp/code/scripts/tests/test_mp_creationjob.py	2009-10-17 14:06:03 +0000
+++ lib/lp/code/scripts/tests/test_mp_creationjob.py	2010-01-07 13:40:34 +0000
@@ -38,7 +38,7 @@
             source_branch=source, target_branch=target,
             registrant=source.owner)
         job = MergeProposalCreatedJob.create(bmp)
-        self.assertIs(None, bmp.review_diff)
+        self.assertIs(None, bmp.preview_diff)
         transaction.commit()
         retcode, stdout, stderr = run_script(
             'cronscripts/mpcreationjobs.py', [])
@@ -47,7 +47,8 @@
         self.assertEqual(
             'INFO    creating lockfile\n'
             'INFO    Ran 1 MergeProposalCreatedJobs.\n', stderr)
-        self.assertIsNot(None, bmp.review_diff)
+        self.assertIs(None, bmp.review_diff)
+        self.assertIsNot(None, bmp.preview_diff)
 
     def test_mpcreationjobs_records_oops(self):
         """Ensure mpcreationjobs logs an oops if the job fails."""

=== modified file 'lib/lp/code/stories/branches/xx-branch-merge-proposals.txt'
--- lib/lp/code/stories/branches/xx-branch-merge-proposals.txt	2009-11-27 01:29:10 +0000
+++ lib/lp/code/stories/branches/xx-branch-merge-proposals.txt	2010-01-07 13:40:34 +0000
@@ -587,20 +587,21 @@
     NotFound: ...
 
 
-Displaying a review diff
-------------------------
+Displaying a preview diff
+-------------------------
 
-Create merge proposal with a review diff, and go to its index page.
+Create merge proposal with a preview diff, and go to its index page.
 
     >>> login('[e-mail]')
-    >>> from lp.code.model.diff import StaticDiff
+    >>> from zope.security.proxy import removeSecurityProxy
+    >>> from lp.code.model.diff import PreviewDiff
     >>> from difflib import unified_diff
     >>> diff_text = ''.join(
     ...     unified_diff('', ['Fake Diff' + u'\u1010'.encode('utf-8')]))
-    >>> review_diff = StaticDiff.acquireFromText(
-    ...     'a', 'b', diff_text)
+    >>> preview_diff = PreviewDiff.create(diff_text, u'a', u'b', None, u'')
     >>> transaction.commit()
-    >>> bmp = factory.makeBranchMergeProposal(review_diff=review_diff)
+    >>> bmp = factory.makeBranchMergeProposal()
+    >>> removeSecurityProxy(bmp).preview_diff = preview_diff
     >>> url = canonical_url(bmp)
     >>> logout()
     >>> def get_review_diff():
@@ -613,7 +614,7 @@
 
 If no diff is present, nothing is shown.
     >>> login('[e-mail]')
-    >>> bmp.review_diff = None
+    >>> removeSecurityProxy(bmp).preview_diff = None
     >>> logout()
     >>> print get_review_diff()
     None
@@ -621,7 +622,8 @@
 If the review diff is empty, then we say it is empty.
 
     >>> login('[e-mail]')
-    >>> bmp.review_diff = StaticDiff.acquireFromText('c', 'd', '')
+    >>> removeSecurityProxy(bmp).preview_diff = PreviewDiff.create(
+    ...     '', u'c', u'd', None, u'')
     >>> logout()
     >>> print extract_text(get_review_diff())
     Preview Diff