Launchpad itself

Code review comment for lp:~salgado/launchpad/edge-redirect-bugs

  1. edge-redirect-bugs
  2. Merge into devel
Revision history for this message
Guilherme Salgado (salgado) wrote :

On Wed, 2010-03-10 at 17:06 +0000, Gavin Panella wrote:
> Review: Approve code
> Hi,
>
> The "Disable edge redirect" message is only shown when is_edge is true
> *and* site_message is set. The cover letter didn't mention that, but I
> assume that's fine.

Indeed, I forgot to mention that.

Thanks for the review!

>
> I have a few comments, but nothing terrible :)
>

> > === modified file 'lib/canonical/launchpad/templates/launchpad-requestexpired.pt'
> > --- lib/canonical/launchpad/templates/launchpad-requestexpired.pt 2009-07-28 17:19:42 +0000
> > +++ lib/canonical/launchpad/templates/launchpad-requestexpired.pt 2010-03-10 16:32:41 +0000
> > @@ -9,27 +9,16 @@
> > <body>
> > <div class="top-portlet" metal:fill-slot="main">
> > <h1 class="exception">Timeout error</h1>
> > - <div
> > - id="redirect_notice" style="display:none" class="informational message">
> > - <p>Our edge server has a lower timeout threshold than launchpad.net,
> > - so we can catch those before they hit a wider audience.
> > - As a member of the Launchpad Beta Testers team, you're more
> > - likely to experience them. If this is blocking your work, you
> > - can disable redirection.</p>
> > - <p><button onclick="setBetaRedirect(false)">Disable redirection
> > - for 2 hours</button></p>
> > + <div tal:condition="is_edge"
> > + id="redirect_notice" class="informational message">
> > + <p>Our edge server has a lower timeout threshold than launchpad.net,
> > + so we can catch those before they hit a wider audience.
> > + As a member of the Launchpad Beta Testers team, you're more
>
> You haven't changed it, but this isn't strictly true - that the viewer
> is a beta tester - any more is it? Doesn't matter much though.

You're right. How about this:

  <p>Our edge server has a lower timeout threshold than launchpad.net,
  so we can catch those before they hit a wider audience.
  If this is blocking your work, you can disable automatic redirection
  to edge in order to use launchpad.net.</p>
?

>
> > + likely to experience them. If this is blocking your work, you
> > + can disable redirection.</p>
> > + <p><button onclick="setBetaRedirect(false)">Disable redirection
> > + for 2 hours</button></p>
> > </div>
> > - <script type="text/javascript">
> > - var edge_cookie = document.cookie.match('edge');
> > - var edge_host = document.location.hostname.match('edge');
> > -
> > - // Logic has been inverted in the next line to avoid breaking
> > - // XHTML compliance of the template due to ampersand usage.
> > - if (!(edge_cookie || edge_host)) {
> > - redirect_notice_div = document.getElementById('redirect_notice');
> > - redirect_notice_div.style.display = "block";
> > - };
> > - </script>
> > <p>
> > Sorry, something just went wrong in Launchpad.
> > </p>
> >
> > === modified file 'lib/canonical/launchpad/templates/oops.pt'
> > --- lib/canonical/launchpad/templates/oops.pt 2010-01-13 17:58:30 +0000
> > +++ lib/canonical/launchpad/templates/oops.pt 2010-03-10 16:32:41 +0000
> > @@ -9,6 +9,9 @@
> > <style type="text/css" media="screen, print">
> > @import url(/+icing/rev5/combo.css);
> > </style>
> > + <script type="text/javascript"
> > + tal:content="string:var cookie_scope = '${request/lp:cookie_scope}';"></script>
> > + <script type="text/javascript" src="/+icing/rev5/build/lp/lp.js" />
>
> Do you mean to hard-code the "rev5" there?

Unfortunately, yes. This template doesn't use the base layout so it
doesn't get all the goodies that would allow us to easily generate a
link for the current revision. Also, I think it's no big deal to let the
browser use an older (cached) version of the JS/CSS in the OOPS page.
(I could've hard-coded any other number there, though)

>
> > </head>
> > <body>
> > <div class="yui-d0">
> > @@ -27,6 +30,27 @@
> > (Error <abbr>ID</abbr>:
> > <tal:oops replace="structure view/oops_id_text" />)
> > </p>
> > +
> > + <div id="redirect_notice" style="display:none"
> > + class="informational message">
> > + <p>This server runs pre-release code, so it's possible this problem
> > + doesn't affect launchpad.net. If you're a member of the Launchpad
> > + Beta Testers team, you can disable redirection in order to use
> > + launchpad.net.</p>
> > + <p><button onclick="setBetaRedirect(false)">Disable redirection
> > + for 2 hours</button></p>
> > + </div>
> > + <tal:comment condition="nothing">
> > + Can't use the 'is_edge' global because we don't use our page
> > + macros here.
> > + </tal:comment>
> > + <script type="text/javascript">
> > + if (document.location.hostname.match('.dev')) {
>
> Does this need to be changed to "edge."? Ah, perhaps you left this so
> it's easier to review?

It should be 'edge.', yes. I changed it in order to see the notification
on an OOPS page, but forgot to revert to edge before committing.

>
> > + redirect_notice_div = document.getElementById('redirect_notice');
> > + redirect_notice_div.style.display = "block";
> > + };
> > + </script>
> > +
> > <tal:traceback replace="structure view/maybeShowTraceback" />
> >
> > <div class="related">
> >

--
Guilherme Salgado <email address hidden>

« Back to merge proposal