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
1=== modified file 'lib/lp/code/browser/diff.py'
2--- lib/lp/code/browser/diff.py 2009-11-18 01:53:19 +0000
3+++ lib/lp/code/browser/diff.py 2010-02-03 03:58:14 +0000
4@@ -41,7 +41,7 @@
5 """
6 diff = self._context
7 conflict_text = ''
8- if diff.conflicts is not None:
9+ if diff.has_conflicts:
10 conflict_text = _(' (has conflicts)')
11
12 count_text = ''
13@@ -74,4 +74,5 @@
14 else:
15 return (
16 '<a href="%(url)s" class="diff-link">'
17- '%(line_count)s%(count_text)s%(file_text)s%(conflict_text)s</a>' % args)
18+ '%(line_count)s%(count_text)s%(file_text)s%(conflict_text)s'
19+ '</a>' % args)
20
21=== added file 'lib/lp/code/browser/tests/test_diff.py'
22--- lib/lp/code/browser/tests/test_diff.py 1970-01-01 00:00:00 +0000
23+++ lib/lp/code/browser/tests/test_diff.py 2010-02-03 03:58:15 +0000
24@@ -0,0 +1,41 @@
25+# Copyright 2009 Canonical Ltd. This software is licensed under the
26+# GNU Affero General Public License version 3 (see the file LICENSE).
27+
28+"""Unit tests for DiffView."""
29+
30+
31+from unittest import TestLoader
32+
33+from canonical.testing import LaunchpadFunctionalLayer
34+from lp.code.browser.diff import PreviewDiffFormatterAPI
35+from lp.testing import TestCaseWithFactory
36+
37+
38+class TestFormatterAPI(TestCaseWithFactory):
39+
40+ layer = LaunchpadFunctionalLayer
41+
42+ def test_empty_conflicts(self):
43+ """'has conflicts' does not appear if conflicts is empty string."""
44+ diff = self.factory.makePreviewDiff(conflicts=u'')
45+ self.assertEqual('', diff.conflicts)
46+ formatter = PreviewDiffFormatterAPI(diff)
47+ self.assertNotIn('has conflicts', formatter.link(None))
48+
49+ def test_none_conflicts(self):
50+ """'has conflicts' does not appear if conflicts is None."""
51+ diff = self.factory.makePreviewDiff(conflicts=None)
52+ self.assertIs(None, diff.conflicts)
53+ formatter = PreviewDiffFormatterAPI(diff)
54+ self.assertNotIn('has conflicts', formatter.link(None))
55+
56+ def test_with_conflicts(self):
57+ """'has conflicts' appears if conflicts is a non-empty string."""
58+ diff = self.factory.makePreviewDiff(conflicts=u'bork')
59+ self.assertEqual('bork', diff.conflicts)
60+ formatter = PreviewDiffFormatterAPI(diff)
61+ self.assertIn('has conflicts', formatter.link(None))
62+
63+
64+def test_suite():
65+ return TestLoader().loadTestsFromName(__name__)
66
67=== modified file 'lib/lp/code/interfaces/diff.py'
68--- lib/lp/code/interfaces/diff.py 2009-10-01 13:25:12 +0000
69+++ lib/lp/code/interfaces/diff.py 2010-02-03 03:58:15 +0000
70@@ -121,6 +121,10 @@
71 'The conflicts text describing any path or text conflicts.'),
72 readonly=True))
73
74+ has_conflicts = Bool(
75+ title=_('Has conflicts'), readonly=True,
76+ description=_('The previewed merge produces conflicts.'))
77+
78 # The schema for the Reference gets patched in _schema_circular_imports.
79 branch_merge_proposal = exported(
80 Reference(
81
82=== modified file 'lib/lp/code/model/diff.py'
83--- lib/lp/code/model/diff.py 2010-01-07 13:35:25 +0000
84+++ lib/lp/code/model/diff.py 2010-02-03 03:58:14 +0000
85@@ -305,6 +305,10 @@
86
87 conflicts = Unicode()
88
89+ @property
90+ def has_conflicts(self):
91+ return self.conflicts is not None and self.conflicts != ''
92+
93 branch_merge_proposal = Reference(
94 "PreviewDiff.id", "BranchMergeProposal.preview_diff_id",
95 on_remote=True)
96
97=== modified file 'lib/lp/code/model/tests/test_diff.py'
98--- lib/lp/code/model/tests/test_diff.py 2010-01-07 20:32:06 +0000
99+++ lib/lp/code/model/tests/test_diff.py 2010-02-03 03:58:15 +0000
100@@ -376,6 +376,7 @@
101 self.assertIs(None, preview.diff_text)
102 self.assertEqual(0, preview.diff_lines_count)
103 self.assertEqual(mp, preview.branch_merge_proposal)
104+ self.assertFalse(preview.has_conflicts)
105
106 def test_stale_allInSync(self):
107 # If the revision ids of the preview diff match the source and target
108@@ -419,6 +420,20 @@
109 dep_branch.last_scanned_id = 'rev-d'
110 self.assertEqual(True, mp.preview_diff.stale)
111
112+ def test_fromPreviewDiff_with_no_conflicts(self):
113+ """Test fromPreviewDiff when no conflicts are present."""
114+ self.useBzrBranches()
115+ bmp = self.factory.makeBranchMergeProposal()
116+ bzr_target = self.createBzrBranch(bmp.target_branch)
117+ self.commitFile(bmp.target_branch, 'foo', 'a\n')
118+ self.createBzrBranch(bmp.source_branch, bzr_target)
119+ source_rev_id = self.commitFile(bmp.source_branch, 'foo', 'a\nb\n')
120+ target_rev_id = self.commitFile(bmp.target_branch, 'foo', 'c\na\n')
121+ diff = PreviewDiff.fromBranchMergeProposal(bmp)
122+ self.assertEqual('', diff.conflicts)
123+ self.assertFalse(diff.has_conflicts)
124+
125+
126 def test_fromBranchMergeProposal(self):
127 # Correctly generates a PreviewDiff from a BranchMergeProposal.
128 bmp, source_rev_id, target_rev_id = self.createExampleMerge()
129@@ -445,6 +460,7 @@
130 bmp, source_rev_id, target_rev_id = self.createExampleMerge()
131 preview = PreviewDiff.fromBranchMergeProposal(bmp)
132 self.assertEqual('Text conflict in foo\n', preview.conflicts)
133+ self.assertTrue(preview.has_conflicts)
134
135 def test_fromBranchMergeProposal_does_not_warn_on_conflicts(self):
136 """PreviewDiff generation emits no conflict warnings."""
137
138=== modified file 'lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt'
139--- lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2009-11-09 21:45:42 +0000
140+++ lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2010-02-03 03:58:15 +0000
141@@ -113,7 +113,7 @@
142 <th>Diff against target:</th>
143 <td>
144 <tal:diff replace="structure context/preview_diff/fmt:link"/>
145- <pre tal:condition="context/preview_diff/conflicts"
146+ <pre tal:condition="context/preview_diff/has_conflicts"
147 tal:content="context/preview_diff/conflicts"/>
148 </td>
149 </tr>
150
151=== modified file 'lib/lp/testing/factory.py'
152--- lib/lp/testing/factory.py 2010-01-30 05:27:48 +0000
153+++ lib/lp/testing/factory.py 2010-02-03 03:58:15 +0000
154@@ -101,6 +101,7 @@
155 from lp.code.interfaces.codeimportmachine import ICodeImportMachineSet
156 from lp.code.interfaces.codeimportresult import ICodeImportResultSet
157 from lp.code.interfaces.revision import IRevisionSet
158+from lp.code.model.diff import Diff, PreviewDiff
159 from lp.registry.model.distributionsourcepackage import (
160 DistributionSourcePackage)
161 from lp.registry.model.milestone import Milestone
162@@ -134,6 +135,23 @@
163
164 SPACE = ' '
165
166+DIFF = """\
167+=== zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/qvss.cl'
168+--- yvo/yc/pbqr/vagresnprf/qvss.cl 2009-10-01 13:25:12 +0000
169++++ yvo/yc/pbqr/vagresnprf/qvss.cl 2010-02-02 15:48:56 +0000
170+@@ -121,6 +121,10 @@
171+ 'Gur pbasyvpgf grkg qrfpevovat nal cngu be grkg pbasyvpgf.'),
172+ ernqbayl=Gehr))
173+
174++ unf_pbasyvpgf = Obby(
175++ gvgyr=_('Unf pbasyvpgf'), ernqbayl=Gehr,
176++ qrfpevcgvba=_('Gur cerivrjrq zretr cebqhprf pbasyvpgf.'))
177++
178+ # Gur fpurzn sbe gur Ersrerapr trgf cngpurq va _fpurzn_pvephyne_vzcbegf.
179+ oenapu_zretr_cebcbfny = rkcbegrq(
180+ Ersrerapr(
181+"""
182+
183
184 def default_master_store(func):
185 """Decorator to temporarily set the default Store to the master.
186@@ -216,6 +234,9 @@
187 string = "%s%s" % (prefix, self.getUniqueInteger())
188 return string.replace('_', '-').lower()
189
190+ def getUniqueUnicode(self):
191+ return self.getUniqueString().decode('latin-1')
192+
193 def getUniqueURL(self, scheme=None, host=None):
194 """Return a URL unique to this run of the test case."""
195 if scheme is None:
196@@ -917,6 +938,20 @@
197 BranchSubscriptionNotificationLevel.NOEMAIL, None,
198 CodeReviewNotificationLevel.NOEMAIL)
199
200+ def makeDiff(self, diff_text=DIFF):
201+ return Diff.fromFile(StringIO(diff_text), len(diff_text))
202+
203+ def makePreviewDiff(self, conflicts=u''):
204+ diff = self.makeDiff()
205+ bmp = self.makeBranchMergeProposal()
206+ preview_diff = PreviewDiff()
207+ preview_diff.branch_merge_proposal = bmp
208+ preview_diff.conflicts = conflicts
209+ preview_diff.diff = diff
210+ preview_diff.source_revision_id = self.getUniqueUnicode()
211+ preview_diff.target_revision_id = self.getUniqueUnicode()
212+ return preview_diff
213+
214 def makeRevision(self, author=None, revision_date=None, parent_ids=None,
215 rev_id=None, log_body=None, date_created=None):
216 """Create a single `Revision`."""