Merge lp:~abentley/launchpad/restricted-diffs into lp:launchpad
- restricted-diffs
- Merge into devel
Proposed by
Aaron Bentley
| Status: | Merged | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Merged at revision: | not available | ||||||||
| Proposed branch: | lp:~abentley/launchpad/restricted-diffs | ||||||||
| Merge into: | lp:launchpad | ||||||||
| Diff against target: |
337 lines (+101/-21) 10 files modified
lib/canonical/launchpad/security.py (+24/-0) lib/lp/code/browser/configure.zcml (+5/-0) lib/lp/code/browser/diff.py (+10/-1) lib/lp/code/browser/tests/test_tales.py (+16/-15) lib/lp/code/configure.zcml (+3/-1) lib/lp/code/interfaces/diff.py (+3/-0) lib/lp/code/model/diff.py (+10/-2) lib/lp/code/model/tests/test_diff.py (+13/-0) lib/lp/code/stories/branches/xx-branchmergeproposals.txt (+15/-0) lib/lp/code/templates/branchmergeproposal-diff.pt (+2/-2) |
||||||||
| To merge this branch: | bzr merge lp:~abentley/launchpad/restricted-diffs | ||||||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Michael Nelson (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
| Michael Nelson (michael.nelson) wrote : | # |
Approve, just one missing blank line below, in addition to our IRC exchange.
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -10,10 +10,18 @@
>
>
> from canonical.launchpad import _
> +from canonical.
> +from canonical.
> +from canonical.
> from canonical.
> +from lp.code.
> from lp.services.
>
>
> +class PreviewDiffNavi
> +
> + usedfor = IPreviewDiff
> +
Just need an extra line here.
> class PreviewDiffForm
> """Formatter for preview diffs."""
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -614,10 +614,25 @@
> ... return find_tag_
>
> The text of the review diff is in the page.
> +
> >>> print repr(extract_
> u'Preview Diff\nDownload diff\n1\n...Fake Diff\u1010'
>
> +There is also a link to the diff URL, which is the preview diff URL plus
> +"+files/
> +
> + >>> link = get_review_
> + >>> print link['href']
> + http://
17:20 < noodles775> abentley: your branch looks great. The only question I
have is whether there's really a need to include each segment of the url in
the doctest:
http://
17:21 < abentley> noodles775, I would be fine with just showing "/+preview-
17:22 < noodles775> abentley: yeah (not that I care much either way, it'd just mean it was within 78chars :))
Thanks Aaron.
review:
Approve
(code)
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
| 1 | === modified file 'lib/canonical/launchpad/security.py' | |||
| 2 | --- lib/canonical/launchpad/security.py 2010-02-18 09:04:51 +0000 | |||
| 3 | +++ lib/canonical/launchpad/security.py 2010-02-20 04:33:20 +0000 | |||
| 4 | @@ -42,6 +42,7 @@ | |||
| 5 | 42 | ICodeReviewComment, ICodeReviewCommentDeletion) | 42 | ICodeReviewComment, ICodeReviewCommentDeletion) |
| 6 | 43 | from lp.code.interfaces.codereviewvote import ( | 43 | from lp.code.interfaces.codereviewvote import ( |
| 7 | 44 | ICodeReviewVoteReference) | 44 | ICodeReviewVoteReference) |
| 8 | 45 | from lp.code.interfaces.diff import IPreviewDiff | ||
| 9 | 45 | from lp.registry.interfaces.distribution import IDistribution | 46 | from lp.registry.interfaces.distribution import IDistribution |
| 10 | 46 | from lp.registry.interfaces.distributionmirror import ( | 47 | from lp.registry.interfaces.distributionmirror import ( |
| 11 | 47 | IDistributionMirror) | 48 | IDistributionMirror) |
| 12 | @@ -1708,6 +1709,29 @@ | |||
| 13 | 1708 | AccessBranch(self.obj.target_branch).checkUnauthenticated()) | 1709 | AccessBranch(self.obj.target_branch).checkUnauthenticated()) |
| 14 | 1709 | 1710 | ||
| 15 | 1710 | 1711 | ||
| 16 | 1712 | class PreviewDiffView(AuthorizationBase): | ||
| 17 | 1713 | permission = 'launchpad.View' | ||
| 18 | 1714 | usedfor = IPreviewDiff | ||
| 19 | 1715 | |||
| 20 | 1716 | @property | ||
| 21 | 1717 | def bmp_view(self): | ||
| 22 | 1718 | return BranchMergeProposalView(self.obj.branch_merge_proposal) | ||
| 23 | 1719 | |||
| 24 | 1720 | def checkAuthenticated(self, user): | ||
| 25 | 1721 | """Is the user able to view the preview diff? | ||
| 26 | 1722 | |||
| 27 | 1723 | The user can see a preview diff if they can see the merge proposal. | ||
| 28 | 1724 | """ | ||
| 29 | 1725 | return self.bmp_view.checkAuthenticated(user) | ||
| 30 | 1726 | |||
| 31 | 1727 | def checkUnauthenticated(self): | ||
| 32 | 1728 | """Is anyone able to view the branch merge proposal? | ||
| 33 | 1729 | |||
| 34 | 1730 | The user can see a preview diff if they can see the merge proposal. | ||
| 35 | 1731 | """ | ||
| 36 | 1732 | return self.bmp_view.checkUnauthenticated() | ||
| 37 | 1733 | |||
| 38 | 1734 | |||
| 39 | 1711 | class CodeReviewVoteReferenceEdit(AuthorizationBase): | 1735 | class CodeReviewVoteReferenceEdit(AuthorizationBase): |
| 40 | 1712 | permission = 'launchpad.Edit' | 1736 | permission = 'launchpad.Edit' |
| 41 | 1713 | usedfor = ICodeReviewVoteReference | 1737 | usedfor = ICodeReviewVoteReference |
| 42 | 1714 | 1738 | ||
| 43 | === modified file 'lib/lp/code/browser/configure.zcml' | |||
| 44 | --- lib/lp/code/browser/configure.zcml 2010-02-17 11:19:42 +0000 | |||
| 45 | +++ lib/lp/code/browser/configure.zcml 2010-02-20 04:33:20 +0000 | |||
| 46 | @@ -946,6 +946,11 @@ | |||
| 47 | 946 | attribute_to_parent="branch_merge_proposal" | 946 | attribute_to_parent="branch_merge_proposal" |
| 48 | 947 | rootsite="code" /> | 947 | rootsite="code" /> |
| 49 | 948 | 948 | ||
| 50 | 949 | <browser:navigation | ||
| 51 | 950 | module="lp.code.browser.diff" | ||
| 52 | 951 | classes=" | ||
| 53 | 952 | PreviewDiffNavigation"/> | ||
| 54 | 953 | |||
| 55 | 949 | <browser:page | 954 | <browser:page |
| 56 | 950 | name="+diff" | 955 | name="+diff" |
| 57 | 951 | for="lp.code.interfaces.diff.IPreviewDiff" | 956 | for="lp.code.interfaces.diff.IPreviewDiff" |
| 58 | 952 | 957 | ||
| 59 | === modified file 'lib/lp/code/browser/diff.py' | |||
| 60 | --- lib/lp/code/browser/diff.py 2010-02-02 17:32:23 +0000 | |||
| 61 | +++ lib/lp/code/browser/diff.py 2010-02-20 04:33:20 +0000 | |||
| 62 | @@ -10,10 +10,19 @@ | |||
| 63 | 10 | 10 | ||
| 64 | 11 | 11 | ||
| 65 | 12 | from canonical.launchpad import _ | 12 | from canonical.launchpad import _ |
| 66 | 13 | from canonical.launchpad.browser.librarian import FileNavigationMixin | ||
| 67 | 14 | from canonical.launchpad.webapp import Navigation | ||
| 68 | 15 | from canonical.launchpad.webapp.publisher import canonical_url | ||
| 69 | 13 | from canonical.launchpad.webapp.tales import ObjectFormatterAPI | 16 | from canonical.launchpad.webapp.tales import ObjectFormatterAPI |
| 70 | 17 | from lp.code.interfaces.diff import IPreviewDiff | ||
| 71 | 14 | from lp.services.browser_helpers import get_plural_text | 18 | from lp.services.browser_helpers import get_plural_text |
| 72 | 15 | 19 | ||
| 73 | 16 | 20 | ||
| 74 | 21 | class PreviewDiffNavigation(Navigation, FileNavigationMixin): | ||
| 75 | 22 | |||
| 76 | 23 | usedfor = IPreviewDiff | ||
| 77 | 24 | |||
| 78 | 25 | |||
| 79 | 17 | class PreviewDiffFormatterAPI(ObjectFormatterAPI): | 26 | class PreviewDiffFormatterAPI(ObjectFormatterAPI): |
| 80 | 18 | """Formatter for preview diffs.""" | 27 | """Formatter for preview diffs.""" |
| 81 | 19 | 28 | ||
| 82 | @@ -24,7 +33,7 @@ | |||
| 83 | 24 | if librarian_alias is None: | 33 | if librarian_alias is None: |
| 84 | 25 | return None | 34 | return None |
| 85 | 26 | else: | 35 | else: |
| 87 | 27 | return librarian_alias.getURL() | 36 | return canonical_url(self._context) + '/+files/preview.diff' |
| 88 | 28 | 37 | ||
| 89 | 29 | def link(self, view_name): | 38 | def link(self, view_name): |
| 90 | 30 | """The link to the diff should show the line count. | 39 | """The link to the diff should show the line count. |
| 91 | 31 | 40 | ||
| 92 | === modified file 'lib/lp/code/browser/tests/test_tales.py' | |||
| 93 | --- lib/lp/code/browser/tests/test_tales.py 2009-11-18 01:53:19 +0000 | |||
| 94 | +++ lib/lp/code/browser/tests/test_tales.py 2010-02-20 04:33:20 +0000 | |||
| 95 | @@ -11,8 +11,9 @@ | |||
| 96 | 11 | from storm.store import Store | 11 | from storm.store import Store |
| 97 | 12 | from zope.security.proxy import removeSecurityProxy | 12 | from zope.security.proxy import removeSecurityProxy |
| 98 | 13 | 13 | ||
| 99 | 14 | from canonical.launchpad.webapp.publisher import canonical_url | ||
| 100 | 15 | from canonical.testing import LaunchpadFunctionalLayer | ||
| 101 | 14 | from lp.testing import login, TestCaseWithFactory, test_tales | 16 | from lp.testing import login, TestCaseWithFactory, test_tales |
| 102 | 15 | from canonical.testing import LaunchpadFunctionalLayer | ||
| 103 | 16 | 17 | ||
| 104 | 17 | 18 | ||
| 105 | 18 | class TestPreviewDiffFormatter(TestCaseWithFactory): | 19 | class TestPreviewDiffFormatter(TestCaseWithFactory): |
| 106 | @@ -79,18 +80,18 @@ | |||
| 107 | 79 | # If the lines added and removed are None, they are now shown. | 80 | # If the lines added and removed are None, they are now shown. |
| 108 | 80 | preview = self._createPreviewDiff(10, added=None, removed=None) | 81 | preview = self._createPreviewDiff(10, added=None, removed=None) |
| 109 | 81 | self.assertEqual( | 82 | self.assertEqual( |
| 111 | 82 | '<a href="%s" class="diff-link">' | 83 | '<a href="%s/+files/preview.diff" class="diff-link">' |
| 112 | 83 | '10 lines</a>' | 84 | '10 lines</a>' |
| 114 | 84 | % preview.diff_text.getURL(), | 85 | % canonical_url(preview), |
| 115 | 85 | test_tales('preview/fmt:link', preview=preview)) | 86 | test_tales('preview/fmt:link', preview=preview)) |
| 116 | 86 | 87 | ||
| 117 | 87 | def test_fmt_lines_some_added_no_removed(self): | 88 | def test_fmt_lines_some_added_no_removed(self): |
| 118 | 88 | # If the added and removed values are not None, they are shown. | 89 | # If the added and removed values are not None, they are shown. |
| 119 | 89 | preview = self._createPreviewDiff(10, added=4, removed=0) | 90 | preview = self._createPreviewDiff(10, added=4, removed=0) |
| 120 | 90 | self.assertEqual( | 91 | self.assertEqual( |
| 122 | 91 | '<a href="%s" class="diff-link">' | 92 | '<a href="%s/+files/preview.diff" class="diff-link">' |
| 123 | 92 | '10 lines (+4/-0)</a>' | 93 | '10 lines (+4/-0)</a>' |
| 125 | 93 | % preview.diff_text.getURL(), | 94 | % canonical_url(preview), |
| 126 | 94 | test_tales('preview/fmt:link', preview=preview)) | 95 | test_tales('preview/fmt:link', preview=preview)) |
| 127 | 95 | 96 | ||
| 128 | 96 | def test_fmt_lines_files_modified(self): | 97 | def test_fmt_lines_files_modified(self): |
| 129 | @@ -101,9 +102,9 @@ | |||
| 130 | 101 | 'file1': (1, 0), | 102 | 'file1': (1, 0), |
| 131 | 102 | 'file2': (3, 0)}) | 103 | 'file2': (3, 0)}) |
| 132 | 103 | self.assertEqual( | 104 | self.assertEqual( |
| 134 | 104 | '<a href="%s" class="diff-link">' | 105 | '<a href="%s/+files/preview.diff" class="diff-link">' |
| 135 | 105 | '10 lines (+4/-0) 2 files modified</a>' | 106 | '10 lines (+4/-0) 2 files modified</a>' |
| 137 | 106 | % preview.diff_text.getURL(), | 107 | % canonical_url(preview), |
| 138 | 107 | test_tales('preview/fmt:link', preview=preview)) | 108 | test_tales('preview/fmt:link', preview=preview)) |
| 139 | 108 | 109 | ||
| 140 | 109 | def test_fmt_lines_one_file_modified(self): | 110 | def test_fmt_lines_one_file_modified(self): |
| 141 | @@ -112,18 +113,18 @@ | |||
| 142 | 112 | 10, added=4, removed=0, diffstat={ | 113 | 10, added=4, removed=0, diffstat={ |
| 143 | 113 | 'file': (3, 0)}) | 114 | 'file': (3, 0)}) |
| 144 | 114 | self.assertEqual( | 115 | self.assertEqual( |
| 146 | 115 | '<a href="%s" class="diff-link">' | 116 | '<a href="%s/+files/preview.diff" class="diff-link">' |
| 147 | 116 | '10 lines (+4/-0) 1 file modified</a>' | 117 | '10 lines (+4/-0) 1 file modified</a>' |
| 149 | 117 | % preview.diff_text.getURL(), | 118 | % canonical_url(preview), |
| 150 | 118 | test_tales('preview/fmt:link', preview=preview)) | 119 | test_tales('preview/fmt:link', preview=preview)) |
| 151 | 119 | 120 | ||
| 152 | 120 | def test_fmt_simple_conflicts(self): | 121 | def test_fmt_simple_conflicts(self): |
| 153 | 121 | # Conflicts are indicated using text in the link. | 122 | # Conflicts are indicated using text in the link. |
| 154 | 122 | preview = self._createPreviewDiff(10, 2, 3, u'conflicts') | 123 | preview = self._createPreviewDiff(10, 2, 3, u'conflicts') |
| 155 | 123 | self.assertEqual( | 124 | self.assertEqual( |
| 157 | 124 | '<a href="%s" class="diff-link">' | 125 | '<a href="%s/+files/preview.diff" class="diff-link">' |
| 158 | 125 | '10 lines (+2/-3) (has conflicts)</a>' | 126 | '10 lines (+2/-3) (has conflicts)</a>' |
| 160 | 126 | % preview.diff_text.getURL(), | 127 | % canonical_url(preview), |
| 161 | 127 | test_tales('preview/fmt:link', preview=preview)) | 128 | test_tales('preview/fmt:link', preview=preview)) |
| 162 | 128 | 129 | ||
| 163 | 129 | def test_fmt_stale_empty_diff(self): | 130 | def test_fmt_stale_empty_diff(self): |
| 164 | @@ -140,9 +141,9 @@ | |||
| 165 | 140 | preview = self._createStalePreviewDiff( | 141 | preview = self._createStalePreviewDiff( |
| 166 | 141 | 500, 89, 340, diffstat=diffstat) | 142 | 500, 89, 340, diffstat=diffstat) |
| 167 | 142 | self.assertEqual( | 143 | self.assertEqual( |
| 169 | 143 | '<a href="%s" class="diff-link">' | 144 | '<a href="%s/+files/preview.diff" class="diff-link">' |
| 170 | 144 | '500 lines (+89/-340) 23 files modified</a>' | 145 | '500 lines (+89/-340) 23 files modified</a>' |
| 172 | 145 | % preview.diff_text.getURL(), | 146 | % canonical_url(preview), |
| 173 | 146 | test_tales('preview/fmt:link', preview=preview)) | 147 | test_tales('preview/fmt:link', preview=preview)) |
| 174 | 147 | 148 | ||
| 175 | 148 | def test_fmt_stale_non_empty_diff_with_conflicts(self): | 149 | def test_fmt_stale_non_empty_diff_with_conflicts(self): |
| 176 | @@ -152,9 +153,9 @@ | |||
| 177 | 152 | preview = self._createStalePreviewDiff( | 153 | preview = self._createStalePreviewDiff( |
| 178 | 153 | 500, 89, 340, u'conflicts', diffstat=diffstat) | 154 | 500, 89, 340, u'conflicts', diffstat=diffstat) |
| 179 | 154 | self.assertEqual( | 155 | self.assertEqual( |
| 181 | 155 | '<a href="%s" class="diff-link">' | 156 | '<a href="%s/+files/preview.diff" class="diff-link">' |
| 182 | 156 | '500 lines (+89/-340) 23 files modified (has conflicts)</a>' | 157 | '500 lines (+89/-340) 23 files modified (has conflicts)</a>' |
| 184 | 157 | % preview.diff_text.getURL(), | 158 | % canonical_url(preview), |
| 185 | 158 | test_tales('preview/fmt:link', preview=preview)) | 159 | test_tales('preview/fmt:link', preview=preview)) |
| 186 | 159 | 160 | ||
| 187 | 160 | 161 | ||
| 188 | 161 | 162 | ||
| 189 | === modified file 'lib/lp/code/configure.zcml' | |||
| 190 | --- lib/lp/code/configure.zcml 2010-02-18 16:00:24 +0000 | |||
| 191 | +++ lib/lp/code/configure.zcml 2010-02-20 04:33:20 +0000 | |||
| 192 | @@ -929,7 +929,9 @@ | |||
| 193 | 929 | <allow interface="lp.code.interfaces.diff.IStaticDiff" /> | 929 | <allow interface="lp.code.interfaces.diff.IStaticDiff" /> |
| 194 | 930 | </class> | 930 | </class> |
| 195 | 931 | <class class="lp.code.model.diff.PreviewDiff"> | 931 | <class class="lp.code.model.diff.PreviewDiff"> |
| 197 | 932 | <allow interface="lp.code.interfaces.diff.IPreviewDiff" /> | 932 | <require |
| 198 | 933 | permission="launchpad.View" | ||
| 199 | 934 | interface="lp.code.interfaces.diff.IPreviewDiff"/> | ||
| 200 | 933 | </class> | 935 | </class> |
| 201 | 934 | <securedutility | 936 | <securedutility |
| 202 | 935 | component="lp.code.model.diff.StaticDiff" | 937 | component="lp.code.model.diff.StaticDiff" |
| 203 | 936 | 938 | ||
| 204 | === modified file 'lib/lp/code/interfaces/diff.py' | |||
| 205 | --- lib/lp/code/interfaces/diff.py 2010-02-02 17:19:05 +0000 | |||
| 206 | +++ lib/lp/code/interfaces/diff.py 2010-02-20 04:33:20 +0000 | |||
| 207 | @@ -136,3 +136,6 @@ | |||
| 208 | 136 | 'If the preview diff is stale, it is out of date when ' | 136 | 'If the preview diff is stale, it is out of date when ' |
| 209 | 137 | 'compared to the tip revisions of the source, target, and ' | 137 | 'compared to the tip revisions of the source, target, and ' |
| 210 | 138 | 'possibly prerequisite branches.'))) | 138 | 'possibly prerequisite branches.'))) |
| 211 | 139 | |||
| 212 | 140 | def getFileByName(filename): | ||
| 213 | 141 | """Return the file under +files with specified name.""" | ||
| 214 | 139 | 142 | ||
| 215 | === modified file 'lib/lp/code/model/diff.py' | |||
| 216 | --- lib/lp/code/model/diff.py 2010-02-02 17:19:05 +0000 | |||
| 217 | +++ lib/lp/code/model/diff.py 2010-02-20 04:33:20 +0000 | |||
| 218 | @@ -27,9 +27,10 @@ | |||
| 219 | 27 | from canonical.database.sqlbase import SQLBase | 27 | from canonical.database.sqlbase import SQLBase |
| 220 | 28 | from canonical.uuid import generate_uuid | 28 | from canonical.uuid import generate_uuid |
| 221 | 29 | 29 | ||
| 222 | 30 | from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet | ||
| 223 | 31 | from canonical.launchpad.interfaces.launchpad import NotFoundError | ||
| 224 | 30 | from lp.code.interfaces.diff import ( | 32 | from lp.code.interfaces.diff import ( |
| 225 | 31 | IDiff, IPreviewDiff, IStaticDiff, IStaticDiffSource) | 33 | IDiff, IPreviewDiff, IStaticDiff, IStaticDiffSource) |
| 226 | 32 | from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet | ||
| 227 | 33 | 34 | ||
| 228 | 34 | 35 | ||
| 229 | 35 | class Diff(SQLBase): | 36 | class Diff(SQLBase): |
| 230 | @@ -195,7 +196,7 @@ | |||
| 231 | 195 | if filename is None: | 196 | if filename is None: |
| 232 | 196 | filename = generate_uuid() + '.txt' | 197 | filename = generate_uuid() + '.txt' |
| 233 | 197 | diff_text = getUtility(ILibraryFileAliasSet).create( | 198 | diff_text = getUtility(ILibraryFileAliasSet).create( |
| 235 | 198 | filename, size, diff_content, 'text/x-diff') | 199 | filename, size, diff_content, 'text/x-diff', restricted=True) |
| 236 | 199 | diff_content.seek(0) | 200 | diff_content.seek(0) |
| 237 | 200 | diff_content_bytes = diff_content.read(size) | 201 | diff_content_bytes = diff_content.read(size) |
| 238 | 201 | diff_lines_count = len(diff_content_bytes.strip().split('\n')) | 202 | diff_lines_count = len(diff_content_bytes.strip().split('\n')) |
| 239 | @@ -383,3 +384,10 @@ | |||
| 240 | 383 | return True | 384 | return True |
| 241 | 384 | else: | 385 | else: |
| 242 | 385 | return False | 386 | return False |
| 243 | 387 | |||
| 244 | 388 | def getFileByName(self, filename): | ||
| 245 | 389 | """See `IPreviewDiff`.""" | ||
| 246 | 390 | if filename == 'preview.diff' and self.diff_text is not None: | ||
| 247 | 391 | return self.diff_text | ||
| 248 | 392 | else: | ||
| 249 | 393 | raise NotFoundError(filename) | ||
| 250 | 386 | 394 | ||
| 251 | === modified file 'lib/lp/code/model/tests/test_diff.py' | |||
| 252 | --- lib/lp/code/model/tests/test_diff.py 2010-02-02 17:19:05 +0000 | |||
| 253 | +++ lib/lp/code/model/tests/test_diff.py 2010-02-20 04:33:20 +0000 | |||
| 254 | @@ -15,6 +15,7 @@ | |||
| 255 | 15 | from bzrlib import trace | 15 | from bzrlib import trace |
| 256 | 16 | import transaction | 16 | import transaction |
| 257 | 17 | 17 | ||
| 258 | 18 | from canonical.launchpad.interfaces.launchpad import NotFoundError | ||
| 259 | 18 | from canonical.launchpad.webapp import canonical_url, errorlog | 19 | from canonical.launchpad.webapp import canonical_url, errorlog |
| 260 | 19 | from canonical.launchpad.webapp.testing import verifyObject | 20 | from canonical.launchpad.webapp.testing import verifyObject |
| 261 | 20 | from canonical.testing import LaunchpadFunctionalLayer, LaunchpadZopelessLayer | 21 | from canonical.testing import LaunchpadFunctionalLayer, LaunchpadZopelessLayer |
| 262 | @@ -123,6 +124,7 @@ | |||
| 263 | 123 | content = ''.join(unified_diff('', "1234567890" * 10)) | 124 | content = ''.join(unified_diff('', "1234567890" * 10)) |
| 264 | 124 | diff = self._create_diff(content) | 125 | diff = self._create_diff(content) |
| 265 | 125 | self.assertEqual(content, diff.text) | 126 | self.assertEqual(content, diff.text) |
| 266 | 127 | self.assertTrue(diff.diff_text.restricted) | ||
| 267 | 126 | 128 | ||
| 268 | 127 | def test_oversized_normal(self): | 129 | def test_oversized_normal(self): |
| 269 | 128 | # A diff smaller than config.diff.max_read_size is not oversized. | 130 | # A diff smaller than config.diff.max_read_size is not oversized. |
| 270 | @@ -478,6 +480,17 @@ | |||
| 271 | 478 | finally: | 480 | finally: |
| 272 | 479 | logger.removeHandler(handler) | 481 | logger.removeHandler(handler) |
| 273 | 480 | 482 | ||
| 274 | 483 | def test_getFileByName(self): | ||
| 275 | 484 | diff = self._createProposalWithPreviewDiff().preview_diff | ||
| 276 | 485 | self.assertEqual(diff.diff_text, diff.getFileByName('preview.diff')) | ||
| 277 | 486 | self.assertRaises( | ||
| 278 | 487 | NotFoundError, diff.getFileByName, 'different.name') | ||
| 279 | 488 | |||
| 280 | 489 | def test_getFileByName_with_no_diff(self): | ||
| 281 | 490 | diff = self._createProposalWithPreviewDiff(content='').preview_diff | ||
| 282 | 491 | self.assertRaises( | ||
| 283 | 492 | NotFoundError, diff.getFileByName, 'preview.diff') | ||
| 284 | 493 | |||
| 285 | 481 | 494 | ||
| 286 | 482 | def test_suite(): | 495 | def test_suite(): |
| 287 | 483 | return TestLoader().loadTestsFromName(__name__) | 496 | return TestLoader().loadTestsFromName(__name__) |
| 288 | 484 | 497 | ||
| 289 | === modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt' | |||
| 290 | --- lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2010-01-12 17:34:12 +0000 | |||
| 291 | +++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2010-02-20 04:33:20 +0000 | |||
| 292 | @@ -614,10 +614,25 @@ | |||
| 293 | 614 | ... return find_tag_by_id(nopriv_browser.contents, 'review-diff') | 614 | ... return find_tag_by_id(nopriv_browser.contents, 'review-diff') |
| 294 | 615 | 615 | ||
| 295 | 616 | The text of the review diff is in the page. | 616 | The text of the review diff is in the page. |
| 296 | 617 | |||
| 297 | 617 | >>> print repr(extract_text(get_review_diff())) | 618 | >>> print repr(extract_text(get_review_diff())) |
| 298 | 618 | u'Preview Diff\nDownload diff\n1\n...Fake Diff\u1010' | 619 | u'Preview Diff\nDownload diff\n1\n...Fake Diff\u1010' |
| 299 | 619 | 620 | ||
| 300 | 621 | There is also a link to the diff URL, which is the preview diff URL plus | ||
| 301 | 622 | "+files/preview.diff" | ||
| 302 | 623 | |||
| 303 | 624 | >>> link = get_review_diff().find('a') | ||
| 304 | 625 | >>> print link['href'] | ||
| 305 | 626 | http.../+preview-diff/+files/preview.diff | ||
| 306 | 627 | >>> nopriv_browser.open(str(link['href'])) | ||
| 307 | 628 | >>> print nopriv_browser.contents | ||
| 308 | 629 | --- | ||
| 309 | 630 | +++ | ||
| 310 | 631 | @@ -1,0 +1,1 @@ | ||
| 311 | 632 | +Fake Diff... | ||
| 312 | 633 | |||
| 313 | 620 | If no diff is present, nothing is shown. | 634 | If no diff is present, nothing is shown. |
| 314 | 635 | |||
| 315 | 621 | >>> login('admin@canonical.com') | 636 | >>> login('admin@canonical.com') |
| 316 | 622 | >>> removeSecurityProxy(bmp).preview_diff = None | 637 | >>> removeSecurityProxy(bmp).preview_diff = None |
| 317 | 623 | >>> logout() | 638 | >>> logout() |
| 318 | 624 | 639 | ||
| 319 | === modified file 'lib/lp/code/templates/branchmergeproposal-diff.pt' | |||
| 320 | --- lib/lp/code/templates/branchmergeproposal-diff.pt 2009-11-14 09:36:50 +0000 | |||
| 321 | +++ lib/lp/code/templates/branchmergeproposal-diff.pt 2010-02-20 04:33:20 +0000 | |||
| 322 | @@ -8,7 +8,7 @@ | |||
| 323 | 8 | <div class="boardCommentDetails filename"> | 8 | <div class="boardCommentDetails filename"> |
| 324 | 9 | <ul class="horizontal"> | 9 | <ul class="horizontal"> |
| 325 | 10 | <li> | 10 | <li> |
| 327 | 11 | <a tal:attributes="href attachment/getURL"> | 11 | <a tal:attributes="href view/preview_diff/fmt:url"> |
| 328 | 12 | <img src="/@@/download"/> | 12 | <img src="/@@/download"/> |
| 329 | 13 | Download diff | 13 | Download diff |
| 330 | 14 | </a> | 14 | </a> |
| 331 | @@ -30,4 +30,4 @@ | |||
| 332 | 30 | </tal:empty-diff> | 30 | </tal:empty-diff> |
| 333 | 31 | </div> | 31 | </div> |
| 334 | 32 | 32 | ||
| 335 | 33 | </tal:root> | ||
| 336 | 34 | \ No newline at end of file | 33 | \ No newline at end of file |
| 337 | 34 | </tal:root> | ||
= Summary =
Fix bug #328271, "Use the restricted librarian for diffs for private branches".
== Proposed fix ==
Use the restricted librarian for diffs of all branches, and let the standard
security mechanisms decide who can view the diffs.
== Pre-implementation notes ==
None
== Implementation details ==
PreviewDiffs are now accessible via the +files path segment, with the same mechanism as Launchpad components.
IPreviewDiff access was allowed to anyone. Now it is only allowed to those who
can access the merge proposal.
== Tests ==
bin/test -vt xx-branchmergep
== Demo and Q/A ==
Create a private branch and propose it for merging. Wait until the diff is
generated. Go to the URL for the diff. Log out. Go to the URL for the diff.
You should get an access denied message.
= 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/canonical
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
== Pylint notices ==
lib/lp/
18: [F0401] Unable to import 'lazr.delegates' (No module named delegates)
210: [W0703, Diff.fromFile] Catch "Exception"
lib/lp/
20: [F0401] Unable to import 'lazr.restful.
21: [F0401] Unable to import 'lazr.restful.