Merge into devel : ui-convert-bug-activity-bug-412523 : Code : Launchpad itself

Reviewer	Review Type	Date Requested	Status
Celso Providelo (community)	code	2009-08-12	Approve on 2009-08-14
Michael Nelson (community)	ui	2009-08-12	Approve on 2009-08-13
Martin Albisetti		2009-08-12	Pending
Review via email: mp+10026@code.launchpad.net

Revision history for this message

Gavin Panella (allenap) wrote on 2009-08-12:

#

This branch converts bug-activity.pt to the new 3.0 base template according to the instructions at <https://dev.launchpad.net/VersionThreeDotO/UI/Conversion>. I am expecting to have to make more changes arising from this review, but that's what I want: it is the first bugs template to be converted, so it's also so I can figure out the process. Once I understand it better I will split up the work so the whole team can get to work on it.

I have before and after screenshots (also linked from the bug report):

http://launchpadlibrarian.net/30223879/before-mechanical-changes.png

http://launchpadlibrarian.net/30223898/after-mechanical-changes.png

My comments on the changes so far, taken from <https://bugs.edge.launchpad.net/malone/+bug/412523/comments/3>:

> The changes so far have been almost exclusively mechanical. I've
> chosen the main_only base template, and tried to apply the CSS
> rules. I can see the following issues with the new page:
>
> * "Mozilla Firefox" is repeated in the breadcrumbs and in the
> location-portlet.
>
> * The bug activity log has always been shown in bugtask
> context. The new application tabs block is much more prominent
> than the old and looks a bit daft in the activity log which is
> bug-related, not bugtask related.
>
>
> * Repetition of "bug #1" close together.
>
> * The table border butts against the divider at the bottom of the
> page, and creates the appearance of a double thickness border. A
> margin-bottom on that table would fix it, but that smells a
> but. Is there already a way to handle this "nicely"?

Test: ./bin/test -vvt xx-bug-activity

Lint free.

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2009-08-12:

#

Download full text (7.2 KiB)

Hi Gavin,

Great start! It's a nice simple informational page without actions so a good one to begin with!

I've marked it as needs fixing, just to sort out the portlet divs etc.

> This branch converts bug-activity.pt to the new 3.0 base template according to
> the instructions at
> <https://dev.launchpad.net/VersionThreeDotO/UI/Conversion>. I am expecting to
> have to make more changes arising from this review, but that's what I want: it
> is the first bugs template to be converted, so it's also so I can figure out
> the process. Once I understand it better I will split up the work so the whole
> team can get to work on it.

Great idea!

>
> I have before and after screenshots (also linked from the bug report):
>
> http://launchpadlibrarian.net/30223879/before-mechanical-changes.png
>
> http://launchpadlibrarian.net/30223898/after-mechanical-changes.png

Thanks for that! So much easier... and great to enable discussion on the bug too!

I'll respond to your thoughts below, and then summarise more generally at the bottom...

>
> My comments on the changes so far, taken from
> <https://bugs.edge.launchpad.net/malone/+bug/412523/comments/3>:
>
> > The changes so far have been almost exclusively mechanical. I've
> > chosen the main_only base template, and tried to apply the CSS
> > rules. I can see the following issues with the new page:
> >
> > * "Mozilla Firefox" is repeated in the breadcrumbs and in the
> > location-portlet.

The breadcrumbs will be moving to directly below your h1 heading, and will be less prominent. I personally don't see the repetition as a problem in the context of breadcrumbs (ie. they will always display the path of your traversal by necessity).

> >
> > * The bug activity log has always been shown in bugtask
> > context. The new application tabs block is much more prominent
> > than the old and looks a bit daft in the activity log which is
> > bug-related, not bugtask related.

Hmmm... But the application tabs block is showing the user the related pillar-or-person - in this case the product. So I'm not sure why you think it's confusing in either bug/bugtask context? But I'm not sure I've properly understood your point here.

> >
> >
> > * Repetition of "bug #1" close together.

Yes, but as we chatted, I really think that 'bug #1' link should be part of the breadcrumb...

Firefox > Bug #1

Perhaps you should chat with salgado and find out if it will indeed be included in the new breadcrumb (or if he can ensure that it is). In which case, I'd be for removing it.

> >
> > * The table border butts against the divider at the bottom of the
> > page, and creates the appearance of a double thickness border. A
> > margin-bottom on that table would fix it, but that smells a
> > but. Is there already a way to handle this "nicely"?

I think this is just because you don't have the table in a portlet div. You seem to have just the top-portlet which has the link.

I think you should either have one single top-portlet (my preference), or if you need to leave the link in and you think it should be in a separate portlet, then you'd need to wrap your table in a portlet div too, but I really think you hav...

Hi Gavin,

Great start! It's a nice simple informational page without actions so a good one to begin with!

I've marked it as needs fixing, just to sort out the portlet divs etc.

> This branch converts bug-activity.pt to the new 3.0 base template according to
> the instructions at
> <https://dev.launchpad.net/VersionThreeDotO/UI/Conversion>. I am expecting to
> have to make more changes arising from this review, but that's what I want: it
> is the first bugs template to be converted, so it's also so I can figure out
> the process. Once I understand it better I will split up the work so the whole
> team can get to work on it.

Great idea!

> 
> I have before and after screenshots (also linked from the bug report):
> 
>   http://launchpadlibrarian.net/30223879/before-mechanical-changes.png
> 
>   http://launchpadlibrarian.net/30223898/after-mechanical-changes.png

Thanks for that! So much easier... and great to enable discussion on the bug too!

I'll respond to your thoughts below, and then summarise more generally at the bottom...

> 
> My comments on the changes so far, taken from
> <https://bugs.edge.launchpad.net/malone/+bug/412523/comments/3>:
> 
> > The changes so far have been almost exclusively mechanical. I've
> > chosen the main_only base template, and tried to apply the CSS
> > rules. I can see the following issues with the new page:
> >
> >  * "Mozilla Firefox" is repeated in the breadcrumbs and in the
> >    location-portlet.

