Merge lp:~bac/launchpad/lp3-announcements into lp:launchpad

= Summary =

Update the <project>/+announcements page for 3.0 goodness, which is bug 414836.

Also solves:

Bug #175936: Announcements action (edit, publish & delete) links too close
 Remedy: links removed

Bug #253920: Project +announcements page should have a "Make announcement" link on it
 Remedy: add link in portlet

Bug #297518: "Created" doesn't make sense for announcements
 Remedy: reworded to 'written by'

== Proposed fix ==

Redesign the page to make individual announcements stand-out more. Removed editing
links which were problematic.

== Pre-implementation notes ==

Lots of helpful discussions with Curtis.

== Implementation details ==

As above.

== Tests ==

bin/test --vvm lp.registry -t announcement

== Demo and Q/A ==

Go to the following and ensure all looks right:
https://launchpad.dev/mozilla/+announcements
https://launchpad.dev/firefox/+announcements
https://launchpad.dev/kubuntu/+announcements

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/templates/hasannouncements-index.pt
  lib/lp/registry/stories/announcements/xx-announcements.txt
  lib/lp/registry/browser/announcement.py
  lib/lp/registry/templates/announcement-edit.pt
  lib/canonical/launchpad/icing/style-3-0.css
  lib/lp/registry/templates/announcement-listing-detailed.pt

== XmlLint notices ==

lib/lp/registry/templates/announcement-listing-detailed.pt
    1: namespace error : Namespace prefix tal for attributes on div is not defined
    <div class="portlet" tal:attributes="id string:${context/id}">
    ^
    3: namespace error : Namespace prefix tal for attributes on a is not defined
    <a tal:attributes="href string:${context/fmt:url}">
    ^
    5: namespace error : Namespace prefix tal for condition on date is not defined
    <tal:date tal:condition="context/date_announced">
    ^
    5: namespace error : Namespace prefix tal on date is not defined
    <tal:date tal:condition="context/date_announced">
    ^
    6: namespace error : Namespace prefix tal for replace on span is not defined
    <span tal:replace="context/date_announced/fmt:date"/>:
    ^
    8: namespace error : Namespace prefix tal on title is not defined
    <tal:title content="context/title">News item title</tal:title>
    ^
    14: namespace error : Namespace prefix tal on pillar is not defined
    <tal:pillar replace="structure context/target/fmt:link" />
    ^
    16: namespace error : Namespace prefix tal on registrant is not defined
    <tal:registrant replace="structure context/registrant/fmt:link" />.
    ^
    20: namespace error : Namespace prefix tal for replace on summary is not defined
    tal:replace="structure context/summary/fmt:text-to-html" />
    ^
    20: namespace error : Namespace prefix tal on summary is not defined
    tal:replace="structure context/summary/fmt:text-to-html" />
    ^
    22: namespace error : Namespace prefix tal on future is not defined
    <tal:future condition="context/future">
    ^
    24: namespace error : Namespace prefix tal for condition on strong is not define...

Hi Brad.

This is very nice. I think we should tweak the layout a bit before
landing this. I explain XMLLint's spazzing below.

> === modified file 'lib/canonical/launchpad/icing/style-3-0.css'
> --- lib/canonical/launchpad/icing/style-3-0.css 2009-08-17 11:22:39 +0000
> +++ lib/canonical/launchpad/icing/style-3-0.css 2009-08-17 20:36:01 +0000
...

> @@ -581,6 +581,10 @@
> border-top: 1px solid #d0d0d0;
> border-bottom: 1px solid #d0d0d0;
> }
> +.announcement .registered {
> + font-size: 95%;

This is not a support font-size you must use one of the percentages
from the table to guarantee that they display the same on all platforms.

    font-size: 93%;

is official.

> + margin-top: -2px

Missing semi-colon.

+ }

...

> === modified file 'lib/lp/registry/browser/announcement.py'
> --- lib/lp/registry/browser/announcement.py 2009-08-11 04:00:47 +0000
> +++ lib/lp/registry/browser/announcement.py 2009-08-17 20:36:01 +0000

