opendev/gerrit
Code Issues Proposed changes

Use the download attribute on patch links

Browse Source 
Patch links were being wrongfully captured by the catchall route and
resulted in a visual 404 when clicked. The "pass-through" approach to
instructing page.js to ignore these links does not work because clicking
the link should result in a download and should not change the browser's
URL.

However, page.js will ignore links with the download attribute, and this
perfectly aligns with the intent of the problematic links. With this
change all download links in the download dialog bear the `download`
attribute and a test ensures it.

Tests are lightly cleaned up to avoid repeating some setup code.

Bug: Issue 7149
Change-Id: I83fa1b4bbd0d3b6d347d0b057ca7033db8cd82d6
This commit is contained in:
Wyatt Allen 2017-09-05 13:42:19 -07:00
parent 72a67eef56
commit ebf009cc5c
2 changed files with 31 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.html
View File

@ -77,10 +77,15 @@ limitations under the License.
<div class="patchFiles">
<label>Patch file</label>
<div>
<a id="download" href$="[[_computeDownloadLink(change, patchNum)]]">
<a
id="download"
href$="[[_computeDownloadLink(change, patchNum)]]"
download>
[[_computeDownloadFilename(change, patchNum)]]
</a>
<a href$="[[_computeZipDownloadLink(change, patchNum)]]">
<a
href$="[[_computeZipDownloadLink(change, patchNum)]]"
download>
[[_computeZipDownloadFilename(change, patchNum)]]
</a>
</div>
@ -89,7 +94,9 @@ limitations under the License.
<label>Archive</label>
<div id="archives" class="archives">
<template is="dom-repeat" items="[[config.archives]]" as="format">
<a href$="[[_computeArchiveDownloadLink(change, patchNum, format)]]">
<a
href$="[[_computeArchiveDownloadLink(change, patchNum, format)]]"
download>
[[format]]
</a>
</template>

44
polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog_test.html
View File

@ -115,31 +115,39 @@ limitations under the License.
let sandbox;
setup(() => {
element = fixture('basic');
sandbox = sinon.sandbox.create();
element = fixture('basic');
element.patchNum = '1';
element.config = {
schemes: {
'anonymous http': {},
'http': {},
'repo': {},
'ssh': {},
},
archives: ['tgz', 'tar', 'tbz2', 'txz'],
};
flushAsynchronousOperations();
});
teardown(() => {
sandbox.restore();
});
test('anchors use download attribute', () => {
const anchors = Polymer.dom(element.root).querySelectorAll('a');
assert.isTrue(!anchors.some(a => !a.hasAttribute('download')));
});
suite('gr-download-dialog tests with no fetch options', () => {
setup(() => {
element.change = getChangeObjectNoFetch();
element.patchNum = '1';
element.config = {
schemes: {
'anonymous http': {},
'http': {},
'repo': {},
'ssh': {},
},
archives: ['tgz', 'tar', 'tbz2', 'txz'],
};
flushAsynchronousOperations();
});
test('focuses on first download link if no copy links', () => {
flushAsynchronousOperations();
const focusStub = sandbox.stub(element.$.download, 'focus');
element.focus();
assert.isTrue(focusStub.called);
@ -150,20 +158,10 @@ limitations under the License.
suite('gr-download-dialog with fetch options', () => {
setup(() => {
element.change = getChangeObject();
element.patchNum = '1';
element.config = {
schemes: {
'anonymous http': {},
'http': {},
'repo': {},
'ssh': {},
},
archives: ['tgz', 'tar', 'tbz2', 'txz'],
};
flushAsynchronousOperations();
});
test('focuses on first copy link', () => {
flushAsynchronousOperations();
const focusStub = sinon.stub(element.$.downloadCommands, 'focusOnCopy');
element.focus();
flushAsynchronousOperations();