The breadcrumbs will be moving to directly below your h1 heading, and will be less prominent. I personally don't see the repetition as a problem in the context of breadcrumbs (ie. they will always display the path of your traversal by necessity).

> >
> >  * The bug activity log has always been shown in bugtask
> >    context. The new application tabs block is much more prominent
> >    than the old and looks a bit daft in the activity log which is
> >    bug-related, not bugtask related.

Hmmm... But the application tabs block is showing the user the related pillar-or-person - in this case the product. So I'm not sure why you think it's confusing in either bug/bugtask context? But I'm not sure I've properly understood your point here.

> >
> >
> >  * Repetition of "bug #1" close together.

Yes, but as we chatted, I really think that 'bug #1' link should be part of the breadcrumb...

Firefox > Bug #1

Perhaps you should chat with salgado and find out if it will indeed be included in the new breadcrumb (or if he can ensure that it is). In which case, I'd be for removing it.

> >
> >  * The table border butts against the divider at the bottom of the
> >    page, and creates the appearance of a double thickness border. A
> >    margin-bottom on that table would fix it, but that smells a
> >    but. Is there already a way to handle this "nicely"?

I think this is just because you don't have the table in a portlet div. You seem to have just the top-portlet which has the link.

I think you should either have one single top-portlet (my preference), or if you need to leave the link in and you think it should be in a separate portlet, then you'd need to wrap your table in a portlet div too, but I really think you have just one piece of information on that page (the activity log).
> 
> Test: ./bin/test -vvt xx-bug-activity
> 
> Lint free.

So, other general thoughts:
 * The date formatting (which beuno mentioned)
 * Assuming the breadcrumb will include Bug #1, would it make sense to update the H1 to instead include the name of the bug rather than the number (or in addition to)?
 * Given that this page is traversable by other products (and the activity is related to other products as you said), I'd expect to see "Other related products" or something on this page, when there are some. I realise this wouldn't be part of the mechanical update, but it would be great to do. At least an XXX for a bug?

OK, sorry I can't be more thorough, but I literally need to run! Look forward to seeing your thoughts.

<noodles775> allenap: I like the way you're doing the whole process through the bug... allowing anyone to comment as you go :)
<allenap> noodles775: Release early :)
<noodles775> Is there a reason why you put the para with the formatted link in a separate portlet?
<noodles775> ie. why not just have one portlet with the formatted link followed by the table?
* matsubara-lunch has quit (Read error: 110 (Connection timed out))
<allenap> noodles775: I got the impression from the wiki page that I should do something like that. The link isn't directly related to the table; it doesn't describe the table, or alter it, so it sort-of seemed like a distinct piece of content.
<noodles775> allenap: actually, I don't think you need the yui-g div either, as you're not using any grid here (it's only needed if you've got child-divs that you want in a column layout afaik).
<noodles775> Hmm.
<noodles775> Yep, I can see why you'd say that, but that should also then mean that the table should be in a portlet div (but that woudl put a horizontal line between them right?)
* deryck is now known as deryck[lunch]
<allenap> noodles775: Would it? Ah, so portlet divs contain yui-u and yui-g divs?
<noodles775> Other way around, if you are using a grid layout then your portlet div will be a *child* of the yui-u div.
<noodles775> (That was a recent change aiui, so that portlets could be re-usable on different pages without inheriting the yui layout)
<allenap> noodles775: Right, so something like: div.yui-g > div.top-portlet > (link and table)
<noodles775> allenap: yes, although I've just realised what that link actually is... it really should be in the breadcrumbs right?
<sinzui> salgado: beuno: I approved the projectgroup page. There are some minor things that can be fixed, but I think we should land it today and file bugs about issues that can be fixed after the distribution layout is updated.
<allenap> noodles775: I guess so. Something like: Mozilla Firefox > Bug #1234 > Activity Log   -- even though the Mozilla Firefox bit is a bit smelly.
<mup> Bug #1234: Gina is an unmaintainable mess of command line options, environment variables and shell scripts <Launchpad Foundations:Fix Released by debonzi> <https://launchpad.net/bugs/1234>
<beuno> sinzui, woooooooooooooooooooo
<noodles775> allenap: Yeah, exactly. Why do you think that including the product as the base of the breadcrumbs is smelly?
<allenap> noodles775: When there's more than one bug task it implies (to me) that the activity is relevant only to that product, but it's actually bug activity for all bug tasks.
<beuno> allenap, the date is also formatted in a way that it isn't elsewhere
<allenap> beuno: Ah, good spot. I'll use a standard formatter.
<noodles775> allenap: Sure, but doesn't that mean that the same activity log is traversable via a different url?
<allenap> noodles775: It is :)
<noodles775> So I think that's fine then for the breadcrumb - it's representing the path to how the user got here, but given what you said, as a user I would then also expect to see other products affected here (if there are any?)
<noodles775> (but that wouldn't be part of a mechanical change :) ).
<beuno> well, mechanical changes are a loose term  :)
<noodles775> lol
<noodles775> OK, I'll summarise in the MP my other thoughts so I can see both yours and beuno's feedback (as I have to go soon).

review: Needs Fixing (ui)

Revision history for this message

Gavin Panella (allenap) wrote on 2009-08-13:

#

Thanks for the review Michael!

I've add a BugTask breadcrumb, that just says "Bug #X", and tests in
lp/bugs/test/test_breadcrumbs.py which is ripped off from salgado's
work on breadcrumbs.

> > > * The table border butts against the divider at the bottom of the
> > > page, and creates the appearance of a double thickness border. A
> > > margin-bottom on that table would fix it, but that smells a
> > > but. Is there already a way to handle this "nicely"?
>
> I think this is just because you don't have the table in a portlet div. You
> seem to have just the top-portlet which has the link.
>
> I think you should either have one single top-portlet (my preference), or if
> you need to leave the link in and you think it should be in a separate
> portlet, then you'd need to wrap your table in a portlet div too, but I really
> think you have just one piece of information on that page (the activity log).