...

> @@ -70,6 +69,12 @@
> text = 'Delete announcement'
> return Link('+delete', text, icon='trash-icon')
>
> + @enabled_with_permission('launchpad.Edit')
> + def announce(self):
> + text = 'Make announcement'
> + summary = 'Publish an item of news for this project'
> + return Link('+announce', text, summary, icon='add')

We use publish to describe the act of making public, not creating.

...

> @@ -100,6 +105,19 @@
> self.context = context
>
>
> +class IAnnouncementCreateMenu(Interface):
> + """A marker interface for creation announcement navigation menu."""
> +
> +
> +class AnnouncementCreateNavigationMenu(NavigationMenu, AnnouncementMenuMixin):
> + """A sub-menu for different aspects of modifying an announcement."""
> +
> + usedfor = IAnnouncementCreateMenu
> + facet = 'overview'
> + title = 'Create announcement'
> + links = ('announce', )xmlns:tal="http://xml.zope.org/namespaces/tal"

Menus assume that links are a mutable list. This works, but may lead to
cryptic errors if someone extends the menu,

...

> === modified file 'lib/lp/registry/stories/announcements/xx-announcements.txt'
> --- lib/lp/registry/stories/announcements/xx-announcements.txt 2009-08-13 18:10:15 +0000
> +++ lib/lp/registry/stories/announcements/xx-announcements.txt 2009-08-17 20:36:01 +0000

...

> @@ -418,47 +419,16 @@
> Editing announcements
> ---------------------
...

> -If you have the necessary privileges you will see links to edit the
> -announcements.
> +To edit an announcement you must go to the individual announcement
> +page and then follow the proper links.

Me? I am not doing anything. I think No Privileges Person is doing this.

> >>> priv_browser.open('http://launchpad.dev/tomcat/+announcements')
> - >>> listed_announcements = find_tag_by_id(
> - ... priv_browser.contents, 'announcements')
> - >>> edit_links = listed_announcements.fetch('a', text=edit_link_text)
> - >>> len(edit_links)
> - 2
> -
> -Following a link takes you to a form where you can change the headline,
> -summary and URL of the announcement:
> -
> - >>> 'Modified headline' not in announcements(priv_browser.contents)
> - True
> - ...

Thanks for the review Curtis.

On Aug 18, 2009, at 11:37 , Curtis Hovey wrote:

> Review: Needs Fixing code
> Hi Brad.
>
> This is very nice. I think we should tweak the layout a bit before
> landing this. I explain XMLLint's spazzing below.

Great.

>
>> === modified file 'lib/canonical/launchpad/icing/style-3-0.css'
>> --- lib/canonical/launchpad/icing/style-3-0.css 2009-08-17 11:22:39
>> +0000
>> +++ lib/canonical/launchpad/icing/style-3-0.css 2009-08-17 20:36:01
>> +0000
> ...
>
>> @@ -581,6 +581,10 @@
>> border-top: 1px solid #d0d0d0;
>> border-bottom: 1px solid #d0d0d0;
>> }
>> +.announcement .registered {
>> + font-size: 95%;
>
> This is not a support font-size you must use one of the percentages
> from the table to guarantee that they display the same on all
> platforms.
>
> font-size: 93%;
>
> is official.

Didn't see that comment in the CSS. Thanks for catching it.

>
>> + margin-top: -2px
>
> Missing semi-colon.
>

Duh.

> + }
>
> ...
>
>> === modified file 'lib/lp/registry/browser/announcement.py'
>> --- lib/lp/registry/browser/announcement.py 2009-08-11 04:00:47 +0000
>> +++ lib/lp/registry/browser/announcement.py 2009-08-17 20:36:01 +0000
>
> ...
>
>> @@ -70,6 +69,12 @@
>> text = 'Delete announcement'
>> return Link('+delete', text, icon='trash-icon')
>>
>> + @enabled_with_permission('launchpad.Edit')
>> + def announce(self):
>> + text = 'Make announcement'
>> + summary = 'Publish an item of news for this project'
>> + return Link('+announce', text, summary, icon='add')
>
> We use publish to describe the act of making public, not creating.

