Code review comment for lp:~deryck/launchpad/edit-bug-ui-update-412971

Hi Deryck,

I noticed the link to the bug's page on the heading doesn't exist in the
new UI, but I don't think that's a problem as we now have the 'Cancel'
link at the bottom. It'd be nice if you could double check that with
Martin.

Other than that, it looks nice, and I only have one suggestion below.

 review approve

On Fri, 2009-08-14 at 13:25 +0000, Deryck Hodge wrote:
> = Summary =
>
> This branch is my first attempt at mechanical conversions for 3.0 UI
> updates for bugs templates. This works on bug-edit.pt, which will
> rarely be hit with all the recent AJAX work. The idea was to do an
> easy, low-visibility template to start, to understand some basics of
> the
> updating process.
>
> See bug 412971 for a good bit of discussion/mid-implementation notes
> as
> I worked out what to do.
>
> I think most of the comments there also get me pretty far along in UI
> review. The only question I have remaining is minor. In the original
> version the page heading was "Edit bug description" and I changed that
> to "Edit bug information" since the page encompasses more fields than
> description. Perhaps description is a better word or there is yet
> another better term.
>
> == Screenshots ==
>
> Before:
> http://launchpadlibrarian.net/30273670/edit-bug-page-before-changes.png
>
> After:
> http://launchpadlibrarian.net/30279936/edit-bug-using-generic-edit.png
>
> == Tests ==
>
> ./bin/test -vv -t xx-bug-edit.txt
>
> The test is currently broken, and I'm waiting on a fix to base layout
> by
> Edwin to land before fixing the test. There may be other tests
> broken,
> too, and I have the branch in ec2test for now to know for sure.
>
> I'll amend the MP with a diff of test updates when done.
>

> === modified file 'lib/lp/bugs/browser/bug.py'
> --- lib/lp/bugs/browser/bug.py 2009-08-04 05:30:51 +0000
> +++ lib/lp/bugs/browser/bug.py 2009-08-14 13:06:27 +0000
> @@ -412,7 +412,8 @@
> @cachedproperty
> def subscriber_ids(self):
> """Return a dictionary mapping a css_name to user name."""
> - subscribers = self.direct_subscribers.union(self.duplicate_subscribers)
> + subscribers = self.direct_subscribers.union(
> + self.duplicate_subscribers)
>
> # The current user has to be in subscribers_id so
> # in case the id is needed for a new subscription.
> @@ -555,6 +556,21 @@
> BugEditViewBase.__init__(self, context, request)
> self.notifications = []
>
> + @property
> + def label(self):
> + """The form label."""
> + return 'Edit Bug Information'

I guess we don't need this to be a property, do we?

> +
> + @property
> + def page_title(self):
> + """The page title."""
> + return 'Edit %s ' % self.context.title
> +
> + @property
> + def cancel_url(self):
> + """See `LaunchpadFormView`."""
> + return canonical_url(self.context)
> +
> def validate(self, data):
> """Make sure new tags are confirmed."""
> if 'tags' not in data:
>

>
--
Guilherme Salgado <email address hidden>