I've added a single top-portlet div.

> So, other general thoughts:
> * The date formatting (which beuno mentioned)

Fixed, though I'm not using fmt:datetime because of bug 412963
(fmt:datetime displays date and time in current timezone, even when
UTC only is desired). I've added an XXX to the template.

> * Assuming the breadcrumb will include Bug #1, would it make sense to update
> the H1 to instead include the name of the bug rather than the number (or in
> addition to)?

Yes, the bug title is now in the heading.

> * Given that this page is traversable by other products (and the activity is
> related to other products as you said), I'd expect to see "Other related
> products" or something on this page, when there are some. I realise this
> wouldn't be part of the mechanical update, but it would be great to do. At
> least an XXX for a bug?

This is a weird one. The presence of the product heading doesn't
always make sense at first on the activity log page (or many other
pages that are found below the bug page). Once the breadcrumbs are
moved to below the product heading it will make a bit more sense. But
I'm not sure what value there is in drawing attention to the fact that
this activity log page can be seen in the context of another product;
the main page content will be identical. This is definitely something
the bugs team are going to have to talk about as part of our 3.0
redesign.

I've attached a couple of diffs showing the changes I've made. I had
to merge devel in the middle of my changes because the tree would no
longer build (and the bin/buildout workaround was also not working), I
made some whitespace changes, and I also fixed some lint, none of
which is interesting here.

Thanks for the review Michael!

I've add a BugTask breadcrumb, that just says "Bug #X", and tests in
lp/bugs/test/test_breadcrumbs.py which is ripped off from salgado's
work on breadcrumbs.

> > >  * The table border butts against the divider at the bottom of the
> > >    page, and creates the appearance of a double thickness border. A
> > >    margin-bottom on that table would fix it, but that smells a
> > >    but. Is there already a way to handle this "nicely"?
>
> I think this is just because you don't have the table in a portlet div. You
> seem to have just the top-portlet which has the link.
>
> I think you should either have one single top-portlet (my preference), or if
> you need to leave the link in and you think it should be in a separate
> portlet, then you'd need to wrap your table in a portlet div too, but I really
> think you have just one piece of information on that page (the activity log).

I've added a single top-portlet div.

> So, other general thoughts:
>  * The date formatting (which beuno mentioned)

Fixed, though I'm not using fmt:datetime because of bug 412963
(fmt:datetime displays date and time in current timezone, even when
UTC only is desired). I've added an XXX to the template.

>  * Assuming the breadcrumb will include Bug #1, would it make sense to update
> the H1 to instead include the name of the bug rather than the number (or in
> addition to)?

Yes, the bug title is now in the heading.

>  * Given that this page is traversable by other products (and the activity is
> related to other products as you said), I'd expect to see "Other related
> products" or something on this page, when there are some. I realise this
> wouldn't be part of the mechanical update, but it would be great to do. At
> least an XXX for a bug?

This is a weird one. The presence of the product heading doesn't
always make sense at first on the activity log page (or many other
pages that are found below the bug page). Once the breadcrumbs are
moved to below the product heading it will make a bit more sense. But
I'm not sure what value there is in drawing attention to the fact that
this activity log page can be seen in the context of another product;
the main page content will be identical. This is definitely something
the bugs team are going to have to talk about as part of our 3.0
redesign.

I've attached a couple of diffs showing the changes I've made. I had
to merge devel in the middle of my changes because the tree would no
longer build (and the bin/buildout workaround was also not working), I
made some whitespace changes, and I also fixed some lint, none of
which is interesting here.

9095-9097.diff

9098-9102.diff

Revision history for this message

Gavin Panella (allenap) wrote on 2009-08-13:

#

Also see the screenshot of what it looks like now:

http://launchpadlibrarian.net/30272857/post-review.png

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2009-08-13:

#

> Also see the screenshot of what it looks like now:
>
> http://launchpadlibrarian.net/30272857/post-review.png

Thanks Gavin, for the screenshot and for adding the breadcrumb - looks great!

So, as we discussed, r=me assuming that:

1. The lack of padding between the heading and the table will be fixed by either the placement of the new breadcrumbs or a general css rule to apply padding to tables within portlets (as you've correctly got it in a top-portlet now), and,

2. The table itself will be redesigned as part of the bugs ui work later, and will include a link to the relevant product's bugtask for an activity log entry when it is different from the product displayed in the traversal (and above the app. tabs).

-M.

review: Approve (ui)

Revision history for this message

Celso Providelo (cprov) wrote on 2009-08-13:

#

Download full text (8.1 KiB)

> === modified file 'lib/lp/bugs/browser/bugtask.py'
> --- lib/lp/bugs/browser/bugtask.py 2009-08-10 21:05:59 +0000
> +++ lib/lp/bugs/browser/bugtask.py 2009-08-13 16:47:38 +0000
> @@ -14,6 +14,7 @@
> 'BugTargetView',
> 'BugTaskContextMenu',
> 'BugTaskCreateQuestionView',
> + 'BugTaskBreadcrumbBuilder',
> 'BugTaskEditView',
> 'BugTaskExpirableListingView',
> 'BugTaskListingItem',
> @@ -45,7 +46,6 @@
> import re
> from simplejson import dumps
> import urllib
> -from urlparse import urlparse, urlunparse
> from operator import attrgetter, itemgetter
>
> from zope import component
> @@ -116,6 +116,7 @@
> from lp.registry.interfaces.sourcepackage import ISourcePackage
> from canonical.launchpad.interfaces.validation import (
> valid_upstreamtask, validate_distrotask)
> +from canonical.launchpad.webapp.breadcrumb import BreadcrumbBuilder
> from canonical.launchpad.webapp.interfaces import (
> ILaunchBag, NotFoundError, UnexpectedFormData)
>
> @@ -3490,3 +3491,10 @@
> return_dict[key] = cgi.escape(return_dict[key])
>
> return "%(old_value)s → %(new_value)s" % return_dict
> +
> +
> +class BugTaskBreadcrumbBuilder(BreadcrumbBuilder):
> + """Builds a breadcrumb for an `IBugTask`."""
> + @property
> + def text(self):
> + return self.context.bug.displayname
>

Nice! It will look much nicer when we have the breadcrumbs below the
heading section.

> === modified file 'lib/lp/bugs/configure.zcml'
> --- lib/lp/bugs/configure.zcml 2009-07-23 13:44:13 +0000
> +++ lib/lp/bugs/configure.zcml 2009-08-13 16:47:38 +0000
> @@ -263,6 +263,12 @@
> <adapter
> factory="lp.bugs.browser.bugtask.assignee_renderer"
> name="assignee"/>
> + <adapter
> + provides="canonical.launchpad.webapp.interfaces.IBreadcrumbBuilder"
> + for="lp.bugs.interfaces.bugtask.IBugTask"
> + factory="lp.bugs.browser.bugtask.BugTaskBreadcrumbBuilder"
> + permission="zope.Public"/>
> +
>
> 
>
>

It looks like you have TABs before and after this comment.

> === modified file 'lib/lp/bugs/stories/bugs/xx-bug-activity.txt'
> --- lib/lp/bugs/stories/bugs/xx-bug-activity.txt 2009-08-06 13:51:38 +0000
> +++ lib/lp/bugs/stories/bugs/xx-bug-activity.txt 2009-08-13 16:47:38 +0000
> @@ -7,16 +7,19 @@
> ... 'mozilla-firefox/+bug/3/+activity')
> >>> main_content = find_main_content(anon_browser.contents)
>
> -The page heading contains a link back to the bug page.
> +The page contains a link back to the bug page:
>
> - >>> bug_link = main_content.p.a
> - >>> print '%s --> %s' % (extract_text(bug_link), bug_link['href'])
> - Bug #3: Bug Title Test --> /debian/+source/mozilla-firefox/+bug/3
> + >>> document = find_tag_by_id(anon_browser.contents, 'document')
> + >>> bug_link = document.find(
> + ... 'a', href=lambda href: href.endswith('/+bug/3'))
> + >>> print '%s\n--> %s' % (extract_text(bug_link), bug_link['href'])
> + Bug #3
> + --> http://bugs.launchpad.dev/debian/+source/mozilla-firefox/+bug/3
>

I wonder if there would be any value in using print_locatio...

> === modified file 'lib/lp/bugs/browser/bugtask.py'
> --- lib/lp/bugs/browser/bugtask.py	2009-08-10 21:05:59 +0000
> +++ lib/lp/bugs/browser/bugtask.py	2009-08-13 16:47:38 +0000
> @@ -14,6 +14,7 @@
>      'BugTargetView',
>      'BugTaskContextMenu',
>      'BugTaskCreateQuestionView',
> +    'BugTaskBreadcrumbBuilder',
>      'BugTaskEditView',
>      'BugTaskExpirableListingView',
>      'BugTaskListingItem',
> @@ -45,7 +46,6 @@
>  import re
>  from simplejson import dumps
>  import urllib
> -from urlparse import urlparse, urlunparse
>  from operator import attrgetter, itemgetter
>
>  from zope import component
> @@ -116,6 +116,7 @@
>  from lp.registry.interfaces.sourcepackage import ISourcePackage
>  from canonical.launchpad.interfaces.validation import (
>      valid_upstreamtask, validate_distrotask)
> +from canonical.launchpad.webapp.breadcrumb import BreadcrumbBuilder
>  from canonical.launchpad.webapp.interfaces import (
>      ILaunchBag, NotFoundError, UnexpectedFormData)
>
> @@ -3490,3 +3491,10 @@
>                  return_dict[key] = cgi.escape(return_dict[key])
>
>          return "%(old_value)s &#8594; %(new_value)s" % return_dict
> +
> +
> +class BugTaskBreadcrumbBuilder(BreadcrumbBuilder):
> +    """Builds a breadcrumb for an `IBugTask`."""
> +    @property
> +    def text(self):
> +        return self.context.bug.displayname
>

Nice! It will look much nicer when we have the breadcrumbs below the
heading section.

> === modified file 'lib/lp/bugs/configure.zcml'
> --- lib/lp/bugs/configure.zcml	2009-07-23 13:44:13 +0000
> +++ lib/lp/bugs/configure.zcml	2009-08-13 16:47:38 +0000
> @@ -263,6 +263,12 @@
>          <adapter
>              factory="lp.bugs.browser.bugtask.assignee_renderer"
>              name="assignee"/>
> +        <adapter
> +            provides="canonical.launchpad.webapp.interfaces.IBreadcrumbBuilder"
> +            for="lp.bugs.interfaces.bugtask.IBugTask"
> +            factory="lp.bugs.browser.bugtask.BugTaskBreadcrumbBuilder"
> +            permission="zope.Public"/>
> +
>
>          
>
>

It looks like you have TABs before and after this comment.

> === modified file 'lib/lp/bugs/stories/bugs/xx-bug-activity.txt'
> --- lib/lp/bugs/stories/bugs/xx-bug-activity.txt	2009-08-06 13:51:38 +0000
> +++ lib/lp/bugs/stories/bugs/xx-bug-activity.txt	2009-08-13 16:47:38 +0000
> @@ -7,16 +7,19 @@
>      ...     'mozilla-firefox/+bug/3/+activity')
>      >>> main_content = find_main_content(anon_browser.contents)
>
> -The page heading contains a link back to the bug page.
> +The page contains a link back to the bug page:
>
> -    >>> bug_link = main_content.p.a
> -    >>> print '%s --> %s' % (extract_text(bug_link), bug_link['href'])
> -    Bug #3: Bug Title Test --> /debian/+source/mozilla-firefox/+bug/3
> +    >>> document = find_tag_by_id(anon_browser.contents, 'document')
> +    >>> bug_link = document.find(
> +    ...     'a', href=lambda href: href.endswith('/+bug/3'))
> +    >>> print '%s\n--> %s' % (extract_text(bug_link), bug_link['href'])
> +    Bug #3
> +    --> http://bugs.launchpad.dev/debian/+source/mozilla-firefox/+bug/3
>

