Launchpad itself

Merge lp:~abentley/launchpad/empty-conflicts into lp:launchpad

  1. empty-conflicts
  2. Merge into devel
Proposed by Aaron Bentley
Status: Merged
Merged at revision: not available
Proposed branch: lp:~abentley/launchpad/empty-conflicts
Merge into: lp:launchpad
Diff against target: 216 lines (+104/-3)
7 files modified
lib/lp/code/browser/diff.py (+3/-2)
lib/lp/code/browser/tests/test_diff.py (+41/-0)
lib/lp/code/interfaces/diff.py (+4/-0)
lib/lp/code/model/diff.py (+4/-0)
lib/lp/code/model/tests/test_diff.py (+16/-0)
lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt (+1/-1)
lib/lp/testing/factory.py (+35/-0)
To merge this branch: bzr merge lp:~abentley/launchpad/empty-conflicts
Reviewer Review Type Date Requested Status
Francis J. Lacoste (community) release-critical Disapprove
Gary Poster (community) release-critical Abstain
Paul Hummer (community) code Approve
Review via email: mp+18460@code.launchpad.net

Commit message

Fix spurious conflict message in merge proposals.

To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) wrote :

= Summary =
This fixes bug #515775, which is caused by our new conflict-detection.
Unfortunately, the code in place for handling conflicts assumed only None
indicated no conflicts, but our new conflict detection (unlike MAD), uses the
empty string.

== Proposed fix ==
Update our UI to treat '' as indicating no conflicts.

== Pre-implementation notes ==
Post-implementation call with Thumper.

== Implementation details ==
This determination was previously made by the PreviewDiff's link formatter,
but is now changed to be a method on the model class.

Some lint fixes were made to other code.

== Tests ==
bin/test -vt PreviewDiff -t FormatterAPI

== Demo and Q/A ==
Create a merge proposal that has a diff with no conflicts. Observe that it
does not say "(has conflicts)".

= 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/browser/diff.py
  lib/lp/code/model/diff.py
  lib/lp/testing/factory.py
  lib/lp/code/model/tests/test_diff.py
  lib/lp/code/browser/tests/test_diff.py
  lib/lp/code/interfaces/diff.py
  lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt

== Pylint notices ==

lib/lp/code/model/diff.py
    18: [F0401] Unable to import 'lazr.delegates' (No module named delegates)
    209: [W0703, Diff.fromFile] Catch "Exception"

lib/lp/testing/factory.py
    1619: [F0401, LaunchpadObjectFactory.makeRecipe] Unable to import 'bzrlib.plugins.builder.recipe' (No module named builder)

lib/lp/code/interfaces/diff.py
    20: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    21: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)

Revision history for this message
Paul Hummer (rockstar) wrote :

On our phone call, we discussed why this issue occurred, which was mostly because the MAD interface wasn't really documented. I like that you've written the code in a manner that means the code kinda "documents" itself so we don't get into this problem again.

review: Approve (code)
Revision history for this message
Gary Poster (gary) wrote :

Sorry for not seeing this until you pointed out to me! I only have RC privs if there's no time to go to flacoste or bjornt, and I only have those privs because of my release manager role in the last release (as in, I will no longer have that privilege after the next release).

Gary

review: Abstain (release-critical)
Revision history for this message
Francis J. Lacoste (flacoste) wrote :

Although a big wart, I don't feel like this cuts the criteria for a release-critical candidate.

