Properly encode URL for next/back links

Adds gr-url-encoding-behavior, which abstracts and centralizes the URL encoding and prettifying responsibility for the app. Also adds a testing suite for gr-change-list-view. Bug: Issue 4602 Change-Id: I8c8605e1b79dffbbabf92b57ef0cb1bb6260e4f9
2016-09-23 15:14:59 -07:00 · 2016-09-23 15:14:59 -07:00 · f99ce6e307
commit f99ce6e307
parent 1b944dffd8
12 changed files with 116 additions and 23 deletions
--- a/polygerrit-ui/app/behaviors/gr-url-encoding-behavior.html
+++ b/polygerrit-ui/app/behaviors/gr-url-encoding-behavior.html
@ -0,0 +1,42 @@
+<!--
+Copyright (C) 2016 The Android Open Source Project
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+-->
+<script>
+(function(window) {
+  'use strict';
+
+  /** @polymerBehavior Gerrit.URLEncodingBehavior */
+  var URLEncodingBehavior = {
+    /**
+     * Pretty-encodes a URL. Double-encodes the string, and then replaces
+     *   benevolent characters for legibility.
+     */
+    encodeURL: function(url, replaceSlashes) {
+      // @see Issue 4255 regarding double-encoding.
+      var output = encodeURIComponent(encodeURIComponent(url));
+      // @see Issue 4577 regarding more readable URLs.
+      output = output.replace(/%253A/g, ':');
+      output = output.replace(/%2520/g, '+');
+      if (replaceSlashes) {
+        output = output.replace(/%252F/g, '/');
+      }
+      return output;
+    },
+  };
+
+  window.Gerrit = window.Gerrit || {};
+  window.Gerrit.URLEncodingBehavior = URLEncodingBehavior;
+})(window);
+</script>
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
@ -14,9 +14,10 @@ See the License for the specific language governing permissions and
 limitations under the License.
 -->

+<link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
+<link rel="import" href="../../../behaviors/rest-client-behavior.html">
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../../styles/gr-change-list-styles.html">
-<link rel="import" href="../../../behaviors/rest-client-behavior.html">
 <link rel="import" href="../../shared/gr-account-link/gr-account-link.html">
 <link rel="import" href="../../shared/gr-change-star/gr-change-star.html">
 <link rel="import" href="../../shared/gr-date-formatter/gr-date-formatter.html">
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js
@ -44,6 +44,7 @@

    behaviors: [
      Gerrit.RESTClientBehavior,
+      Gerrit.URLEncodingBehavior,
    ],

    _computeChangeURL: function(changeNum) {
@ -108,15 +109,14 @@
    },

    _computeProjectURL: function(project) {
-      // @see Issue 4255.
      return '/q/status:open+project:' +
-          encodeURIComponent(encodeURIComponent(project));
+          this.encodeURL(project, false);
    },

    _computeProjectBranchURL: function(project, branch) {
      // @see Issue 4255.
      return this._computeProjectURL(project) +
-          '+branch:' + encodeURIComponent(encodeURIComponent(branch));
+          '+branch:' + this.encodeURL(branch, false);
    },
  });
 })();
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.html
@ -14,6 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 -->

+<link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
 <link rel="import" href="../gr-change-list/gr-change-list.html">
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.js
@ -23,6 +23,7 @@
     * @event title-change
     */

