Launchpad itself

Merge lp:~sinzui/launchpad/bug-listings-urls into lp:launchpad

  1. bug-listings-urls
  2. Merge into devel
Proposed by Curtis Hovey
Status: Merged
Approved by: Ian Booth
Approved revision: no longer in the source branch.
Merged at revision: 15269
Proposed branch: lp:~sinzui/launchpad/bug-listings-urls
Merge into: lp:launchpad
Diff against target: 95 lines (+74/-1)
3 files modified
lib/lp/app/javascript/lp.js (+10/-1)
lib/lp/app/javascript/tests/test_lp.html (+38/-0)
lib/lp/app/javascript/tests/test_lp.js (+26/-0)
To merge this branch: bzr merge lp:~sinzui/launchpad/bug-listings-urls
Reviewer Review Type Date Requested Status
Ian Booth (community) Approve
Review via email: mp+106266@code.launchpad.net

Commit message

Enable bug listing navigation and sorting in browsers with broken pathname.

Description of the change

Pre-implementation: wgrant

Bug listings are very broken in MSIE because calls to
preserve history corrupt the URLs and state of the page. Users cannot
see more than one batch nor can they sort the listing.

--------------------------------------------------------------------

RULES

    * We had agreed to disable the history module for MSIE and Opera.
      I tested the latest version of Opera and it does work contrary to
      reports that bug listings were broken. The History module has
      a lot of improvements in YUI 3.5 so I decided to look into to
      the double pillar in the URL. My suspicious were confirmed that
      IE was getting a relative URL when I compared IE and Chromium in
      their debuggers.
      * Ensure pathname are absolute.

QA

    Using MSIE and Opera
    * Visit https://bugs.qastaging.launchpad.net/pocket-lint
    * Verify you can sort the listing (No error dialog).
    * Verify you can see the next page (No error dialog).

LINT

    lib/lp/app/javascript/lp.js
    lib/lp/app/javascript/tests/test_lp.html
    lib/lp/app/javascript/tests/test_lp.js

TEST

    ./bin/test -vvc --layer=YUITest

IMPLEMENTATION

I fixed a helper method that assumes the browser returns a same pathname,
which IE does not. I added a rule to ensure a double slash was not
created because I recall we have seen this in the past.
    lib/lp/app/javascript/lp.js
    lib/lp/app/javascript/tests/test_lp.html
    lib/lp/app/javascript/tests/test_lp.js

To post a comment you must log in.
Revision history for this message
Ian Booth (wallyworld) wrote :

Very nice.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/app/javascript/lp.js'
2--- lib/lp/app/javascript/lp.js 2012-03-20 03:01:07 +0000
3+++ lib/lp/app/javascript/lp.js 2012-05-17 21:32:20 +0000
4@@ -77,7 +77,16 @@
5 * Return the path portion of the specified URL.
6 */
7 Y.lp.get_url_path = function(url) {
8- return get_hyperlink(url).get('pathname');
9+ pathname = get_hyperlink(url).get('pathname');
10+ if (!pathname || pathname[0] !== '/') {
11+ // Ensure the leading slash often dropped by msie.
12+ pathname = '/' + pathname;
13+ }
14+ if (pathname.length > 1 && pathname[1] === '/') {
15+ // Ensure a single root often broken by a concatenation error.
16+ pathname = pathname.substring(1, pathname.length);
17+ }
18+ return pathname;
19 };
20
21 /**
22
23=== added file 'lib/lp/app/javascript/tests/test_lp.html'
24--- lib/lp/app/javascript/tests/test_lp.html 1970-01-01 00:00:00 +0000
25+++ lib/lp/app/javascript/tests/test_lp.html 2012-05-17 21:32:20 +0000
26@@ -0,0 +1,38 @@
27+<!DOCTYPE html>
28+<!--
29+Copyright 2012 Canonical Ltd. This software is licensed under the
30+GNU Affero General Public License version 3 (see the file LICENSE).
31+-->
32+<html>
33+ <head>
34+ <title>Test lp-names</title>
35+ <!-- YUI and test setup -->
36+ <script type="text/javascript"
37+ src="../../../../../build/js/yui/yui/yui.js">
38+ </script>
39+ <link rel="stylesheet"
40+ href="../../../../../build/js/yui/console/assets/console-core.css" />
41+ <link rel="stylesheet"
42+ href="../../../../../build/js/yui/console/assets/skins/sam/console.css" />
43+ <link rel="stylesheet"
44+ href="../../../../../build/js/yui/test/assets/skins/sam/test.css" />
45+
46+ <script type="text/javascript"
47+ src="../../../../../build/js/lp/app/testing/testrunner.js"></script>
48+ <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
49+
50+ <!-- The module under test. -->
51+ <script type="text/javascript" src="../lp.js"></script>
52+
53+ <!-- The test suite. -->
54+ <script type="text/javascript" src="test_lp.js"></script>
55+
56+ </head>
57+ <body class="yui3-skin-sam">
58+ <ul id="suites">
59+ <li>lp.test</li>
60+ </ul>
61+
62+ <div id="fixture"></div>
63+ </body>
64+</html>
65
66=== added file 'lib/lp/app/javascript/tests/test_lp.js'
67--- lib/lp/app/javascript/tests/test_lp.js 1970-01-01 00:00:00 +0000
68+++ lib/lp/app/javascript/tests/test_lp.js 2012-05-17 21:32:20 +0000
69@@ -0,0 +1,26 @@
70+YUI().use('lp.testing.runner', 'test', 'console', 'node', 'lp', function(Y) {
71+
72+ var suite = new Y.Test.Suite("lp helper tests");
73+
74+ suite.add(new Y.Test.Case({
75+ name: 'test_bugs_branches',
76+
77+ test_get_url_path_with_pillar: function () {
78+ Y.Assert.areEqual(
79+ '/fnord', Y.lp.get_url_path('http://launchpad.dev/fnord'));
80+ },
81+
82+ test_get_url_path_without_slash: function () {
83+ Y.Assert.areEqual(
84+ '/', Y.lp.get_url_path('http://launchpad.dev'));
85+ },
86+
87+ test_get_url_path_with_double_slash: function () {
88+ Y.Assert.areEqual(
89+ '/fnord', Y.lp.get_url_path('http://launchpad.dev//fnord'));
90+ }
91+ }));
92+
93+ Y.lp.testing.Runner.run(suite);
94+});
95+