review: Disapprove (release-critical)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/browser/diff.py'
--- lib/lp/code/browser/diff.py 2009-11-18 01:53:19 +0000
+++ lib/lp/code/browser/diff.py 2010-02-03 03:58:14 +0000
@@ -41,7 +41,7 @@
41 """41 """
42 diff = self._context42 diff = self._context
43 conflict_text = ''43 conflict_text = ''
44 if diff.conflicts is not None:44 if diff.has_conflicts:
45 conflict_text = _(' (has conflicts)')45 conflict_text = _(' (has conflicts)')
4646
47 count_text = ''47 count_text = ''
@@ -74,4 +74,5 @@
74 else:74 else:
75 return (75 return (
76 '<a href="%(url)s" class="diff-link">'76 '<a href="%(url)s" class="diff-link">'
77 '%(line_count)s%(count_text)s%(file_text)s%(conflict_text)s</a>' % args)77 '%(line_count)s%(count_text)s%(file_text)s%(conflict_text)s'
78 '</a>' % args)
7879
=== added file 'lib/lp/code/browser/tests/test_diff.py'
--- lib/lp/code/browser/tests/test_diff.py 1970-01-01 00:00:00 +0000
+++ lib/lp/code/browser/tests/test_diff.py 2010-02-03 03:58:15 +0000
@@ -0,0 +1,41 @@
1# Copyright 2009 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Unit tests for DiffView."""
5
6
7from unittest import TestLoader
8
9from canonical.testing import LaunchpadFunctionalLayer
10from lp.code.browser.diff import PreviewDiffFormatterAPI
11from lp.testing import TestCaseWithFactory
12
13
14class TestFormatterAPI(TestCaseWithFactory):
15
16 layer = LaunchpadFunctionalLayer
17
18 def test_empty_conflicts(self):
19 """'has conflicts' does not appear if conflicts is empty string."""
20 diff = self.factory.makePreviewDiff(conflicts=u'')
21 self.assertEqual('', diff.conflicts)
22 formatter = PreviewDiffFormatterAPI(diff)
23 self.assertNotIn('has conflicts', formatter.link(None))
24
25 def test_none_conflicts(self):
26 """'has conflicts' does not appear if conflicts is None."""
27 diff = self.factory.makePreviewDiff(conflicts=None)
28 self.assertIs(None, diff.conflicts)
29 formatter = PreviewDiffFormatterAPI(diff)
30 self.assertNotIn('has conflicts', formatter.link(None))
31
32 def test_with_conflicts(self):
33 """'has conflicts' appears if conflicts is a non-empty string."""
34 diff = self.factory.makePreviewDiff(conflicts=u'bork')
35 self.assertEqual('bork', diff.conflicts)
36 formatter = PreviewDiffFormatterAPI(diff)
37 self.assertIn('has conflicts', formatter.link(None))
38
39
40def test_suite():
41 return TestLoader().loadTestsFromName(__name__)
042
=== modified file 'lib/lp/code/interfaces/diff.py'
--- lib/lp/code/interfaces/diff.py 2009-10-01 13:25:12 +0000
+++ lib/lp/code/interfaces/diff.py 2010-02-03 03:58:15 +0000
@@ -121,6 +121,10 @@
121 'The conflicts text describing any path or text conflicts.'),121 'The conflicts text describing any path or text conflicts.'),
122 readonly=True))122 readonly=True))
123123
124 has_conflicts = Bool(
125 title=_('Has conflicts'), readonly=True,
126 description=_('The previewed merge produces conflicts.'))
127
124 # The schema for the Reference gets patched in _schema_circular_imports.128 # The schema for the Reference gets patched in _schema_circular_imports.
125 branch_merge_proposal = exported(129 branch_merge_proposal = exported(
126 Reference(130 Reference(
127131
=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py 2010-01-07 13:35:25 +0000
+++ lib/lp/code/model/diff.py 2010-02-03 03:58:14 +0000
@@ -305,6 +305,10 @@
305305
306 conflicts = Unicode()306 conflicts = Unicode()
307307
308 @property
309 def has_conflicts(self):
310 return self.conflicts is not None and self.conflicts != ''
311
308 branch_merge_proposal = Reference(312 branch_merge_proposal = Reference(
309 "PreviewDiff.id", "BranchMergeProposal.preview_diff_id",313 "PreviewDiff.id", "BranchMergeProposal.preview_diff_id",
310 on_remote=True)314 on_remote=True)
311315
=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py 2010-01-07 20:32:06 +0000
+++ lib/lp/code/model/tests/test_diff.py 2010-02-03 03:58:15 +0000
@@ -376,6 +376,7 @@
376 self.assertIs(None, preview.diff_text)376 self.assertIs(None, preview.diff_text)
377 self.assertEqual(0, preview.diff_lines_count)377 self.assertEqual(0, preview.diff_lines_count)
378 self.assertEqual(mp, preview.branch_merge_proposal)378 self.assertEqual(mp, preview.branch_merge_proposal)
379 self.assertFalse(preview.has_conflicts)
379380
380 def test_stale_allInSync(self):381 def test_stale_allInSync(self):
381 # If the revision ids of the preview diff match the source and target382 # If the revision ids of the preview diff match the source and target
@@ -419,6 +420,20 @@
419 dep_branch.last_scanned_id = 'rev-d'420 dep_branch.last_scanned_id = 'rev-d'
420 self.assertEqual(True, mp.preview_diff.stale)421 self.assertEqual(True, mp.preview_diff.stale)
421422
423 def test_fromPreviewDiff_with_no_conflicts(self):
424 """Test fromPreviewDiff when no conflicts are present."""
425 self.useBzrBranches()
426 bmp = self.factory.makeBranchMergeProposal()
427 bzr_target = self.createBzrBranch(bmp.target_branch)
428 self.commitFile(bmp.target_branch, 'foo', 'a\n')
429 self.createBzrBranch(bmp.source_branch, bzr_target)
430 source_rev_id = self.commitFile(bmp.source_branch, 'foo', 'a\nb\n')
431 target_rev_id = self.commitFile(bmp.target_branch, 'foo', 'c\na\n')
432 diff = PreviewDiff.fromBranchMergeProposal(bmp)
433 self.assertEqual('', diff.conflicts)
434 self.assertFalse(diff.has_conflicts)
435
436
422 def test_fromBranchMergeProposal(self):437 def test_fromBranchMergeProposal(self):
423 # Correctly generates a PreviewDiff from a BranchMergeProposal.438 # Correctly generates a PreviewDiff from a BranchMergeProposal.
424 bmp, source_rev_id, target_rev_id = self.createExampleMerge()439 bmp, source_rev_id, target_rev_id = self.createExampleMerge()
@@ -445,6 +460,7 @@
445 bmp, source_rev_id, target_rev_id = self.createExampleMerge()460 bmp, source_rev_id, target_rev_id = self.createExampleMerge()
446 preview = PreviewDiff.fromBranchMergeProposal(bmp)461 preview = PreviewDiff.fromBranchMergeProposal(bmp)
447 self.assertEqual('Text conflict in foo\n', preview.conflicts)462 self.assertEqual('Text conflict in foo\n', preview.conflicts)
463 self.assertTrue(preview.has_conflicts)
448464
449 def test_fromBranchMergeProposal_does_not_warn_on_conflicts(self):465 def test_fromBranchMergeProposal_does_not_warn_on_conflicts(self):
450 """PreviewDiff generation emits no conflict warnings."""466 """PreviewDiff generation emits no conflict warnings."""
451467
=== modified file 'lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt'
--- lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2009-11-09 21:45:42 +0000
+++ lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2010-02-03 03:58:15 +0000
@@ -113,7 +113,7 @@
113 <th>Diff against target:</th>113 <th>Diff against target:</th>
114 <td>114 <td>
115 <tal:diff replace="structure context/preview_diff/fmt:link"/>115 <tal:diff replace="structure context/preview_diff/fmt:link"/>
116 <pre tal:condition="context/preview_diff/conflicts"116 <pre tal:condition="context/preview_diff/has_conflicts"
117 tal:content="context/preview_diff/conflicts"/>117 tal:content="context/preview_diff/conflicts"/>
118 </td>118 </td>
119 </tr>119 </tr>
120120
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-01-30 05:27:48 +0000
+++ lib/lp/testing/factory.py 2010-02-03 03:58:15 +0000
@@ -101,6 +101,7 @@
101from lp.code.interfaces.codeimportmachine import ICodeImportMachineSet101from lp.code.interfaces.codeimportmachine import ICodeImportMachineSet
102from lp.code.interfaces.codeimportresult import ICodeImportResultSet102from lp.code.interfaces.codeimportresult import ICodeImportResultSet
103from lp.code.interfaces.revision import IRevisionSet103from lp.code.interfaces.revision import IRevisionSet
104from lp.code.model.diff import Diff, PreviewDiff
104from lp.registry.model.distributionsourcepackage import (105from lp.registry.model.distributionsourcepackage import (
105 DistributionSourcePackage)106 DistributionSourcePackage)
106from lp.registry.model.milestone import Milestone107from lp.registry.model.milestone import Milestone
@@ -134,6 +135,23 @@
134135
135SPACE = ' '136SPACE = ' '
136137
138DIFF = """\
139=== zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/qvss.cl'
140--- yvo/yc/pbqr/vagresnprf/qvss.cl 2009-10-01 13:25:12 +0000
141+++ yvo/yc/pbqr/vagresnprf/qvss.cl 2010-02-02 15:48:56 +0000
142@@ -121,6 +121,10 @@
143 'Gur pbasyvpgf grkg qrfpevovat nal cngu be grkg pbasyvpgf.'),
144 ernqbayl=Gehr))
145
146+ unf_pbasyvpgf = Obby(
147+ gvgyr=_('Unf pbasyvpgf'), ernqbayl=Gehr,
148+ qrfpevcgvba=_('Gur cerivrjrq zretr cebqhprf pbasyvpgf.'))
149+
150 # Gur fpurzn sbe gur Ersrerapr trgf cngpurq va _fpurzn_pvephyne_vzcbegf.
151 oenapu_zretr_cebcbfny = rkcbegrq(
152 Ersrerapr(
153"""
154
137155
138def default_master_store(func):156def default_master_store(func):
139 """Decorator to temporarily set the default Store to the master.157 """Decorator to temporarily set the default Store to the master.
@@ -216,6 +234,9 @@
216 string = "%s%s" % (prefix, self.getUniqueInteger())234 string = "%s%s" % (prefix, self.getUniqueInteger())
217 return string.replace('_', '-').lower()235 return string.replace('_', '-').lower()
218236
237 def getUniqueUnicode(self):
238 return self.getUniqueString().decode('latin-1')
239
219 def getUniqueURL(self, scheme=None, host=None):240 def getUniqueURL(self, scheme=None, host=None):
220 """Return a URL unique to this run of the test case."""241 """Return a URL unique to this run of the test case."""
221 if scheme is None:242 if scheme is None:
@@ -917,6 +938,20 @@
917 BranchSubscriptionNotificationLevel.NOEMAIL, None,938 BranchSubscriptionNotificationLevel.NOEMAIL, None,
918 CodeReviewNotificationLevel.NOEMAIL)939 CodeReviewNotificationLevel.NOEMAIL)
919940
941 def makeDiff(self, diff_text=DIFF):
942 return Diff.fromFile(StringIO(diff_text), len(diff_text))
943
944 def makePreviewDiff(self, conflicts=u''):
945 diff = self.makeDiff()
946 bmp = self.makeBranchMergeProposal()
947 preview_diff = PreviewDiff()
948 preview_diff.branch_merge_proposal = bmp
949 preview_diff.conflicts = conflicts
950 preview_diff.diff = diff
951 preview_diff.source_revision_id = self.getUniqueUnicode()
952 preview_diff.target_revision_id = self.getUniqueUnicode()
953 return preview_diff
954
920 def makeRevision(self, author=None, revision_date=None, parent_ids=None,955 def makeRevision(self, author=None, revision_date=None, parent_ids=None,
921 rev_id=None, log_body=None, date_created=None):956 rev_id=None, log_body=None, date_created=None):
922 """Create a single `Revision`."""957 """Create a single `Revision`."""