Merge into trunk : ie10-upgrade-1246946 : Code : juju-gui

Reviewer	Review Type	Date Requested	Status
Juju GUI Hackers		2013-11-03	Pending
Review via email: mp+193716@code.launchpad.net

Revision history for this message

Jeff Pihach (hatch) wrote on 2013-11-03:

#

Reviewers: mp+193716_code.launchpad.net,

Message:
Please take a look.

Description:
Fixes upgrade charm page reload in IE10

When clicking a link in the inspector in the upgrade charm
section in IE10 the page would reload instead of dispatching
to the router. This fixes it by monkey patching the offending
method in the YUI source.

https://code.launchpad.net/~hatch/juju-gui/ie10-upgrade-1246946/+merge/193716

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/21430043/

Affected files (+62, -2 lines):
   A [revision details]
   M app/app.js
   A app/assets/javascripts/app-patches-extension.js
   M app/modules-debug.js
   M bin/merge-files

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: app/app.js
=== modified file 'app/app.js'
--- app/app.js 2013-10-30 11:59:48 +0000
+++ app/app.js 2013-11-03 20:10:26 +0000
@@ -51,7 +51,8 @@

Y.juju.SubAppRegistration,
Y.juju.NSRouter,
Y.juju.Cookies,
- Y.juju.GhostDeployer], {
+ Y.juju.GhostDeployer,
+ Y.juju.Patches], {

      /*
        Extension properties
@@ -1349,6 +1350,7 @@
      'juju-inspector-widget',
      'juju-ghost-inspector',
      'juju-view-bundle',
- 'viewmode-controls'
+ 'viewmode-controls',
+ 'app-patches-extension'
    ]
  });

Index: app/modules-debug.js
=== modified file 'app/modules-debug.js'
--- app/modules-debug.js 2013-10-18 16:47:34 +0000
+++ app/modules-debug.js 2013-11-03 20:10:26 +0000
@@ -181,6 +181,10 @@
fullpath: '/juju-ui/assets/javascripts/app-cookies-extension.js'
},

+ 'app-patches-extension': {
+ fullpath: '/juju-ui/assets/javascripts/app-patches-extension.js'
+ },
+
          'sub-app': {
            fullpath: '/juju-ui/assets/javascripts/sub-app.js'
          },

Index: bin/merge-files
=== modified file 'bin/merge-files'
--- bin/merge-files 2013-10-31 03:26:53 +0000
+++ bin/merge-files 2013-11-03 20:48:36 +0000
@@ -88,6 +88,7 @@
    filesToLoad.js.push.apply(filesToLoad.js, [
      'app/assets/javascripts/app-subapp-extension.js',
      'app/assets/javascripts/app-cookies-extension.js',
+ 'app/assets/javascripts/app-patches-extension.js',
      'app/assets/javascripts/d3-components.js',
      'app/assets/javascripts/d3.min.js',
      'app/assets/javascripts/d3.status.js',

Index: app/assets/javascripts/app-patches-extension.js
=== added file 'app/assets/javascripts/app-patches-extension.js'
--- app/assets/javascripts/app-patches-extension.js 1970-01-01 00:00:00
+0000
+++ app/assets/javascripts/app-patches-extension.js 2013-11-03 20:10:26
+0000
@@ -0,0 +1,51 @@
+/*
+This file i...

Reviewers: mp+193716_code.launchpad.net,

Message:
Please take a look.

Description:
Fixes upgrade charm page reload in IE10

When clicking a link in the inspector in the upgrade charm
section in IE10 the page would reload instead of dispatching
to the router. This fixes it by monkey patching the offending
method in the YUI source.

https://code.launchpad.net/~hatch/juju-gui/ie10-upgrade-1246946/+merge/193716

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/21430043/

Affected files (+62, -2 lines):
   A [revision details]
   M app/app.js
   A app/assets/javascripts/app-patches-extension.js
   M app/modules-debug.js
   M bin/merge-files

Index: [revision details]
=== added file '[revision details]'
--- [revision details]	2012-01-01 00:00:00 +0000
+++ [revision details]	2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: gary.poster@canonical.com-20131101002202-bowjun1wqa3q6f70
+New revision: jeff.pihach@canonical.com-20131103204836-t3hj39bkbi3ziznj

Index: app/app.js
=== modified file 'app/app.js'
--- app/app.js	2013-10-30 11:59:48 +0000
+++ app/app.js	2013-11-03 20:10:26 +0000
@@ -51,7 +51,8 @@
                                                     
Y.juju.SubAppRegistration,
                                                    Y.juju.NSRouter,
                                                    Y.juju.Cookies,
-                                                  Y.juju.GhostDeployer], {
+                                                  Y.juju.GhostDeployer,
+                                                  Y.juju.Patches], {

/*
        Extension properties
@@ -1349,6 +1350,7 @@
      'juju-inspector-widget',
      'juju-ghost-inspector',
      'juju-view-bundle',
-    'viewmode-controls'
+    'viewmode-controls',
+    'app-patches-extension'
    ]
  });

Index: app/modules-debug.js
=== modified file 'app/modules-debug.js'
--- app/modules-debug.js	2013-10-18 16:47:34 +0000
+++ app/modules-debug.js	2013-11-03 20:10:26 +0000
@@ -181,6 +181,10 @@
            fullpath: '/juju-ui/assets/javascripts/app-cookies-extension.js'
          },

+        'app-patches-extension': {
+          fullpath: '/juju-ui/assets/javascripts/app-patches-extension.js'
+        },
+
          'sub-app': {
            fullpath: '/juju-ui/assets/javascripts/sub-app.js'
          },

Index: bin/merge-files
=== modified file 'bin/merge-files'
--- bin/merge-files	2013-10-31 03:26:53 +0000
+++ bin/merge-files	2013-11-03 20:48:36 +0000
@@ -88,6 +88,7 @@
    filesToLoad.js.push.apply(filesToLoad.js, [
      'app/assets/javascripts/app-subapp-extension.js',
      'app/assets/javascripts/app-cookies-extension.js',
+    'app/assets/javascripts/app-patches-extension.js',
      'app/assets/javascripts/d3-components.js',
      'app/assets/javascripts/d3.min.js',
      'app/assets/javascripts/d3.status.js',

Index: app/assets/javascripts/app-patches-extension.js
=== added file 'app/assets/javascripts/app-patches-extension.js'
--- app/assets/javascripts/app-patches-extension.js	1970-01-01 00:00:00  
+0000
+++ app/assets/javascripts/app-patches-extension.js	2013-11-03 20:10:26  
+0000
@@ -0,0 +1,51 @@
+/*
+This file is part of the Juju GUI, which lets users view and manage Juju
+environments within a graphical interface (https://launchpad.net/juju-gui).
+Copyright (C) 2013 Canonical Ltd.
+
+This program is free software: you can redistribute it and/or modify it  
under
+the terms of the GNU Affero General Public License version 3, as published  
by
+the Free Software Foundation.
+
+This program is distributed in the hope that it will be useful, but WITHOUT
+ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU  
Affero
+General Public License for more details.
+
+You should have received a copy of the GNU Affero General Public License  
along
+with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+'use strict';
+
+YUI.add('app-patches-extension', function(Y) {
+
+  /**
+    If a method in the App needs to be monkey patched for
+    whatever reason it should go here ot make it easy to find.
+
+     @class Patches
+     @extension App
+   */
+  function Patches() {}
+
+  Patches.prototype = {
+
+    /**
+      Monkey patch for the _isLinkSameOrigin method in YUI's pjax.base
+
+      This method does not work correctly in IE10 in YUI 3.11 - It  
incorrectly
+      determines that the 'upgrade service' links point to a different  
domain
+      and because of this does not allow pjax to handle the routing. We  
should
+      revisit this when we upgrade YUI to see if it has been fixed so this
+      can be removed.
+    */
+    _isLinkSameOrigin: function() {
+      return true;
+    }
+
+  };
+
+  Y.namespace('juju').Patches = Patches;
+
+});

