Merge lp:~abentley/launchpad/no-review-diff into lp:launchpad
- no-review-diff
- 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 | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Edwin Grubbs (community) | code | Approve | |
|
Review via email:
|
|||
Commit message
Description of the change
To post a comment you must log in.
Revision history for this message
| Aaron Bentley (abentley) wrote : | # |
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_
[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_
[11:06 AM] <abentley> EdwinGrubbs: I think review_diff should be preview_diff_text.
[11:06 AM] <EdwinGrubbs> abentley: The docstring for test_preview_
[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-
[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 | 488 | {'review_id': review.id}) | 488 | {'review_id': review.id}) |
| 6 | 489 | 489 | ||
| 7 | 490 | def test_preview_diff_text_with_no_diff(self): | 490 | def test_preview_diff_text_with_no_diff(self): |
| 9 | 491 | """review_diff should be None when there is no context.review_diff.""" | 491 | """preview_diff_text should be None if context has no preview_diff.""" |
| 10 | 492 | view = create_initialized_view(self.bmp, '+index') | 492 | view = create_initialized_view(self.bmp, '+index') |
| 11 | 493 | self.assertIs(None, view.preview_diff_text) | 493 | self.assertIs(None, view.preview_diff_text) |
| 12 | 494 | 494 | ||
| 15 | 495 | def test_review_diff_utf8(self): | 495 | def test_preview_diff_utf8(self): |
| 16 | 496 | """A review_diff in utf-8 should be converted to utf-8.""" | 496 | """A preview_diff in utf-8 should decoded as utf-8.""" |
| 17 | 497 | text = ''.join(unichr(x) for x in range(255)) | 497 | text = ''.join(unichr(x) for x in range(255)) |
| 18 | 498 | diff_bytes = ''.join(unified_diff('', text)).encode('utf-8') | 498 | diff_bytes = ''.join(unified_diff('', text)).encode('utf-8') |
| 20 | 499 | diff = StaticDiff.acquireFromText('x', 'y', diff_bytes) | 499 | self.setPreviewDiff(diff_bytes) |
| 21 | 500 | transaction.commit() | 500 | transaction.commit() |
| 22 | 501 | self.bmp.review_diff = diff | ||
| 23 | 502 | view = create_initialized_view(self.bmp, '+index') | 501 | view = create_initialized_view(self.bmp, '+index') |
| 24 | 503 | self.assertEqual(diff_bytes.decode('utf-8'), | 502 | self.assertEqual(diff_bytes.decode('utf-8'), |
| 25 | 504 | view.preview_diff_text) | 503 | view.preview_diff_text) |
| 26 | 505 | 504 | ||
| 29 | 506 | def test_review_diff_all_chars(self): | 505 | def test_preview_diff_all_chars(self): |
| 30 | 507 | """review_diff should work on diffs containing all possible bytes.""" | 506 | """preview_diff should work on diffs containing all possible bytes.""" |
| 31 | 508 | text = ''.join(chr(x) for x in range(255)) | 507 | text = ''.join(chr(x) for x in range(255)) |
| 32 | 509 | diff_bytes = ''.join(unified_diff('', text)) | 508 | diff_bytes = ''.join(unified_diff('', text)) |
| 34 | 510 | diff = StaticDiff.acquireFromText('x', 'y', diff_bytes) | 509 | self.setPreviewDiff(diff_bytes) |
| 35 | 511 | transaction.commit() | 510 | transaction.commit() |
| 36 | 512 | self.bmp.review_diff = diff | ||
| 37 | 513 | view = create_initialized_view(self.bmp, '+index') | 511 | view = create_initialized_view(self.bmp, '+index') |
| 38 | 514 | self.assertEqual(diff_bytes.decode('windows-1252', 'replace'), | 512 | self.assertEqual(diff_bytes.decode('windows-1252', 'replace'), |
| 39 | 515 | view.preview_diff_text) | 513 | view.preview_diff_text) |
| 40 | @@ -523,6 +521,9 @@ | |||
| 41 | 523 | def addBothDiffs(self): | 521 | def addBothDiffs(self): |
| 42 | 524 | self.addReviewDiff() | 522 | self.addReviewDiff() |
| 43 | 525 | preview_diff_bytes = ''.join(unified_diff('', 'preview')) | 523 | preview_diff_bytes = ''.join(unified_diff('', 'preview')) |
| 44 | 524 | return self.setPreviewDiff(preview_diff_bytes) | ||
| 45 | 525 | |||
| 46 | 526 | def setPreviewDiff(self, preview_diff_bytes): | ||
| 47 | 526 | preview_diff = PreviewDiff.create( | 527 | preview_diff = PreviewDiff.create( |
| 48 | 527 | preview_diff_bytes, u'a', u'b', None, u'') | 528 | preview_diff_bytes, u'a', u'b', None, u'') |
| 49 | 528 | removeSecurityProxy(self.bmp).preview_diff = preview_diff | 529 | removeSecurityProxy(self.bmp).preview_diff = preview_diff |
| 50 | 529 | 530 | ||
| 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 | 16 | ... CodeReviewNotificationLevel) | 16 | ... CodeReviewNotificationLevel) |
| 56 | 17 | >>> from lp.code.interfaces.branchmergeproposal import ( | 17 | >>> from lp.code.interfaces.branchmergeproposal import ( |
| 57 | 18 | ... IMergeProposalCreatedJobSource) | 18 | ... IMergeProposalCreatedJobSource) |
| 58 | 19 | >>> from lp.code.interfaces.diff import IStaticDiffSource | ||
| 59 | 20 | >>> from lp.code.model.diff import PreviewDiff | 19 | >>> from lp.code.model.diff import PreviewDiff |
| 60 | 21 | >>> from lp.testing.mail_helpers import pop_notifications | 20 | >>> from lp.testing.mail_helpers import pop_notifications |
| 61 | 22 | >>> import transaction | 21 | >>> import transaction |
| 62 | 23 | >>> login('test@canonical.com') | 22 | >>> login('test@canonical.com') |
| 63 | 24 | >>> diff_bytes = ''.join(unified_diff('', 'c')) | 23 | >>> diff_bytes = ''.join(unified_diff('', 'c')) |
| 64 | 25 | >>> review_diff = getUtility(IStaticDiffSource).acquireFromText( | ||
| 65 | 26 | ... 'a', 'b', diff_bytes) | ||
| 66 | 27 | >>> preview_diff = PreviewDiff.create(diff_bytes, u'a', u'b', None, None) | 24 | >>> preview_diff = PreviewDiff.create(diff_bytes, u'a', u'b', None, None) |
| 67 | 28 | >>> # Make Librarian happy. | 25 | >>> # Make Librarian happy. |
| 68 | 29 | >>> transaction.commit() | 26 | >>> transaction.commit() |
| 69 | @@ -104,10 +101,10 @@ | |||
| 70 | 104 | ... displayname="Eric", email="eric@example.com") | 101 | ... displayname="Eric", email="eric@example.com") |
| 71 | 105 | >>> # To avoid needing to access branches, pre-populate diffs. | 102 | >>> # To avoid needing to access branches, pre-populate diffs. |
| 72 | 106 | >>> bmp = source_branch.addLandingTarget( | 103 | >>> bmp = source_branch.addLandingTarget( |
| 74 | 107 | ... registrant, target_branch, review_diff=review_diff) | 104 | ... registrant, target_branch) |
| 75 | 108 | >>> removeSecurityProxy(bmp).preview_diff = preview_diff | 105 | >>> removeSecurityProxy(bmp).preview_diff = preview_diff |
| 76 | 109 | >>> [job,] = list(getUtility(IMergeProposalCreatedJobSource).iterReady()) | 106 | >>> [job,] = list(getUtility(IMergeProposalCreatedJobSource).iterReady()) |
| 78 | 110 | >>> _unused_diff = job.run(_create_preview=False) | 107 | >>> job.run(_create_preview=False) |
| 79 | 111 | >>> notifications = pop_notifications( | 108 | >>> notifications = pop_notifications( |
| 80 | 112 | ... sort_key=lambda n: n.get('X-Envelope-To')) | 109 | ... sort_key=lambda n: n.get('X-Envelope-To')) |
| 81 | 113 | 110 | ||
| 82 | @@ -156,11 +153,10 @@ | |||
| 83 | 156 | >>> bmp.deleteProposal() | 153 | >>> bmp.deleteProposal() |
| 84 | 157 | >>> bmp = source_branch.addLandingTarget( | 154 | >>> bmp = source_branch.addLandingTarget( |
| 85 | 158 | ... registrant, target_branch, | 155 | ... registrant, target_branch, |
| 88 | 159 | ... initial_comment=initial_comment, review_requests=reviewers, | 156 | ... initial_comment=initial_comment, review_requests=reviewers) |
| 87 | 160 | ... review_diff=review_diff) | ||
| 89 | 161 | >>> removeSecurityProxy(bmp).preview_diff = preview_diff | 157 | >>> removeSecurityProxy(bmp).preview_diff = preview_diff |
| 90 | 162 | >>> [job,] = list(getUtility(IMergeProposalCreatedJobSource).iterReady()) | 158 | >>> [job,] = list(getUtility(IMergeProposalCreatedJobSource).iterReady()) |
| 92 | 163 | >>> _unused_diff = job.run(_create_preview=False) | 159 | >>> job.run(_create_preview=False) |
| 93 | 164 | >>> notifications = pop_notifications( | 160 | >>> notifications = pop_notifications( |
| 94 | 165 | ... sort_key=lambda n: n.get('X-Envelope-To')) | 161 | ... sort_key=lambda n: n.get('X-Envelope-To')) |
| 95 | 166 | >>> for notification in notifications: | 162 | >>> for notification in notifications: |
| 96 | 167 | 163 | ||
| 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 | 15 | ] | 15 | ] |
| 102 | 16 | 16 | ||
| 103 | 17 | import contextlib | 17 | import contextlib |
| 105 | 18 | from email.Utils import parseaddr | 18 | from email.utils import parseaddr |
| 106 | 19 | import transaction | 19 | import transaction |
| 107 | 20 | 20 | ||
| 108 | 21 | from lazr.delegates import delegates | 21 | from lazr.delegates import delegates |
| 109 | @@ -45,7 +45,7 @@ | |||
| 110 | 45 | ) | 45 | ) |
| 111 | 46 | from lp.code.mail.branchmergeproposal import BMPMailer | 46 | from lp.code.mail.branchmergeproposal import BMPMailer |
| 112 | 47 | from lp.code.model.branchmergeproposal import BranchMergeProposal | 47 | from lp.code.model.branchmergeproposal import BranchMergeProposal |
| 114 | 48 | from lp.code.model.diff import PreviewDiff, StaticDiff | 48 | from lp.code.model.diff import PreviewDiff |
| 115 | 49 | from lp.codehosting.vfs import get_multi_server, get_scanner_server | 49 | from lp.codehosting.vfs import get_multi_server, get_scanner_server |
| 116 | 50 | from lp.services.job.model.job import Job | 50 | from lp.services.job.model.job import Job |
| 117 | 51 | from lp.services.job.interfaces.job import IRunnableJob | 51 | from lp.services.job.interfaces.job import IRunnableJob |
| 118 | @@ -223,41 +223,14 @@ | |||
| 119 | 223 | def run(self, _create_preview=True): | 223 | def run(self, _create_preview=True): |
| 120 | 224 | """See `IMergeProposalCreatedJob`.""" | 224 | """See `IMergeProposalCreatedJob`.""" |
| 121 | 225 | # _create_preview can be set False for testing purposes. | 225 | # _create_preview can be set False for testing purposes. |
| 122 | 226 | diff_created = False | ||
| 123 | 227 | if self.branch_merge_proposal.review_diff is None: | ||
| 124 | 228 | self.branch_merge_proposal.review_diff = self._makeReviewDiff() | ||
| 125 | 229 | diff_created = True | ||
| 126 | 230 | if _create_preview: | 226 | if _create_preview: |
| 127 | 231 | preview_diff = PreviewDiff.fromBranchMergeProposal( | 227 | preview_diff = PreviewDiff.fromBranchMergeProposal( |
| 128 | 232 | self.branch_merge_proposal) | 228 | self.branch_merge_proposal) |
| 129 | 233 | self.branch_merge_proposal.preview_diff = preview_diff | 229 | self.branch_merge_proposal.preview_diff = preview_diff |
| 130 | 234 | diff_created = True | ||
| 131 | 235 | if diff_created: | ||
| 132 | 236 | transaction.commit() | 230 | transaction.commit() |
| 133 | 237 | mailer = BMPMailer.forCreation( | 231 | mailer = BMPMailer.forCreation( |
| 134 | 238 | self.branch_merge_proposal, self.branch_merge_proposal.registrant) | 232 | self.branch_merge_proposal, self.branch_merge_proposal.registrant) |
| 135 | 239 | mailer.sendAll() | 233 | mailer.sendAll() |
| 136 | 240 | return self.branch_merge_proposal.review_diff | ||
| 137 | 241 | |||
| 138 | 242 | def _makeReviewDiff(self): | ||
| 139 | 243 | """Return a StaticDiff to be used as a review diff.""" | ||
| 140 | 244 | cleanups = [] | ||
| 141 | 245 | def get_branch(branch): | ||
| 142 | 246 | bzr_branch = branch.getBzrBranch() | ||
| 143 | 247 | bzr_branch.lock_read() | ||
| 144 | 248 | cleanups.append(bzr_branch.unlock) | ||
| 145 | 249 | return bzr_branch | ||
| 146 | 250 | try: | ||
| 147 | 251 | bzr_source = get_branch(self.branch_merge_proposal.source_branch) | ||
| 148 | 252 | bzr_target = get_branch(self.branch_merge_proposal.target_branch) | ||
| 149 | 253 | lca, source_revision = self._findRevisions( | ||
| 150 | 254 | bzr_source, bzr_target) | ||
| 151 | 255 | diff = StaticDiff.acquire( | ||
| 152 | 256 | lca, source_revision, bzr_source.repository) | ||
| 153 | 257 | finally: | ||
| 154 | 258 | for cleanup in reversed(cleanups): | ||
| 155 | 259 | cleanup() | ||
| 156 | 260 | return diff | ||
| 157 | 261 | 234 | ||
| 158 | 262 | @staticmethod | 235 | @staticmethod |
| 159 | 263 | def _findRevisions(bzr_source, bzr_target): | 236 | def _findRevisions(bzr_source, bzr_target): |
| 160 | 264 | 237 | ||
| 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 | 48 | MergeProposalCreatedJob, UpdatePreviewDiffJob) | 48 | MergeProposalCreatedJob, UpdatePreviewDiffJob) |
| 166 | 49 | from lp.code.model.branchmergeproposal import ( | 49 | from lp.code.model.branchmergeproposal import ( |
| 167 | 50 | BranchMergeProposal, BranchMergeProposalGetter, is_valid_transition) | 50 | BranchMergeProposal, BranchMergeProposalGetter, is_valid_transition) |
| 168 | 51 | from lp.code.model.diff import StaticDiff | ||
| 169 | 52 | from lp.code.model.tests.test_diff import DiffTestCase | 51 | from lp.code.model.tests.test_diff import DiffTestCase |
| 170 | 53 | from lp.registry.interfaces.person import IPersonSet | 52 | from lp.registry.interfaces.person import IPersonSet |
| 171 | 54 | from lp.registry.interfaces.product import IProductSet | 53 | from lp.registry.interfaces.product import IProductSet |
| 172 | @@ -1292,50 +1291,29 @@ | |||
| 173 | 1292 | job = MergeProposalCreatedJob.create(bmp) | 1291 | job = MergeProposalCreatedJob.create(bmp) |
| 174 | 1293 | transaction.commit() | 1292 | transaction.commit() |
| 175 | 1294 | self.layer.switchDbUser(config.mpcreationjobs.dbuser) | 1293 | self.layer.switchDbUser(config.mpcreationjobs.dbuser) |
| 179 | 1295 | diff = job.run() | 1294 | job.run() |
| 180 | 1296 | self.assertIsNot(None, diff) | 1295 | self.assertIs(None, bmp.review_diff) |
| 178 | 1297 | self.assertEqual(diff, bmp.review_diff) | ||
| 181 | 1298 | self.assertIsNot(None, bmp.preview_diff) | 1296 | self.assertIsNot(None, bmp.preview_diff) |
| 182 | 1299 | transaction.commit() | 1297 | transaction.commit() |
| 183 | 1300 | self.checkDiff(diff) | ||
| 184 | 1301 | self.checkDiff(bmp.preview_diff) | 1298 | self.checkDiff(bmp.preview_diff) |
| 185 | 1302 | 1299 | ||
| 186 | 1303 | def checkDiff(self, diff): | 1300 | def checkDiff(self, diff): |
| 187 | 1304 | self.assertNotIn('+bar', diff.diff.text) | 1301 | self.assertNotIn('+bar', diff.diff.text) |
| 188 | 1305 | self.assertIn('+qux', diff.diff.text) | 1302 | self.assertIn('+qux', diff.diff.text) |
| 189 | 1306 | 1303 | ||
| 191 | 1307 | def createProposalWithEmptyBranches(self, review_diff=None): | 1304 | def createProposalWithEmptyBranches(self): |
| 192 | 1308 | target_branch, tree = self.create_branch_and_tree() | 1305 | target_branch, tree = self.create_branch_and_tree() |
| 193 | 1309 | tree.commit('test') | 1306 | tree.commit('test') |
| 194 | 1310 | source_branch = self.factory.makeProductBranch( | 1307 | source_branch = self.factory.makeProductBranch( |
| 195 | 1311 | product=target_branch.product) | 1308 | product=target_branch.product) |
| 196 | 1312 | self.createBzrBranch(source_branch, tree.branch) | 1309 | self.createBzrBranch(source_branch, tree.branch) |
| 197 | 1313 | return self.factory.makeBranchMergeProposal( | 1310 | return self.factory.makeBranchMergeProposal( |
| 215 | 1314 | source_branch=source_branch, target_branch=target_branch, | 1311 | source_branch=source_branch, target_branch=target_branch) |
| 199 | 1315 | review_diff=review_diff) | ||
| 200 | 1316 | |||
| 201 | 1317 | def test_run_skips_diff_if_present(self): | ||
| 202 | 1318 | """The review diff is only generated if not already assigned.""" | ||
| 203 | 1319 | # We want to make sure that we don't try to do anything with the | ||
| 204 | 1320 | # bzr branch if there's already a diff. So here, we create a | ||
| 205 | 1321 | # database branch that has no bzr branch. | ||
| 206 | 1322 | self.useBzrBranches() | ||
| 207 | 1323 | bmp = self.createProposalWithEmptyBranches() | ||
| 208 | 1324 | job = MergeProposalCreatedJob.create(bmp) | ||
| 209 | 1325 | diff_bytes = ''.join(unified_diff('', 'foo')) | ||
| 210 | 1326 | review_diff = StaticDiff.acquireFromText('rev1', 'rev2', diff_bytes) | ||
| 211 | 1327 | transaction.commit() | ||
| 212 | 1328 | removeSecurityProxy(bmp).review_diff = review_diff | ||
| 213 | 1329 | job.run() | ||
| 214 | 1330 | self.assertEqual(review_diff, bmp.review_diff) | ||
| 216 | 1331 | 1312 | ||
| 217 | 1332 | def test_run_sends_email(self): | 1313 | def test_run_sends_email(self): |
| 218 | 1333 | """MergeProposalCreationJob.run sends an email.""" | 1314 | """MergeProposalCreationJob.run sends an email.""" |
| 219 | 1334 | self.useBzrBranches() | 1315 | self.useBzrBranches() |
| 224 | 1335 | diff_bytes = ''.join(unified_diff('', 'foo')) | 1316 | bmp = self.createProposalWithEmptyBranches() |
| 221 | 1336 | review_diff = StaticDiff.acquireFromText('rev1', 'rev2', diff_bytes) | ||
| 222 | 1337 | transaction.commit() | ||
| 223 | 1338 | bmp = self.createProposalWithEmptyBranches(review_diff) | ||
| 225 | 1339 | job = MergeProposalCreatedJob.create(bmp) | 1317 | job = MergeProposalCreatedJob.create(bmp) |
| 226 | 1340 | self.assertEqual([], pop_notifications()) | 1318 | self.assertEqual([], pop_notifications()) |
| 227 | 1341 | job.run() | 1319 | job.run() |
| 228 | 1342 | 1320 | ||
| 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 | 38 | source_branch=source, target_branch=target, | 38 | source_branch=source, target_branch=target, |
| 234 | 39 | registrant=source.owner) | 39 | registrant=source.owner) |
| 235 | 40 | job = MergeProposalCreatedJob.create(bmp) | 40 | job = MergeProposalCreatedJob.create(bmp) |
| 237 | 41 | self.assertIs(None, bmp.review_diff) | 41 | self.assertIs(None, bmp.preview_diff) |
| 238 | 42 | transaction.commit() | 42 | transaction.commit() |
| 239 | 43 | retcode, stdout, stderr = run_script( | 43 | retcode, stdout, stderr = run_script( |
| 240 | 44 | 'cronscripts/mpcreationjobs.py', []) | 44 | 'cronscripts/mpcreationjobs.py', []) |
| 241 | @@ -47,7 +47,8 @@ | |||
| 242 | 47 | self.assertEqual( | 47 | self.assertEqual( |
| 243 | 48 | 'INFO creating lockfile\n' | 48 | 'INFO creating lockfile\n' |
| 244 | 49 | 'INFO Ran 1 MergeProposalCreatedJobs.\n', stderr) | 49 | 'INFO Ran 1 MergeProposalCreatedJobs.\n', stderr) |
| 246 | 50 | self.assertIsNot(None, bmp.review_diff) | 50 | self.assertIs(None, bmp.review_diff) |
| 247 | 51 | self.assertIsNot(None, bmp.preview_diff) | ||
| 248 | 51 | 52 | ||
| 249 | 52 | def test_mpcreationjobs_records_oops(self): | 53 | def test_mpcreationjobs_records_oops(self): |
| 250 | 53 | """Ensure mpcreationjobs logs an oops if the job fails.""" | 54 | """Ensure mpcreationjobs logs an oops if the job fails.""" |
| 251 | 54 | 55 | ||
| 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 | 587 | NotFound: ... | 587 | NotFound: ... |
| 257 | 588 | 588 | ||
| 258 | 589 | 589 | ||
| 261 | 590 | Displaying a review diff | 590 | Displaying a preview diff |
| 262 | 591 | ------------------------ | 591 | ------------------------- |
| 263 | 592 | 592 | ||
| 265 | 593 | Create merge proposal with a review diff, and go to its index page. | 593 | Create merge proposal with a preview diff, and go to its index page. |
| 266 | 594 | 594 | ||
| 267 | 595 | >>> login('admin@canonical.com') | 595 | >>> login('admin@canonical.com') |
| 269 | 596 | >>> from lp.code.model.diff import StaticDiff | 596 | >>> from zope.security.proxy import removeSecurityProxy |
| 270 | 597 | >>> from lp.code.model.diff import PreviewDiff | ||
| 271 | 597 | >>> from difflib import unified_diff | 598 | >>> from difflib import unified_diff |
| 272 | 598 | >>> diff_text = ''.join( | 599 | >>> diff_text = ''.join( |
| 273 | 599 | ... unified_diff('', ['Fake Diff' + u'\u1010'.encode('utf-8')])) | 600 | ... unified_diff('', ['Fake Diff' + u'\u1010'.encode('utf-8')])) |
| 276 | 600 | >>> review_diff = StaticDiff.acquireFromText( | 601 | >>> preview_diff = PreviewDiff.create(diff_text, u'a', u'b', None, u'') |
| 275 | 601 | ... 'a', 'b', diff_text) | ||
| 277 | 602 | >>> transaction.commit() | 602 | >>> transaction.commit() |
| 279 | 603 | >>> bmp = factory.makeBranchMergeProposal(review_diff=review_diff) | 603 | >>> bmp = factory.makeBranchMergeProposal() |
| 280 | 604 | >>> removeSecurityProxy(bmp).preview_diff = preview_diff | ||
| 281 | 604 | >>> url = canonical_url(bmp) | 605 | >>> url = canonical_url(bmp) |
| 282 | 605 | >>> logout() | 606 | >>> logout() |
| 283 | 606 | >>> def get_review_diff(): | 607 | >>> def get_review_diff(): |
| 284 | @@ -613,7 +614,7 @@ | |||
| 285 | 613 | 614 | ||
| 286 | 614 | If no diff is present, nothing is shown. | 615 | If no diff is present, nothing is shown. |
| 287 | 615 | >>> login('admin@canonical.com') | 616 | >>> login('admin@canonical.com') |
| 289 | 616 | >>> bmp.review_diff = None | 617 | >>> removeSecurityProxy(bmp).preview_diff = None |
| 290 | 617 | >>> logout() | 618 | >>> logout() |
| 291 | 618 | >>> print get_review_diff() | 619 | >>> print get_review_diff() |
| 292 | 619 | None | 620 | None |
| 293 | @@ -621,7 +622,8 @@ | |||
| 294 | 621 | If the review diff is empty, then we say it is empty. | 622 | If the review diff is empty, then we say it is empty. |
| 295 | 622 | 623 | ||
| 296 | 623 | >>> login('admin@canonical.com') | 624 | >>> login('admin@canonical.com') |
| 298 | 624 | >>> bmp.review_diff = StaticDiff.acquireFromText('c', 'd', '') | 625 | >>> removeSecurityProxy(bmp).preview_diff = PreviewDiff.create( |
| 299 | 626 | ... '', u'c', u'd', None, u'') | ||
| 300 | 625 | >>> logout() | 627 | >>> logout() |
| 301 | 626 | >>> print extract_text(get_review_diff()) | 628 | >>> print extract_text(get_review_diff()) |
| 302 | 627 | Preview Diff | 629 | Preview Diff |
= Summary =
Review diffs are supplanted by preview diffs, so stop generating them.
== Proposed fix ==
This patch changes MergeProposalCr
== 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, BranchMergeProp
== 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/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
== Pylint notices ==
lib/lp/
21: [F0401] Unable to import 'lazr.delegates' (No module named delegates)
22: [F0401] Unable to import 'lazr.enum' (No module named enum)