I wonder if there would be any value in using print_location() helper
here. it prints the hierarchy, not exactly the links, but we know the
links are properly rendered by BreadcrumbBuilder (we have specific
tests for that).

What do you think ?

Shouldn't we include the timezone information on those dates (
... UTC) ?

Ideally it should be possible to traverse the existing datetime
formatter with a '/utc' or something similar, so callsites could
decide between the user TZ or UTC.

>  == Activity interleaved with comments ==
>
> === modified file 'lib/lp/bugs/templates/bug-activity.pt'

Looks good!

> === added file 'lib/lp/bugs/tests/test_breadcrumbs.py'
> --- lib/lp/bugs/tests/test_breadcrumbs.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/bugs/tests/test_breadcrumbs.py	2009-08-13 16:47:38 +0000
> @@ -0,0 +1,71 @@
> +# Copyright 2009 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__metaclass__ = type
> +
> +import unittest
> +
> +from zope.component import getMultiAdapter
> +
> +from canonical.lazr.testing.menus import make_fake_request
> +from canonical.launchpad.webapp.publisher import RootObject
> +from canonical.testing import DatabaseFunctionalLayer
> +from lp.testing import TestCaseWithFactory
> +
> +
> +class TestBugTaskBreadcrumbBuilder(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def setUp(self):
> +        TestCaseWithFactory.setUp(self)
> +        self.product = self.factory.makeProduct(
> +            name='crumb-tester', displayname="Crumb Tester")
> +        self.bug = self.factory.makeBug(product=self.product)
> +        self.root = RootObject()
> +
> +    def test_bugtask_on_mainsite(self):
> +        request = make_fake_request(
> +            'http://launchpad.dev/%s/+bug/%d' % (
> +                self.product.name, self.bug.id),
> +            [self.root, self.product, self.bug.default_bugtask])
> +        hierarchy = getMultiAdapter((self.root, request), name='+hierarchy')
> +        self.assertEquals(
> +            ['http://launchpad.dev/crumb-tester',
> +             'http://launchpad.dev/crumb-tester/+bug/%d' % self.bug.id],
> +            [crumb.url for crumb in hierarchy.items()])
> +        self.assertEquals(
> +            ["Crumb Tester", "Bug #%d" % self.bug.id],
> +            [crumb.text for crumb in hierarchy.items()])
> +
> +    def test_bugtask_on_vhost(self):
> +        request = make_fake_request(
> +            'http://bugs.launchpad.dev/%s/+bug/%d' % (
> +                self.product.name, self.bug.id),
> +            [self.root, self.product, self.bug.default_bugtask])
> +        hierarchy = getMultiAdapter((self.root, request), name='+hierarchy')
> +        self.assertEquals(
> +            ['http://bugs.launchpad.dev/crumb-tester',
> +             'http://bugs.launchpad.dev/crumb-tester/+bug/%d' % self.bug.id],
> +            [crumb.url for crumb in hierarchy.items()])
> +        self.assertEquals(
> +            ["Crumb Tester", "Bug #%d" % self.bug.id],
> +            [crumb.text for crumb in hierarchy.items()])
> +
> +    def test_bugtask_child_on_vhost(self):
> +        request = make_fake_request(
> +            'http://bugs.launchpad.dev/%s/+bug/%d/+activity' % (
> +                self.product.name, self.bug.id),
> +            [self.root, self.product, self.bug.default_bugtask])
> +        hierarchy = getMultiAdapter((self.root, request), name='+hierarchy')
> +        self.assertEquals(
> +            ['http://bugs.launchpad.dev/crumb-tester',
> +             'http://bugs.launchpad.dev/crumb-tester/+bug/%d' % self.bug.id],
> +            [crumb.url for crumb in hierarchy.items()])
> +        self.assertEquals(
> +            ["Crumb Tester", "Bug #%d" % self.bug.id],
> +            [crumb.text for crumb in hierarchy.items()])
> +
> +
> +def test_suite():
> +    return unittest.TestLoader().loadTestsFromName(__name__)
>

Nice test! Thank you.

Two issues for for your consideration, none of them are blockers for
landing, IMO.

1. use print_location() on the pagetest, for readability.

2. Judge if including the timezone information on the local datetime
    formatter is a good move (UI, in fact, not code)

review: merge-conditional

review: Approve

Revision history for this message

Gavin Panella (allenap) wrote on 2009-08-14:

#

Download full text (4.4 KiB)

On Thu, 13 Aug 2009 17:09:06 -0000
Celso Providelo <email address hidden> wrote:

> Review: Approve

Woohoo! Thank you :)

...
> > === modified file 'lib/lp/bugs/configure.zcml'
> > --- lib/lp/bugs/configure.zcml 2009-07-23 13:44:13 +0000
> > +++ lib/lp/bugs/configure.zcml 2009-08-13 16:47:38 +0000
> > @@ -263,6 +263,12 @@
> > <adapter
> > factory="lp.bugs.browser.bugtask.assignee_renderer"
> > name="assignee"/>
> > + <adapter
> > + provides="canonical.launchpad.webapp.interfaces.IBreadcrumbBuilder"
> > + for="lp.bugs.interfaces.bugtask.IBugTask"
> > + factory="lp.bugs.browser.bugtask.BugTaskBreadcrumbBuilder"
> > + permission="zope.Public"/>
> > +
> >
> > 
> >
> >
>
> It looks like you have TABs before and after this comment.