+    behaviors: [Gerrit.URLEncodingBehavior],
    properties: {
      /**
       * URL params passed from the router.
@ -116,7 +117,8 @@
      // Offset could be a string when passed from the router.
      offset = +(offset || 0);
      var newOffset = Math.max(0, offset + (changesPerPage * direction));
-      var href = '/q/' + query;
+      // Double encode URI component.
+      var href = '/q/' + this.encodeURL(query, false);
      if (newOffset > 0) {
        href += ',' + newOffset;
      }
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view_test.html
@ -0,0 +1,51 @@
+<!DOCTYPE html>
+<!--
+Copyright (C) 2016 The Android Open Source Project
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+-->
+
+<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
+<title>gr-change-list-view</title>
+
+<script src="../../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script>
+<script src="../../../bower_components/web-component-tester/browser.js"></script>
+
+<link rel="import" href="gr-change-list-view.html">
+
+<test-fixture id="basic">
+  <template>
+    <gr-change-list-view></gr-change-list-view>
+  </template>
+</test-fixture>
+
+<script>
+  suite('gr-change-list-view tests', function() {
+    var element;
+
+    setup(function() {
+      element = fixture('basic');
+    });
+
+    test('url is properly encoded', function() {
+      assert.equal(element._computeNavLink(
+          'status:open project:platform/frameworks/base', 0, -1, 25),
+          '/q/status:open+project:platform%252Fframeworks%252Fbase'
+      );
+      assert.equal(element._computeNavLink(
+          'status:open project:platform/frameworks/base', 0, 1, 25),
+          '/q/status:open+project:platform%252Fframeworks%252Fbase,25'
+      );
+    });
+  });
+</script>
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@ -14,8 +14,9 @@ See the License for the specific language governing permissions and
 limitations under the License.
 -->

-<link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../../behaviors/keyboard-shortcut-behavior.html">
+<link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
+<link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../diff/gr-diff/gr-diff.html">
 <link rel="import" href="../../diff/gr-diff-cursor/gr-diff-cursor.html">
 <link rel="import" href="../../shared/gr-button/gr-button.html">
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@ -57,6 +57,7 @@

    behaviors: [
      Gerrit.KeyboardShortcutBehavior,
+      Gerrit.URLEncodingBehavior,
    ],

    reload: function() {
@ -128,8 +129,8 @@
    _handlePatchChange: function(e) {
      var patchRange = Object.assign({}, this.patchRange);
      patchRange.basePatchNum = Polymer.dom(e).rootTarget.value;
-      page.show('/c/' + encodeURIComponent(this.changeNum) + '/' +
-          encodeURIComponent(this._patchRangeStr(patchRange)));
+      page.show(this.encodeURL('/c/' + this.changeNum + '/' +
+          this._patchRangeStr(patchRange), true));
    },

    _forEachDiff: function(fn) {
@ -383,17 +384,8 @@
    },

    _computeDiffURL: function(changeNum, patchRange, path) {
-      // @see Issue 4255 regarding double-encoding.
-      path = encodeURIComponent(encodeURIComponent(path));
-      // @see Issue 4577 regarding more readable URLs.
-      path = path.replace(/%252F/g, '/');
-      path = path.replace(/%2520/g, '+');
-      return '/c/' +
-          encodeURIComponent(changeNum) +
-          '/' +
-          encodeURIComponent(this._patchRangeStr(patchRange)) +
-          '/' +
-          path;
+      return this.encodeURL('/c/' + changeNum + '/' +
+          this._patchRangeStr(patchRange) + '/' + path, true);
    },

    _patchRangeStr: function(patchRange) {
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.js
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.js
@ -134,6 +134,8 @@
      };
      // Don't allow diffing the same patch number against itself.
      if (params.basePatchNum === params.patchNum) {
+        // TODO(kaspern): Utilize gr-url-encoding-behavior.html when the router
+        // is replaced with a Polymer counterpart.
        // @see Issue 4255 regarding double-encoding.
        var path = encodeURIComponent(encodeURIComponent(path));
        // @see Issue 4577 regarding more readable URLs.
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.html
@ -14,8 +14,9 @@ See the License for the specific language governing permissions and
 limitations under the License.
 -->

-<link rel="import" href="../../../bower_components/polymer/polymer.html">
+<link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
 <link rel="import" href="../../../behaviors/keyboard-shortcut-behavior.html">
+<link rel="import" href="../../../bower_components/polymer/polymer.html">
 <link rel="import" href="../../shared/gr-autocomplete/gr-autocomplete.html">
 <link rel="import" href="../../shared/gr-button/gr-button.html">
 <link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
@ -87,6 +87,7 @@

    behaviors: [
      Gerrit.KeyboardShortcutBehavior,
+      Gerrit.URLEncodingBehavior,
    ],

    listeners: {
@ -140,9 +141,7 @@
        target.blur();
      }
      if (this._inputVal) {
-        // @see Issue 4255.
-        page.show('/q/' +
-            encodeURIComponent(encodeURIComponent(this._inputVal)));
+        page.show('/q/' + this.encodeURL(this._inputVal, false));
      }
    },

--- a/polygerrit-ui/app/test/index.html
+++ b/polygerrit-ui/app/test/index.html
@ -43,6 +43,7 @@ limitations under the License.
    'change/gr-reviewer-list/gr-reviewer-list_test.html',
    'change-list/gr-change-list/gr-change-list_test.html',
    'change-list/gr-change-list-item/gr-change-list-item_test.html',
+    'change-list/gr-change-list-view/gr-change-list-view_test.html',
    'core/gr-account-dropdown/gr-account-dropdown_test.html',
    'core/gr-error-manager/gr-error-manager_test.html',
    'core/gr-main-header/gr-main-header_test.html',