launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/front-page-layout into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #962043 in Launchpad itself: "Homepage header is broken"
  https://bugs.launchpad.net/launchpad/+bug/962043

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/front-page-layout/+merge/99121

Pre-implementation: no one

Launchpad root was switched to main_only layout when the locationless
layout was removed. The watermark does not align with the page content
because the root page defines its own styles :(

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

RULES

    * Add a hack to permit the Lp front page to say STFU to the watermark.
    * Add the Lp logo and style hack beneath it to restore the previous
      layout (removed in revno 14867).


QA

    See https://launchpadlibrarian.net/98063949/lp-logo-front-page.png
    for an example of the reverted change.

    * Visit https://qastaging.launchpad.net/
    * Verify the application links are not shown.
    * Verify the Lp logo is shown above the two columns
      with a dotted line below it.


LINT

    lib/lp/app/browser/root.py
    lib/lp/app/browser/tales.py
    lib/lp/app/browser/tests/test_launchpadroot.py
    lib/lp/app/browser/tests/test_page_macro.py
    lib/lp/app/templates/base-layout.pt
    lib/lp/app/templates/root-index.pt


TEST

    ./bin/test -vv -t page_macro -t base_layout -t root lp.app.browser.tests


IMPLEMENTATION

Added a hack to allow a view to say it does not have the watermark. We
do not want to make this easy for developers to do because the locationless
layout was abused causing lots of breadcrumb and heading issues.
    lib/lp/app/browser/tales.py
    lib/lp/app/browser/tests/test_page_macro.py
    lib/lp/app/templates/base-layout.pt

Updated the launchpadRootView to state is does not have a watermark and
reverted the change to the page template made in revno 14867.
    lib/lp/app/browser/root.py
    lib/lp/app/browser/tests/test_launchpadroot.py
    lib/lp/app/templates/root-index.pt
-- 
https://code.launchpad.net/~sinzui/launchpad/front-page-layout/+merge/99121
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/front-page-layout into lp:launchpad.

=== modified file 'lib/lp/app/browser/root.py'
--- lib/lp/app/browser/root.py	2011-12-30 08:13:14 +0000
+++ lib/lp/app/browser/root.py	2012-03-23 21:19:21 +0000
@@ -65,6 +65,7 @@
 
     # Used by the footer to display the lp-arcana section.
     is_root_page = True
+    has_watermark = False
 
     @staticmethod
     def _get_day_of_year():

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2012-03-02 16:17:46 +0000
+++ lib/lp/app/browser/tales.py	2012-03-23 21:19:21 +0000
@@ -2520,6 +2520,7 @@
         view/macro:pagetype
 
         view/macro:is-page-contentless
+        view/macro:has-watermark
     """
 
     implements(ITraversable)
@@ -2554,6 +2555,8 @@
             return self.pagetype()
         elif name == 'is-page-contentless':
             return self.isPageContentless()
+        elif name == 'has-watermark':
+            return self.hasWatermark()
         else:
             raise TraversalError(name)
 
@@ -2569,6 +2572,14 @@
             pagetype = 'unset'
         return self._pagetypes[pagetype][layoutelement]
 
+    def hasWatermark(self):
+        """Does the page havethe watermark block.
+
+        The default value is True, but the view can provide has_watermark
+        to force the page not render the standard location information.
+        """
+        return getattr(self.context, 'has_watermark', True)
+
     def isPageContentless(self):
         """Should the template avoid rendering detailed information.
 

=== modified file 'lib/lp/app/browser/tests/test_launchpadroot.py'
--- lib/lp/app/browser/tests/test_launchpadroot.py	2012-01-01 02:58:52 +0000
+++ lib/lp/app/browser/tests/test_launchpadroot.py	2012-03-23 21:19:21 +0000
@@ -108,3 +108,23 @@
         self.assertEqual(
             'https://help.launchpad.net/Feedback',
             request.response.getHeader('location'))
+
+
+class LaunchpadRootIndexViewTestCase(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_has_logo_without_watermark(self):
+        root = getUtility(ILaunchpadRoot)
+        user = self.factory.makePerson()
+        login_person(user)
+        view = create_initialized_view(root, 'index.html', principal=user)
+        # Replace the blog posts so the view does not make a network request.
+        view.getRecentBlogPosts = lambda: []
+        markup = BeautifulSoup(
+            view(), parseOnlyThese=SoupStrainer(id='document'))
+        self.assertIs(False, view.has_watermark)
+        self.assertIs(None, markup.find(True, id='watermark'))
+        logo = markup.find(True, id='launchpad-logo-and-name')
+        self.assertIsNot(None, logo)
+        self.assertEqual('/@@/launchpad-logo-and-name.png', logo['src'])

=== modified file 'lib/lp/app/browser/tests/test_page_macro.py'
--- lib/lp/app/browser/tests/test_page_macro.py	2012-01-01 02:58:52 +0000
+++ lib/lp/app/browser/tests/test_page_macro.py	2012-03-23 21:19:21 +0000
@@ -123,6 +123,31 @@
             KeyError, "'fnord'",
             self._call_test_tales, 'view/macro:pagehas/fnord')
 
+    def test_has_watermark_default(self):
+        # All pages have a watermark if the view does not provide the attr.
+        has_watermark = test_tales('view/macro:has-watermark', view=self.view)
+        self.assertIs(True, has_watermark)
+
+    def test_has_watermark_false(self):
+        # A view cand define has_watermark as False.
+        class NoWatermarkView(TestView):
+            has_watermark = False
+
+        self.registerBrowserViewAdapter(NoWatermarkView, ITest, '+test')
+        view = create_view(TestObject(), name='+test')
+        has_watermark = test_tales('view/macro:has-watermark', view=view)
+        self.assertIs(False, has_watermark)
+
+    def test_has_watermark_true(self):
+        # A view cand define has_watermark as True.
+        class NoWatermarkView(TestView):
+            has_watermark = True
+
+        self.registerBrowserViewAdapter(NoWatermarkView, ITest, '+test')
+        view = create_view(TestObject(), name='+test')
+        has_watermark = test_tales('view/macro:has-watermark', view=view)
+        self.assertIs(True, has_watermark)
+
 
 class PageMacroDispatcherInteractionTestCase(TestPageMacroDispatcherMixin,
                                              TestCaseWithFactory):

=== modified file 'lib/lp/app/templates/base-layout.pt'
--- lib/lp/app/templates/base-layout.pt	2012-03-14 06:58:58 +0000
+++ lib/lp/app/templates/base-layout.pt	2012-03-23 21:19:21 +0000
@@ -79,7 +79,8 @@
         <tal:login replace="structure context/@@login_status" />
       </div><!--id="locationbar"-->
 
-      <div id="watermark" class="watermark-apps-portlet">
+      <div id="watermark" class="watermark-apps-portlet"
+        tal:condition="view/macro:has-watermark">
         <div>
           <span tal:replace="structure view/watermark:logo"></span>
         </div>

=== modified file 'lib/lp/app/templates/root-index.pt'
--- lib/lp/app/templates/root-index.pt	2012-03-06 11:19:22 +0000
+++ lib/lp/app/templates/root-index.pt	2012-03-23 21:19:21 +0000
@@ -57,6 +57,13 @@
       <!-- Is your project registered yet? -->
 
       <div id="homepage" class="homepage">
+
+        <div class="top-portlet" style="border-bottom: 1px dotted #999;">
+          <img src="/@@/launchpad-logo-and-name.png"
+               id="launchpad-logo-and-name" alt=""
+               style="margin: 0 9em 1em 0"/>
+        </div>
+
         <div class="yui-g">
           <div class="yui-u first" style="margin-top: 1.5em;">
             <div class="homepage-whatslaunchpad"