'Create an item...'

>
> ...
>
>> @@ -100,6 +105,19 @@
>> self.context = context
>>
>>
>> +class IAnnouncementCreateMenu(Interface):
>> + """A marker interface for creation announcement navigation
>> menu."""
>> +
>> +
>> +class AnnouncementCreateNavigationMenu(NavigationMenu,
>> AnnouncementMenuMixin):
>> + """A sub-menu for different aspects of modifying an
>> announcement."""
>> +
>> + usedfor = IAnnouncementCreateMenu
>> + facet = 'overview'
>> + title = 'Create announcement'
>> + links = ('announce', )xmlns:tal="http://xml.zope.org/namespaces/tal
>> "
>
> Menus assume that links are a mutable list. This works, but may lead
> to
> cryptic errors if someone extends the menu,

Really? From webapp/menu.py:

         assert isinstance(self.links, (tuple, list)), (
             "self.links must be a tuple or list.")

Just grepping through registry/browser/*.py I see a few other examples
of links being tuples. I've changed this one and the others in this
module.

>
> ...
>
>> === modified file 'lib/lp/registry/stories/announcements/xx-
>> announcements.txt'
>> --- lib/lp/registry/stories/announcements/xx-announcements.txt
>> 2009-08-13 18:10:15 +0000
>> +++ lib/lp/registry/stories/announcements/xx-announcements.txt
>> 2009-08-17 20:36:01 +0000
>
> ...
>
>> @@ -418,47 +419,16 @@
>> Editing announcements
>> ---------------------
> ...
>
>> -If you have the necessary privileges you will see links to edit the
>> -announcements.
>> +To edit an announcement you must go to the i...

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
--- lib/canonical/launchpad/icing/style-3-0.css 2009-08-17 20:36:01 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css 2009-08-18 15:40:42 +0000
@@ -582,8 +582,8 @@
     border-bottom: 1px solid #d0d0d0;
     }
 .announcement .registered {
- font-size: 95%;
- margin-top: -2px
+ font-size: 93%;
+ margin-top: -2px;
     }

 /* From nice_pre in tales.py */

=== modified file 'lib/lp/registry/browser/announcement.py'
--- lib/lp/registry/browser/announcement.py 2009-08-17 20:36:01 +0000
+++ lib/lp/registry/browser/announcement.py 2009-08-19 00:45:18 +0000
@@ -72,14 +72,14 @@
     @enabled_with_permission('launchpad.Edit')
     def announce(self):
         text = 'Make announcement'
- summary = 'Publish an item of news for this project'
+ summary = 'Create an item of news for this project'
         return Link('+announce', text, summary, icon='add')

 class AnnouncementContextMenu(ContextMenu, AnnouncementMenuMixin):
     """The menu for working with an Announcement."""
     usedfor = IAnnouncement
- links = ('edit', 'retarget', 'publish', 'retract', 'delete')
+ links = ['edit', 'retarget', 'publish', 'retract', 'delete']

 class IAnnouncementEditMenu(Interface):
@@ -92,7 +92,7 @@
     usedfor = IAnnouncementEditMenu
     facet = 'overview'
     title = 'Change announcement'
- links = ('edit', 'retarget', 'publish', 'retract', 'delete')
+ links = ['edit', 'retarget', 'publish', 'retract', 'delete']

     def __init__(self, context):
         super(AnnouncementEditNavigationMenu, self).__init__(context)
@@ -115,7 +115,7 @@
     usedfor = IAnnouncementCreateMenu
     facet = 'overview'
     title = 'Create announcement'
- links = ('announce', )
+ links = ['announce']

 class AnnouncementFormMixin:

=== modified file 'lib/lp/registry/stories/announcements/xx-announcements.txt'
--- lib/lp/registry/stories/announcements/xx-announcements.txt 2009-08-17 20:36:01 +0000
+++ lib/lp/registry/stories/announcements/xx-announcements.txt 2009-08-19 01:15:01 +0000
@@ -75,6 +75,8 @@

     >>> print priv_browser.url
     http://launchpad.dev/apache
+ >>> print priv_browser.title
+ Apache in Launchpad

 We'll repeat the process for Tomcat, an IProduct that is part of the
 Apache project, but this time we won't specify a URL, and we will
@@ -107,8 +109,8 @@
     ... name='field.publication_date.announcement_date').value = (
     ... '2007-11-24 09:00:00')
     >>> priv_browser.getControl('Make announcement').click()
- >>> print priv_browser.url
- http://launchpad.dev/tomcat
+ >>> print priv_browser.title
+ Tomcat in Launchpad

 And check out the results:

@@ -138,8 +140,8 @@
     >>> priv_browser.getControl('Summary').value = (
     ... 'Derby announcement summary')
     >>> priv_browser.getControl('Make announcement').click()
- >>> print priv_browser.url
- http://launchpad.dev/derby
+ >>> print priv_browser.title
+ Derby - Java Database in Launchpad
     >>> 'Derby announcement' in latest_news(priv_browser.contents)
     True

@@ -158,8 +160,8 @@
     ... name='field.publication_date.announce...

Hi Brad.

This looks good. I have a few remarks. Since we are adding the index
page to this I am marking this

   review needs_fixing