Revision history for this message

Richard Harding (rharding) wrote on 2013-11-03:

#

Does this break any external links then? We've got links to launchpad in
the charm details view for bugs and source that must work.

https://codereview.appspot.com/21430043/

Revision history for this message

Jeff Pihach (hatch) wrote on 2013-11-03:

#

Good catch! I'll implement a proper fix for this

https://codereview.appspot.com/21430043/

lp:~hatch/juju-gui/ie10-upgrade-1246946 updated on 2013-11-04

1178. By Jeff Pihach on 2013-11-04: Figured out alternative fix instead of monkeypatching broken YUI method

Revision history for this message

Jeff Pihach (hatch) wrote on 2013-11-04:

#

Please take a look.

https://codereview.appspot.com/21430043/

lp:~hatch/juju-gui/ie10-upgrade-1246946 updated on 2013-11-06

1179. By Jeff Pihach on 2013-11-06: merged trunk
1180. By Jeff Pihach on 2013-11-06: Added test to make sure that the links are generated properly for the upgrade charm detail links

Revision history for this message

Madison Scott-Clary (makyo) wrote on 2013-11-06:

#

QA ok, LGTM. Any way to test this?

https://codereview.appspot.com/21430043/

Revision history for this message

Jeff Pihach (hatch) wrote on 2013-11-06:

#

Please take a look.

https://codereview.appspot.com/21430043/

Revision history for this message

Jeff Pihach (hatch) wrote on 2013-11-06:

#

Thanks for the review/qa!

https://codereview.appspot.com/21430043/

Revision history for this message

Jeff Pihach (hatch) wrote on 2013-11-06:

#

*** Submitted:

Fixes upgrade charm page reload in IE10

When clicking a link in the inspector in the upgrade charm
section in IE10 the page would reload instead of dispatching
to the router. This fixes it by generating complete relative
links so that the faulty method in YUI's pjax.base class can
parse them and react accordingly.

R=rharding, matthew.scott
CC=
https://codereview.appspot.com/21430043

https://codereview.appspot.com/21430043/

 === modified file 'app/views/viewlets/service-overview.js'
 --- app/views/viewlets/service-overview.js	2013-11-05 03:30:06 +0000
 +++ app/views/viewlets/service-overview.js	2013-11-06 17:43:11 +0000
@@ -192,6 +192,15 @@
      var self = this,
          buttonHeight;
++    /*
++      The _isLinkSameOrigin method in YUI's pjax.base class does not
++      properly account for IE10's issues when parsing protocol and
++      host from anchor tags unless the tags contain the full protocol
++      and host for relative hrefs.
++    */
++    var wl = window.location;
++    var locationPrefix = wl.protocol + '//' + wl.host;
++
      var categoryWrapperNodes = d3.select(node.getDOMNode())
      .selectAll('.unit-list-wrapper')
      .data(statuses, function(d) {
@@ -237,7 +246,7 @@
      serviceUpgradeLi.append('a')
        .attr('href', function(d) {
--          return '/' + d.replace(/^cs:/, '');
++          return locationPrefix + '/' + d.replace(/^cs:/, '');
          })
        .text(function(d) { return d; });
@@ -284,7 +293,7 @@
      serviceUpgradeOtherCharms
        .append('a')
        .attr('href', function(d) {
--          return '/' + d.replace(/^cs:/, '');
++          return locationPrefix + '/' + d.replace(/^cs:/, '');
          })
        .text(function(d) { return d; });
 === modified file 'test/test_inspector_overview.js'
 --- test/test_inspector_overview.js	2013-11-05 18:10:05 +0000
 +++ test/test_inspector_overview.js	2013-11-06 17:43:11 +0000
@@ -675,6 +675,19 @@
      assert.equal(serviceWrapper.one(SUC).all('.top-upgrade').size(), 5);
      assert.equal(serviceWrapper.one(SUC).all('.other-charm').size(), 8);
++    // Check to make sure that the links to view the charm details in the
++    // upgrade section are full links instead of relative ones to allow
++    // the YUI Pjax module to properly parse them in IE10
++    serviceWrapper.one(SUC).all('.top-upgrade').each(function(node) {
++      // Selects the first anchor tag which is the 'view charm details' link
++      assert.isTrue(Y.PjaxBase.prototype._isLinkSameOrigin(node.one('a')));
++    });
++
++    serviceWrapper.one(SUC).all('.other-charm').each(function(node) {
++      // Selects the first anchor tag which is the 'view charm details' link
++      assert.isTrue(Y.PjaxBase.prototype._isLinkSameOrigin(node.one('a')));
++    });
++
      newContainer.remove(true);
    });

juju-gui

Merge lp:~hatch/juju-gui/ie10-upgrade-1246946 into lp:juju-gui/experimental

Commit message

Description of the change

Preview Diff

Subscribers