Launchpad itself

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

  1. no-review-diff
  2. Merge into devel
Proposed by Aaron Bentley
Status: Merged
Approved by: Edwin Grubbs
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~abentley/launchpad/no-review-diff
Merge into: lp:launchpad
Diff against target: 302 lines (+35/-84)
6 files modified
lib/lp/code/browser/tests/test_branchmergeproposal.py (+10/-9)
lib/lp/code/doc/branch-merge-proposal-notifications.txt (+4/-8)
lib/lp/code/model/branchmergeproposaljob.py (+2/-29)
lib/lp/code/model/tests/test_branchmergeproposals.py (+5/-27)
lib/lp/code/scripts/tests/test_mp_creationjob.py (+3/-2)
lib/lp/code/stories/branches/xx-branch-merge-proposals.txt (+11/-9)
To merge this branch: bzr merge lp:~abentley/launchpad/no-review-diff
Reviewer Review Type Date Requested Status
Edwin Grubbs (community) code Approve
Review via email: mp+16900@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) wrote :

= 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)

Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
2--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2009-12-10 20:04:49 +0000
3+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2010-01-07 13:40:34 +0000
4@@ -488,28 +488,26 @@
5 {'review_id': review.id})
6
7 def test_preview_diff_text_with_no_diff(self):
8- """review_diff should be None when there is no context.review_diff."""
9+ """preview_diff_text should be None if context has no preview_diff."""
10 view = create_initialized_view(self.bmp, '+index')
11 self.assertIs(None, view.preview_diff_text)
12
13- def test_review_diff_utf8(self):
14- """A review_diff in utf-8 should be converted to utf-8."""
15+ def test_preview_diff_utf8(self):
16+ """A preview_diff in utf-8 should decoded as utf-8."""
17 text = ''.join(unichr(x) for x in range(255))
18 diff_bytes = ''.join(unified_diff('', text)).encode('utf-8')
19- diff = StaticDiff.acquireFromText('x', 'y', diff_bytes)
20+ self.setPreviewDiff(diff_bytes)
21 transaction.commit()
22- self.bmp.review_diff = diff
23 view = create_initialized_view(self.bmp, '+index')
24 self.assertEqual(diff_bytes.decode('utf-8'),
25 view.preview_diff_text)
26
27- def test_review_diff_all_chars(self):
28- """review_diff should work on diffs containing all possible bytes."""
29+ def test_preview_diff_all_chars(self):
30+ """preview_diff should work on diffs containing all possible bytes."""
31 text = ''.join(chr(x) for x in range(255))
32 diff_bytes = ''.join(unified_diff('', text))
33- diff = StaticDiff.acquireFromText('x', 'y', diff_bytes)
34+ self.setPreviewDiff(diff_bytes)
35 transaction.commit()
36- self.bmp.review_diff = diff
37 view = create_initialized_view(self.bmp, '+index')
38 self.assertEqual(diff_bytes.decode('windows-1252', 'replace'),
39 view.preview_diff_text)
40@@ -523,6 +521,9 @@
41 def addBothDiffs(self):
42 self.addReviewDiff()
43 preview_diff_bytes = ''.join(unified_diff('', 'preview'))
44+ return self.setPreviewDiff(preview_diff_bytes)
45+
46+ def setPreviewDiff(self, preview_diff_bytes):
47 preview_diff = PreviewDiff.create(
48 preview_diff_bytes, u'a', u'b', None, u'')
49 removeSecurityProxy(self.bmp).preview_diff = preview_diff
50
51=== modified file 'lib/lp/code/doc/branch-merge-proposal-notifications.txt'
52--- lib/lp/code/doc/branch-merge-proposal-notifications.txt 2009-09-16 13:47:41 +0000
53+++ lib/lp/code/doc/branch-merge-proposal-notifications.txt 2010-01-07 13:40:34 +0000
54@@ -16,14 +16,11 @@
55 ... CodeReviewNotificationLevel)
56 >>> from lp.code.interfaces.branchmergeproposal import (
57 ... IMergeProposalCreatedJobSource)
58- >>> from lp.code.interfaces.diff import IStaticDiffSource
59 >>> from lp.code.model.diff import PreviewDiff
60 >>> from lp.testing.mail_helpers import pop_notifications
61 >>> import transaction
62 >>> login('test@canonical.com')
63 >>> diff_bytes = ''.join(unified_diff('', 'c'))
64- >>> review_diff = getUtility(IStaticDiffSource).acquireFromText(
65- ... 'a', 'b', diff_bytes)
66 >>> preview_diff = PreviewDiff.create(diff_bytes, u'a', u'b', None, None)
67 >>> # Make Librarian happy.
68 >>> transaction.commit()
69@@ -104,10 +101,10 @@
70 ... displayname="Eric", email="eric@example.com")
71 >>> # To avoid needing to access branches, pre-populate diffs.
72 >>> bmp = source_branch.addLandingTarget(
73- ... registrant, target_branch, review_diff=review_diff)
74+ ... registrant, target_branch)
75 >>> removeSecurityProxy(bmp).preview_diff = preview_diff
76 >>> [job,] = list(getUtility(IMergeProposalCreatedJobSource).iterReady())
77- >>> _unused_diff = job.run(_create_preview=False)
78+ >>> job.run(_create_preview=False)
79 >>> notifications = pop_notifications(
80 ... sort_key=lambda n: n.get('X-Envelope-To'))
81
82@@ -156,11 +153,10 @@
83 >>> bmp.deleteProposal()
84 >>> bmp = source_branch.addLandingTarget(
85 ... registrant, target_branch,
86- ... initial_comment=initial_comment, review_requests=reviewers,
87- ... review_diff=review_diff)
88+ ... initial_comment=initial_comment, review_requests=reviewers)
89 >>> removeSecurityProxy(bmp).preview_diff = preview_diff
90 >>> [job,] = list(getUtility(IMergeProposalCreatedJobSource).iterReady())
91- >>> _unused_diff = job.run(_create_preview=False)
92+ >>> job.run(_create_preview=False)
93 >>> notifications = pop_notifications(
94 ... sort_key=lambda n: n.get('X-Envelope-To'))
95 >>> for notification in notifications:
96
97=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
98--- lib/lp/code/model/branchmergeproposaljob.py 2009-12-10 18:20:09 +0000
99+++ lib/lp/code/model/branchmergeproposaljob.py 2010-01-07 13:40:34 +0000
100@@ -15,7 +15,7 @@
101 ]
102
103 import contextlib
104-from email.Utils import parseaddr
105+from email.utils import parseaddr
106 import transaction
107
108 from lazr.delegates import delegates
109@@ -45,7 +45,7 @@
110 )
111 from lp.code.mail.branchmergeproposal import BMPMailer
112 from lp.code.model.branchmergeproposal import BranchMergeProposal
113-from lp.code.model.diff import PreviewDiff, StaticDiff
114+from lp.code.model.diff import PreviewDiff
115 from lp.codehosting.vfs import get_multi_server, get_scanner_server
116 from lp.services.job.model.job import Job
117 from lp.services.job.interfaces.job import IRunnableJob
118@@ -223,41 +223,14 @@
119 def run(self, _create_preview=True):
120 """See `IMergeProposalCreatedJob`."""
121 # _create_preview can be set False for testing purposes.
122- diff_created = False
123- if self.branch_merge_proposal.review_diff is None:
124- self.branch_merge_proposal.review_diff = self._makeReviewDiff()
125- diff_created = True
126 if _create_preview:
127 preview_diff = PreviewDiff.fromBranchMergeProposal(
128 self.branch_merge_proposal)
129 self.branch_merge_proposal.preview_diff = preview_diff
130- diff_created = True
131- if diff_created:
132 transaction.commit()
133 mailer = BMPMailer.forCreation(
134 self.branch_merge_proposal, self.branch_merge_proposal.registrant)
135 mailer.sendAll()
136- return self.branch_merge_proposal.review_diff
137-
138- def _makeReviewDiff(self):
139- """Return a StaticDiff to be used as a review diff."""
140- cleanups = []
141- def get_branch(branch):
142- bzr_branch = branch.getBzrBranch()
143- bzr_branch.lock_read()
144- cleanups.append(bzr_branch.unlock)
145- return bzr_branch
146- try:
147- bzr_source = get_branch(self.branch_merge_proposal.source_branch)
148- bzr_target = get_branch(self.branch_merge_proposal.target_branch)
149- lca, source_revision = self._findRevisions(
150- bzr_source, bzr_target)
151- diff = StaticDiff.acquire(
152- lca, source_revision, bzr_source.repository)
153- finally:
154- for cleanup in reversed(cleanups):
155- cleanup()
156- return diff
157
158 @staticmethod
159 def _findRevisions(bzr_source, bzr_target):
160
161=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
162--- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-18 15:02:34 +0000
163+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2010-01-07 13:40:34 +0000
164@@ -48,7 +48,6 @@
165 MergeProposalCreatedJob, UpdatePreviewDiffJob)
166 from lp.code.model.branchmergeproposal import (
167 BranchMergeProposal, BranchMergeProposalGetter, is_valid_transition)
168-from lp.code.model.diff import StaticDiff
169 from lp.code.model.tests.test_diff import DiffTestCase
170 from lp.registry.interfaces.person import IPersonSet
171 from lp.registry.interfaces.product import IProductSet
172@@ -1292,50 +1291,29 @@
173 job = MergeProposalCreatedJob.create(bmp)
174 transaction.commit()
175 self.layer.switchDbUser(config.mpcreationjobs.dbuser)
176- diff = job.run()
177- self.assertIsNot(None, diff)
178- self.assertEqual(diff, bmp.review_diff)
179+ job.run()
180+ self.assertIs(None, bmp.review_diff)
181 self.assertIsNot(None, bmp.preview_diff)
182 transaction.commit()
183- self.checkDiff(diff)
184 self.checkDiff(bmp.preview_diff)
185
186 def checkDiff(self, diff):
187 self.assertNotIn('+bar', diff.diff.text)
188 self.assertIn('+qux', diff.diff.text)
189
190- def createProposalWithEmptyBranches(self, review_diff=None):
191+ def createProposalWithEmptyBranches(self):
192 target_branch, tree = self.create_branch_and_tree()
193 tree.commit('test')
194 source_branch = self.factory.makeProductBranch(
195 product=target_branch.product)
196 self.createBzrBranch(source_branch, tree.branch)
197 return self.factory.makeBranchMergeProposal(
198- source_branch=source_branch, target_branch=target_branch,
199- review_diff=review_diff)
200-
201- def test_run_skips_diff_if_present(self):
202- """The review diff is only generated if not already assigned."""
203- # We want to make sure that we don't try to do anything with the
204- # bzr branch if there's already a diff. So here, we create a
205- # database branch that has no bzr branch.
206- self.useBzrBranches()
207- bmp = self.createProposalWithEmptyBranches()
208- job = MergeProposalCreatedJob.create(bmp)
209- diff_bytes = ''.join(unified_diff('', 'foo'))
210- review_diff = StaticDiff.acquireFromText('rev1', 'rev2', diff_bytes)
211- transaction.commit()
212- removeSecurityProxy(bmp).review_diff = review_diff
213- job.run()
214- self.assertEqual(review_diff, bmp.review_diff)
215+ source_branch=source_branch, target_branch=target_branch)
216
217 def test_run_sends_email(self):
218 """MergeProposalCreationJob.run sends an email."""
219 self.useBzrBranches()
220- diff_bytes = ''.join(unified_diff('', 'foo'))
221- review_diff = StaticDiff.acquireFromText('rev1', 'rev2', diff_bytes)
222- transaction.commit()
223- bmp = self.createProposalWithEmptyBranches(review_diff)
224+ bmp = self.createProposalWithEmptyBranches()
225 job = MergeProposalCreatedJob.create(bmp)
226 self.assertEqual([], pop_notifications())
227 job.run()
228
229=== modified file 'lib/lp/code/scripts/tests/test_mp_creationjob.py'
230--- lib/lp/code/scripts/tests/test_mp_creationjob.py 2009-10-17 14:06:03 +0000
231+++ lib/lp/code/scripts/tests/test_mp_creationjob.py 2010-01-07 13:40:34 +0000
232@@ -38,7 +38,7 @@
233 source_branch=source, target_branch=target,
234 registrant=source.owner)
235 job = MergeProposalCreatedJob.create(bmp)
236- self.assertIs(None, bmp.review_diff)
237+ self.assertIs(None, bmp.preview_diff)
238 transaction.commit()
239 retcode, stdout, stderr = run_script(
240 'cronscripts/mpcreationjobs.py', [])
241@@ -47,7 +47,8 @@
242 self.assertEqual(
243 'INFO creating lockfile\n'
244 'INFO Ran 1 MergeProposalCreatedJobs.\n', stderr)
245- self.assertIsNot(None, bmp.review_diff)
246+ self.assertIs(None, bmp.review_diff)
247+ self.assertIsNot(None, bmp.preview_diff)
248
249 def test_mpcreationjobs_records_oops(self):
250 """Ensure mpcreationjobs logs an oops if the job fails."""
251
252=== modified file 'lib/lp/code/stories/branches/xx-branch-merge-proposals.txt'
253--- lib/lp/code/stories/branches/xx-branch-merge-proposals.txt 2009-11-27 01:29:10 +0000
254+++ lib/lp/code/stories/branches/xx-branch-merge-proposals.txt 2010-01-07 13:40:34 +0000
255@@ -587,20 +587,21 @@
256 NotFound: ...
257
258
259-Displaying a review diff
260-------------------------
261+Displaying a preview diff
262+-------------------------
263
264-Create merge proposal with a review diff, and go to its index page.
265+Create merge proposal with a preview diff, and go to its index page.
266
267 >>> login('admin@canonical.com')
268- >>> from lp.code.model.diff import StaticDiff
269+ >>> from zope.security.proxy import removeSecurityProxy
270+ >>> from lp.code.model.diff import PreviewDiff
271 >>> from difflib import unified_diff
272 >>> diff_text = ''.join(
273 ... unified_diff('', ['Fake Diff' + u'\u1010'.encode('utf-8')]))
274- >>> review_diff = StaticDiff.acquireFromText(
275- ... 'a', 'b', diff_text)
276+ >>> preview_diff = PreviewDiff.create(diff_text, u'a', u'b', None, u'')
277 >>> transaction.commit()
278- >>> bmp = factory.makeBranchMergeProposal(review_diff=review_diff)
279+ >>> bmp = factory.makeBranchMergeProposal()
280+ >>> removeSecurityProxy(bmp).preview_diff = preview_diff
281 >>> url = canonical_url(bmp)
282 >>> logout()
283 >>> def get_review_diff():
284@@ -613,7 +614,7 @@
285
286 If no diff is present, nothing is shown.
287 >>> login('admin@canonical.com')
288- >>> bmp.review_diff = None
289+ >>> removeSecurityProxy(bmp).preview_diff = None
290 >>> logout()
291 >>> print get_review_diff()
292 None
293@@ -621,7 +622,8 @@
294 If the review diff is empty, then we say it is empty.
295
296 >>> login('admin@canonical.com')
297- >>> bmp.review_diff = StaticDiff.acquireFromText('c', 'd', '')
298+ >>> removeSecurityProxy(bmp).preview_diff = PreviewDiff.create(
299+ ... '', u'c', u'd', None, u'')
300 >>> logout()
301 >>> print extract_text(get_review_diff())
302 Preview Diff