All of the configure.zcml files generated during the move from c/l to
lp/* are chock full of weird whitespace. I started a utility to clean
this up a while back, but have not finished it. If I fix this comment,
I need to fix all the whitespace in this file, so I'd like to leave it
for now, until I have an automated solution.

>
> > === modified file 'lib/lp/bugs/stories/bugs/xx-bug-activity.txt'
> > --- lib/lp/bugs/stories/bugs/xx-bug-activity.txt 2009-08-06 13:51:38 +0000
> > +++ lib/lp/bugs/stories/bugs/xx-bug-activity.txt 2009-08-13 16:47:38 +0000
> > @@ -7,16 +7,19 @@
> > ... 'mozilla-firefox/+bug/3/+activity')
> > >>> main_content = find_main_content(anon_browser.contents)
> >
> > -The page heading contains a link back to the bug page.
> > +The page contains a link back to the bug page:
> >
> > - >>> bug_link = main_content.p.a
> > - >>> print '%s --> %s' % (extract_text(bug_link), bug_link['href'])
> > - Bug #3: Bug Title Test --> /debian/+source/mozilla-firefox/+bug/3
> > + >>> document = find_tag_by_id(anon_browser.contents, 'document')
> > + >>> bug_link = document.find(
> > + ... 'a', href=lambda href: href.endswith('/+bug/3'))
> > + >>> print '%s\n--> %s' % (extract_text(bug_link), bug_link['href'])
> > + Bug #3
> > + --> http://bugs.launchpad.dev/debian/+source/mozilla-firefox/+bug/3
> >
>
> I wonder if there would be any value in using print_location() helper
> here. it prints the hierarchy, not exactly the links, but we know the
> links are properly rendered by BreadcrumbBuilder (we have specific
> tests for that).
>
> What do you think ?

I've never used print_location() before, but you're right, it fits the
bill nicely. I've had to modify it to work with the new application
tabs, so maybe you could have a look at the attached diff just to make
sure it's okay?

>
> > The title repeats the bug number for clarity.
> >
> > >>> print extract_text(main_content.h1)
> > - Activity log for bug #3
> > + Activity log for bug #3 - Bug Title Test
> >
> > The activity log itself is presented as a table.
> >
> > @@ -27,9 +30,9 @@
> > >>> for row in main_content.table('tr'):
> > ... print_row(row)
> > Date | Who | What changed | Old value | New value | Message
> > - 10 Aug 05 16:30 | Sample Person | bug | | | assigne...

 === modified file 'lib/canonical/launchpad/testing/pages.py'
 --- lib/canonical/launchpad/testing/pages.py	2009-06-25 05:30:52 +0000
 +++ lib/canonical/launchpad/testing/pages.py	2009-08-14 09:19:59 +0000
@@ -542,15 +542,26 @@
  def print_location_apps(contents):
      """Print the application tabs' text and URL."""
      location_apps = find_tag_by_id(contents, 'lp-apps')
--    for tab in location_apps.findAll('span'):
--        tab_text = extract_text(tab)
--        if tab['class'].find('active') != -1:
--            tab_text += ' (selected)'
--        if tab.a:
--            link = tab.a['href']
--        else:
--            link = 'not linked'
--        print "* %s - %s" % (tab_text, link)
++    if location_apps is None:
++        location_apps = first_tag_by_class(contents, 'location-portlet')
++        if location_apps is not None:
++            location_apps = location_apps.ul.findAll('li')
++    else:
++        location_apps = location_apps.findAll('span')
++    if location_apps is None:
++        print "(Application tabs omitted)"
++    elif len(location_apps) == 0:
++        print "(No application tabs)"
++    else:
++        for tab in location_apps:
++            tab_text = extract_text(tab)
++            if tab['class'].find('active') != -1:
++                tab_text += ' (selected)'
++            if tab.a:
++                link = tab.a['href']
++            else:
++                link = 'not linked'
++            print "* %s - %s" % (tab_text, link)
  def print_tag_with_id(contents, id):
 === modified file 'lib/lp/bugs/stories/bugs/xx-bug-activity.txt'
 --- lib/lp/bugs/stories/bugs/xx-bug-activity.txt	2009-08-13 12:59:41 +0000
 +++ lib/lp/bugs/stories/bugs/xx-bug-activity.txt	2009-08-14 09:20:08 +0000
@@ -7,19 +7,20 @@
      ...     'mozilla-firefox/+bug/3/+activity')
      >>> main_content = find_main_content(anon_browser.contents)
--The page contains a link back to the bug page:
--
--    >>> document = find_tag_by_id(anon_browser.contents, 'document')
--    >>> bug_link = document.find(
--    ...     'a', href=lambda href: href.endswith('/+bug/3'))
--    >>> print '%s\n--> %s' % (extract_text(bug_link), bug_link['href'])
--    Bug #3
--    --> http://bugs.launchpad.dev/debian/+source/mozilla-firefox/+bug/3
--
--The title repeats the bug number for clarity.
--
--    >>> print extract_text(main_content.h1)
--    Activity log for bug #3 - Bug Title Test
++
++The page contains a link back to the bug page in the breadcrumbs and
++the main heading repeats the bug number for clarity:
++
++    >>> print_location(anon_browser.contents)
++    Hierarchy: Launchpad > Debian > ?mozilla-firefox? package > Bug #3
++    Tabs:
++    * Overview - http://launchpad.dev/debian/+source/mozilla-firefox
++    * Code - http://code.launchpad.dev/debian/+source/mozilla-firefox
++    * Bugs (selected) - http://bugs.launchpad.dev/debian/+source/mozilla-firefox
++    * Blueprints - not linked
++    * Translations - not linked
++    * Answers - http://answers.launchpad.dev/debian/+source/mozilla-firefox
++    Main heading: Activity log for bug #3: Bug Title Test
  The activity log itself is presented as a table.
 === modified file 'lib/lp/bugs/templates/bug-activity.pt'
 --- lib/lp/bugs/templates/bug-activity.pt	2009-08-13 14:06:31 +0000
 +++ lib/lp/bugs/templates/bug-activity.pt	2009-08-13 14:25:35 +0000
@@ -7,7 +7,7 @@
    <div metal:fill-slot="heading">
      <h1>
--      Activity log for bug #<span tal:replace="context/bug/id" /> -
++      Activity log for bug #<span tal:replace="context/bug/id" />:
        <span tal:replace="context/bug/title" />
      </h1>
    </div>

 === modified file 'lib/lp/bugs/browser/bugtask.py'
 --- lib/lp/bugs/browser/bugtask.py	2009-08-13 14:07:18 +0000
 +++ lib/lp/bugs/browser/bugtask.py	2009-08-14 16:30:27 +0000
@@ -62,6 +62,7 @@
  from zope.schema.interfaces import IContextSourceBinder, IList
  from zope.schema.vocabulary import (
      getVocabularyRegistry, SimpleVocabulary, SimpleTerm)
++from zope.security.interfaces import Unauthorized
  from zope.security.proxy import (
      isinstance as zope_isinstance, removeSecurityProxy)
  from zope.traversing.interfaces import IPathAdapter
@@ -3495,6 +3496,18 @@
  class BugTaskBreadcrumbBuilder(BreadcrumbBuilder):
      """Builds a breadcrumb for an `IBugTask`."""
++
      @property
      def text(self):
          return self.context.bug.displayname
++
++    def make_breadcrumb(self):
++        """Return a breadcrumb for this `BugTask`.
++
++        If the user does not have permission to view the bug for
++        whatever reason, don't return a breadcrumb.
++        """
++        try:
++            return super(BugTaskBreadcrumbBuilder, self).make_breadcrumb()
++        except Unauthorized:
++            return None
 === modified file 'lib/lp/bugs/stories/bug-release-management/nomination-navigation.txt'
 --- lib/lp/bugs/stories/bug-release-management/nomination-navigation.txt	2009-07-01 13:16:44 +0000
 +++ lib/lp/bugs/stories/bug-release-management/nomination-navigation.txt	2009-08-14 14:44:07 +0000
@@ -11,7 +11,7 @@
      ... 'http://bugs.launchpad.dev/ubuntu/+source/mozilla-firefox/+bug/1'
      ... '/nominations/2/+editstatus')
      >>> print_location(admin_browser.contents)
--    Hierarchy: Launchpad > Ubuntu > ?mozilla-firefox? package
++    Hierarchy: Launchpad > Ubuntu > ?mozilla-firefox? package > Bug #1
      Tabs:
      * Overview - http://launchpad.dev/ubuntu/+source/mozilla-firefox
      * Code - http://code.launchpad.dev/ubuntu/+source/mozilla-firefox
 === modified file 'lib/lp/bugs/stories/bugs/xx-bug-index.txt'
 --- lib/lp/bugs/stories/bugs/xx-bug-index.txt	2009-07-31 08:12:47 +0000
 +++ lib/lp/bugs/stories/bugs/xx-bug-index.txt	2009-08-14 15:11:58 +0000
@@ -10,9 +10,25 @@
      >>> anon_browser.title
      '...Blackhole Trash folder...'
--The bug id in the upper left corner of the page is linked to the bug itself.
--
--    >>> anon_browser.getLink('Bug #2').click()
++The breadcrumbs and other navigation include a link to the bug itself.
++
++    >>> print_location(anon_browser.contents)
++    Hierarchy: Launchpad > Debian > ?mozilla-firefox? package > Bug #2 (blackhole)
++    Tabs:
++    * Overview - http://launchpad.dev/debian/+source/mozilla-firefox
++    * Code - http://code.launchpad.dev/debian/+source/mozilla-firefox
++    * Bugs (selected) - http://bugs.launchpad.dev/debian/+source/mozilla-firefox
++    * Blueprints - not linked
++    * Translations - not linked
++    * Answers - http://answers.launchpad.dev/debian/+source/mozilla-firefox
++    Main heading: Blackhole Trash folder
++
++The bug id in the upper left corner of the main page content is also
++linked to the bug itself.
++
++    >>> bug_link = first_tag_by_class(
++    ...     anon_browser.contents, 'object identifier').a
++    >>> anon_browser.open(bug_link.get('href'))
      >>> print anon_browser.title
      Bug #2 in Tomcat:...Blackhole Trash folder...
 === modified file 'lib/lp/bugs/tests/test_breadcrumbs.py'
 --- lib/lp/bugs/tests/test_breadcrumbs.py	2009-08-13 13:57:47 +0000
 +++ lib/lp/bugs/tests/test_breadcrumbs.py	2009-08-14 16:32:02 +0000
@@ -10,7 +10,8 @@
  from canonical.lazr.testing.menus import make_fake_request
  from canonical.launchpad.webapp.publisher import RootObject
  from canonical.testing import DatabaseFunctionalLayer
--from lp.testing import TestCaseWithFactory
++from lp.testing import (
++    ANONYMOUS, TestCaseWithFactory, login)
  class TestBugTaskBreadcrumbBuilder(TestCaseWithFactory):
@@ -18,7 +19,7 @@
      layer = DatabaseFunctionalLayer
      def setUp(self):
--        TestCaseWithFactory.setUp(self)
++        super(TestBugTaskBreadcrumbBuilder, self).setUp()
          self.product = self.factory.makeProduct(
              name='crumb-tester', displayname="Crumb Tester")
          self.bug = self.factory.makeBug(product=self.product)
@@ -66,6 +67,23 @@
              ["Crumb Tester", "Bug #%d" % self.bug.id],
              [crumb.text for crumb in hierarchy.items()])
++    def test_bugtask_private_bug(self):
++        # A breadcrumb is not generated for a bug that the user does
++        # not have permission to view.
++        login('foo.bar@canonical.com')
++        self.bug.setPrivate(True, self.bug.owner)
++        login(ANONYMOUS)
++        request = make_fake_request(
++            'http://bugs.launchpad.dev/%s/+bug/%d/+activity' % (
++                self.product.name, self.bug.id),
++            [self.root, self.product, self.bug.default_bugtask])
++        hierarchy = getMultiAdapter((self.root, request), name='+hierarchy')
++        self.assertEquals(
++            ['http://bugs.launchpad.dev/crumb-tester'],
++            [crumb.url for crumb in hierarchy.items()])
++        self.assertEquals(
++            ["Crumb Tester"], [crumb.text for crumb in hierarchy.items()])
++
  def test_suite():
      return unittest.TestLoader().loadTestsFromName(__name__)

 === modified file 'lib/lp/bugs/templates/bug-activity.pt'
 --- lib/lp/bugs/templates/bug-activity.pt	2009-07-17 17:59:07 +0000
 +++ lib/lp/bugs/templates/bug-activity.pt	2009-08-12 13:02:26 +0000
@@ -1,20 +1,19 @@
--<html
--  xmlns="http://www.w3.org/1999/xhtml"
--  xmlns:tal="http://xml.zope.org/namespaces/tal"
--  xmlns:metal="http://xml.zope.org/namespaces/metal"
--  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
--  xml:lang="en"
--  lang="en"
--  dir="ltr"
--  metal:use-macro="view/macro:page/onecolumn"
-->
--  <body>
--<div metal:fill-slot="main">
++<bug-activity
++    xmlns="http://www.w3.org/1999/xhtml"
++    xmlns:tal="http://xml.zope.org/namespaces/tal"
++    xmlns:metal="http://xml.zope.org/namespaces/metal"
++    xmlns:i18n="http://xml.zope.org/namespaces/i18n"
++    metal:use-macro="view/macro:page/main_only">
--  <div id="maincontent">
--    <p tal:content="structure view/current_bugtask/fmt:link" />
++  <div metal:fill-slot="heading">
      <h1>Activity log for bug #<span tal:replace="context/bug/id" /></h1>
++  </div>
++  <div metal:fill-slot="main">
++    <div class="top-portlet">
++      <p tal:content="structure context/fmt:link" />
++    </div>
++    <div class="yui-g">
      <table class="listing">
        <thead>
          <tr>
@@ -39,9 +38,6 @@
          </tr>
        </tbody>
      </table>
++    </div>
    </div>
--
--</div>
--
--</body>
--</html>
++</bug-activity>

Launchpad itself

Merge lp:~allenap/launchpad/ui-convert-bug-activity-bug-412523 into lp:launchpad

Commit message

Description of the change

Preview Diff

Subscribers

1	=== modified file 'lib/lp/bugs/browser/bugtask.py'
2	--- lib/lp/bugs/browser/bugtask.py 2009-08-07 23:20:33 +0000
3	+++ lib/lp/bugs/browser/bugtask.py 2009-08-13 10:14:48 +0000
4	@@ -14,6 +14,7 @@
5	'BugTargetView',
6	'BugTaskContextMenu',
7	'BugTaskCreateQuestionView',
8	+ 'BugTaskBreadcrumbBuilder',
9	'BugTaskEditView',
10	'BugTaskExpirableListingView',
11	'BugTaskListingItem',
12	@@ -116,6 +117,7 @@
13	from lp.registry.interfaces.sourcepackage import ISourcePackage
14	from canonical.launchpad.interfaces.validation import (
15	valid_upstreamtask, validate_distrotask)
16	+from canonical.launchpad.webapp.breadcrumb import BreadcrumbBuilder
17	from canonical.launchpad.webapp.interfaces import (
18	ILaunchBag, NotFoundError, UnexpectedFormData)
19
20	@@ -3476,3 +3478,12 @@
21	return_dict[key] = cgi.escape(return_dict[key])
22
23	return "%(old_value)s → %(new_value)s" % return_dict
24	+
25	+
26	+class BugTaskBreadcrumbBuilder(BreadcrumbBuilder):
27	+ """Builds a breadcrumb for an `IBugTask`."""
28	+ @property
29	+ def text(self):
30	+ return "%s: %s" % (
31	+ self.context.bug.displayname,
32	+ self.context.bug.title)
33
34	=== modified file 'lib/lp/bugs/configure.zcml'
35	--- lib/lp/bugs/configure.zcml 2009-07-23 13:44:13 +0000
36	+++ lib/lp/bugs/configure.zcml 2009-08-13 10:14:48 +0000
37	@@ -263,6 +263,12 @@
38	<adapter
39	factory="lp.bugs.browser.bugtask.assignee_renderer"
40	name="assignee"/>
41	+ <adapter
42	+ provides="canonical.launchpad.webapp.interfaces.IBreadcrumbBuilder"
43	+ for="lp.bugs.interfaces.bugtask.IBugTask"
44	+ factory="lp.bugs.browser.bugtask.BugTaskBreadcrumbBuilder"
45	+ permission="zope.Public"/>
46	+
47
48	<!-- NullBugTask -->
49
50
51	=== modified file 'lib/lp/bugs/templates/bug-activity.pt'
52	--- lib/lp/bugs/templates/bug-activity.pt 2009-08-12 13:02:26 +0000
53	+++ lib/lp/bugs/templates/bug-activity.pt 2009-08-13 10:14:13 +0000
54	@@ -10,10 +10,8 @@
55	</div>
56
57	<div metal:fill-slot="main">
58	+ <div class="yui-g">
59	<div class="top-portlet">
60	- <p tal:content="structure context/fmt:link" />
61	- </div>
62	- <div class="yui-g">
63	<table class="listing">
64	<thead>
65	<tr>
66	@@ -39,5 +37,6 @@
67	</tbody>
68	</table>
69	</div>
70	+ </div>
71	</div>
72	</bug-activity>