On Wed, 2009-08-19 at 00:45 +0000, Brad Crittenden wrote:
> Thanks for the review Curtis.
...
> class AnnouncementCreateNavigationMenu(NavigationMenu,
> >> AnnouncementMenuMixin):
> >> + """A sub-menu for different aspects of modifying an
> >> announcement."""
> >> +
> >> + usedfor = IAnnouncementCreateMenu
> >> + facet = 'overview'
> >> + title = 'Create announcement'
> >> + links = ('announce', )xmlns:tal="http://xml.zope.org/namespaces/tal
> >> "
> >
> > Menus assume that links are a mutable list. This works, but may lead
> > to
> > cryptic errors if someone extends the menu,
>
> Really? From webapp/menu.py:
>
> assert isinstance(self.links, (tuple, list)), (
> "self.links must be a tuple or list.")
>
> Just grepping through registry/browser/*.py I see a few other examples
> of links being tuples. I've changed this one and the others in this
> module.

Ah! Thank you for showing me this. I am now understand the problem was
not the use if a tuple, but that the engineer assumed the menu he was
extending was a mutable list. You can use a list or a tuple as you want.

...

> > + <a tal:condition="context/url"
> > + tal:attributes="href context/url">
> > + <img src="/@@/link" alt="Link" /> Read more</a>
> >
> > This does not look right. We use sprites. The icon used is for
> > offsite links
> > which these announcements are not. Maybe we need an new sprite for
> > announcement. I think we should report a bug and assign it to Martin.
>
> It is an off-site link. There is no sprite for an off-site link.
> Should we go that route or just leave this as is?

Yes, there is a sprite. The external links on the project page use it.
'external-link' which is the globe the link us showing now.

...
...

> class AnnouncementContextMenu(ContextMenu, AnnouncementMenuMixin):
> """The menu for working with an Announcement."""
> usedfor = IAnnouncement
> - links = ('edit', 'retarget', 'publish', 'retract', 'delete')
> + links = ['edit', 'retarget', 'publish', 'retract', 'delete']

I think we can delete this class. I do not think it will be used since
we have converted all the pages that use this with the index page

...

> === modified file 'lib/lp/registry/templates/hasannouncements-index.pt'
> --- lib/lp/registry/templates/hasannouncements-index.pt 2009-08-17 20:36:01 +0000
> +++ lib/lp/registry/templates/hasannouncements-index.pt 2009-08-19 00:43:48 +0000
> @@ -17,6 +17,7 @@
> <h1>News and announcements</h1>
> </tal:heading>
> <div metal:fill-slot="main">
> + <br />

This looks wrong. <br> are not meant to create blank lines. They so not
belong between block elements because block elements define margins and
padding.

I think you want the op content, or all the content wrapped in a
<div class="top-portlet"> which sets margins and padding.

--

__C U R T I S C. H O V E Y_______
Guilty of stealing everything I am.

On Aug 19, 2009, at 16:30 , Curtis Hovey wrote:

> Review: Needs Fixing
> Hi Brad.
>
> This looks good. I have a few remarks. Since we are adding the index
> page to this I am marking this
>
> review needs_fixing

Sure, that work is included in the diff I've got now.

>
> On Wed, 2009-08-19 at 00:45 +0000, Brad Crittenden wrote:
>
>>> + <a tal:condition="context/url"
>>> + tal:attributes="href context/url">
>>> + <img src="/@@/link" alt="Link" /> Read more</a>
>>>
>>> This does not look right. We use sprites. The icon used is for
>>> offsite links
>>> which these announcements are not. Maybe we need an new sprite for
>>> announcement. I think we should report a bug and assign it to
>>> Martin.
>>
>> It is an off-site link. There is no sprite for an off-site link.
>> Should we go that route or just leave this as is?
>
> Yes, there is a sprite. The external links on the project page use it.
> 'external-link' which is the globe the link us showing now.

Thanks for the pointer. I see it uses 'no-follow' too, which I
definitely should have here.

>
> ...
> ...
>
>> class AnnouncementContextMenu(ContextMenu, AnnouncementMenuMixin):
>> """The menu for working with an Announcement."""
>> usedfor = IAnnouncement
>> - links = ('edit', 'retarget', 'publish', 'retract', 'delete')
>> + links = ['edit', 'retarget', 'publish', 'retract', 'delete']
>
> I think we can delete this class. I do not think it will be used since
> we have converted all the pages that use this with the index page

Good call

>
> ...
>
>> === modified file 'lib/lp/registry/templates/hasannouncements-
>> index.pt'
>> --- lib/lp/registry/templates/hasannouncements-index.pt 2009-08-17
>> 20:36:01 +0000
>> +++ lib/lp/registry/templates/hasannouncements-index.pt 2009-08-19
>> 00:43:48 +0000
>> @@ -17,6 +17,7 @@
>> <h1>News and announcements</h1>
>> </tal:heading>
>> <div metal:fill-slot="main">
>> + <br />
>
> This looks wrong. <br> are not meant to create blank lines. They so
> not
> belong between block elements because block elements define margins
> and
> padding.
>
> I think you want the op content, or all the content wrapped in a
> <div class="top-portlet"> which sets margins and padding.

I inserted <div class="top-portlet" style="margin-top: 0.5em;"> which
seems to give a reasonable amount of spacing.

In the update I've also updated announcement-index.pt and made a fix
where the index page for a distribution was getting messed up due to
changes to HasAnnouncementsView (it is marked with an XXX that needs
to be removed when the update to distribution happens).

The diff is at:

http://pastebin.ubuntu.com/256050/

> The diff is at:
>
> http://pastebin.ubuntu.com/256050/

Wrong diff. The real one is at: http://pastebin.ubuntu.com/256052/

Thanks for the fixes and leaving me a XXX to complete my part of task. This is good to land.

There were additional test problems caused by the fact the distribution page has not been updated. The fixes here are temporary as the tests will be reworked in that effort, especially the distribution navigation tests.

Diff: http://pastebin.ubuntu.com/256282/

Screenshots for UI review may be found at: https://devpad.canonical.com/~bac/announcements/

So, you noticed that the bylines are styled wrong. If the problem is invasive, please file a bug a fix it soon. If it's not, please fix before landing. Otherwise, this looks really good.