Code review comment for lp:~michael.nelson/launchpad/429353-site-message-to-footer
- 429353-site-message-to-footer
- Merge into devel
Revision history for this message
| Michael Nelson (michael.nelson) wrote : | # |
| 1 | === modified file 'lib/canonical/launchpad/doc/launchpadview.txt' |
| 2 | --- lib/canonical/launchpad/doc/launchpadview.txt 2009-09-15 17:16:17 +0000 |
| 3 | +++ lib/canonical/launchpad/doc/launchpadview.txt 2009-09-16 08:00:49 +0000 |
| 4 | @@ -111,9 +111,3 @@ |
| 5 | >>> view.isRedirectInhibited() |
| 6 | True |
| 7 | |
| 8 | -Every Launchpad view knows whether redirection is possible. |
| 9 | - |
| 10 | - >>> view.canRedirect() |
| 11 | - False |
| 12 | - |
| 13 | - |
| 14 | |
| 15 | === modified file 'lib/canonical/launchpad/pagetests/basics/demo-and-lpnet.txt' |
| 16 | --- lib/canonical/launchpad/pagetests/basics/demo-and-lpnet.txt 2009-09-15 13:55:00 +0000 |
| 17 | +++ lib/canonical/launchpad/pagetests/basics/demo-and-lpnet.txt 2009-09-16 07:59:36 +0000 |
| 18 | @@ -101,6 +101,69 @@ |
| 19 | 0 |
| 20 | |
| 21 | |
| 22 | +== Launchpad Edge == |
| 23 | + |
| 24 | +Additionally, when a server is running as an edge server, the site-message |
| 25 | +is appended with a link to disable edge redirects. |
| 26 | + |
| 27 | +In addition to this prominent display on the root page, each page will |
| 28 | +also include the disable-redirect link in the site_message - if the |
| 29 | +user is a member of the beta group and has not already disabled |
| 30 | +the redirects. |
| 31 | + |
| 32 | + # Now setup an edge site-message config and re-check. |
| 33 | + >>> edge_config_data = """ |
| 34 | + ... [launchpad] |
| 35 | + ... site_message: This is a beta site. |
| 36 | + ... is_edge: True |
| 37 | + ... """ |
| 38 | + >>> config.push('edge_config_data', edge_config_data) |
| 39 | + >>> beta_browser = setupBrowser( |
| 40 | + ... auth='Basic beta-admin@launchpad.net:test') |
| 41 | + >>> beta_browser.open('http://launchpad.dev/ubuntu') |
| 42 | + >>> site_message = find_tags_by_class( |
| 43 | + ... beta_browser.contents, 'sitemessage')[0] |
| 44 | + >>> print extract_text(site_message) |
| 45 | + This is a beta site. Disable edge redirect. |
| 46 | + >>> print extract_text(site_message.find( |
| 47 | + ... 'a', onclick="setBetaRedirect(false)")) |
| 48 | + Disable edge redirect. |
| 49 | + |
| 50 | +The disable-redirect link will not appear in the site_message when |
| 51 | +browsed by non-beta users. |
| 52 | + |
| 53 | + >>> browser.open('http://launchpad.dev/ubuntu') |
| 54 | + >>> print extract_text(find_tags_by_class( |
| 55 | + ... browser.contents, 'sitemessage')[0]) |
| 56 | + This is a beta site. |
| 57 | + |
| 58 | +Similarly, once the redirection has been inhibited, the link will no longer |
| 59 | +appear in the footer of edge pages. |
| 60 | + |
| 61 | + # Workaround bug in mechanize where you cannot use the Cookie |
| 62 | + # header with the CookieJar |
| 63 | + >>> from mechanize._clientcookie import Cookie |
| 64 | + >>> cookiejar = ( |
| 65 | + ... beta_browser.mech_browser._ua_handlers['_cookies'].cookiejar) |
| 66 | + >>> cookiejar.set_cookie( |
| 67 | + ... Cookie( |
| 68 | + ... version=0, name='inhibit_beta_redirect', value='1', port=None, |
| 69 | + ... port_specified=False, domain='.launchpad.dev', |
| 70 | + ... domain_specified=True, domain_initial_dot=True, path='/', |
| 71 | + ... path_specified=True, secure=False, expires=None, |
| 72 | + ... discard=None, comment=None, comment_url=None, rest={})) |
| 73 | + >>> beta_browser.open('http://launchpad.dev/ubuntu') |
| 74 | + >>> site_message = find_tags_by_class( |
| 75 | + ... beta_browser.contents, 'sitemessage')[0] |
| 76 | + >>> print extract_text(site_message) |
| 77 | + This is a beta site. |
| 78 | + >>> print site_message.find('a') |
| 79 | + None |
| 80 | + |
| 81 | + # Remove the specific site-message config data before continuing. |
| 82 | + >>> dummy = config.pop('edge_config_data') |
| 83 | + |
| 84 | + |
| 85 | == Launchpad.net == |
| 86 | |
| 87 | On every instance except launchpad.net, Launchpad's version and |
| 88 | |
| 89 | === modified file 'lib/canonical/launchpad/pagetests/standalone/xx-beta-testers-redirection.txt' |
| 90 | --- lib/canonical/launchpad/pagetests/standalone/xx-beta-testers-redirection.txt 2009-09-15 17:16:17 +0000 |
| 91 | +++ lib/canonical/launchpad/pagetests/standalone/xx-beta-testers-redirection.txt 2009-09-16 07:44:23 +0000 |
| 92 | @@ -126,40 +126,6 @@ |
| 93 | for 2 hours</button></p> |
| 94 | </div> |
| 95 | |
| 96 | -In addition to this prominent display on the root page, each page will |
| 97 | -also include the disable-redirect link in the site_message. |
| 98 | - |
| 99 | - # First ensure it's not there when there is no site-message to display. |
| 100 | - >>> check(beta_browser.open, 'http://launchpad.dev') |
| 101 | - >>> len(find_tags_by_class(beta_browser.contents, 'sitemessage')) |
| 102 | - 0 |
| 103 | - |
| 104 | - # Now setup a site-message config and re-check. |
| 105 | - >>> site_msg_data = """ |
| 106 | - ... [launchpad] |
| 107 | - ... site_message: This is a beta site. |
| 108 | - ... """ |
| 109 | - >>> config.push('site_msg_data', site_msg_data) |
| 110 | - >>> check(beta_browser.open, 'http://launchpad.dev') |
| 111 | - >>> site_message = find_tags_by_class( |
| 112 | - ... beta_browser.contents, 'sitemessage')[0] |
| 113 | - >>> print extract_text(site_message) |
| 114 | - This is a beta site. Disable edge redirect. |
| 115 | - >>> print extract_text(site_message.find( |
| 116 | - ... 'a', onclick="setBetaRedirect(false)")) |
| 117 | - Disable edge redirect. |
| 118 | - |
| 119 | -The disable-redirect link will not appear in the site_message when |
| 120 | -browsed by non-beta users. |
| 121 | - |
| 122 | - >>> check(user_browser.open, 'http://launchpad.dev') |
| 123 | - >>> print extract_text(find_tags_by_class( |
| 124 | - ... user_browser.contents, 'sitemessage')[0]) |
| 125 | - This is a beta site. |
| 126 | - |
| 127 | - # Remove the specific site-message config data before continuing. |
| 128 | - >>> dummy = config.pop('site_msg_data') |
| 129 | - |
| 130 | This is so that the user can make use of some client side JS on that |
| 131 | page that sets a cookie to inhibit the redirection. When the |
| 132 | redirection is inhibited, the info message changes to one that lets |
| 133 | @@ -191,20 +157,6 @@ |
| 134 | redirection</button></p> |
| 135 | </div> |
| 136 | |
| 137 | -Similarly, once the redirection has been inhibited, the link will no longer |
| 138 | -appear in the footer of edge pages. |
| 139 | - |
| 140 | - >>> config.push('site_msg_data', site_msg_data) |
| 141 | - >>> check(beta_browser.open, 'http://launchpad.dev') |
| 142 | - >>> site_message = find_tags_by_class( |
| 143 | - ... beta_browser.contents, 'sitemessage')[0] |
| 144 | - >>> print extract_text(site_message) |
| 145 | - This is a beta site. |
| 146 | - >>> print site_message.find('a') |
| 147 | - None |
| 148 | - |
| 149 | - >>> dummy = config.pop('site_msg_data') |
| 150 | - |
| 151 | |
| 152 | Now when they go to a page on the site, it loads as normal: |
| 153 | |
| 154 | |
| 155 | === modified file 'lib/canonical/launchpad/webapp/publisher.py' |
| 156 | --- lib/canonical/launchpad/webapp/publisher.py 2009-09-15 17:16:17 +0000 |
| 157 | +++ lib/canonical/launchpad/webapp/publisher.py 2009-09-16 08:01:04 +0000 |
| 158 | @@ -277,12 +277,6 @@ |
| 159 | """Returns True if redirection has been inhibited.""" |
| 160 | return self.request.cookies.get('inhibit_beta_redirect', '0') == '1' |
| 161 | |
| 162 | - def canRedirect(self): |
| 163 | - """Return True if the beta server is available to the user.""" |
| 164 | - return bool( |
| 165 | - config.launchpad.beta_testers_redirection_host is not None and |
| 166 | - self.isBetaUser) |
| 167 | - |
| 168 | def __call__(self): |
| 169 | self.initialize() |
| 170 | if self._isRedirected(): |
| 171 | |
| 172 | === modified file 'lib/lp/app/templates/base-layout-macros.pt' |
| 173 | --- lib/lp/app/templates/base-layout-macros.pt 2009-09-15 17:30:59 +0000 |
| 174 | +++ lib/lp/app/templates/base-layout-macros.pt 2009-09-16 07:58:24 +0000 |
| 175 | @@ -376,12 +376,14 @@ |
| 176 | <tal:site_message tal:content="structure site_message"> |
| 177 | This site is running pre-release code. |
| 178 | </tal:site_message> |
| 179 | - <tal:redirect_only condition="view/canRedirect"> |
| 180 | - <a href="#" class="js-action" onclick="setBetaRedirect(false)" |
| 181 | - tal:condition="not:view/isRedirectInhibited"> |
| 182 | - Disable edge redirect. |
| 183 | - </a> |
| 184 | - </tal:redirect_only> |
| 185 | + <tal:edge_only condition="is_edge"> |
| 186 | + <tal:beta_user condition="view/isBetaUser"> |
| 187 | + <a href="#" class="js-action" onclick="setBetaRedirect(false)" |
| 188 | + tal:condition="not:view/isRedirectInhibited"> |
| 189 | + Disable edge redirect. |
| 190 | + </a> |
| 191 | + </tal:beta_user> |
| 192 | + </tal:edge_only> |
| 193 | </div> |
| 194 | </metal:site-message> |
| 195 | |
| 196 | |
| 197 | === modified file 'lib/lp/registry/browser/root.py' |
| 198 | --- lib/lp/registry/browser/root.py 2009-09-15 17:16:17 +0000 |
| 199 | +++ lib/lp/registry/browser/root.py 2009-09-16 08:01:30 +0000 |
| 200 | @@ -98,6 +98,12 @@ |
| 201 | """The total blueprint count in all of Launchpad.""" |
| 202 | return getUtility(ILaunchpadStatisticSet).value('question_count') |
| 203 | |
| 204 | + def canRedirect(self): |
| 205 | + """Return True if the beta server is available to the user.""" |
| 206 | + return bool( |
| 207 | + config.launchpad.beta_testers_redirection_host is not None and |
| 208 | + self.isBetaUser) |
| 209 | + |
| 210 | |
| 211 | class LaunchpadSearchFormView(LaunchpadView): |
| 212 | """A view to display the global search form in any page.""" |
Brad Crittenden wrote:
> Review: Approve code
> This change looks very good Michael, thanks for doing it. Having the message up top was very prominent but did interfere with the page flow. I hope it won't be too discreet way down there.
>
> I don't see any problem moving those methods to LaunchpadView.
>
Thanks Brad!
Unfortunately, after sleeping on it I realised that it was not quite
right. As it was, the link to disable the redirections would never have
been displayed as it was conditional on a site-message being present in
the config and redirection being enabled on the *current* app server
(which is only ever production, afaiui).
So I've updated and tested the branch to ensure that the
disable-redirection link only appears when:
1. a site-message is defined (as it's displayed with the site-message),
2. the app server is an edge app server (ie. the *target* of redirections),
3. the page is *not* a locationless page (ie. one where the view may not
necessarily inherit from LPView - and therefore view/isBetaUser will error)
4. the user is a beta user (non-beta users can visit edge also), and
5. the user has not already inhibited the redirection link.
*phew*
Additionally, I've updated the way it displays after Martins comments.
Screenshots here:
http://
http://
http://
Or to demo:
1. apply diff http://
2. as anonymous or any non-beta user, view the footer at both:
https:/
https:/
It should state only "This site is running... Please report all bugs."
3. Logout and in as beta-admin, refresh the above urls and note the extra
link.
4. Click on the 'Disable edge redirect' link. Reload the page and verify
the disable edge redirect link is no-longer there.
The incremental is attached - part of it is reverting some previous
changes/tests, so it might be easier to look at the full diff on the branch.
--
Michael